[PATCH 0/8] spi: Introduce spi-cs-setup-ns dt property

[PATCH 0/8] spi: Introduce spi-cs-setup-ns dt property

[Tudor Ambarus]

SPI NOR flashes have specific cs-setup time requirements without which
they can't work at frequencies close to their maximum supported frequency,
as they miss the first bits of the instruction command. Unrecognized
commands are ignored, thus the flash will be unresponsive. Introduce the
spi-cs-setup-ns property to allow spi devices to specify their cs setup
time.

Tudor Ambarus (8):
  spi: dt-bindings: Introduce spi-cs-setup-ns property
  spi: Introduce spi-cs-setup-ns property
  spi: Reintroduce spi_set_cs_timing()
  spi: atmel-quadspi: Add support for configuring CS timing
  ARM: dts: at91-sama5d27_wlsom1: Set sst26vf064b SPI NOR flash at its
    maximum frequency
  ARM: dts: at91-sama5d27_som1: Set sst26vf064b SPI NOR flash at its
    maximum frequency
  ARM: dts: at91: sama5d2_icp: Set sst26vf064b SPI NOR flash at its
    maximum frequency
  ARM: dts: at91: sam9x60ek: Set sst26vf064b SPI NOR flash at its
    maximum frequency

 .../bindings/spi/spi-peripheral-props.yaml    |  5 +++
 arch/arm/boot/dts/at91-sam9x60ek.dts          |  3 +-
 arch/arm/boot/dts/at91-sama5d27_som1.dtsi     |  3 +-
 arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi   |  3 +-
 arch/arm/boot/dts/at91-sama5d2_icp.dts        |  3 +-
 drivers/spi/atmel-quadspi.c                   | 34 +++++++++++++++
 drivers/spi/spi.c                             | 43 +++++++++++++++++++
 7 files changed, 90 insertions(+), 4 deletions(-)

-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/8] spi: dt-bindings: Introduce spi-cs-setup-ns property

[Tudor Ambarus]

SPI NOR flashes have specific cs-setup time requirements without which
they can't work at frequencies close to their maximum supported frequency,
as they miss the first bits of the instruction command. Unrecognized
commands are ignored, thus the flash will be unresponsive. Introduce the
spi-cs-setup-ns property to allow spi devices to specify their cs setup
time.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 .../devicetree/bindings/spi/spi-peripheral-props.yaml        | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index dca677f9e1b9..ead2cccf658f 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -44,6 +44,11 @@ properties:
     description:
       Maximum SPI clocking speed of the device in Hz.
 
+  spi-cs-setup-ns:
+    description:
+      Delay in nanosecods to be introduced by the controller after CS is
+      asserted.
+
   spi-rx-bus-width:
     description:
       Bus width to the SPI bus used for read transfers.
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/8] spi: Introduce spi-cs-setup-ns property

[Tudor Ambarus]

SPI NOR flashes have specific cs-setup time requirements without which
they can't work at frequencies close to their maximum supported frequency,
as they miss the first bits of the instruction command. Unrecognized
commands are ignored, thus the flash will be unresponsive. Introduce the
spi-cs-setup-ns property to allow spi devices to specify their cs setup
time.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/spi/spi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 4ddd250481f5..b93a6085d9a0 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2224,6 +2224,7 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 			   struct device_node *nc)
 {
 	u32 value;
+	u16 cs_setup;
 	int rc;
 
 	/* Mode (clock phase/polarity/etc.) */
@@ -2309,6 +2310,11 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
 		spi->max_speed_hz = value;
 
+	if (!of_property_read_u16(nc, "spi-cs-setup-ns", &cs_setup)) {
+		spi->cs_setup.value = cs_setup;
+		spi->cs_setup.unit = SPI_DELAY_UNIT_NSECS;
+	}
+
 	return 0;
 }
 
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/8] spi: Reintroduce spi_set_cs_timing()

[Tudor Ambarus]

commit 4ccf359849ce ("spi: remove spi_set_cs_timing()"), removed the
method as noboby used it. Nobody used it probably because some SPI
controllers use some default large cs-setup time that covers the usual
cs-setup time required by the spi devices. There are though SPI controllers
that have a smaller granularity for the cs-setup time and their default
value can't fulfill the spi device requirements. That's the case for the
at91 QSPI IPs where the default cs-setup time is half of the QSPI clock
period. This was observed when using an sst26vf064b SPI NOR flash which
needs a spi-cs-setup-ns = <7>; in order to be operated close to its maximum
104 MHz frequency.

Call spi_set_cs_timing() in spi_setup() just before calling spi_set_cs(),
as the latter needs the CS timings already set.
If spi->controller->set_cs_timing is not set, the method will return 0.
There's no functional impact expected for the existing drivers. Even if the
spi-mt65xx.c and spi-tegra114.c drivers set the set_cs_timing method,
there's no user for them as of now. The only tested user of this support
will be a SPI NOR flash that comunicates with the Atmel QSPI controller for
which the support follows in the next patches.

One will notice that this support is a bit different from the one that was
removed in commit 4ccf359849ce ("spi: remove spi_set_cs_timing()"),
because this patch adapts to the changes done after the removal: the move
of the cs delays to the spi device, the retirement of the lelgacy GPIO
handling. The mutex handling was removed from spi_set_cs_timing() because
we now always call spi_set_cs_timing() in spi_setup(), which already
handles the spi->controller->io_mutex, so use the mutex handling from
spi_setup().

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/spi/spi.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b93a6085d9a0..3cc7bb4d03de 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3621,6 +3621,37 @@ static int __spi_validate_bits_per_word(struct spi_controller *ctlr,
 	return 0;
 }
 
+/**
+ * spi_set_cs_timing - configure CS setup, hold, and inactive delays
+ * @spi: the device that requires specific CS timing configuration
+ *
+ * Return: zero on success, else a negative error code.
+ */
+static int spi_set_cs_timing(struct spi_device *spi)
+{
+	struct device *parent = spi->controller->dev.parent;
+	int status = 0;
+
+	if (spi->controller->set_cs_timing && !spi->cs_gpiod) {
+		if (spi->controller->auto_runtime_pm) {
+			status = pm_runtime_get_sync(parent);
+			if (status < 0) {
+				pm_runtime_put_noidle(parent);
+				dev_err(&spi->controller->dev, "Failed to power device: %d\n",
+					status);
+				return status;
+			}
+
+			status = spi->controller->set_cs_timing(spi);
+			pm_runtime_mark_last_busy(parent);
+			pm_runtime_put_autosuspend(parent);
+		} else {
+			status = spi->controller->set_cs_timing(spi);
+		}
+	}
+	return status;
+}
+
 /**
  * spi_setup - setup SPI mode and clock rate
  * @spi: the device whose settings are being modified
@@ -3717,6 +3748,12 @@ int spi_setup(struct spi_device *spi)
 		}
 	}
 
+	status = spi_set_cs_timing(spi);
+	if (status) {
+		mutex_unlock(&spi->controller->io_mutex);
+		return status;
+	}
+
 	if (spi->controller->auto_runtime_pm && spi->controller->set_cs) {
 		status = pm_runtime_resume_and_get(spi->controller->dev.parent);
 		if (status < 0) {
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 4/8] spi: atmel-quadspi: Add support for configuring CS timing

[Tudor Ambarus]

The at91 QSPI IP uses a default value of half of the period of the QSPI
clock period for the cs-setup time, which is not always enough, an example
being the sst26vf064b SPI NOR flash which requires a minimum cs-setup time
of 5 ns. It was observed that none of the at91 SoCs can fulfill the
minimum CS setup time for the aforementioned flash, as they operate at
high frequencies and half a period does not suffice for the required CS
setup time. Add support for configuring the CS timing in the controller.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/spi/atmel-quadspi.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index 976a217e356d..70637e46290a 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -510,6 +510,39 @@ static int atmel_qspi_setup(struct spi_device *spi)
 	return 0;
 }
 
+static int atmel_qspi_set_cs_timing(struct spi_device *spi)
+{
+	struct spi_controller *ctrl = spi->master;
+	struct atmel_qspi *aq = spi_controller_get_devdata(ctrl);
+	unsigned long clk_rate;
+	u32 cs_setup;
+	int delay;
+	int ret;
+
+	delay = spi_delay_to_ns(&spi->cs_setup, NULL);
+	if (delay <= 0)
+		return delay;
+
+	clk_rate = clk_get_rate(aq->pclk);
+	if (!clk_rate)
+		return -EINVAL;
+
+	cs_setup = DIV_ROUND_UP((delay * DIV_ROUND_UP(clk_rate, 1000000)),
+				1000);
+
+	ret = pm_runtime_resume_and_get(ctrl->dev.parent);
+	if (ret < 0)
+		return ret;
+
+	aq->scr |= QSPI_SCR_DLYBS(cs_setup);
+	atmel_qspi_write(aq->scr, aq, QSPI_SCR);
+
+	pm_runtime_mark_last_busy(ctrl->dev.parent);
+	pm_runtime_put_autosuspend(ctrl->dev.parent);
+
+	return 0;
+}
+
 static void atmel_qspi_init(struct atmel_qspi *aq)
 {
 	/* Reset the QSPI controller */
