[U-Boot,2/4] net: fec: do not access reserved register for i.MX6UL

The MIB RAM and FIFO receive start register does not exist on
i.MX6UL. Accessing these register will cause enet not work well.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
Signed-off-by: Fugang Duan <B38611@freescale.com>
Cc: Joe Hershberger <joe.hershberger@ni.com>
---
 drivers/net/fec_mxc.c | 4 ++++
 1 file changed, 4 insertions(+)

Hi Peng,

On 08/03/2015 01:06 PM, Peng Fan wrote:
> The MIB RAM and FIFO receive start register does not exist on
> i.MX6UL. Accessing these register will cause enet not work well.
>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Signed-off-by: Fugang Duan <B38611@freescale.com>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> ---
>   drivers/net/fec_mxc.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index c5dcbbb..7fb1d5f 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -520,8 +520,10 @@ static int fec_open(struct eth_device *edev)
>   static int fec_init(struct eth_device *dev, bd_t* bd)
>   {
>   	struct fec_priv *fec = (struct fec_priv *)dev->priv;
> +#if !defined(CONFIG_MX6UL)
>   	uint32_t mib_ptr = (uint32_t)&fec->eth->rmon_t_drop;
>   	int i;
> +#endif
>
>   	/* Initialize MAC address */
>   	fec_set_hwaddr(dev);
> @@ -551,12 +553,14 @@ static int fec_init(struct eth_device *dev, bd_t* bd)
>   	writel(0x00000000, &fec->eth->gaddr2);
>
>
> +#if !defined(CONFIG_MX6UL)
>   	/* clear MIB RAM */
>   	for (i = mib_ptr; i <= mib_ptr + 0xfc; i += 4)
>   		writel(0, i);
>
>   	/* FIFO receive start register */
>   	writel(0x520, &fec->eth->r_fstart);
> +#endif
>
>   	/* size and address of each buffer */
>   	writel(FEC_MAX_PKT_SIZE, &fec->eth->emrbr);
>

Is it possible to do runtime check for the SoC type, instead of ifdefs?

Regards,
Nikolay

Hi Nikolay,
On Wed, Aug 05, 2015 at 05:31:27PM +0300, Nikolay Dimitrov wrote:
>Hi Peng,
>
>On 08/03/2015 01:06 PM, Peng Fan wrote:
>>The MIB RAM and FIFO receive start register does not exist on
>>i.MX6UL. Accessing these register will cause enet not work well.
>>
>>Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>>Signed-off-by: Fugang Duan <B38611@freescale.com>
>>Cc: Joe Hershberger <joe.hershberger@ni.com>
>>---
>>  drivers/net/fec_mxc.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>>diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
>>index c5dcbbb..7fb1d5f 100644
>>--- a/drivers/net/fec_mxc.c
>>+++ b/drivers/net/fec_mxc.c
>>@@ -520,8 +520,10 @@ static int fec_open(struct eth_device *edev)
>>  static int fec_init(struct eth_device *dev, bd_t* bd)
>>  {
>>  	struct fec_priv *fec = (struct fec_priv *)dev->priv;
>>+#if !defined(CONFIG_MX6UL)
>>  	uint32_t mib_ptr = (uint32_t)&fec->eth->rmon_t_drop;
>>  	int i;
>>+#endif
>>
>>  	/* Initialize MAC address */
>>  	fec_set_hwaddr(dev);
>>@@ -551,12 +553,14 @@ static int fec_init(struct eth_device *dev, bd_t* bd)
>>  	writel(0x00000000, &fec->eth->gaddr2);
>>
>>
>>+#if !defined(CONFIG_MX6UL)
>>  	/* clear MIB RAM */
>>  	for (i = mib_ptr; i <= mib_ptr + 0xfc; i += 4)
>>  		writel(0, i);
>>
>>  	/* FIFO receive start register */
>>  	writel(0x520, &fec->eth->r_fstart);
>>+#endif
>>
>>  	/* size and address of each buffer */
>>  	writel(FEC_MAX_PKT_SIZE, &fec->eth->emrbr);
>>
>
>Is it possible to do runtime check for the SoC type, instead of ifdefs?

This driver is used by i.MX7 and i.MX6, but i.MX7 patchset has not been
upstreamed now.

I considered using "if (!is_cpu_type(MXC_CPU_MX6UL))", but this will cause
i.MX7 fail to complile successfully, because i.MX7 does not support the macro
MXC_CPU_MX6UL.

The way I can think out is to refactor the code to support DM or FDT,
using compatible string to figure out which SoC. But now I do not have
much time to refactor the driver. So I just use the "#if !defined" way
which is not good solution.

Regards,
Peng.

Hi Peng,

On 08/06/2015 07:41 AM, Peng Fan wrote:
> Hi Nikolay,
> On Wed, Aug 05, 2015 at 05:31:27PM +0300, Nikolay Dimitrov wrote:
>> Hi Peng,
>>
>> On 08/03/2015 01:06 PM, Peng Fan wrote:
>>> The MIB RAM and FIFO receive start register does not exist on
>>> i.MX6UL. Accessing these register will cause enet not work well.
>>>
>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>>> Signed-off-by: Fugang Duan <B38611@freescale.com>
>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>> ---
>>>   drivers/net/fec_mxc.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
>>> index c5dcbbb..7fb1d5f 100644
>>> --- a/drivers/net/fec_mxc.c
>>> +++ b/drivers/net/fec_mxc.c
>>> @@ -520,8 +520,10 @@ static int fec_open(struct eth_device *edev)
>>>   static int fec_init(struct eth_device *dev, bd_t* bd)
>>>   {
>>>   	struct fec_priv *fec = (struct fec_priv *)dev->priv;
>>> +#if !defined(CONFIG_MX6UL)
>>>   	uint32_t mib_ptr = (uint32_t)&fec->eth->rmon_t_drop;
>>>   	int i;
>>> +#endif
>>>
>>>   	/* Initialize MAC address */
>>>   	fec_set_hwaddr(dev);
>>> @@ -551,12 +553,14 @@ static int fec_init(struct eth_device *dev, bd_t* bd)
>>>   	writel(0x00000000, &fec->eth->gaddr2);
>>>
>>>
>>> +#if !defined(CONFIG_MX6UL)
>>>   	/* clear MIB RAM */
>>>   	for (i = mib_ptr; i <= mib_ptr + 0xfc; i += 4)
>>>   		writel(0, i);
>>>
>>>   	/* FIFO receive start register */
>>>   	writel(0x520, &fec->eth->r_fstart);
>>> +#endif
>>>
>>>   	/* size and address of each buffer */
>>>   	writel(FEC_MAX_PKT_SIZE, &fec->eth->emrbr);
>>>
>>
>> Is it possible to do runtime check for the SoC type, instead of ifdefs?
>
> This driver is used by i.MX7 and i.MX6, but i.MX7 patchset has not been
> upstreamed now.
>
> I considered using "if (!is_cpu_type(MXC_CPU_MX6UL))", but this will cause
> i.MX7 fail to complile successfully, because i.MX7 does not support the macro
> MXC_CPU_MX6UL.
>
> The way I can think out is to refactor the code to support DM or FDT,
> using compatible string to figure out which SoC. But now I do not have
> much time to refactor the driver. So I just use the "#if !defined" way
> which is not good solution.

Thanks for explaining. Please check this code:


#ifdef CONFIG_MX6
     if (!is_cpu_type(MXC_CPU_MX6UL))
#endif
     {
         /* soc-specific code here */
     }


This code will allow runtime detection for imx6 socs, and will not
break the imx7 builds. What do you think?

Regards,
Nikolay

Hi Peng,

On 06/08/2015 06:41, Peng Fan wrote:
> Hi Nikolay,
> On Wed, Aug 05, 2015 at 05:31:27PM +0300, Nikolay Dimitrov wrote:
>> Hi Peng,
>>
>> On 08/03/2015 01:06 PM, Peng Fan wrote:
>>> The MIB RAM and FIFO receive start register does not exist on
>>> i.MX6UL. Accessing these register will cause enet not work well.
>>>
>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>>> Signed-off-by: Fugang Duan <B38611@freescale.com>
>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>> ---
>>>  drivers/net/fec_mxc.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
>>> index c5dcbbb..7fb1d5f 100644
>>> --- a/drivers/net/fec_mxc.c
>>> +++ b/drivers/net/fec_mxc.c
>>> @@ -520,8 +520,10 @@ static int fec_open(struct eth_device *edev)
>>>  static int fec_init(struct eth_device *dev, bd_t* bd)
>>>  {
>>>  	struct fec_priv *fec = (struct fec_priv *)dev->priv;
>>> +#if !defined(CONFIG_MX6UL)
>>>  	uint32_t mib_ptr = (uint32_t)&fec->eth->rmon_t_drop;
>>>  	int i;
>>> +#endif
>>>
>>>  	/* Initialize MAC address */
>>>  	fec_set_hwaddr(dev);
>>> @@ -551,12 +553,14 @@ static int fec_init(struct eth_device *dev, bd_t* bd)
>>>  	writel(0x00000000, &fec->eth->gaddr2);
>>>
>>>
>>> +#if !defined(CONFIG_MX6UL)
>>>  	/* clear MIB RAM */
>>>  	for (i = mib_ptr; i <= mib_ptr + 0xfc; i += 4)
>>>  		writel(0, i);
>>>
>>>  	/* FIFO receive start register */
>>>  	writel(0x520, &fec->eth->r_fstart);
>>> +#endif
>>>
>>>  	/* size and address of each buffer */
>>>  	writel(FEC_MAX_PKT_SIZE, &fec->eth->emrbr);
>>>
>>
>> Is it possible to do runtime check for the SoC type, instead of ifdefs?
> 
> This driver is used by i.MX7 and i.MX6, but i.MX7 patchset has not been
> upstreamed now.

Wait...this driver is used by all i.MXes. We have to take this in mind
to avoid to break some SOCs - even if this is not the case.

Anyway, the rule is to push patches based on mainline, without supposing
what happens with patches that are not yet applied. i.MX7 should be then
adjusted for this.

> 
> I considered using "if (!is_cpu_type(MXC_CPU_MX6UL))", but this will cause
> i.MX7 fail to complile successfully, because i.MX7 does not support the macro
> MXC_CPU_MX6UL.

It looks like we have a problem in design - we have to move this macros
to make available to all i.MXes.

In fact, it is even plausible that MX35 code runs
"is_cpu_type(MXC_CPU_MX6UL)", and the macros must return false. Having
these checks working for some SOCs vanifies the goal: check at runtime
if a SOC is of a certain type.

> 
> The way I can think out is to refactor the code to support DM or FDT,
> using compatible string to figure out which SoC. But now I do not have
> much time to refactor the driver. So I just use the "#if !defined" way
> which is not good solution.

It is not - anyway, making macros commonly available to all i.MXes could
be done with a smaller effort. Currently, macros are available only to
mx6 (define in sys_proto.h). We have to move them in a header in
arch/arm/include/asm/imx-common/, that is accessible by all SOCs.

Best regards,
Stefano Babic

Hi Stefano,

On Thu, Aug 06, 2015 at 03:54:22PM +0200, Stefano Babic wrote:
>Hi Peng,
>
>On 06/08/2015 06:41, Peng Fan wrote:
>> Hi Nikolay,
>> On Wed, Aug 05, 2015 at 05:31:27PM +0300, Nikolay Dimitrov wrote:
>>> Hi Peng,
>>>
>>> On 08/03/2015 01:06 PM, Peng Fan wrote:
>>>> The MIB RAM and FIFO receive start register does not exist on
>>>> i.MX6UL. Accessing these register will cause enet not work well.
>>>>
>>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>>>> Signed-off-by: Fugang Duan <B38611@freescale.com>
>>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>>> ---
>>>>  drivers/net/fec_mxc.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
>>>> index c5dcbbb..7fb1d5f 100644
>>>> --- a/drivers/net/fec_mxc.c
>>>> +++ b/drivers/net/fec_mxc.c
>>>> @@ -520,8 +520,10 @@ static int fec_open(struct eth_device *edev)
>>>>  static int fec_init(struct eth_device *dev, bd_t* bd)
>>>>  {
>>>>  	struct fec_priv *fec = (struct fec_priv *)dev->priv;
>>>> +#if !defined(CONFIG_MX6UL)
>>>>  	uint32_t mib_ptr = (uint32_t)&fec->eth->rmon_t_drop;
>>>>  	int i;
>>>> +#endif
>>>>
>>>>  	/* Initialize MAC address */
>>>>  	fec_set_hwaddr(dev);
>>>> @@ -551,12 +553,14 @@ static int fec_init(struct eth_device *dev, bd_t* bd)
>>>>  	writel(0x00000000, &fec->eth->gaddr2);
>>>>
>>>>
>>>> +#if !defined(CONFIG_MX6UL)
>>>>  	/* clear MIB RAM */
>>>>  	for (i = mib_ptr; i <= mib_ptr + 0xfc; i += 4)
>>>>  		writel(0, i);
>>>>
>>>>  	/* FIFO receive start register */
>>>>  	writel(0x520, &fec->eth->r_fstart);
>>>> +#endif
>>>>
>>>>  	/* size and address of each buffer */
>>>>  	writel(FEC_MAX_PKT_SIZE, &fec->eth->emrbr);
>>>>
>>>
>>> Is it possible to do runtime check for the SoC type, instead of ifdefs?
>> 
>> This driver is used by i.MX7 and i.MX6, but i.MX7 patchset has not been
>> upstreamed now.
>
>Wait...this driver is used by all i.MXes. We have to take this in mind
>to avoid to break some SOCs - even if this is not the case.
>
>Anyway, the rule is to push patches based on mainline, without supposing
>what happens with patches that are not yet applied. i.MX7 should be then
>adjusted for this.
>
>> 
>> I considered using "if (!is_cpu_type(MXC_CPU_MX6UL))", but this will cause
>> i.MX7 fail to complile successfully, because i.MX7 does not support the macro
>> MXC_CPU_MX6UL.
>
>It looks like we have a problem in design - we have to move this macros
>to make available to all i.MXes.
>
>In fact, it is even plausible that MX35 code runs
>"is_cpu_type(MXC_CPU_MX6UL)", and the macros must return false. Having
>these checks working for some SOCs vanifies the goal: check at runtime
>if a SOC is of a certain type.

I checked related code for i.MX25/28/31/35/5x/6x.

We can add following define in imx-common/xxx.h
#define MXC_CPU_MX35 0x35
#define MXC_CPU_MX31 0x31
#define MXC_CPU_MX25 0x25
#define MXC_CPU_MX23 xxxx
#define MXC_CPU_MX28 yyyy
About i.mx23/28, I am not sure, since they have different get_cpu_rev
implementation. Also they have different chipid layout.

To i.MX31, we can do following change:
return mx31_cpu_type[i].v | 0x31000; to replace return mx31_cpu_type[i].v;

Then we can use:
#define is_cpu_type(xxx) (((get_cpu_rev() & 0xFF000) >> 12) == xxx)

To i.MX23/28, they have different get_cpu_rev() prototype, maybe need to
rewrite the function?

I am not familar with SoCs prior to i.MX6, not sure whether this ok.

>
>> 
>> The way I can think out is to refactor the code to support DM or FDT,
>> using compatible string to figure out which SoC. But now I do not have
>> much time to refactor the driver. So I just use the "#if !defined" way
>> which is not good solution.
>
>It is not - anyway, making macros commonly available to all i.MXes could
>be done with a smaller effort. Currently, macros are available only to
>mx6 (define in sys_proto.h). We have to move them in a header in
>arch/arm/include/asm/imx-common/, that is accessible by all SOCs.

Maybe need to add sys_proto.h in arch/arm/include/asm/imx-common.

Regards,
Peng.

>
>Best regards,
>Stefano Babic
>
>
>-- 
>=====================================================================
>DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
>=====================================================================

Hi Peng,

On 07/08/2015 03:08, Peng Fan wrote:

>>> I considered using "if (!is_cpu_type(MXC_CPU_MX6UL))", but this will cause
>>> i.MX7 fail to complile successfully, because i.MX7 does not support the macro
>>> MXC_CPU_MX6UL.
>>
>> It looks like we have a problem in design - we have to move this macros
>> to make available to all i.MXes.
>>
>> In fact, it is even plausible that MX35 code runs
>> "is_cpu_type(MXC_CPU_MX6UL)", and the macros must return false. Having
>> these checks working for some SOCs vanifies the goal: check at runtime
>> if a SOC is of a certain type.
> 
> I checked related code for i.MX25/28/31/35/5x/6x.
> 
> We can add following define in imx-common/xxx.h
> #define MXC_CPU_MX35 0x35
> #define MXC_CPU_MX31 0x31
> #define MXC_CPU_MX25 0x25
> #define MXC_CPU_MX23 xxxx
> #define MXC_CPU_MX28 yyyy
> About i.mx23/28, I am not sure, since they have different get_cpu_rev
> implementation.

As far as I understand, the chip id is retrieved and then converted as
"23" or "28" (see get_cpu_type). I think it is plausible to define them
in this way as well as using the waqy in get_cpu_type() to identify the SOC.

get_cpu_rev() in MX23/MX28 is also not compliant with the rest of SOCs.
It returns a string intead of a u32.

> Also they have different chipid layout.
> 
> To i.MX31, we can do following change:
> return mx31_cpu_type[i].v | 0x31000; to replace return mx31_cpu_type[i].v;
> 
> Then we can use:
> #define is_cpu_type(xxx) (((get_cpu_rev() & 0xFF000) >> 12) == xxx)
> 
> To i.MX23/28, they have different get_cpu_rev() prototype, maybe need to
> rewrite the function?

Agree - it is an exception in U-Boot for i.MXEs, and it defines a
different prototype for the function.

> 
> I am not familar with SoCs prior to i.MX6, not sure whether this ok.
> 

IMHO it looks ok ;-)

>>
>>>
>>> The way I can think out is to refactor the code to support DM or FDT,
>>> using compatible string to figure out which SoC. But now I do not have
>>> much time to refactor the driver. So I just use the "#if !defined" way
>>> which is not good solution.
>>
>> It is not - anyway, making macros commonly available to all i.MXes could
>> be done with a smaller effort. Currently, macros are available only to
>> mx6 (define in sys_proto.h). We have to move them in a header in
>> arch/arm/include/asm/imx-common/, that is accessible by all SOCs.
> 
> Maybe need to add sys_proto.h in arch/arm/include/asm/imx-common.

+1

Right - we have several sys_proto.h, iun most cases they have prototypes
for the same functions.

Best regards,
Stefano Babic