[PATCH] net/fsl_pq_mdio: fix handling of TBIPA register

[PATCH] net/fsl_pq_mdio: fix handling of TBIPA register

[Lutz Jaenicke]

The TBIPA register is part of gianfar's full register set. When starting
from the MII registers, the start address of struct gfar needs to
be determined via container_of().
Experienced with mpc8313 and "fsl,gianfar-mdio" device tree entries.

Signed-off-by: Lutz Jaenicke <ljaenicke@innominate.com>
---
 drivers/net/ethernet/freescale/fsl_pq_mdio.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index c93a056..9485fdb 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -193,7 +193,8 @@ static int fsl_pq_mdio_reset(struct mii_bus *bus)
  */
 static uint32_t __iomem *get_gfar_tbipa(void __iomem *p)
 {
-	struct gfar __iomem *enet_regs = p;
+	struct gfar __iomem *enet_regs = 
+		container_of(p, struct gfar, gfar_mii_regs);
 
 	return &enet_regs->tbipa;
 }
-- 
1.7.10.4

Re: [PATCH] net/fsl_pq_mdio: fix handling of TBIPA register

[Scott Wood]

On Thu, 2013-08-01 at 19:49 +0200, Lutz Jaenicke wrote:
> The TBIPA register is part of gianfar's full register set. When starting
> from the MII registers, the start address of struct gfar needs to
> be determined via container_of().
> Experienced with mpc8313 and "fsl,gianfar-mdio" device tree entries.
> 
> Signed-off-by: Lutz Jaenicke <ljaenicke@innominate.com>
> ---
>  drivers/net/ethernet/freescale/fsl_pq_mdio.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> index c93a056..9485fdb 100644
> --- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> +++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> @@ -193,7 +193,8 @@ static int fsl_pq_mdio_reset(struct mii_bus *bus)
>   */
>  static uint32_t __iomem *get_gfar_tbipa(void __iomem *p)
>  {
> -	struct gfar __iomem *enet_regs = p;
> +	struct gfar __iomem *enet_regs = 
> +		container_of(p, struct gfar, gfar_mii_regs);
>  
>  	return &enet_regs->tbipa;
>  }

Please send this to the netdev list/maintainer.

-Scott

Re: [PATCH] net/fsl_pq_mdio: fix handling of TBIPA register

[Scott Wood]

On Tue, 2013-08-06 at 18:10 -0500, Scott Wood wrote:
> On Thu, 2013-08-01 at 19:49 +0200, Lutz Jaenicke wrote:
> > The TBIPA register is part of gianfar's full register set. When starting
> > from the MII registers, the start address of struct gfar needs to
> > be determined via container_of().
> > Experienced with mpc8313 and "fsl,gianfar-mdio" device tree entries.
> > 
> > Signed-off-by: Lutz Jaenicke <ljaenicke@innominate.com>
> > ---
> >  drivers/net/ethernet/freescale/fsl_pq_mdio.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> > index c93a056..9485fdb 100644
> > --- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> > +++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> > @@ -193,7 +193,8 @@ static int fsl_pq_mdio_reset(struct mii_bus *bus)
> >   */
> >  static uint32_t __iomem *get_gfar_tbipa(void __iomem *p)
> >  {
> > -	struct gfar __iomem *enet_regs = p;
> > +	struct gfar __iomem *enet_regs = 
> > +		container_of(p, struct gfar, gfar_mii_regs);
> >  
> >  	return &enet_regs->tbipa;
> >  }
> 
> Please send this to the netdev list/maintainer.

Though, do we have any guarantee that p is contained by a "struct gfar"?
It looks like the code of_iomap()s the reg of the mdio node, which is
just the MDIO registers.  Don't access outside that area.