@@ -555,6 +588,7 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 
 	ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD;
 	ctrl->setup = atmel_qspi_setup;
+	ctrl->set_cs_timing = atmel_qspi_set_cs_timing;
 	ctrl->bus_num = -1;
 	ctrl->mem_ops = &atmel_qspi_mem_ops;
 	ctrl->num_chipselect = 1;
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 5/8] ARM: dts: at91-sama5d27_wlsom1: Set sst26vf064b SPI NOR flash at its maximum frequency

[Tudor Ambarus]

sama5d27-wlsom1 populates an sst26vf064b SPI NOR flash. Its maximum
operating frequency for 2.7-3.6V is 104 MHz. As the flash is operated
at 3.3V, increase its maximum supported frequency to 104MHz. The
increasing of the spi-max-frequency value requires the setting of the
"CE# Not Active Hold Time", thus set the spi-cs-setup-ns to a value of 7.

The sst26vf064b datasheet specifies just a minimum value for the
"CE# Not Active Hold Time" and it advertises it to 5 ns. There's no
maximum time specified. I determined experimentally that 5 ns for the
spi-cs-setup-ns is not enough when the flash is operated close to its
maximum frequency and tests showed that 7 ns is just fine, so set the
spi-cs-setup-ns dt property to 7.

With the increase of frequency the reads are now faster with ~37%.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi b/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi
index 83bcf9fe0152..20caf40b4755 100644
--- a/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi
+++ b/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi
@@ -220,7 +220,8 @@ qspi1_flash: flash@0 {
 		#size-cells = <1>;
 		compatible = "jedec,spi-nor";
 		reg = <0>;
-		spi-max-frequency = <80000000>;
+		spi-max-frequency = <104000000>;
+		spi-cs-setup-ns = /bits/ 16 <7>;
 		spi-rx-bus-width = <4>;
 		spi-tx-bus-width = <4>;
 		m25p,fast-read;
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 6/8] ARM: dts: at91-sama5d27_som1: Set sst26vf064b SPI NOR flash at its maximum frequency

[Tudor Ambarus]

sama5d27-som1 populates an sst26vf064b SPI NOR flash. Its maximum
operating frequency for 2.7-3.6V is 104 MHz. As the flash is operated
at 3.3V, increase its maximum supported frequency to 104MHz. The
increasing of the spi-max-frequency value requires the setting of the
"CE# Not Active Hold Time", thus set the spi-cs-setup-ns to a value of 7.

The sst26vf064b datasheet specifies just a minimum value for the
"CE# Not Active Hold Time" and it advertises it to 5 ns. There's no
maximum time specified. I determined experimentally that 5 ns for the
spi-cs-setup-ns is not enough when the flash is operated close to its
maximum frequency and tests showed that 7 ns is just fine, so set the
spi-cs-setup-ns dt property to 7.

