[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(®s->mdio_stat, MDIO_STAT_ENC);
> + memac_clrbits_32(®s->mdio_stat, MDIO_STAT_ENC);
> } else
> - setbits_be32(®s->mdio_stat, MDIO_STAT_ENC);
> + memac_setbits_32(®s->mdio_stat, MDIO_STAT_ENC);
>
> /* Wait till the bus is free */
> - while ((in_be32(®s->mdio_stat)) & MDIO_STAT_BSY)
> + while ((memac_in_32(®s->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(®s->mdio_ctl, mdio_ctl);
> + memac_out_32(®s->mdio_ctl, mdio_ctl);
>
> /* Set the register address */
> if (c45)
> - out_be32(®s->mdio_addr, regnum & 0xffff);
> + memac_out_32(®s->mdio_addr, regnum & 0xffff);
>
> /* Wait till the bus is free */
> - while ((in_be32(®s->mdio_stat)) & MDIO_STAT_BSY)
> + while ((memac_in_32(®s->mdio_stat)) & MDIO_STAT_BSY)
> ;
>
> /* Write the value to the register */
> - out_be32(®s->mdio_data, MDIO_DATA(value));
> + memac_out_32(®s->mdio_data, MDIO_DATA(value));
>
> /* Wait till the MDIO write is complete */
> - while ((in_be32(®s->mdio_data)) & MDIO_DATA_BSY)
> + while ((memac_in_32(®s->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(®s->mdio_stat, MDIO_STAT_ENC);
> + memac_clrbits_32(®s->mdio_stat, MDIO_STAT_ENC);
> } else
> - setbits_be32(®s->mdio_stat, MDIO_STAT_ENC);
> + memac_setbits_32(®s->mdio_stat, MDIO_STAT_ENC);
>
> /* Wait till the bus is free */
> - while ((in_be32(®s->mdio_stat)) & MDIO_STAT_BSY)
> + while ((memac_in_32(®s->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(®s->mdio_ctl, mdio_ctl);
> + memac_out_32(®s->mdio_ctl, mdio_ctl);
>
> /* Set the register address */
> if (c45)
> - out_be32(®s->mdio_addr, regnum & 0xffff);
> + memac_out_32(®s->mdio_addr, regnum & 0xffff);
>
> /* Wait till the bus is free */
> - while ((in_be32(®s->mdio_stat)) & MDIO_STAT_BSY)
> + while ((memac_in_32(®s->mdio_stat)) & MDIO_STAT_BSY)
> ;
>
> /* Initiate the read */
> mdio_ctl |= MDIO_CTL_READ;
> - out_be32(®s->mdio_ctl, mdio_ctl);
> + memac_out_32(®s->mdio_ctl, mdio_ctl);
>
> /* Wait till the MDIO write is complete */
> - while ((in_be32(®s->mdio_data)) & MDIO_DATA_BSY)
> + while ((memac_in_32(®s->mdio_data)) & MDIO_DATA_BSY)
> ;
>
> /* Return all Fs if nothing was there */
> - if (in_be32(®s->mdio_stat) & MDIO_STAT_RD_ER)
> + if (memac_in_32(®s->mdio_stat) & MDIO_STAT_RD_ER)
> return 0xffff;
>
> - return in_be32(®s->mdio_data) & 0xffff;
> + return memac_in_32(®s->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