Messages in this thread | | | Subject | Re: [PATCH net-next 0/1] Add BASE-T1 PHY support | From | Heiner Kallweit <> | Date | Sat, 24 Aug 2019 17:03:33 +0200 |
| |
On 22.08.2019 09:18, Christian Herber wrote: > On 21.08.2019 20:57, Andrew Lunn wrote: >> >>> The current patch set IMO is a little bit hacky. I'm not 100% happy >>> with the implicit assumption that there can't be devices supporting >>> T1 and classic BaseT modes or fiber modes. >> >>> Andrew: Do you have an opinion on that? >> >> Hi Heiner >> >> I would also like cleaner integration. I doubt here is anything in the >> standard which says you cannot combine these modes. It is more a >> marketing question if anybody would build such a device. Maybe not >> directly into a vehicle, but you could imaging a mobile test device >> which uses T1 to talk to the car and T4 to connect to the garage >> network? >> >> So i don't think we should limit ourselves. phylib should provide a >> clean, simple set of helpers to perform standard operations for >> various modes. Drivers can make use of those helpers. That much should >> be clear. If we try to make genphy support them all simultaneously, is >> less clear. >> >> Andrew >> > > If you want to go down this path, then i think we have to ask some more > questions. Clause 45 is a very scalable register scheme, it is not a > specific class of devices and will be extended and extended. > > Currently, the phy-c45.c supports 10/100/1000/2500/5000/10000 Mbps > consumer/enterprise PHYs. This is also an implicit assumption. The > register set (e.g. on auto-neg) used for this will also only support > these modes and nothing more, as it is done scaling. > > Currently not supported, but already present in IEEE 802.3: > - MultiGBASE-T (25/40 Gbps) (see e.g. MultiGBASE-T AN control 1 register) > - BASE-T1 > - 10BASE-T1 > - NGBASE-T1 > > And surely there are some on the way or already there that I am not > aware of. > > To me, one architectural decision point is if you want to have generic > support for all C45 PHYs in one file, or if you want to split it by > device class. I went down the first path with my patch, as this is the > road gone also with the existing code. > > If you want to split BASE-T1, i think you will need one basic C45 > library (genphy_c45_pma_read_abilities() is a good example of a function > that is not specific to a device class). On the other hand, > genphy_c45_pma_setup_forced() is not a generic function at this point as > it supports only a subset of devices managed in C45. > > I tend to agree with you that splitting is the best way to go in the > long run, but that also requires a split of the existing phy-c45.c into > two IMHO. > BASE-T1 seems to be based on Clause 45 (at least Clause 45 MDIO), but it's not fully compliant with Clause 45. Taking AN link status as an example: 45.2.7.2.7 states that link-up is signaled in bit 7.1.2. If BASE-T1 uses a different register, then it's not fully Clause 45 compatible. Therefore also my question for the datasheet of an actual BASE-T1 PHY, as I would be curious whether it shadows the link-up bit from 7.513.2 to 7.1.2 to be Clause 45 compliant. Definitely reading bit 7.513.2 is nothing that belongs into a genphy_c45_ function.
The extension to genphy_c45_pma_read_abilities() looks good to me, for the other parts I'd like to see first how real world BASE-T1 PHYs handle it. If they shadow the T1-specific bits to the Clause 45 standard ones, we should be fine. Otherwise IMO we have to add separate T1 functions to phylib.
Heiner
| |