[U-Boot] [PATCH 15/28] net/memac_phy: reuse driver for little endian SoCs

Shaohui Xie Shaohui.Xie at freescale.com
Fri Mar 20 04:06:16 CET 2015


Hello Joe,

Thank you for reviewing this patch!
Please see inline.

Best Regards,
Shaohui Xie

From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
Sent: Friday, March 20, 2015 2:04 AM
To: Sun York-R58495
Cc: u-boot; Joe Hershberger; Xie Shaohui-B21989
Subject: Re: [U-Boot] [PATCH 15/28] net/memac_phy: reuse driver for little endian SoCs

Hi Shaohui Xie,

On Thu, Mar 19, 2015 at 11:45 AM, York Sun <yorksun at freescale.com<mailto:yorksun at freescale.com>> wrote:
>
> From: Shaohui Xie <Shaohui.Xie at freescale.com<mailto:Shaohui.Xie at freescale.com>>
>
> The memac for PHY management on little endian SoCs is similar on big
> endian SoCs, so we modify the driver by using I/O accessor function to
> handle the endianness, so the driver can be reused on little endian
> SoCs, we introduce CONFIG_SYS_MEMAC_LITTLE_ENDIAN for little endian
> SoCs, if the CONFIG_SYS_MEMAC_LITTLE_ENDIAN is defined, the I/O access
> is little endian, if not, the I/O access is big endian. Move fsl_memac.h
> out of powerpc include.
>
> Signed-off-by: Shaohui Xie <Shaohui.Xie at freescale.com<mailto:Shaohui.Xie at freescale.com>>
> CC: Joe Hershberger <joe.hershberger at ni.com<mailto:joe.hershberger at ni.com>>
> ---
>  arch/arm/include/asm/arch-fsl-lsch3/config.h      |    1 +
>  drivers/net/Makefile                              |    1 +
>  drivers/net/fm/eth.c                              |    2 +-
>  drivers/net/fm/memac.c                            |    2 +-
>  drivers/net/fm/memac_phy.c                        |   62 ++++++++++++++-------
>  drivers/net/vsc9953.c                             |    2 +-
>  {arch/powerpc/include/asm => include}/fsl_memac.h |    0
>  7 files changed, 46 insertions(+), 24 deletions(-)
>  rename {arch/powerpc/include/asm => include}/fsl_memac.h (100%)
>
> diff --git a/arch/arm/include/asm/arch-fsl-lsch3/config.h b/arch/arm/include/asm/arch-fsl-lsch3/config.h
> index 98db1ef..684c70f 100644
> --- a/arch/arm/include/asm/arch-fsl-lsch3/config.h
> +++ b/arch/arm/include/asm/arch-fsl-lsch3/config.h
> @@ -109,6 +109,7 @@
>
>  /* IFC */
>  #define CONFIG_SYS_FSL_IFC_LE
> +#define CONFIG_SYS_MEMAC_LITTLE_ENDIAN

It seems tedious to have to define this. Can't you just use the functions available?
[S.H] To use a define is based on a concern that we cannot assume the I/O access of an IP share same endianness as the Soc(s), we cannot assume on little endian Soc the I/O access is little endian, on big endian Soc the I/O access is big endian, the I/O access could be little endian on big endian Soc and vice versa.


>  /* PCIe */
>  #define CONFIG_SYS_PCIE1_ADDR                  (CONFIG_SYS_IMMR + 0x2400000)
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 5497934..d871093 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -66,4 +66,5 @@ obj-$(CONFIG_XILINX_LL_TEMAC) += xilinx_ll_temac.o xilinx_ll_temac_mdio.o \
>  obj-$(CONFIG_ZYNQ_GEM) += zynq_gem.o
>  obj-$(CONFIG_FSL_MC_ENET) += fsl-mc/
>  obj-$(CONFIG_FSL_MC_ENET) += ldpaa_eth/
> +obj-$(CONFIG_FSL_MEMAC) += fm/memac_phy.o
>  obj-$(CONFIG_VSC9953) += vsc9953.o
> diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c
> index 1d1089d..a7a5c69 100644
> --- a/drivers/net/fm/eth.c
> +++ b/drivers/net/fm/eth.c
> @@ -15,7 +15,7 @@
>  #include <phy.h>
>  #include <asm/fsl_dtsec.h>
>  #include <asm/fsl_tgec.h>
> -#include <asm/fsl_memac.h>
> +#include <fsl_memac.h>
>
>  #include "fm.h"
>
> diff --git a/drivers/net/fm/memac.c b/drivers/net/fm/memac.c
> index 60e898c..81a64bf 100644
> --- a/drivers/net/fm/memac.c
> +++ b/drivers/net/fm/memac.c
> @@ -12,7 +12,7 @@
>  #include <phy.h>
>  #include <asm/types.h>
>  #include <asm/io.h>
> -#include <asm/fsl_memac.h>
> +#include <fsl_memac.h>
>
>  #include "fm.h"
>
> diff --git a/drivers/net/fm/memac_phy.c b/drivers/net/fm/memac_phy.c
> index a155d89..4ab78e6 100644
> --- a/drivers/net/fm/memac_phy.c
> +++ b/drivers/net/fm/memac_phy.c
> @@ -10,9 +10,28 @@
>  #include <miiphy.h>
>  #include <phy.h>
>  #include <asm/io.h>
> -#include <asm/fsl_memac.h>
> +#include <fsl_memac.h>
>  #include <fm_eth.h>
>
> +#ifdef CONFIG_SYS_MEMAC_LITTLE_ENDIAN
This can already be detected, right?

