[PATCH 0/2] spi: spi-fsl-qspi: support setting sampling delay through devicetree

[PATCH 0/2] spi: spi-fsl-qspi: support setting sampling delay through devicetree

[Mario Kicherer]

The internal sampling point of incoming data can be delayed by modifying
the QuadSPI_SMPR register. This patch enables setting this delay using a
device tree entry.

Mario Kicherer (2):
  spi: dt-bindings: spi-fsl-qspi: add optional sampling-delay
  spi: spi-fsl-qspi: support setting sampling delay through devicetree

 .../bindings/spi/fsl,spi-fsl-qspi.yaml        |  6 ++++++
 drivers/spi/spi-fsl-qspi.c                    | 21 +++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH 1/2] spi: dt-bindings: spi-fsl-qspi: add optional sampling-delay

[Mario Kicherer]

Add optional sampling-delay property to delay the internal sampling point for
incoming data.

Signed-off-by: Mario Kicherer <dev@kicherer.org>
---
 Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml b/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml
index e58644558412..7952a4be938b 100644
--- a/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml
+++ b/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml
@@ -54,6 +54,12 @@ properties:
       - const: qspi_en
       - const: qspi
 
+  fsl,qspi-sampling-delay:
+    description: delay sampling of incoming data by this number of half cycles
+    minimum: 0
+    maximum: 3
+    default: 0
+
 required:
   - compatible
   - reg
-- 
2.34.1

[PATCH 2/2] spi: spi-fsl-qspi: support setting sampling delay through devicetree

[Mario Kicherer]

The internal sampling point of incoming data can be delayed by modifying
the QuadSPI_SMPR register. This patch enables setting this delay using a
device tree entry.

Signed-off-by: Mario Kicherer <dev@kicherer.org>
---
 drivers/spi/spi-fsl-qspi.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
index 85cc71ba624a..10faaf57db28 100644
--- a/drivers/spi/spi-fsl-qspi.c
+++ b/drivers/spi/spi-fsl-qspi.c
@@ -722,6 +722,7 @@ static int fsl_qspi_default_setup(struct fsl_qspi *q)
 {
 	void __iomem *base = q->iobase;
 	u32 reg, addr_offset = 0;
+	u8 sampling_delay;
 	int ret;
 
 	/* disable and unprepare clock to avoid glitch pass to controller */
@@ -756,10 +757,22 @@ static int fsl_qspi_default_setup(struct fsl_qspi *q)
 			    base + QUADSPI_FLSHCR);
 
 	reg = qspi_readl(q, base + QUADSPI_SMPR);
-	qspi_writel(q, reg & ~(QUADSPI_SMPR_FSDLY_MASK
-			| QUADSPI_SMPR_FSPHS_MASK
-			| QUADSPI_SMPR_HSENA_MASK
-			| QUADSPI_SMPR_DDRSMP_MASK), base + QUADSPI_SMPR);
+	reg = reg & ~(QUADSPI_SMPR_FSDLY_MASK
+		      | QUADSPI_SMPR_FSPHS_MASK
+		      | QUADSPI_SMPR_HSENA_MASK
+		      | QUADSPI_SMPR_DDRSMP_MASK);
+	ret = of_property_read_u8(q->dev->of_node,
+				   "fsl,qspi-sampling-delay",
+				   &sampling_delay);
+	if (!ret) {
+		if (sampling_delay <= 3)
+			reg = reg | (sampling_delay << 5);
+		else
+			dev_err(q->dev,
+				"fsl,qspi-sampling_delay %u greater than 3\n",
+				sampling_delay);
+	}
+	qspi_writel(q, reg, base + QUADSPI_SMPR);
 
 	/* We only use the buffer3 for AHB read */
 	qspi_writel(q, 0, base + QUADSPI_BUF0IND);
-- 
2.34.1

Re: [PATCH 1/2] spi: dt-bindings: spi-fsl-qspi: add optional sampling-delay

[Rob Herring]

On Mon, 16 Jan 2023 12:50:49 +0100, Mario Kicherer wrote:
> Add optional sampling-delay property to delay the internal sampling point for
> incoming data.
> 
> Signed-off-by: Mario Kicherer <dev@kicherer.org>
> ---
>  Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml: properties:fsl,qspi-sampling-delay: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('default', 'maximum', 'minimum' were unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml: properties:fsl,qspi-sampling-delay: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml: properties:fsl,qspi-sampling-delay: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230116115050.2983406-2-dev@kicherer.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/2] spi: dt-bindings: spi-fsl-qspi: add optional sampling-delay

[Rob Herring]