With the increase of frequency the reads are now faster with ~37%.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 arch/arm/boot/dts/at91-sama5d27_som1.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/at91-sama5d27_som1.dtsi b/arch/arm/boot/dts/at91-sama5d27_som1.dtsi
index 8aa9e8dea337..243f09f40761 100644
--- a/arch/arm/boot/dts/at91-sama5d27_som1.dtsi
+++ b/arch/arm/boot/dts/at91-sama5d27_som1.dtsi
@@ -43,7 +43,8 @@ flash@0 {
 					#size-cells = <1>;
 					compatible = "jedec,spi-nor";
 					reg = <0>;
-					spi-max-frequency = <80000000>;
+					spi-max-frequency = <104000000>;
+					spi-cs-setup-ns = /bits/ 16 <7>;
 					spi-tx-bus-width = <4>;
 					spi-rx-bus-width = <4>;
 					m25p,fast-read;
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 7/8] ARM: dts: at91: sama5d2_icp: Set sst26vf064b SPI NOR flash at its maximum frequency

[Tudor Ambarus]

sama5d2_icp populates an sst26vf064b SPI NOR flash. Its maximum operating
frequency for 2.7-3.6V is 104 MHz. As the flash is operated at 3.3V,
increase its maximum supported frequency to 104MHz. The increasing of the
spi-max-frequency value requires the setting of the
"CE# Not Active Hold Time", thus set the spi-cs-setup-ns to a value of 7.

The sst26vf064b datasheet specifies just a minimum value for the
"CE# Not Active Hold Time" and it advertises it to 5 ns. There's no
maximum time specified. I determined experimentally that 5 ns for the
spi-cs-setup-ns is not enough when the flash is operated close to its
maximum frequency and tests showed that 7 ns is just fine, so set the
spi-cs-setup-ns dt property to 7.

With the increase of frequency the reads are now faster with ~37%.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 arch/arm/boot/dts/at91-sama5d2_icp.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/at91-sama5d2_icp.dts b/arch/arm/boot/dts/at91-sama5d2_icp.dts
index dd1dec9d4e07..ffd9627874c9 100644
--- a/arch/arm/boot/dts/at91-sama5d2_icp.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_icp.dts
@@ -669,7 +669,8 @@ flash@0 {
 		#size-cells = <1>;
 		compatible = "jedec,spi-nor";
 		reg = <0>;
-		spi-max-frequency = <80000000>;
+		spi-max-frequency = <104000000>;
+		spi-cs-setup-ns = /bits/ 16 <7>;
 		spi-tx-bus-width = <4>;
 		spi-rx-bus-width = <4>;
 		m25p,fast-read;
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 8/8] ARM: dts: at91: sam9x60ek: Set sst26vf064b SPI NOR flash at its maximum frequency

[Tudor Ambarus]

sam9x60ek populates an sst26vf064b SPI NOR flash. Its maximum operating
frequency for 2.7-3.6V is 104 MHz. As the flash is operated at 3.3V,
increase its maximum supported frequency to 104MHz. The increasing of the
spi-max-frequency value requires the setting of the
"CE# Not Active Hold Time", thus set the spi-cs-setup-ns to a value of 7.

The sst26vf064b datasheet specifies just a minimum value for the
"CE# Not Active Hold Time" and it advertises it to 5 ns. There's no
maximum time specified. I determined experimentally that 5 ns for the
spi-cs-setup-ns is not enough when the flash is operated close to its
maximum frequency and tests showed that 7 ns is just fine, so set the
spi-cs-setup-ns dt property to 7.

