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
