| Message ID | 1563979301-596-1-git-send-email-claudiu.manoil@nxp.com |
|---|---|
| Headers | show |
| Series | enetc: Add mdio bus driver for the PCIe MDIO endpoint | expand |
On Wed, Jul 24, 2019 at 05:41:38PM +0300, Claudiu Manoil wrote: > Though it works, this is not how it should have been. > What's needed is a pointer to the mdio registers. > Store it properly inside bus->priv allocated space. > Use devm_* variant to further clean up the init error / > remove paths. > > Fixes following sparse warning: > warning: incorrect type in assignment (different address spaces) > expected void *priv > got struct enetc_mdio_regs [noderef] <asn:2>*[assigned] regs > > Fixes: ebfcb23d62ab ("enetc: Add ENETC PF level external MDIO support") > > Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> > --- > v1 - added this patch > > .../net/ethernet/freescale/enetc/enetc_mdio.c | 31 +++++++------------ > 1 file changed, 12 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c > index 77b9cd10ba2b..1e3cd21c13ee 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c > @@ -15,7 +15,8 @@ struct enetc_mdio_regs { > u32 mdio_addr; /* MDIO address */ > }; > > -#define bus_to_enetc_regs(bus) (struct enetc_mdio_regs __iomem *)((bus)->priv) > +#define bus_to_enetc_regs(bus) (*(struct enetc_mdio_regs __iomem **) \ > + ((bus)->priv)) > > #define ENETC_MDIO_REG_OFFSET 0x1c00 > #define ENETC_MDC_DIV 258 > @@ -146,12 +147,12 @@ static int enetc_mdio_read(struct mii_bus *bus, int phy_id, int regnum) > int enetc_mdio_probe(struct enetc_pf *pf) > { > struct device *dev = &pf->si->pdev->dev; > - struct enetc_mdio_regs __iomem *regs; > + struct enetc_mdio_regs __iomem **regsp; > struct device_node *np; > struct mii_bus *bus; > - int ret; > + int err; > > - bus = mdiobus_alloc_size(sizeof(regs)); > + bus = devm_mdiobus_alloc_size(dev, sizeof(*regsp)); > if (!bus) > return -ENOMEM; > > @@ -159,41 +160,33 @@ int enetc_mdio_probe(struct enetc_pf *pf) > bus->read = enetc_mdio_read; > bus->write = enetc_mdio_write; > bus->parent = dev; > + regsp = bus->priv; > snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev)); > > /* store the enetc mdio base address for this bus */ > - regs = pf->si->hw.port + ENETC_MDIO_REG_OFFSET; > - bus->priv = regs; > + *regsp = pf->si->hw.port + ENETC_MDIO_REG_OFFSET; This is all very odd and different to every other driver. If i get the code write, there are 4 registers, each u32 in size, starting at pf->si->hw.port + ENETC_MDIO_REG_OFFSET? There are macros like enetc_port_wr() and enetc_global_wr(). It think it would be much cleaner to add a macro enet_mdio_wr() which takes hw, off, val. #define enet_mdio_wr(hw, off, val) enet_port_wr(hw, off + ENETC_MDIO_REG_OFFSET, val) struct enetc_mdio_priv { struct enetc_hw *hw; } struct enetc_mdio_priv *mdio_priv; bus = devm_mdiobus_alloc_size(dev, sizeof(*mdio_priv)); mdio_priv = bus->priv; mdio_priv->hw = pf->si->hw; static int enetc_mdio_write(struct mii_bus *bus, int phy_id, int regnum, u16 value) { struct enetc_mdio_priv *mdio_priv = bus->priv; ... enet_mdio_wr(priv->hw, ENETC_MDIO_CFG, mdio_cfg); } All the horrible casts go away, the driver is structured like every other driver, sparse is probably happy, etc. Andrew
>-----Original Message----- >From: Andrew Lunn <andrew@lunn.ch> >Sent: Wednesday, July 24, 2019 6:18 PM >To: Claudiu Manoil <claudiu.manoil@nxp.com> >Cc: David S . Miller <davem@davemloft.net>; Rob Herring ><robh+dt@kernel.org>; Leo Li <leoyang.li@nxp.com>; Alexandru Marginean ><alexandru.marginean@nxp.com>; netdev@vger.kernel.org; >devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >kernel@vger.kernel.org >Subject: Re: [PATCH net-next v1 1/4] enetc: Clean up local mdio bus allocation > >On Wed, Jul 24, 2019 at 05:41:38PM +0300, Claudiu Manoil wrote: >> Though it works, this is not how it should have been. >> What's needed is a pointer to the mdio registers. >> Store it properly inside bus->priv allocated space. >> Use devm_* variant to further clean up the init error / >> remove paths. >> >> Fixes following sparse warning: >> warning: incorrect type in assignment (different address spaces) >> expected void *priv >> got struct enetc_mdio_regs [noderef] <asn:2>*[assigned] regs >> >> Fixes: ebfcb23d62ab ("enetc: Add ENETC PF level external MDIO support") >> >> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> >> --- >> v1 - added this patch >> >> .../net/ethernet/freescale/enetc/enetc_mdio.c | 31 +++++++------------ >> 1 file changed, 12 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c >b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c >> index 77b9cd10ba2b..1e3cd21c13ee 100644 >> --- a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c >> +++ b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c >> @@ -15,7 +15,8 @@ struct enetc_mdio_regs { >> u32 mdio_addr; /* MDIO address */ >> }; >> >> -#define bus_to_enetc_regs(bus) (struct enetc_mdio_regs __iomem >*)((bus)->priv) >> +#define bus_to_enetc_regs(bus) (*(struct enetc_mdio_regs __iomem >**) \ >> + ((bus)->priv)) >> >> #define ENETC_MDIO_REG_OFFSET 0x1c00 >> #define ENETC_MDC_DIV 258 >> @@ -146,12 +147,12 @@ static int enetc_mdio_read(struct mii_bus *bus, int >phy_id, int regnum) >> int enetc_mdio_probe(struct enetc_pf *pf) >> { >> struct device *dev = &pf->si->pdev->dev; >> - struct enetc_mdio_regs __iomem *regs; >> + struct enetc_mdio_regs __iomem **regsp; >> struct device_node *np; >> struct mii_bus *bus; >> - int ret; >> + int err; >> >> - bus = mdiobus_alloc_size(sizeof(regs)); >> + bus = devm_mdiobus_alloc_size(dev, sizeof(*regsp)); >> if (!bus) >> return -ENOMEM; >> >> @@ -159,41 +160,33 @@ int enetc_mdio_probe(struct enetc_pf *pf) >> bus->read = enetc_mdio_read; >> bus->write = enetc_mdio_write; >> bus->parent = dev; >> + regsp = bus->priv; >> snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev)); >> >> /* store the enetc mdio base address for this bus */ >> - regs = pf->si->hw.port + ENETC_MDIO_REG_OFFSET; >> - bus->priv = regs; >> + *regsp = pf->si->hw.port + ENETC_MDIO_REG_OFFSET; > >This is all very odd and different to every other driver. > >If i get the code write, there are 4 registers, each u32 in size, >starting at pf->si->hw.port + ENETC_MDIO_REG_OFFSET? > >There are macros like enetc_port_wr() and enetc_global_wr(). It think >it would be much cleaner to add a macro enet_mdio_wr() which takes >hw, off, val. > >#define enet_mdio_wr(hw, off, val) enet_port_wr(hw, off + >ENETC_MDIO_REG_OFFSET, val) > >struct enetc_mdio_priv { > struct enetc_hw *hw; >} > > struct enetc_mdio_priv *mdio_priv; > > bus = devm_mdiobus_alloc_size(dev, sizeof(*mdio_priv)); > > mdio_priv = bus->priv; > mdio_priv->hw = pf->si->hw; > > >static int enetc_mdio_write(struct mii_bus *bus, int phy_id, int regnum, > u16 value) >{ > struct enetc_mdio_priv *mdio_priv = bus->priv; >... > enet_mdio_wr(priv->hw, ENETC_MDIO_CFG, mdio_cfg); >} > >All the horrible casts go away, the driver is structured like every >other driver, sparse is probably happy, etc. > This looks more like a matter cosmetic preferences. I mean, I didn't notice anything "horrible" in the code so far. I actually find it more ugly to define a new structure with only one element inside, like: struct enetc_mdio_priv { struct enetc_hw *hw; } What is this technique called? Looks like a second type definition for another type. Anyway, if others already did this in the kernel, what can I do?
> >All the horrible casts go away, the driver is structured like every > >other driver, sparse is probably happy, etc. > > > > This looks more like a matter cosmetic preferences. I mean, I didn't > notice anything "horrible" in the code so far. #define bus_to_enetc_regs(bus) (struct enetc_mdio_regs __iomem *)((bus)->priv) You should not need a cast here, bus->priv is a void *. But bus->priv is being abused to hold a __iomem pointer. enetc_wr_reg(®s->mdio_cfg, mdio_cfg); This is also rather odd, passing the address of something to an IO operator? I also don't know the C standard well enough to know if it is guaranteed that: struct enetc_mdio_regs { u32 mdio_cfg; /* MDIO configuration and status */ u32 mdio_ctl; /* MDIO control */ u32 mdio_data; /* MDIO data */ u32 mdio_addr; /* MDIO address */ }; actually works. On a 64bit system is the compiler allowed to put in padding to keep the u32 64 bit aligned? > I actually find it more > ugly to define a new structure with only one element inside, like: > struct enetc_mdio_priv { > struct enetc_hw *hw; > } One advantage of this is that struct enetc_hw correctly has all the __iomem attributes. All the casts to __iomem go away, and sparse is happy. > Anyway, if others already did this in the kernel, what can I do? Clean it up. Make the code more readable and easy to maintain. Andrew
Second patch just registers the PCIe endpoint device containing the MDIO registers as a standalone MDIO bus driver, to allow an alternative way to control the MDIO bus. The same code used by the ENETC ports (eth controllers) to manage MDIO via local registers applies and is reused. Bindings are provided for the new MDIO node, similarly to ENETC port nodes bindings. Last patch enables the ENETC port 1 and its RGMII PHY on the LS1028A QDS board, where the MDIO muxing configuration relies on the MDIO support provided in the first patch. Claudiu Manoil (4): enetc: Clean up local mdio bus allocation enetc: Add mdio bus driver for the PCIe MDIO endpoint dt-bindings: net: fsl: enetc: Add bindings for the central MDIO PCIe endpoint arm64: dts: fsl: ls1028a: Enable eth port1 on the ls1028a QDS board .../devicetree/bindings/net/fsl-enetc.txt | 42 ++++++- .../boot/dts/freescale/fsl-ls1028a-qds.dts | 40 ++++++ .../arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 6 + .../net/ethernet/freescale/enetc/enetc_mdio.c | 119 +++++++++++++++--- .../net/ethernet/freescale/enetc/enetc_pf.c | 5 +- 5 files changed, 190 insertions(+), 22 deletions(-)