With the increase of frequency the reads are now faster with ~33%.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 arch/arm/boot/dts/at91-sam9x60ek.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/at91-sam9x60ek.dts b/arch/arm/boot/dts/at91-sam9x60ek.dts
index 4ba52ba11dc6..6cbb4e0d9938 100644
--- a/arch/arm/boot/dts/at91-sam9x60ek.dts
+++ b/arch/arm/boot/dts/at91-sam9x60ek.dts
@@ -612,7 +612,8 @@ flash@0 {
 		#size-cells = <1>;
 		compatible = "jedec,spi-nor";
 		reg = <0>;
-		spi-max-frequency = <80000000>;
+		spi-max-frequency = <104000000>;
+		spi-cs-setup-ns = /bits/ 16 <7>;
 		spi-tx-bus-width = <4>;
 		spi-rx-bus-width = <4>;
 		m25p,fast-read;
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: (subset) [PATCH 0/8] spi: Introduce spi-cs-setup-ns dt property

[Mark Brown]

On Thu, 17 Nov 2022 12:52:41 +0200, Tudor Ambarus wrote:
> SPI NOR flashes have specific cs-setup time requirements without which
> they can't work at frequencies close to their maximum supported frequency,
> as they miss the first bits of the instruction command. Unrecognized
> commands are ignored, thus the flash will be unresponsive. Introduce the
> spi-cs-setup-ns property to allow spi devices to specify their cs setup
> time.
> 
> [...]

Applied to

   broonie/spi.git for-next

Thanks!

[1/8] spi: dt-bindings: Introduce spi-cs-setup-ns property
      commit: f6c911f3308c1cfb97ae1da6654080d7104e2df2
[2/8] spi: Introduce spi-cs-setup-ns property
      commit: 33a2fde5f77bd744b8bd0c694bc173cc968e55a5
[3/8] spi: Reintroduce spi_set_cs_timing()
      commit: 684a47847ae639689e7b823251975348a8e5434f
[4/8] spi: atmel-quadspi: Add support for configuring CS timing
      commit: f732646d0ccd22f42ed7de5e59c0abb7a848e034

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/8] spi: dt-bindings: Introduce spi-cs-setup-ns property

[Michael Walle]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index dca677f9e1b9..ead2cccf658f 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -44,6 +44,11 @@ properties:
>      description:
>        Maximum SPI clocking speed of the device in Hz.
>  
> +  spi-cs-setup-ns:
> +    description:
> +      Delay in nanosecods to be introduced by the controller after CS is
> +      asserted.
> +

Does this need a type as the spi-cs-setup-ns is apparently just 16bit? At
least the driver uses it that way.

But IMHO this should just be a normal uint32 value to be consistent with
all the other properties. Also the max value with 16bit will be 'just'
65us.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/8] spi: dt-bindings: Introduce spi-cs-setup-ns property

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 756 bytes --]

On Fri, Nov 18, 2022 at 03:14:58PM +0100, Michael Walle wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>

> > +  spi-cs-setup-ns:
> > +    description:
> > +      Delay in nanosecods to be introduced by the controller after CS is
> > +      asserted.

> Does this need a type as the spi-cs-setup-ns is apparently just 16bit? At
> least the driver uses it that way.

> But IMHO this should just be a normal uint32 value to be consistent with
> all the other properties. Also the max value with 16bit will be 'just'
> 65us.

Making it 32 bit does seem safer.  I've applied the series
already, mainly for the reintroduction of spi_set_cs_timing()
since there was another driver that needed it, but we can still
fix things up until the merge window.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/8] spi: dt-bindings: Introduce spi-cs-setup-ns property

[Tudor Ambarus]

Hi,

On 18.11.2022 17:30, Mark Brown wrote:
> On Fri, Nov 18, 2022 at 03:14:58PM +0100, Michael Walle wrote:
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
>>> +  spi-cs-setup-ns:
>>> +    description:
>>> +      Delay in nanosecods to be introduced by the controller after CS is
>>> +      asserted.
> 
>> Does this need a type as the spi-cs-setup-ns is apparently just 16bit? At
>> least the driver uses it that way.
> 
>> But IMHO this should just be a normal uint32 value to be consistent with
>> all the other properties. Also the max value with 16bit will be 'just'
>> 65us.
> 
> Making it 32 bit does seem safer.  I've applied the series

Thanks. There are few implications to consider before making this prop a
u32, and I'd like to check them with you.

