diff mbox series

[net-next,RFC,03/14] dt-bindings: net: document ethernet PHY package nodes

Message ID 20231120135041.15259-4-ansuelsmth@gmail.com
State New
Headers show
Series None | expand

Commit Message

Christian Marangi Nov. 20, 2023, 1:50 p.m. UTC
Document ethernet PHY package nodes used to describe PHY shipped in
bundle of 4-5 PHY. These particular PHY require specific PHY in the
package for global onfiguration of the PHY package.

Example are PHY package that have some regs only in one PHY of the
package and will affect every other PHY in the package, for example
related to PHY interface mode calibration or global PHY mode selection.

The PHY package node should use the global-phys property and the
global-phy-names to define PHY in the package required by the PHY driver
for global configuration.

It's also possible to specify the property phy-mode to specify that the
PHY package sets a global PHY interface mode and every PHY of the
package requires to have the same PHY interface mode.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../bindings/net/ethernet-phy-package.yaml    | 86 +++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml

Comments

Rob Herring Nov. 21, 2023, 2:42 p.m. UTC | #1
On Mon, Nov 20, 2023 at 09:44:58PM +0100, Andrew Lunn wrote:
> On Mon, Nov 20, 2023 at 02:50:30PM +0100, Christian Marangi wrote:
> > Document ethernet PHY package nodes used to describe PHY shipped in
> > bundle of 4-5 PHY. These particular PHY require specific PHY in the
> > package for global onfiguration of the PHY package.
> > 
> > Example are PHY package that have some regs only in one PHY of the
> > package and will affect every other PHY in the package, for example
> > related to PHY interface mode calibration or global PHY mode selection.
> 
> I think you are being overly narrow here. The 'global' registers could
> be spread over multiple addresses. Particularly for a C22 PHY. I
> suppose they could even be in a N+1 address space, where there is no
> PHY at all.
> 
> Where the global registers are is specific to a PHY package
> vendor/model.

For this reason in particular, the package needs a specific compatible.

> The PHY driver should know this. All the PHY driver
> needs to know is some sort of base offset. PHY0 in this package is
> using address X. It can then use relative addressing from this base to
> access the global registers for this package.
>  
> > It's also possible to specify the property phy-mode to specify that the
> > PHY package sets a global PHY interface mode and every PHY of the
> > package requires to have the same PHY interface mode.
> 
> I don't think it is what simple. See the QCA8084 for example. 3 of the
> 4 PHYs must use QXGMII. The fourth PHY can also use QXGMII but it can
> be multiplexed to a different PMA and use 1000BaseX, SGMII or
> 2500BaseX.
> 
> I do think we need somewhere to put package properties. But i don't
> think phy-mode is such a property. At the moment, i don't have a good
> example of a package property.

What about power supplies and reset/enable lines?

Rob
Christian Marangi Nov. 22, 2023, 6:32 p.m. UTC | #2
On Tue, Nov 21, 2023 at 03:45:42PM +0100, Andrew Lunn wrote:
> > > I do think we need somewhere to put package properties. But i don't
> > > think phy-mode is such a property. At the moment, i don't have a good
> > > example of a package property.
> > 
> > What about power supplies and reset/enable lines?
> 
> Yes, good point. I can imagine some packages sharing regulators. Reset
> might also be shared, but it makes things messy to handle.
>

Sooooo.... Sorry if I insist but I would really love to have something
""stable"" to move this further. (the changes are easy enough so it's
really a matter of finding a good DT structure)

Maybe a good idea would be summarize the concern and see what solution
was proposed:

Concern list:
1. ethernet-phy-package MUST be placed in mdio node (not in ethernet,
   the example was wrong anyway) and MUST have an addr

   Current example doesn't have an addr. I would prefer this way but
   no problem in changing this.

   Solution:
     - Add reg to the ethernet-phy-package node with the base address of
       the PHY package (base address = the first PHY address of the
       package)

       We will have a PHY node with the same address of the PHY package
       node. Each PHY node in the PHY package node will have reg set to
       the REAL address in the mdio bus.

2. global-phys are redundant and can be dropped.

   They are used to facilitate and make it less obscure how the PHY
   package is described. Can totally be handled internally by the PHY
   driver. Still I would prefer to keep them as is.

   Solution:
     - Drop the thing and leave the PHY driver handle it with hardcoded
       values.
       Due to point 1, the shared struct will have the base address of
       the PHY package and will be handle to reference the global PHY at
       an offset from the base address.

3. phy-mode is problematic.

   It's an optional value to enforce a specific mode for each PHY in the
   package. For complex configuration the mode won't be defined.

   Solution:
    - Rename it to package-phy-mode to make it less confusing.

    - Add an additional function that PHY package can use to make custom
      validation on the mode for every PHY attached (in the PHY package).

      Would make it less clear but more flexible for complex
      configuration. Maybe both solution can be implemented and the
      special function is used if the mode is not defined?

