Re: [PATCH 1/2] can: flexcan: Remodel FlexCAN register r/w APIs for big endian FlexCAN controllers.

Re: [PATCH 1/2] can: flexcan: Remodel FlexCAN register r/w APIs for big endian FlexCAN controllers.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2326 bytes --]

On 11/10/2017 01:35 PM, Pankaj Bansal wrote:
>>> 3. Regarding backward compatibility with PowerPC, I see that there is only one platform in PowerPC architecture that is using flexcan.
>>>     There is only one platform in "arch/powerpc/boot/dts/" having flexcan node. p1010si-post.dtsi
>>>     I have added the big-endian property in that platform and have tested it on P1010 platform after backporting the patch to Freescale SDK 1.4 linux.
>>>     I have sent the patch for this. See "[PATCH 2/3] powerpc: dts: P1010: Add endianness property to flexcan node"
>>>     I believe these changes can be accepted for both powerpc and arm and other architectures that use flexcan.
>>
>> No, this is not acceptable. You break the device tree. Please add the le, be information to the devtype data.
> 
> I don't understand how this breaks device tree? can you please elaborate? This method is already being used in other specifications.

Boot a new kernel with an old tree on a PPC board -> flexcan will not work.

See this talk for more information for stable device tree ABI:

https://elinux.org/images/0/0e/OSELAS.Presentation-ELCE2017-DT.pdf
https://www.youtube.com/watch?v=6iguKSJJfxo

> You can refer "Documentation/devicetree/bindings/usb/usb-ehci.txt" or
> "Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt" Or
> "Documentation/devicetree/bindings/regmap/regmap.txt".

> In my opinion, keeping this info in devtype data is not good idea.
> E.g. say two platforms which have same FlexCAN hardware revision,
> will have same quirks. BUT these two platforms implement FlexCAN in
> le and be fashion respectively.

Then it's two different platforms. Simply add another compatible to the
driver.

> With current FlexCAN driver, these two platforms can have same
> devtype data. And we need not to change flexcan.c if we want to add
> support for a second platform. We can just add device tree for that
> platform (which would be needed anyway), and it can work. We can
> mention endianness in device tree.

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: 488 bytes --]

RE: [PATCH 1/2] can: flexcan: Remodel FlexCAN register r/w APIs for big endian FlexCAN controllers.

[Pankaj Bansal]

Hi Marc,

Thanks for sharing the video and slides. It was really helpful.
But I would still like to solve this problem using device tree property.
My rationale behind it is that, if a platform designer uses same IP block whose support is present is linux kernel, but with different endianness.
Then he would not need to add his platform data in each driver to support his platform in linux.
He can just add endianness property in device tree and can use latest linux kernel right off the bat.
This is also mentioned in "Documentation/devicetree/bindings/regmap/regmap.txt".

Regmap defaults to little-endian register access on MMIO based
devices, this is by far the most common setting. On CPU
architectures that typically run big-endian operating systems
(e.g. PowerPC), registers can be defined as big-endian and must
be marked that way in the devicetree.

This rule was apparently not followed in P1010RDB flexcan node.

To solve this problem, I suggest that we define 2 optional device tree properties for flexcan.
little-endian : for powerpc architecture, if this property is defined then controller is little endian otherwise big endian (default)
big-endian : for other architectures, if this property is defined then controller is big endian otherwise little endian (default)

Although the controller drivers should be architecture independent, but apparently there is no way around it in flexcan.

let me know your thoughts.

Thanks & Regards,
Pankaj Bansal

-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] 
Sent: Friday, November 10, 2017 6:19 PM
To: Pankaj Bansal <pankaj.bansal@nxp.com>; wg@grandegger.com; linux-can@vger.kernel.org
Cc: Varun Sethi <V.Sethi@nxp.com>; Poonam Aggrwal <poonam.aggrwal@nxp.com>; devicetree@vger.kernel.org
Subject: Re: [PATCH 1/2] can: flexcan: Remodel FlexCAN register r/w APIs for big endian FlexCAN controllers.

On 11/10/2017 01:35 PM, Pankaj Bansal wrote:
>>> 3. Regarding backward compatibility with PowerPC, I see that there is only one platform in PowerPC architecture that is using flexcan.
>>>     There is only one platform in "arch/powerpc/boot/dts/" having flexcan node. p1010si-post.dtsi
>>>     I have added the big-endian property in that platform and have tested it on P1010 platform after backporting the patch to Freescale SDK 1.4 linux.
>>>     I have sent the patch for this. See "[PATCH 2/3] powerpc: dts: P1010: Add endianness property to flexcan node"
>>>     I believe these changes can be accepted for both powerpc and arm and other architectures that use flexcan.
>>
>> No, this is not acceptable. You break the device tree. Please add the le, be information to the devtype data.
> 
> I don't understand how this breaks device tree? can you please elaborate? This method is already being used in other specifications.

Boot a new kernel with an old tree on a PPC board -> flexcan will not work.

See this talk for more information for stable device tree ABI:

https://elinux.org/images/0/0e/OSELAS.Presentation-ELCE2017-DT.pdf
https://www.youtube.com/watch?v=6iguKSJJfxo

> You can refer "Documentation/devicetree/bindings/usb/usb-ehci.txt" or 
> "Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt" Or 
> "Documentation/devicetree/bindings/regmap/regmap.txt".

> In my opinion, keeping this info in devtype data is not good idea.
> E.g. say two platforms which have same FlexCAN hardware revision, will 
> have same quirks. BUT these two platforms implement FlexCAN in le and 
> be fashion respectively.

Then it's two different platforms. Simply add another compatible to the driver.

> With current FlexCAN driver, these two platforms can have same devtype 
> data. And we need not to change flexcan.c if we want to add support 
> for a second platform. We can just add device tree for that platform 
> (which would be needed anyway), and it can work. We can mention 
> endianness in device tree.

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   |

Re: [PATCH 1/2] can: flexcan: Remodel FlexCAN register r/w APIs for big endian FlexCAN controllers.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2323 bytes --]

On 11/10/2017 05:32 PM, Pankaj Bansal wrote:
> Thanks for sharing the video and slides. It was really helpful. But I
> would still like to solve this problem using device tree property. My
> rationale behind it is that, if a platform designer uses same IP
> block whose support is present is linux kernel, but with different
> endianness. Then he would not need to add his platform data in each
> driver to support his platform in linux. He can just add endianness
> property in device tree and can use latest linux kernel right off the
> bat. This is also mentioned in
> "Documentation/devicetree/bindings/regmap/regmap.txt".
> 
> Regmap defaults to little-endian register access on MMIO based
> devices, this is by far the most common setting. On CPU
> architectures that typically run big-endian operating systems
> (e.g. PowerPC), registers can be defined as big-endian and must
> be marked that way in the devicetree.
> 
> This rule was apparently not followed in P1010RDB flexcan node.
> 
> To solve this problem, I suggest that we define 2 optional device
> tree properties for flexcan. little-endian : for powerpc
> architecture, if this property is defined then controller is little
> endian otherwise big endian (default) big-endian : for other
> architectures, if this property is defined then controller is big
> endian otherwise little endian (default)
> 
> Although the controller drivers should be architecture independent,
> but apparently there is no way around it in flexcan.

Please keep the endianess as default es stated in the comment in the driver:

>> /* 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
>>  * endianness.
>>  */

See description of commit 0e4b949e6620 ("can: flexcan: fix flexcan
driver build for big endian on ARM and little endian on PowerPc") for
more information.

I'll not ACK a change in the driver, that's breaking PPC.

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: 488 bytes --]