[07/11] dt-bindings: spi: spi-nxp-fspi: add a new property nxp,fspi-dll-slvdly

Commit Message

Bough Chen July 5, 2022, 9:11 a.m. UTC

From: Haibo Chen <[e-mail]>

Add one optional property nxp,fspi-dll-slvdly

Signed-off-by: Haibo Chen <[e-mail]>
---
 Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Krzysztof Kozlowski July 5, 2022, 9:48 a.m. UTC | #1

On 05/07/2022 11:11, [e-mail] wrote:
> From: Haibo Chen <[e-mail]>
> 
> Add one optional property nxp,fspi-dll-slvdly
> 
> Signed-off-by: Haibo Chen <[e-mail]>
> ---
>  Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml b/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
> index 1b552c298277..6bd61565686a 100644
> --- a/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
> @@ -45,6 +45,12 @@ properties:
>        - const: fspi_en
>        - const: fspi
>  
> +  nxp,fspi-dll-slvdly:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Specify the DLL slave line delay value.

What are the units?

Best regards,
Krzysztof

Bough Chen July 5, 2022, 10:28 a.m. UTC | #2

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: 2022年7月5日 17:48
> To: Bough Chen <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Han Xu <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>;
> [e-mail]; [e-mail]
> Subject: Re: [PATCH 07/11] dt-bindings: spi: spi-nxp-fspi: add a new property
> nxp,fspi-dll-slvdly
> 
> On 05/07/2022 11:11, [e-mail] wrote:
> > From: Haibo Chen <[e-mail]>
> >
> > Add one optional property nxp,fspi-dll-slvdly
> >
> > Signed-off-by: Haibo Chen <[e-mail]>
> > ---
> >  Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
> b/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
> > index 1b552c298277..6bd61565686a 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
> > @@ -45,6 +45,12 @@ properties:
> >        - const: fspi_en
> >        - const: fspi
> >
> > +  nxp,fspi-dll-slvdly:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      Specify the DLL slave line delay value.
> 
> What are the units?

Do you mean here need to give more detail explain about this properity?