On Mon, Jan 16, 2023 at 12:50:49PM +0100, Mario Kicherer wrote:
> Add optional sampling-delay property to delay the internal sampling point for
> incoming data.
> 
> Signed-off-by: Mario Kicherer <dev@kicherer.org>
> ---
>  Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml b/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml
> index e58644558412..7952a4be938b 100644
> --- a/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml
> +++ b/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml
> @@ -54,6 +54,12 @@ properties:
>        - const: qspi_en
>        - const: qspi
>  
> +  fsl,qspi-sampling-delay:
> +    description: delay sampling of incoming data by this number of half cycles

Use the common rx-sample-delay-ns property.

> +    minimum: 0
> +    maximum: 3
> +    default: 0
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] spi: dt-bindings: spi-fsl-qspi: add optional sampling-delay

[Mario Kicherer]

Hello,

unfortunately, the rx-sample-delay-ns property does not fit here, as we 
can only delay
the sampling point between zero and three "half cycles" (or edges), not 
by an arbitrary
number of nanoseconds.

Regarding the bot message, I do not understand what is wrong in my 
patch. I found similar
property descriptions in other files and also in the official doc [1] 
there is an equal
(to me) example under "clock-frequency", if I am not missing something.

Thank you for the review!

Best regards,
Mario

[1] https://docs.kernel.org/devicetree/bindings/writing-schema.html

On 2023-01-17 15:10, Rob Herring wrote:
> On Mon, Jan 16, 2023 at 12:50:49PM +0100, Mario Kicherer wrote:
>> Add optional sampling-delay property to delay the internal sampling 
>> point for
>> incoming data.
>> 
>> Signed-off-by: Mario Kicherer <dev@kicherer.org>
>> ---
>>  Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml | 6 
>> ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml 
>> b/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml
>> index e58644558412..7952a4be938b 100644
>> --- a/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/fsl,spi-fsl-qspi.yaml
>> @@ -54,6 +54,12 @@ properties:
>>        - const: qspi_en
>>        - const: qspi
>> 
>> +  fsl,qspi-sampling-delay:
>> +    description: delay sampling of incoming data by this number of 
>> half cycles
> 
> Use the common rx-sample-delay-ns property.
> 
>> +    minimum: 0
>> +    maximum: 3
>> +    default: 0
>> +
>>  required:
>>    - compatible
>>    - reg
>> --
>> 2.34.1
>> 

Re: [PATCH 1/2] spi: dt-bindings: spi-fsl-qspi: add optional sampling-delay

[Krzysztof Kozlowski]

On 17/01/2023 17:33, Mario Kicherer wrote:
> Hello,
> 
> unfortunately, the rx-sample-delay-ns property does not fit here, as we 
> can only delay
> the sampling point between zero and three "half cycles" (or edges), not 
> by an arbitrary
> number of nanoseconds.

Why this is a problem for FSL but not for other platforms having exactly
the same constraints/property?
https://lore.kernel.org/all/20221208062955.2546-6-xiangsheng.hou@mediatek.com/

Best regards,
Krzysztof

Re: [PATCH 1/2] spi: dt-bindings: spi-fsl-qspi: add optional sampling-delay

[han.xu]

On 23/01/17 06:10PM, Krzysztof Kozlowski wrote:
> On 17/01/2023 17:33, Mario Kicherer wrote:
> > Hello,
> > 
> > unfortunately, the rx-sample-delay-ns property does not fit here, as we 
> > can only delay
> > the sampling point between zero and three "half cycles" (or edges), not 
> > by an arbitrary
> > number of nanoseconds.
> 
> Why this is a problem for FSL but not for other platforms having exactly
> the same constraints/property?

Hi Mario,

Please use the common delay in DT and calculate to half cycle in driver, we have
the similar discussion before for fspi controller delay settings.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] spi: dt-bindings: spi-fsl-qspi: add optional sampling-delay

[Michael Walle]

From: "han.xu" <han.xu@nxp.com>

Hi,

>>> unfortunately, the rx-sample-delay-ns property does not fit here, as we 
>>> can only delay
>>> the sampling point between zero and three "half cycles" (or edges), not 
>>> by an arbitrary
>>> number of nanoseconds.
>> 
>> Why this is a problem for FSL but not for other platforms having exactly
>> the same constraints/property?
>
> Please use the common delay in DT and calculate to half cycle in driver, we have
> the similar discussion before for fspi controller delay settings.

