[PATCH v2 00/10] riscv: Add SPI support for Kendryte K210

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 00/10] riscv: Add SPI support for Kendryte K210

Sean Anderson
On 8/10/20 6:49 AM, Eugeniy Paltsev wrote:
> Hi Sean,
>
> do you have any public git branch with this patch series?
> I want to test these changes on our board with DW SPI controller.

https://github.com/Forty-Bot/u-boot/tree/maix_spi

--Sean
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 00/10] riscv: Add SPI support for Kendryte K210

Eugeniy Paltsev
FYI: I've tested on commit aa68b00a8259aa026591475f21a5c51311252ef2 (current branch head) and I don't see any build/runtime issues.

Tested-by Eugeniy Paltsev <[hidden email]>

---
 Eugeniy Paltsev


________________________________________
From: Sean Anderson <[hidden email]>
Sent: Monday, August 10, 2020 14:13
To: Eugeniy Paltsev; [hidden email]; [hidden email]
Cc: Heinrich Schuchardt; Jagan Teki; Horatiu Vultur; Marek Vasut; Alexey Brodkin; Daniel Schwierzeck; Gregory CLEMENT; Lars Povlsen; Ley Foon Tan; Rick Chen; Simon Goldschmidt
Subject: Re: [PATCH v2 00/10] riscv: Add SPI support for Kendryte K210

On 8/10/20 6:49 AM, Eugeniy Paltsev wrote:
> Hi Sean,
>
> do you have any public git branch with this patch series?
> I want to test these changes on our board with DW SPI controller.

https://urldefense.com/v3/__https://github.com/Forty-Bot/u-boot/tree/maix_spi__;!!A4F2R9G_pg!JnihRPoojBUo2j8mMgJNH_K5dis6o-g7c2YGmb0SYh2wBkWGASlPXxMFW-TPbdSOtlYUiTs$

--Sean
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 09/10] riscv: Add device tree bindings for SPI

Rick Chen
In reply to this post by Sean Anderson
> This patch adds bindings for the MMC slot and SPI flash on the Sipeed Maix
> Bit.
>
> Signed-off-by: Sean Anderson <[hidden email]>
> ---
> This patch was previously part of
> https://patchwork.ozlabs.org/project/uboot/list/?series=161576
>
> Changes in v2:
> - Remove broken-wp property (implicit due to no wp gpio)
> - Remove ctrl0 field offsets from device tree
> - Switch to new compatible strings
> - Switch to new pinmux binding style
>
>  arch/riscv/dts/k210-maix-bit.dts | 46 +++++++++++++++++++++++++++++++-
>  arch/riscv/dts/k210.dtsi         |  2 ++
>  2 files changed, 47 insertions(+), 1 deletion(-)

Acked-by: Rick Chen <[hidden email]>

I am OK that this patch can be pulled via SPI tree.

Thanks,
Rick

>
> diff --git a/arch/riscv/dts/k210-maix-bit.dts b/arch/riscv/dts/k210-maix-bit.dts
> index e840e04ada..73892c6450 100644
> --- a/arch/riscv/dts/k210-maix-bit.dts
> +++ b/arch/riscv/dts/k210-maix-bit.dts
> @@ -141,7 +141,7 @@
>                 pinmux = <K210_FPIOA(26, K210_PCF_SPI1_D1)>,
>                          <K210_FPIOA(27, K210_PCF_SPI1_SCLK)>,
>                          <K210_FPIOA(28, K210_PCF_SPI1_D0)>,
> -                        <K210_FPIOA(29, K210_PCF_GPIOHS13)>;
> +                        <K210_FPIOA(29, K210_PCF_GPIOHS13)>; /* cs */
>         };
>  };
>
> @@ -149,3 +149,47 @@
>         pinctrl-0 = <&fpioa_dvp>;
>         pinctrl-names = "default";
>  };
> +
> +&spi0 {
> +       pinctrl-0 = <&fpioa_spi0>;
> +       pinctrl-names = "default";
> +       num-cs = <1>;
> +       cs-gpios = <&gpio0 20 0>;
> +
> +       panel@0 {
> +               compatible = "sitronix,st7789v";
> +               reg = <0>;
> +               reset-gpios = <&gpio0 21 GPIO_ACTIVE_LOW>;
> +               dc-gpios = <&gpio0 22 0>;
> +               spi-max-frequency = <15000000>;
> +               status = "disabled";
> +       };
> +};
> +
> +&spi1 {
> +       pinctrl-0 = <&fpioa_spi1>;
> +       pinctrl-names = "default";
> +       num-cs = <1>;
> +       cs-gpios = <&gpio0 13 0>;
> +       status = "okay";
> +
> +       slot@0 {
> +               compatible = "mmc-spi-slot";
> +               reg = <0>;
> +               spi-max-frequency = <25000000>;
> +               voltage-ranges = <3300 3300>;
> +               broken-cd;
> +       };
> +};
> +
> +&spi3 {
> +       status = "okay";
> +
> +       spi-flash@0 {
> +               compatible = "jedec,spi-nor";
> +               reg = <0>;
> +               spi-max-frequency = <50000000>;
> +               m25p,fast-read;
> +               broken-flash-reset;
> +       };
> +};
> diff --git a/arch/riscv/dts/k210.dtsi b/arch/riscv/dts/k210.dtsi
> index 429891d651..fe47ee7aaf 100644
> --- a/arch/riscv/dts/k210.dtsi
> +++ b/arch/riscv/dts/k210.dtsi
> @@ -495,6 +495,8 @@
>                                 interrupts = <24>;
>                                 clocks = <&sysclk K210_CLK_DVP>;
>                                 resets = <&sysrst K210_RST_DVP>;
> +                               kendryte,sysctl = <&sysctl>;
> +                               kendryte,misc-offset = <K210_SYSCTL_MISC>;
>                                 status = "disabled";
>                         };
>
> --
> 2.28.0
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