struct spi_delay will have to be updated to have a u32 value, now it's a
u16. This means that we'll have to update spi_delay_to_ns() to either
return a s64 or to add a u64 *delay parameter to the function so that we
can still handle the conversions from usecs and the error codes in the
SPI_DELAY_UNIT_SCK case. Then all its callers have to be updated to
consider the u64 delay.

I don't know what to say, I'm in between. 65us delays are improbable,
but I'm fine to update this as well. Let me know your preference.

Thanks,
ta

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/8] spi: dt-bindings: Introduce spi-cs-setup-ns property

[Michael Walle]

Hi,

Am 2023-01-02 10:37, schrieb Tudor Ambarus:
> Hi,
> 
> On 18.11.2022 17:30, Mark Brown wrote:
>> On Fri, Nov 18, 2022 at 03:14:58PM +0100, Michael Walle wrote:
>>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>> 
>>>> +  spi-cs-setup-ns:
>>>> +    description:
>>>> +      Delay in nanosecods to be introduced by the controller after 
>>>> CS is
>>>> +      asserted.
>> 
>>> Does this need a type as the spi-cs-setup-ns is apparently just 
>>> 16bit? At
>>> least the driver uses it that way.
>> 
>>> But IMHO this should just be a normal uint32 value to be consistent 
>>> with
>>> all the other properties. Also the max value with 16bit will be 
>>> 'just'
>>> 65us.
>> 
>> Making it 32 bit does seem safer.  I've applied the series
> 
> Thanks. There are few implications to consider before making this prop 
> a
> u32, and I'd like to check them with you.
> 
> struct spi_delay will have to be updated to have a u32 value, now it's 
> a
> u16. This means that we'll have to update spi_delay_to_ns() to either
> return a s64 or to add a u64 *delay parameter to the function so that 
> we
> can still handle the conversions from usecs and the error codes in the
> SPI_DELAY_UNIT_SCK case. Then all its callers have to be updated to
> consider the u64 delay.

I was talking about the device tree property. Even if the driver 
continue
to use just 16bit, the DT property could be 32bit IMHO.

At the moment, the schema says its 32bit (if I'm not mistaken, because
it doesn't have a type), but the driver will parse the property as
16bit and your device tree also has this /bits/ thingy. So regardless
if the driver is using 16bit or 32bit for the value, there seems to be
a discrepancy between the schema and the devicetree (and driver).

All other properties are just the regular 32bit values, thus I was
suggesting to change the DT property to 32bit.

-michael

> I don't know what to say, I'm in between. 65us delays are improbable,
> but I'm fine to update this as well. Let me know your preference.
> 
> Thanks,
> ta

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/8] spi: dt-bindings: Introduce spi-cs-setup-ns property

[Tudor Ambarus]

On 02.01.2023 13:48, Michael Walle wrote:
> Hi,

Hi,

> 
> Am 2023-01-02 10:37, schrieb Tudor Ambarus:
>> Hi,
>>
>> On 18.11.2022 17:30, Mark Brown wrote:
>>> On Fri, Nov 18, 2022 at 03:14:58PM +0100, Michael Walle wrote:
>>>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>
>>>>> +  spi-cs-setup-ns:
>>>>> +    description:
>>>>> +      Delay in nanosecods to be introduced by the controller after 
>>>>> CS is
>>>>> +      asserted.
>>>
>>>> Does this need a type as the spi-cs-setup-ns is apparently just 
>>>> 16bit? At
>>>> least the driver uses it that way.
>>>
>>>> But IMHO this should just be a normal uint32 value to be consistent 
>>>> with
>>>> all the other properties. Also the max value with 16bit will be 'just'
>>>> 65us.
>>>
>>> Making it 32 bit does seem safer.  I've applied the series
>>
>> Thanks. There are few implications to consider before making this prop a
>> u32, and I'd like to check them with you.
>>
>> struct spi_delay will have to be updated to have a u32 value, now it's a
>> u16. This means that we'll have to update spi_delay_to_ns() to either
>> return a s64 or to add a u64 *delay parameter to the function so that we
>> can still handle the conversions from usecs and the error codes in the
>> SPI_DELAY_UNIT_SCK case. Then all its callers have to be updated to
>> consider the u64 delay.
> 
> I was talking about the device tree property. Even if the driver continue
> to use just 16bit, the DT property could be 32bit IMHO.

