Re: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property

[Krzysztof Kozlowski <krzk@kernel.org>]

On 08/02/2023 23:30, Daniel Golle wrote:
> Hi Krzysztof,
> 
> thank you for taking the time to review and explain.
> 
> On Wed, Feb 08, 2023 at 09:08:40PM +0100, Krzysztof Kozlowski wrote:
>> On 08/02/2023 14:51, Daniel Golle wrote:
>>> On Wed, Feb 08, 2023 at 10:32:53AM +0100, Krzysztof Kozlowski wrote:
>>>> On 07/02/2023 19:00, Daniel Golle wrote:
>>>>> ...
>>>>>> 3. Does not look like property of this node. This is a clock controller
>>>>>> or system controller, not SGMII/phy etc.
>>>>>
>>>>> The register range referred to by this node *does* represent also an
>>>>> SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and
>>>>> are referenced in the node of the Ethernet core, and then used by
>>>>> drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap.
>>>>> (This is the current situation already, and not related to the patchset
>>>>> now adding only a new property to support hardware which needs that)
>>>>
>>>> Just because a register is located in syscon block, does not mean that
>>>> SGMII configuration is a property of this device.
>>>
>>> It's not just one register, the whole SGMII PCS is located in those
>>> mediatek,sgmiisys syscon nodes.
>>
>> Then maybe this should be a PCS PHY device instead of adding properties
>> unrelated to clock/system controller? I don't know, currently this
>> binding says it is a provider of clocks...
> 
> As in reality it is really a clock provider and also SGMII PCS at the
> same time, maybe we should just update the description of the binding
> to match that:
> 
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> index d2c24c2775141..db6f75df200ba 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> @@ -2,6 +2,7 @@ MediaTek SGMIISYS controller
>  ============================
>  
>  The MediaTek SGMIISYS controller provides various clocks to the system.
> +It also represents the SGMII PCS used by the Ethernet core.
>  
>  Required Properties:
>  
> 
> See
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/mediatek/mtk_sgmii.c#n179
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt7986a.dtsi#n409

But it is not used as phy or PCS. It is used as syscon. If it was a phy
or PCS, then the property probably belonged here, but bindings and Linux
implementation were created in different way, so it is not a phy or PCS,
just a syscon.

>>>>> So: Should I introduce a new binding for the same compatible strings
>>>>> related to the SGMII PHY features? Or is it fine in this case to add
>>>>> this property to the existing binding?
>>>>
>>>> The user of syscon should configure it. I don't think you need new
>>>> binding. You just have to update the user of this syscon.
>>>
>>> Excuse my confusion, but it's still not entirely clear to me.
>>> So in this case I should add the description of the added propterty of
>>> the individual SGMII units (there can be more than one) to
>>> Documentation/devicetree/bindings/net/mediatek,net.yaml
>>> eventhough the properties are in the sgmiisys syscon nodes?
>>
>> I guess the property should be in the node representing the SGMII. You
>> add it now to the clock (or system) controller, so it does not look
>> right. It's not a property of a clock controller.
> 
> Well maybe this node needs to be split then into one node representing
> only the clock controller and another node representing the SGMII PCS?
> I'm not sure if this is even possible, some registers in this range
> represent clocks, other registers are accessed using regmap API in
> mtk_sgmii.c.

This can be the same node, but it must be used like PCS. syscon phandle
for getting regmap is something entirely else.

> 
> And (see the rest of this series) the exact same SGMII PCS can also be
> found in MT7531 switch IC which has it's own (a bit odd) way to access
> 32-bit registers over MDIO, also in this case it is simply not easily
> possible to represent the SGMII PCS in device tree.
> 
>>
>> Now which node should have this property depends on your devices - which
>> I have no clue about, I read what is in the bindings.
> 
> There isn't any other node exclusively representing the SGMII PCS.
> I guess the only other option would be to move the property to the
> Ethernet controller node, which imho complicates things as it is
> really a property of an individual SGMII PHY (of which there can be
> more than one).
> 
>>
>>>
>>> If so I will have to figure out how to describe properties of other
>>> nodes in the binding of the node referencing them. Are there any
>>> good examples for that?
>>
>> phys and pcs'es?
> 
> Hm, none of the current PCS (or PHY) drivers are represented by a
> syscon node... (and maybe that's the mistake in first place?)

Yes.


Best regards,
Krzysztof