[v2] dt-bindings: net: nxp,sja1105: document spi-cpol/cpha

  Some boards use SJA1105 Ethernet Switch with SPI CPOL and CPHA, so
document this to fix dtbs_check warnings:

  arch/arm64/boot/dts/freescale/fsl-lx2160a-bluebox3.dtb: ethernet-switch@0: Unevaluated properties are not allowed ('spi-cpol' was unexpected)

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Changes since v1:
1. Add also cpha
---
 Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml | 3 +++
 1 file changed, 3 insertions(+)
  

Hi Krzysztof,

On Wed, Nov 02, 2022 at 02:52:32PM -0400, Krzysztof Kozlowski wrote:
> Some boards use SJA1105 Ethernet Switch with SPI CPOL and CPHA, so
> document this to fix dtbs_check warnings:
> 
>   arch/arm64/boot/dts/freescale/fsl-lx2160a-bluebox3.dtb: ethernet-switch@0: Unevaluated properties are not allowed ('spi-cpol' was unexpected)
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---
> 
> Changes since v1:
> 1. Add also cpha
> ---
>  Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> index 1e26d876d146..3debbf0f3789 100644
> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> @@ -36,6 +36,9 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  spi-cpha: true
> +  spi-cpol: true
> +
>    # Optional container node for the 2 internal MDIO buses of the SJA1110
>    # (one for the internal 100base-T1 PHYs and the other for the single
>    # 100base-TX PHY). The "reg" property does not have physical significance.
> -- 
> 2.34.1
> 

Don't these belong to spi-peripheral-props.yaml?
  

On 03/11/2022 19:33, Vladimir Oltean wrote:
> Hi Krzysztof,
> 
> On Wed, Nov 02, 2022 at 02:52:32PM -0400, Krzysztof Kozlowski wrote:
>> Some boards use SJA1105 Ethernet Switch with SPI CPOL and CPHA, so
>> document this to fix dtbs_check warnings:
>>
>>   arch/arm64/boot/dts/freescale/fsl-lx2160a-bluebox3.dtb: ethernet-switch@0: Unevaluated properties are not allowed ('spi-cpol' was unexpected)
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> ---
>>
>> Changes since v1:
>> 1. Add also cpha
>> ---
>>  Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> index 1e26d876d146..3debbf0f3789 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>> @@ -36,6 +36,9 @@ properties:
>>    reg:
>>      maxItems: 1
>>  
>> +  spi-cpha: true
>> +  spi-cpol: true
>> +
>>    # Optional container node for the 2 internal MDIO buses of the SJA1110
>>    # (one for the internal 100base-T1 PHYs and the other for the single
>>    # 100base-TX PHY). The "reg" property does not have physical significance.
>> -- 
>> 2.34.1
>>
> 
> Don't these belong to spi-peripheral-props.yaml?

No, they are device specific, not controller specific. Every device
requiring them must explicitly include them.

See:
https://lore.kernel.org/all/20220816124321.67817-1-krzysztof.kozlowski@linaro.org/

Best regards,
Krzysztof
  

On Thu, Nov 03, 2022 at 09:44:36PM -0400, Krzysztof Kozlowski wrote:
> > Don't these belong to spi-peripheral-props.yaml?
> 
> No, they are device specific, not controller specific. Every device
> requiring them must explicitly include them.
> 
> See:
> https://lore.kernel.org/all/20220816124321.67817-1-krzysztof.kozlowski@linaro.org/
> 
> Best regards,
> Krzysztof
> 

I think you really mean to link to:
https://lore.kernel.org/all/20220718220012.GA3625497-robh@kernel.org/

oh and btw, doesn't that mean that the patch is missing
Fixes: 233363aba72a ("spi/panel: dt-bindings: drop CPHA and CPOL from common properties")
?

but I'm not sure I understand the reasoning? I mean, from the
perspective of the common schema, isn't it valid to put "spi-cpha" on a
SPI peripheral OF node even if the hardware doesn't support it, in the
same way that it's valid to put spi-max-frequency = 1 GHz even if the
hardware doesn't support it? Or maybe I'm missing the point of
spi-peripheral-props.yaml entirely? Since when is stacked-memories/
parallel-memories something that should be accepted by all schemas of
all SPI peripherals (for example here, an Ethernet switch)?
I think that spi-cpha/spi-cpol belongs to spi-peripheral-props.yaml just
as much as the others do.

The distinction "device specific, not controller specific" is arbitrary
to me. These are settings that the controller has to make in order to
talk to that specific peripheral. Same as many others in that file.
  

On 03/11/2022 22:03, Vladimir Oltean wrote:
> On Thu, Nov 03, 2022 at 09:44:36PM -0400, Krzysztof Kozlowski wrote:
>>> Don't these belong to spi-peripheral-props.yaml?
>>
>> No, they are device specific, not controller specific. Every device
>> requiring them must explicitly include them.
>>
>> See:
>> https://lore.kernel.org/all/20220816124321.67817-1-krzysztof.kozlowski@linaro.org/
>>
>> Best regards,
>> Krzysztof
>>
> 
> I think you really mean to link to:
> https://lore.kernel.org/all/20220718220012.GA3625497-robh@kernel.org/
> 
> oh and btw, doesn't that mean that the patch is missing
> Fixes: 233363aba72a ("spi/panel: dt-bindings: drop CPHA and CPOL from common properties")
> ?
> 
> but I'm not sure I understand the reasoning? I mean, from the
> perspective of the common schema, isn't it valid to put "spi-cpha" on a
> SPI peripheral OF node even if the hardware doesn't support it, in the
> same way that it's valid to put spi-max-frequency = 1 GHz even if the

