| Message ID | 20210630174022.1016525-1-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: axienet: Allow phytool access to PCS/PMA PHY | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: linux-arm-kernel@lists.infradead.org michal.simek@xilinx.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 41 this patch: 41 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 88 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 34 this patch: 34 |
| netdev/header_inline | success | Link |
On Wed, 2021-06-30 at 18:46 +0100, Russell King (Oracle) wrote: > On Wed, Jun 30, 2021 at 11:40:22AM -0600, Robert Hancock wrote: > > Allow phytool ioctl access to read/write registers in the internal > > PCS/PMA PHY if it is enabled. > > I wonder if this is something that should happen in phylink? > If there are other drivers which have a PCS which could be accessed with phytool etc., it might make sense. Right now phylink core doesn't really have any knowledge that the PCS PHY actually exists as something that can be accessed via MDIO registers, it just talks to it indirectly through the mac_config and mac_pcs_get_state callbacks in the driver which then call back into the c22_pcs helper functions to actually talk to the PCS. I'm not sure phylink could generically assume that the PCS can be accessed over MDIO however, as I believe that the Cadence MACB IP, for example, at least as implemented in the Xilinx ZynqMP parts, exposes its PCS with some PHY-style registers but they are just a portion of the device's register space and not accessed via MDIO, so we'd want to support that kind of setup. I suppose it could implement an emulated MDIO bus to access those registers for that purpose?
On Wed, Jun 30, 2021 at 06:23:46PM +0000, Robert Hancock wrote: > On Wed, 2021-06-30 at 18:46 +0100, Russell King (Oracle) wrote: > > On Wed, Jun 30, 2021 at 11:40:22AM -0600, Robert Hancock wrote: > > > Allow phytool ioctl access to read/write registers in the internal > > > PCS/PMA PHY if it is enabled. > > > > I wonder if this is something that should happen in phylink? > > > > If there are other drivers which have a PCS which could be accessed with > phytool etc., it might make sense. Right now phylink core doesn't really have > any knowledge that the PCS PHY actually exists as something that can be > accessed via MDIO registers, it just talks to it indirectly through the > mac_config and mac_pcs_get_state callbacks in the driver which then call back > into the c22_pcs helper functions to actually talk to the PCS. Phylink does know that a PCS exists. It has separate pcs_ops for it, and slightly changes its behaviour when a PCS exists.
On Wed, Jun 30, 2021 at 11:40:22AM -0600, Robert Hancock wrote: > Allow phytool ioctl access to read/write registers in the internal > PCS/PMA PHY if it is enabled. > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > --- > .../net/ethernet/xilinx/xilinx_axienet_main.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > index 13cd799541aa..41f2c2255118 100644 > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > @@ -1213,10 +1213,29 @@ static void axienet_poll_controller(struct net_device *ndev) > static int axienet_ioctl(struct net_device *dev, struct ifreq *rq, int cmd) > { > struct axienet_local *lp = netdev_priv(dev); > + struct mii_ioctl_data *mii = if_mii(rq); > > if (!netif_running(dev)) > return -EINVAL; > > + if (lp->pcs_phy && lp->pcs_phy->addr == mii->phy_id) { > + int ret; > + > + switch (cmd) { > + case SIOCGMIIREG: > + ret = mdiobus_read(lp->pcs_phy->bus, mii->phy_id, mii->reg_num); > + if (ret >= 0) { > + mii->val_out = ret; > + ret = 0; > + } > + return ret; > + > + case SIOCSMIIREG: > + return mdiobus_write(lp->pcs_phy->bus, mii->phy_id, > + mii->reg_num, mii->val_in); > + } I would prefer not to allow write. The kernel should be driving the hardware, and if user space changes values, the kernel has no idea about it, and can do the wrong things. Andrew
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c index 13cd799541aa..41f2c2255118 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c @@ -1213,10 +1213,29 @@ static void axienet_poll_controller(struct net_device *ndev) static int axienet_ioctl(struct net_device *dev, struct ifreq *rq, int cmd) { struct axienet_local *lp = netdev_priv(dev); + struct mii_ioctl_data *mii = if_mii(rq); if (!netif_running(dev)) return -EINVAL; + if (lp->pcs_phy && lp->pcs_phy->addr == mii->phy_id) { + int ret; + + switch (cmd) { + case SIOCGMIIREG: + ret = mdiobus_read(lp->pcs_phy->bus, mii->phy_id, mii->reg_num); + if (ret >= 0) { + mii->val_out = ret; + ret = 0; + } + return ret; + + case SIOCSMIIREG: + return mdiobus_write(lp->pcs_phy->bus, mii->phy_id, + mii->reg_num, mii->val_in); + } + } + return phylink_mii_ioctl(lp->phylink, rq, cmd); }
Allow phytool ioctl access to read/write registers in the internal PCS/PMA PHY if it is enabled. Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- .../net/ethernet/xilinx/xilinx_axienet_main.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)