Add documentation for the newly introduced 'mediatek,pn_swap' property
to mediatek,sgmiisys.txt.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt    | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
index d2c24c277514..b38dd0fde21d 100644
--- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
@@ -14,6 +14,10 @@ Required Properties:
 	- "mediatek,mt7986-sgmiisys_1", "syscon"
 - #clock-cells: Must be 1
 
+Optional Properties:
+
+- mediatek,pn_swap: Invert polarity of the SGMII data lanes.
+
 The SGMIISYS controller uses the common clk binding from
 Documentation/devicetree/bindings/clock/clock-bindings.txt
 The available clocks are defined in dt-bindings/clock/mt*-clk.h.
-- 
2.39.1

On 07/02/2023 15:19, Daniel Golle wrote:
> Add documentation for the newly introduced 'mediatek,pn_swap' property
> to mediatek,sgmiisys.txt.
> 

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt    | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> index d2c24c277514..b38dd0fde21d 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> @@ -14,6 +14,10 @@ Required Properties:
>  	- "mediatek,mt7986-sgmiisys_1", "syscon"
>  - #clock-cells: Must be 1
>  
> +Optional Properties:
> +
> +- mediatek,pn_swap: Invert polarity of the SGMII data lanes.

No:
1. No new properties for TXT bindings,
2. Underscore is not allowed.
3. Does not look like property of this node. This is a clock controller
or system controller, not SGMII/phy etc.

Best regards,
Krzysztof

On Tue, Feb 07, 2023 at 06:38:23PM +0100, Krzysztof Kozlowski wrote:
> On 07/02/2023 15:19, Daniel Golle wrote:
> > +Optional Properties:
> > +
> > +- mediatek,pn_swap: Invert polarity of the SGMII data lanes.
> 
> No:
> 3. Does not look like property of this node. This is a clock controller
> or system controller, not SGMII/phy etc.

Wrong. Totally wrong.

SGMII data lines consist of a "positive" and a "negative" signal.
Clearly, this PHY supports swapping the sense of those.

This CLEARLY has nothing to do with a clock controller. It's also not
system-controller related either.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Hi Krzysztof,

On Tue, Feb 07, 2023 at 06:38:23PM +0100, Krzysztof Kozlowski wrote:
> On 07/02/2023 15:19, Daniel Golle wrote:
> > Add documentation for the newly introduced 'mediatek,pn_swap' property
> > to mediatek,sgmiisys.txt.
> > 
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt    | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> > index d2c24c277514..b38dd0fde21d 100644
> > --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> > @@ -14,6 +14,10 @@ Required Properties:
> >  	- "mediatek,mt7986-sgmiisys_1", "syscon"
> >  - #clock-cells: Must be 1
> >  
> > +Optional Properties:
> > +
> > +- mediatek,pn_swap: Invert polarity of the SGMII data lanes.
> 
> No:
> 1. No new properties for TXT bindings,

So I'll have to convert the bindings to YAML, right?

> 2. Underscore is not allowed.

Ack, will change the name of the property.

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

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?

Thank you in advance for your advise!


Daniel

On 07/02/2023 19:00, Daniel Golle wrote:
> Hi Krzysztof,
> 
> On Tue, Feb 07, 2023 at 06:38:23PM +0100, Krzysztof Kozlowski wrote:
>> On 07/02/2023 15:19, Daniel Golle wrote:
>>> Add documentation for the newly introduced 'mediatek,pn_swap' property
>>> to mediatek,sgmiisys.txt.
>>>
>>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC.  It might happen, that command when run on an older
>> kernel, gives you outdated entries.  Therefore please be sure you base
>> your patches on recent Linux kernel.
>>
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>> ---
>>>  .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt    | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
>>> index d2c24c277514..b38dd0fde21d 100644
>>> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
>>> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
>>> @@ -14,6 +14,10 @@ Required Properties:
>>>  	- "mediatek,mt7986-sgmiisys_1", "syscon"
>>>  - #clock-cells: Must be 1
>>>  
>>> +Optional Properties:
>>> +
>>> +- mediatek,pn_swap: Invert polarity of the SGMII data lanes.
>>
>> No:
>> 1. No new properties for TXT bindings,
> 
> So I'll have to convert the bindings to YAML, right?

Yes, please.

> 
>> 2. Underscore is not allowed.
> 
> Ack, will change the name of the property.
> 
>> 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.

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

Best regards,
Krzysztof

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.

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

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?

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?

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

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

