Hi Sagar, On Wed, Jan 29, 2020 at 2:02 AM Sagar Shrikant Kadam <sagar.ka...@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.ka...@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...@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...@gmail.com> Tested-by: Jagan Teki <ja...@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