It is not valid to put spi-max-frequency = 1 GHz in
spi-peripheral-props.yaml.

> hardware doesn't support it? Or maybe I'm missing the point of
> spi-peripheral-props.yaml entirely? Since when is stacked-memories/
> parallel-memories something that should be accepted by all schemas of
> all SPI peripherals (for example here, an Ethernet switch)?

Since we discussed it last time.  What is not clear in Rob's response?
He nicely explained the purpose of spi-peripheral-props.yaml.

> I think that spi-cpha/spi-cpol belongs to spi-peripheral-props.yaml just
> as much as the others do.
> 
> The distinction "device specific, not controller specific" is arbitrary
> to me. These are settings that the controller has to make in order to
> talk to that specific peripheral. Same as many others in that file.

Not every fruit is an orange, but every orange is a fruit. You do not
put "color: orange" to schema for fruits. You put it to the schema for
oranges.

IOW, CPHA/CPOL are not valid for most devices, so they cannot be in
spi-peripheral-props.yaml.

Best regards,
Krzysztof
  

On Fri, Nov 04, 2022 at 09:09:02AM -0400, Krzysztof Kozlowski wrote:
> > I think that spi-cpha/spi-cpol belongs to spi-peripheral-props.yaml just
> > as much as the others do.
> > 
> > The distinction "device specific, not controller specific" is arbitrary
> > to me. These are settings that the controller has to make in order to
> > talk to that specific peripheral. Same as many others in that file.
> 
> Not every fruit is an orange, but every orange is a fruit. You do not
> put "color: orange" to schema for fruits. You put it to the schema for
> oranges.
> 
> IOW, CPHA/CPOL are not valid for most devices, so they cannot be in
> spi-peripheral-props.yaml.

Ok, then this patch is not correct either. The "nxp,sja1105*" devices
need to have only "spi-cpha", and the "nxp,sja1110*" devices need to
have only "spi-cpol".
  

On Fri, Nov 04, 2022 at 09:09:02AM -0400, Krzysztof Kozlowski wrote:
> It is not valid to put spi-max-frequency = 1 GHz in
> spi-peripheral-props.yaml.
...
> IOW, CPHA/CPOL are not valid for most devices, so they cannot be in
> spi-peripheral-props.yaml.

Your understanding of SPI clock polarity/phase is probably not the same
as mine. "Not valid for most devices" is a gross misrepresentation.
There are 4 electrical modes of communication between a SPI controller
and a peripheral, formed by the 0/1 combination of the CPOL and CPHA bits.
Some peripherals support only a subset of these modes of operation, that
is completely true and I agree with it. But they're still SPI devices,
and all 4 modes of communication apply to them all. That's why I made
the comparison with the 1 GHz frequency. The spi-peripheral-props.yaml
schema only says what properties are valid for a peripheral, and both
CPOL and CPHA are valid for all SPI peripherals, even if some combos
don't work (when neither spi-cpol nor spi-cpha is present, they are 0
and 0, so the connection works in SPI mode 0).
  

On 04/11/2022 12:52, Vladimir Oltean wrote:
> On Fri, Nov 04, 2022 at 09:09:02AM -0400, Krzysztof Kozlowski wrote:
>>> I think that spi-cpha/spi-cpol belongs to spi-peripheral-props.yaml just
>>> as much as the others do.
>>>
>>> The distinction "device specific, not controller specific" is arbitrary
>>> to me. These are settings that the controller has to make in order to
>>> talk to that specific peripheral. Same as many others in that file.
>>
>> Not every fruit is an orange, but every orange is a fruit. You do not
>> put "color: orange" to schema for fruits. You put it to the schema for
>> oranges.
>>
>> IOW, CPHA/CPOL are not valid for most devices, so they cannot be in
>> spi-peripheral-props.yaml.
> 
> Ok, then this patch is not correct either. The "nxp,sja1105*" devices
> need to have only "spi-cpha", and the "nxp,sja1110*" devices need to
> have only "spi-cpol".

Sure, I'll add allOf:if:then based on your input.

Best regards,
Krzysztof
  

On Fri, Nov 04, 2022 at 01:13:34PM -0400, Krzysztof Kozlowski wrote:
> On 04/11/2022 12:52, Vladimir Oltean wrote:
> > Ok, then this patch is not correct either. The "nxp,sja1105*" devices
> > need to have only "spi-cpha", and the "nxp,sja1110*" devices need to
> > have only "spi-cpol".
> 
> Sure, I'll add allOf:if:then based on your input.

No, actually my input is that removing such core properties as spi-cpol/
spi-cpha from spi-peripheral-props.yaml challenges the whole purpose of
that schema fragment.

I can go back at it and complain all day that my peripheral doesn't need
spi-cs-high, spi-lsb-first, spi-rx-bus-width, spi-tx-bus-width,
stacked-memories, parallel-memories and what not. Or that the SJA1105
switch will never need the properties of nvidia,tegra210-quad-peripheral-props.yaml#,
because the former speaks SPI and the latter speaks QSPI (for flashes).
By this logic, eventually that schema will be reduced to nothing.

Yet I don't believe that including just the intersection of properties
that actually lead to functional hardware for all peripherals was the
*intention* of that schema. Just the properties which are semantically
valid, and cpol/cpha are absolutely semantically valid.
The justification that cpol/cpha are "not valid" for some peripherals
(or correctly said, those peripherals only work in mode 0) is very weak
to begin with, and restricting the SPI modes to only those that
physically work should IMO be the duty of the hardware schema and not
the common schema. The common schema just provides the type and
description, the hardware gives the valid ranges.