diff mbox series

[U-Boot,v2,1/4] fu540: dtsi: spi: add num-cs info to device tree

Message ID 1580234527-29280-2-git-send-email-sagar.kadam@sifive.com
State Rejected
Delegated to: Andes
Headers show
Series Fix currently available support for flash on HiFive Unleashed | expand

Commit Message

Sagar Shrikant Kadam Jan. 28, 2020, 6:02 p.m. UTC
Add the number of chip select information to spi nodes which
can be used by spi-uclass for error handling if invalid cs
number passed from command.

Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
---
 arch/riscv/dts/fu540-c000.dtsi | 3 +++
 1 file changed, 3 insertions(+)

Comments

Bin Meng Feb. 4, 2020, 12:13 p.m. UTC | #1
Hi Sagar,

On Wed, Jan 29, 2020 at 2:02 AM Sagar Shrikant Kadam
<sagar.kadam@sifive.com> wrote:
>
> Add the number of chip select information to spi nodes which
> can be used by spi-uclass for error handling if invalid cs
> number passed from command.
>
> Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
> ---
>  arch/riscv/dts/fu540-c000.dtsi | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/riscv/dts/fu540-c000.dtsi b/arch/riscv/dts/fu540-c000.dtsi
> index afa43c7..9c6ab21 100644
> --- a/arch/riscv/dts/fu540-c000.dtsi
> +++ b/arch/riscv/dts/fu540-c000.dtsi
> @@ -191,6 +191,7 @@
>                         clocks = <&prci PRCI_CLK_TLCLK>;
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> +                       num-cs = <1>;

Why is this necessary? I can't find the codes that handle the num-cs
property. In the SiFive SPI driver, num_cs is determined from register
SIFIVE_SPI_REG_CSDEF.

>                         status = "disabled";
>                 };
>                 qspi1: spi@10041000 {
> @@ -202,6 +203,7 @@
>                         clocks = <&prci PRCI_CLK_TLCLK>;
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> +                       num-cs = <1>;
>                         status = "disabled";
>                 };
>                 qspi2: spi@10050000 {
> @@ -212,6 +214,7 @@
>                         clocks = <&prci PRCI_CLK_TLCLK>;
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> +                       num-cs = <1>;
>                         status = "disabled";
>                 };
>                 eth0: ethernet@10090000 {
> --

Regards,
Bin
Sagar Shrikant Kadam Feb. 5, 2020, 6:17 p.m. UTC | #2
Hello Bin,

> -----Original Message-----
> From: Bin Meng <bmeng.cn@gmail.com>
> Sent: Tuesday, February 4, 2020 5:43 PM
> To: Sagar Kadam <sagar.kadam@sifive.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Rick Chen
> <rick@andestech.com>; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>;
> Jagan Teki <jagan@amarulasolutions.com>; Anup Patel
> <anup.patel@wdc.com>
> Subject: Re: [U-Boot Patch v2 1/4] fu540: dtsi: spi: add num-cs info to device
> tree
> 
> Hi Sagar,
> 
> On Wed, Jan 29, 2020 at 2:02 AM Sagar Shrikant Kadam
> <sagar.kadam@sifive.com> wrote:
> >
> > Add the number of chip select information to spi nodes which can be
> > used by spi-uclass for error handling if invalid cs number passed from
> > command.
> >
> > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
> > ---
> >  arch/riscv/dts/fu540-c000.dtsi | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/riscv/dts/fu540-c000.dtsi
> > b/arch/riscv/dts/fu540-c000.dtsi index afa43c7..9c6ab21 100644
> > --- a/arch/riscv/dts/fu540-c000.dtsi
> > +++ b/arch/riscv/dts/fu540-c000.dtsi
> > @@ -191,6 +191,7 @@
> >                         clocks = <&prci PRCI_CLK_TLCLK>;
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> > +                       num-cs = <1>;
> 
> Why is this necessary? I can't find the codes that handle the num-cs
> property.
The code handling num-cs is a part of patch 2. I intended to keep it separate
thinking better to isolate dt and c files patches. Please let me know if I need
to keep it together.

> In the SiFive SPI driver, num_cs is determined from register
> SIFIVE_SPI_REG_CSDEF.
> 
The SiFive SPI driver does read the num_cs defined in the register you mentioned.
but it's hard defined with cs_width, eg: QSPI0 = 1, QSPI1 = 4, QSPI2 = 1.
I have added the option after sifive_spi_init_hw to read from  device tree as well
so that if the user want's to change the chip select's (less than that defined in
SIFIVE_SPI_REG_CSDEF) then it can be done in hifive-unleashed-a00.dtsi
Please let me know your thoughts over here.

BR,
Sagar

> >                         status = "disabled";
> >                 };
> >                 qspi1: spi@10041000 {
> > @@ -202,6 +203,7 @@
> >                         clocks = <&prci PRCI_CLK_TLCLK>;
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> > +                       num-cs = <1>;
> >                         status = "disabled";
> >                 };
> >                 qspi2: spi@10050000 {
> > @@ -212,6 +214,7 @@
> >                         clocks = <&prci PRCI_CLK_TLCLK>;
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> > +                       num-cs = <1>;
> >                         status = "disabled";
> >                 };
> >                 eth0: ethernet@10090000 {
> > --
> 
> Regards,
> Bin
Bin Meng Feb. 19, 2020, 4:21 p.m. UTC | #3
Hi Sagar,

On Thu, Feb 6, 2020 at 2:17 AM Sagar Kadam <sagar.kadam@sifive.com> wrote:
>
> Hello Bin,
>
> > -----Original Message-----
> > From: Bin Meng <bmeng.cn@gmail.com>
> > Sent: Tuesday, February 4, 2020 5:43 PM
> > To: Sagar Kadam <sagar.kadam@sifive.com>
> > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Rick Chen
> > <rick@andestech.com>; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>;
> > Jagan Teki <jagan@amarulasolutions.com>; Anup Patel
> > <anup.patel@wdc.com>
> > Subject: Re: [U-Boot Patch v2 1/4] fu540: dtsi: spi: add num-cs info to device
> > tree
> >
> > Hi Sagar,
> >
> > On Wed, Jan 29, 2020 at 2:02 AM Sagar Shrikant Kadam
> > <sagar.kadam@sifive.com> wrote:
> > >
> > > Add the number of chip select information to spi nodes which can be
> > > used by spi-uclass for error handling if invalid cs number passed from
> > > command.
> > >
> > > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
> > > ---
> > >  arch/riscv/dts/fu540-c000.dtsi | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/riscv/dts/fu540-c000.dtsi
> > > b/arch/riscv/dts/fu540-c000.dtsi index afa43c7..9c6ab21 100644
> > > --- a/arch/riscv/dts/fu540-c000.dtsi
> > > +++ b/arch/riscv/dts/fu540-c000.dtsi
> > > @@ -191,6 +191,7 @@
> > >                         clocks = <&prci PRCI_CLK_TLCLK>;
> > >                         #address-cells = <1>;
> > >                         #size-cells = <0>;
> > > +                       num-cs = <1>;
> >
> > Why is this necessary? I can't find the codes that handle the num-cs
> > property.
> The code handling num-cs is a part of patch 2. I intended to keep it separate
> thinking better to isolate dt and c files patches. Please let me know if I need
> to keep it together.
>
> > In the SiFive SPI driver, num_cs is determined from register
> > SIFIVE_SPI_REG_CSDEF.
> >
> The SiFive SPI driver does read the num_cs defined in the register you mentioned.
> but it's hard defined with cs_width, eg: QSPI0 = 1, QSPI1 = 4, QSPI2 = 1.
> I have added the option after sifive_spi_init_hw to read from  device tree as well
> so that if the user want's to change the chip select's (less than that defined in
> SIFIVE_SPI_REG_CSDEF) then it can be done in hifive-unleashed-a00.dtsi
> Please let me know your thoughts over here.
>

My understanding is that "nun-cs" represents the actual number of chip
selects a controller supports. In your patch you changed all 3
controllers num-cs to 1, but they should be 1/4/1 respectively.

I see changes are made in fu540-c000.dtsi. Was such change discussed
in the kernel DT community?

Is this change actually fixing any issue with what we saw on the
Unleashed board?

Regards,
Bin
diff mbox series

Patch

diff --git a/arch/riscv/dts/fu540-c000.dtsi b/arch/riscv/dts/fu540-c000.dtsi
index afa43c7..9c6ab21 100644
--- a/arch/riscv/dts/fu540-c000.dtsi
+++ b/arch/riscv/dts/fu540-c000.dtsi
@@ -191,6 +191,7 @@ 
 			clocks = <&prci PRCI_CLK_TLCLK>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			num-cs = <1>;
 			status = "disabled";
 		};
 		qspi1: spi@10041000 {
@@ -202,6 +203,7 @@ 
 			clocks = <&prci PRCI_CLK_TLCLK>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			num-cs = <1>;
 			status = "disabled";
 		};
 		qspi2: spi@10050000 {
@@ -212,6 +214,7 @@ 
 			clocks = <&prci PRCI_CLK_TLCLK>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			num-cs = <1>;
 			status = "disabled";
 		};
 		eth0: ethernet@10090000 {