Re: [PATCH 1/4] dt-bindings: net: qcom,ethqos: Convert bindings to yaml

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 12/09/2022 19:28, Bhupesh Sharma wrote:
> Hi Krzysztof,
> 
> Thanks for your comments.
> 
> On 9/8/22 8:08 PM, Krzysztof Kozlowski wrote:
>> On 07/09/2022 22:49, Bhupesh Sharma wrote:
>>> Convert Qualcomm ETHQOS Ethernet devicetree binding to YAML.
>>>
>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>> Cc: Rob Herring <robh@kernel.org>
>>> Cc: Vinod Koul <vkoul@kernel.org>
>>> Cc: David Miller <davem@davemloft.net>
>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> ---
>>>   .../devicetree/bindings/net/qcom,ethqos.txt   |  66 ---------
>>>   .../devicetree/bindings/net/qcom,ethqos.yaml  | 139 ++++++++++++++++++
>>
>> You need to update maintainers - old path.
> 
> Sure, my bad. Will do in v2.
> 
>>>   2 files changed, 139 insertions(+), 66 deletions(-)
>>>   delete mode 100644 Documentation/devicetree/bindings/net/qcom,ethqos.txt
>>>   create mode 100644 Documentation/devicetree/bindings/net/qcom,ethqos.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.txt b/Documentation/devicetree/bindings/net/qcom,ethqos.txt
>>> deleted file mode 100644
>>> index 1f5746849a71..000000000000
>>> --- a/Documentation/devicetree/bindings/net/qcom,ethqos.txt
>>> +++ /dev/null
>>> @@ -1,66 +0,0 @@
>>> -Qualcomm Ethernet ETHQOS device
>>> -
>>> -This documents dwmmac based ethernet device which supports Gigabit
>>> -ethernet for version v2.3.0 onwards.
>>> -
>>> -This device has following properties:
>>> -
>>> -Required properties:
>>> -
>>> -- compatible: Should be one of:
>>> -		"qcom,qcs404-ethqos"
>>> -		"qcom,sm8150-ethqos"
>>> -
>>> -- reg: Address and length of the register set for the device
>>> -
>>> -- reg-names: Should contain register names "stmmaceth", "rgmii"
>>> -
>>> -- clocks: Should contain phandle to clocks
>>> -
>>> -- clock-names: Should contain clock names "stmmaceth", "pclk",
>>> -		"ptp_ref", "rgmii"
>>> -
>>> -- interrupts: Should contain phandle to interrupts
>>> -
>>> -- interrupt-names: Should contain interrupt names "macirq", "eth_lpi"
>>> -
>>> -Rest of the properties are defined in stmmac.txt file in same directory
>>> -
>>> -
>>> -Example:
>>> -
>>> -ethernet: ethernet@7a80000 {
>>> -	compatible = "qcom,qcs404-ethqos";
>>> -	reg = <0x07a80000 0x10000>,
>>> -		<0x07a96000 0x100>;
>>> -	reg-names = "stmmaceth", "rgmii";
>>> -	clock-names = "stmmaceth", "pclk", "ptp_ref", "rgmii";
>>> -	clocks = <&gcc GCC_ETH_AXI_CLK>,
>>> -		<&gcc GCC_ETH_SLAVE_AHB_CLK>,
>>> -		<&gcc GCC_ETH_PTP_CLK>,
>>> -		<&gcc GCC_ETH_RGMII_CLK>;
>>> -	interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
>>> -			<GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
>>> -	interrupt-names = "macirq", "eth_lpi";
>>> -	snps,reset-gpio = <&tlmm 60 GPIO_ACTIVE_LOW>;
>>> -	snps,reset-active-low;
>>> -
>>> -	snps,txpbl = <8>;
>>> -	snps,rxpbl = <2>;
>>> -	snps,aal;
>>> -	snps,tso;
>>> -
>>> -	phy-handle = <&phy1>;
>>> -	phy-mode = "rgmii";
>>> -
>>> -	mdio {
>>> -		#address-cells = <0x1>;
>>> -		#size-cells = <0x0>;
>>> -		compatible = "snps,dwmac-mdio";
>>> -		phy1: phy@4 {
>>> -			device_type = "ethernet-phy";
>>> -			reg = <0x4>;
>>> -		};
>>> -	};
>>> -
>>> -};
>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
>>> new file mode 100644
>>> index 000000000000..f05df9b0d106
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
>>> @@ -0,0 +1,139 @@
>>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/qcom,ethqos.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm Ethernet ETHQOS device
>>> +
>>> +maintainers:
>>> +  - Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>> +
>>> +description:
>>> +  This binding describes the dwmmac based Qualcomm ethernet devices which
>>> +  support Gigabit ethernet (version v2.3.0 onwards).
>>> +
>>> +  So, this file documents platform glue layer for dwmmac stmmac based Qualcomm
>>> +  ethernet devices.
>>> +
>>> +allOf:
>>> +  - $ref: "snps,dwmac.yaml#"
>>
>> No need for quotes.
> 
> Ok.
> 
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - qcom,qcs404-ethqos
>>> +      - qcom,sm8150-ethqos
>>> +
>>> +  reg: true
>>
>> I think both devices use two reg spaces.
> 
> On this platform the two reg spaces are 64-bit, whereas for other
> platforms based on dwmmac, for e.g. stm32 have 32-bit address space.

