* [PATCH net-next 0/2] net: phy: use phy_id_mask value zero for exact match
@ 2018-11-07 20:52 Heiner Kallweit
2018-11-07 20:53 ` [PATCH net-next 1/2] " Heiner Kallweit
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Heiner Kallweit @ 2018-11-07 20:52 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev
A phy_id_mask value zero means every PHYID matches, therefore
value zero isn't used. So we can safely redefine the semantics
of value zero to mean "exact match". This allows to avoid some
boilerplate code in PHY driver configs.
Realtek PHY driver is the first user of this change.
Heiner Kallweit (2):
net: phy: use phy_id_mask value zero for exact match
net: phy: realtek: remove boilerplate code from PHY driver definitions
drivers/net/phy/phy_device.c | 21 +++++++++++++++------
drivers/net/phy/realtek.c | 9 ---------
include/linux/phy.h | 2 +-
3 files changed, 16 insertions(+), 16 deletions(-)
--
2.19.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH net-next 1/2] net: phy: use phy_id_mask value zero for exact match 2018-11-07 20:52 [PATCH net-next 0/2] net: phy: use phy_id_mask value zero for exact match Heiner Kallweit @ 2018-11-07 20:53 ` Heiner Kallweit 2018-11-08 18:34 ` Andrew Lunn 2018-11-08 19:44 ` Florian Fainelli 2018-11-07 20:54 ` [PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs Heiner Kallweit 2018-11-08 23:05 ` [PATCH net-next 0/2] net: phy: use phy_id_mask value zero for exact match David Miller 2 siblings, 2 replies; 11+ messages in thread From: Heiner Kallweit @ 2018-11-07 20:53 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev A phy_id_mask value zero means every PHYID matches, therefore value zero isn't used. So we can safely redefine the semantics of value zero to mean "exact match". This allows to avoid some boilerplate code in PHY driver configs. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy_device.c | 21 +++++++++++++++------ include/linux/phy.h | 2 +- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index ab33d1777..d165a2c82 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -483,15 +483,24 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv) if (!(phydev->c45_ids.devices_in_package & (1 << i))) continue; - if ((phydrv->phy_id & phydrv->phy_id_mask) == - (phydev->c45_ids.device_ids[i] & - phydrv->phy_id_mask)) - return 1; + if (!phydrv->phy_id_mask) { + if (phydrv->phy_id == + phydev->c45_ids.device_ids[i]) + return 1; + } else { + if ((phydrv->phy_id & phydrv->phy_id_mask) == + (phydev->c45_ids.device_ids[i] & + phydrv->phy_id_mask)) + return 1; + } } return 0; } else { - return (phydrv->phy_id & phydrv->phy_id_mask) == - (phydev->phy_id & phydrv->phy_id_mask); + if (!phydrv->phy_id_mask) + return phydrv->phy_id == phydev->phy_id; + else + return (phydrv->phy_id & phydrv->phy_id_mask) == + (phydev->phy_id & phydrv->phy_id_mask); } } diff --git a/include/linux/phy.h b/include/linux/phy.h index 2090277ea..e30ca2fdd 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -500,7 +500,7 @@ struct phy_driver { struct mdio_driver_common mdiodrv; u32 phy_id; char *name; - u32 phy_id_mask; + u32 phy_id_mask; /* value 0 means exact match */ const unsigned long * const features; u32 flags; const void *driver_data; -- 2.19.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: use phy_id_mask value zero for exact match 2018-11-07 20:53 ` [PATCH net-next 1/2] " Heiner Kallweit @ 2018-11-08 18:34 ` Andrew Lunn 2018-11-08 19:44 ` Florian Fainelli 1 sibling, 0 replies; 11+ messages in thread From: Andrew Lunn @ 2018-11-08 18:34 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev On Wed, Nov 07, 2018 at 09:53:32PM +0100, Heiner Kallweit wrote: > @@ -500,7 +500,7 @@ struct phy_driver { > struct mdio_driver_common mdiodrv; > u32 phy_id; > char *name; > - u32 phy_id_mask; > + u32 phy_id_mask; /* value 0 means exact match */ Hi Heiner Please make this comment part of the kerneldoc which is just above: * phy_id: The result of reading the UID registers of this PHY * type, and ANDing them with the phy_id_mask. Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: use phy_id_mask value zero for exact match 2018-11-07 20:53 ` [PATCH net-next 1/2] " Heiner Kallweit 2018-11-08 18:34 ` Andrew Lunn @ 2018-11-08 19:44 ` Florian Fainelli 2018-11-08 20:06 ` Heiner Kallweit 1 sibling, 1 reply; 11+ messages in thread From: Florian Fainelli @ 2018-11-08 19:44 UTC (permalink / raw) To: Heiner Kallweit, Andrew Lunn, David Miller; +Cc: netdev On 11/7/18 12:53 PM, Heiner Kallweit wrote: > A phy_id_mask value zero means every PHYID matches, therefore > value zero isn't used. So we can safely redefine the semantics > of value zero to mean "exact match". This allows to avoid some > boilerplate code in PHY driver configs. Having run recently into some ethtool quirkyness about how masks are supposed to be specified between ntuple/nfc, where the meaning of 0 is either don't care or match, I would rather we stick with the current behavior where every bit set to 0 is a don't care and bits set t 1 are not. Maybe we can find a clever way with a macro to specify only the PHY OUI and compute a suitable mask automatically? > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/phy/phy_device.c | 21 +++++++++++++++------ > include/linux/phy.h | 2 +- > 2 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index ab33d1777..d165a2c82 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -483,15 +483,24 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv) > if (!(phydev->c45_ids.devices_in_package & (1 << i))) > continue; > > - if ((phydrv->phy_id & phydrv->phy_id_mask) == > - (phydev->c45_ids.device_ids[i] & > - phydrv->phy_id_mask)) > - return 1; > + if (!phydrv->phy_id_mask) { > + if (phydrv->phy_id == > + phydev->c45_ids.device_ids[i]) > + return 1; > + } else { > + if ((phydrv->phy_id & phydrv->phy_id_mask) == > + (phydev->c45_ids.device_ids[i] & > + phydrv->phy_id_mask)) > + return 1; > + } > } > return 0; > } else { > - return (phydrv->phy_id & phydrv->phy_id_mask) == > - (phydev->phy_id & phydrv->phy_id_mask); > + if (!phydrv->phy_id_mask) > + return phydrv->phy_id == phydev->phy_id; > + else > + return (phydrv->phy_id & phydrv->phy_id_mask) == > + (phydev->phy_id & phydrv->phy_id_mask); > } > } > > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 2090277ea..e30ca2fdd 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -500,7 +500,7 @@ struct phy_driver { > struct mdio_driver_common mdiodrv; > u32 phy_id; > char *name; > - u32 phy_id_mask; > + u32 phy_id_mask; /* value 0 means exact match */ > const unsigned long * const features; > u32 flags; > const void *driver_data; > -- Florian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: use phy_id_mask value zero for exact match 2018-11-08 19:44 ` Florian Fainelli @ 2018-11-08 20:06 ` Heiner Kallweit 2018-11-08 20:53 ` Andrew Lunn 0 siblings, 1 reply; 11+ messages in thread From: Heiner Kallweit @ 2018-11-08 20:06 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev On 08.11.2018 20:44, Florian Fainelli wrote: > On 11/7/18 12:53 PM, Heiner Kallweit wrote: >> A phy_id_mask value zero means every PHYID matches, therefore >> value zero isn't used. So we can safely redefine the semantics >> of value zero to mean "exact match". This allows to avoid some >> boilerplate code in PHY driver configs. > > Having run recently into some ethtool quirkyness about how masks are > supposed to be specified between ntuple/nfc, where the meaning of 0 is > either don't care or match, I would rather we stick with the current > behavior where every bit set to 0 is a don't care and bits set t 1 are not. > I agree that using a mask value 0 as either exact match or don't care can be confusing. However I think that the advantages here outweigh this confusion aspect. - We get a meaningful default in case a driver author misses to set the phy_id_mask. - If a driver author wants to enforce an exact match, he has to do nothing and can rely on the core. This avoids mistakes like in the Realtek case where the driver author meant exact match but specified something completely different. - Avoid boilerplate code > Maybe we can find a clever way with a macro to specify only the PHY OUI > and compute a suitable mask automatically? > I don't think so. For Realtek each driver is specific even to a model revision (therefore mask 0xffffffff). Same applies to intel-xway. In broadcom.c we have masks 0xfffffff0, so for each model, independent of revision number. There is no general rule. Also we can't simply check for the first-bit-set to derive a mask. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/phy/phy_device.c | 21 +++++++++++++++------ >> include/linux/phy.h | 2 +- >> 2 files changed, 16 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index ab33d1777..d165a2c82 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -483,15 +483,24 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv) >> if (!(phydev->c45_ids.devices_in_package & (1 << i))) >> continue; >> >> - if ((phydrv->phy_id & phydrv->phy_id_mask) == >> - (phydev->c45_ids.device_ids[i] & >> - phydrv->phy_id_mask)) >> - return 1; >> + if (!phydrv->phy_id_mask) { >> + if (phydrv->phy_id == >> + phydev->c45_ids.device_ids[i]) >> + return 1; >> + } else { >> + if ((phydrv->phy_id & phydrv->phy_id_mask) == >> + (phydev->c45_ids.device_ids[i] & >> + phydrv->phy_id_mask)) >> + return 1; >> + } >> } >> return 0; >> } else { >> - return (phydrv->phy_id & phydrv->phy_id_mask) == >> - (phydev->phy_id & phydrv->phy_id_mask); >> + if (!phydrv->phy_id_mask) >> + return phydrv->phy_id == phydev->phy_id; >> + else >> + return (phydrv->phy_id & phydrv->phy_id_mask) == >> + (phydev->phy_id & phydrv->phy_id_mask); >> } >> } >> >> diff --git a/include/linux/phy.h b/include/linux/phy.h >> index 2090277ea..e30ca2fdd 100644 >> --- a/include/linux/phy.h >> +++ b/include/linux/phy.h >> @@ -500,7 +500,7 @@ struct phy_driver { >> struct mdio_driver_common mdiodrv; >> u32 phy_id; >> char *name; >> - u32 phy_id_mask; >> + u32 phy_id_mask; /* value 0 means exact match */ >> const unsigned long * const features; >> u32 flags; >> const void *driver_data; >> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: use phy_id_mask value zero for exact match 2018-11-08 20:06 ` Heiner Kallweit @ 2018-11-08 20:53 ` Andrew Lunn 2018-11-08 23:06 ` Florian Fainelli 0 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2018-11-08 20:53 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev > > Maybe we can find a clever way with a macro to specify only the PHY OUI > > and compute a suitable mask automatically? > > > I don't think so. For Realtek each driver is specific even to a model > revision (therefore mask 0xffffffff). Same applies to intel-xway. > In broadcom.c we have masks 0xfffffff0, so for each model, independent > of revision number. There is no general rule. > Also we can't simply check for the first-bit-set to derive a mask. I'm crystal ball gazing, but i think Florian was meaning something like #define PHY_ID_UNIQUE(_id) \ .phy_id = _id_; \ .phy_id_mask = 0xffffffff; It is the boilerplate setting .phy_id_mask which you don't like. This removes that boilerplate. Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: use phy_id_mask value zero for exact match 2018-11-08 20:53 ` Andrew Lunn @ 2018-11-08 23:06 ` Florian Fainelli 0 siblings, 0 replies; 11+ messages in thread From: Florian Fainelli @ 2018-11-08 23:06 UTC (permalink / raw) To: Andrew Lunn, Heiner Kallweit; +Cc: David Miller, netdev On 11/8/18 12:53 PM, Andrew Lunn wrote: >>> Maybe we can find a clever way with a macro to specify only the PHY OUI >>> and compute a suitable mask automatically? >>> >> I don't think so. For Realtek each driver is specific even to a model >> revision (therefore mask 0xffffffff). Same applies to intel-xway. >> In broadcom.c we have masks 0xfffffff0, so for each model, independent >> of revision number. There is no general rule. >> Also we can't simply check for the first-bit-set to derive a mask. > > I'm crystal ball gazing, but i think Florian was meaning something like > > #define PHY_ID_UNIQUE(_id) \ > .phy_id = _id_; \ > .phy_id_mask = 0xffffffff; > > It is the boilerplate setting .phy_id_mask which you don't like. This removes that boilerplate. Your crystal ball gazing skills are good, that is what I meant, we could also define another macro which does not match the revision bits, and that would likely cover everything that is already out there. -- Florian ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs 2018-11-07 20:52 [PATCH net-next 0/2] net: phy: use phy_id_mask value zero for exact match Heiner Kallweit 2018-11-07 20:53 ` [PATCH net-next 1/2] " Heiner Kallweit @ 2018-11-07 20:54 ` Heiner Kallweit 2018-11-08 18:37 ` Andrew Lunn 2018-11-08 23:05 ` [PATCH net-next 0/2] net: phy: use phy_id_mask value zero for exact match David Miller 2 siblings, 1 reply; 11+ messages in thread From: Heiner Kallweit @ 2018-11-07 20:54 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev After a recent change phy_id_mask value 0 means "exact match". This allows to remove setting phy_id_mask values from the driver configs. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/realtek.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index 7b1c89b38..abff4cdc9 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -215,13 +215,11 @@ static struct phy_driver realtek_drvs[] = { { .phy_id = 0x00008201, .name = "RTL8201CP Ethernet", - .phy_id_mask = 0x0000ffff, .features = PHY_BASIC_FEATURES, .flags = PHY_HAS_INTERRUPT, }, { .phy_id = 0x001cc816, .name = "RTL8201F Fast Ethernet", - .phy_id_mask = 0x001fffff, .features = PHY_BASIC_FEATURES, .flags = PHY_HAS_INTERRUPT, .ack_interrupt = &rtl8201_ack_interrupt, @@ -233,7 +231,6 @@ static struct phy_driver realtek_drvs[] = { }, { .phy_id = 0x001cc910, .name = "RTL8211 Gigabit Ethernet", - .phy_id_mask = 0x001fffff, .features = PHY_GBIT_FEATURES, .config_aneg = rtl8211_config_aneg, .read_mmd = &genphy_read_mmd_unsupported, @@ -241,7 +238,6 @@ static struct phy_driver realtek_drvs[] = { }, { .phy_id = 0x001cc912, .name = "RTL8211B Gigabit Ethernet", - .phy_id_mask = 0x001fffff, .features = PHY_GBIT_FEATURES, .flags = PHY_HAS_INTERRUPT, .ack_interrupt = &rtl821x_ack_interrupt, @@ -253,7 +249,6 @@ static struct phy_driver realtek_drvs[] = { }, { .phy_id = 0x001cc913, .name = "RTL8211C Gigabit Ethernet", - .phy_id_mask = 0x001fffff, .features = PHY_GBIT_FEATURES, .config_init = rtl8211c_config_init, .read_mmd = &genphy_read_mmd_unsupported, @@ -261,7 +256,6 @@ static struct phy_driver realtek_drvs[] = { }, { .phy_id = 0x001cc914, .name = "RTL8211DN Gigabit Ethernet", - .phy_id_mask = 0x001fffff, .features = PHY_GBIT_FEATURES, .flags = PHY_HAS_INTERRUPT, .ack_interrupt = rtl821x_ack_interrupt, @@ -271,7 +265,6 @@ static struct phy_driver realtek_drvs[] = { }, { .phy_id = 0x001cc915, .name = "RTL8211E Gigabit Ethernet", - .phy_id_mask = 0x001fffff, .features = PHY_GBIT_FEATURES, .flags = PHY_HAS_INTERRUPT, .ack_interrupt = &rtl821x_ack_interrupt, @@ -281,7 +274,6 @@ static struct phy_driver realtek_drvs[] = { }, { .phy_id = 0x001cc916, .name = "RTL8211F Gigabit Ethernet", - .phy_id_mask = 0x001fffff, .features = PHY_GBIT_FEATURES, .flags = PHY_HAS_INTERRUPT, .config_init = &rtl8211f_config_init, @@ -294,7 +286,6 @@ static struct phy_driver realtek_drvs[] = { }, { .phy_id = 0x001cc961, .name = "RTL8366RB Gigabit Ethernet", - .phy_id_mask = 0x001fffff, .features = PHY_GBIT_FEATURES, .flags = PHY_HAS_INTERRUPT, .config_init = &rtl8366rb_config_init, -- 2.19.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs 2018-11-07 20:54 ` [PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs Heiner Kallweit @ 2018-11-08 18:37 ` Andrew Lunn 2018-11-08 19:38 ` Heiner Kallweit 0 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2018-11-08 18:37 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev > { > .phy_id = 0x00008201, > .name = "RTL8201CP Ethernet", > - .phy_id_mask = 0x0000ffff, > .features = PHY_BASIC_FEATURES, > .flags = PHY_HAS_INTERRUPT, > }, { > .phy_id = 0x001cc816, > .name = "RTL8201F Fast Ethernet", > - .phy_id_mask = 0x001fffff, Hi Heiner "RTL8201CP Ethernet" has a mask of 0x0000ffff, where as all the others use 0x001fffff. Is this correct? Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs 2018-11-08 18:37 ` Andrew Lunn @ 2018-11-08 19:38 ` Heiner Kallweit 0 siblings, 0 replies; 11+ messages in thread From: Heiner Kallweit @ 2018-11-08 19:38 UTC (permalink / raw) To: Andrew Lunn; +Cc: Florian Fainelli, David Miller, netdev On 08.11.2018 19:37, Andrew Lunn wrote: >> { >> .phy_id = 0x00008201, >> .name = "RTL8201CP Ethernet", >> - .phy_id_mask = 0x0000ffff, >> .features = PHY_BASIC_FEATURES, >> .flags = PHY_HAS_INTERRUPT, >> }, { >> .phy_id = 0x001cc816, >> .name = "RTL8201F Fast Ethernet", >> - .phy_id_mask = 0x001fffff, > > Hi Heiner > > "RTL8201CP Ethernet" has a mask of 0x0000ffff, where as all the others > use 0x001fffff. Is this correct? > IMO none of the masks is correct. All of them should be 0xffffffff. Nowadays the 32 bit PHYID is assembled from (MSB to LSB): 22 bits vendor OUI, 6 bit model number, 4 bit revision number Just the old 8201 doesn't follow this scheme. With the current masks, a PHYID 0x12348201 would be recognized as Realtek 8201 too, what's obviously wrong. > Andrew > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 0/2] net: phy: use phy_id_mask value zero for exact match 2018-11-07 20:52 [PATCH net-next 0/2] net: phy: use phy_id_mask value zero for exact match Heiner Kallweit 2018-11-07 20:53 ` [PATCH net-next 1/2] " Heiner Kallweit 2018-11-07 20:54 ` [PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs Heiner Kallweit @ 2018-11-08 23:05 ` David Miller 2 siblings, 0 replies; 11+ messages in thread From: David Miller @ 2018-11-08 23:05 UTC (permalink / raw) To: hkallweit1; +Cc: f.fainelli, andrew, netdev From: Heiner Kallweit <hkallweit1@gmail.com> Date: Wed, 7 Nov 2018 21:52:31 +0100 > A phy_id_mask value zero means every PHYID matches, therefore > value zero isn't used. So we can safely redefine the semantics > of value zero to mean "exact match". This allows to avoid some > boilerplate code in PHY driver configs. > > Realtek PHY driver is the first user of this change. It looks like there will be some changes to this series, so I will wait for the next version. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-11-09 8:44 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-07 20:52 [PATCH net-next 0/2] net: phy: use phy_id_mask value zero for exact match Heiner Kallweit 2018-11-07 20:53 ` [PATCH net-next 1/2] " Heiner Kallweit 2018-11-08 18:34 ` Andrew Lunn 2018-11-08 19:44 ` Florian Fainelli 2018-11-08 20:06 ` Heiner Kallweit 2018-11-08 20:53 ` Andrew Lunn 2018-11-08 23:06 ` Florian Fainelli 2018-11-07 20:54 ` [PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs Heiner Kallweit 2018-11-08 18:37 ` Andrew Lunn 2018-11-08 19:38 ` Heiner Kallweit 2018-11-08 23:05 ` [PATCH net-next 0/2] net: phy: use phy_id_mask value zero for exact match David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.