Hi Geert,
Thanks for testing and your feedback.
On 2016-12-07 19:14:40 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
>
> On Wed, Dec 7, 2016 at 5:28 PM, Niklas Söderlund
> <niklas.soderlund+renesas@xxxxxxxxxxxx> wrote:
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
>
> Thanks, works fine on r8a7791/koelsch!
>
> Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> > --- a/drivers/net/ethernet/renesas/sh_eth.c
> > +++ b/drivers/net/ethernet/renesas/sh_eth.c
> > @@ -624,7 +624,7 @@ static struct sh_eth_cpu_data r8a779x_data = {
> >
> > .register_type = SH_ETH_REG_FAST_RCAR,
> >
> > - .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD,
> > + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD,
>
> Interestingly, the ECSR_MPD bit is already set for several SoCs.
Yes, I noticed that and my assumption was that it was set 'just in case'
to clear any MagicPacket interrupts at probe time.
>
> Hence adding ".magic = 1" to the entry for r8a7740 instantly gave me working
> WoL support on r8a7740/armadillo. Cool!
Cool, I will set ".magic = 1" for r8a7740 in v2.
>
> > --- a/drivers/net/ethernet/renesas/sh_eth.h
> > +++ b/drivers/net/ethernet/renesas/sh_eth.h
> > @@ -493,6 +493,7 @@ struct sh_eth_cpu_data {
> > unsigned shift_rd0:1; /* shift Rx descriptor word 0 right by 16 */
> > unsigned rmiimode:1; /* EtherC has RMIIMODE register */
> > unsigned rtrate:1; /* EtherC has RTRATE register */
> > + unsigned magic:1; /* EtherC have PMDE in ECMR and MPDIP in ECSIPR */
>
> Instead of adding a new flag, perhaps you can just check for the ECSR_MPD flag
> in ecsr_value?
I briefly considered this but decided against it since I do not have
documentation for all versions of the device and no way to test it. You
tested and confirmed functionality on r8a7740, which leaves:
- sh7734-gether
- sh7763-gether
- sh7757-gether
To figure out if they support MagicPacket in the same fashion as r8a7740
and r8a779x. If anyone have access to documentation or hardware to
confirm this I be more then happy to get rid of the magic flag in favor
och checking for ECSR_MPD in ecsr_value.
>
> > @@ -529,6 +530,9 @@ struct sh_eth_private {
> > unsigned no_ether_link:1;
> > unsigned ether_link_active_low:1;
> > unsigned is_opened:1;
> > +
> > + bool wol_enabled;
>
> "unsigned wol_enabled:1", to merge with the bitfield above?
Thanks, looking it it now I don't know what I was thinking. I will
changes it for v2.
>
> > + struct clk *clk;
>
> It's a good practice to keep all pointers at the top of the struct, to avoid
> gaps due to alignment restrictions, especially on 64-bit (I know that's not
> the case here).
Thanks, you learn new things everyday. I will move it for v2.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
--
Regards,
Niklas Söderlund
