| Message ID | 20231120135041.15259-4-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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.
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
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
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.
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)
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 > " >
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.
> 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
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.)
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.
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!
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.
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.
> 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
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 --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>; + }; + }; + };
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