This is an alternative implementation of "net: phy: detach PHY driver OPs
from phy_driver struct" with the same object in mind.
As was pointed out in the previous series, deatching OPs is a way too big
change (although IMHO needed, but I understand the problem with downstream
and ugly code). As suggested and was already an idea discussed privately,
a more easier approach is introduce an alternative way in phy_driver
struct to declare PHY with the use of an array of IDs.
Some small changes were needed to mod_devicetable to make mdio_device_id
more suitable for the task.
This implementation keep the single way to declare PHY ID in phy_driver
but also indotruce .ids where a table of mdio_device_id can be defined.
Each entry can optionally have a .name variable to define a more specific
PHY name (for phydev_info/err.. usage) that if detected will overwrite
the dev name.
An example of this name is a phy_driver with a .name "Aquantia 107/102"
and .ids with single mdio_device_id with "Aquantia 107" and "Aquantia 102"
"Aquantia 107/102" will be used for early PHY driver probe and the more
specific name will be used as soon as the phydev dev_id is populated.
Aquantia driver is reworked to this implementation and BCM7xxx driver
table is rewritten to greatly benefits from this implementation.
While at it I also notice a strange problem with detected PHY ID and
C45 PHYs. Probably i will have to drop it, but including in this series
just to make someone aware and maybe discuss about it too?
Christian Marangi (6):
net: phy: add support for defining multiple PHY IDs in PHY driver
net: phy: fill phy_id with C45 PHY
mod_devicetable: permit to define a name for an mdio_device_id
net: phy: support named mdio_device_id PHY IDs
net: phy: aquantia: group common OPs for PHYs where possible
net: phy: bcm7xxx: rework phy_driver table to new multiple PHY ID
format
drivers/net/phy/aquantia/aquantia_main.c | 170 +++++++++--------------
drivers/net/phy/bcm7xxx.c | 140 +++++++++++--------
drivers/net/phy/phy_device.c | 104 +++++++++++---
include/linux/mod_devicetable.h | 2 +
include/linux/phy.h | 8 +-
5 files changed, 243 insertions(+), 181 deletions(-)
--
2.43.0
With C45 PHYs that provide PHY ID in C45 Package regs, PHY device
phy_id is not filled.
Correctly fill .phy_id from matching dev_id or phy_driver info.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/phy_device.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9b96357e4de8..60a60f182729 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3476,6 +3476,10 @@ static int phy_probe(struct device *dev)
phy_dev_id->phy_id_mask = phydrv->phy_id_mask;
}
+ /* Fill PHY ID with dev_id if empty and PHY is C45 */
+ if (!phydev->phy_id && phydev->is_c45)
+ phydev->phy_id = phy_dev_id->phy_id;
+
/* Disable the interrupt if the PHY doesn't support it
* but the interrupt is still a valid one
*/
--
2.43.0
Some PHY driver might implement the same OPs for different PHY ID and
using a mask is not enough to match similar PHYs.
To reduce code duplication, add support for defining multiple PHY IDs in
PHY driver struct.
Introduce a new variable in phy_driver struct, .ids, where a table array of
mdio_device_id can be defined to reference multiple PHY IDs (with their
own masks) supporting the same group of OPs and flags.
Introduce a new variable in phy_device, .dev_id, where the matching
mdio_device_id is stored. PHYs supporting multiple PHYs for one PHY
driver struct, should use this instead of matching for phy_id.
Single PHY ID implementation is still supported and dev_id is filled
with the data from phy_driver in this case.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/phy_device.c | 94 ++++++++++++++++++++++++++++++------
include/linux/phy.h | 8 ++-
2 files changed, 85 insertions(+), 17 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d63dca535746..9b96357e4de8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -522,12 +522,74 @@ static int phy_scan_fixups(struct phy_device *phydev)
return 0;
}
+static int phy_driver_match_id(struct phy_driver *phydrv, u32 id,
+ const struct mdio_device_id **dev_id)
+{
+ const struct mdio_device_id *ids = phydrv->ids;
+
+ /* PHY driver might provide an array of different PHY IDs and
+ * masks. Walk them if this is the case and compare with ID.
+ */
+ if (ids) {
+ /* From mdio_device_id struct phy_id_mask MUST
+ * be used as sentinel.
+ */
+ while (ids->phy_id_mask) {
+ if (phy_id_compare(id, ids->phy_id, ids->phy_id_mask)) {
+ if (dev_id)
+ *dev_id = ids;
+
+ return 1;
+ }
+
+ ids++;
+ }
+ }
+
+ if (phy_id_compare(id, phydrv->phy_id, phydrv->phy_id_mask))
+ return 1;
+
+ return 0;
+}
+
+/**
+ * phy_driver_match - match a phydriver with a given PHY istance
+ * @phydrv: PHY driver to compare with
+ * @phydev: PHY istance to use for comparison. Either PHY ID will be used or
+ * with C45 PHY ID is extracted from Package regs.
+ * @dev_id: Pointer where to store pointer to a matchin mdio_device_id.
+ * mdio_device_id are assumed to be statically allocated for each PHY driver,
+ * hence the reference to this struct is returned here.
+ *
+ * Returns 1 if matching, 0 otherwise. dev_id can be passed as NULL to skip
+ * referecing a matching mdio_device_id if found.
+ */
+static int phy_driver_match(struct phy_driver *phydrv, struct phy_device *phydev,
+ const struct mdio_device_id **dev_id)
+{
+ const int num_ids = ARRAY_SIZE(phydev->c45_ids.device_ids);
+ int i;
+
+ if (!phydev->is_c45)
+ return phy_driver_match_id(phydrv, phydev->phy_id,
+ dev_id);
+
+ for (i = 1; i < num_ids; i++) {
+ if (phydev->c45_ids.device_ids[i] == 0xffffffff)
+ continue;
+
+ if (phy_driver_match_id(phydrv, phydev->c45_ids.device_ids[i],
+ dev_id))
+ return 1;
+ }
+
+ return 0;
+}
+
static int phy_bus_match(struct device *dev, struct device_driver *drv)
{
struct phy_device *phydev = to_phy_device(dev);
struct phy_driver *phydrv = to_phy_driver(drv);
- const int num_ids = ARRAY_SIZE(phydev->c45_ids.device_ids);
- int i;
if (!(phydrv->mdiodrv.flags & MDIO_DEVICE_IS_PHY))
return 0;
@@ -535,20 +597,7 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv)
if (phydrv->match_phy_device)
return phydrv->match_phy_device(phydev);
- if (phydev->is_c45) {
- for (i = 1; i < num_ids; i++) {
- if (phydev->c45_ids.device_ids[i] == 0xffffffff)
- continue;
-
- if (phy_id_compare(phydev->c45_ids.device_ids[i],
- phydrv->phy_id, phydrv->phy_id_mask))
- return 1;
- }
- return 0;
- } else {
- return phy_id_compare(phydev->phy_id, phydrv->phy_id,
- phydrv->phy_id_mask);
- }
+ return phy_driver_match(phydrv, phydev, NULL);
}
static ssize_t
@@ -3410,9 +3459,22 @@ static int phy_probe(struct device *dev)
struct phy_device *phydev = to_phy_device(dev);
struct device_driver *drv = phydev->mdio.dev.driver;
struct phy_driver *phydrv = to_phy_driver(drv);
+ const struct mdio_device_id *dev_id = NULL;
+ struct mdio_device_id *phy_dev_id;
int err = 0;
phydev->drv = phydrv;
+ phy_dev_id = (struct mdio_device_id *)&phydev->dev_id;
+ /* Fill the mdio_device_id for the PHY istance.
+ * If PHY driver provide an array of PHYs, search the right one,
+ * in the other case fill it with the phy_driver data.
+ */
+ if (phy_driver_match(phydrv, phydev, &dev_id) && dev_id) {
+ memcpy(phy_dev_id, dev_id, sizeof(*dev_id));
+ } else {
+ phy_dev_id->phy_id = phydrv->phy_id;
+ phy_dev_id->phy_id_mask = phydrv->phy_id_mask;
+ }
/* Disable the interrupt if the PHY doesn't support it
* but the interrupt is still a valid one
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c2dda21b39e1..f0313b9e0173 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -547,6 +547,7 @@ struct macsec_ops;
* @drv: Pointer to the driver for this PHY instance
* @devlink: Create a link between phy dev and mac dev, if the external phy
* used by current mac interface is managed by another mac interface.
+ * @dev_id: The matched device ID for this PHY instance
* @phy_id: UID for this device found during discovery
* @c45_ids: 802.3-c45 Device Identifiers if is_c45.
* @is_c45: Set to true if this PHY uses clause 45 addressing.
@@ -645,6 +646,7 @@ struct phy_device {
struct device_link *devlink;
+ const struct mdio_device_id dev_id;
u32 phy_id;
struct phy_c45_device_ids c45_ids;
@@ -885,6 +887,8 @@ struct phy_led {
* struct phy_driver - Driver structure for a particular PHY type
*
* @mdiodrv: Data common to all MDIO devices
+ * @ids: array of mdio device IDs to match this driver (terminated with
+ * zero phy_id_mask)
* @phy_id: The result of reading the UID registers of this PHY
* type, and ANDing them with the phy_id_mask. This driver
* only works for PHYs with IDs which match this field
@@ -906,6 +910,7 @@ struct phy_led {
*/
struct phy_driver {
struct mdio_driver_common mdiodrv;
+ const struct mdio_device_id *ids;
u32 phy_id;
char *name;
u32 phy_id_mask;
@@ -1206,7 +1211,8 @@ static inline bool phy_id_compare(u32 id1, u32 id2, u32 mask)
*/
static inline bool phydev_id_compare(struct phy_device *phydev, u32 id)
{
- return phy_id_compare(id, phydev->phy_id, phydev->drv->phy_id_mask);
+ return phy_id_compare(id, phydev->dev_id.phy_id,
+ phydev->dev_id.phy_id_mask);
}
/* A Structure for boards to register fixups with the PHY Lib */
--
2.43.0
Permit to optionally define a name for an mdio_device_id. This can be
used for PHY driver that might define multiple PHY IDs for the same group
of PHY driver OPs to define different names for each PHY ID and better
identify the different models at runtime.
Signed-off-by: Christian Marangi <[email protected]>
---
include/linux/mod_devicetable.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index f458469c5ce5..9dc6f0cc26b4 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -630,10 +630,12 @@ struct platform_device_id {
* for this PHY type
* @phy_id_mask: Defines the significant bits of @phy_id. A value of 0
* is used to terminate an array of struct mdio_device_id.
+ * @name: Optional Friendly name that identify the PHY device/family.
*/
struct mdio_device_id {
__u32 phy_id;
__u32 phy_id_mask;
+ const char *name;
};
struct zorro_device_id {
--
2.43.0
PHY IDs defined in PHY driver .ids can have a more specific name
defined. If this is the case, overwrite the PHY istance dev name with
the more specific one from the matching dev_id.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/phy_device.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 60a60f182729..9e359e304f91 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3471,9 +3471,15 @@ static int phy_probe(struct device *dev)
*/
if (phy_driver_match(phydrv, phydev, &dev_id) && dev_id) {
memcpy(phy_dev_id, dev_id, sizeof(*dev_id));
+ /* If defined, overwrite the PHY driver dev name with a
+ * more specific one from the matching dev_id.
+ */
+ if (dev_id->name)
+ drv->name = dev_id->name;
} else {
phy_dev_id->phy_id = phydrv->phy_id;
phy_dev_id->phy_id_mask = phydrv->phy_id_mask;
+ phy_dev_id->name = phydrv->name;
}
/* Fill PHY ID with dev_id if empty and PHY is C45 */
--
2.43.0
Group common OPS for PHY where possible by defining multiple PHYs for
similar PHY drivers instead of duplicating them for each PHY.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/aquantia/aquantia_main.c | 170 +++++++++--------------
1 file changed, 64 insertions(+), 106 deletions(-)
diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index a6a7980585f5..4a788fc8e26a 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -770,16 +770,26 @@ static int aqr111_config_init(struct phy_device *phydev)
static struct phy_driver aqr_driver[] = {
{
- PHY_ID_MATCH_MODEL(PHY_ID_AQ1202),
- .name = "Aquantia AQ1202",
- .config_aneg = aqr_config_aneg,
- .config_intr = aqr_config_intr,
- .handle_interrupt = aqr_handle_interrupt,
- .read_status = aqr_read_status,
-},
-{
- PHY_ID_MATCH_MODEL(PHY_ID_AQ2104),
- .name = "Aquantia AQ2104",
+ .name = "Aquantia AQ1202/AQ2104/AQR106/AQR405",
+ .ids = (const struct mdio_device_id []){
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQ1202),
+ .name = "Aquantia AQ1202"
+ },
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQ2104),
+ .name = "Aquantia AQ2104",
+ },
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR106),
+ .name = "Aquantia AQR106",
+ },
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR405),
+ .name = "Aquantia AQR405",
+ },
+ { /* sentinel */ },
+ },
.config_aneg = aqr_config_aneg,
.config_intr = aqr_config_intr,
.handle_interrupt = aqr_handle_interrupt,
@@ -796,16 +806,22 @@ static struct phy_driver aqr_driver[] = {
.resume = aqr107_resume,
},
{
- PHY_ID_MATCH_MODEL(PHY_ID_AQR106),
- .name = "Aquantia AQR106",
- .config_aneg = aqr_config_aneg,
- .config_intr = aqr_config_intr,
- .handle_interrupt = aqr_handle_interrupt,
- .read_status = aqr_read_status,
-},
-{
- PHY_ID_MATCH_MODEL(PHY_ID_AQR107),
- .name = "Aquantia AQR107",
+ .name = "Aquantia AQR107/AQR112/AQR412",
+ .ids = (const struct mdio_device_id []){
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR107),
+ .name = "Aquantia AQR107"
+ },
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR112),
+ .name = "Aquantia AQR112",
+ },
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR412),
+ .name = "Aquantia AQR412",
+ },
+ { /* sentinel */ },
+ },
.probe = aqr107_probe,
.get_rate_matching = aqr107_get_rate_matching,
.config_init = aqr107_config_init,
@@ -842,27 +858,22 @@ static struct phy_driver aqr_driver[] = {
.link_change_notify = aqr107_link_change_notify,
},
{
- PHY_ID_MATCH_MODEL(PHY_ID_AQR111),
- .name = "Aquantia AQR111",
- .probe = aqr107_probe,
- .get_rate_matching = aqr107_get_rate_matching,
- .config_init = aqr111_config_init,
- .config_aneg = aqr_config_aneg,
- .config_intr = aqr_config_intr,
- .handle_interrupt = aqr_handle_interrupt,
- .read_status = aqr107_read_status,
- .get_tunable = aqr107_get_tunable,
- .set_tunable = aqr107_set_tunable,
- .suspend = aqr107_suspend,
- .resume = aqr107_resume,
- .get_sset_count = aqr107_get_sset_count,
- .get_strings = aqr107_get_strings,
- .get_stats = aqr107_get_stats,
- .link_change_notify = aqr107_link_change_notify,
-},
-{
- PHY_ID_MATCH_MODEL(PHY_ID_AQR111B0),
- .name = "Aquantia AQR111B0",
+ .name = "Aquantia AQR111/AQR111B0",
+ .ids = (const struct mdio_device_id []){
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR111),
+ .name = "Aquantia AQR111"
+ },
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR111B0),
+ .name = "Aquantia AQR111B0",
+ },
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR106),
+ .name = "Aquantia AQR106",
+ },
+ { /* sentinel */ },
+ },
.probe = aqr107_probe,
.get_rate_matching = aqr107_get_rate_matching,
.config_init = aqr111_config_init,
@@ -880,71 +891,18 @@ static struct phy_driver aqr_driver[] = {
.link_change_notify = aqr107_link_change_notify,
},
{
- PHY_ID_MATCH_MODEL(PHY_ID_AQR405),
- .name = "Aquantia AQR405",
- .config_aneg = aqr_config_aneg,
- .config_intr = aqr_config_intr,
- .handle_interrupt = aqr_handle_interrupt,
- .read_status = aqr_read_status,
-},
-{
- PHY_ID_MATCH_MODEL(PHY_ID_AQR112),
- .name = "Aquantia AQR112",
- .probe = aqr107_probe,
- .config_aneg = aqr_config_aneg,
- .config_intr = aqr_config_intr,
- .handle_interrupt = aqr_handle_interrupt,
- .get_tunable = aqr107_get_tunable,
- .set_tunable = aqr107_set_tunable,
- .suspend = aqr107_suspend,
- .resume = aqr107_resume,
- .read_status = aqr107_read_status,
- .get_rate_matching = aqr107_get_rate_matching,
- .get_sset_count = aqr107_get_sset_count,
- .get_strings = aqr107_get_strings,
- .get_stats = aqr107_get_stats,
- .link_change_notify = aqr107_link_change_notify,
-},
-{
- PHY_ID_MATCH_MODEL(PHY_ID_AQR412),
- .name = "Aquantia AQR412",
- .probe = aqr107_probe,
- .config_aneg = aqr_config_aneg,
- .config_intr = aqr_config_intr,
- .handle_interrupt = aqr_handle_interrupt,
- .get_tunable = aqr107_get_tunable,
- .set_tunable = aqr107_set_tunable,
- .suspend = aqr107_suspend,
- .resume = aqr107_resume,
- .read_status = aqr107_read_status,
- .get_rate_matching = aqr107_get_rate_matching,
- .get_sset_count = aqr107_get_sset_count,
- .get_strings = aqr107_get_strings,
- .get_stats = aqr107_get_stats,
- .link_change_notify = aqr107_link_change_notify,
-},
-{
- PHY_ID_MATCH_MODEL(PHY_ID_AQR113),
- .name = "Aquantia AQR113",
- .probe = aqr107_probe,
- .get_rate_matching = aqr107_get_rate_matching,
- .config_init = aqr113c_config_init,
- .config_aneg = aqr_config_aneg,
- .config_intr = aqr_config_intr,
- .handle_interrupt = aqr_handle_interrupt,
- .read_status = aqr107_read_status,
- .get_tunable = aqr107_get_tunable,
- .set_tunable = aqr107_set_tunable,
- .suspend = aqr107_suspend,
- .resume = aqr107_resume,
- .get_sset_count = aqr107_get_sset_count,
- .get_strings = aqr107_get_strings,
- .get_stats = aqr107_get_stats,
- .link_change_notify = aqr107_link_change_notify,
-},
-{
- PHY_ID_MATCH_MODEL(PHY_ID_AQR113C),
- .name = "Aquantia AQR113C",
+ .name = "Aquantia AQR113/AQR113C",
+ .ids = (const struct mdio_device_id []){
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR113),
+ .name = "Aquantia AQR113"
+ },
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR113C),
+ .name = "Aquantia AQR113C",
+ },
+ { /* sentinel */ },
+ },
.probe = aqr107_probe,
.get_rate_matching = aqr107_get_rate_matching,
.config_init = aqr113c_config_init,
--
2.43.0
Rework bcm7xxx PHY driver table to new multiple PHY format
implementation to reduce code duplication and final size of the compiled
module.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/bcm7xxx.c | 140 ++++++++++++++++++++++----------------
1 file changed, 82 insertions(+), 58 deletions(-)
diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
index 97638ba7ae85..4d886bb8a3e2 100644
--- a/drivers/net/phy/bcm7xxx.c
+++ b/drivers/net/phy/bcm7xxx.c
@@ -845,16 +845,6 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
.phy_id = (_oui), \
.phy_id_mask = 0xfffffff0, \
.name = _name, \
- /* PHY_GBIT_FEATURES */ \
- .flags = PHY_IS_INTERNAL, \
- .config_init = bcm7xxx_28nm_config_init, \
- .resume = bcm7xxx_28nm_resume, \
- .get_tunable = bcm7xxx_28nm_get_tunable, \
- .set_tunable = bcm7xxx_28nm_set_tunable, \
- .get_sset_count = bcm_phy_get_sset_count, \
- .get_strings = bcm_phy_get_strings, \
- .get_stats = bcm7xxx_28nm_get_phy_stats, \
- .probe = bcm7xxx_28nm_probe, \
}
#define BCM7XXX_28NM_EPHY(_oui, _name) \
@@ -862,16 +852,6 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
.phy_id = (_oui), \
.phy_id_mask = 0xfffffff0, \
.name = _name, \
- /* PHY_BASIC_FEATURES */ \
- .flags = PHY_IS_INTERNAL, \
- .config_init = bcm7xxx_28nm_ephy_config_init, \
- .resume = bcm7xxx_28nm_ephy_resume, \
- .get_sset_count = bcm_phy_get_sset_count, \
- .get_strings = bcm_phy_get_strings, \
- .get_stats = bcm7xxx_28nm_get_phy_stats, \
- .probe = bcm7xxx_28nm_probe, \
- .read_mmd = bcm7xxx_28nm_ephy_read_mmd, \
- .write_mmd = bcm7xxx_28nm_ephy_write_mmd, \
}
#define BCM7XXX_40NM_EPHY(_oui, _name) \
@@ -879,12 +859,6 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
.phy_id = (_oui), \
.phy_id_mask = 0xfffffff0, \
.name = _name, \
- /* PHY_BASIC_FEATURES */ \
- .flags = PHY_IS_INTERNAL, \
- .soft_reset = genphy_soft_reset, \
- .config_init = bcm7xxx_config_init, \
- .suspend = bcm7xxx_suspend, \
- .resume = bcm7xxx_config_init, \
}
#define BCM7XXX_16NM_EPHY(_oui, _name) \
@@ -892,41 +866,91 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
.phy_id = (_oui), \
.phy_id_mask = 0xfffffff0, \
.name = _name, \
- /* PHY_BASIC_FEATURES */ \
- .flags = PHY_IS_INTERNAL, \
- .get_sset_count = bcm_phy_get_sset_count, \
- .get_strings = bcm_phy_get_strings, \
- .get_stats = bcm7xxx_28nm_get_phy_stats, \
- .probe = bcm7xxx_28nm_probe, \
- .config_init = bcm7xxx_16nm_ephy_config_init, \
- .config_aneg = genphy_config_aneg, \
- .read_status = genphy_read_status, \
- .resume = bcm7xxx_16nm_ephy_resume, \
}
static struct phy_driver bcm7xxx_driver[] = {
- BCM7XXX_28NM_EPHY(PHY_ID_BCM72113, "Broadcom BCM72113"),
- BCM7XXX_28NM_EPHY(PHY_ID_BCM72116, "Broadcom BCM72116"),
- BCM7XXX_16NM_EPHY(PHY_ID_BCM72165, "Broadcom BCM72165"),
- BCM7XXX_28NM_GPHY(PHY_ID_BCM7250, "Broadcom BCM7250"),
- BCM7XXX_28NM_EPHY(PHY_ID_BCM7255, "Broadcom BCM7255"),
- BCM7XXX_28NM_EPHY(PHY_ID_BCM7260, "Broadcom BCM7260"),
- BCM7XXX_28NM_EPHY(PHY_ID_BCM7268, "Broadcom BCM7268"),
- BCM7XXX_28NM_EPHY(PHY_ID_BCM7271, "Broadcom BCM7271"),
- BCM7XXX_28NM_GPHY(PHY_ID_BCM7278, "Broadcom BCM7278"),
- BCM7XXX_28NM_GPHY(PHY_ID_BCM7364, "Broadcom BCM7364"),
- BCM7XXX_28NM_GPHY(PHY_ID_BCM7366, "Broadcom BCM7366"),
- BCM7XXX_16NM_EPHY(PHY_ID_BCM74165, "Broadcom BCM74165"),
- BCM7XXX_28NM_GPHY(PHY_ID_BCM74371, "Broadcom BCM74371"),
- BCM7XXX_28NM_GPHY(PHY_ID_BCM7439, "Broadcom BCM7439"),
- BCM7XXX_28NM_GPHY(PHY_ID_BCM7439_2, "Broadcom BCM7439 (2)"),
- BCM7XXX_28NM_GPHY(PHY_ID_BCM7445, "Broadcom BCM7445"),
- BCM7XXX_40NM_EPHY(PHY_ID_BCM7346, "Broadcom BCM7346"),
- BCM7XXX_40NM_EPHY(PHY_ID_BCM7362, "Broadcom BCM7362"),
- BCM7XXX_40NM_EPHY(PHY_ID_BCM7425, "Broadcom BCM7425"),
- BCM7XXX_40NM_EPHY(PHY_ID_BCM7429, "Broadcom BCM7429"),
- BCM7XXX_40NM_EPHY(PHY_ID_BCM7435, "Broadcom BCM7435"),
- BCM7XXX_16NM_EPHY(PHY_ID_BCM7712, "Broadcom BCM7712"),
+{
+ .name = "Broadcom BCM7XXX 16NM EPHY",
+ .ids = (const struct mdio_device_id []){
+ BCM7XXX_16NM_EPHY(PHY_ID_BCM72165, "Broadcom BCM72165"),
+ BCM7XXX_16NM_EPHY(PHY_ID_BCM74165, "Broadcom BCM74165"),
+ BCM7XXX_16NM_EPHY(PHY_ID_BCM7712, "Broadcom BCM7712"),
+ { /* sentinel */ },
+ },
+ /* PHY_BASIC_FEATURES */
+ .flags = PHY_IS_INTERNAL,
+ .get_sset_count = bcm_phy_get_sset_count,
+ .get_strings = bcm_phy_get_strings,
+ .get_stats = bcm7xxx_28nm_get_phy_stats,
+ .probe = bcm7xxx_28nm_probe,
+ .config_init = bcm7xxx_16nm_ephy_config_init,
+ .config_aneg = genphy_config_aneg,
+ .read_status = genphy_read_status,
+ .resume = bcm7xxx_16nm_ephy_resume,
+},
+{
+ .name = "Broadcom BCM7XXX 28NM GPHY",
+ .ids = (const struct mdio_device_id []){
+ BCM7XXX_28NM_GPHY(PHY_ID_BCM7250, "Broadcom BCM7250"),
+ BCM7XXX_28NM_GPHY(PHY_ID_BCM7278, "Broadcom BCM7278"),
+ BCM7XXX_28NM_GPHY(PHY_ID_BCM7364, "Broadcom BCM7364"),
+ BCM7XXX_28NM_GPHY(PHY_ID_BCM7366, "Broadcom BCM7366"),
+ BCM7XXX_28NM_GPHY(PHY_ID_BCM74371, "Broadcom BCM74371"),
+ BCM7XXX_28NM_GPHY(PHY_ID_BCM7439, "Broadcom BCM7439"),
+ BCM7XXX_28NM_GPHY(PHY_ID_BCM7439_2, "Broadcom BCM7439 (2)"),
+ BCM7XXX_28NM_GPHY(PHY_ID_BCM7445, "Broadcom BCM7445"),
+ { /* sentinel */ },
+ },
+ /* PHY_GBIT_FEATURES */
+ .flags = PHY_IS_INTERNAL,
+ .config_init = bcm7xxx_28nm_config_init,
+ .resume = bcm7xxx_28nm_resume,
+ .get_tunable = bcm7xxx_28nm_get_tunable,
+ .set_tunable = bcm7xxx_28nm_set_tunable,
+ .get_sset_count = bcm_phy_get_sset_count,
+ .get_strings = bcm_phy_get_strings,
+ .get_stats = bcm7xxx_28nm_get_phy_stats,
+ .probe = bcm7xxx_28nm_probe,
+},
+{
+ .name = "Broadcom BCM7XXX 28NM EPHY",
+ .ids = (const struct mdio_device_id []){
+ BCM7XXX_28NM_EPHY(PHY_ID_BCM72113, "Broadcom BCM72113"),
+ BCM7XXX_28NM_EPHY(PHY_ID_BCM72116, "Broadcom BCM72116"),
+ BCM7XXX_28NM_EPHY(PHY_ID_BCM7255, "Broadcom BCM7255"),
+ BCM7XXX_28NM_EPHY(PHY_ID_BCM7260, "Broadcom BCM7260"),
+ BCM7XXX_28NM_EPHY(PHY_ID_BCM7268, "Broadcom BCM7268"),
+ BCM7XXX_28NM_EPHY(PHY_ID_BCM7271, "Broadcom BCM7271"),
+ { /* sentinel */ },
+ },
+ /* PHY_BASIC_FEATURES */
+ .flags = PHY_IS_INTERNAL,
+ .config_init = bcm7xxx_28nm_ephy_config_init,
+ .resume = bcm7xxx_28nm_ephy_resume,
+ .get_sset_count = bcm_phy_get_sset_count,
+ .get_strings = bcm_phy_get_strings,
+ .get_stats = bcm7xxx_28nm_get_phy_stats,
+ .probe = bcm7xxx_28nm_probe,
+ .read_mmd = bcm7xxx_28nm_ephy_read_mmd,
+ .write_mmd = bcm7xxx_28nm_ephy_write_mmd,
+},
+{
+ .name = "Broadcom BCM7XXX 40NM EPHY",
+ .ids = (const struct mdio_device_id []){
+ BCM7XXX_40NM_EPHY(PHY_ID_BCM7346, "Broadcom BCM7346"),
+ BCM7XXX_40NM_EPHY(PHY_ID_BCM7362, "Broadcom BCM7362"),
+ BCM7XXX_40NM_EPHY(PHY_ID_BCM7425, "Broadcom BCM7425"),
+ BCM7XXX_40NM_EPHY(PHY_ID_BCM7429, "Broadcom BCM7429"),
+ BCM7XXX_40NM_EPHY(PHY_ID_BCM7435, "Broadcom BCM7435"),
+ { /* sentinel */ },
+ },
+ /* PHY_BASIC_FEATURES */
+ .flags = PHY_IS_INTERNAL,
+ .soft_reset = genphy_soft_reset,
+ .config_init = bcm7xxx_config_init,
+ .suspend = bcm7xxx_suspend,
+ .resume = bcm7xxx_config_init,
+},
};
static struct mdio_device_id __maybe_unused bcm7xxx_tbl[] = {
--
2.43.0
On Sun, Feb 18, 2024 at 08:00:27PM +0100, Christian Marangi wrote:
> Some PHY driver might implement the same OPs for different PHY ID and
> using a mask is not enough to match similar PHYs.
>
> To reduce code duplication, add support for defining multiple PHY IDs in
> PHY driver struct.
>
> Introduce a new variable in phy_driver struct, .ids, where a table array of
> mdio_device_id can be defined to reference multiple PHY IDs (with their
> own masks) supporting the same group of OPs and flags.
>
> Introduce a new variable in phy_device, .dev_id, where the matching
> mdio_device_id is stored. PHYs supporting multiple PHYs for one PHY
> driver struct, should use this instead of matching for phy_id.
>
> Single PHY ID implementation is still supported and dev_id is filled
> with the data from phy_driver in this case.
This looks like it's been reworked somewhat with my suggestion, or maybe
we just came across a similar structure for comparing the IDs?
> + phy_dev_id = (struct mdio_device_id *)&phydev->dev_id;
Why this cast? Try to write code that doesn't need casts.
> + /* Fill the mdio_device_id for the PHY istance.
> + * If PHY driver provide an array of PHYs, search the right one,
> + * in the other case fill it with the phy_driver data.
> + */
> + if (phy_driver_match(phydrv, phydev, &dev_id) && dev_id) {
> + memcpy(phy_dev_id, dev_id, sizeof(*dev_id));
> + } else {
> + phy_dev_id->phy_id = phydrv->phy_id;
> + phy_dev_id->phy_id_mask = phydrv->phy_id_mask;
So this is the _driver_ phy_id.
> static inline bool phydev_id_compare(struct phy_device *phydev, u32 id)
> {
> - return phy_id_compare(id, phydev->phy_id, phydev->drv->phy_id_mask);
> + return phy_id_compare(id, phydev->dev_id.phy_id,
> + phydev->dev_id.phy_id_mask);
And thus this code is now different (since it _was_ comparing the
phydev phy_id, and you've changed it to effectively the driver's
phy_id. While that should be the same for a matched driver, that
is still a change that probably is't intentional.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sun, Feb 18, 2024 at 08:00:28PM +0100, Christian Marangi wrote:
> With C45 PHYs that provide PHY ID in C45 Package regs, PHY device
> .phy_id is not filled.
Intentionally so. Clause 45 PHYs don't have a single ID. Marvell
88X3310 is a case in point - there are at least two different vendor
IDs in this PHY.
Trying to squash Clause 45 PHY IDs down to a single identifier is
not sensible.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sun, Feb 18, 2024 at 07:33:06PM +0000, Russell King (Oracle) wrote:
> On Sun, Feb 18, 2024 at 08:00:27PM +0100, Christian Marangi wrote:
> > Some PHY driver might implement the same OPs for different PHY ID and
> > using a mask is not enough to match similar PHYs.
> >
> > To reduce code duplication, add support for defining multiple PHY IDs in
> > PHY driver struct.
> >
> > Introduce a new variable in phy_driver struct, .ids, where a table array of
> > mdio_device_id can be defined to reference multiple PHY IDs (with their
> > own masks) supporting the same group of OPs and flags.
> >
> > Introduce a new variable in phy_device, .dev_id, where the matching
> > mdio_device_id is stored. PHYs supporting multiple PHYs for one PHY
> > driver struct, should use this instead of matching for phy_id.
> >
> > Single PHY ID implementation is still supported and dev_id is filled
> > with the data from phy_driver in this case.
>
> This looks like it's been reworked somewhat with my suggestion, or maybe
> we just came across a similar structure for comparing the IDs?
>
Hi, I forgot to include this question in the cover letter. Yes the
matching logic is from your suggestion but I changed the other part of
the logic. What credits should I use? From and Sob? Suggested-by?
Make it a separate patch?
> > + phy_dev_id = (struct mdio_device_id *)&phydev->dev_id;
>
> Why this cast? Try to write code that doesn't need casts.
>
This cast is needed to keep the dev_id const in the phy_device struct so
that other are warned to not modify it and should only be handled by
phy_probe since it's the one that fills it.
Alternative is to drop const and drop the warning.
> > + /* Fill the mdio_device_id for the PHY istance.
> > + * If PHY driver provide an array of PHYs, search the right one,
> > + * in the other case fill it with the phy_driver data.
> > + */
> > + if (phy_driver_match(phydrv, phydev, &dev_id) && dev_id) {
> > + memcpy(phy_dev_id, dev_id, sizeof(*dev_id));
> > + } else {
> > + phy_dev_id->phy_id = phydrv->phy_id;
> > + phy_dev_id->phy_id_mask = phydrv->phy_id_mask;
>
> So this is the _driver_ phy_id.
>
Ok I think I need some help with the naming here.
phy_id refer to the real PHY ID (but it's empty with C45)
and anything in dev_id would be the one from the phy_driver.
I was confused by this as I wasn't thinking that phy_id from driver
might apply mask and is not always MATCH_EXACT.
> > static inline bool phydev_id_compare(struct phy_device *phydev, u32 id)
> > {
> > - return phy_id_compare(id, phydev->phy_id, phydev->drv->phy_id_mask);
> > + return phy_id_compare(id, phydev->dev_id.phy_id,
> > + phydev->dev_id.phy_id_mask);
>
> And thus this code is now different (since it _was_ comparing the
> phydev phy_id, and you've changed it to effectively the driver's
> phy_id. While that should be the same for a matched driver, that
> is still a change that probably is't intentional.
>
Yes this change was done with the assumption that MATCH_EXACT is always
used but this is not the case and actually makes the thing wrong. Will
drop thanks for poitining this out!
My original idea was to "migrate" to device_match_id and trying to
deprecate phy_id but this doesn't makes sense at all since they reflect
different kind of data.
--
Ansuel
On Sun, Feb 18, 2024 at 07:35:30PM +0000, Russell King (Oracle) wrote:
> On Sun, Feb 18, 2024 at 08:00:28PM +0100, Christian Marangi wrote:
> > With C45 PHYs that provide PHY ID in C45 Package regs, PHY device
> > .phy_id is not filled.
>
> Intentionally so. Clause 45 PHYs don't have a single ID. Marvell
> 88X3310 is a case in point - there are at least two different vendor
> IDs in this PHY.
>
> Trying to squash Clause 45 PHY IDs down to a single identifier is
> not sensible.
>
As said in the cover letter was something I notice and was curious if it
was intentional or not. Thanks for the clarification, I will drop.
--
Ansuel
> > > + phy_dev_id = (struct mdio_device_id *)&phydev->dev_id;
> >
> > Why this cast? Try to write code that doesn't need casts.
> >
>
> This cast is needed to keep the dev_id const in the phy_device struct so
> that other are warned to not modify it and should only be handled by
> phy_probe since it's the one that fills it.
>
> Alternative is to drop const and drop the warning.
Can you propagate the const. Make phy_dev_id point to a const?
Andrew
On Sun, Feb 18, 2024 at 09:10:30PM +0100, Andrew Lunn wrote:
> > > > + phy_dev_id = (struct mdio_device_id *)&phydev->dev_id;
> > >
> > > Why this cast? Try to write code that doesn't need casts.
> > >
> >
> > This cast is needed to keep the dev_id const in the phy_device struct so
> > that other are warned to not modify it and should only be handled by
> > phy_probe since it's the one that fills it.
> >
> > Alternative is to drop const and drop the warning.
>
> Can you propagate the const. Make phy_dev_id point to a const?
>
Mhh not following, I tried changing to const struct mdio_device_id *phy_dev_id
but that results in memcpy complain (dest is void * not const) and
writing in read-only for the single PHY part (the else part)
An alternative might be to make dev_id a pointer in struct phy_device
and dynamically allocate a mdio_device_id for the case of single PHY
(else case). That effectively remove the need of this cast but I would
love to skip checking for -ENOMEM (this is why i made that local)
If it's OK to dynamically allocate for the else case then I will make
this change. I just tested this implementation and works correctly with
not warning.
--
Ansuel
On Sun, Feb 18, 2024 at 09:27:22PM +0100, Christian Marangi wrote:
> On Sun, Feb 18, 2024 at 09:10:30PM +0100, Andrew Lunn wrote:
> > > > > + phy_dev_id = (struct mdio_device_id *)&phydev->dev_id;
> > > >
> > > > Why this cast? Try to write code that doesn't need casts.
> > > >
> > >
> > > This cast is needed to keep the dev_id const in the phy_device struct so
> > > that other are warned to not modify it and should only be handled by
> > > phy_probe since it's the one that fills it.
> > >
> > > Alternative is to drop const and drop the warning.
> >
> > Can you propagate the const. Make phy_dev_id point to a const?
> >
>
> Mhh not following, I tried changing to const struct mdio_device_id *phy_dev_id
> but that results in memcpy complain (dest is void * not const) and
> writing in read-only for the single PHY part (the else part)
>
> An alternative might be to make dev_id a pointer in struct phy_device
> and dynamically allocate a mdio_device_id for the case of single PHY
> (else case). That effectively remove the need of this cast but I would
> love to skip checking for -ENOMEM (this is why i made that local)
>
> If it's OK to dynamically allocate for the else case then I will make
> this change. I just tested this implementation and works correctly with
> not warning.
Why do we need memcpy() etc - as I demonstrated in my proposal, it's
not necessary if we introduce a mdio_device_id within struct phy_driver
and we can just store a const pointer to the mdio_device_id that
matched. That was very much an intentional decision in my proposal to
make the code simple.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sun, Feb 18, 2024 at 08:34:16PM +0000, Russell King (Oracle) wrote:
> On Sun, Feb 18, 2024 at 09:27:22PM +0100, Christian Marangi wrote:
> > On Sun, Feb 18, 2024 at 09:10:30PM +0100, Andrew Lunn wrote:
> > > > > > + phy_dev_id = (struct mdio_device_id *)&phydev->dev_id;
> > > > >
> > > > > Why this cast? Try to write code that doesn't need casts.
> > > > >
> > > >
> > > > This cast is needed to keep the dev_id const in the phy_device struct so
> > > > that other are warned to not modify it and should only be handled by
> > > > phy_probe since it's the one that fills it.
> > > >
> > > > Alternative is to drop const and drop the warning.
> > >
> > > Can you propagate the const. Make phy_dev_id point to a const?
> > >
> >
> > Mhh not following, I tried changing to const struct mdio_device_id *phy_dev_id
> > but that results in memcpy complain (dest is void * not const) and
> > writing in read-only for the single PHY part (the else part)
> >
> > An alternative might be to make dev_id a pointer in struct phy_device
> > and dynamically allocate a mdio_device_id for the case of single PHY
> > (else case). That effectively remove the need of this cast but I would
> > love to skip checking for -ENOMEM (this is why i made that local)
> >
> > If it's OK to dynamically allocate for the else case then I will make
> > this change. I just tested this implementation and works correctly with
> > not warning.
>
> Why do we need memcpy() etc - as I demonstrated in my proposal, it's
> not necessary if we introduce a mdio_device_id within struct phy_driver
> and we can just store a const pointer to the mdio_device_id that
> matched. That was very much an intentional decision in my proposal to
> make the code simple.
>
With the allocated mdio_devic_id it would result in this snipped
const struct mdio_device_id *driver_dev_id;
struct mdio_device_id *dev_id;
int err = 0;
phydev->drv = phydrv;
/* Fill the mdio_device_id for the PHY istance.
* If PHY driver provide an array of PHYs, search the right one,
* in the other case fill it with the phy_driver data.
*/
if (phy_driver_match(phydrv, phydev, &driver_dev_id) && driver_dev_id) {
/* If defined, overwrite the PHY driver dev name with a
* more specific one from the matching dev_id.
*/
phydev->dev_id = driver_dev_id;
if (driver_dev_id->name)
drv->name = driver_dev_id->name;
} else {
dev_id = kzalloc(sizeof(*phydev->dev_id), GFP_KERNEL);
if (!dev_id) {
err = -ENOMEM;
goto out;
}
dev_id->phy_id = phydrv->phy_id;
dev_id->phy_id_mask = phydrv->phy_id_mask;
dev_id->name = phydrv->name;
phydev->dev_id = dev_id;
}
Is it ok? (in phy.h the thing is const struct mdio_device_id *ids)
I don't really like modifying phy_driver too much.
--
Ansuel
On Sun, Feb 18, 2024 at 09:44:03PM +0100, Christian Marangi wrote:
> On Sun, Feb 18, 2024 at 08:34:16PM +0000, Russell King (Oracle) wrote:
> > On Sun, Feb 18, 2024 at 09:27:22PM +0100, Christian Marangi wrote:
> > > On Sun, Feb 18, 2024 at 09:10:30PM +0100, Andrew Lunn wrote:
> > > > > > > + phy_dev_id = (struct mdio_device_id *)&phydev->dev_id;
> > > > > >
> > > > > > Why this cast? Try to write code that doesn't need casts.
> > > > > >
> > > > >
> > > > > This cast is needed to keep the dev_id const in the phy_device struct so
> > > > > that other are warned to not modify it and should only be handled by
> > > > > phy_probe since it's the one that fills it.
> > > > >
> > > > > Alternative is to drop const and drop the warning.
> > > >
> > > > Can you propagate the const. Make phy_dev_id point to a const?
> > > >
> > >
> > > Mhh not following, I tried changing to const struct mdio_device_id *phy_dev_id
> > > but that results in memcpy complain (dest is void * not const) and
> > > writing in read-only for the single PHY part (the else part)
> > >
> > > An alternative might be to make dev_id a pointer in struct phy_device
> > > and dynamically allocate a mdio_device_id for the case of single PHY
> > > (else case). That effectively remove the need of this cast but I would
> > > love to skip checking for -ENOMEM (this is why i made that local)
> > >
> > > If it's OK to dynamically allocate for the else case then I will make
> > > this change. I just tested this implementation and works correctly with
> > > not warning.
> >
> > Why do we need memcpy() etc - as I demonstrated in my proposal, it's
> > not necessary if we introduce a mdio_device_id within struct phy_driver
> > and we can just store a const pointer to the mdio_device_id that
> > matched. That was very much an intentional decision in my proposal to
> > make the code simple.
> >
>
> With the allocated mdio_devic_id it would result in this snipped
>
> const struct mdio_device_id *driver_dev_id;
> struct mdio_device_id *dev_id;
> int err = 0;
>
> phydev->drv = phydrv;
> /* Fill the mdio_device_id for the PHY istance.
> * If PHY driver provide an array of PHYs, search the right one,
> * in the other case fill it with the phy_driver data.
> */
> if (phy_driver_match(phydrv, phydev, &driver_dev_id) && driver_dev_id) {
> /* If defined, overwrite the PHY driver dev name with a
> * more specific one from the matching dev_id.
> */
> phydev->dev_id = driver_dev_id;
> if (driver_dev_id->name)
> drv->name = driver_dev_id->name;
> } else {
> dev_id = kzalloc(sizeof(*phydev->dev_id), GFP_KERNEL);
> if (!dev_id) {
> err = -ENOMEM;
> goto out;
> }
> dev_id->phy_id = phydrv->phy_id;
> dev_id->phy_id_mask = phydrv->phy_id_mask;
> dev_id->name = phydrv->name;
> phydev->dev_id = dev_id;
> }
>
> Is it ok? (in phy.h the thing is const struct mdio_device_id *ids)
> I don't really like modifying phy_driver too much.
The thing that I don't like about this is that we need to free this
allocation (and know that we need to free it) which adds more
complexity and more possibilities for things leaking.
The advantage to putting it in the phy_driver structure is that its
lifetime is inherently tied to the driver structure and we don't
have the issue of having to free it - nor do we need to have separate
allocations for each PHY device.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> With the allocated mdio_devic_id it would result in this snipped
>
> const struct mdio_device_id *driver_dev_id;
> struct mdio_device_id *dev_id;
> int err = 0;
>
> phydev->drv = phydrv;
> /* Fill the mdio_device_id for the PHY istance.
> * If PHY driver provide an array of PHYs, search the right one,
> * in the other case fill it with the phy_driver data.
> */
> if (phy_driver_match(phydrv, phydev, &driver_dev_id) && driver_dev_id) {
> /* If defined, overwrite the PHY driver dev name with a
> * more specific one from the matching dev_id.
> */
> phydev->dev_id = driver_dev_id;
> if (driver_dev_id->name)
> drv->name = driver_dev_id->name;
What is drv here? You should not be changing the name within the
driver structure, since that is shared by a number of devices.
Andrew
On 2/18/2024 11:00 AM, Christian Marangi wrote:
> Rework bcm7xxx PHY driver table to new multiple PHY format
> implementation to reduce code duplication and final size of the compiled
> module.
I like the idea of sharing as much code as possible and creating a
smaller module, however by changing the name, you are creating an
user-space ABI change, we rely upon the exact PHY name being shown under
/sys/class/mdio_bus/*/* and this change will break that.
Thanks!
--
Florian
On Sun, Feb 18, 2024 at 08:26:29PM -0800, Florian Fainelli wrote:
>
>
> On 2/18/2024 11:00 AM, Christian Marangi wrote:
> > Rework bcm7xxx PHY driver table to new multiple PHY format
> > implementation to reduce code duplication and final size of the compiled
> > module.
>
> I like the idea of sharing as much code as possible and creating a smaller
> module, however by changing the name, you are creating an user-space ABI
> change, we rely upon the exact PHY name being shown under
> /sys/class/mdio_bus/*/* and this change will break that.
>
Thanks for putting this concern on the table but isn't that generated by
dev_set_name and PHY_ID_FMT? from bus->id and addr?
Can't find reference of the name entry in sysfs. Am I missing something?
The name seems to be used only by logging to print info/err/warn.
--
Ansuel
On 2/19/2024 8:41 AM, Christian Marangi wrote:
> On Sun, Feb 18, 2024 at 08:26:29PM -0800, Florian Fainelli wrote:
>>
>>
>> On 2/18/2024 11:00 AM, Christian Marangi wrote:
>>> Rework bcm7xxx PHY driver table to new multiple PHY format
>>> implementation to reduce code duplication and final size of the compiled
>>> module.
>>
>> I like the idea of sharing as much code as possible and creating a smaller
>> module, however by changing the name, you are creating an user-space ABI
>> change, we rely upon the exact PHY name being shown under
>> /sys/class/mdio_bus/*/* and this change will break that.
>>
>
> Thanks for putting this concern on the table but isn't that generated by
> dev_set_name and PHY_ID_FMT? from bus->id and addr?
>
> Can't find reference of the name entry in sysfs. Am I missing something?
> The name seems to be used only by logging to print info/err/warn.
The name will appear under /sys/ like this:
ls -ls /sys/class/mdio_bus/unimac-mdio-0/unimac-mdio-0\:01/
total 0
0 lrwxrwxrwx 1 root root 0 Jan 4 21:27
attached_dev -> ../../../../net/eth0
0 lrwxrwxrwx 1 root root 0 Jan 4 21:27 driver
-> ../../../../../../../../bus/mdio_bus/drivers/Broadcom BCM7712
it might be OK to change the driver, but I can tell you this is going to
be breaking a number of our scripts here...
--
Florian
On Mon, Feb 19, 2024 at 12:15:14PM -0800, Florian Fainelli wrote:
>
>
> On 2/19/2024 8:41 AM, Christian Marangi wrote:
> > On Sun, Feb 18, 2024 at 08:26:29PM -0800, Florian Fainelli wrote:
> > >
> > >
> > > On 2/18/2024 11:00 AM, Christian Marangi wrote:
> > > > Rework bcm7xxx PHY driver table to new multiple PHY format
> > > > implementation to reduce code duplication and final size of the compiled
> > > > module.
> > >
> > > I like the idea of sharing as much code as possible and creating a smaller
> > > module, however by changing the name, you are creating an user-space ABI
> > > change, we rely upon the exact PHY name being shown under
> > > /sys/class/mdio_bus/*/* and this change will break that.
> > >
> >
> > Thanks for putting this concern on the table but isn't that generated by
> > dev_set_name and PHY_ID_FMT? from bus->id and addr?
> >
> > Can't find reference of the name entry in sysfs. Am I missing something?
> > The name seems to be used only by logging to print info/err/warn.
>
> The name will appear under /sys/ like this:
>
> ls -ls /sys/class/mdio_bus/unimac-mdio-0/unimac-mdio-0\:01/
> total 0
> 0 lrwxrwxrwx 1 root root 0 Jan 4 21:27 attached_dev
> -> ../../../../net/eth0
> 0 lrwxrwxrwx 1 root root 0 Jan 4 21:27 driver ->
> ../../../../../../../../bus/mdio_bus/drivers/Broadcom BCM7712
>
> it might be OK to change the driver, but I can tell you this is going to be
> breaking a number of our scripts here...
Thanks for the command, yes I just notice the problem and yes it's
problematic... Starting to think that the only way to have this cleanup
is to detach ops from the phy_driver struct.
Not fiding a good way to handle the name problem...
--
Ansuel