Message-ID: <536c077e2e8b4dfabb4c07f696753a3c@BN1PR03MB220.namprd03.prod.outlook.com>
Date:	Wed, 25 Jun 2014 11:01:12 +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

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@...gutronix.de]
> Sent: Wednesday, June 25, 2014 3:57 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/25/2014 11:41 AM, bhupesh.sharma@...escale.com wrote:
> >>> 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.
> 
> If no attributes means native endianess the dts will stay compatible.
> 
> >> 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?
> 
> I don't know, probably for historic reasons, feel free to post a patch.

Ok.

> 
> >> 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.
> 
> What about function pointers in the priv? So that flexcan_read() becomes
> priv->read().
> 

That's what I propose (similar to what I did for C_CAN driver for 16-bit and 32-bit
reg interfaces using platform data):

bool module_is_be = false;

module_is_be = get-endianess-from-DT-node;

priv->read() = module_is_be ? flexcan_read_be : flexcan_read_le;

Regards,
Bhupesh

Powered by blists - more mailing lists