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> # SoPineAdding 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