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

[Daniel Golle]

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


> 
> > 
> >>
> >>>
> >>> 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.

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?)

> 
> > 
> > Or should the property itself be moved into yet another array of
> > booleans which should be added in the node describing the ethernet
> > controller and referencing these sgmiisys syscons using phandles?
> 
> Best regards,
> Krzysztof
>