Instead of tracing whether the PHY is a C45 one, use the method how the
PHY is accessed. For now, that is either by C22 or by C45 transactions.
There is no functional change, just a semantical difference.
This is a preparation patch to add a third access method C45-over-C22.
Signed-off-by: Michael Walle <[email protected]>
---
v3:
- use 22 and 45 as constants for the enum to catch errors
- move enum in struct next to enum phy_state
---
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 8 +++----
drivers/net/mdio/fwnode_mdio.c | 6 +++--
drivers/net/phy/mdio_bus.c | 9 +++----
drivers/net/phy/nxp-tja11xx.c | 3 ++-
drivers/net/phy/phy_device.c | 29 ++++++++++++++---------
drivers/net/phy/sfp.c | 12 ++++++----
include/linux/phy.h | 27 +++++++++++++++------
7 files changed, 60 insertions(+), 34 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
index 928d934cb21a..74cd6197735b 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
@@ -687,9 +687,9 @@ static int
hns_mac_register_phydev(struct mii_bus *mdio, struct hns_mac_cb *mac_cb,
u32 addr)
{
+ enum phy_access_mode mode;
struct phy_device *phy;
const char *phy_type;
- bool is_c45;
int rc;
rc = fwnode_property_read_string(mac_cb->fw_port,
@@ -698,13 +698,13 @@ hns_mac_register_phydev(struct mii_bus *mdio, struct hns_mac_cb *mac_cb,
return rc;
if (!strcmp(phy_type, phy_modes(PHY_INTERFACE_MODE_XGMII)))
- is_c45 = true;
+ mode = PHY_ACCESS_C45;
else if (!strcmp(phy_type, phy_modes(PHY_INTERFACE_MODE_SGMII)))
- is_c45 = false;
+ mode = PHY_ACCESS_C22;
else
return -ENODATA;
- phy = get_phy_device(mdio, addr, is_c45);
+ phy = get_phy_device(mdio, addr, mode);
if (!phy || IS_ERR(phy))
return -EIO;
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1183ef5e203e..972c8932c2fe 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -131,9 +131,11 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
if (is_c45 || fwnode_get_phy_id(child, &phy_id))
- phy = get_phy_device(bus, addr, is_c45);
+ phy = get_phy_device(bus, addr,
+ is_c45 ? PHY_ACCESS_C45 : PHY_ACCESS_C22);
else
- phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+ phy = phy_device_create(bus, addr, phy_id, PHY_ACCESS_C22,
+ NULL);
if (IS_ERR(phy)) {
rc = PTR_ERR(phy);
goto clean_mii_ts;
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 8b3618d3da4a..a31eb1204f63 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -513,12 +513,13 @@ static int mdiobus_create_device(struct mii_bus *bus,
return ret;
}
-static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr, bool c45)
+static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr,
+ enum phy_access_mode mode)
{
struct phy_device *phydev = ERR_PTR(-ENODEV);
int err;
- phydev = get_phy_device(bus, addr, c45);
+ phydev = get_phy_device(bus, addr, mode);
if (IS_ERR(phydev))
return phydev;
@@ -550,7 +551,7 @@ static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr, bool c45)
*/
struct phy_device *mdiobus_scan_c22(struct mii_bus *bus, int addr)
{
- return mdiobus_scan(bus, addr, false);
+ return mdiobus_scan(bus, addr, PHY_ACCESS_C22);
}
EXPORT_SYMBOL(mdiobus_scan_c22);
@@ -568,7 +569,7 @@ EXPORT_SYMBOL(mdiobus_scan_c22);
*/
static struct phy_device *mdiobus_scan_c45(struct mii_bus *bus, int addr)
{
- return mdiobus_scan(bus, addr, true);
+ return mdiobus_scan(bus, addr, PHY_ACCESS_C45);
}
static int mdiobus_scan_bus_c22(struct mii_bus *bus)
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index b13e15310feb..1c6c1523540e 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -580,7 +580,8 @@ static void tja1102_p1_register(struct work_struct *work)
}
/* Real PHY ID of Port 1 is 0 */
- phy = phy_device_create(bus, addr, PHY_ID_TJA1102, false, NULL);
+ phy = phy_device_create(bus, addr, PHY_ID_TJA1102,
+ PHY_ACCESS_C22, NULL);
if (IS_ERR(phy)) {
dev_err(dev, "Can't create PHY device for Port 1: %i\n",
addr);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 44968ea447fc..6254212d65a5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -630,7 +630,7 @@ static int phy_request_driver_module(struct phy_device *dev, u32 phy_id)
}
struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
- bool is_c45,
+ enum phy_access_mode mode,
struct phy_c45_device_ids *c45_ids)
{
struct phy_device *dev;
@@ -664,7 +664,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
dev->autoneg = AUTONEG_ENABLE;
dev->pma_extable = -ENODATA;
- dev->is_c45 = is_c45;
+ dev->access_mode = mode;
dev->phy_id = phy_id;
if (c45_ids)
dev->c45_ids = *c45_ids;
@@ -926,7 +926,7 @@ EXPORT_SYMBOL(fwnode_get_phy_id);
* struct
* @bus: the target MII bus
* @addr: PHY address on the MII bus
- * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
+ * @mode: Access mode of the PHY
*
* Probe for a PHY at @addr on @bus.
*
@@ -940,7 +940,8 @@ EXPORT_SYMBOL(fwnode_get_phy_id);
* Returns an allocated &struct phy_device on success, %-ENODEV if there is
* no PHY present, or %-EIO on bus access error.
*/
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
+struct phy_device *get_phy_device(struct mii_bus *bus, int addr,
+ enum phy_access_mode mode)
{
struct phy_c45_device_ids c45_ids;
u32 phy_id = 0;
@@ -950,10 +951,16 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
c45_ids.mmds_present = 0;
memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
- if (is_c45)
- r = get_phy_c45_ids(bus, addr, &c45_ids);
- else
+ switch (mode) {
+ case PHY_ACCESS_C22:
r = get_phy_c22_id(bus, addr, &phy_id);
+ break;
+ case PHY_ACCESS_C45:
+ r = get_phy_c45_ids(bus, addr, &c45_ids);
+ break;
+ default:
+ return ERR_PTR(-EIO);
+ }
if (r)
return ERR_PTR(r);
@@ -963,15 +970,15 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
* probe with C45 to see if we're able to get a valid PHY ID in the C45
* space, if successful, create the C45 PHY device.
*/
- if (!is_c45 && phy_id == 0 && bus->read_c45) {
+ if (mode == PHY_ACCESS_C22 && phy_id == 0 && bus->read_c45) {
r = get_phy_c45_ids(bus, addr, &c45_ids);
if (!r)
return phy_device_create(bus, addr, phy_id,
- true, &c45_ids);
+ PHY_ACCESS_C45, &c45_ids);
}
- return phy_device_create(bus, addr, phy_id, is_c45,
- !is_c45 ? NULL : &c45_ids);
+ return phy_device_create(bus, addr, phy_id, mode,
+ mode == PHY_ACCESS_C22 ? NULL : &c45_ids);
}
EXPORT_SYMBOL(get_phy_device);
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index d855a18308d7..e7f8decaf3ff 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1750,12 +1750,13 @@ static void sfp_sm_phy_detach(struct sfp *sfp)
sfp->mod_phy = NULL;
}
-static int sfp_sm_probe_phy(struct sfp *sfp, int addr, bool is_c45)
+static int sfp_sm_probe_phy(struct sfp *sfp, int addr,
+ enum phy_access_mode mode)
{
struct phy_device *phy;
int err;
- phy = get_phy_device(sfp->i2c_mii, addr, is_c45);
+ phy = get_phy_device(sfp->i2c_mii, addr, mode);
if (phy == ERR_PTR(-ENODEV))
return PTR_ERR(phy);
if (IS_ERR(phy)) {
@@ -1879,15 +1880,16 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)
break;
case MDIO_I2C_MARVELL_C22:
- err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, false);
+ err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, PHY_ACCESS_C22);
break;
case MDIO_I2C_C45:
- err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, true);
+ err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, PHY_ACCESS_C45);
break;
case MDIO_I2C_ROLLBALL:
- err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR_ROLLBALL, true);
+ err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR_ROLLBALL,
+ PHY_ACCESS_C45);
break;
}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fdb3774e99fc..fb7481715c3b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -531,6 +531,17 @@ struct phy_c45_device_ids {
struct macsec_context;
struct macsec_ops;
+/**
+ * enum phy_access_mode - PHY register access mode definitions
+ *
+ * @PHY_ACCESS_C22: use 802.3 c22 MDIO transactions
+ * @PHY_ACCESS_C45: use 802.3 c45 MDIO transactions
+ */
+enum phy_access_mode {
+ PHY_ACCESS_C22 = 22,
+ PHY_ACCESS_C45 = 45,
+};
+
/**
* struct phy_device - An instance of a PHY
*
@@ -539,8 +550,7 @@ struct macsec_ops;
* @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.
* @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.
+ * @c45_ids: 802.3-c45 Device Identifiers if it's an C45 PHY.
* @is_internal: Set to true if this PHY is internal to a MAC.
* @is_pseudo_fixed_link: Set to true if this PHY is an Ethernet switch, etc.
* @is_gigabit_capable: Set to true if PHY supports 1000Mbps
@@ -555,6 +565,7 @@ struct macsec_ops;
* @wol_enabled: Set to true if the PHY or the attached MAC have Wake-on-LAN
* enabled.
* @state: State of the PHY for management purposes
+ * @access_mode: MDIO access mode of the PHY.
* @dev_flags: Device-specific flags used by the PHY driver.
*
* - Bits [15:0] are free to use by the PHY driver to communicate
@@ -638,7 +649,6 @@ struct phy_device {
u32 phy_id;
struct phy_c45_device_ids c45_ids;
- unsigned is_c45:1;
unsigned is_internal:1;
unsigned is_pseudo_fixed_link:1;
unsigned is_gigabit_capable:1;
@@ -665,6 +675,7 @@ struct phy_device {
int rate_matching;
enum phy_state state;
+ enum phy_access_mode access_mode;
u32 dev_flags;
@@ -768,7 +779,7 @@ static inline struct phy_device *to_phy_device(const struct device *dev)
static inline bool phy_has_c45_registers(struct phy_device *phydev)
{
- return phydev->is_c45;
+ return phydev->access_mode != PHY_ACCESS_C22;
}
/**
@@ -1624,7 +1635,7 @@ int phy_modify_paged(struct phy_device *phydev, int page, u32 regnum,
u16 mask, u16 set);
struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
- bool is_c45,
+ enum phy_access_mode mode,
struct phy_c45_device_ids *c45_ids);
#if IS_ENABLED(CONFIG_PHYLIB)
int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id);
@@ -1632,7 +1643,8 @@ struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode);
struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
struct phy_device *device_phy_find_device(struct device *dev);
struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode);
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
+struct phy_device *get_phy_device(struct mii_bus *bus, int addr,
+ enum phy_access_mode mode);
int phy_device_register(struct phy_device *phy);
void phy_device_free(struct phy_device *phydev);
#else
@@ -1664,7 +1676,8 @@ struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
}
static inline
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
+struct phy_device *get_phy_device(struct mii_bus *bus, int addr,
+ enum phy_access_mode mode)
{
return NULL;
}
--
2.39.2
On Tue, Jul 18, 2023 at 07:40:49PM +0200, Andrew Lunn wrote:
> > static inline bool phy_has_c45_registers(struct phy_device *phydev)
> > {
> > - return phydev->is_c45;
> > + return phydev->access_mode != PHY_ACCESS_C22;
> > }
>
> So this is making me wounder if we have a clean separation between
> register spaces and access methods.
>
> Should there be a phy_has_c22_registers() ?
Yes, I've been wondering that. I've recently heard about a Realtek PHY
which is supported by our realtek driver, but appears on a SFP that
can only do C45 accesses. However, the realtek driver is written to
use C22 accesses to this PHY - and the PHY supports both. So currently
it doesn't work.
That's just an additional data point for thinking about this, I haven't
formulated a solution to it yet.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> static inline bool phy_has_c45_registers(struct phy_device *phydev)
> {
> - return phydev->is_c45;
> + return phydev->access_mode != PHY_ACCESS_C22;
> }
So this is making me wounder if we have a clean separation between
register spaces and access methods.
Should there be a phy_has_c22_registers() ?
A PHY can have both C22 registers and C45 registers. It is up to the
driver to decide which it wants to access when.
Should phydev->access_mode really be phydev->access_mode_c45_registers
to indicate how to access the C45 registers if phy_has_c45_registers()
is true?
Has there been a review of all uses of phydev->is_c45 to determine if
the user wants to know if C45 registers exist,
a.k.a. phy_has_c45_registers(), or if C45 bus transactions can be
performed, and then later in this series, additionally if C45 over C22
can be performed. These are different things.
I need to keep reading the patches...
Andrew
On Tue, Jul 18, 2023 at 06:52:12PM +0100, Russell King (Oracle) wrote:
> On Tue, Jul 18, 2023 at 07:40:49PM +0200, Andrew Lunn wrote:
> > > static inline bool phy_has_c45_registers(struct phy_device *phydev)
> > > {
> > > - return phydev->is_c45;
> > > + return phydev->access_mode != PHY_ACCESS_C22;
> > > }
> >
> > So this is making me wounder if we have a clean separation between
> > register spaces and access methods.
> >
> > Should there be a phy_has_c22_registers() ?
>
> Yes, I've been wondering that. I've recently heard about a Realtek PHY
> which is supported by our realtek driver, but appears on a SFP that
> can only do C45 accesses. However, the realtek driver is written to
> use C22 accesses to this PHY - and the PHY supports both. So currently
> it doesn't work.
>
> That's just an additional data point for thinking about this, I haven't
> formulated a solution to it yet.
That kind of sounds like two drivers. Or two drivers in one .c
file. Do you know what C45 IDs it has? Same as the C22? If it is
different, each could have its own struct phy_driver.
Andrew
> Maybe we need to clarify what "has c22/c45 registers space" actually
> means. Responds to MII c22/c45 access?
No. That is not what it means to me. Bus transactions and register
spaces are different.
There are C22 registers, and there are C45 registers.
The bus can do C22 transfers, and it can do C45 transfers.
C22 registers can be access using C22 bus transfers.
C45 registers can be accessed using either C45 bus transfers, or
indirectly via C45 over C22 transfers.
Currently, there is no C22 over C45, but given the oddball Realtek PHY
Russell is talking about in an SFP, it might have a proprietary C22
over C45?
Andrew
Am 2023-07-18 19:40, schrieb Andrew Lunn:
>> static inline bool phy_has_c45_registers(struct phy_device *phydev)
>> {
>> - return phydev->is_c45;
>> + return phydev->access_mode != PHY_ACCESS_C22;
>> }
>
> So this is making me wounder if we have a clean separation between
> register spaces and access methods.
The idea is to at least have it behind a helper which can be changed
if we get that information from somewhere else.
But right now, a PHY is considered to have c45 registers if it is
probed via c45 (accesses).
Instead of checking the access mode, I could also introduce a
bitmask(?)/flags has_c22/has_c45_registers. But how would you tell
if a PHY as c22 registers? Probe both c22 and c45? What if the bus
can't do c45?
> Should there be a phy_has_c22_registers() ?
>
> A PHY can have both C22 registers and C45 registers. It is up to the
> driver to decide which it wants to access when.
But isn't it also the driver which has the ultimate information whether
a PHY has c22 register space and/or c45 one?
Maybe we need to clarify what "has c22/c45 registers space" actually
means. Responds to MII c22/c45 access?
-michael
> Should phydev->access_mode really be phydev->access_mode_c45_registers
> to indicate how to access the C45 registers if phy_has_c45_registers()
> is true?
>
> Has there been a review of all uses of phydev->is_c45 to determine if
> the user wants to know if C45 registers exist,
> a.k.a. phy_has_c45_registers(), or if C45 bus transactions can be
> performed, and then later in this series, additionally if C45 over C22
> can be performed. These are different things.
>
> I need to keep reading the patches...
>
> Andrew
On Tue, Jul 18, 2023 at 09:18:51PM +0200, Andrew Lunn wrote:
> On Tue, Jul 18, 2023 at 06:52:12PM +0100, Russell King (Oracle) wrote:
> > On Tue, Jul 18, 2023 at 07:40:49PM +0200, Andrew Lunn wrote:
> > > > static inline bool phy_has_c45_registers(struct phy_device *phydev)
> > > > {
> > > > - return phydev->is_c45;
> > > > + return phydev->access_mode != PHY_ACCESS_C22;
> > > > }
> > >
> > > So this is making me wounder if we have a clean separation between
> > > register spaces and access methods.
> > >
> > > Should there be a phy_has_c22_registers() ?
> >
> > Yes, I've been wondering that. I've recently heard about a Realtek PHY
> > which is supported by our realtek driver, but appears on a SFP that
> > can only do C45 accesses. However, the realtek driver is written to
> > use C22 accesses to this PHY - and the PHY supports both. So currently
> > it doesn't work.
> >
> > That's just an additional data point for thinking about this, I haven't
> > formulated a solution to it yet.
>
> That kind of sounds like two drivers. Or two drivers in one .c
> file. Do you know what C45 IDs it has? Same as the C22? If it is
> different, each could have its own struct phy_driver.
Why would it be two drivers?
The PHY in question is RTL8221B-VB-CG, and it has the same ID in both
the C22 and C45 registers, since the realtek driver attempts to drive
the device, but fails because __phy_read() errors out (because there's
no C22 bus access.)
If one looks at:
https://www.realtek.com/en/products/connected-media-ics/item/rtl8221b-i-vb-cg
it states "Supports clause 22 and Clause 45 MDC/MDIO management
interface".
which to me suggests it can be driven via clause 22 MDIO cycles on
the bus as a clause 22 PHY and using indirect access to clause 45
registers, or only via clause 45 MDIO cycles.
In the case of this particular SFP, it apparently uses the Rollball
access method which is C45-only.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> Why would it be two drivers?
I'm assuming there is no C22 over C45. So phy_read() is not going to
work. I'm also assuming the current driver is using the C22 register
space.
So either the driver needs re-writing to use only the C45 register
space, maybe using C45 over C22, or it needs a second parallel driver
using just the C45 register space when only C45 bus transfers are
available.
Andrew