4. Not finding a correct place to put PHY package info.

   I'm still convinced the mdio node is the correct place.
   - PHY package are PHY in bundle so they are actual PHY
   - We already have in the mdio node special handling (every DSA switch
     use custom compatible and PHY ID is not used to probe them
     normally)
   - Node this way won't be treated as PHY as they won't match the PHY
     node name pattern and also won't have the compatible pattern for
     PHY.

   Solution:
    - ethernet-phy-package node is OK given a reg is defined.

These are the 4 concern we have currently, hoping I didn't miss any, I
hope we can sort those so I can send a v2 and make some progress on
this.
Andrew Lunn Nov. 23, 2023, 3:30 a.m. UTC | #3
On Wed, Nov 22, 2023 at 07:32:22PM +0100, Christian Marangi wrote:
> On Tue, Nov 21, 2023 at 03:45:42PM +0100, Andrew Lunn wrote:
> > > > I do think we need somewhere to put package properties. But i don't
> > > > think phy-mode is such a property. At the moment, i don't have a good
> > > > example of a package property.
> > > 
> > > What about power supplies and reset/enable lines?
> > 
> > Yes, good point. I can imagine some packages sharing regulators. Reset
> > might also be shared, but it makes things messy to handle.
> >
> 
> Sooooo.... Sorry if I insist but I would really love to have something
> ""stable"" to move this further. (the changes are easy enough so it's
> really a matter of finding a good DT structure)
> 
> Maybe a good idea would be summarize the concern and see what solution
> was proposed:
> 
> Concern list:
> 1. ethernet-phy-package MUST be placed in mdio node (not in ethernet,
>    the example was wrong anyway) and MUST have an addr
> 
>    Current example doesn't have an addr. I would prefer this way but
>    no problem in changing this.
> 
>    Solution:
>      - Add reg to the ethernet-phy-package node with the base address of
>        the PHY package (base address = the first PHY address of the
>        package)

Yes.

>        We will have a PHY node with the same address of the PHY package
>        node. Each PHY node in the PHY package node will have reg set to
>        the REAL address in the mdio bus.

Basically Yes. I actually think the first sentence is not 100%
correct. It could be all the package configuration registers are in
the base address, without an actual PHY. The PHYs then follow at
addresses above it. I can also imagine a case where the first PHY in
the package is not used, so its not listed at all. So i think it
should be "We often have a PHY node with the same address of the PHY
package node."

> 3. phy-mode is problematic.
> 
>    It's an optional value to enforce a specific mode for each PHY in the
>    package. For complex configuration the mode won't be defined.
> 
>    Solution:
>     - Rename it to package-phy-mode to make it less confusing.
> 
>     - Add an additional function that PHY package can use to make custom
>       validation on the mode for every PHY attached (in the PHY package).
> 
>       Would make it less clear but more flexible for complex
>       configuration. Maybe both solution can be implemented and the
>       special function is used if the mode is not defined?

The description you give is that there are two SERDES, and both could
be connected to a MAC. What does package-phy-mode represent then? 

Luo Jie patch for the qca8084 seems to have the same issue. It has two
SERDES/PMA, and both could be connected to the MACs. So it seems like
QCA devices don't actually fit this model. If we want to describe the
package link mode, we probably need to list each PMA, and have a
property in the PMA node indicating what link mode the PMA is
operating at.

At least for the moment, its not clear we actually need this at
all. It seems like the phy-mode is all we need. The PHY driver should
know what values are valid per port, and so can validate the value.

> 4. Not finding a correct place to put PHY package info.
> 
>    I'm still convinced the mdio node is the correct place.
>    - PHY package are PHY in bundle so they are actual PHY
>    - We already have in the mdio node special handling (every DSA switch
>      use custom compatible and PHY ID is not used to probe them
>      normally)
>    - Node this way won't be treated as PHY as they won't match the PHY
>      node name pattern and also won't have the compatible pattern for
>      PHY.

We do need a compatible for the package. The kernel is unlikely to use
it, but the validation tools will. Each package can have its own DT
properties, so we need a .yaml to describe those properties. So i
would expect to have a "qca807x-package" compatible, which then lists
the tx driver strength etc. The PHYs within the package don't need
compatible, they are just linux PHYs, probed using the same code as
PHYs outside of a package.

    Andrew
Andrew Lunn Nov. 23, 2023, 2:57 p.m. UTC | #4
On Thu, Nov 23, 2023 at 02:35:31PM +0000, Russell King (Oracle) wrote:
> On Thu, Nov 23, 2023 at 03:27:05PM +0100, Andrew Lunn wrote:
> > > Just to be more precise qca807x can operate in 3 different mode:
> > > (this is controlled by the MODE_CFG bits)
> > 
> > > - QSGMII: 5 copper port
> > 
> > 4 slots over QSGMII, plus the second SERDES is connected to the MAC
> > using SGMII/1000BaseX?
> > 
> > > - PSGMII: 5 copper port
> > 
> > 5 slots over QSGMII, the second SERDES is idle?
> > 
> > > - PSGMII: 4 copper port + 1 combo (that can be both fiber or copper)
> > 
> > 5 slots over QSGMII, with the second SERDES connected to an SFP cage.
> > 
> > Are ports 1-4 always connected to the P/Q SGMII. Its only port 5 which
> > can use the second SERDES?
> 
> I think what would really help here is if there was an ascii table to
> describe the configurations, rather than trying to put it into words.