#if __BYTE_ORDER == __LITTLE_ENDIAN
[S.H] The issue is the IP’s I/O access order on LS2 is different from big endian Soc(s).
> +#define memac_out_32(a, v)     out_le32(a, v)
> +#define memac_clrbits_32(a, v) clrbits_le32(a, v)
> +#define memac_setbits_32(a, v) setbits_le32(a, v)
> +#else
> +#define memac_out_32(a, v)     out_be32(a, v)
> +#define memac_clrbits_32(a, v) clrbits_be32(a, v)
> +#define memac_setbits_32(a, v) setbits_be32(a, v)
> +#endif
> +
> +static u32 memac_in_32(u32 *reg)
> +{
> +#ifdef CONFIG_SYS_MEMAC_LITTLE_ENDIAN
> +       return in_le32(reg);
> +#else
> +       return in_be32(reg);
> +#endif
> +}
Another option you have is to take the approach that you don't care the endianness. Something like using the this type of pattern:

value = ntohl(in_be32(reg));
out_be32(reg, htonl(value));
[S.H] same concern as above.

> +
>  /*
>   * Write value to the PHY for this device to the register at regnum, waiting
>   * until the write is done before it returns.  All PHY configuration has to be
> @@ -28,31 +47,31 @@ int memac_mdio_write(struct mii_dev *bus, int port_addr, int dev_addr,
>         if (dev_addr == MDIO_DEVAD_NONE) {
>                 c45 = 0; /* clause 22 */
>                 dev_addr = regnum & 0x1f;
> -               clrbits_be32(&regs->mdio_stat, MDIO_STAT_ENC);
> +               memac_clrbits_32(&regs->mdio_stat, MDIO_STAT_ENC);
>         } else
> -               setbits_be32(&regs->mdio_stat, MDIO_STAT_ENC);
> +               memac_setbits_32(&regs->mdio_stat, MDIO_STAT_ENC);
>
>         /* Wait till the bus is free */
> -       while ((in_be32(&regs->mdio_stat)) & MDIO_STAT_BSY)
> +       while ((memac_in_32(&regs->mdio_stat)) & MDIO_STAT_BSY)
>                 ;
>
>         /* Set the port and dev addr */
>         mdio_ctl = MDIO_CTL_PORT_ADDR(port_addr) | MDIO_CTL_DEV_ADDR(dev_addr);
> -       out_be32(&regs->mdio_ctl, mdio_ctl);
> +       memac_out_32(&regs->mdio_ctl, mdio_ctl);
>
>         /* Set the register address */
>         if (c45)
> -               out_be32(&regs->mdio_addr, regnum & 0xffff);
> +               memac_out_32(&regs->mdio_addr, regnum & 0xffff);
>
>         /* Wait till the bus is free */
> -       while ((in_be32(&regs->mdio_stat)) & MDIO_STAT_BSY)
> +       while ((memac_in_32(&regs->mdio_stat)) & MDIO_STAT_BSY)
>                 ;
>
>         /* Write the value to the register */
> -       out_be32(&regs->mdio_data, MDIO_DATA(value));
> +       memac_out_32(&regs->mdio_data, MDIO_DATA(value));
>
>         /* Wait till the MDIO write is complete */
> -       while ((in_be32(&regs->mdio_data)) & MDIO_DATA_BSY)
> +       while ((memac_in_32(&regs->mdio_data)) & MDIO_DATA_BSY)
>                 ;
>
>         return 0;
> @@ -75,39 +94,39 @@ int memac_mdio_read(struct mii_dev *bus, int port_addr, int dev_addr,
>                         return 0xffff;
>                 c45 = 0; /* clause 22 */
>                 dev_addr = regnum & 0x1f;
> -               clrbits_be32(&regs->mdio_stat, MDIO_STAT_ENC);
> +               memac_clrbits_32(&regs->mdio_stat, MDIO_STAT_ENC);
>         } else
> -               setbits_be32(&regs->mdio_stat, MDIO_STAT_ENC);
> +               memac_setbits_32(&regs->mdio_stat, MDIO_STAT_ENC);
>
>         /* Wait till the bus is free */
> -       while ((in_be32(&regs->mdio_stat)) & MDIO_STAT_BSY)
> +       while ((memac_in_32(&regs->mdio_stat)) & MDIO_STAT_BSY)
>                 ;
>
>         /* Set the Port and Device Addrs */
>         mdio_ctl = MDIO_CTL_PORT_ADDR(port_addr) | MDIO_CTL_DEV_ADDR(dev_addr);
> -       out_be32(&regs->mdio_ctl, mdio_ctl);
> +       memac_out_32(&regs->mdio_ctl, mdio_ctl);
>
>         /* Set the register address */
>         if (c45)
> -               out_be32(&regs->mdio_addr, regnum & 0xffff);
> +               memac_out_32(&regs->mdio_addr, regnum & 0xffff);
>
>         /* Wait till the bus is free */
> -       while ((in_be32(&regs->mdio_stat)) & MDIO_STAT_BSY)
> +       while ((memac_in_32(&regs->mdio_stat)) & MDIO_STAT_BSY)
>                 ;
>
>         /* Initiate the read */
>         mdio_ctl |= MDIO_CTL_READ;
> -       out_be32(&regs->mdio_ctl, mdio_ctl);
> +       memac_out_32(&regs->mdio_ctl, mdio_ctl);
>
>         /* Wait till the MDIO write is complete */
> -       while ((in_be32(&regs->mdio_data)) & MDIO_DATA_BSY)
> +       while ((memac_in_32(&regs->mdio_data)) & MDIO_DATA_BSY)
>                 ;
>
>         /* Return all Fs if nothing was there */
> -       if (in_be32(&regs->mdio_stat) & MDIO_STAT_RD_ER)
> +       if (memac_in_32(&regs->mdio_stat) & MDIO_STAT_RD_ER)
>                 return 0xffff;
>
> -       return in_be32(&regs->mdio_data) & 0xffff;
> +       return memac_in_32(&regs->mdio_data) & 0xffff;
>  }
>
>  int memac_mdio_reset(struct mii_dev *bus)
> @@ -143,8 +162,9 @@ int fm_memac_mdio_init(bd_t *bis, struct memac_mdio_info *info)
>          * like T2080QDS, this bit default is '0', which leads to MDIO failure
>          * on XAUI PHY, so set this bit definitely.
>          */
> -       setbits_be32(&((struct memac_mdio_controller *)info->regs)->mdio_stat,
> -                    MDIO_STAT_CLKDIV(258) | MDIO_STAT_NEG);
> +       memac_setbits_32(
> +               &((struct memac_mdio_controller *)info->regs)->mdio_stat,
> +               MDIO_STAT_CLKDIV(258) | MDIO_STAT_NEG);
>
>         return mdio_register(bus);
>  }
> diff --git a/drivers/net/vsc9953.c b/drivers/net/vsc9953.c
> index 9fc3c18..fed7358 100644
> --- a/drivers/net/vsc9953.c
> +++ b/drivers/net/vsc9953.c
> @@ -9,7 +9,7 @@
>  #include <asm/io.h>
>  #include <asm/fsl_serdes.h>
>  #include <fm_eth.h>
> -#include <asm/fsl_memac.h>
> +#include <fsl_memac.h>
>  #include <vsc9953.h>
>
>  static struct vsc9953_info vsc9953_l2sw = {
> diff --git a/arch/powerpc/include/asm/fsl_memac.h b/include/fsl_memac.h
> similarity index 100%
> rename from arch/powerpc/include/asm/fsl_memac.h
> rename to include/fsl_memac.h
> --
> 1.7.9.5
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de<mailto:U-Boot at lists.denx.de>
> http://lists.denx.de/mailman/listinfo/u-boot


More information about the U-Boot mailing list