Date:	Wed, 25 Jun 2014 09:41:49 +0000
From:	"bhupesh.sharma@...escale.com" <bhupesh.sharma@...escale.com>
To:	Marc Kleine-Budde <mkl@...gutronix.de>,
	"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>
CC:	"wg@...ndegger.com" <wg@...ndegger.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE
 instances

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@...gutronix.de]
> Sent: Wednesday, June 25, 2014 1:58 PM
> To: Sharma Bhupesh-B45370; linux-can@...r.kernel.org
> Cc: wg@...ndegger.com; netdev@...r.kernel.org
> Subject: Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs
> for BE instances
> 
> On 06/24/2014 05:54 PM, Bhupesh Sharma wrote:
> > The FlexCAN IP on certain SoCs like (Freescale's LS1021A) is modelled
> > in a big-endian fashion, i.e. the registers and the message buffers
> > are organized in a BE way.
> 
> Do you have any idea, why fsl did this? The messed up the network
> controller on the mx28, too. :/

Not really. I guess s/w drivers are meant to hide h/w obscurities :)

> 
> > More details about the LS1021A SoC can be seen here:
> > http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=LS1021A
> > &nodeId=018rH325E4017B#
> 
> Is there any "real" documentation for this SoC available?

At the moment only the product brief is available for public distribution,
which I have pointed-to in the URL above.

> 
> > This patch ensures that the register read/write APIs are remodelled to
> > address such cases, while ensuring that existing platforms (where the
> > FlexCAN IP was modelled in LE way) do not break.
> 
> I'm not sure if it's better to handle this via the DT attributes big-
> endian, little-endian, no attribute would mean native endianess for
> backwards compatibility.

My 1st approach path was do it via DT itself, but that would mean
changing existing DTS/DTSI for SoCs which use FlexCAN, unless we
say no endianess attribute means that the module is still LE, and thus
effectively add 'big-endian' only a node to the LS1021A FlexCAN DT node.

> With this solution, you're breaking all ARM non DT boards, as the struct
> platform_device_id still uses fsl_p1010_devtype_data. You're breaking DT
> based mx35, as struct of_device_id has no entry for mx35.
> 
> With this patch fsl,p1010-flexcan isn't compatible with the imx/mxs any
> more, please change the device trees in the kernel.
> 
> Please update the "FLEXCAN hardware feature flags" table in the driver
> and check if any of the mentioned quirks are needed for the LS1021A.

I have confirmed the same. No quirks are required for LS1021A.
BTW, can't we have the quirks field in the DT node itself?

> 
> See comment inline.....
> 
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@...escale.com>
> > ---
> > Rebased againt v3.16-rc1
> >
> >  drivers/net/can/flexcan.c |  213
> > +++++++++++++++++++++++++++------------------
> >  1 file changed, 126 insertions(+), 87 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index f425ec2..00c4740 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> 
> [...]
> 
> >  static const struct can_bittiming_const flexcan_bittiming_const = {
> > @@ -237,32 +256,36 @@ static const struct can_bittiming_const
> > flexcan_bittiming_const = {  };
> >
> >  /*
> > - * Abstract off the read/write for arm versus ppc. This
> > - * assumes that PPC uses big-endian registers and everything
> > - * else uses little-endian registers, independent of CPU
> > - * endianess.
> > + * FlexCAN module is essentially modelled as a little-endian IP in
> > + most
> > + * SoCs, i.e the registers as well as the message buffer areas are
> > + * implemented in a little-endian fashion.
> > + *
> > + * However there are some SoCs (e.g. LS1021A) which implement the
> > + FlexCAN
> > + * module in a big-endian fashion (i.e the registers as well as the
> > + * message buffer areas are implemented in a big-endian way).
> > + *
> > + * In addition, the FlexCAN module can be found on SoCs having ARM or
> > + * PPC cores. So, we need to abstract off the register read/write
> > + * functions, ensuring that these cater to all the combinations of
> > + module
> > + * endianess and underlying CPU endianess.
> >   */
> > -#if defined(CONFIG_PPC)
> > -static inline u32 flexcan_read(void __iomem *addr)
> > +static inline u32 flexcan_read(const struct flexcan_priv *priv,
> > +			       void __iomem *addr)
> >  {
> > -	return in_be32(addr);
> > -}
> > -
> > -static inline void flexcan_write(u32 val, void __iomem *addr) -{
> > -	out_be32(addr, val);
> > -}
> > -#else
> > -static inline u32 flexcan_read(void __iomem *addr) -{
> > -	return readl(addr);
> > +	if (priv->devtype_data->module_is_big_endian)
> > +		return ioread32be(addr);
> > +	else
> > +		return ioread32(addr);
> >  }
> >
> > -static inline void flexcan_write(u32 val, void __iomem *addr)
> > +static inline void flexcan_write(const struct flexcan_priv *priv,
> > +				 u32 val, void __iomem *addr)
> >  {
> > -	writel(val, addr);
> > +	if (priv->devtype_data->module_is_big_endian)
> 
> Please move the devtype_data into the struct flexcan_priv, so that you
> don't need a double pointer dereference in the hot path.

Ok. Or should I create two functions for read and write - one does it in LE way and the other
in BE way and parse the DT to understand which endianness the module supports.

> 
> > +		iowrite32be(val, addr);
> > +	else
> > +		iowrite32(val, addr);
> >  }
> > -#endif
> 

Please share your views.

Regards,
Bhupesh

Powered by blists - more mailing lists