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?

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