Do you mean [1]? There my suggestion was to use a -degrees property (because
it doesn't depend on the frequency). There wasn't any follow-up, or did I miss
something?

-michael

[1] https://lore.kernel.org/linux-spi/62f113a0cdb0d58bf04ab0b274912eb7@walle.cc/

Re: [PATCH 1/2] spi: dt-bindings: spi-fsl-qspi: add optional sampling-delay

[Krzysztof Kozlowski]

On 18/01/2023 09:01, Michael Walle wrote:
> From: "han.xu" <han.xu@nxp.com>
> 
> Hi,
> 
>>>> unfortunately, the rx-sample-delay-ns property does not fit here, as we 
>>>> can only delay
>>>> the sampling point between zero and three "half cycles" (or edges), not 
>>>> by an arbitrary
>>>> number of nanoseconds.
>>>
>>> Why this is a problem for FSL but not for other platforms having exactly
>>> the same constraints/property?
>>
>> Please use the common delay in DT and calculate to half cycle in driver, we have
>> the similar discussion before for fspi controller delay settings.
> 
> Do you mean [1]? There my suggestion was to use a -degrees property (because
> it doesn't depend on the frequency). There wasn't any follow-up, or did I miss
> something?
> 
> -michael
> 
> [1] https://lore.kernel.org/linux-spi/62f113a0cdb0d58bf04ab0b274912eb7@walle.cc/

I think the patch using existing ns property (and calculating cycles or
phase shift or whatever was needed) was merged. In such case please go
the same way.

Best regards,
Krzysztof

Re: [PATCH 1/2] spi: dt-bindings: spi-fsl-qspi: add optional sampling-delay

[Michael Walle]

>>>>> unfortunately, the rx-sample-delay-ns property does not fit here, 
>>>>> as we
>>>>> can only delay
>>>>> the sampling point between zero and three "half cycles" (or edges), 
>>>>> not
>>>>> by an arbitrary
>>>>> number of nanoseconds.
>>>> 
>>>> Why this is a problem for FSL but not for other platforms having 
>>>> exactly
>>>> the same constraints/property?
>>> 
>>> Please use the common delay in DT and calculate to half cycle in 
>>> driver, we have
>>> the similar discussion before for fspi controller delay settings.
>> 
>> Do you mean [1]? There my suggestion was to use a -degrees property 
>> (because
>> it doesn't depend on the frequency). There wasn't any follow-up, or 
>> did I miss
>> something?
>> 
>> -michael
>> 
>> [1] 
>> https://lore.kernel.org/linux-spi/62f113a0cdb0d58bf04ab0b274912eb7@walle.cc/
> 
> I think the patch using existing ns property (and calculating cycles or
> phase shift or whatever was needed) was merged. In such case please go
> the same way.

I couldn't find anything that the fspi driver now supports this delay.

I still think this is the wrong way to go. Like I said, it depends on
the frequency, which means that you need to change the delay-ns property
everytime you change the frequency. Think of a bootloader which patches
the frequency (or something like that). But what's worse is that you
cannot have an enum in the binding for this property then.

Now OTOH, you could actually have a hardware which take the
delay as a time period, in that case this -delay-ns makes sense
and recalculating that into -degrees would be impossible.

Are there standard properties which expresses the same, but just in a
different way? Like for example drive strength as an impedance or in
current at a specific voltage. But having two different properties for
the same thing might be just as bad..

-michael

Re: [PATCH 1/2] spi: dt-bindings: spi-fsl-qspi: add optional sampling-delay

[Mario Kicherer]

Hello Han,

on my SoC (LS1021a), the QSPI clock is derived from the CPU clock 
(cluster1)
and neither is controlled by the Linux kernel (afair) but by the RCW (or 
U-Boot).
I could create a function to read the corresponding registers but I do 
not know
where I should place this function and how I should call this function 
in a
portable way in the QSPI module to convert the nanoseconds into delay 
cycles.

I thought this would be a small and simple patch but I guess these 
changes will
require quite some time with my knowledge level that I do not have right 
now.

Thank you all for the review!

Best regards,
Mario

On 2023-01-17 22:05, han.xu wrote:
> On 23/01/17 06:10PM, Krzysztof Kozlowski wrote:
>> On 17/01/2023 17:33, Mario Kicherer wrote:
>> > Hello,
>> >
>> > unfortunately, the rx-sample-delay-ns property does not fit here, as we
>> > can only delay
>> > the sampling point between zero and three "half cycles" (or edges), not
>> > by an arbitrary
>> > number of nanoseconds.
>> 
>> Why this is a problem for FSL but not for other platforms having 
>> exactly
>> the same constraints/property?
> 
> Hi Mario,
> 
> Please use the common delay in DT and calculate to half cycle in 
> driver, we have
> the similar discussion before for fspi controller delay settings.
> 
>> 
>> Best regards,
>> Krzysztof
>>