but then you'll have an implicit cast to u16 at:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c#n2314
which will make the u32 dt prop misleading.

> 
> At the moment, the schema says its 32bit (if I'm not mistaken, because
> it doesn't have a type), but the driver will parse the property as
> 16bit and your device tree also has this /bits/ thingy. So regardless
> if the driver is using 16bit or 32bit for the value, there seems to be
> a discrepancy between the schema and the devicetree (and driver).

okay, thanks for pointing it out. Let's decide how we fix this.

> 
> All other properties are just the regular 32bit values, thus I was
> suggesting to change the DT property to 32bit.

If we want to change the dt prop to 32bit I think we should also handle
the parsed value as u32, not as u16.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/8] spi: dt-bindings: Introduce spi-cs-setup-ns property

[Michael Walle]

>>>>>> +  spi-cs-setup-ns:
>>>>>> +    description:
>>>>>> +      Delay in nanosecods to be introduced by the controller 
>>>>>> after CS is
>>>>>> +      asserted.
>>>> 
>>>>> Does this need a type as the spi-cs-setup-ns is apparently just 
>>>>> 16bit? At
>>>>> least the driver uses it that way.
>>>> 
>>>>> But IMHO this should just be a normal uint32 value to be consistent 
>>>>> with
>>>>> all the other properties. Also the max value with 16bit will be 
>>>>> 'just'
>>>>> 65us.
>>>> 
>>>> Making it 32 bit does seem safer.  I've applied the series
>>> 
>>> Thanks. There are few implications to consider before making this 
>>> prop a
>>> u32, and I'd like to check them with you.
>>> 
>>> struct spi_delay will have to be updated to have a u32 value, now 
>>> it's a
>>> u16. This means that we'll have to update spi_delay_to_ns() to either
>>> return a s64 or to add a u64 *delay parameter to the function so that 
>>> we
>>> can still handle the conversions from usecs and the error codes in 
>>> the
>>> SPI_DELAY_UNIT_SCK case. Then all its callers have to be updated to
>>> consider the u64 delay.
>> 
>> I was talking about the device tree property. Even if the driver 
>> continue
>> to use just 16bit, the DT property could be 32bit IMHO.
> 
> but then you'll have an implicit cast to u16 at:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c#n2314
> which will make the u32 dt prop misleading.

Nothing will prevent you from checking for a valid range and return an
error :)
But I agree, that converting the u16 to u32 in the driver is probably
the better way.

