On 26/11/2017 12:39, Simon Glass wrote: > Hi Neil, > > On 25 November 2017 at 02:45, Neil Armstrong <narmstr...@baylibre.com> wrote: >> Hi Simon, >> >> Le 24/11/2017 23:35, Simon Glass a écrit : >>> Hi Neil, >>> >>> On 22 November 2017 at 06:25, Neil Armstrong <narmstr...@baylibre.com> >>> wrote: >>>> Introduce a generic common Ethernet Hardware init function >>>> common to all Amlogic GX SoCs with support for the >>>> Internal PHY enable for GXL SoCs. >>>> >>>> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com> >>>> --- >>>> arch/arm/include/asm/arch-meson/eth.h | 15 ++++++++++ >>>> arch/arm/mach-meson/Makefile | 2 +- >>>> arch/arm/mach-meson/eth.c | 53 >>>> +++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 69 insertions(+), 1 deletion(-) >>>> create mode 100644 arch/arm/include/asm/arch-meson/eth.h >>>> create mode 100644 arch/arm/mach-meson/eth.c >>>> >>>> diff --git a/arch/arm/include/asm/arch-meson/eth.h >>>> b/arch/arm/include/asm/arch-meson/eth.h >>>> new file mode 100644 >>>> index 0000000..8ea8e10 >>>> --- /dev/null >>>> +++ b/arch/arm/include/asm/arch-meson/eth.h >>>> @@ -0,0 +1,15 @@ >>>> +/* >>>> + * Copyright (C) 2016 BayLibre, SAS >>>> + * Author: Neil Armstrong <narmstr...@baylibre.com> >>>> + * >>>> + * SPDX-License-Identifier: GPL-2.0+ >>>> + */ >>>> + >>>> +#ifndef __MESON_ETH_H__ >>>> +#define __MESON_ETH_H__ >>>> + >>>> +#include <phy.h> >>>> + >>>> +void meson_gx_eth_init(phy_interface_t mode, bool use_internal_phy); >>>> + >>>> +#endif /* __MESON_ETH_H__ */ >>>> diff --git a/arch/arm/mach-meson/Makefile b/arch/arm/mach-meson/Makefile >>>> index bf49b8b..b4e8dde 100644 >>>> --- a/arch/arm/mach-meson/Makefile >>>> +++ b/arch/arm/mach-meson/Makefile >>>> @@ -4,4 +4,4 @@ >>>> # SPDX-License-Identifier: GPL-2.0+ >>>> # >>>> >>>> -obj-y += board.o sm.o >>>> +obj-y += board.o sm.o eth.o >>>> diff --git a/arch/arm/mach-meson/eth.c b/arch/arm/mach-meson/eth.c >>>> new file mode 100644 >>>> index 0000000..46ecb5e >>>> --- /dev/null >>>> +++ b/arch/arm/mach-meson/eth.c >>>> @@ -0,0 +1,53 @@ >>>> +/* >>>> + * Copyright (C) 2016 BayLibre, SAS >>>> + * Author: Neil Armstrong <narmstr...@baylibre.com> >>>> + * >>>> + * SPDX-License-Identifier: GPL-2.0+ >>>> + */ >>>> + >>>> +#include <common.h> >>>> +#include <dm.h> >>>> +#include <asm/io.h> >>>> +#include <asm/arch/gxbb.h> >>>> +#include <asm/arch/eth.h> >>>> +#include <phy.h> >>>> + >>>> +void meson_gx_eth_init(phy_interface_t mode, bool use_internal_phy) >>> >>> Can you add a header file addition for this somewhere, with comments? >> >> Do you mean comments in the added arch/arm/include/asm/arch-meson/eth.h in >> this >> same patchset ? > > Yes that's what I meant. > >> >>> >>>> +{ >>>> + switch (mode) { >>>> + case PHY_INTERFACE_MODE_RGMII: >>>> + case PHY_INTERFACE_MODE_RGMII_ID: >>>> + case PHY_INTERFACE_MODE_RGMII_RXID: >>>> + case PHY_INTERFACE_MODE_RGMII_TXID: >>>> + /* Set RGMII mode */ >>>> + setbits_le32(GXBB_ETH_REG_0, GXBB_ETH_REG_0_PHY_INTF | >>>> + GXBB_ETH_REG_0_TX_PHASE(1) | >>>> + GXBB_ETH_REG_0_TX_RATIO(4) | >>>> + GXBB_ETH_REG_0_PHY_CLK_EN | >>>> + GXBB_ETH_REG_0_CLK_EN); >>>> + break; >>>> + >>>> + case PHY_INTERFACE_MODE_RMII: >>>> + /* Set RMII mode */ >>>> + out_le32(GXBB_ETH_REG_0, GXBB_ETH_REG_0_INVERT_RMII_CLK | >>>> + GXBB_ETH_REG_0_CLK_EN); >>> >>> How come this is using out_le32() instead of (for example) writel()? >> >> It should be writel(), but since the register size is 32bit, it can be >> out_le32 > > So why do we have out_le32()? What is the difference? > >> >>> >>>> + >>>> +#ifdef CONFIG_MESON_GXL >>> >>> This doesn't seem fully generic if it has this #ifdef in it. Can this >>> be a parameter? At worst can we use if() instead of #ifdef ? >> >> Yeah, I didn't really figured out how to specify the internal PHY. >> >> GXL and GXM SoCs have an internal PHY, but yeah GXBB does't. >> I hesitated to add a different meson_gx_eth_init() signature >> for these different SoCs, what do you think about that ? >> >> The new AXG SoC does not have internal PHY, so it will use the same >> code as GXBB. > > I think this function needs to be told which SoC type it is dealing > with, as a separate enum parameters.
It's more complex since the S905D from the same GXL family can also use an external RGMII interface *and* the internal PHY depending on the board. Could a generic flags parameter be better, and add a GXL specific MESON_GXL_USE_INTERNAL_RMII_PHY to be passed only on GXL platforms ? > >> >>> >>>> + if (use_internal_phy) { >>>> + /* Use Internal PHY */ >>>> + out_le32(GXBB_ETH_REG_2, 0x10110181); >>>> + out_le32(GXBB_ETH_REG_3, 0xe40908ff); >>>> + } >>>> +#endif >>>> + >>>> + break; >>>> + >>>> + default: >>>> + printf("Invalid Ethernet interface mode\n"); >>>> + return; >>>> + } >>>> + >>>> + /* Enable power and clock gate */ >>>> + setbits_le32(GXBB_GCLK_MPEG_1, GXBB_GCLK_MPEG_1_ETH); >>>> + clrbits_le32(GXBB_MEM_PD_REG_0, GXBB_MEM_PD_REG_0_ETH_MASK); >>> >>> Seems like this should be in a clock driver. >> >> It should, in next release ? Beniamino's I2C driver also used this, >> but yes a proper clock driver becomes necessary here. > > OK. > Regards, > > Simon > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot