Re: [PATCH] net: fec: stop the "rcv is not +last, " error messages

Re: [PATCH] net: fec: stop the "rcv is not +last, " error messages

[Greg Ungerer]

Hi Troy,

Commit 55cd48c8 ('net: fec: stop the "rcv is not +last, " error
messages') adds a write to a register that is not present in all
implementations of the FEC hardware module. None of the ColdFire
SoC parts with the FEC module have the FTRL (0x1b0) register.

Does this need a quirk flag to key access to this register of?
Or can you piggyback on the FEC_QUIRK_HAS_RACC flag?

Regards
Greg

Re: [PATCH] net: fec: stop the "rcv is not +last, " error messages

[Troy Kisky]

On 3/29/2016 8:24 PM, Greg Ungerer wrote:
> Hi Troy,
> 
> Commit 55cd48c8 ('net: fec: stop the "rcv is not +last, " error
> messages') adds a write to a register that is not present in all
> implementations of the FEC hardware module. None of the ColdFire
> SoC parts with the FEC module have the FTRL (0x1b0) register.
> 
> Does this need a quirk flag to key access to this register of?
> Or can you piggyback on the FEC_QUIRK_HAS_RACC flag?
> 
> Regards
> Greg

I'm no expert on what hardware has which registers, but piggybacking
works for all the processors that I use. Let's see what Freescale
says, but would you like to submit a patch to move it inside the quirk's
"if", or do you want me to do it?

Re: [PATCH] net: fec: stop the "rcv is not +last, " error messages

[Fabio Estevam]

Hi Greg,

On Wed, Mar 30, 2016 at 12:24 AM, Greg Ungerer <gerg@uclinux.org> wrote:
> Hi Troy,
>
> Commit 55cd48c8 ('net: fec: stop the "rcv is not +last, " error
> messages') adds a write to a register that is not present in all
> implementations of the FEC hardware module. None of the ColdFire
> SoC parts with the FEC module have the FTRL (0x1b0) register.
>
> Does this need a quirk flag to key access to this register of?
> Or can you piggyback on the FEC_QUIRK_HAS_RACC flag?

Would the change below work on Coldfire?

--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -943,8 +943,8 @@ fec_restart(struct net_device *ndev)
                else
                        val &= ~FEC_RACC_OPTIONS;
                writel(val, fep->hwp + FEC_RACC);
-       }
        writel(PKT_MAXBUF_SIZE, fep->hwp + FEC_FTRL);
+       }
 #endif

        /*

Re: [PATCH] net: fec: stop the "rcv is not +last, " error messages

[Greg Ungerer]

Hi Fabio,

On 31/03/16 04:37, Fabio Estevam wrote:
> Hi Greg,
> 
> On Wed, Mar 30, 2016 at 12:24 AM, Greg Ungerer <gerg@uclinux.org> wrote:
>> Hi Troy,
>>
>> Commit 55cd48c8 ('net: fec: stop the "rcv is not +last, " error
>> messages') adds a write to a register that is not present in all
>> implementations of the FEC hardware module. None of the ColdFire
>> SoC parts with the FEC module have the FTRL (0x1b0) register.
>>
>> Does this need a quirk flag to key access to this register of?
>> Or can you piggyback on the FEC_QUIRK_HAS_RACC flag?
> 
> Would the change below work on Coldfire?
> 
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -943,8 +943,8 @@ fec_restart(struct net_device *ndev)
>                 else
>                         val &= ~FEC_RACC_OPTIONS;
>                 writel(val, fep->hwp + FEC_RACC);
> -       }
>         writel(PKT_MAXBUF_SIZE, fep->hwp + FEC_FTRL);
> +       }
>  #endif
> 
>         /*

Yes, that fixes it. Will you carry this change?

Regards
Greg

RE: [PATCH] net: fec: stop the "rcv is not +last, " error messages

[Fugang Duan]

From: Fabio Estevam <festevam@gmail.com> Sent: Thursday, March 31, 2016 2:37 AM
> To: Greg Ungerer <gerg@uclinux.org>
> Cc: Troy Kisky <troy.kisky@boundarydevices.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH] net: fec: stop the "rcv is not +last, " error messages
> 
> Hi Greg,
> 
> On Wed, Mar 30, 2016 at 12:24 AM, Greg Ungerer <gerg@uclinux.org> wrote:
> > Hi Troy,
> >
> > Commit 55cd48c8 ('net: fec: stop the "rcv is not +last, " error
> > messages') adds a write to a register that is not present in all
> > implementations of the FEC hardware module. None of the ColdFire SoC
> > parts with the FEC module have the FTRL (0x1b0) register.
> >
> > Does this need a quirk flag to key access to this register of?
> > Or can you piggyback on the FEC_QUIRK_HAS_RACC flag?
> 
> Would the change below work on Coldfire?
> 
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -943,8 +943,8 @@ fec_restart(struct net_device *ndev)
>                 else
>                         val &= ~FEC_RACC_OPTIONS;
>                 writel(val, fep->hwp + FEC_RACC);
> -       }
>         writel(PKT_MAXBUF_SIZE, fep->hwp + FEC_FTRL);
> +       }
>  #endif
> 
>         /*

Fabio, we cannot do it like this that may cause confused for the quirk flag "FEC_QUIRK_HAS_RACC".


Hi, Greg,

The header file fec.h define the FEC_FTRL as below,  if ColdFire SoC has no this register,  we may remove the define in here and define the register according to SOC type. For example, it is ColdFire Soc, define it as 0xFFF. Is it  feasible ?

#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
    defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM)
...
#define FEC_FTRL                0x1b0
...
#else
...
#endif


Regards,
Andy

Re: [PATCH] net: fec: stop the "rcv is not +last, " error messages

[Greg Ungerer]

Hi Andy,

On 31/03/16 11:41, Fugang Duan wrote:
> From: Fabio Estevam <festevam@gmail.com> Sent: Thursday, March 31, 2016 2:37 AM
>> To: Greg Ungerer <gerg@uclinux.org>
>> Cc: Troy Kisky <troy.kisky@boundarydevices.com>; netdev@vger.kernel.org
>> Subject: Re: [PATCH] net: fec: stop the "rcv is not +last, " error messages
>>
>> Hi Greg,
>>
>> On Wed, Mar 30, 2016 at 12:24 AM, Greg Ungerer <gerg@uclinux.org> wrote:
>>> Hi Troy,
>>>
>>> Commit 55cd48c8 ('net: fec: stop the "rcv is not +last, " error
>>> messages') adds a write to a register that is not present in all
>>> implementations of the FEC hardware module. None of the ColdFire SoC
>>> parts with the FEC module have the FTRL (0x1b0) register.
>>>
>>> Does this need a quirk flag to key access to this register of?
>>> Or can you piggyback on the FEC_QUIRK_HAS_RACC flag?
>>
>> Would the change below work on Coldfire?
>>
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -943,8 +943,8 @@ fec_restart(struct net_device *ndev)
>>                 else
>>                         val &= ~FEC_RACC_OPTIONS;
>>                 writel(val, fep->hwp + FEC_RACC);
>> -       }
>>         writel(PKT_MAXBUF_SIZE, fep->hwp + FEC_FTRL);
>> +       }
>>  #endif
>>
>>         /*
> 
> Fabio, we cannot do it like this that may cause confused for the quirk flag "FEC_QUIRK_HAS_RACC".
> 
> 
> Hi, Greg,
> 
> The header file fec.h define the FEC_FTRL as below,  if ColdFire SoC has no this register,  we may remove the define in here and define the register according to SOC type. For example, it is ColdFire Soc, define it as 0xFFF. Is it  feasible ?
> 
> #if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
>     defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM)
> ...
> #define FEC_FTRL                0x1b0
> ...
> #else
> ...
> #endif

Sure you could do that. But... you still have to be careful
with references to it in fec_main.c. They will need be conditional
on existence of FEC_FTRL, otherwise you break compilation.

Even if you define it to some bogus value we still don't 
want the code actually writing to it.

Regards
Greg

Re: [PATCH] net: fec: stop the "rcv is not +last, " error messages

[Fabio Estevam]

Hi Andy,

On Wed, Mar 30, 2016 at 10:41 PM, Fugang Duan <fugang.duan@nxp.com> wrote:
>
> Fabio, we cannot do it like this that may cause confused for the quirk flag "FEC_QUIRK_HAS_RACC".

We can treat FEC_QUIRK_HAS_RACC flag as "this is a non-Coldfire SoC".

>
>
> Hi, Greg,
>
> The header file fec.h define the FEC_FTRL as below,  if ColdFire SoC has no this register,  we may remove the define in here and define the register according to SOC type. For example, it is ColdFire Soc, define it as 0xFFF. Is it  feasible ?
>

This is even worse IMHO. We should not write to a 'fake' register
offset of 0xFFF.

Re: [PATCH] net: fec: stop the "rcv is not +last, " error messages

[Fabio Estevam]

Hi Greg,

On Wed, Mar 30, 2016 at 10:17 PM, Greg Ungerer <gerg@uclinux.org> wrote:

> Yes, that fixes it. Will you carry this change?

Thanks for confirming.

Yes, I will submit it later today;

RE: [PATCH] net: fec: stop the "rcv is not +last, " error messages

[Fugang Duan]

From: Fabio Estevam <festevam@gmail.com> Sent: Thursday, March 31, 2016 6:57 PM
> To: Fugang Duan <fugang.duan@nxp.com>
> Cc: Greg Ungerer <gerg@uclinux.org>; Troy Kisky
> <troy.kisky@boundarydevices.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH] net: fec: stop the "rcv is not +last, " error messages
> 
> Hi Andy,
> 
> On Wed, Mar 30, 2016 at 10:41 PM, Fugang Duan <fugang.duan@nxp.com>
> wrote:
> >
> > Fabio, we cannot do it like this that may cause confused for the quirk flag
> "FEC_QUIRK_HAS_RACC".
> 
> We can treat FEC_QUIRK_HAS_RACC flag as "this is a non-Coldfire SoC".
> 

FEC_QUIRK_HAS_RACC means the HW support "Receive Accelerator Function Configuration". It is really make somebody confused.

To save trouble,  you treat  FEC_QUIRK_HAS_RACC flag as "this is a non-Coldfire SoC",  you must add comment on the flag define.

> >
> >
> > Hi, Greg,
> >
> > The header file fec.h define the FEC_FTRL as below,  if ColdFire SoC has no this
> register,  we may remove the define in here and define the register according
> to SOC type. For example, it is ColdFire Soc, define it as 0xFFF. Is it  feasible ?
> >
> 
> This is even worse IMHO. We should not write to a 'fake' register offset of 0xFFF.

We can do it like this:

#if defined(CONFIG_ARM)
        writel(PKT_MAXBUF_SIZE, fep->hwp + FEC_FTRL);
#endif