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

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

Hi Sagar,

On Wed, Jan 29, 2020 at 2:02 AM Sagar Shrikant Kadam sagar.kadam@sifive.com wrote:

Add missing bus claim/release method to spi driver for HiFive Unleashed, and handle num_cs generously so that it generates an error if invalid cs number is passed to sf probe.

Is adding the claim/release method the fix to the weird "sf probe 0:2/4/6/8" issue?

Signed-off-by: Sagar Shrikant Kadam sagar.kadam@sifive.com

drivers/spi/spi-sifive.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)

diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c index 969bd4b..f990ad6 100644 --- a/drivers/spi/spi-sifive.c +++ b/drivers/spi/spi-sifive.c @@ -186,6 +186,36 @@ static void sifive_spi_tx(struct sifive_spi *spi, const u8 *tx_ptr) writel(tx_data, spi->regs + SIFIVE_SPI_REG_TXDATA); }

+static int sifive_spi_claim_bus(struct udevice *dev) +{

•   int ret;
  

•   struct udevice *bus = dev->parent;
  

•   struct sifive_spi *spi = dev_get_priv(bus);
  

•   struct dm_spi_slave_platdata *slave = dev_get_parent_platdata(dev);
  

•   if (!(slave->cs < spi->num_cs)) {
  

slave->cs >= spi->num_cs

But there is already check added recently in the spi-uclass driver, see below:

commit 7bacce524d48594dae399f9ee9280ab105f6c8cf Author: Bin Meng bmeng.cn@gmail.com Date: Mon Sep 9 06:00:02 2019 -0700

dm: spi: Check cs number before accessing slaves

Add chip select number check in spi_find_chip_select().

Signed-off-by: Bin Meng bmeng.cn@gmail.com Tested-by: Jagan Teki jagan@amarulasolutions.com # SoPine

Adding check in the sifive spi driver seems to unnecessary. Could you please confirm?

•           printf("Invalid cs number = %d\n", slave->cs);
  

•           return -EINVAL;
  

•   }
  

•   sifive_spi_prep_device(spi, slave);
  

•   ret = sifive_spi_set_cs(spi, slave);
  

•   if (ret)
  

•           return ret;
  

•   return 0;
  

+}

+static int sifive_spi_release_bus(struct udevice *dev) +{

•   struct sifive_spi *spi = dev_get_priv(dev->parent);
  

•   sifive_spi_clear_cs(spi);
  

•   return 0;
  

+}

What was done in the sifive_spi_claim_bus / sifive_spi_release_bus seems to be already done in sifive_spi_xfer(). See flags testing against SPI_XFER_BEGIN and SPI_XFER_END.

Could you please explain a little bit on why adding these 2 fixes things?

static int sifive_spi_xfer(struct udevice *dev, unsigned int bitlen, const void *dout, void *din, unsigned long flags) { @@ -345,6 +375,10 @@ static int sifive_spi_probe(struct udevice *bus) /* init the sifive spi hw */ sifive_spi_init_hw(spi);

•   /* Fetch number of chip selects from DT if present */
  

•   ret = dev_read_u32_default(bus, "num-cs", spi->num_cs);
  

•   spi->num_cs = ret;
  

spi->num_cs is already assigned a value in sifive_spi_init_hw(), Why do we override the value using DT's?

•   return 0;
  

}

@@ -353,6 +387,8 @@ static const struct dm_spi_ops sifive_spi_ops = { .set_speed = sifive_spi_set_speed, .set_mode = sifive_spi_set_mode, .cs_info = sifive_spi_cs_info,

•   .claim_bus      = sifive_spi_claim_bus,
  

•   .release_bus    = sifive_spi_release_bus,
  

};

static const struct udevice_id sifive_spi_ids[] = {

Regards, Bin

Hi Sagar,

On Fri, Feb 7, 2020 at 2:25 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 2/4] spi: fu540: add claim and release method to spi-sifive.c

Hi Sagar,

On Wed, Jan 29, 2020 at 2:02 AM Sagar Shrikant Kadam sagar.kadam@sifive.com wrote:

...

Add missing bus claim/release method to spi driver for HiFive Unleashed, and handle num_cs generously so that it generates an error if invalid cs number is passed to sf probe.

Is adding the claim/release method the fix to the weird "sf probe 0:2/4/6/8" issue?

Please see below.

...

...

Signed-off-by: Sagar Shrikant Kadam sagar.kadam@sifive.com

drivers/spi/spi-sifive.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)

diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c index 969bd4b..f990ad6 100644 --- a/drivers/spi/spi-sifive.c +++ b/drivers/spi/spi-sifive.c @@ -186,6 +186,36 @@ static void sifive_spi_tx(struct sifive_spi *spi, const

u8 *tx_ptr)

...

    writel(tx_data, spi->regs + SIFIVE_SPI_REG_TXDATA);  }

