| Message ID | 20210225205212.728105-1-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | spi: Update speed/mode on change | expand |
Hi Marek, On 2/25/21 9:52 PM, Marek Vasut wrote: > The spi_get_bus_and_cs() may be called on the same bus and chipselect > with different frequency or mode. This is valid usecase, but the code > fails to notify the controller of such a configuration change. Call > spi_set_speed_mode() in case bus frequency or bus mode changed to let > the controller update the configuration. > > The problem can easily be triggered using the sspi command: > => sspi 0:0@1000 > => sspi 0:0@2000 > Without this patch, both transfers happen at 1000 Hz. With this patch, > the later transfer happens correctly at 2000 Hz. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Patrick Delaunay <patrick.delaunay@st.com> > --- > drivers/spi/spi-uclass.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c > index 7155d4aebd6..96c9a83e761 100644 > --- a/drivers/spi/spi-uclass.c > +++ b/drivers/spi/spi-uclass.c > @@ -405,12 +405,22 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, > goto err; > } > > + /* In case bus frequency or mode changed, update it. */ > + if ((speed && slave->speed && slave->speed != speed) || > + (plat->mode != mode)) { > + ret = spi_set_speed_mode(bus, speed, mode); > + if (ret) > + goto err_speed_mode; > + } > + We compare bus (with lastest configured value ) or slave (value requested) here ? in dm_spi_claim_bus() it is compared with bus : spi->speed and spi->mode and spi = dev_get_uclass_priv(bus); + /* In case bus frequency or mode changed, update it. */ + if ((speed && bus->speed != speed) || (bus->mode != mode)) { + ret = spi_set_speed_mode(bus, speed, mode); + if (ret) + goto err_speed_mode; + bus->speed = speed; + bus->mode = mode; + } NB: bus->speed can't be 0 here after previous spi_claim_bus() PS: the update of spi->speed and spi->mode can be done in spi_set_speed_mode() function > *busp = bus; > *devp = slave; > log_debug("%s: bus=%p, slave=%p\n", __func__, bus, *devp); > > return 0; > > +err_speed_mode: > + spi_claim_bus(slave); here I think it is: spi_release_bus(slave); called after previous spi_claim_bus(). > err: > log_debug("%s: Error path, created=%d, device '%s'\n", __func__, > created, dev->name); Regards Patrick
On 2/26/21 2:07 PM, Patrick DELAUNAY wrote: > Hi Marek, > > On 2/25/21 9:52 PM, Marek Vasut wrote: >> The spi_get_bus_and_cs() may be called on the same bus and chipselect >> with different frequency or mode. This is valid usecase, but the code >> fails to notify the controller of such a configuration change. Call >> spi_set_speed_mode() in case bus frequency or bus mode changed to let >> the controller update the configuration. >> >> The problem can easily be triggered using the sspi command: >> => sspi 0:0@1000 >> => sspi 0:0@2000 >> Without this patch, both transfers happen at 1000 Hz. With this patch, >> the later transfer happens correctly at 2000 Hz. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Jagan Teki <jagan@amarulasolutions.com> >> Cc: Patrick Delaunay <patrick.delaunay@st.com> >> --- >> drivers/spi/spi-uclass.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c >> index 7155d4aebd6..96c9a83e761 100644 >> --- a/drivers/spi/spi-uclass.c >> +++ b/drivers/spi/spi-uclass.c >> @@ -405,12 +405,22 @@ int spi_get_bus_and_cs(int busnum, int cs, int >> speed, int mode, >> goto err; >> } >> + /* In case bus frequency or mode changed, update it. */ >> + if ((speed && slave->speed && slave->speed != speed) || >> + (plat->mode != mode)) { >> + ret = spi_set_speed_mode(bus, speed, mode); >> + if (ret) >> + goto err_speed_mode; >> + } >> + > > We compare bus (with lastest configured value ) or slave (value > requested) here ? Argh, this should in fact be bus_data->speed , it seems I sent un-rebased patches. > in dm_spi_claim_bus() it is compared with bus : > > spi->speed and spi->mode and spi = dev_get_uclass_priv(bus); > > + /* In case bus frequency or mode changed, update it. */ > + if ((speed && bus->speed != speed) || (bus->mode != mode)) { > + ret = spi_set_speed_mode(bus, speed, mode); > + if (ret) > + goto err_speed_mode; > + bus->speed = speed; > + bus->mode = mode; > + } > > NB: bus->speed can't be 0 here after previous spi_claim_bus() > > PS: the update of spi->speed and spi->mode can be done in > spi_set_speed_mode() function I don't think you want to do this every time you call dm_spi_claim_bus() >> *busp = bus; >> *devp = slave; >> log_debug("%s: bus=%p, slave=%p\n", __func__, bus, *devp); >> return 0; >> +err_speed_mode: >> + spi_claim_bus(slave); > > > here I think it is: > > spi_release_bus(slave); > > called after previous spi_claim_bus(). Right
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index 7155d4aebd6..96c9a83e761 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -405,12 +405,22 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, goto err; } + /* In case bus frequency or mode changed, update it. */ + if ((speed && slave->speed && slave->speed != speed) || + (plat->mode != mode)) { + ret = spi_set_speed_mode(bus, speed, mode); + if (ret) + goto err_speed_mode; + } + *busp = bus; *devp = slave; log_debug("%s: bus=%p, slave=%p\n", __func__, bus, *devp); return 0; +err_speed_mode: + spi_claim_bus(slave); err: log_debug("%s: Error path, created=%d, device '%s'\n", __func__, created, dev->name);
The spi_get_bus_and_cs() may be called on the same bus and chipselect with different frequency or mode. This is valid usecase, but the code fails to notify the controller of such a configuration change. Call spi_set_speed_mode() in case bus frequency or bus mode changed to let the controller update the configuration. The problem can easily be triggered using the sspi command: => sspi 0:0@1000 => sspi 0:0@2000 Without this patch, both transfers happen at 1000 Hz. With this patch, the later transfer happens correctly at 2000 Hz. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Jagan Teki <jagan@amarulasolutions.com> Cc: Patrick Delaunay <patrick.delaunay@st.com> --- drivers/spi/spi-uclass.c | 10 ++++++++++ 1 file changed, 10 insertions(+)