Messages in this thread Patch in this message | | | Date | Tue, 29 Nov 2022 11:57:36 -0500 | Subject | Re: [PATCH net v2 2/2] phy: aquantia: Determine rate adaptation support from registers | From | Sean Anderson <> |
| |
On 11/29/22 11:46, Russell King (Oracle) wrote: > On Tue, Nov 29, 2022 at 11:29:39AM -0500, Sean Anderson wrote: >> On 11/29/22 11:17, Russell King (Oracle) wrote: >> > On Tue, Nov 29, 2022 at 10:56:56AM -0500, Sean Anderson wrote: >> >> On 11/28/22 19:42, Russell King (Oracle) wrote: >> >> > On Mon, Nov 28, 2022 at 07:21:56PM -0500, Sean Anderson wrote: >> >> >> On 11/28/22 18:22, Russell King (Oracle) wrote: >> >> >> > This doesn't make any sense. priv->supported_speeds is the set of speeds >> >> >> > read from the PMAPMD. The only bits that are valid for this are the >> >> >> > MDIO_PMA_SPEED_* definitions, but teh above switch makes use of the >> >> >> > MDIO_PCS_SPEED_* definitions. To see why this is wrong, look at these >> >> >> > two definitions: >> >> >> > >> >> >> > #define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */ >> >> >> > #define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */ >> >> >> > >> >> >> > Note that they are the same value, yet above, you're testing for bit 6 >> >> >> > being clear effectively for both 10M and 2.5G speeds. I suspect this >> >> >> > is *not* what you want. >> >> >> > >> >> >> > MDIO_PMA_SPEED_* are only valid for the PMAPMD MMD (MMD 1). >> >> >> > MDIO_PCS_SPEED_* are only valid for the PCS MMD (MMD 3). >> >> >> >> >> >> Ugh. I almost noticed this from the register naming... >> >> >> >> >> >> Part of the problem is that all the defines are right next to each other >> >> >> with no indication of what you just described. >> >> > >> >> > That's because they all refer to the speed register which is at the same >> >> > address, but for some reason the 802.3 committees decided to make the >> >> > register bits mean different things depending on the MMD. That's why the >> >> > definition states the MMD name in it. >> >> >> >> Well, then it's really a different register per MMD (and therefore the >> >> definitions should be better separated). Grouping them together implies >> >> that they share bits, when they do not (except for the 10G bit). >> > >> > What about bits that are shared amongst the different registers. >> > Should we have multiple definitions for the link status bit in _all_ >> > the different MMDs, despite it being the same across all status 1 >> > registers? >> >> No, but for registers which are 95% difference we should at least separate >> them and add a comment. >> >> > Clause 45 is quite a trainwreck when it comes to these register >> > definitions. >> >> Maybe they should have randomized the bit orders in the first place to discourage this sort of thing :) >> >> > As I've stated, there is a pattern to the naming. Understand it, >> > and it isn't confusing. >> > >> >> I don't have a problem with the naming, just the organization of the >> source file. > > The organisation is sane. There are some shared bits in the SPEED > register between different MMDs. > > If we separate the PMA and PCS with a blink line, do we then seperate > the register groups with two blank lines? No, people will complain > about that (they already do if you think about doing that in source > files.) > > Sorry, but... one has to pay attention to the whole of the macro name, > not just the last few characters... and think "is something that > contains "_PCS_" in its name really suitable for use with a PMA/PMD > MMD register when there's a PCS MMD? Now let me think... umm. no. >
Well, what I had in mind was
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h index 75b7257a51e1..d700e9e886b9 100644 --- a/include/uapi/linux/mdio.h +++ b/include/uapi/linux/mdio.h @@ -127,16 +127,36 @@ #define MDIO_AN_STAT1_PAGE 0x0040 /* Page received */ #define MDIO_AN_STAT1_XNP 0x0080 /* Extended next page status */ -/* Speed register. */ +/* Speed register common. */ #define MDIO_SPEED_10G 0x0001 /* 10G capable */ + +/* PMA/PMD Speed register. */ +#define MDIO_PMA_SPEED_10G MDIO_SPEED_10G #define MDIO_PMA_SPEED_2B 0x0002 /* 2BASE-TL capable */ #define MDIO_PMA_SPEED_10P 0x0004 /* 10PASS-TS capable */ #define MDIO_PMA_SPEED_1000 0x0010 /* 1000M capable */ #define MDIO_PMA_SPEED_100 0x0020 /* 100M capable */ #define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */ +#define MDIO_PMA_SPEED_10G1G 0x0080 /* 10/1G capable */ +#define MDIO_PMA_SPEED_40G 0x0100 /* 40G capable */ +#define MDIO_PMA_SPEED_100G 0x0200 /* 100G capable */ +#define MDIO_PMA_SPEED_10GP 0x0400 /* 10GPASS-XR capable */ +#define MDIO_PMA_SPEED_25G 0x0800 /* 25G capable */ +#define MDIO_PMA_SPEED_200G 0x1000 /* 200G capable */ +#define MDIO_PMA_SPEED_2_5G 0x2000 /* 2.5G capable */ +#define MDIO_PMA_SPEED_5G 0x4000 /* 5G capable */ +#define MDIO_PMA_SPEED_400G 0x8000 /* 400G capable */ + +/* PCS et al. Speed register. */ +#define MDIO_PCS_SPEED_10G MDIO_SPEED_10G #define MDIO_PCS_SPEED_10P2B 0x0002 /* 10PASS-TS/2BASE-TL capable */ +#define MDIO_PCS_SPEED_40G 0x0004 /* 450G capable */ +#define MDIO_PCS_SPEED_100G 0x0008 /* 100G capable */ +#define MDIO_PCS_SPEED_25G 0x0010 /* 25G capable */ #define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */ #define MDIO_PCS_SPEED_5G 0x0080 /* 5G capable */ +#define MDIO_PCS_SPEED_200G 0x0100 /* 200G capable */ +#define MDIO_PCS_SPEED_400G 0x0200 /* 400G capable */ /* Device present registers. */ #define MDIO_DEVS_PRESENT(devad) (1 << (devad)) Really, these registers have almost nothing in common except their concept and sub-address.
On another note: is BIT() allowed in uapi headers?
--Sean
| |