| Message ID | 20221102185232.131168-1-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] dt-bindings: net: nxp,sja1105: document spi-cpol/cpha | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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.
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.
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(+)