Simon Glass-3
In reply to this post by Sean Anderson
On Fri, 7 Aug 2020 at 08:43, Sean Anderson <[hidden email]> wrote:

>
> This allows different log levels to be enabled or disabled depending on the
> desired level of verbosity. In particular, it allows for general debug
> information to be printed while excluding more verbose logging which may
> interfere with timing.
>
> Signed-off-by: Sean Anderson <[hidden email]>
> ---
>
> Changes in v2:
> - New
>
>  drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
>

Reviewed-by: Simon Glass <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

Marek Vasut-3
On 8/20/20 3:26 PM, Simon Glass wrote:

> On Fri, 7 Aug 2020 at 08:43, Sean Anderson <[hidden email]> wrote:
>>
>> This allows different log levels to be enabled or disabled depending on the
>> desired level of verbosity. In particular, it allows for general debug
>> information to be printed while excluding more verbose logging which may
>> interfere with timing.
>>
>> Signed-off-by: Sean Anderson <[hidden email]>
>> ---
>>
>> Changes in v2:
>> - New
>>
>>  drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
>>  1 file changed, 16 insertions(+), 17 deletions(-)
>>
>
> Reviewed-by: Simon Glass <[hidden email]>

NAK, please use dev_dbg() instead.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

Tom Rini-4
On Thu, Aug 20, 2020 at 03:36:31PM +0200, Marek Vasut wrote:

> On 8/20/20 3:26 PM, Simon Glass wrote:
> > On Fri, 7 Aug 2020 at 08:43, Sean Anderson <[hidden email]> wrote:
> >>
> >> This allows different log levels to be enabled or disabled depending on the
> >> desired level of verbosity. In particular, it allows for general debug
> >> information to be printed while excluding more verbose logging which may
> >> interfere with timing.
> >>
> >> Signed-off-by: Sean Anderson <[hidden email]>
> >> ---
> >>
> >> Changes in v2:
> >> - New
> >>
> >>  drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
> >>  1 file changed, 16 insertions(+), 17 deletions(-)
> >>
> >
> > Reviewed-by: Simon Glass <[hidden email]>
>
> NAK, please use dev_dbg() instead.
For the record, Marek and I (and Sean) had talked a bit on IRC.  These
messages being log_xxx() and not dev_xx() instead make the more
functionally useful as dev_xxx() converts to printf() and messes with
the timing.  log_xxx() puts them in the buffer and will not break the
timing.  Marek objects to the fundamentals that dev_xxx() does not
function like it does in Linux (and please correct me if you feel I'm
mis-representing your views).  This could be solved by plumbing
dev_xxx() to use log_xxx() if LOG is enabled automagically.  I don't see
transitions like that as blocking for this patch, Marek does.

--
Tom

signature.asc (673 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 01/10] spi: dw: Convert calls to debug to log_*

Marek Vasut-3
On 8/21/20 2:08 AM, Tom Rini wrote:

> On Thu, Aug 20, 2020 at 03:36:31PM +0200, Marek Vasut wrote:
>> On 8/20/20 3:26 PM, Simon Glass wrote:
>>> On Fri, 7 Aug 2020 at 08:43, Sean Anderson <[hidden email]> wrote:
>>>>
>>>> This allows different log levels to be enabled or disabled depending on the
>>>> desired level of verbosity. In particular, it allows for general debug
>>>> information to be printed while excluding more verbose logging which may
>>>> interfere with timing.
>>>>
>>>> Signed-off-by: Sean Anderson <[hidden email]>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - New
>>>>
>>>>  drivers/spi/designware_spi.c | 33 ++++++++++++++++-----------------
>>>>  1 file changed, 16 insertions(+), 17 deletions(-)
>>>>
>>>
>>> Reviewed-by: Simon Glass <[hidden email]>
>>
>> NAK, please use dev_dbg() instead.
>
> For the record, Marek and I (and Sean) had talked a bit on IRC.  These
> messages being log_xxx() and not dev_xx() instead make the more
> functionally useful as dev_xxx() converts to printf() and messes with
> the timing.

The ones which were printf() before are not impacted, since nothing
changes there. The ones which were debug() were compiled out anyway or
turned into printf(), so nothing changes there either.

In this case , the timing argument simply does not apply.

> log_xxx() puts them in the buffer and will not break the
> timing.  Marek objects to the fundamentals that dev_xxx() does not
> function like it does in Linux (and please correct me if you feel I'm
> mis-representing your views).

I do NOT object to the way dev_err() works, see below.

> This could be solved by plumbing
> dev_xxx() to use log_xxx() if LOG is enabled automagically.  I don't see
> transitions like that as blocking for this patch, Marek does.

My objection is to yet another new logging facility, which is
incompatible with any of the Linux ones. Both pr_() and dev_() we
already have in U-Boot, and there is already ongoing conversion attempt
to move drivers to dev_(), but now another log_() one appears out of
nowhere and starts adding to the already bad mess. Moreover, there is
zero information or guideline on which logging primitive to use where.

It is likely others are used to those Linux logging facilities, because
embedded systems developers likely develop both U-Boot and Linux, likely
at the same time. So making the U-Boot more and more incompatible does
not help at all.

What I do propose is to put the new logging facility underneath the
existing logging primitives -- pr_() and dev_() -- if applicable and
make the dynamic logging configurable. That way the current mess will be
cleaned up. The use of log_() should then only be limited to where no
other logging primitives apply.
12