Yes.

And also for ipq4019. We need to merge these two threads of
conversation, since in the end they are probably the same driver, same
device tree etc.

       Andrew
Christian Marangi Nov. 23, 2023, 7:33 p.m. UTC | #5
On Thu, Nov 23, 2023 at 03:57:58PM +0100, Andrew Lunn wrote:
> On Thu, Nov 23, 2023 at 02:35:31PM +0000, Russell King (Oracle) wrote:
> > On Thu, Nov 23, 2023 at 03:27:05PM +0100, Andrew Lunn wrote:
> > > > Just to be more precise qca807x can operate in 3 different mode:
> > > > (this is controlled by the MODE_CFG bits)
> > > 
> > > > - QSGMII: 5 copper port
> > > 
> > > 4 slots over QSGMII, plus the second SERDES is connected to the MAC
> > > using SGMII/1000BaseX?
> > > 
> > > > - PSGMII: 5 copper port
> > > 
> > > 5 slots over QSGMII, the second SERDES is idle?
> > > 
> > > > - PSGMII: 4 copper port + 1 combo (that can be both fiber or copper)
> > > 
> > > 5 slots over QSGMII, with the second SERDES connected to an SFP cage.
> > > 
> > > Are ports 1-4 always connected to the P/Q SGMII. Its only port 5 which
> > > can use the second SERDES?
> > 
> > I think what would really help here is if there was an ascii table to
> > describe the configurations, rather than trying to put it into words.
> 
> Yes.
> 
> And also for ipq4019. We need to merge these two threads of
> conversation, since in the end they are probably the same driver, same
> device tree etc.
>

For everyone that missed Robert response in patch 12 let me quote him
also here.

"
Hi Andrew,
I think that the description is confusing.
QCA807x supports 3 different modes:
1. PSGMII (5 copper ports)
2. PSGMII (4 copper ports + 1 combo port)
3. QSGMII+SGMII

So, in case option 2 is selected then the combo port can also be used for
1000Base-X and 100Base-FX modules or copper and it will autodetect the
exact media.
This is supported via the SFP op-s and I have been using it without issues
for a while.

I have not tested option 3 in combination with SFP to the copper
module so I cant
say whether that works.
Christian Marangi Nov. 23, 2023, 7:36 p.m. UTC | #6
On Thu, Nov 23, 2023 at 04:07:14PM +0100, Andrew Lunn wrote:
> > compatible = "ethernet-phy-package", "qca807x-phy-package";
> > 
> > With "ethernet-phy-package" a must and "qca807x-phy-package" used only
> > if additional property are used.
> > 
> > My current idea was to use select and base everything on the possible
> > PHY compatible (and it does work, tested by adding bloat in the DT
> > example and seeing if the schema was rejected). Had this idea since the
> > compatible would never be used.
> 
> The DT people are unhappy with PHYs don't use compatibles, so
> validation does not work. It probably too late to add compatibles to
> very PHY driver. But this is new development work, we don't have any
> history. So we can add a compatible per package to make the validation
> tools work.
> 
> So for parsing the tree in the kernel we look for
> 'ethernet-phy-package'. For validating the tree using the yaml tools
> we use the 'qca807x-phy-package'.
>

Ok clear, what about the generic ethernet-phy-package.yaml?