Now which node should have this property depends on your devices - which
I have no clue about, I read what is in the bindings.

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

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

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
>

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

On Thu, Feb 09, 2023 at 12:30:27PM +0100, Krzysztof Kozlowski wrote:
> On 08/02/2023 23:30, Daniel Golle wrote:
> > 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.

Nos, it isn't.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

On Fri, Feb 10, 2023 at 10:34:17AM +0000, Russell King (Oracle) wrote:
> On Thu, Feb 09, 2023 at 12:30:27PM +0100, Krzysztof Kozlowski wrote:
> > On 08/02/2023 23:30, Daniel Golle wrote:
> > > 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.
> 
> Nos, it isn't.

To expand on this - I have no idea why you consider it a mistake that
apparently all PCS aren't represented by a syscon node.

PCS is a sub-block in an ethernet system, just the same as a MAC is a
sub-block. PCS can appear in several locations of an ethernet system,
but are generally found either side of a serial ethernet link such
as 1000base-X, SGMII, USXGMII, 10Gbase-R etc.

So, one can find PCS within an ethernet PHY - and there may be one
facing the MAC connection, and there will be another facing the media.
We generally do not need to separate these PCS from the PHY itself
because we view the PHY as one whole device.

The optional PCS on the MAC side of the link is something that we
need to know about, because this has to be configured to talk to the
PHY, or to configure and obtain negotiation results from in the case of
fibre links.

PCS on the MAC side are not a system level device, they are very much a
specific piece of ethernet hardware in the same way that the MAC is,
and we don't represent the MAC as a syscon node. There is no reason
to do so with PCS.

These PCS on the MAC side tend to be accessed via direct MMIO accesses,
or over a MDIO bus.

There's other blocks in the IEEE 802.3 ethernet layering, such as the
PMA/PMD module (which for the MAC side we tend to model with the
drivers/phy layer) - but again, these also appear in ethernet PHYs
in order to produce the electrical signals for e.g. twisted pair
ethernet.

So, to effectively state that you consider that PCS should always be
represented as a syscon node is rather naieve, and really as a DT
reviewer you should not be making such decisions, but soliciting
opinions from those who know this subject area in detail _whether_
they are some kind of system controller before making such a
decision.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

On 10/02/2023 13:23, Russell King (Oracle) wrote:
> On Fri, Feb 10, 2023 at 10:34:17AM +0000, Russell King (Oracle) wrote:
>> On Thu, Feb 09, 2023 at 12:30:27PM +0100, Krzysztof Kozlowski wrote:
>>> On 08/02/2023 23:30, Daniel Golle wrote:
>>>> 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.
>>
>> Nos, it isn't.
> 
> To expand on this - I have no idea why you consider it a mistake that
> apparently all PCS aren't represented by a syscon node.
> 
> PCS is a sub-block in an ethernet system, just the same as a MAC is a
> sub-block. PCS can appear in several locations of an ethernet system,
> but are generally found either side of a serial ethernet link such
> as 1000base-X, SGMII, USXGMII, 10Gbase-R etc.
> 
> So, one can find PCS within an ethernet PHY - and there may be one
> facing the MAC connection, and there will be another facing the media.
> We generally do not need to separate these PCS from the PHY itself
> because we view the PHY as one whole device.
> 
> The optional PCS on the MAC side of the link is something that we
> need to know about, because this has to be configured to talk to the
> PHY, or to configure and obtain negotiation results from in the case of
> fibre links.
> 
> PCS on the MAC side are not a system level device, they are very much a
> specific piece of ethernet hardware in the same way that the MAC is,
> and we don't represent the MAC as a syscon node. There is no reason
> to do so with PCS.
> 
> These PCS on the MAC side tend to be accessed via direct MMIO accesses,
> or over a MDIO bus.
> 
> There's other blocks in the IEEE 802.3 ethernet layering, such as the
> PMA/PMD module (which for the MAC side we tend to model with the
> drivers/phy layer) - but again, these also appear in ethernet PHYs
> in order to produce the electrical signals for e.g. twisted pair
> ethernet.
> 
> So, to effectively state that you consider that PCS should always be
> represented as a syscon node is rather naieve, and really as a DT
> reviewer you should not be making such decisions, but soliciting
> opinions from those who know this subject area in detail _whether_
> they are some kind of system controller before making such a
> decision.

Daniel switched to private emails, so unfortunately our talk is not
visible here, nevertheless thanks for the feedback. Much appreciated!

Best regards,
Krzysztof