[PATCH v2 1/5] ARM: arch-meson: add ethernet common init function

[Simon Glass]

Hi Neil,

On 25 November 2017 at 02:45, Neil Armstrong <narmstrong at 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 <narmstrong at 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 <narmstrong at 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 <narmstrong at 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 <narmstrong at 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.

>
>>
>>> +               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