How about change like this?
   Specify the DLL slave line delay value. The delay target for slave delay line is: ((nxp,fspi-dll-slvdly+1) * 1/32 * clock cycle of reference clock (serial root clock). The range of this value is 0~16.


Best Regards
Haibo Chen

> 
> Best regards,
> Krzysztof

Krzysztof Kozlowski July 5, 2022, 10:36 a.m. UTC | #3

On 05/07/2022 12:28, Bough Chen wrote:
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: 2022年7月5日 17:48
>> To: Bough Chen <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; Han Xu <[e-mail]>;
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; dl-linux-imx <[e-mail]>;
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH 07/11] dt-bindings: spi: spi-nxp-fspi: add a new property
>> nxp,fspi-dll-slvdly
>>
>> On 05/07/2022 11:11, [e-mail] wrote:
>>> From: Haibo Chen <[e-mail]>
>>>
>>> Add one optional property nxp,fspi-dll-slvdly
>>>
>>> Signed-off-by: Haibo Chen <[e-mail]>
>>> ---
>>>  Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
>> b/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
>>> index 1b552c298277..6bd61565686a 100644
>>> --- a/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
>>> @@ -45,6 +45,12 @@ properties:
>>>        - const: fspi_en
>>>        - const: fspi
>>>
>>> +  nxp,fspi-dll-slvdly:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description: |
>>> +      Specify the DLL slave line delay value.
>>
>> What are the units?
> 
> Do you mean here need to give more detail explain about this properity?
> 
> How about change like this?
>    Specify the DLL slave line delay value. The delay target for slave delay line is: ((nxp,fspi-dll-slvdly+1) * 1/32 * clock cycle of reference clock (serial root clock). 

This would be good.

> The range of this value is 0~16.

This needs to go to schema instead as "maximum: 16".

But still the question is - what are the units used in this "delay"? ms? us?

Best regards,
Krzysztof

Han Xu July 5, 2022, 1:19 p.m. UTC | #4

>-----Original Message-----
>From: Krzysztof Kozlowski <[e-mail]>
>Sent: Tuesday, July 5, 2022 5:37 AM
>To: Bough Chen <[e-mail]>; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; Han Xu <[e-mail]>;
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail]
>Cc: [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>dl-linux-imx <[e-mail]>; [e-mail];
>[e-mail]
>Subject: Re: [PATCH 07/11] dt-bindings: spi: spi-nxp-fspi: add a new property
>nxp,fspi-dll-slvdly
>
>On 05/07/2022 12:28, Bough Chen wrote:
>>> -----Original Message-----
>>> From: Krzysztof Kozlowski <[e-mail]>
>>> Sent: 2022年7月5日 17:48
>>> To: Bough Chen <[e-mail]>; [e-mail];
>>> [e-mail]; [e-mail]; [e-mail];
>>> [e-mail]; Han Xu <[e-mail]>;
>>> [e-mail]; [e-mail];
>>> [e-mail]; [e-mail]; [e-mail];
>>> [e-mail]; [e-mail]; [e-mail];
>>> [e-mail]; [e-mail]
>>> Cc: [e-mail]; [e-mail];
>>> [e-mail]; [e-mail];
>>> [e-mail]; dl-linux-imx <[e-mail]>;
>>> [e-mail]; [e-mail]
>>> Subject: Re: [PATCH 07/11] dt-bindings: spi: spi-nxp-fspi: add a new
>>> property nxp,fspi-dll-slvdly
>>>
>>> On 05/07/2022 11:11, [e-mail] wrote:
>>>> From: Haibo Chen <[e-mail]>
>>>>
>>>> Add one optional property nxp,fspi-dll-slvdly
>>>>
>>>> Signed-off-by: Haibo Chen <[e-mail]>
>>>> ---
>>>>  Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
>>> b/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
>>>> index 1b552c298277..6bd61565686a 100644
>>>> --- a/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
>>>> @@ -45,6 +45,12 @@ properties:
>>>>        - const: fspi_en
>>>>        - const: fspi
>>>>
>>>> +  nxp,fspi-dll-slvdly:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description: |
>>>> +      Specify the DLL slave line delay value.
>>>
>>> What are the units?
>>
>> Do you mean here need to give more detail explain about this properity?
>>
>> How about change like this?
>>    Specify the DLL slave line delay value. The delay target for slave delay line is:
>((nxp,fspi-dll-slvdly+1) * 1/32 * clock cycle of reference clock (serial root clock).
>
>This would be good.
>
>> The range of this value is 0~16.
>
>This needs to go to schema instead as "maximum: 16".
>
>But still the question is - what are the units used in this "delay"? ms? us?

HI Krzysztof,

According to the formula, the range should be 0~15, 16 should do nothing or no delay.

The unit should be clock phase. In other words, the delay can be in range of 1/32~1/2 clock cycle.


>
>Best regards,
>Krzysztof

Krzysztof Kozlowski July 5, 2022, 1:29 p.m. UTC | #5

On 05/07/2022 15:19, Han Xu wrote:
>>>>> +  nxp,fspi-dll-slvdly:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    description: |
>>>>> +      Specify the DLL slave line delay value.
>>>>
>>>> What are the units?
>>>
>>> Do you mean here need to give more detail explain about this properity?
>>>
>>> How about change like this?
>>>    Specify the DLL slave line delay value. The delay target for slave delay line is:
>> ((nxp,fspi-dll-slvdly+1) * 1/32 * clock cycle of reference clock (serial root clock).
>>
>> This would be good.
>>
>>> The range of this value is 0~16.
>>
>> This needs to go to schema instead as "maximum: 16".
>>
>> But still the question is - what are the units used in this "delay"? ms? us?
> 
> HI Krzysztof,
> 
> According to the formula, the range should be 0~15, 16 should do nothing or no delay.

Sure, just add some constraint.

> 
> The unit should be clock phase. In other words, the delay can be in range of 1/32~1/2 clock cycle.

So we probably misunderstood each other... looking at the driver it also
explains the confusing. You encoded here register value which is pretty
often wrong approach.

This should be instead meaningful value for the user of the bindings, so
usually using one of property units:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

I think you could use here clock cycles or clock phase, but then it has
to be obvious it is that unit.

Best regards,
Krzysztof

Han Xu July 5, 2022, 2 p.m. UTC | #6

>-----Original Message-----
>From: Krzysztof Kozlowski <[e-mail]>
>Sent: Tuesday, July 5, 2022 8:29 AM
>To: Han Xu <[e-mail]>; Bough Chen <[e-mail]>;
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail]
>Cc: [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>dl-linux-imx <[e-mail]>; [e-mail];
>[e-mail]
>Subject: Re: [PATCH 07/11] dt-bindings: spi: spi-nxp-fspi: add a new property
>nxp,fspi-dll-slvdly
>
>On 05/07/2022 15:19, Han Xu wrote:
>>>>>> +  nxp,fspi-dll-slvdly:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +    description: |
>>>>>> +      Specify the DLL slave line delay value.
>>>>>
>>>>> What are the units?
>>>>
>>>> Do you mean here need to give more detail explain about this properity?
>>>>
>>>> How about change like this?
>>>>    Specify the DLL slave line delay value. The delay target for slave delay line is:
>>> ((nxp,fspi-dll-slvdly+1) * 1/32 * clock cycle of reference clock (serial root clock).
>>>
>>> This would be good.
>>>
>>>> The range of this value is 0~16.
>>>
>>> This needs to go to schema instead as "maximum: 16".
>>>
>>> But still the question is - what are the units used in this "delay"? ms? us?
>>
>> HI Krzysztof,
>>
>> According to the formula, the range should be 0~15, 16 should do nothing or no
>delay.
>
>Sure, just add some constraint.
>
>>
>> The unit should be clock phase. In other words, the delay can be in range of
>1/32~1/2 clock cycle.
>
>So we probably misunderstood each other... looking at the driver it also explains
>the confusing. You encoded here register value which is pretty often wrong
>approach.
>
>This should be instead meaningful value for the user of the bindings, so usually
>using one of property units:
>https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com
>%2Fdevicetree-org%2Fdt-
>schema%2Fblob%2Fmain%2Fdtschema%2Fschemas%2Fproperty-
>units.yaml&amp;data=05%7C01%7Chan.xu%40nxp.com%7C0ffe3d706e064f14382
>108da5e8a5add%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379262
>45564450475%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
>2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Q4
>SfVnBN%2BQ0vYKJzRf%2FXZkCA1WGyPV9doFcb%2BLSKx4w%3D&amp;reserved=0
>
>I think you could use here clock cycles or clock phase, but then it has to be obvious
>it is that unit.

Hi Krzysztof, 

Let me clarify it, in the document a term "delay cell" was used to descript this register bit. Each delay cell equals "1/32 clock phase", so the unit of delay cell is clock phase. The value user need set in DT just number to define how many delay cells needed.

>
>Best regards,
>Krzysztof

Krzysztof Kozlowski July 5, 2022, 2:03 p.m. UTC | #7

On 05/07/2022 16:00, Han Xu wrote:
>> So we probably misunderstood each other... looking at the driver it also explains
>> the confusing. You encoded here register value which is pretty often wrong
>> approach.
>>
>> This should be instead meaningful value for the user of the bindings, so usually
>> using one of property units:
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com
>> %2Fdevicetree-org%2Fdt-
>> schema%2Fblob%2Fmain%2Fdtschema%2Fschemas%2Fproperty-
>> units.yaml&data=05%7C01%7Chan.xu%40nxp.com%7C0ffe3d706e064f14382
>> 108da5e8a5add%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379262
>> 45564450475%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
>> 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Q4
>> SfVnBN%2BQ0vYKJzRf%2FXZkCA1WGyPV9doFcb%2BLSKx4w%3D&reserved=0
>>
>> I think you could use here clock cycles or clock phase, but then it has to be obvious
>> it is that unit.
> 
> Hi Krzysztof, 
> 
> Let me clarify it, in the document a term "delay cell" was used to descript this register bit. 

Which document? The bindings (I cannot find it there)? Commit msg?

> Each delay cell equals "1/32 clock phase", so the unit of delay cell is clock phase. The value user need set in DT just number to define how many delay cells needed.

Your bindings did not say this at all.

Best regards,
Krzysztof

Michael Walle July 5, 2022, 2:06 p.m. UTC | #8

Am 2022-07-05 16:00, schrieb Han Xu:
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Tuesday, July 5, 2022 8:29 AM
>> To: Han Xu <[e-mail]>; Bough Chen <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; 
>> [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; 
>> [e-mail];
>> [e-mail]; [e-mail]; [e-mail]
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; 
>> [e-mail];
>> dl-linux-imx <[e-mail]>; 
>> [e-mail];
>> [e-mail]
>> Subject: Re: [PATCH 07/11] dt-bindings: spi: spi-nxp-fspi: add a new 
>> property
>> nxp,fspi-dll-slvdly
>> 
>> On 05/07/2022 15:19, Han Xu wrote:
>>>>>>> +  nxp,fspi-dll-slvdly:
>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>> +    description: |
>>>>>>> +      Specify the DLL slave line delay value.
>>>>>> 
>>>>>> What are the units?
>>>>> 
>>>>> Do you mean here need to give more detail explain about this 
>>>>> properity?
>>>>> 
>>>>> How about change like this?
>>>>>    Specify the DLL slave line delay value. The delay target for 
>>>>> slave delay line is:
>>>> ((nxp,fspi-dll-slvdly+1) * 1/32 * clock cycle of reference clock 
>>>> (serial root clock).
>>>> 
>>>> This would be good.
>>>> 
>>>>> The range of this value is 0~16.
>>>> 
>>>> This needs to go to schema instead as "maximum: 16".
>>>> 
>>>> But still the question is - what are the units used in this "delay"? 
>>>> ms? us?
>>> 
>>> HI Krzysztof,
>>> 
>>> According to the formula, the range should be 0~15, 16 should do 
>>> nothing or no
>> delay.
>> 
>> Sure, just add some constraint.
>> 
>>> 
>>> The unit should be clock phase. In other words, the delay can be in 
>>> range of
>> 1/32~1/2 clock cycle.
>> 
>> So we probably misunderstood each other... looking at the driver it 
>> also explains
>> the confusing. You encoded here register value which is pretty often 
>> wrong
>> approach.
>> 
>> This should be instead meaningful value for the user of the bindings, 
>> so usually
>> using one of property units:
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com
>> %2Fdevicetree-org%2Fdt-
>> schema%2Fblob%2Fmain%2Fdtschema%2Fschemas%2Fproperty-
>> units.yaml&amp;data=05%7C01%7Chan.xu%40nxp.com%7C0ffe3d706e064f14382
>> 108da5e8a5add%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379262
>> 45564450475%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
>> 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Q4
>> SfVnBN%2BQ0vYKJzRf%2FXZkCA1WGyPV9doFcb%2BLSKx4w%3D&amp;reserved=0

Hm, you should fix your mail server.

>> 
>> I think you could use here clock cycles or clock phase, but then it 
>> has to be obvious
>> it is that unit.
> 
> Hi Krzysztof,
> 
> Let me clarify it, in the document a term "delay cell" was used to
> descript this register bit. Each delay cell equals "1/32 clock phase",
> so the unit of delay cell is clock phase. The value user need set in
> DT just number to define how many delay cells needed.

Then should the unit be "-degrees" and the possible range 0-180?

-michael

Krzysztof Kozlowski July 5, 2022, 2:12 p.m. UTC | #9

On 05/07/2022 16:06, Michael Walle wrote:
> 
>>>
>>> I think you could use here clock cycles or clock phase, but then it 
>>> has to be obvious
>>> it is that unit.
>>
>> Hi Krzysztof,
>>
>> Let me clarify it, in the document a term "delay cell" was used to
>> descript this register bit. Each delay cell equals "1/32 clock phase",
>> so the unit of delay cell is clock phase. The value user need set in
>> DT just number to define how many delay cells needed.
> 
> Then should the unit be "-degrees" and the possible range 0-180?

Thanks. We don't have it documented currently, but the unit seems
reasonable.

Best regards,
Krzysztof

Han Xu July 5, 2022, 2:31 p.m. UTC | #10

On 22/07/05 04:03PM, Krzysztof Kozlowski wrote:
> On 05/07/2022 16:00, Han Xu wrote:
> >> So we probably misunderstood each other... looking at the driver it also explains
> >> the confusing. You encoded here register value which is pretty often wrong
> >> approach.
> >>
> >> This should be instead meaningful value for the user of the bindings, so usually
> >> using one of property units:
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F&data=05%7C01%7Chan.xu%40nxp.com%7C8b8e3e6291c24579020308da5e8f2916%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637926266207468995%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=KHgjKLX7M8CYfJoWqpVhNdZc%2FlZhZxp6CuaPTUYgwE8%3D&reserved=0
> >> %2Fdevicetree-org%2Fdt-
> >> schema%2Fblob%2Fmain%2Fdtschema%2Fschemas%2Fproperty-
> >> units.yaml&data=05%7C01%7Chan.xu%40nxp.com%7C0ffe3d706e064f14382
> >> 108da5e8a5add%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379262
> >> 45564450475%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> >> 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Q4
> >> SfVnBN%2BQ0vYKJzRf%2FXZkCA1WGyPV9doFcb%2BLSKx4w%3D&reserved=0
> >>
> >> I think you could use here clock cycles or clock phase, but then it has to be obvious
> >> it is that unit.
> > 
> > Hi Krzysztof, 
> > 
> > Let me clarify it, in the document a term "delay cell" was used to descript this register bit. 
> 
> Which document? The bindings (I cannot find it there)? Commit msg?

The SoC Reference Manual.

> 
> > Each delay cell equals "1/32 clock phase", so the unit of delay cell is clock phase. The value user need set in DT just number to define how many delay cells needed.
> 
> Your bindings did not say this at all.

I will explain all details in v2 patch.

> 
> Best regards,
> Krzysztof

Han Xu July 5, 2022, 2:52 p.m. UTC | #11

On 22/07/05 04:12PM, Krzysztof Kozlowski wrote:
> On 05/07/2022 16:06, Michael Walle wrote:
> > 
> >>>
> >>> I think you could use here clock cycles or clock phase, but then it 
> >>> has to be obvious
> >>> it is that unit.
> >>
> >> Hi Krzysztof,
> >>
> >> Let me clarify it, in the document a term "delay cell" was used to
> >> descript this register bit. Each delay cell equals "1/32 clock phase",
> >> so the unit of delay cell is clock phase. The value user need set in
> >> DT just number to define how many delay cells needed.
> > 
> > Then should the unit be "-degrees" and the possible range 0-180?
> 
> Thanks. We don't have it documented currently, but the unit seems
> reasonable.

IMO, use the unit "-degrees" makes it more complicate. Personaly I would
calculate how many clock cycle delay needed, such as 1/4 clock cycle or half
clock cycle. Using degree brings extra calculation.

The granularity of the clock phase change is 1/32 of 180 degree, but the range
0-180 make people feel it can be set in any degree in range.

If I describe all details of the relation between "nxp,fspi-dll-slvdly" and
"delay cell" in patch v2, do you think it's clear for users?

> 
> Best regards,
> Krzysztof

Michael Walle July 5, 2022, 2:58 p.m. UTC | #12

Am 2022-07-05 16:52, schrieb Han Xu:
> On 22/07/05 04:12PM, Krzysztof Kozlowski wrote:
>> On 05/07/2022 16:06, Michael Walle wrote:
>> >
>> >>>
>> >>> I think you could use here clock cycles or clock phase, but then it
>> >>> has to be obvious
>> >>> it is that unit.
>> >>
>> >> Hi Krzysztof,
>> >>
>> >> Let me clarify it, in the document a term "delay cell" was used to
>> >> descript this register bit. Each delay cell equals "1/32 clock phase",
>> >> so the unit of delay cell is clock phase. The value user need set in
>> >> DT just number to define how many delay cells needed.
>> >
>> > Then should the unit be "-degrees" and the possible range 0-180?
>> 
>> Thanks. We don't have it documented currently, but the unit seems
>> reasonable.
> 
> IMO, use the unit "-degrees" makes it more complicate. Personaly I 
> would
> calculate how many clock cycle delay needed, such as 1/4 clock cycle or 
> half
> clock cycle. Using degree brings extra calculation.

What is the extra calculation here? For hardware engineer who has to
specify this, it is easier to give the delay in clock phase (in degrees)
rather than reading the documentation and transform that into a value
given in 1/32 part of a clock, that should be part of the driver.

> The granularity of the clock phase change is 1/32 of 180 degree, but 
> the range
> 0-180 make people feel it can be set in any degree in range.

I'm not sure if the DT bindings have a granularity feature but you
could just round to the next possible value. I guess that is
the case for any value which isn't given as the raw value.

-michael

> If I describe all details of the relation between "nxp,fspi-dll-slvdly" 
> and
> "delay cell" in patch v2, do you think it's clear for users?

Mark Brown July 5, 2022, 3:07 p.m. UTC | #13

On Tue, Jul 05, 2022 at 04:58:40PM +0200, Michael Walle wrote:
> Am 2022-07-05 16:52, schrieb Han Xu:

> > IMO, use the unit "-degrees" makes it more complicate. Personaly I would
> > calculate how many clock cycle delay needed, such as 1/4 clock cycle or
> > half
> > clock cycle. Using degree brings extra calculation.

> What is the extra calculation here? For hardware engineer who has to
> specify this, it is easier to give the delay in clock phase (in degrees)
> rather than reading the documentation and transform that into a value
> given in 1/32 part of a clock, that should be part of the driver.

IME if it's a hardware engineer specifying things by the time they get
as far as a software engineer they'll often have been turned into "write
these values to these registers".

Krzysztof Kozlowski July 5, 2022, 3:38 p.m. UTC | #14

On 05/07/2022 16:52, Han Xu wrote:
> On 22/07/05 04:12PM, Krzysztof Kozlowski wrote:
>> On 05/07/2022 16:06, Michael Walle wrote:
>>>
>>>>>
>>>>> I think you could use here clock cycles or clock phase, but then it 
>>>>> has to be obvious
>>>>> it is that unit.
>>>>
>>>> Hi Krzysztof,
>>>>
>>>> Let me clarify it, in the document a term "delay cell" was used to
>>>> descript this register bit. Each delay cell equals "1/32 clock phase",
>>>> so the unit of delay cell is clock phase. The value user need set in
>>>> DT just number to define how many delay cells needed.
>>>
>>> Then should the unit be "-degrees" and the possible range 0-180?
>>
>> Thanks. We don't have it documented currently, but the unit seems
>> reasonable.
> 
> IMO, use the unit "-degrees" makes it more complicate. Personaly I would
> calculate how many clock cycle delay needed, such as 1/4 clock cycle or half
> clock cycle. Using degree brings extra calculation.

And what if the next device uses a bit different divider? Like 1/16?
This is why we have standard units so people won't push register values
into the bindings.

> 
> The granularity of the clock phase change is 1/32 of 180 degree, but the range
> 0-180 make people feel it can be set in any degree in range.

Yes, because that's how the bindings are being written - allowing any
reasonable value, not register-specific value, to be used because it is
the most flexible, hardware-independent and allows further customization
of bindings (e.g. new devices). Embedding device programming model into
the bindings contradicts it.

Second, nothing stops you from narrowing the acceptable values with an
enum. This still allows extension. Your 1/32 does not.

> 
> If I describe all details of the relation between "nxp,fspi-dll-slvdly" and
> "delay cell" in patch v2, do you think it's clear for users?

1/32 could be a nice unit, but degrees is better. Just like uV is better
than 1/32 of V. Like 1 us is better than 1/32 of ms.

Do you see  in the bindings many other values like time, potential,
current or power described in 1/32 units?

Best regards,
Krzysztof

Han Xu July 5, 2022, 3:50 p.m. UTC | #15

On 22/07/05 05:38PM, Krzysztof Kozlowski wrote:
> On 05/07/2022 16:52, Han Xu wrote:
> > On 22/07/05 04:12PM, Krzysztof Kozlowski wrote:
> >> On 05/07/2022 16:06, Michael Walle wrote:
> >>>
> >>>>>
> >>>>> I think you could use here clock cycles or clock phase, but then it 
> >>>>> has to be obvious
> >>>>> it is that unit.
> >>>>
> >>>> Hi Krzysztof,
> >>>>
> >>>> Let me clarify it, in the document a term "delay cell" was used to
> >>>> descript this register bit. Each delay cell equals "1/32 clock phase",
> >>>> so the unit of delay cell is clock phase. The value user need set in
> >>>> DT just number to define how many delay cells needed.
> >>>
> >>> Then should the unit be "-degrees" and the possible range 0-180?
> >>
> >> Thanks. We don't have it documented currently, but the unit seems
> >> reasonable.
> > 
> > IMO, use the unit "-degrees" makes it more complicate. Personaly I would
> > calculate how many clock cycle delay needed, such as 1/4 clock cycle or half
> > clock cycle. Using degree brings extra calculation.
> 
> And what if the next device uses a bit different divider? Like 1/16?
> This is why we have standard units so people won't push register values
> into the bindings.
> 
> > 
> > The granularity of the clock phase change is 1/32 of 180 degree, but the range
> > 0-180 make people feel it can be set in any degree in range.
> 
> Yes, because that's how the bindings are being written - allowing any
> reasonable value, not register-specific value, to be used because it is
> the most flexible, hardware-independent and allows further customization
> of bindings (e.g. new devices). Embedding device programming model into
> the bindings contradicts it.
> 
> Second, nothing stops you from narrowing the acceptable values with an
> enum. This still allows extension. Your 1/32 does not.
> 
> > 
> > If I describe all details of the relation between "nxp,fspi-dll-slvdly" and
> > "delay cell" in patch v2, do you think it's clear for users?
> 
> 1/32 could be a nice unit, but degrees is better. Just like uV is better
> than 1/32 of V. Like 1 us is better than 1/32 of ms.
> 
> Do you see  in the bindings many other values like time, potential,
> current or power described in 1/32 units?

That make sense. I will use degree as the unit and round to register proper
value in driver as Michael suggested. Thanks for all comments.

> 
> Best regards,
> Krzysztof

Rob Herring (Arm) July 6, 2022, 4:11 p.m. UTC | #16

On Tue, Jul 05, 2022 at 10:50:31AM -0500, Han Xu wrote:
> On 22/07/05 05:38PM, Krzysztof Kozlowski wrote:
> > On 05/07/2022 16:52, Han Xu wrote:
> > > On 22/07/05 04:12PM, Krzysztof Kozlowski wrote:
> > >> On 05/07/2022 16:06, Michael Walle wrote:
> > >>>
> > >>>>>
> > >>>>> I think you could use here clock cycles or clock phase, but then it 
> > >>>>> has to be obvious
> > >>>>> it is that unit.
> > >>>>
> > >>>> Hi Krzysztof,
> > >>>>
> > >>>> Let me clarify it, in the document a term "delay cell" was used to
> > >>>> descript this register bit. Each delay cell equals "1/32 clock phase",
> > >>>> so the unit of delay cell is clock phase. The value user need set in
> > >>>> DT just number to define how many delay cells needed.
> > >>>
> > >>> Then should the unit be "-degrees" and the possible range 0-180?
> > >>
> > >> Thanks. We don't have it documented currently, but the unit seems
> > >> reasonable.
> > > 
> > > IMO, use the unit "-degrees" makes it more complicate. Personaly I would
> > > calculate how many clock cycle delay needed, such as 1/4 clock cycle or half
> > > clock cycle. Using degree brings extra calculation.
> > 
> > And what if the next device uses a bit different divider? Like 1/16?
> > This is why we have standard units so people won't push register values
> > into the bindings.
> > 
> > > 
> > > The granularity of the clock phase change is 1/32 of 180 degree, but the range
> > > 0-180 make people feel it can be set in any degree in range.
> > 
> > Yes, because that's how the bindings are being written - allowing any
> > reasonable value, not register-specific value, to be used because it is
> > the most flexible, hardware-independent and allows further customization
> > of bindings (e.g. new devices). Embedding device programming model into
> > the bindings contradicts it.
> > 
> > Second, nothing stops you from narrowing the acceptable values with an
> > enum. This still allows extension. Your 1/32 does not.
> > 
> > > 
> > > If I describe all details of the relation between "nxp,fspi-dll-slvdly" and
> > > "delay cell" in patch v2, do you think it's clear for users?

What's a cell?

> > 
> > 1/32 could be a nice unit, but degrees is better. Just like uV is better
> > than 1/32 of V. Like 1 us is better than 1/32 of ms.
> > 
> > Do you see  in the bindings many other values like time, potential,
> > current or power described in 1/32 units?
> 
> That make sense. I will use degree as the unit and round to register proper
> value in driver as Michael suggested. Thanks for all comments.

I'm still wondering what this is delaying? From what to what? We already 
have numerous common delay properties for SPI. If one of those doesn't 
work, then should this be a new common property? And if common, I think 
time units is better to use than degrees.

If this is vendor specific, then I'd just use the register value. 
There's not much point in using common units unless it is a common 
property.

Rob

Michael Walle July 6, 2022, 8:59 p.m. UTC | #17

Am 2022-07-06 18:11, schrieb Rob Herring:
> On Tue, Jul 05, 2022 at 10:50:31AM -0500, Han Xu wrote:
>> On 22/07/05 05:38PM, Krzysztof Kozlowski wrote:
>> > On 05/07/2022 16:52, Han Xu wrote:
>> > > On 22/07/05 04:12PM, Krzysztof Kozlowski wrote:
>> > >> On 05/07/2022 16:06, Michael Walle wrote:
>> > >>>
>> > >>>>>
>> > >>>>> I think you could use here clock cycles or clock phase, but then it
>> > >>>>> has to be obvious
>> > >>>>> it is that unit.
>> > >>>>
>> > >>>> Hi Krzysztof,
>> > >>>>
>> > >>>> Let me clarify it, in the document a term "delay cell" was used to
>> > >>>> descript this register bit. Each delay cell equals "1/32 clock phase",
>> > >>>> so the unit of delay cell is clock phase. The value user need set in
>> > >>>> DT just number to define how many delay cells needed.
>> > >>>
>> > >>> Then should the unit be "-degrees" and the possible range 0-180?
>> > >>
>> > >> Thanks. We don't have it documented currently, but the unit seems
>> > >> reasonable.
>> > >
>> > > IMO, use the unit "-degrees" makes it more complicate. Personaly I would
>> > > calculate how many clock cycle delay needed, such as 1/4 clock cycle or half
>> > > clock cycle. Using degree brings extra calculation.
>> >
>> > And what if the next device uses a bit different divider? Like 1/16?
>> > This is why we have standard units so people won't push register values
>> > into the bindings.
>> >
>> > >
>> > > The granularity of the clock phase change is 1/32 of 180 degree, but the range
>> > > 0-180 make people feel it can be set in any degree in range.
>> >
>> > Yes, because that's how the bindings are being written - allowing any
>> > reasonable value, not register-specific value, to be used because it is
>> > the most flexible, hardware-independent and allows further customization
>> > of bindings (e.g. new devices). Embedding device programming model into
>> > the bindings contradicts it.
>> >
>> > Second, nothing stops you from narrowing the acceptable values with an
>> > enum. This still allows extension. Your 1/32 does not.
>> >
>> > >
>> > > If I describe all details of the relation between "nxp,fspi-dll-slvdly" and
>> > > "delay cell" in patch v2, do you think it's clear for users?
> 
> What's a cell?

A delay cell I presume. Which if I read the datasheet correctly is
somewhere between 75ps and 225ps per cell *in an unlocked state*.

I don't understand what the intention of this setting in the device
tree is. I does *not* specify the delay of the DLL, rather it specifies
the target value to be achieved by the DLL. And the RM tells the
recommended value is 0xf. Which makes sense, because it's half a clock
cycle and you'd want to sample in the middle of the clock (note this is
double data rate and you are sampling on falling and rising edge). But
and this is the catch I think, the DLL will only lock if the frequency
is >100MHz. Now if the frequency is lower than 100MHz you can actually
set the value manually (see above), but this is not what the driver
does. You'd need to write the manual value into OVRDVAL, not into
SLVDLYTARGET.

So I'm confused. Why can't you just set SLVDLYTARGET to 0xf if the
frequency is larger than 100MHz? How is this supposed to be used?
Do I need to set the value to 0xf if the frequency is higher than
100MHz? I see you used 4 in your device tree, why don't you use
the recommended value?

>> > 1/32 could be a nice unit, but degrees is better. Just like uV is better
>> > than 1/32 of V. Like 1 us is better than 1/32 of ms.
>> >
>> > Do you see  in the bindings many other values like time, potential,
>> > current or power described in 1/32 units?
>> 
>> That make sense. I will use degree as the unit and round to register 
>> proper
>> value in driver as Michael suggested. Thanks for all comments.
> 
> I'm still wondering what this is delaying? From what to what? We 
> already
> have numerous common delay properties for SPI. If one of those doesn't
> work, then should this be a new common property? And if common, I think
> time units is better to use than degrees.

It's delaying the internal sampling clock with respect to the
internal clock (which also drivers SCK then). With that clock
the data written by the flash device is then sampled. Now there
is also the read strobe; I'm unsure if that can also be delayed.

Time units depend on the frequency (or changes with the frequency),
whereas the phase angle doesn't.

> If this is vendor specific, then I'd just use the register value.
> There's not much point in using common units unless it is a common
> property.

-michael

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml b/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
index 1b552c298277..6bd61565686a 100644
--- a/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-nxp-fspi.yaml
@@ -45,6 +45,12 @@  properties:
       - const: fspi_en
       - const: fspi
 
+  nxp,fspi-dll-slvdly:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Specify the DLL slave line delay value.
+    default: 0
+
 required:
   - compatible
   - reg