Re: [PATCH v2] spi: Add spi_get_bus_and_cs() new use_dt param
Hi
Don't take care of this patch, a v3 will be send
Patrice
On 2/21/22 08:33, Patrice Chotard wrote:
> Add spi_get_bus_and_cs() new "use_dt" param which allows to
> select SPI speed and mode from DT or from default value passed
> in parameters.
>
> Introduce spi_flash_probe_bus_cs_default() which is identical
> to spi_flash_probe_bus_cs() except it calls spi_get_bus_and_cs()
> with use_dt param set to false.
>
> Since commit e2e95e5e2542 ("spi: Update speed/mode on change")
> when calling "sf probe" or "env save" on SPI flash,
> spi_set_speed_mode() is called twice.
>
> spi_get_bus_and_cs()
> |--> spi_claim_bus()
> | |--> spi_set_speed_mode(speed and mode from DT)
> ...
> |--> spi_set_speed_mode(default speed and mode value)
>
> The first spi_set_speed_mode() call is done with speed and mode
> values from DT, whereas the second call is done with speed
> and mode set to default value (speed is set to CONFIG_SF_DEFAULT_SPEED)
>
> This is an issue because SPI flash performance are impacted by
> using default speed which can be lower than the one defined in DT.
>
> One solution is to set CONFIG_SF_DEFAULT_SPEED to the speed defined
> in DT, but we loose flexibility offered by DT.
>
> Another issue can be encountered with 2 SPI flashes using 2 different
> speeds. In this specific case usage of CONFIG_SF_DEFAULT_SPEED is not
> flexible compared to get the 2 different speeds from DT.
>
> By adding new parameter "use_dt" to spi_get_bus_and_cs(), this allows
> to force usage of either speed and mode from DT (use_dt = true) or to
> use speed and mode parameter value.
>
> Signed-off-by: Patrice Chotard
> Cc: Marek Behun
> Cc: Jagan Teki
> Cc: Vignesh R
> Cc: Joe Hershberger
> Cc: Ramon Fried
> Cc: Lukasz Majewski
> Cc: Marek Vasut
> Cc: Wolfgang Denk
> Cc: Simon Glass
> Cc: Stefan Roese
> Cc: "Pali Rohár"
> Cc: Konstantin Porotchkin
> Cc: Igal Liberman
> Cc: Bin Meng
> Cc: Pratyush Yadav
> Cc: Sean Anderson
> Cc: Anji J
> Cc: Biwen Li
> Cc: Priyanka Jain
> Cc: Chaitanya Sakinam
>
> ---
>
> Changes in v2:
> - add spi_flash_probe_bus_cs_default() which calls spi_get_bus_and_cs()
> with "use_dt" param set to true, whereas spi_flash_probe_bus_cs() calls
> spi_get_bus_and_cs() with "use_dt" param set to true.
>
> board/CZ.NIC/turris_mox/turris_mox.c | 2 +-
> cmd/sf.c | 9 -
> cmd/spi.c| 2 +-
> drivers/mtd/spi/sf-uclass.c | 30 ++--
> drivers/spi/spi-uclass.c | 8
> drivers/usb/gadget/max3420_udc.c | 2 +-
> include/spi.h| 7 ---
> include/spi_flash.h | 4
> test/dm/spi.c| 15 +++---
> 9 files changed, 59 insertions(+), 20 deletions(-)
>
> diff --git a/board/CZ.NIC/turris_mox/turris_mox.c
> b/board/CZ.NIC/turris_mox/turris_mox.c
> index f0c5aa6a52..4b755e1420 100644
> --- a/board/CZ.NIC/turris_mox/turris_mox.c
> +++ b/board/CZ.NIC/turris_mox/turris_mox.c
> @@ -148,7 +148,7 @@ static int mox_do_spi(u8 *in, u8 *out, size_t size)
> struct udevice *dev;
> int ret;
>
> - ret = spi_get_bus_and_cs(0, 1, 100, SPI_CPHA | SPI_CPOL,
> + ret = spi_get_bus_and_cs(0, 1, 100, SPI_CPHA | SPI_CPOL, false,
>"spi_generic_drv", "moxtet@1", ,
>);
> if (ret)
> diff --git a/cmd/sf.c b/cmd/sf.c
> index 8bdebd9fd8..40b2cc3297 100644
> --- a/cmd/sf.c
> +++ b/cmd/sf.c
> @@ -91,6 +91,7 @@ static int do_spi_flash_probe(int argc, char *const argv[])
> unsigned int speed = CONFIG_SF_DEFAULT_SPEED;
> unsigned int mode = CONFIG_SF_DEFAULT_MODE;
> char *endp;
> + bool use_dt = true;
> #if CONFIG_IS_ENABLED(DM_SPI_FLASH)
> struct udevice *new, *bus_dev;
> int ret;
> @@ -117,11 +118,13 @@ static int do_spi_flash_probe(int argc, char *const
> argv[])
> speed = simple_strtoul(argv[2], , 0);
> if (*argv[2] == 0 || *endp != 0)
> return -1;
> + use_dt = false;
> }
> if (argc >= 4) {
> mode = hextoul(argv[3], );
> if (*argv[3] == 0 || *endp != 0)
> return -1;
> + use_dt = false;
> }
>
> #if CONFIG_IS_ENABLED(DM_SPI_FLASH)
> @@ -131,7 +134,11 @@ static int do_spi_flash_probe(int argc, char *const
> argv[])
> device_remove(new, DM_REMOVE_NORMAL);
> }
> flash = NULL;
> - ret = spi_flash_probe_bus_cs(bus, cs, speed, mode, );
> + if (use_dt)
> + ret = spi_flash_probe_bus_cs(bus, cs, speed, mode, );
> + else
> + ret = spi_flash_probe_bus_cs_default(bus, cs, speed, mode,
> );
> +
> if (ret) {
> printf("Failed to initialize SPI flash at %u:%u (error %d)\n",
> bus, cs, ret);
> diff --git a/cmd/spi.c
[PATCH v2] spi: Add spi_get_bus_and_cs() new use_dt param
Add spi_get_bus_and_cs() new "use_dt" param which allows to
select SPI speed and mode from DT or from default value passed
in parameters.
Introduce spi_flash_probe_bus_cs_default() which is identical
to spi_flash_probe_bus_cs() except it calls spi_get_bus_and_cs()
with use_dt param set to false.
Since commit e2e95e5e2542 ("spi: Update speed/mode on change")
when calling "sf probe" or "env save" on SPI flash,
spi_set_speed_mode() is called twice.
spi_get_bus_and_cs()
|--> spi_claim_bus()
| |--> spi_set_speed_mode(speed and mode from DT)
...
|--> spi_set_speed_mode(default speed and mode value)
The first spi_set_speed_mode() call is done with speed and mode
values from DT, whereas the second call is done with speed
and mode set to default value (speed is set to CONFIG_SF_DEFAULT_SPEED)
This is an issue because SPI flash performance are impacted by
using default speed which can be lower than the one defined in DT.
One solution is to set CONFIG_SF_DEFAULT_SPEED to the speed defined
in DT, but we loose flexibility offered by DT.
Another issue can be encountered with 2 SPI flashes using 2 different
speeds. In this specific case usage of CONFIG_SF_DEFAULT_SPEED is not
flexible compared to get the 2 different speeds from DT.
By adding new parameter "use_dt" to spi_get_bus_and_cs(), this allows
to force usage of either speed and mode from DT (use_dt = true) or to
use speed and mode parameter value.
Signed-off-by: Patrice Chotard
Cc: Marek Behun
Cc: Jagan Teki
Cc: Vignesh R
Cc: Joe Hershberger
Cc: Ramon Fried
Cc: Lukasz Majewski
Cc: Marek Vasut
Cc: Wolfgang Denk
Cc: Simon Glass
Cc: Stefan Roese
Cc: "Pali Rohár"
Cc: Konstantin Porotchkin
Cc: Igal Liberman
Cc: Bin Meng
Cc: Pratyush Yadav
Cc: Sean Anderson
Cc: Anji J
Cc: Biwen Li
Cc: Priyanka Jain
Cc: Chaitanya Sakinam
---
Changes in v2:
- add spi_flash_probe_bus_cs_default() which calls spi_get_bus_and_cs()
with "use_dt" param set to true, whereas spi_flash_probe_bus_cs() calls
spi_get_bus_and_cs() with "use_dt" param set to true.
board/CZ.NIC/turris_mox/turris_mox.c | 2 +-
cmd/sf.c | 9 -
cmd/spi.c| 2 +-
drivers/mtd/spi/sf-uclass.c | 30 ++--
drivers/spi/spi-uclass.c | 8
drivers/usb/gadget/max3420_udc.c | 2 +-
include/spi.h| 7 ---
include/spi_flash.h | 4
test/dm/spi.c| 15 +++---
9 files changed, 59 insertions(+), 20 deletions(-)
diff --git a/board/CZ.NIC/turris_mox/turris_mox.c
b/board/CZ.NIC/turris_mox/turris_mox.c
index f0c5aa6a52..4b755e1420 100644
--- a/board/CZ.NIC/turris_mox/turris_mox.c
+++ b/board/CZ.NIC/turris_mox/turris_mox.c
@@ -148,7 +148,7 @@ static int mox_do_spi(u8 *in, u8 *out, size_t size)
struct udevice *dev;
int ret;
- ret = spi_get_bus_and_cs(0, 1, 100, SPI_CPHA | SPI_CPOL,
+ ret = spi_get_bus_and_cs(0, 1, 100, SPI_CPHA | SPI_CPOL, false,
"spi_generic_drv", "moxtet@1", ,
);
if (ret)
diff --git a/cmd/sf.c b/cmd/sf.c
index 8bdebd9fd8..40b2cc3297 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -91,6 +91,7 @@ static int do_spi_flash_probe(int argc, char *const argv[])
unsigned int speed = CONFIG_SF_DEFAULT_SPEED;
unsigned int mode = CONFIG_SF_DEFAULT_MODE;
char *endp;
+ bool use_dt = true;
#if CONFIG_IS_ENABLED(DM_SPI_FLASH)
struct udevice *new, *bus_dev;
int ret;
@@ -117,11 +118,13 @@ static int do_spi_flash_probe(int argc, char *const
argv[])
speed = simple_strtoul(argv[2], , 0);
if (*argv[2] == 0 || *endp != 0)
return -1;
+ use_dt = false;
}
if (argc >= 4) {
mode = hextoul(argv[3], );
if (*argv[3] == 0 || *endp != 0)
return -1;
+ use_dt = false;
}
#if CONFIG_IS_ENABLED(DM_SPI_FLASH)
@@ -131,7 +134,11 @@ static int do_spi_flash_probe(int argc, char *const argv[])
device_remove(new, DM_REMOVE_NORMAL);
}
flash = NULL;
- ret = spi_flash_probe_bus_cs(bus, cs, speed, mode, );
+ if (use_dt)
+ ret = spi_flash_probe_bus_cs(bus, cs, speed, mode, );
+ else
+ ret = spi_flash_probe_bus_cs_default(bus, cs, speed, mode,
);
+
if (ret) {
printf("Failed to initialize SPI flash at %u:%u (error %d)\n",
bus, cs, ret);
diff --git a/cmd/spi.c b/cmd/spi.c
index 6dc32678da..46bd817e60 100644
--- a/cmd/spi.c
+++ b/cmd/spi.c
@@ -46,7 +46,7 @@ static int do_spi_xfer(int bus, int cs)
str = strdup(name);
if (!str)
return -ENOMEM;
- ret = spi_get_bus_and_cs(bus, cs, freq, mode, "spi_generic_drv",
+ ret =
