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

2017-11-27 Thread Simon Glass 
Hi Neil,

On 27 November 2017 at 01:48, Neil Armstrong  wrote:
> On 26/11/2017 12:39, Simon Glass wrote:
>> Hi Neil,
>>
>> On 25 November 2017 at 02:45, Neil Armstrong  wrote:
>>> Hi Simon,
>>>
>>> Le 24/11/2017 23:35, Simon Glass a écrit :
 Hi Neil,

 On 22 November 2017 at 06:25, Neil Armstrong  
 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 
> ---
>  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 000..8ea8e10
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-meson/eth.h
> @@ -0,0 +1,15 @@
> +/*
> + * Copyright (C) 2016 BayLibre, SAS
> + * Author: Neil Armstrong 
> + *
> + * SPDX-License-Identifier:GPL-2.0+
> + */
> +
> +#ifndef __MESON_ETH_H__
> +#define __MESON_ETH_H__
> +
> +#include 
> +
> +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 000..46ecb5e
> --- /dev/null
> +++ b/arch/arm/mach-meson/eth.c
> @@ -0,0 +1,53 @@
> +/*
> + * Copyright (C) 2016 BayLibre, SAS
> + * Author: Neil Armstrong 
> + *
> + * SPDX-License-Identifier:GPL-2.0+
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +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?

Still unsure about this one.

>>
>>>

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

Yes that sounds good and will get rid of the

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

2017-11-27 Thread Neil Armstrong 
On 26/11/2017 12:39, Simon Glass wrote:
> Hi Neil,
> 
> On 25 November 2017 at 02:45, Neil Armstrong  wrote:
>> Hi Simon,
>>
>> Le 24/11/2017 23:35, Simon Glass a écrit :
>>> Hi Neil,
>>>
>>> On 22 November 2017 at 06:25, Neil Armstrong  
>>> 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 
 ---
  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 000..8ea8e10
 --- /dev/null
 +++ b/arch/arm/include/asm/arch-meson/eth.h
 @@ -0,0 +1,15 @@
 +/*
 + * Copyright (C) 2016 BayLibre, SAS
 + * Author: Neil Armstrong 
 + *
 + * SPDX-License-Identifier:GPL-2.0+
 + */
 +
 +#ifndef __MESON_ETH_H__
 +#define __MESON_ETH_H__
 +
 +#include 
 +
 +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 000..46ecb5e
 --- /dev/null
 +++ b/arch/arm/mach-meson/eth.c
 @@ -0,0 +1,53 @@
 +/*
 + * Copyright (C) 2016 BayLibre, SAS
 + * Author: Neil Armstrong 
 + *
 + * SPDX-License-Identifier:GPL-2.0+
 + */
 +
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +
 +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
 +
 +

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

2017-11-26 Thread Simon Glass 
Hi Neil,

On 25 November 2017 at 02:45, Neil Armstrong  wrote:
> Hi Simon,
>
> Le 24/11/2017 23:35, Simon Glass a écrit :
>> Hi Neil,
>>
>> On 22 November 2017 at 06:25, Neil Armstrong  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 
>>> ---
>>>  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 000..8ea8e10
>>> --- /dev/null
>>> +++ b/arch/arm/include/asm/arch-meson/eth.h
>>> @@ -0,0 +1,15 @@
>>> +/*
>>> + * Copyright (C) 2016 BayLibre, SAS
>>> + * Author: Neil Armstrong 
>>> + *
>>> + * SPDX-License-Identifier:GPL-2.0+
>>> + */
>>> +
>>> +#ifndef __MESON_ETH_H__
>>> +#define __MESON_ETH_H__
>>> +
>>> +#include 
>>> +
>>> +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 000..46ecb5e
>>> --- /dev/null
>>> +++ b/arch/arm/mach-meson/eth.c
>>> @@ -0,0 +1,53 @@
>>> +/*
>>> + * Copyright (C) 2016 BayLibre, SAS
>>> + * Author: Neil Armstrong 
>>> + *
>>> + * SPDX-License-Identifier:GPL-2.0+
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +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

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

2017-11-26 Thread Beniamino Galvani 
On Sat, Nov 25, 2017 at 10:45:30AM +0100, Neil Armstrong wrote:
> > 
> >> +   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.

I have written a basic clock driver that allows to enable/disable
gates and get their frequency. Do you think this is enough? I will
submit it soon (hopefully later today).

Beniamino
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

2017-11-25 Thread Neil Armstrong 
Hi Simon,

Le 24/11/2017 23:35, Simon Glass a écrit :
> Hi Neil,
> 
> On 22 November 2017 at 06:25, Neil Armstrong  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 
>> ---
>>  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 000..8ea8e10
>> --- /dev/null
>> +++ b/arch/arm/include/asm/arch-meson/eth.h
>> @@ -0,0 +1,15 @@
>> +/*
>> + * Copyright (C) 2016 BayLibre, SAS
>> + * Author: Neil Armstrong 
>> + *
>> + * SPDX-License-Identifier:GPL-2.0+
>> + */
>> +
>> +#ifndef __MESON_ETH_H__
>> +#define __MESON_ETH_H__
>> +
>> +#include 
>> +
>> +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 000..46ecb5e
>> --- /dev/null
>> +++ b/arch/arm/mach-meson/eth.c
>> @@ -0,0 +1,53 @@
>> +/*
>> + * Copyright (C) 2016 BayLibre, SAS
>> + * Author: Neil Armstrong 
>> + *
>> + * SPDX-License-Identifier:GPL-2.0+
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +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 ?

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

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

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

> 
>> +}
>> --
>> 2.7.4
>>
> 
> Regards,
> Simon
> 

Thanks,
Neil
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

2017-11-24 Thread Simon Glass 
Hi Neil,

On 22 November 2017 at 06:25, Neil Armstrong  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 
> ---
>  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 000..8ea8e10
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-meson/eth.h
> @@ -0,0 +1,15 @@
> +/*
> + * Copyright (C) 2016 BayLibre, SAS
> + * Author: Neil Armstrong 
> + *
> + * SPDX-License-Identifier:GPL-2.0+
> + */
> +
> +#ifndef __MESON_ETH_H__
> +#define __MESON_ETH_H__
> +
> +#include 
> +
> +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 000..46ecb5e
> --- /dev/null
> +++ b/arch/arm/mach-meson/eth.c
> @@ -0,0 +1,53 @@
> +/*
> + * Copyright (C) 2016 BayLibre, SAS
> + * Author: Neil Armstrong 
> + *
> + * SPDX-License-Identifier:GPL-2.0+
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +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?

> +{
> +   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()?

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

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

> +}
> --
> 2.7.4
>

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

2017-11-22 Thread Neil Armstrong 
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 
---
 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 000..8ea8e10
--- /dev/null
+++ b/arch/arm/include/asm/arch-meson/eth.h
@@ -0,0 +1,15 @@
+/*
+ * Copyright (C) 2016 BayLibre, SAS
+ * Author: Neil Armstrong 
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#ifndef __MESON_ETH_H__
+#define __MESON_ETH_H__
+
+#include 
+
+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 000..46ecb5e
--- /dev/null
+++ b/arch/arm/mach-meson/eth.c
@@ -0,0 +1,53 @@
+/*
+ * Copyright (C) 2016 BayLibre, SAS
+ * Author: Neil Armstrong 
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+void meson_gx_eth_init(phy_interface_t mode, bool use_internal_phy)
+{
+   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);
+
+#ifdef CONFIG_MESON_GXL
+   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);
+}
-- 
2.7.4

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot