>-----Original Message-----
>From: Krzysztof Kozlowski <[email protected]>
>Sent: Monday, February 7, 2022 5:27 PM
>To: Jonathan Neuschäfer <[email protected]>; Tyrone Ting <[email protected]>
>Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; IS20 Tali Perry <[email protected]>; IS20 Avi Fishman <[email protected]>; IS20 Tomer Maimon <[email protected]>; CS20 KWLiu <[email protected]>; CS20 JJLiu0 <[email protected]>; CS20 KFTing <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
>Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>
>On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
>> Hello,
>>
>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>>> From: Tyrone Ting <[email protected]>
>>>
>>> NPCM8XX uses a similar i2c module as NPCM7XX.
>>> The only difference is that the internal HW FIFO is larger.
>>>
>>> Related Makefile and Kconfig files are modified to support as well.
>>>
>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
>>> driver")
>>
>> It's not really a bug fix, but rather an additional feature.
>> Therefore, I suggest removing the Fixes tag from this patch.
>>
>>> Signed-off-by: Tyrone Ting <[email protected]>
>>> Signed-off-by: Tali Perry <[email protected]>
>>> ---
>> [...]
>>> /* init register and default value required to enable module */
>>> #define NPCM_I2CSEGCTL 0xE4
>>> +#ifdef CONFIG_ARCH_NPCM7XX
>>> #define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
>>> +#else
>>> +#define NPCM_I2CSEGCTL_INIT_VAL 0x9333F000
>>> +#endif
>>
>> This is going to cause problems when someone tries to compile a kernel
>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
>> only work on NPCM7xx).
>
>Yes, good catch.
>
>The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
>
NPCM7xx and NPCM8xx are very different devices.
They share same driver sources for some of the modules but it's not ABI.
Users cannot compile a single kernel with two separate DTS.
In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
and the NPCM8xx has 32 bytes HW FIFO.
This also means that registers fields are slightly different.
For init data we can move it to the DTS, but register field sizes
can't be handled with this approach.
>>
>> And every time another platform is added, this approach will make the
>> code less readable.
>>
>> A more future-proof approach is probably to have a struct with chip-
>> specific data (such as the I2CSECCTL initialization value), which is
>> then selected via the .data field in of_device_id.
>
>
>Best regards,
>Krzysztof
Thanks for the review!
Tali Perry, Nuvoton.
On 08/02/2022 08:14, Tali Perry wrote:
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[email protected]>
>> Sent: Monday, February 7, 2022 5:27 PM
>> To: Jonathan Neuschäfer <[email protected]>; Tyrone Ting <[email protected]>
>> Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; IS20 Tali Perry <[email protected]>; IS20 Avi Fishman <[email protected]>; IS20 Tomer Maimon <[email protected]>; CS20 KWLiu <[email protected]>; CS20 JJLiu0 <[email protected]>; CS20 KFTing <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
>> Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>>
>> On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
>>> Hello,
>>>
>>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>>>> From: Tyrone Ting <[email protected]>
>>>>
>>>> NPCM8XX uses a similar i2c module as NPCM7XX.
>>>> The only difference is that the internal HW FIFO is larger.
>>>>
>>>> Related Makefile and Kconfig files are modified to support as well.
>>>>
>>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
>>>> driver")
>>>
>>> It's not really a bug fix, but rather an additional feature.
>>> Therefore, I suggest removing the Fixes tag from this patch.
>>>
>>>> Signed-off-by: Tyrone Ting <[email protected]>
>>>> Signed-off-by: Tali Perry <[email protected]>
>>>> ---
>>> [...]
>>>> /* init register and default value required to enable module */
>>>> #define NPCM_I2CSEGCTL 0xE4
>>>> +#ifdef CONFIG_ARCH_NPCM7XX
>>>> #define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
>>>> +#else
>>>> +#define NPCM_I2CSEGCTL_INIT_VAL 0x9333F000
>>>> +#endif
>>>
>>> This is going to cause problems when someone tries to compile a kernel
>>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
>>> only work on NPCM7xx).
>>
>> Yes, good catch.
>>
>> The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
>>
>
> NPCM7xx and NPCM8xx are very different devices.
> They share same driver sources for some of the modules but it's not ABI.
> Users cannot compile a single kernel with two separate DTS.
> In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
> and the NPCM8xx has 32 bytes HW FIFO.
> This also means that registers fields are slightly different.
> For init data we can move it to the DTS, but register field sizes
> can't be handled with this approach.
>
What do you mean they cannot compile a kernel with different DTS? Of
course they can - when we talk about multiplatform sub-architectures!
Maybe there is something specific in NPCMxxx which stops it but then it
should not be marked multiplatform.
Best regards,
Krzysztof
>On 08/02/2022 08:14, Tali Perry wrote:
>>> Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>>>
>>> On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
>>>> Hello,
>>>>
>>>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>>>>> From: Tyrone Ting <[email protected]>
>>>>>
>>>>> NPCM8XX uses a similar i2c module as NPCM7XX.
>>>>> The only difference is that the internal HW FIFO is larger.
>>>>>
>>>>> Related Makefile and Kconfig files are modified to support as well.
>>>>>
>>>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
>>>>> driver")
>>>>
>>>> It's not really a bug fix, but rather an additional feature.
>>>> Therefore, I suggest removing the Fixes tag from this patch.
>>>>
>>>>> Signed-off-by: Tyrone Ting <[email protected]>
>>>>> Signed-off-by: Tali Perry <[email protected]>
>>>>> ---
>>>> [...]
>>>>> /* init register and default value required to enable module */
>>>>> #define NPCM_I2CSEGCTL 0xE4
>>>>> +#ifdef CONFIG_ARCH_NPCM7XX
>>>>> #define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
>>>>> +#else
>>>>> +#define NPCM_I2CSEGCTL_INIT_VAL 0x9333F000
>>>>> +#endif
>>>>
>>>> This is going to cause problems when someone tries to compile a kernel
>>>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
>>>> only work on NPCM7xx).
>>>
>>> Yes, good catch.
>>>
>>> The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
>>>
>>
>> NPCM7xx and NPCM8xx are very different devices.
>> They share same driver sources for some of the modules but it's not ABI.
>> Users cannot compile a single kernel with two separate DTS.
>> In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
>> and the NPCM8xx has 32 bytes HW FIFO.
>> This also means that registers fields are slightly different.
>> For init data we can move it to the DTS, but register field sizes
>> can't be handled with this approach.
>>
>
>What do you mean they cannot compile a kernel with different DTS? Of
>course they can - when we talk about multiplatform sub-architectures!
>Maybe there is something specific in NPCMxxx which stops it but then it
>should not be marked multiplatform.
>
NCPM7xx is ARM32 bit (dual core Cortex A9)
NPCM8xx is ARM64 bit (quad core Cortex A35)
They have completely different architecture so not ABI compliant.
I2C module is similar, but the devices are quite different and have
separate architectures.
Sorry for the confusion.
This is the first patch we try to upstream for NPCM8xx.
In the coming weeks we will upstream the architecture of NPCM8xx as well.
>
>Best regards,
>Krzysztof
Thanks!
Tali