Of course, this register probably *should* be described in the MDIO node
(see http://patchwork.ozlabs.org/patch/250766/), but since it isn't,
we'll need to either find the associated full gianfar device and ask it
to set tbipa, or look up the physical address of tbipa and do a separate
ioremap.  I know that in practice we'll have at least 4K-granular
mappings and thus tbipa will be there, but it's best to avoid such
hacks.

-Scott

Re: [PATCH] net/fsl_pq_mdio: fix handling of TBIPA register

[Lutz Jaenicke]

On Tue, Aug 06, 2013 at 06:55:00PM -0500, Scott Wood wrote:
> On Tue, 2013-08-06 at 18:10 -0500, Scott Wood wrote:
> > On Thu, 2013-08-01 at 19:49 +0200, Lutz Jaenicke wrote:
> > > The TBIPA register is part of gianfar's full register set. When starting
> > > from the MII registers, the start address of struct gfar needs to
> > > be determined via container_of().
> > > Experienced with mpc8313 and "fsl,gianfar-mdio" device tree entries.
> > > 
> > > Signed-off-by: Lutz Jaenicke <ljaenicke@innominate.com>
> > > ---
> > >  drivers/net/ethernet/freescale/fsl_pq_mdio.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> > > index c93a056..9485fdb 100644
> > > --- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> > > +++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> > > @@ -193,7 +193,8 @@ static int fsl_pq_mdio_reset(struct mii_bus *bus)
> > >   */
> > >  static uint32_t __iomem *get_gfar_tbipa(void __iomem *p)
> > >  {
> > > -	struct gfar __iomem *enet_regs = p;
> > > +	struct gfar __iomem *enet_regs = 
> > > +		container_of(p, struct gfar, gfar_mii_regs);
> > >  
> > >  	return &enet_regs->tbipa;
> > >  }
> > 
> > Please send this to the netdev list/maintainer.
> 
> Though, do we have any guarantee that p is contained by a "struct gfar"?
> It looks like the code of_iomap()s the reg of the mdio node, which is
> just the MDIO registers.  Don't access outside that area.

If I did not understand the setup totally wrong, The proposed patch just
restores the behavior before application of
  commit afae5ad78b342f401c28b0bb1adb3cd494cb125a
  Author: Timur Tabi <timur@freescale.com>
  Date:   Wed Aug 29 08:08:01 2012 +0000

    net/fsl_pq_mdio: streamline probing of MDIO nodes

I guess that is meant with
  /*
   * This is mildly evil, but so is our hardware for doing this.
   * Also, we have to cast back to struct gfar because of
   * definition weirdness done in gianfar.h.
   */

> Of course, this register probably *should* be described in the MDIO node
> (see http://patchwork.ozlabs.org/patch/250766/), but since it isn't,
> we'll need to either find the associated full gianfar device and ask it
> to set tbipa, or look up the physical address of tbipa and do a separate
> ioremap.  I know that in practice we'll have at least 4K-granular
> mappings and thus tbipa will be there, but it's best to avoid such
> hacks.

This patch does make some sense, it however only covers the pure
fsl,gianfar-tbi case (just a TBI PHY) but not the fsl,gianfar-mdio
with a full MDIO (@24520). On my MPC8313 based custom board I have
a PHY at address 0 and I can only access this PHY if the TBI PHY's address
is first rewritten to another id (from the "0" which is the reset
default).
The device trees shipped with Linux indicate that the boards listed
typically do not have PHYs at address 0 so that the effect won't be
noted. (And: writing the TBI-PHY's mdio_bus-address into another
register (with offset 0x520!?) instead of the real TBIPA register
does not seem to create any ill effects.

As to ask the gianfar device: It is fine to setup the mdio_bus first and
only load the gianfar module later, so at least the gianfar driver itself
cannot do it.

Best regards,
	Lutz
-- 
Dr.-Ing. Lutz Jänicke
CTO
Innominate Security Technologies AG  /protecting industrial networks/
tel: +49.30.921028-200
fax: +49.30.921028-020
Rudower Chaussee 13
D-12489 Berlin, Germany
www.innominate.com

Register Court: AG Charlottenburg, HR B 81603
Management Board: Dirk Seewald
Chairman of the Supervisory Board: Christoph Leifer

Re: [PATCH] net/fsl_pq_mdio: fix handling of TBIPA register

[Scott Wood]

On Wed, 2013-08-07 at 16:22 +0200, Lutz Jaenicke wrote:
> On Tue, Aug 06, 2013 at 06:55:00PM -0500, Scott Wood wrote:
> > On Tue, 2013-08-06 at 18:10 -0500, Scott Wood wrote:
> > > On Thu, 2013-08-01 at 19:49 +0200, Lutz Jaenicke wrote:
> > > > The TBIPA register is part of gianfar's full register set. When starting
> > > > from the MII registers, the start address of struct gfar needs to
> > > > be determined via container_of().
> > > > Experienced with mpc8313 and "fsl,gianfar-mdio" device tree entries.
> > > > 
> > > > Signed-off-by: Lutz Jaenicke <ljaenicke@innominate.com>
> > > > ---
> > > >  drivers/net/ethernet/freescale/fsl_pq_mdio.c |    3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> > > > index c93a056..9485fdb 100644
> > > > --- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> > > > +++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
> > > > @@ -193,7 +193,8 @@ static int fsl_pq_mdio_reset(struct mii_bus *bus)
> > > >   */
> > > >  static uint32_t __iomem *get_gfar_tbipa(void __iomem *p)
> > > >  {
> > > > -	struct gfar __iomem *enet_regs = p;
> > > > +	struct gfar __iomem *enet_regs = 
> > > > +		container_of(p, struct gfar, gfar_mii_regs);
> > > >  
> > > >  	return &enet_regs->tbipa;
> > > >  }
> > > 
> > > Please send this to the netdev list/maintainer.
> > 
> > Though, do we have any guarantee that p is contained by a "struct gfar"?
> > It looks like the code of_iomap()s the reg of the mdio node, which is
> > just the MDIO registers.  Don't access outside that area.
> 
> If I did not understand the setup totally wrong, The proposed patch just
> restores the behavior before application of
>   commit afae5ad78b342f401c28b0bb1adb3cd494cb125a
>   Author: Timur Tabi <timur@freescale.com>
>   Date:   Wed Aug 29 08:08:01 2012 +0000
> 
>     net/fsl_pq_mdio: streamline probing of MDIO nodes

It's not exactly new ugliness, but it's still ugly. :-)

Though I think you need to go back before commit
1d2397d742b7a2b39b2f09dd9da3b9d1463f55e9 ("fsl_pq_mdio: Add Suport for
etsec2.0 devices.") for the offset to be "properly" adjusted.

> > Of course, this register probably *should* be described in the MDIO node
> > (see http://patchwork.ozlabs.org/patch/250766/), but since it isn't,
> > we'll need to either find the associated full gianfar device and ask it
> > to set tbipa, or look up the physical address of tbipa and do a separate
> > ioremap.  I know that in practice we'll have at least 4K-granular
> > mappings and thus tbipa will be there, but it's best to avoid such
> > hacks.
> 
> This patch does make some sense, it however only covers the pure
> fsl,gianfar-tbi case (just a TBI PHY) but not the fsl,gianfar-mdio
> with a full MDIO (@24520). 

I'm not seeing any difference between the address for fsl,gianfar-tbi
and fsl,gianfar-mdio.

> On my MPC8313 based custom board I have
> a PHY at address 0 and I can only access this PHY if the TBI PHY's address
> is first rewritten to another id (from the "0" which is the reset
> default).

Yes, I understand the need to write TBIPA -- I was just hoping it could
be done in a less hacky way.

> As to ask the gianfar device: It is fine to setup the mdio_bus first and
> only load the gianfar module later, so at least the gianfar driver itself
> cannot do it.

You could look up the main gianfar node from the mdio driver.  Or at
least modify the ioremap so that it covers the whole struct gfar,
instead of using of_iomap().  It'd still be a little hacky but only in
that we're recognizing a bad device tree and interpreting it how it
should have been written, rather than accessing outside the bounds of an
ioremap request.

-Scott