Re: [PATCH V2 1/1] net: can: Remodel FlexCAN register read/write APIs for BE instances

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3405 bytes --]

On 07/04/2014 03:47 PM, bhupesh.sharma@freescale.com wrote:
> Hi Marc,
> 
> Thanks for your review.
> 
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>> Sent: Friday, July 04, 2014 7:11 PM
>> To: Sharma Bhupesh-B45370; linux-can@vger.kernel.org
>> Cc: wg@grandegger.com; netdev@vger.kernel.org
>> Subject: Re: [PATCH V2 1/1] net: can: Remodel FlexCAN register read/write
>> APIs for BE instances
>>
>> On 07/04/2014 03:01 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.
>>>
>>> More details about the LS1021A SoC can be seen here:
>>> http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=LS1021A
>>> &nodeId=018rH325E4017B#
>>>
>>> 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.
>>>
>>> Tested on LS1021A-QDS board.
>>>
>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@freescale.com>
>>> ---
>>> Changes since v1:
>>> - Addressed Marc's review comments.
>>> - Also tried on ARM big-endian kernel
>>>
>>> Rebased against v3.16-rc2
>>
>> Please use net-next or linux-can-next, but I think this makes no
>> difference here.
>>
>>>  drivers/net/can/flexcan.c |  192
>>> +++++++++++++++++++++++++++------------------
>>>  1 file changed, 114 insertions(+), 78 deletions(-)
>>
>> I'm missing the DT documentation update.
> 
> Yes. I just wanted to get some early comments on this.
> 
>> [...]
>>
>> 		of_property_read_u32(pdev->dev.of_node,
>>> @@ -1149,6 +1166,25 @@ static int flexcan_probe(struct platform_device
>> *pdev)
>>>  	dev->flags |= IFF_ECHO;
>>>
>>>  	priv = netdev_priv(dev);
>>> +
>>> +	if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>>> +		core_is_little = false;
>>> +
>>> +	if (of_property_read_bool(dev->dev.of_node, "big-endian"))
>>> +		module_is_little = false;
>>> +
>>> +	if ((core_is_little && module_is_little) ||
>>> +	    (!core_is_little && !module_is_little)) {
>>
>> I think this is broken on PPC, where both core and module are BE. Please
>> assume native endianess an default, if neither big-endian nor little-
>> endian is present.
> 
> On PPC platforms (which are BE) the IP is essentially LE (I verified this on P1010 RDB).
> 
> If both are BE, then we need no swap operations, right? Or am I missing something.

Have a look at the existing code:

> /*
>  * 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.
>  */
> #if defined(CONFIG_PPC)
> static inline u32 flexcan_read(void __iomem *addr)
> {
> 	return in_be32(addr);
> }
> 
> static inline void flexcan_write(u32 val, void __iomem *addr)
> {
> 	out_be32(addr, val);
> }
> #else

I think in_be32() does the same regarding to endianess as ioread32be()

Marc


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]