Then for this platform this should be made specific/constrained, so it
must be two items.

> 
>>> +
>>> +  reg-names:
>>> +    minItems: 1
>>
>> Why allowing only one item?
> 
> Ok, let me remove this in v2.

And then as well you allow only one item... This should be specific. If
not - why?

> 
>>> +    items:
>>> +      - const: stmmaceth
>>> +      - const: rgmii
>>> +
>>> +  interrupts: true
>>
>> This should be specific/fixed.
>>
>>> +
>>> +  interrupt-names: true
>>
>> This should be specific/fixed.
> 
> These are same as in $ref: "snps,dwmac.yaml#", so
> do we really need to specify them here? I remember on the sdhci-msm
> YAML patch review, Rob mentioned that we should just set the property to 
> true, in such cases.

But it is not specific in dwmac.yaml. You use "xxx: true" when you want
to accept property from other schema, assuming it is defined there
properly. However the snps,dwmac does not define it in specific way
because it expects specific implementation to narrow the details.

> 
> Am I missing something here?
> 
>>> +
>>> +  clocks:
>>> +    minItems: 1
>>> +    maxItems: 4
>>
>> Why such flexibility?
> 
> Ok, let me just keep 'maxItems: 4' here for now.
> 
>>> +
>>> +  clock-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: stmmaceth
>>> +      - const: pclk
>>> +      - const: ptp_ref
>>> +      - const: rgmii
>>> +
>>> +  iommus:
>>> +    minItems: 1
>>> +    maxItems: 2
>>
>> Aren't we using only one MMU?
> 
> It was just for future compatibility, but I get your point.
> Let me keep the 'maxItems: 1' here for now.
> 
>>> +
>>> +  mdio: true
>>> +
>>> +  phy-handle: true
>>> +
>>> +  phy-mode: true
>>> +
>>> +  snps,reset-gpio: true
>>> +
>>> +  snps,tso:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description:
>>> +      Enables the TSO feature otherwise it will be managed by MAC HW capability register.
>>> +
>>> +  power-domains: true
>>> +
>>> +  resets: true
>>> +
>>> +  rx-fifo-depth: true
>>> +
>>> +  tx-fifo-depth: true
>>
>> You do not list all these properties, because you use
>> unevaluatedProperties. Drop all of these "xxx :true".
> 
> Same query as above. May be I am missing something here.

You do not list any properties:true from other schema, if you use
unevaluatedProperties:false.


Best regards,
Krzysztof