Idea was to describe common properties there and then specific PHY
package would add every common property with $ref and add their special
ones on top of that. Would that be ok? (similar to the current
implementation with ethernet-phy-package and qcom,qca807x with the only
difference that qcom,qca807x.yaml would also have the compatible set
(currently missing from this RFC)
Luo Jie Nov. 24, 2023, 11:49 a.m. UTC | #7
On 11/24/2023 3:33 AM, Christian Marangi wrote:
> On Thu, Nov 23, 2023 at 03:57:58PM +0100, Andrew Lunn wrote:
>> On Thu, Nov 23, 2023 at 02:35:31PM +0000, Russell King (Oracle) wrote:
>>> On Thu, Nov 23, 2023 at 03:27:05PM +0100, Andrew Lunn wrote:
>>>>> Just to be more precise qca807x can operate in 3 different mode:
>>>>> (this is controlled by the MODE_CFG bits)
>>>>
>>>>> - QSGMII: 5 copper port
>>>>
>>>> 4 slots over QSGMII, plus the second SERDES is connected to the MAC
>>>> using SGMII/1000BaseX?
>>>>
>>>>> - PSGMII: 5 copper port
>>>>
>>>> 5 slots over QSGMII, the second SERDES is idle?
>>>>
>>>>> - PSGMII: 4 copper port + 1 combo (that can be both fiber or copper)
>>>>
>>>> 5 slots over QSGMII, with the second SERDES connected to an SFP cage.
>>>>
>>>> Are ports 1-4 always connected to the P/Q SGMII. Its only port 5 which
>>>> can use the second SERDES?
>>>
>>> I think what would really help here is if there was an ascii table to
>>> describe the configurations, rather than trying to put it into words.
>>
>> Yes.
>>
>> And also for ipq4019. We need to merge these two threads of
>> conversation, since in the end they are probably the same driver, same
>> device tree etc.
>>
> 
> For everyone that missed Robert response in patch 12 let me quote him
> also here.
> 
> "
> Hi Andrew,
> I think that the description is confusing.
> QCA807x supports 3 different modes:
> 1. PSGMII (5 copper ports)
> 2. PSGMII (4 copper ports + 1 combo port)
> 3. QSGMII+SGMII
> 
> So, in case option 2 is selected then the combo port can also be used for
> 1000Base-X and 100Base-FX modules or copper and it will autodetect the
> exact media.
> This is supported via the SFP op-s and I have been using it without issues
> for a while.
> 
> I have not tested option 3 in combination with SFP to the copper
> module so I cant
> say whether that works.

For the option3, the last PHY works on SGMII mode, then it can't be
used with SFP, only copper is supported in this mode.

>  From what I can gather from the typical usage examples in the
> datasheet, this QSMII+SGMII
> mode is basically intended as a backward compatibility thing as only
> QCA SoC-s have PSGMII
> support so that you could still use SoC-s with QSGMII and SGMII support only.
> 
> So there is no way to control the SerDes-es individually, only the
> global mode can be changed via
> the Chip configuration register in the Combo port.
> 
> You can see the block diagram of this PHY in this public PDF on page 2[1].
> 
> [1] https://content.codico.com/fileadmin/media/download/datasheets/qualcomm/qualcomm_qca8075.pdf
> "
>
Russell King (Oracle) Nov. 24, 2023, 12:02 p.m. UTC | #8
On Fri, Nov 24, 2023 at 07:49:27PM +0800, Jie Luo wrote:
> 
> 
> On 11/24/2023 3:33 AM, Christian Marangi wrote:
> > On Thu, Nov 23, 2023 at 03:57:58PM +0100, Andrew Lunn wrote:
> > > On Thu, Nov 23, 2023 at 02:35:31PM +0000, Russell King (Oracle) wrote:
> > > > On Thu, Nov 23, 2023 at 03:27:05PM +0100, Andrew Lunn wrote:
> > > > > > Just to be more precise qca807x can operate in 3 different mode:
> > > > > > (this is controlled by the MODE_CFG bits)
> > > > > 
> > > > > > - QSGMII: 5 copper port
> > > > > 
> > > > > 4 slots over QSGMII, plus the second SERDES is connected to the MAC
> > > > > using SGMII/1000BaseX?
> > > > > 
> > > > > > - PSGMII: 5 copper port
> > > > > 
> > > > > 5 slots over QSGMII, the second SERDES is idle?
> > > > > 
> > > > > > - PSGMII: 4 copper port + 1 combo (that can be both fiber or copper)
> > > > > 
> > > > > 5 slots over QSGMII, with the second SERDES connected to an SFP cage.
> > > > > 
> > > > > Are ports 1-4 always connected to the P/Q SGMII. Its only port 5 which
> > > > > can use the second SERDES?
> > > > 
> > > > I think what would really help here is if there was an ascii table to
> > > > describe the configurations, rather than trying to put it into words.
> > > 
> > > Yes.
> > > 
> > > And also for ipq4019. We need to merge these two threads of
> > > conversation, since in the end they are probably the same driver, same
> > > device tree etc.
> > > 
> > 
> > For everyone that missed Robert response in patch 12 let me quote him
> > also here.
> > 
> > "
> > Hi Andrew,
> > I think that the description is confusing.
> > QCA807x supports 3 different modes:
> > 1. PSGMII (5 copper ports)
> > 2. PSGMII (4 copper ports + 1 combo port)
> > 3. QSGMII+SGMII
> > 
> > So, in case option 2 is selected then the combo port can also be used for
> > 1000Base-X and 100Base-FX modules or copper and it will autodetect the
> > exact media.
> > This is supported via the SFP op-s and I have been using it without issues
> > for a while.
> > 
> > I have not tested option 3 in combination with SFP to the copper
> > module so I cant
> > say whether that works.
> 
> For the option3, the last PHY works on SGMII mode, then it can't be
> used with SFP, only copper is supported in this mode.

So, from what you've written, and looking at the PDF for QCA8075:

		First Serdes mode	Second Serdes mode
Option 1	PSGMII for copper	Disabled
		ports 0-4
Option 2	PSGMII for copper	1000BASE-X / 100BASE-FX
		ports 0-4
Option 3	QSGMII for copper	SGMII for
		ports 0-3		copper port 4

In all cases, ports 0-4 have a copper (baseT/baseTx/baseTe) port
available.



This is a much easier to understand presentation than writing out a
longwinded description of the three modes. Please include the table
and the statement below the table in the commit description as that
is necessary to describe the hardware setup being addressed in these
patches.

Thanks.
Andrew Lunn Nov. 24, 2023, 2:44 p.m. UTC | #9
> 		First Serdes mode	Second Serdes mode
> Option 1	PSGMII for copper	Disabled
> 		ports 0-4
> Option 2	PSGMII for copper	1000BASE-X / 100BASE-FX
> 		ports 0-4
> Option 3	QSGMII for copper	SGMII for
> 		ports 0-3		copper port 4

With option 2, can the second SERDES also do SGMII? You are likely to
need that when a Copper SFP module is inserted into the cage.

     Andrew
Russell King (Oracle) Nov. 24, 2023, 3:16 p.m. UTC | #10
On Fri, Nov 24, 2023 at 03:44:20PM +0100, Andrew Lunn wrote:
> > 		First Serdes mode	Second Serdes mode
> > Option 1	PSGMII for copper	Disabled
> > 		ports 0-4
> > Option 2	PSGMII for copper	1000BASE-X / 100BASE-FX
> > 		ports 0-4
> > Option 3	QSGMII for copper	SGMII for
> > 		ports 0-3		copper port 4
> 
> With option 2, can the second SERDES also do SGMII? You are likely to
> need that when a Copper SFP module is inserted into the cage.

The document states "The fiber port supports 1000BASE-X/100BASE-FX".

The same is true of Marvell 88x3310's fiber port - it supports only
fiber not SGMII. This is actually something else that - when the
patches for stacked PHYs mature - will need to be addressed. If we
have a 1G copper SFP plugged into an interface that only supports
1000base-X then we need a way to switch the PHY on the SFP module
to 1000base-X if it's in SGMII mode.

Some copper SFPs come up in 1000base-X mode, and we currently rely
on the 88e1111 driver to switch them to SGMII mode. Others do want
SGMII mode (like Mikrotik RJ01 where the PHY is inaccessible and
thus can't be reconfigured.)
Christian Marangi Nov. 24, 2023, 4:25 p.m. UTC | #11
On Fri, Nov 24, 2023 at 06:59:23PM +0200, Vladimir Oltean wrote:
> On Wed, Nov 22, 2023 at 07:32:22PM +0100, Christian Marangi wrote:
> > Sooooo.... Sorry if I insist but I would really love to have something
> > ""stable"" to move this further. (the changes are easy enough so it's
> > really a matter of finding a good DT structure)
> > 
> > Maybe a good idea would be summarize the concern and see what solution
> > was proposed:
> 
> Sorry, I didn't follow the entire discussion. I hope I'm not too far off
> with my understanding of your problems.
> 
> I think you are hitting some of the same points I have hit with DSA.
> The PHY package could be considered an SoC with lots of peripherals on
> it, for which you'd want separate drivers. Just like a DSA switch would.
> I don't think it's exactly phylib's place to deal with that, just like
> it's not DSA's place to deal with complex SoCs, just with the switching
> IP (like the Ethernet PHY IP for phylib).
> https://lore.kernel.org/lkml/20221222134844.lbzyx5hz7z5n763n@skbuf/
> 
> Why does the ethernet-phy-package DT binding attempt to be so grand and
> generic? I would expect the 180 degree opposite. Make it have a _single_
> compatible of "qcom,qca807x" (but don't use an "x" wildcard, do specify
> the full package name).
> 
> Make it have a "reg" property which is the base MDIO address of the package.
> 
> Write an mdio_device driver that probes on that. The PHY core already
> knows that if a child on the MDIO bus has a compatible string of the
> normal form (not like "ethernet-phy-id004d.d0b2"), then it's looking at
> an mdio_device.
> 
> Make the OF node of the package have an "mdio" child with its own
> compatible string, which represents the place where PHYs are. The driver
> for the "mdio" child has a very simple implementation of the mii_bus
> ops, which just calls into the device parent (it can assume a certain
> parent implementation and private data structures).
> 
> Lateral to the "mdio" child node of the "qcom,qca807x" package node, you
> could put any other device tree properties that you want.
> 
> Make the mdio_device driver for "qcom,qca807x" use shared code if you
> want - but keep the device tree structure hardware-specific. Look at the
> compatible strings that e.g. the drivers/mfd/simple-mfd-i2c.c driver
> probes on. You could always change the driver implementation for a
> certain compatible string, but you'll be stuck with the ultra-generic
> compatible = "ethernet-phy-package", which has the problems that you
> mention.
>

The main reason is the fact that PHY package are already a thing and API
are already there (phy_package_join/leave...) so we just lack any way to
support this in DT without using specialized code in the PHY driver.

This is really completing the feature.

The only reason for the generic "ethernet-phy-package" compatible is to
have a solid detection of the node in PHY core. (I assume parsing the
node name might be problematic? Or maybe not now that we require adding
a reg to it)

Also I don't expect tons of special properties for PHY package, with the
current probe/config implementation, a PHY driver have lots of
flexibility in all kind of validation.

Consider that the additional global-phys and global-phy-names are
already expected to be dropped.
(we know the PHY package base address and we can calculate the offset of
the global phy from there in the PHY package probe)

And even the phy-mode has been scrapped for more specific solution...
(some assumption can be done on probe by checking the PHY mode and set
stuff accordingly or even do parsing in the PHY package node as we pass
the OF node of the phy package)

The PHY package node would be reduced to a simple compatible (and even
this can be dropped) and a reg.

I feel there is a big chance here to generalize it and prevent any kind
of mess with all kind of similar/equal code that just do the same thing.
(and we already have an example with the PHY package API usage with
every PHY having the same exact pattern for probe/config and nothing
describing that the PHY are actually a package in DT)

Hope it all makes sense to you.

> > 
> > Concern list:
> > 1. ethernet-phy-package MUST be placed in mdio node (not in ethernet,
> >    the example was wrong anyway) and MUST have an addr
> > 
> >    Current example doesn't have an addr. I would prefer this way but
> >    no problem in changing this.
> > 
> >    Solution:
> >      - Add reg to the ethernet-phy-package node with the base address of
> >        the PHY package (base address = the first PHY address of the
> >        package)
> 
> Correct, what I'm saying is compatible with this.
> 
> > 
> >        We will have a PHY node with the same address of the PHY package
> >        node. Each PHY node in the PHY package node will have reg set to
> >        the REAL address in the mdio bus.
> 
> If the real PHY IPs are children of the package rather than on the same
> level with it, I don't think this will be a problem. I wonder if some
> address translation could be done with the "ranges" device tree property.
> I've never seen this with MDIO though.
> 

I can check it, I would love some way to describe the address used by
the PHY package. (since everything will be handled internally with
offsets, would be good to define in DT that (for example) addrs from 0
to 5 are used). Some PHY might be not attached but still used for global
configuration of the PHY package.

> > 4. Not finding a correct place to put PHY package info.
> > 
> >    I'm still convinced the mdio node is the correct place.
> >    - PHY package are PHY in bundle so they are actual PHY
> >    - We already have in the mdio node special handling (every DSA switch
> >      use custom compatible and PHY ID is not used to probe them
> >      normally)
> >    - Node this way won't be treated as PHY as they won't match the PHY
> >      node name pattern and also won't have the compatible pattern for
> >      PHY.
> > 
> >    Solution:
> >     - ethernet-phy-package node is OK given a reg is defined.
> 
> I agree that it should sit under the MDIO node. I disagree with the idea
> of a standardized binding for PHY packages.
Robert Marko Nov. 24, 2023, 4:59 p.m. UTC | #12
On Fri, Nov 24, 2023 at 4:16 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Fri, Nov 24, 2023 at 03:44:20PM +0100, Andrew Lunn wrote:
> > >             First Serdes mode       Second Serdes mode
> > > Option 1    PSGMII for copper       Disabled
> > >             ports 0-4
> > > Option 2    PSGMII for copper       1000BASE-X / 100BASE-FX
> > >             ports 0-4
> > > Option 3    QSGMII for copper       SGMII for
> > >             ports 0-3               copper port 4
> >
> > With option 2, can the second SERDES also do SGMII? You are likely to
> > need that when a Copper SFP module is inserted into the cage.
>
> The document states "The fiber port supports 1000BASE-X/100BASE-FX".
>
> The same is true of Marvell 88x3310's fiber port - it supports only
> fiber not SGMII. This is actually something else that - when the
> patches for stacked PHYs mature - will need to be addressed. If we
> have a 1G copper SFP plugged into an interface that only supports
> 1000base-X then we need a way to switch the PHY on the SFP module
> to 1000base-X if it's in SGMII mode.
>
> Some copper SFPs come up in 1000base-X mode, and we currently rely
> on the 88e1111 driver to switch them to SGMII mode. Others do want
> SGMII mode (like Mikrotik RJ01 where the PHY is inaccessible and
> thus can't be reconfigured.)

I can confirm that SGMII mode doesn't work with Option 2, I have tested this
a while ago with Mikrotik RJ01 (I think it has AR803x, but its not accessible as
you pointed out) and it was somewhat working but in half duplex only
and dropping
packets.
Currently, SFP mode is checked and only 1000Base-X and 100Base-FX are
accepted, otherwise the module insert will return and error for unsupported
mode.

Regards,
Robert

>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Vladimir Oltean Nov. 24, 2023, 4:59 p.m. UTC | #13
On Wed, Nov 22, 2023 at 07:32:22PM +0100, Christian Marangi wrote:
> Sooooo.... Sorry if I insist but I would really love to have something
> ""stable"" to move this further. (the changes are easy enough so it's
> really a matter of finding a good DT structure)
> 
> Maybe a good idea would be summarize the concern and see what solution
> was proposed:

Sorry, I didn't follow the entire discussion. I hope I'm not too far off
with my understanding of your problems.

I think you are hitting some of the same points I have hit with DSA.
The PHY package could be considered an SoC with lots of peripherals on
it, for which you'd want separate drivers. Just like a DSA switch would.
I don't think it's exactly phylib's place to deal with that, just like
it's not DSA's place to deal with complex SoCs, just with the switching
IP (like the Ethernet PHY IP for phylib).
https://lore.kernel.org/lkml/20221222134844.lbzyx5hz7z5n763n@skbuf/

Why does the ethernet-phy-package DT binding attempt to be so grand and
generic? I would expect the 180 degree opposite. Make it have a _single_
compatible of "qcom,qca807x" (but don't use an "x" wildcard, do specify
the full package name).

Make it have a "reg" property which is the base MDIO address of the package.

Write an mdio_device driver that probes on that. The PHY core already
knows that if a child on the MDIO bus has a compatible string of the
normal form (not like "ethernet-phy-id004d.d0b2"), then it's looking at
an mdio_device.

Make the OF node of the package have an "mdio" child with its own
compatible string, which represents the place where PHYs are. The driver
for the "mdio" child has a very simple implementation of the mii_bus
ops, which just calls into the device parent (it can assume a certain
parent implementation and private data structures).

Lateral to the "mdio" child node of the "qcom,qca807x" package node, you
could put any other device tree properties that you want.

Make the mdio_device driver for "qcom,qca807x" use shared code if you
want - but keep the device tree structure hardware-specific. Look at the
compatible strings that e.g. the drivers/mfd/simple-mfd-i2c.c driver
probes on. You could always change the driver implementation for a
certain compatible string, but you'll be stuck with the ultra-generic
compatible = "ethernet-phy-package", which has the problems that you
mention.

> 
> Concern list:
> 1. ethernet-phy-package MUST be placed in mdio node (not in ethernet,
>    the example was wrong anyway) and MUST have an addr
> 
>    Current example doesn't have an addr. I would prefer this way but
>    no problem in changing this.
> 
>    Solution:
>      - Add reg to the ethernet-phy-package node with the base address of
>        the PHY package (base address = the first PHY address of the
>        package)

Correct, what I'm saying is compatible with this.

> 
>        We will have a PHY node with the same address of the PHY package
>        node. Each PHY node in the PHY package node will have reg set to
>        the REAL address in the mdio bus.

If the real PHY IPs are children of the package rather than on the same
level with it, I don't think this will be a problem. I wonder if some
address translation could be done with the "ranges" device tree property.
I've never seen this with MDIO though.

> 4. Not finding a correct place to put PHY package info.
> 
>    I'm still convinced the mdio node is the correct place.
>    - PHY package are PHY in bundle so they are actual PHY
>    - We already have in the mdio node special handling (every DSA switch
>      use custom compatible and PHY ID is not used to probe them
>      normally)
>    - Node this way won't be treated as PHY as they won't match the PHY
>      node name pattern and also won't have the compatible pattern for
>      PHY.
> 
>    Solution:
>     - ethernet-phy-package node is OK given a reg is defined.

I agree that it should sit under the MDIO node. I disagree with the idea
of a standardized binding for PHY packages.
Vladimir Oltean Nov. 24, 2023, 6:27 p.m. UTC | #14
On Fri, Nov 24, 2023 at 05:25:16PM +0100, Christian Marangi wrote:
> The main reason is the fact that PHY package are already a thing and API
> are already there (phy_package_join/leave...) so we just lack any way to
> support this in DT without using specialized code in the PHY driver.
> 
> This is really completing the feature.

Hmm, I see struct phy_package_shared as a mechanism to tell phylib that
multiple device structures are actually related with each other, because
the device core, and their parent bus, has no idea. If you're under
control of the parent bus code and you can probe PHY devices in any
order you want and do whatever you want before probing them, I don't see
why you would need struct phy_package_shared any longer? I don't see why
this feature needs to be completed, if that involves changes to the
device tree structure. PHY packages assumed no changes to the device
tree (they rely on a hacky interpretation of the MDIO address AFAIU).
If we change that basic premise, all implementation options are on the
table, I think.

> The only reason for the generic "ethernet-phy-package" compatible is to
> have a solid detection of the node in PHY core. (I assume parsing the
> node name might be problematic? Or maybe not now that we require adding
> a reg to it)

Our opinions seem to differ, but I don't think that the package needs a
solid detection of the node in the PHY core :) I think phy_devices and
mdio_devices already cover everything that's necessary to build a
solution.

> Also I don't expect tons of special properties for PHY package, with the
> current probe/config implementation, a PHY driver have lots of
> flexibility in all kind of validation.
> 
> Consider that the additional global-phys and global-phy-names are
> already expected to be dropped.
> (we know the PHY package base address and we can calculate the offset of
> the global phy from there in the PHY package probe)
> 
> And even the phy-mode has been scrapped for more specific solution...
> (some assumption can be done on probe by checking the PHY mode and set
> stuff accordingly or even do parsing in the PHY package node as we pass
> the OF node of the phy package)
> 
> The PHY package node would be reduced to a simple compatible (and even
> this can be dropped) and a reg.

So why does it need to be described in DT, at this stage? :)

> I feel there is a big chance here to generalize it and prevent any kind
> of mess with all kind of similar/equal code that just do the same thing.
> (and we already have an example with the PHY package API usage with
> every PHY having the same exact pattern for probe/config and nothing
> describing that the PHY are actually a package in DT)
> 
> Hope it all makes sense to you.
Andrew Lunn Nov. 24, 2023, 6:35 p.m. UTC | #15
> I think you are hitting some of the same points I have hit with DSA.
> The PHY package could be considered an SoC with lots of peripherals on
> it, for which you'd want separate drivers.

At least at the moment, this is not true. The package does just
contain PHYs. But it also has some properties which are shared across
those PHYs, e.g. reset. 

What you describe might become true in the future. e.g. The LED/GPIO
controller is currently part of the PHY, and each PHY has its own. I
could however imagine that becomes a block of its own, outside of the
PHY address space, and maybe it might want its own class LED
driver. Some PHYs have temperature sensors, which could be a package
sensor, so could in theory be an individual hwmon driver. However,
i've not yet seen such a package.

Do we consider this now? At the moment i don't see an MFD style system
is required. We could crystal ball gaze and come up with some
requirements, but i would prefer to have some real devices and
datasheets. Without them, we will get the requirements wrong.

I also think we are not that far away from it, in terms of DT, if you
consider the later comments. I suggested we need a phy package
specific compatible. At the moment, it will be ignored by the kernel,
the kernel does not need it, it probes the PHYs in the current way,
using the ID registers. But it could in future be used to probe a real
driver, which could be an MFD style driver. We need to see updated DT
binding examples, but i don't see why we cannot slot it in at a later
date.

	Andrew
Vladimir Oltean Nov. 24, 2023, 7:40 p.m. UTC | #16
On Fri, Nov 24, 2023 at 07:35:35PM +0100, Andrew Lunn wrote:
> > I think you are hitting some of the same points I have hit with DSA.
> > The PHY package could be considered an SoC with lots of peripherals on
> > it, for which you'd want separate drivers.
> 
> At least at the moment, this is not true. The package does just
> contain PHYs. But it also has some properties which are shared across
> those PHYs, e.g. reset. 
> 
> What you describe might become true in the future. e.g. The LED/GPIO
> controller is currently part of the PHY, and each PHY has its own. I
> could however imagine that becomes a block of its own, outside of the
> PHY address space, and maybe it might want its own class LED
> driver. Some PHYs have temperature sensors, which could be a package
> sensor, so could in theory be an individual hwmon driver. However,
> i've not yet seen such a package.
> 
> Do we consider this now? At the moment i don't see an MFD style system
> is required. We could crystal ball gaze and come up with some
> requirements, but i would prefer to have some real devices and
> datasheets. Without them, we will get the requirements wrong.
> 
> I also think we are not that far away from it, in terms of DT, if you
> consider the later comments. I suggested we need a phy package
> specific compatible. At the moment, it will be ignored by the kernel,
> the kernel does not need it, it probes the PHYs in the current way,
> using the ID registers. But it could in future be used to probe a real
> driver, which could be an MFD style driver. We need to see updated DT
> binding examples, but i don't see why we cannot slot it in at a later
> date.

I'm not suggesting to go for MFD right away. Just with a structure that
is extensible to possibly cover that. For now, a package node with a
Qualcomm compatible, with the most minimal driver that forwards MDIO
access to PHY children.

I can't speak for the future of PHY drivers, since I don't know enough
about PHYs. I'm just coming from the DSA background where I really wish
we had this sort of infrastructure earlier. Now I have the SJA1110 which
still lacks support for the interrupt controller for its integrated
PHYs, and a bunch of other IP blocks in the package, because it's so
incredibly hard to make the driver support the old-style and the
new-style device trees.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
new file mode 100644
index 000000000000..2aa109e155d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
@@ -0,0 +1,86 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/ethernet-phy-package.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ethernet PHY Package Common Properties
+
+maintainers:
+  - Christian Marangi <ansuelsmth@gmail.com
+
+properties:
+  $nodename:
+    pattern: "^ethernet-phy-package(-[0-9]+)?$"
+
+  compatible:
+    const: ethernet-phy-package
+
+  '#address-cells':
+    description: number of address cells for the MDIO bus
+    const: 1
+
+  '#size-cells':
+    description: number of size cells on the MDIO bus
+    const: 0
+
+  global-phys:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    minItems: 1
+    maxItems: 31
+    description:
+      List of phandle to the PHY in the package required and
+      used to configure the PHY package.
+
+  global-phy-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    minItems: 1
+    maxItems: 31
+    description:
+      List of names of the PHY defined in global-phys.
+
+  phy-connection-type:
+    $ref: /schemas/net/ethernet-phy-mode-types.yaml#definitions/phy-connection-type
+    description:
+      Specifies global interface type for the PHY package.
+
+  phy-mode:
+    $ref: "#/properties/phy-connection-type"
+
+patternProperties:
+  ^ethernet-phy(@[a-f0-9]+)?$:
+    $ref: /schemas/net/ethernet-phy.yaml#
+
+required:
+  - compatible
+
+dependencies:
+  global-phy-names: [global-phys]
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    ethernet {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy-package {
+            compatible = "ethernet-phy-package";
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            global-phys = <&phy4>;
+            global-phy-names = "base";
+
+            ethernet-phy@1 {
+              compatible = "ethernet-phy-ieee802.3-c22";
+              reg = <1>;
+            };
+
+            phy4: ethernet-phy@4 {
+              compatible = "ethernet-phy-ieee802.3-c22";
+              reg = <4>;
+            };
+        };
+    };