>> At the moment, the schema says its 32bit (if I'm not mistaken, because
>> it doesn't have a type), but the driver will parse the property as
>> 16bit and your device tree also has this /bits/ thingy. So regardless
>> if the driver is using 16bit or 32bit for the value, there seems to be
>> a discrepancy between the schema and the devicetree (and driver).
> 
> okay, thanks for pointing it out. Let's decide how we fix this.
> 
>> 
>> All other properties are just the regular 32bit values, thus I was
>> suggesting to change the DT property to 32bit.
> 
> If we want to change the dt prop to 32bit I think we should also handle
> the parsed value as u32, not as u16.

Strictly speaking, your device tree is wrong, because the schema
already says it's 32bit.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 5/8] ARM: dts: at91-sama5d27_wlsom1: Set sst26vf064b SPI NOR flash at its maximum frequency

[Nicolas Ferre]

Hi Tudor,

On 17/11/2022 at 11:52, Tudor Ambarus wrote:
> sama5d27-wlsom1 populates an sst26vf064b SPI NOR flash. Its maximum
> operating frequency for 2.7-3.6V is 104 MHz. As the flash is operated
> at 3.3V, increase its maximum supported frequency to 104MHz. The
> increasing of the spi-max-frequency value requires the setting of the
> "CE# Not Active Hold Time", thus set the spi-cs-setup-ns to a value of 7.
> 
> The sst26vf064b datasheet specifies just a minimum value for the
> "CE# Not Active Hold Time" and it advertises it to 5 ns. There's no
> maximum time specified. I determined experimentally that 5 ns for the
> spi-cs-setup-ns is not enough when the flash is operated close to its
> maximum frequency and tests showed that 7 ns is just fine, so set the
> spi-cs-setup-ns dt property to 7.
> 
> With the increase of frequency the reads are now faster with ~37%.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>   arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi b/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi
> index 83bcf9fe0152..20caf40b4755 100644
> --- a/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi
> +++ b/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi
> @@ -220,7 +220,8 @@ qspi1_flash: flash@0 {
>   		#size-cells = <1>;
>   		compatible = "jedec,spi-nor";
>   		reg = <0>;
> -		spi-max-frequency = <80000000>;
> +		spi-max-frequency = <104000000>;
> +		spi-cs-setup-ns = /bits/ 16 <7>;

Following the different changes that happened to this property after 
this post, am I right saying that this must now be changed to:

spi-cs-setup-delay-ns = <7>;

?

Thanks for your insight. Best regards,
   Nicolas

>   		spi-rx-bus-width = <4>;
>   		spi-tx-bus-width = <4>;
>   		m25p,fast-read;

-- 
Nicolas Ferre


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 5/8] ARM: dts: at91-sama5d27_wlsom1: Set sst26vf064b SPI NOR flash at its maximum frequency

[Tudor Ambarus]

On 3/28/23 09:51, Nicolas Ferre wrote:
> Hi Tudor,

Hi!

> 
> On 17/11/2022 at 11:52, Tudor Ambarus wrote:
>> sama5d27-wlsom1 populates an sst26vf064b SPI NOR flash. Its maximum
>> operating frequency for 2.7-3.6V is 104 MHz. As the flash is operated
>> at 3.3V, increase its maximum supported frequency to 104MHz. The
>> increasing of the spi-max-frequency value requires the setting of the
>> "CE# Not Active Hold Time", thus set the spi-cs-setup-ns to a value of 7.
>>
>> The sst26vf064b datasheet specifies just a minimum value for the
>> "CE# Not Active Hold Time" and it advertises it to 5 ns. There's no
>> maximum time specified. I determined experimentally that 5 ns for the
>> spi-cs-setup-ns is not enough when the flash is operated close to its
>> maximum frequency and tests showed that 7 ns is just fine, so set the
>> spi-cs-setup-ns dt property to 7.
>>
>> With the increase of frequency the reads are now faster with ~37%.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>   arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi
>> b/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi
>> index 83bcf9fe0152..20caf40b4755 100644
>> --- a/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi
>> +++ b/arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi
>> @@ -220,7 +220,8 @@ qspi1_flash: flash@0 {
>>           #size-cells = <1>;
>>           compatible = "jedec,spi-nor";
>>           reg = <0>;
>> -        spi-max-frequency = <80000000>;
>> +        spi-max-frequency = <104000000>;
>> +        spi-cs-setup-ns = /bits/ 16 <7>;
> 
> Following the different changes that happened to this property after
> this post, am I right saying that this must now be changed to:
> 
> spi-cs-setup-delay-ns = <7>;
> 
> ?
> 

Yes, that should do it. I'm amending the series right now. Can you do a
little test on your side so that we make sure everything is in place?
After the update, something like that should be run on any board (maybe
wlsom1-ek?):
#!/bin/sh

dd if=/dev/urandom of=./qspi_test bs=1M count=6
mtd_debug write /dev/mtd5 0 6291456 qspi_test
mtd_debug erase /dev/mtd5 0 6291456
mtd_debug read /dev/mtd5 0 6291456 qspi_read
hexdump qspi_read
mtd_debug write /dev/mtd5 0 6291456 qspi_test
mtd_debug read /dev/mtd5 0 6291456 qspi_read
sha1sum qspi_test qspi_read

brb,
ta

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/