| Message ID | 1580234527-29280-3-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 |
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
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 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. Thanks & BR, Sagar > > + > > 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
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)) { + 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; +} + 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; + 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[] = {
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. Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> --- drivers/spi/spi-sifive.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)