| Message ID | 20200106013417.12154-6-olteanv@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | Convert Felix DSA switch to PHYLINK | expand |
On 1/5/20 5:34 PM, Vladimir Oltean wrote: > From: Claudiu Manoil <claudiu.manoil@nxp.com> > > Within the LS1028A SoC, the register map for the ENETC MDIO controller > is instantiated a few times: for the central (external) MDIO controller, > for the internal bus of each standalone ENETC port, and for the internal > bus of the Felix switch. > > Refactoring is needed to support multiple MDIO buses from multiple > drivers. The enetc_hw structure is made an opaque type and a smaller > enetc_mdio_priv is created. > > 'mdio_base' - MDIO registers base address - is being parameterized, to > be able to work with different MDIO register bases. > > The ENETC MDIO bus operations are exported from the fsl-enetc-mdio > kernel object, the same that registers the central MDIO controller (the > dedicated PF). The ENETC main driver has been changed to select it, and > use its exported helpers to further register its private MDIO bus. The > DSA Felix driver will do the same. This series has already been applied so this may be food for thought at this point, but why was not the solution to create a standalone mii_bus driver and have all consumers be pointed it? It is not uncommon for MDIO controllers to be re-used and integrated within a larger block and when that happens whoever owns the largest address space, say the Ethernet MAC can request the large resource region and the MDIO bus controler can work on that premise, that's what we did with genet/bcmmii.c and mdio-bcm-unimac.c for instance (so we only do an ioremap, not request_mem_region + ioremap). Your commit message does not provide a justification for why this abstraction (mii_bus) was not suitable or considered here. Do you think that could be changed? Thanks!
Hi Florian, On Mon, 6 Jan 2020 at 21:35, Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 1/5/20 5:34 PM, Vladimir Oltean wrote: > > From: Claudiu Manoil <claudiu.manoil@nxp.com> > > > > Within the LS1028A SoC, the register map for the ENETC MDIO controller > > is instantiated a few times: for the central (external) MDIO controller, > > for the internal bus of each standalone ENETC port, and for the internal > > bus of the Felix switch. > > > > Refactoring is needed to support multiple MDIO buses from multiple > > drivers. The enetc_hw structure is made an opaque type and a smaller > > enetc_mdio_priv is created. > > > > 'mdio_base' - MDIO registers base address - is being parameterized, to > > be able to work with different MDIO register bases. > > > > The ENETC MDIO bus operations are exported from the fsl-enetc-mdio > > kernel object, the same that registers the central MDIO controller (the > > dedicated PF). The ENETC main driver has been changed to select it, and > > use its exported helpers to further register its private MDIO bus. The > > DSA Felix driver will do the same. > > This series has already been applied so this may be food for thought at > this point, but why was not the solution to create a standalone mii_bus > driver and have all consumers be pointed it? > I have no real opinion on this. To be honest, the reason is that the existing "culture" of Freescale MDIO drivers wasn't to put them in drivers/net/phy/mdio-*.c, and I just didn't look past the fence. But what is the benefit? What gets passed between bcmgenet and mdio-bcm-unimac with struct bcmgenet_platform_data is equivalent with what gets passed between vsc9959 and enetc_mdio with the manual population of struct mii_bus and struct enetc_mdio_priv, no? I'm not even sure there is a net reduction in code size. And I am not really sure that I want an of_node for the MDIO bus platform device anyway. Whereas genet seems to be instantiating a port-private MDIO bus for the _real_ (but nonetheless embedded) PHY, the MDIO bus we have here is for the MAC PCS, which is more akin to the custom device tree binding "pcsphy-handle" that the DPAA1 driver is using (see arch/arm64/boot/dts/qoriq-fman3-0-10g-0.dtsi for example). So there is no requirement to run the PHY state machine on it, it's just locally driven, so I don't want to add a dependency on device tree where it's really not needed. (By the way I am further confused by the undocumented/unused "brcm,40nm-ephy" compatible string that these device tree bindings for genet have). > It is not uncommon for MDIO controllers to be re-used and integrated > within a larger block and when that happens whoever owns the largest > address space, say the Ethernet MAC can request the large resource > region and the MDIO bus controler can work on that premise, that's what > we did with genet/bcmmii.c and mdio-bcm-unimac.c for instance (so we > only do an ioremap, not request_mem_region + ioremap). > I don't really understand this. In arch/mips/boot/dts, for all of bcm73xx and bcm74xx SoCs, you have a single Ethernet port DT node, and a single MDIO bus as a child beneath it, where is this reuse that you mention? And because I don't really understand what you've said, my following comment maybe makes no sense, but I think what you mean by "MDIO controller reuse" is that there are multiple instantiations of the register map, but ultimately every transaction ends up on the same MDIO/MDC pair of wires and the same electrical bus. We do have some of that with the ENETC, but not with the switch, whose internal MDIO bus has no connection to the outside world, it just holds the PCS front-ends for the SerDes. I also don't understand the reference to request_mem_region, perhaps it would help if you could show some code. > Your commit message does not provide a justification for why this > abstraction (mii_bus) was not suitable or considered here. Do you think > that could be changed? > I'm sorry, was the mii_bus abstraction really not considered here? Based on the stuff exported in this patch, an mii_bus is exactly what I'm registering in 9/9, no? > Thanks! > -- > Florian Regards, -Vladimir
On 1/6/2020 3:00 PM, Vladimir Oltean wrote: > Hi Florian, > > On Mon, 6 Jan 2020 at 21:35, Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> On 1/5/20 5:34 PM, Vladimir Oltean wrote: >>> From: Claudiu Manoil <claudiu.manoil@nxp.com> >>> >>> Within the LS1028A SoC, the register map for the ENETC MDIO controller >>> is instantiated a few times: for the central (external) MDIO controller, >>> for the internal bus of each standalone ENETC port, and for the internal >>> bus of the Felix switch. >>> >>> Refactoring is needed to support multiple MDIO buses from multiple >>> drivers. The enetc_hw structure is made an opaque type and a smaller >>> enetc_mdio_priv is created. >>> >>> 'mdio_base' - MDIO registers base address - is being parameterized, to >>> be able to work with different MDIO register bases. >>> >>> The ENETC MDIO bus operations are exported from the fsl-enetc-mdio >>> kernel object, the same that registers the central MDIO controller (the >>> dedicated PF). The ENETC main driver has been changed to select it, and >>> use its exported helpers to further register its private MDIO bus. The >>> DSA Felix driver will do the same. >> >> This series has already been applied so this may be food for thought at >> this point, but why was not the solution to create a standalone mii_bus >> driver and have all consumers be pointed it? >> > > I have no real opinion on this. > > To be honest, the reason is that the existing "culture" of Freescale > MDIO drivers wasn't to put them in drivers/net/phy/mdio-*.c, and I > just didn't look past the fence. > > But what is the benefit? What gets passed between bcmgenet and > mdio-bcm-unimac with struct bcmgenet_platform_data is equivalent with > what gets passed between vsc9959 and enetc_mdio with the manual > population of struct mii_bus and struct enetc_mdio_priv, no? I'm not > even sure there is a net reduction in code size. And I am not really > sure that I want an of_node for the MDIO bus platform device anyway. > Whereas genet seems to be instantiating a port-private MDIO bus for > the _real_ (but nonetheless embedded) PHY, the MDIO bus we have here > is for the MAC PCS, which is more akin to the custom device tree > binding "pcsphy-handle" that the DPAA1 driver is using (see > arch/arm64/boot/dts/qoriq-fman3-0-10g-0.dtsi for example). So there is > no requirement to run the PHY state machine on it, it's just locally > driven, so I don't want to add a dependency on device tree where it's > really not needed. (By the way I am further confused by the > undocumented/unused "brcm,40nm-ephy" compatible string that these > device tree bindings for genet have). That compatibility string should not have been defined, but the DTS were imported from our Device Tree auto-generation tool that did produce those, when my TODO list empty, I might send an update to remove those, unless someone thinks it's ABI and it would break something (which I can swear won't). > >> It is not uncommon for MDIO controllers to be re-used and integrated >> within a larger block and when that happens whoever owns the largest >> address space, say the Ethernet MAC can request the large resource >> region and the MDIO bus controler can work on that premise, that's what >> we did with genet/bcmmii.c and mdio-bcm-unimac.c for instance (so we >> only do an ioremap, not request_mem_region + ioremap). >> > > I don't really understand this. In arch/mips/boot/dts, for all of > bcm73xx and bcm74xx SoCs, you have a single Ethernet port DT node, and > a single MDIO bus as a child beneath it, where is this reuse that you > mention? > And because I don't really understand what you've said, my following > comment maybe makes no sense, but I think what you mean by "MDIO > controller reuse" is that there are multiple instantiations of the > register map, but ultimately every transaction ends up on the same > MDIO/MDC pair of wires and the same electrical bus. > We do have some of that with the ENETC, but not with the switch, whose > internal MDIO bus has no connection to the outside world, it just > holds the PCS front-ends for the SerDes. > I also don't understand the reference to request_mem_region, perhaps > it would help if you could show some code. What I forgot telling you about is that the same MDIO bus controller is used internally by each GENET instance to "talk" to both external and internal PHYs, but also by the bcm_sf2.c driver which is why it made sense to have a standalone MDIO bus driver that could either be instantiated on its own (as is the case with bcm_sf2) or as part of a larger block within GENET. The request_mem_region() + ioremap() comment is because you cannot have two resources that overlap be used with request_mem_region(), since the MDIO bus driver is embedded into a larger block, it simply does an ioremap. If that confused you, then you can just discard that comment, is it not particularly relevant. > >> Your commit message does not provide a justification for why this >> abstraction (mii_bus) was not suitable or considered here. Do you think >> that could be changed? >> > > I'm sorry, was the mii_bus abstraction really not considered here? > Based on the stuff exported in this patch, an mii_bus is exactly what > I'm registering in 9/9, no? I meat in the commit message, there is no justification why this was not considered or used, by asking you ended up providing one, that is typically what one would expect to find to explain why something was/was not considered. It's fine, the code is merge, I won't object or require you to use a mii_bus abstraction.
On Tue, 7 Jan 2020 at 06:56, Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 1/6/2020 3:00 PM, Vladimir Oltean wrote: > > Hi Florian, > > > > On Mon, 6 Jan 2020 at 21:35, Florian Fainelli <f.fainelli@gmail.com> wrote: > >> > >> On 1/5/20 5:34 PM, Vladimir Oltean wrote: > >>> From: Claudiu Manoil <claudiu.manoil@nxp.com> > >>> > >>> Within the LS1028A SoC, the register map for the ENETC MDIO controller > >>> is instantiated a few times: for the central (external) MDIO controller, > >>> for the internal bus of each standalone ENETC port, and for the internal > >>> bus of the Felix switch. > >>> > >>> Refactoring is needed to support multiple MDIO buses from multiple > >>> drivers. The enetc_hw structure is made an opaque type and a smaller > >>> enetc_mdio_priv is created. > >>> > >>> 'mdio_base' - MDIO registers base address - is being parameterized, to > >>> be able to work with different MDIO register bases. > >>> > >>> The ENETC MDIO bus operations are exported from the fsl-enetc-mdio > >>> kernel object, the same that registers the central MDIO controller (the > >>> dedicated PF). The ENETC main driver has been changed to select it, and > >>> use its exported helpers to further register its private MDIO bus. The > >>> DSA Felix driver will do the same. > >> > >> This series has already been applied so this may be food for thought at > >> this point, but why was not the solution to create a standalone mii_bus > >> driver and have all consumers be pointed it? > >> > > > > I have no real opinion on this. > > > > To be honest, the reason is that the existing "culture" of Freescale > > MDIO drivers wasn't to put them in drivers/net/phy/mdio-*.c, and I > > just didn't look past the fence. > > > > But what is the benefit? What gets passed between bcmgenet and > > mdio-bcm-unimac with struct bcmgenet_platform_data is equivalent with > > what gets passed between vsc9959 and enetc_mdio with the manual > > population of struct mii_bus and struct enetc_mdio_priv, no? I'm not > > even sure there is a net reduction in code size. And I am not really > > sure that I want an of_node for the MDIO bus platform device anyway. > > Whereas genet seems to be instantiating a port-private MDIO bus for > > the _real_ (but nonetheless embedded) PHY, the MDIO bus we have here > > is for the MAC PCS, which is more akin to the custom device tree > > binding "pcsphy-handle" that the DPAA1 driver is using (see > > arch/arm64/boot/dts/qoriq-fman3-0-10g-0.dtsi for example). So there is > > no requirement to run the PHY state machine on it, it's just locally > > driven, so I don't want to add a dependency on device tree where it's > > really not needed. (By the way I am further confused by the > > undocumented/unused "brcm,40nm-ephy" compatible string that these > > device tree bindings for genet have). > > That compatibility string should not have been defined, but the DTS were > imported from our Device Tree auto-generation tool that did produce > those, when my TODO list empty, I might send an update to remove those, > unless someone thinks it's ABI and it would break something (which I can > swear won't). > > > > >> It is not uncommon for MDIO controllers to be re-used and integrated > >> within a larger block and when that happens whoever owns the largest > >> address space, say the Ethernet MAC can request the large resource > >> region and the MDIO bus controler can work on that premise, that's what > >> we did with genet/bcmmii.c and mdio-bcm-unimac.c for instance (so we > >> only do an ioremap, not request_mem_region + ioremap). > >> > > > > I don't really understand this. In arch/mips/boot/dts, for all of > > bcm73xx and bcm74xx SoCs, you have a single Ethernet port DT node, and > > a single MDIO bus as a child beneath it, where is this reuse that you > > mention? > > And because I don't really understand what you've said, my following > > comment maybe makes no sense, but I think what you mean by "MDIO > > controller reuse" is that there are multiple instantiations of the > > register map, but ultimately every transaction ends up on the same > > MDIO/MDC pair of wires and the same electrical bus. > > We do have some of that with the ENETC, but not with the switch, whose > > internal MDIO bus has no connection to the outside world, it just > > holds the PCS front-ends for the SerDes. > > I also don't understand the reference to request_mem_region, perhaps > > it would help if you could show some code. > > What I forgot telling you about is that the same MDIO bus controller is > used internally by each GENET instance to "talk" to both external and > internal PHYs, but also by the bcm_sf2.c driver which is why it made > sense to have a standalone MDIO bus driver that could either be > instantiated on its own (as is the case with bcm_sf2) or as part of a > larger block within GENET. The request_mem_region() + ioremap() comment > is because you cannot have two resources that overlap be used with > request_mem_region(), since the MDIO bus driver is embedded into a > larger block, it simply does an ioremap. If that confused you, then you > can just discard that comment, is it not particularly relevant. > So we don't typically have the external MDIO controller be part of the memory map of an Ethernet port, it has its own region in the SoC. But this is not an external MDIO controller, and it's a completely separate hardware instance with its own memory region not shared with anybody else. And I think your need for registering the slave MDIO bus in the Starfighter 2 switch driver, on top of the same memory region as the GENET's MDIO bus, is just to work around the fact that the switch pseudo-PHY MDIO address is fixed at 30 on some chips, and you need to intercept those transactions by wrapping the mii_bus->read and mii_bus->write methods of the master, otherwise none of this would have been needed. If I understand correctly, couldn't you have just overridden the mii_bus->read and mii_bus->write ops of the master MDIO bus, without registering another one? > > > >> Your commit message does not provide a justification for why this > >> abstraction (mii_bus) was not suitable or considered here. Do you think > >> that could be changed? > >> > > > > I'm sorry, was the mii_bus abstraction really not considered here? > > Based on the stuff exported in this patch, an mii_bus is exactly what > > I'm registering in 9/9, no? > > I meat in the commit message, there is no justification why this was not > considered or used, by asking you ended up providing one, that is > typically what one would expect to find to explain why something was/was > not considered. It's fine, the code is merge, I won't object or require > you to use a mii_bus abstraction. So I answered your question? If so, it was by mistake, because I still don't exactly understand your point (although I would like to). The difference is that we don't register a new platform device for the mii_bus (we use mii_bus->dev = ocelot->dev), and that it's not in drivers/net/phy/mdio-fsl-enetc.c, although it could be, but I don't see a clear benefit. > -- > Florian Regards, -Vladimir
On 1/7/20 1:46 AM, Vladimir Oltean wrote: > > So we don't typically have the external MDIO controller be part of the > memory map of an Ethernet port, it has its own region in the SoC. But > this is not an external MDIO controller, and it's a completely > separate hardware instance with its own memory region not shared with > anybody else. OK, I do not think I managed to express myself correctly, by external MDIO controller I meant a controller that is used to interface with off-chip (and sometimes on-chip, too) MDIO devices. That is an on-chip MDIO controller technically, pardon me for not expressing myself more clearly here. > > And I think your need for registering the slave MDIO bus in the > Starfighter 2 switch driver, on top of the same memory region as the > GENET's MDIO bus, is just to work around the fact that the switch > pseudo-PHY MDIO address is fixed at 30 on some chips, and you need to > intercept those transactions by wrapping the mii_bus->read and > mii_bus->write methods of the master, otherwise none of this would > have been needed. You are conflating two things that are not related to the point being discussed. The SF2 switch used on 7445 and 7278 has some wrapping around the Roboswitch original IP. The roboswitch has an internal MDIO controller which is managed through the register space within the switch. We added a MDIO controller to interface with both our internal Gigabit PHYs as well as external MDIO devices would such thing exist. The binding show the MDIO node as part of "switch_top" but not being part of "core": Documentation/devicetree/bindings/net/brcm,bcm7445-switch-v4.0.txt. The same IP used as the MDIO controller (those two 32-bit words with CFG and CMD) are the same as those that are used on GENET, hence the desire to re-use the same piece of driver. > If I understand correctly, couldn't you have just overridden the > mii_bus->read and mii_bus->write ops of the master MDIO bus, without > registering another one? Yes maybe, I do not think this is particularly relevant to this discussion though. > >>> >>>> Your commit message does not provide a justification for why this >>>> abstraction (mii_bus) was not suitable or considered here. Do you think >>>> that could be changed? >>>> >>> >>> I'm sorry, was the mii_bus abstraction really not considered here? >>> Based on the stuff exported in this patch, an mii_bus is exactly what >>> I'm registering in 9/9, no? >> >> I meat in the commit message, there is no justification why this was not >> considered or used, by asking you ended up providing one, that is >> typically what one would expect to find to explain why something was/was >> not considered. It's fine, the code is merge, I won't object or require >> you to use a mii_bus abstraction. > > So I answered your question? If so, it was by mistake, because I still > don't exactly understand your point (although I would like to). > The difference is that we don't register a new platform device for the > mii_bus (we use mii_bus->dev = ocelot->dev), and that it's not in > drivers/net/phy/mdio-fsl-enetc.c, although it could be, but I don't > see a clear benefit. To go back to the original point, I suppose the answer is no: we did not consider using a mii_bus abstraction and in hindsight, we do not see the point but it was interesting you brought that up. Case closed from my side, thank you.
diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig index edad4ca46327..fe942de19597 100644 --- a/drivers/net/ethernet/freescale/enetc/Kconfig +++ b/drivers/net/ethernet/freescale/enetc/Kconfig @@ -2,6 +2,7 @@ config FSL_ENETC tristate "ENETC PF driver" depends on PCI && PCI_MSI && (ARCH_LAYERSCAPE || COMPILE_TEST) + select FSL_ENETC_MDIO select PHYLIB help This driver supports NXP ENETC gigabit ethernet controller PCIe diff --git a/drivers/net/ethernet/freescale/enetc/Makefile b/drivers/net/ethernet/freescale/enetc/Makefile index d0db33e5b6b7..74f7ac253b8b 100644 --- a/drivers/net/ethernet/freescale/enetc/Makefile +++ b/drivers/net/ethernet/freescale/enetc/Makefile @@ -3,7 +3,7 @@ common-objs := enetc.o enetc_cbdr.o enetc_ethtool.o obj-$(CONFIG_FSL_ENETC) += fsl-enetc.o -fsl-enetc-y := enetc_pf.o enetc_mdio.o $(common-objs) +fsl-enetc-y := enetc_pf.o $(common-objs) fsl-enetc-$(CONFIG_PCI_IOV) += enetc_msg.o fsl-enetc-$(CONFIG_FSL_ENETC_QOS) += enetc_qos.o diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h index 8375cd886dba..62554f28ce07 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h @@ -200,6 +200,7 @@ enum enetc_bdr_type {TX, RX}; #define ENETC_PFPMR 0x1900 #define ENETC_PFPMR_PMACE BIT(1) #define ENETC_PFPMR_MWLM BIT(0) +#define ENETC_EMDIO_BASE 0x1c00 #define ENETC_PSIUMHFR0(n, err) (((err) ? 0x1d08 : 0x1d00) + (n) * 0x10) #define ENETC_PSIUMHFR1(n) (0x1d04 + (n) * 0x10) #define ENETC_PSIMMHFR0(n, err) (((err) ? 0x1d00 : 0x1d08) + (n) * 0x10) diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c index 149883c8f0b8..18c68e048d43 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c @@ -1,24 +1,35 @@ // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) /* Copyright 2019 NXP */ +#include <linux/fsl/enetc_mdio.h> #include <linux/mdio.h> #include <linux/of_mdio.h> #include <linux/iopoll.h> #include <linux/of.h> -#include "enetc_mdio.h" +#include "enetc_pf.h" -#define ENETC_MDIO_REG_OFFSET 0x1c00 #define ENETC_MDIO_CFG 0x0 /* MDIO configuration and status */ #define ENETC_MDIO_CTL 0x4 /* MDIO control */ #define ENETC_MDIO_DATA 0x8 /* MDIO data */ #define ENETC_MDIO_ADDR 0xc /* MDIO address */ -#define enetc_mdio_rd(hw, off) \ - enetc_port_rd(hw, ENETC_##off + ENETC_MDIO_REG_OFFSET) -#define enetc_mdio_wr(hw, off, val) \ - enetc_port_wr(hw, ENETC_##off + ENETC_MDIO_REG_OFFSET, val) -#define enetc_mdio_rd_reg(off) enetc_mdio_rd(hw, off) +static inline u32 _enetc_mdio_rd(struct enetc_mdio_priv *mdio_priv, int off) +{ + return enetc_port_rd(mdio_priv->hw, mdio_priv->mdio_base + off); +} + +static inline void _enetc_mdio_wr(struct enetc_mdio_priv *mdio_priv, int off, + u32 val) +{ + enetc_port_wr(mdio_priv->hw, mdio_priv->mdio_base + off, val); +} + +#define enetc_mdio_rd(mdio_priv, off) \ + _enetc_mdio_rd(mdio_priv, ENETC_##off) +#define enetc_mdio_wr(mdio_priv, off, val) \ + _enetc_mdio_wr(mdio_priv, ENETC_##off, val) +#define enetc_mdio_rd_reg(off) enetc_mdio_rd(mdio_priv, off) #define ENETC_MDC_DIV 258 @@ -35,7 +46,7 @@ #define MDIO_DATA(x) ((x) & 0xffff) #define TIMEOUT 1000 -static int enetc_mdio_wait_complete(struct enetc_hw *hw) +static int enetc_mdio_wait_complete(struct enetc_mdio_priv *mdio_priv) { u32 val; @@ -46,7 +57,6 @@ static int enetc_mdio_wait_complete(struct enetc_hw *hw) int enetc_mdio_write(struct mii_bus *bus, int phy_id, int regnum, u16 value) { struct enetc_mdio_priv *mdio_priv = bus->priv; - struct enetc_hw *hw = mdio_priv->hw; u32 mdio_ctl, mdio_cfg; u16 dev_addr; int ret; @@ -61,39 +71,39 @@ int enetc_mdio_write(struct mii_bus *bus, int phy_id, int regnum, u16 value) mdio_cfg &= ~MDIO_CFG_ENC45; } - enetc_mdio_wr(hw, MDIO_CFG, mdio_cfg); + enetc_mdio_wr(mdio_priv, MDIO_CFG, mdio_cfg); - ret = enetc_mdio_wait_complete(hw); + ret = enetc_mdio_wait_complete(mdio_priv); if (ret) return ret; /* set port and dev addr */ mdio_ctl = MDIO_CTL_PORT_ADDR(phy_id) | MDIO_CTL_DEV_ADDR(dev_addr); - enetc_mdio_wr(hw, MDIO_CTL, mdio_ctl); + enetc_mdio_wr(mdio_priv, MDIO_CTL, mdio_ctl); /* set the register address */ if (regnum & MII_ADDR_C45) { - enetc_mdio_wr(hw, MDIO_ADDR, regnum & 0xffff); + enetc_mdio_wr(mdio_priv, MDIO_ADDR, regnum & 0xffff); - ret = enetc_mdio_wait_complete(hw); + ret = enetc_mdio_wait_complete(mdio_priv); if (ret) return ret; } /* write the value */ - enetc_mdio_wr(hw, MDIO_DATA, MDIO_DATA(value)); + enetc_mdio_wr(mdio_priv, MDIO_DATA, MDIO_DATA(value)); - ret = enetc_mdio_wait_complete(hw); + ret = enetc_mdio_wait_complete(mdio_priv); if (ret) return ret; return 0; } +EXPORT_SYMBOL_GPL(enetc_mdio_write); int enetc_mdio_read(struct mii_bus *bus, int phy_id, int regnum) { struct enetc_mdio_priv *mdio_priv = bus->priv; - struct enetc_hw *hw = mdio_priv->hw; u32 mdio_ctl, mdio_cfg; u16 dev_addr, value; int ret; @@ -107,86 +117,56 @@ int enetc_mdio_read(struct mii_bus *bus, int phy_id, int regnum) mdio_cfg &= ~MDIO_CFG_ENC45; } - enetc_mdio_wr(hw, MDIO_CFG, mdio_cfg); + enetc_mdio_wr(mdio_priv, MDIO_CFG, mdio_cfg); - ret = enetc_mdio_wait_complete(hw); + ret = enetc_mdio_wait_complete(mdio_priv); if (ret) return ret; /* set port and device addr */ mdio_ctl = MDIO_CTL_PORT_ADDR(phy_id) | MDIO_CTL_DEV_ADDR(dev_addr); - enetc_mdio_wr(hw, MDIO_CTL, mdio_ctl); + enetc_mdio_wr(mdio_priv, MDIO_CTL, mdio_ctl); /* set the register address */ if (regnum & MII_ADDR_C45) { - enetc_mdio_wr(hw, MDIO_ADDR, regnum & 0xffff); + enetc_mdio_wr(mdio_priv, MDIO_ADDR, regnum & 0xffff); - ret = enetc_mdio_wait_complete(hw); + ret = enetc_mdio_wait_complete(mdio_priv); if (ret) return ret; } /* initiate the read */ - enetc_mdio_wr(hw, MDIO_CTL, mdio_ctl | MDIO_CTL_READ); + enetc_mdio_wr(mdio_priv, MDIO_CTL, mdio_ctl | MDIO_CTL_READ); - ret = enetc_mdio_wait_complete(hw); + ret = enetc_mdio_wait_complete(mdio_priv); if (ret) return ret; /* return all Fs if nothing was there */ - if (enetc_mdio_rd(hw, MDIO_CFG) & MDIO_CFG_RD_ER) { + if (enetc_mdio_rd(mdio_priv, MDIO_CFG) & MDIO_CFG_RD_ER) { dev_dbg(&bus->dev, "Error while reading PHY%d reg at %d.%hhu\n", phy_id, dev_addr, regnum); return 0xffff; } - value = enetc_mdio_rd(hw, MDIO_DATA) & 0xffff; + value = enetc_mdio_rd(mdio_priv, MDIO_DATA) & 0xffff; return value; } +EXPORT_SYMBOL_GPL(enetc_mdio_read); -int enetc_mdio_probe(struct enetc_pf *pf) +struct enetc_hw *enetc_hw_alloc(struct device *dev, void __iomem *port_regs) { - struct device *dev = &pf->si->pdev->dev; - struct enetc_mdio_priv *mdio_priv; - struct device_node *np; - struct mii_bus *bus; - int err; - - bus = devm_mdiobus_alloc_size(dev, sizeof(*mdio_priv)); - if (!bus) - return -ENOMEM; - - bus->name = "Freescale ENETC MDIO Bus"; - bus->read = enetc_mdio_read; - bus->write = enetc_mdio_write; - bus->parent = dev; - mdio_priv = bus->priv; - mdio_priv->hw = &pf->si->hw; - snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev)); - - np = of_get_child_by_name(dev->of_node, "mdio"); - if (!np) { - dev_err(dev, "MDIO node missing\n"); - return -EINVAL; - } - - err = of_mdiobus_register(bus, np); - if (err) { - of_node_put(np); - dev_err(dev, "cannot register MDIO bus\n"); - return err; - } + struct enetc_hw *hw; - of_node_put(np); - pf->mdio = bus; + hw = devm_kzalloc(dev, sizeof(*hw), GFP_KERNEL); + if (!hw) + return ERR_PTR(-ENOMEM); - return 0; -} + hw->port = port_regs; -void enetc_mdio_remove(struct enetc_pf *pf) -{ - if (pf->mdio) - mdiobus_unregister(pf->mdio); + return hw; } +EXPORT_SYMBOL_GPL(enetc_hw_alloc); diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mdio.h b/drivers/net/ethernet/freescale/enetc/enetc_mdio.h deleted file mode 100644 index 60c9a3889824..000000000000 --- a/drivers/net/ethernet/freescale/enetc/enetc_mdio.h +++ /dev/null @@ -1,12 +0,0 @@ -/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */ -/* Copyright 2019 NXP */ - -#include <linux/phy.h> -#include "enetc_pf.h" - -struct enetc_mdio_priv { - struct enetc_hw *hw; -}; - -int enetc_mdio_write(struct mii_bus *bus, int phy_id, int regnum, u16 value); -int enetc_mdio_read(struct mii_bus *bus, int phy_id, int regnum); diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c b/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c index fbd41ce01f06..87c0e969da40 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c @@ -1,7 +1,8 @@ // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) /* Copyright 2019 NXP */ +#include <linux/fsl/enetc_mdio.h> #include <linux/of_mdio.h> -#include "enetc_mdio.h" +#include "enetc_pf.h" #define ENETC_MDIO_DEV_ID 0xee01 #define ENETC_MDIO_DEV_NAME "FSL PCIe IE Central MDIO" @@ -13,17 +14,29 @@ static int enetc_pci_mdio_probe(struct pci_dev *pdev, { struct enetc_mdio_priv *mdio_priv; struct device *dev = &pdev->dev; + void __iomem *port_regs; struct enetc_hw *hw; struct mii_bus *bus; int err; - hw = devm_kzalloc(dev, sizeof(*hw), GFP_KERNEL); - if (!hw) - return -ENOMEM; + port_regs = pci_iomap(pdev, 0, 0); + if (!port_regs) { + dev_err(dev, "iomap failed\n"); + err = -ENXIO; + goto err_ioremap; + } + + hw = enetc_hw_alloc(dev, port_regs); + if (IS_ERR(enetc_hw_alloc)) { + err = PTR_ERR(hw); + goto err_hw_alloc; + } bus = devm_mdiobus_alloc_size(dev, sizeof(*mdio_priv)); - if (!bus) - return -ENOMEM; + if (!bus) { + err = -ENOMEM; + goto err_mdiobus_alloc; + } bus->name = ENETC_MDIO_BUS_NAME; bus->read = enetc_mdio_read; @@ -31,13 +44,14 @@ static int enetc_pci_mdio_probe(struct pci_dev *pdev, bus->parent = dev; mdio_priv = bus->priv; mdio_priv->hw = hw; + mdio_priv->mdio_base = ENETC_EMDIO_BASE; snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev)); pcie_flr(pdev); err = pci_enable_device_mem(pdev); if (err) { dev_err(dev, "device enable failed\n"); - return err; + goto err_pci_enable; } err = pci_request_region(pdev, 0, KBUILD_MODNAME); @@ -46,13 +60,6 @@ static int enetc_pci_mdio_probe(struct pci_dev *pdev, goto err_pci_mem_reg; } - hw->port = pci_iomap(pdev, 0, 0); - if (!hw->port) { - err = -ENXIO; - dev_err(dev, "iomap failed\n"); - goto err_ioremap; - } - err = of_mdiobus_register(bus, dev->of_node); if (err) goto err_mdiobus_reg; @@ -62,12 +69,14 @@ static int enetc_pci_mdio_probe(struct pci_dev *pdev, return 0; err_mdiobus_reg: - iounmap(mdio_priv->hw->port); -err_ioremap: pci_release_mem_regions(pdev); err_pci_mem_reg: pci_disable_device(pdev); - +err_pci_enable: +err_mdiobus_alloc: + iounmap(port_regs); +err_hw_alloc: +err_ioremap: return err; } diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c index e7482d483b28..fc0d7d99e9a1 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c @@ -2,6 +2,7 @@ /* Copyright 2017-2019 NXP */ #include <linux/module.h> +#include <linux/fsl/enetc_mdio.h> #include <linux/of_mdio.h> #include <linux/of_net.h> #include "enetc_pf.h" @@ -749,6 +750,52 @@ static void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, enetc_get_primary_mac_addr(&si->hw, ndev->dev_addr); } +static int enetc_mdio_probe(struct enetc_pf *pf) +{ + struct device *dev = &pf->si->pdev->dev; + struct enetc_mdio_priv *mdio_priv; + struct device_node *np; + struct mii_bus *bus; + int err; + + bus = devm_mdiobus_alloc_size(dev, sizeof(*mdio_priv)); + if (!bus) + return -ENOMEM; + + bus->name = "Freescale ENETC MDIO Bus"; + bus->read = enetc_mdio_read; + bus->write = enetc_mdio_write; + bus->parent = dev; + mdio_priv = bus->priv; + mdio_priv->hw = &pf->si->hw; + mdio_priv->mdio_base = ENETC_EMDIO_BASE; + snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev)); + + np = of_get_child_by_name(dev->of_node, "mdio"); + if (!np) { + dev_err(dev, "MDIO node missing\n"); + return -EINVAL; + } + + err = of_mdiobus_register(bus, np); + if (err) { + of_node_put(np); + dev_err(dev, "cannot register MDIO bus\n"); + return err; + } + + of_node_put(np); + pf->mdio = bus; + + return 0; +} + +static void enetc_mdio_remove(struct enetc_pf *pf) +{ + if (pf->mdio) + mdiobus_unregister(pf->mdio); +} + static int enetc_of_get_phy(struct enetc_ndev_priv *priv) { struct enetc_pf *pf = enetc_si_priv(priv->si); diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.h b/drivers/net/ethernet/freescale/enetc/enetc_pf.h index 10dd1b53bb08..59e65a6f6c3e 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.h +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.h @@ -49,7 +49,3 @@ struct enetc_pf { int enetc_msg_psi_init(struct enetc_pf *pf); void enetc_msg_psi_free(struct enetc_pf *pf); void enetc_msg_handle_rxmsg(struct enetc_pf *pf, int mbox_id, u16 *status); - -/* MDIO */ -int enetc_mdio_probe(struct enetc_pf *pf); -void enetc_mdio_remove(struct enetc_pf *pf); diff --git a/include/linux/fsl/enetc_mdio.h b/include/linux/fsl/enetc_mdio.h new file mode 100644 index 000000000000..4875dd38af7e --- /dev/null +++ b/include/linux/fsl/enetc_mdio.h @@ -0,0 +1,55 @@ +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */ +/* Copyright 2019 NXP */ + +#ifndef _FSL_ENETC_MDIO_H_ +#define _FSL_ENETC_MDIO_H_ + +#include <linux/phy.h> + +/* PCS registers */ +#define ENETC_PCS_LINK_TIMER1 0x12 +#define ENETC_PCS_LINK_TIMER1_VAL 0x06a0 +#define ENETC_PCS_LINK_TIMER2 0x13 +#define ENETC_PCS_LINK_TIMER2_VAL 0x0003 +#define ENETC_PCS_IF_MODE 0x14 +#define ENETC_PCS_IF_MODE_SGMII_EN BIT(0) +#define ENETC_PCS_IF_MODE_USE_SGMII_AN BIT(1) +#define ENETC_PCS_IF_MODE_SGMII_SPEED(x) (((x) << 2) & GENMASK(3, 2)) + +/* Not a mistake, the SerDes PLL needs to be set at 3.125 GHz by Reset + * Configuration Word (RCW, outside Linux control) for 2.5G SGMII mode. The PCS + * still thinks it's at gigabit. + */ +enum enetc_pcs_speed { + ENETC_PCS_SPEED_10 = 0, + ENETC_PCS_SPEED_100 = 1, + ENETC_PCS_SPEED_1000 = 2, + ENETC_PCS_SPEED_2500 = 2, +}; + +struct enetc_hw; + +struct enetc_mdio_priv { + struct enetc_hw *hw; + int mdio_base; +}; + +#if IS_REACHABLE(CONFIG_FSL_ENETC_MDIO) + +int enetc_mdio_read(struct mii_bus *bus, int phy_id, int regnum); +int enetc_mdio_write(struct mii_bus *bus, int phy_id, int regnum, u16 value); +struct enetc_hw *enetc_hw_alloc(struct device *dev, void __iomem *port_regs); + +#else + +static inline int enetc_mdio_read(struct mii_bus *bus, int phy_id, int regnum) +{ return -EINVAL; } +static inline int enetc_mdio_write(struct mii_bus *bus, int phy_id, int regnum, + u16 value) +{ return -EINVAL; } +struct enetc_hw *enetc_hw_alloc(struct device *dev, void __iomem *port_regs) +{ return ERR_PTR(-EINVAL); } + +#endif + +#endif