+static int sifive_spi_claim_bus(struct udevice *dev) {

•   int ret;
  

•   struct udevice *bus = dev->parent;
  

•   struct sifive_spi *spi = dev_get_priv(bus);
  

•   struct dm_spi_slave_platdata *slave =
  

+dev_get_parent_platdata(dev);

•   if (!(slave->cs < spi->num_cs)) {
  

slave->cs >= spi->num_cs

But there is already check added recently in the spi-uclass driver, see below:

commit 7bacce524d48594dae399f9ee9280ab105f6c8cf Author: Bin Meng bmeng.cn@gmail.com Date: Mon Sep 9 06:00:02 2019 -0700

dm: spi: Check cs number before accessing slaves

Add chip select number check in spi_find_chip_select().

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Jagan Teki <jagan@amarulasolutions.com> # SoPine

Adding check in the sifive spi driver seems to unnecessary. Could you please confirm?

Ahh!!. Thanks for the pointer Bin. This V2 patch here is based on commit c05b38df477a ("common: fix regression on block cache init"), so didn't come across the patch you pointed out. So for now we can skip the check in sifive spi driver.

...

...

•           printf("Invalid cs number = %d\n", slave->cs);
  

•           return -EINVAL;
  

•   }
  

•   sifive_spi_prep_device(spi, slave);
  

•   ret = sifive_spi_set_cs(spi, slave);
  

•   if (ret)
  

•           return ret;
  

•   return 0;
  

+}

+static int sifive_spi_release_bus(struct udevice *dev) {

•   struct sifive_spi *spi = dev_get_priv(dev->parent);
  

•   sifive_spi_clear_cs(spi);
  

•   return 0;
  

+}

What was done in the sifive_spi_claim_bus / sifive_spi_release_bus seems to be already done in sifive_spi_xfer(). See flags testing against SPI_XFER_BEGIN and SPI_XFER_END.

Could you please explain a little bit on why adding these 2 fixes things?

What I saw was that sf probe was detecting flash even at wrong chip select inputs, Like sf probe 0:2/4/6/.. this indicated that a check to validate chip selects needs to be there. The gist of adding the claim function was to introduce this chip select check. The code within SPI_XFER_BEGIN and END functions missed this check. Now that the commit your pointed 7bacce524d48 handles this, I think we can drop this check as of now.

...

...

static int sifive_spi_xfer(struct udevice *dev, unsigned int bitlen, const void *dout, void *din, unsigned long flags) { @@ -345,6 +375,10 @@ static int sifive_spi_probe(struct udevice *bus) /* init the sifive spi hw */ sifive_spi_init_hw(spi);

•   /* Fetch number of chip selects from DT if present */
  

•   ret = dev_read_u32_default(bus, "num-cs", spi->num_cs);
  

•   spi->num_cs = ret;
  

spi->num_cs is already assigned a value in sifive_spi_init_hw(), Why do we override the value using DT's?

Yes, spi_init_hw does initialise the spi->num_cs by reading the register. For QSPI0 and QSPI2 it is set to 1 (as per the cs_width). But for QSPI1 it is set to 4. Consider a case where user wishes to populate only 1 chip select for QSPI1 then it can be done in respective <board>-dts file and can be updated after sifive_spi_init_hw is done. If the board device tree doesn't contain any such entry than above API will retain the value of spi->num_cs populated in sifive_spi_init_hw().

So why is such override necessary? If we don't do this, would it cause any problem?

IOW, is this patch actually fixing anything that was seen on the Unleashed board?

So to summarize, we can drop claim/release addition from this patch and just keep the dev_read_u32_default function done above. Please let me know your opinion here.

Regards, Bin

Hi Sagar,

On Wed, Jan 29, 2020 at 2:02 AM Sagar Shrikant Kadam sagar.kadam@sifive.com wrote:

The hifive-unleashed-a00.dts has flash spi-tx/rx width set to 4-bit mode. During sf probe, spi_nor_scan fails to read the JEDEC ID with reg_proto set to SNOR_PROTO_1_1_1. Setting it to 1-bit mode as of now will help read the JEDEC-ID and perform other flash operations.

So previously with Jagan's series that did not have these changes in this commit, the flash driver worked well. I wonder what real issue was fixed in this commit?

Signed-off-by: Sagar Shrikant Kadam sagar.kadam@sifive.com

arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi b/arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi index d7a6413..dae9f87 100644 --- a/arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi +++ b/arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi @@ -9,3 +9,11 @@ spi2 = &qspi2; }; };

+&qspi0 {

•   flash@0 {
  

•           spi-tx-bus-width = <1>;
  

•           spi-rx-bus-width = <1>;
  

•   };
  

+};

Regards, Bin

Hi 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 4/4] bdinfo: fu540: print fdt base address for debugging

Hi Sagar,

On Wed, Jan 29, 2020 at 2:02 AM Sagar Shrikant Kadam sagar.kadam@sifive.com wrote:

...

Add fdt->gd info to bdinfo so that it is useful for debugging and easily use it with fdt util.

The commit title should be tagged with "riscv: bdinfo" as it is not fu540 specific.

Thanks for the Reviewed-by tag. I will include the change in next series

BR, Sagar

...

Signed-off-by: Sagar Shrikant Kadam sagar.kadam@sifive.com

cmd/bdinfo.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/cmd/bdinfo.c b/cmd/bdinfo.c index d6a7175..96892b3 100644 --- a/cmd/bdinfo.c +++ b/cmd/bdinfo.c @@ -433,6 +433,7 @@ int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc,

char * const argv[])

...

    print_bi_dram(bd);
    print_num("relocaddr", gd->relocaddr);
    print_num("reloc off", gd->reloc_off);

•   print_num("fdt_blob", (ulong)gd->fdt_blob);
    print_eth_ip_addr();
    print_baudrate();
  
  

--

Other than above, Reviewed-by: Bin Meng bmeng.cn@gmail.com

Regards, Bin