Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sergei,

On Tuesday 27 August 2013 01:30:02 Sergei Shtylyov wrote:
> On 08/21/2013 04:58 PM, Laurent Pinchart wrote:
> 
> Sorry for the belated reply.

No worries.

> >>>>>>>>> Now that the 'register_type' field of the 'sh_eth' driver's
> >>>>>>>>> platform
> >>>>>>>>> data is not used by the driver anymore, it's time to remove it and
> >>>>>>>>> its initializers from the SH platform code. Also  move *enum*
> >>>>>>>>> declaring values for this  field from <linux/sh_eth.h>  to  the
> >>>>>>>>> local driver's header file as they're only needed by the driver
> >>>>>>>>> itself now...
> >>>>>>>>> 
> >>>>>>>>> Signed-off-by: Sergei Shtylyov
> >>>>>>>>> <sergei.shtylyov@cogentembedded.com>
> >>>>>> 
> >>>>>> [...]
> >>>>>> 
> >>>>>>>>>      /* Driver's parameters */
> >>>>>>>>>      #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
> >>>>>>>>>      #define SH4_SKB_RX_ALIGN    32
> >>>>>>>>> 
> >>>>>>>>> Index: net-next/include/linux/sh_eth.h
> >>>>>>>>> =================================
> >>>>>>>>> > >>>>>>>>> --- net-next.orig/include/linux/sh_eth.h
> >>>>>>>>> +++ net-next/include/linux/sh_eth.h
> >>>>>>>>> @@ -5,17 +5,10 @@
> >>>>>>>>> 
> >>>>>>>>>      #include <linux/if_ether.h>
> >>>>>>>>>      
> >>>>>>>>>      enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN};
> >>>>>>>>> 
> >>>>>>>>> -enum {
> >>>>>>>>> -    SH_ETH_REG_GIGABIT,
> >>>>>>>>> -    SH_ETH_REG_FAST_RCAR,
> >>>>>>>>> -    SH_ETH_REG_FAST_SH4,
> >>>>>>>>> -    SH_ETH_REG_FAST_SH3_SH2
> >>>>>>>>> -};
> >>>>>>>>> 
> >>>>>>>>>      struct sh_eth_plat_data {
> >>>>>>>>>      
> >>>>>>>>>          int phy;
> >>>>>>>>>          int edmac_endian;
> >>>>>>>> 
> >>>>>>>> Wouldn't it make sense to move the edmac_endian field to
> >>>>>>>> sh_eth_cpu_data as well ?
> >>>>>>> 
> >>>>>>> No, it depends on the SoC endianness which is determined by power-on
> >>>>>>> pin strapping -- which is board specific.
> >>>>> 
> >>>>> Does SoC endianness affect the ARM core endianness, the ethernet
> >>>>> registers endianness, or both ?
> >>>> 
> >>>> Both, AFAIK.
> >>>> 
> >>>>> If it affects the ARM core endianness only, the kernel needs to be
> >>>>> compiled in little-endian or big-endian mode anyway, and the sh_eth
> >>>>> driver should use cpu_to_le32() unconditionally. If it affects both
> >>>>> the ARM core and the ethernet controller there's not need to care
> >>>>> about the endianness, as it will always be good.
> >>>> 
> >>>> No, it won't unless you're using __raw_{readl|writel}() accessors. The
> >>>> driver doesn't do it. {readl|writel}() and io{read|write}32() that use
> >>>> them always assume LE ordering of memory.
> >>>> 
> >>>>> We only need to care about it if it affects the ethernet controller
> >>>>> registers only, which would seem weird to me.
> >>>> 
> >>>> Unfortunately, you are wrong.
> >>> 
> >>> Care to explain *why* ? There might be bugs in the driver (such as using
> >>> the wrong I/O accessors), but I don't see why we need to configure the
> >>> endianness through platform data.
> >> 
> >> Re-read my reply about the power-on pin strapping please. The SoC
> >> endianness setting gets read from the external source to the SoC, i.e.
> >> it's determined by the board.
> > 
> > Unless that pin doesn't affect the CPU core (in which case I wonder what
> > it's used for),
> 
> The thing is I don't have the necessary information about SH SoCs for which
> the 'edmac_endian' field was first added. I'm basing my assumptions on the
> R-Car manuals which claim that both register and DMA descriptor endianness
> depend on the SoC endianness (which is selected by the MD8 pin at power-on).
> So I don't even understand why it's called 'edmac_endian' based on this
> info, as it apparently determines only DMA descriptor endianness.

Neither do I. Reading the documentation I just had doubts, hence my questions. 
I doubt that the MD8 pin modifies the endianness of all registers, as that 
would probably be very costly from a silicon point of view, but everything is 
possible.

> > the kernel will need to be compiled for the specified endianness
> > anyway. Endianness conversion can thus be performed with cpu_to_* (if the
> > registers endianness is fixed) or skipped completely (if the registers
> > endianness is also configured by the bootstrap pin) by using raw I/O
> > accessors.
> 
> Unfortunately, the raw accessors also miss the barriers which might be
> necessary.

But it would be pretty trivial if needed to create and use in the driver 
accessors with barriers based on the raw accessors.

> > Modifications will be need in the sh_eth driver, but I don't see
> > why the driver would need to receive endianness information from platform
> > data or DT.
> 
> So you suggest that we use __LITTLE_ENDIAN/__BIG_ENDIAN pre-defined macros
> as when we're programing the EDMR.EL bit to enable automatic data swapping
> feature (it doesn't swap register or descriptors, only the raw data)?

Based on my understanding, I believe that we either

- don't need different register access codes paths for little and big endian 
(for a variety of reasons, as explained in the previous e-mails in this 
thread)

- need different code paths, but can make the decision at compile time instead 
of runtime because the CPU core endianness changes as well, which forces a 
recompilation anyway

I would thus suggest to use the right combination of cpu_to_[bl]e*, raw 
accessors (with added barriers), #ifdef __LITTLE_ENDIAN/__BIG_ENDIAN and 
EDMR.EL settings to remove the edmac_endian field.

What the right combination is isn't know yet, we can experiment later when 
we'll get a big-endian system. I'm fine with keeping the field in the platform 
data structure for reference until then, but let's not add it to the DT 
bindings.

-- 
Regards,

Laurent Pinchart