Re: [PATCH] spi: Update speed/mode on change
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
Cc: Jagan Teki
Cc: Patrick Delaunay
---
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
Re: [PATCH] spi: Update speed/mode on change
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
Cc: Jagan Teki
Cc: Patrick Delaunay
---
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
[PATCH] spi: Update speed/mode on change
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
Cc: Jagan Teki
Cc: Patrick Delaunay
---
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;
+ }
+
*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);
--
2.30.0
