| Message ID | 20220207063338.6570-7-warp5tw@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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 <kfting@nuvoton.com> >> >> 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 <kfting@nuvoton.com> >> Signed-off-by: Tali Perry <tali.perry1@gmail.com> >> --- > [...] >> /* 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? > > 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
>-----Original Message----- >From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >Sent: Monday, February 7, 2022 5:27 PM >To: Jonathan Neuschäfer <j.neuschaefer@gmx.net>; Tyrone Ting <warp5tw@gmail.com> >Cc: avifishman70@gmail.com; tmaimon77@gmail.com; tali.perry1@gmail.com; venture@google.com; yuenn@google.com; benjaminfair@google.com; robh+dt@kernel.org; semen.protsenko@linaro.org; yangyicong@hisilicon.com; wsa@kernel.org; jie.deng@intel.com; sven@svenpeter.dev; bence98@sch.bme.hu; lukas.bulwahn@gmail.com; arnd@arndb.de; olof@lixom.net; andriy.shevchenko@linux.intel.com; IS20 Tali Perry <tali.perry@nuvoton.com>; IS20 Avi Fishman <Avi.Fishman@nuvoton.com>; IS20 Tomer Maimon <tomer.maimon@nuvoton.com>; CS20 KWLiu <KWLIU@nuvoton.com>; CS20 JJLiu0 <JJLIU0@nuvoton.com>; CS20 KFTing <KFTING@nuvoton.com>; devicetree@vger.kernel.org; openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org >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 <kfting@nuvoton.com> >>> >>> 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 <kfting@nuvoton.com> >>> Signed-off-by: Tali Perry <tali.perry1@gmail.com> >>> --- >> [...] >>> /* 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.
>-----Original Message----- >From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >Sent: Monday, February 7, 2022 5:27 PM >To: Jonathan Neuschäfer <j.neuschaefer@gmx.net>; Tyrone Ting <warp5tw@gmail.com> >Cc: avifishman70@gmail.com; tmaimon77@gmail.com; tali.perry1@gmail.com; venture@google.com; yuenn@google.com; benjaminfair@google.com; robh+dt@kernel.org; semen.protsenko@linaro.org; yangyicong@hisilicon.com; wsa@kernel.org; jie.deng@intel.com; sven@svenpeter.dev; bence98@sch.bme.hu; lukas.bulwahn@gmail.com; arnd@arndb.de; olof@lixom.net; andriy.shevchenko@linux.intel.com; IS20 Tali Perry <tali.perry@nuvoton.com>; IS20 Avi Fishman <Avi.Fishman@nuvoton.com>; IS20 Tomer Maimon <tomer.maimon@nuvoton.com>; CS20 KWLiu <KWLIU@nuvoton.com>; CS20 JJLiu0 <JJLIU0@nuvoton.com>; CS20 KFTing <KFTING@nuvoton.com>; devicetree@vger.kernel.org; openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org >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 <kfting@nuvoton.com> >>> >>> 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 <kfting@nuvoton.com> >>> Signed-off-by: Tali Perry <tali.perry1@gmail.com> >>> --- >> [...] >>> /* 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 <krzysztof.kozlowski@canonical.com> >> Sent: Monday, February 7, 2022 5:27 PM >> To: Jonathan Neuschäfer <j.neuschaefer@gmx.net>; Tyrone Ting <warp5tw@gmail.com> >> Cc: avifishman70@gmail.com; tmaimon77@gmail.com; tali.perry1@gmail.com; venture@google.com; yuenn@google.com; benjaminfair@google.com; robh+dt@kernel.org; semen.protsenko@linaro.org; yangyicong@hisilicon.com; wsa@kernel.org; jie.deng@intel.com; sven@svenpeter.dev; bence98@sch.bme.hu; lukas.bulwahn@gmail.com; arnd@arndb.de; olof@lixom.net; andriy.shevchenko@linux.intel.com; IS20 Tali Perry <tali.perry@nuvoton.com>; IS20 Avi Fishman <Avi.Fishman@nuvoton.com>; IS20 Tomer Maimon <tomer.maimon@nuvoton.com>; CS20 KWLiu <KWLIU@nuvoton.com>; CS20 JJLiu0 <JJLIU0@nuvoton.com>; CS20 KFTing <KFTING@nuvoton.com>; devicetree@vger.kernel.org; openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org >> 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 <kfting@nuvoton.com> >>>> >>>> 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 <kfting@nuvoton.com> >>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com> >>>> --- >>> [...] >>>> /* 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 <kfting@nuvoton.com> >>>>> >>>>> 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 <kfting@nuvoton.com> >>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com> >>>>> --- >>>> [...] >>>>> /* 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
On 08/02/2022 09:51, Tali Perry wrote: >> 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 <kfting@nuvoton.com> >>>>>> >>>>>> 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 <kfting@nuvoton.com> >>>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com> >>>>>> --- >>>>> [...] >>>>>> /* 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. OK, in such case usually you indeed can't have both. :) > 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. Still, ARCH_XXX should not be hard-coded in the drivers to change the driver's behavior, even if driver won't be used simultaneously. It breaks all design principles and prevents any further re-use if a new use case appears. You can use "ifdef ARCH_XXX" to skip building of some parts of the driver, but it's not the case here. Best regards, Krzysztof
>On 08/02/2022 09:51, Tali Perry wrote: >>> 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 <kfting@nuvoton.com> >>>>>>> >>>>>>> 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 <kfting@nuvoton.com> >>>>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com> >>>>>>> --- >>>>>> [...] >>>>>>> /* 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. > >OK, in such case usually you indeed can't have both. :) > >> 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. > >Still, ARCH_XXX should not be hard-coded in the drivers to change the >driver's behavior, even if driver won't be used simultaneously. It >breaks all design principles and prevents any further re-use if a new >use case appears. > >You can use "ifdef ARCH_XXX" to skip building of some parts of the >driver, but it's not the case here. > Correct, the main change is in FIFO size: +#ifdef CONFIG_ARCH_NPCM7XX #define I2C_HW_FIFO_SIZE 16 +#else +#define I2C_HW_FIFO_SIZE 32 +#endif /* CONFIG_ARCH_NPCM7XX */ NPCM7XX will always have 16 bytes, all the next gens will have 32. This impact some registers sizes, like this one: +#ifdef CONFIG_ARCH_NPCM7XX #define NPCM_I2CRXF_STS_RX_BYTES GENMASK(4, 0) +#else +#define NPCM_I2CRXF_STS_RX_BYTES GENMASK(5, 0) +#endif /*CONFIG_ARCH_NPCM7XX*/ For this, the FIFO size should be defined before compilation. I also don't want to let users select FIFO size per architecture. NPCM7XX has 16, NPCM8XX has 32. This is not a user selection. It's part of the arch. > >Best regards, >Krzysztof Thanks, Tali
On 08/02/2022 10:22, Tali Perry wrote: >> On 08/02/2022 09:51, Tali Perry wrote: >>>> 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 <kfting@nuvoton.com> >>>>>>>> >>>>>>>> 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 <kfting@nuvoton.com> >>>>>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com> >>>>>>>> --- >>>>>>> [...] >>>>>>>> /* 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. >> >> OK, in such case usually you indeed can't have both. :) >> >>> 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. >> >> Still, ARCH_XXX should not be hard-coded in the drivers to change the >> driver's behavior, even if driver won't be used simultaneously. It >> breaks all design principles and prevents any further re-use if a new >> use case appears. >> >> You can use "ifdef ARCH_XXX" to skip building of some parts of the >> driver, but it's not the case here. >> > > Correct, the main change is in FIFO size: > +#ifdef CONFIG_ARCH_NPCM7XX > #define I2C_HW_FIFO_SIZE 16 > +#else > +#define I2C_HW_FIFO_SIZE 32 > +#endif /* CONFIG_ARCH_NPCM7XX */ > > NPCM7XX will always have 16 bytes, all the next gens will have 32. > > This impact some registers sizes, like this one: > > +#ifdef CONFIG_ARCH_NPCM7XX > #define NPCM_I2CRXF_STS_RX_BYTES GENMASK(4, 0) > +#else > +#define NPCM_I2CRXF_STS_RX_BYTES GENMASK(5, 0) > +#endif /*CONFIG_ARCH_NPCM7XX*/ > > For this, the FIFO size should be defined before compilation. No, it does not have to. We solved it numerous time with quirks or per-chip-drvdata. It's common. > I also don't want to let users select FIFO size per architecture. > NPCM7XX has 16, NPCM8XX has 32. This is not a user selection. > It's part of the arch. I understand it is part of the architecture but why Nuvoton is different than other architectures and requires special treatment here? With most of the drivers, regardless of possibility of running same build on different hardware, we strive to make it ifdef-independent. Please run: git grep ARCH -- drivers/i2c/busses/*c and see how many of such ifdef patterns you see. You also won't find them if you grep for CONFIG... The driver should be designed in portable way. The driver should not have architecture-dependent code. Best regards, Krzysztof
On Tue, Feb 8, 2022 at 11:22 AM Tali Perry <tali.perry1@gmail.com> wrote: > > >On 08/02/2022 09:51, Tali Perry wrote: > >>> 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 <kfting@nuvoton.com> > >>>>>>> > >>>>>>> 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 <kfting@nuvoton.com> > >>>>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com> > >>>>>>> --- > >>>>>> [...] > >>>>>>> /* 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. > > > >OK, in such case usually you indeed can't have both. :) > > > >> 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. > > > >Still, ARCH_XXX should not be hard-coded in the drivers to change the > >driver's behavior, even if driver won't be used simultaneously. It > >breaks all design principles and prevents any further re-use if a new > >use case appears. > > > >You can use "ifdef ARCH_XXX" to skip building of some parts of the > >driver, but it's not the case here. > > > > Correct, the main change is in FIFO size: > +#ifdef CONFIG_ARCH_NPCM7XX > #define I2C_HW_FIFO_SIZE 16 > +#else > +#define I2C_HW_FIFO_SIZE 32 > +#endif /* CONFIG_ARCH_NPCM7XX */ > > NPCM7XX will always have 16 bytes, all the next gens will have 32. > > This impact some registers sizes, like this one: > > +#ifdef CONFIG_ARCH_NPCM7XX > #define NPCM_I2CRXF_STS_RX_BYTES GENMASK(4, 0) > +#else > +#define NPCM_I2CRXF_STS_RX_BYTES GENMASK(5, 0) > +#endif /*CONFIG_ARCH_NPCM7XX*/ > > For this, the FIFO size should be defined before compilation. > I also don't want to let users select FIFO size per architecture. > NPCM7XX has 16, NPCM8XX has 32. This is not a user selection. > It's part of the arch. Will this be a better approach to add in the Kconfig file a legacy mode: config I2C_NPCM_VER_89 bool "support legacy i2c version 0x89 of npcm" depends on ARCH_NPCM7XX default y help This limits the support to legacy i2c version 0x89 of npcm Will be only 16 bytes fifo and will not include other new features config I2C_NPCM tristate "Nuvoton I2C Controller" depends on ARCH_NPCM || COMPILE_TEST select I2C_NPCM_VER_89 if ARCH_NPCM7XX help If you say yes to this option, support will be included for the Nuvoton I2C controller, which is available on the NPCM BMC controllers. Driver can also support slave mode (select I2C_SLAVE). and in the code replace: #ifdef CONFIG_ARCH_NPCM7XX with #ifdef CONFIG_I2C_NPCM_VER_89 > > > > > > >Best regards, > >Krzysztof > > Thanks, > Tali
On 08/02/2022 10:31, Avi Fishman wrote: > On Tue, Feb 8, 2022 at 11:22 AM Tali Perry <tali.perry1@gmail.com> wrote: >> >>> On 08/02/2022 09:51, Tali Perry wrote: >>>>> 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 <kfting@nuvoton.com> >>>>>>>>> >>>>>>>>> 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 <kfting@nuvoton.com> >>>>>>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com> >>>>>>>>> --- >>>>>>>> [...] >>>>>>>>> /* 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. >>> >>> OK, in such case usually you indeed can't have both. :) >>> >>>> 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. >>> >>> Still, ARCH_XXX should not be hard-coded in the drivers to change the >>> driver's behavior, even if driver won't be used simultaneously. It >>> breaks all design principles and prevents any further re-use if a new >>> use case appears. >>> >>> You can use "ifdef ARCH_XXX" to skip building of some parts of the >>> driver, but it's not the case here. >>> >> >> Correct, the main change is in FIFO size: >> +#ifdef CONFIG_ARCH_NPCM7XX >> #define I2C_HW_FIFO_SIZE 16 >> +#else >> +#define I2C_HW_FIFO_SIZE 32 >> +#endif /* CONFIG_ARCH_NPCM7XX */ >> >> NPCM7XX will always have 16 bytes, all the next gens will have 32. >> >> This impact some registers sizes, like this one: >> >> +#ifdef CONFIG_ARCH_NPCM7XX >> #define NPCM_I2CRXF_STS_RX_BYTES GENMASK(4, 0) >> +#else >> +#define NPCM_I2CRXF_STS_RX_BYTES GENMASK(5, 0) >> +#endif /*CONFIG_ARCH_NPCM7XX*/ >> >> For this, the FIFO size should be defined before compilation. >> I also don't want to let users select FIFO size per architecture. >> NPCM7XX has 16, NPCM8XX has 32. This is not a user selection. >> It's part of the arch. > > Will this be a better approach to add in the Kconfig file a legacy mode: > config I2C_NPCM_VER_89 > bool "support legacy i2c version 0x89 of npcm" > depends on ARCH_NPCM7XX > default y > help > This limits the support to legacy i2c version 0x89 of npcm > Will be only 16 bytes fifo and will not include other new features > > config I2C_NPCM > tristate "Nuvoton I2C Controller" > depends on ARCH_NPCM || COMPILE_TEST > select I2C_NPCM_VER_89 if ARCH_NPCM7XX > help > If you say yes to this option, support will be included for the > Nuvoton I2C controller, which is available on the NPCM BMC > controllers. > Driver can also support slave mode (select I2C_SLAVE). > > and in the code replace: > #ifdef CONFIG_ARCH_NPCM7XX > with > #ifdef CONFIG_I2C_NPCM_VER_89 This looks better but it is still the same approach - embedding ifdefs to control the code flow and logic. Only naming is different. Whenever you come with new I2C block for old or new architecture, you will have the same problem. We solved such problems this already - quirks or per-chip-drvdata - so here should be the same. Even if the code does not end up in the same binary, it should be designed properly. Because in the future it might end-up in same binary. Everyone thought there will be no multiplatform ARMv7 so entire arch/arm/mach-* was written exactly like that. And what? It was a lot of effort to all of us to fix that mistakes. ARMv8 can work in 32-bit, so who knows... Just do it properly. Best regards, Krzysztof
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 42da31c1ab70..ab9ee2de5e00 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -817,13 +817,13 @@ config I2C_NOMADIK I2C interface from ST-Ericsson's Nomadik and Ux500 architectures, as well as the STA2X11 PCIe I/O HUB. -config I2C_NPCM7XX +config I2C_NPCM tristate "Nuvoton I2C Controller" - depends on ARCH_NPCM7XX || COMPILE_TEST + depends on ARCH_NPCM || COMPILE_TEST help If you say yes to this option, support will be included for the - Nuvoton I2C controller, which is available on the NPCM7xx BMC - controller. + Nuvoton I2C controller, which is available on the NPCM BMC + controllers. Driver can also support slave mode (select I2C_SLAVE). config I2C_OCORES diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 1d00dce77098..01fdf74a5565 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -80,7 +80,7 @@ obj-$(CONFIG_I2C_MT7621) += i2c-mt7621.o obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o obj-$(CONFIG_I2C_MXS) += i2c-mxs.o obj-$(CONFIG_I2C_NOMADIK) += i2c-nomadik.o -obj-$(CONFIG_I2C_NPCM7XX) += i2c-npcm7xx.o +obj-$(CONFIG_I2C_NPCM) += i2c-npcm7xx.o obj-$(CONFIG_I2C_OCORES) += i2c-ocores.o obj-$(CONFIG_I2C_OMAP) += i2c-omap.o obj-$(CONFIG_I2C_OWL) += i2c-owl.o diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c index 0af97f179af2..4e0148ccc215 100644 --- a/drivers/i2c/busses/i2c-npcm7xx.c +++ b/drivers/i2c/busses/i2c-npcm7xx.c @@ -91,7 +91,11 @@ enum i2c_addr { /* 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 /* Common regs */ #define NPCM_I2CSDA 0x00 @@ -229,7 +233,11 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { #define NPCM_I2CFIF_CTS_SLVRSTR BIT(7) /* NPCM_I2CTXF_CTL reg fields */ +#ifdef CONFIG_ARCH_NPCM7XX #define NPCM_I2CTXF_CTL_TX_THR GENMASK(4, 0) +#else +#define NPCM_I2CTXF_CTL_TX_THR GENMASK(5, 0) +#endif /*CONFIG_ARCH_NPCM7XX*/ #define NPCM_I2CTXF_CTL_THR_TXIE BIT(6) /* NPCM_I2CT_OUT reg fields */ @@ -238,22 +246,42 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { #define NPCM_I2CT_OUT_T_OUTST BIT(7) /* NPCM_I2CTXF_STS reg fields */ +#ifdef CONFIG_ARCH_NPCM7XX #define NPCM_I2CTXF_STS_TX_BYTES GENMASK(4, 0) +#else +#define NPCM_I2CTXF_STS_TX_BYTES GENMASK(5, 0) +#endif /*CONFIG_ARCH_NPCM7XX*/ #define NPCM_I2CTXF_STS_TX_THST BIT(6) /* NPCM_I2CRXF_STS reg fields */ +#ifdef CONFIG_ARCH_NPCM7XX #define NPCM_I2CRXF_STS_RX_BYTES GENMASK(4, 0) +#else +#define NPCM_I2CRXF_STS_RX_BYTES GENMASK(5, 0) +#endif /*CONFIG_ARCH_NPCM7XX*/ #define NPCM_I2CRXF_STS_RX_THST BIT(6) /* NPCM_I2CFIF_CTL reg fields */ #define NPCM_I2CFIF_CTL_FIFO_EN BIT(4) /* NPCM_I2CRXF_CTL reg fields */ +#ifdef CONFIG_ARCH_NPCM7XX #define NPCM_I2CRXF_CTL_RX_THR GENMASK(4, 0) +#else +#define NPCM_I2CRXF_CTL_RX_THR GENMASK(5, 0) +#endif /*CONFIG_ARCH_NPCM7XX*/ +#ifdef CONFIG_ARCH_NPCM7XX #define NPCM_I2CRXF_CTL_LAST_PEC BIT(5) +#else +#define NPCM_I2CRXF_CTL_LAST_PEC BIT(7) +#endif /*CONFIG_ARCH_NPCM7XX*/ #define NPCM_I2CRXF_CTL_THR_RXIE BIT(6) +#ifdef CONFIG_ARCH_NPCM7XX #define I2C_HW_FIFO_SIZE 16 +#else +#define I2C_HW_FIFO_SIZE 32 +#endif /* CONFIG_ARCH_NPCM7XX */ /* I2C_VER reg fields */ #define I2C_VER_VERSION GENMASK(6, 0) @@ -2356,6 +2384,7 @@ static int npcm_i2c_remove_bus(struct platform_device *pdev) static const struct of_device_id npcm_i2c_bus_of_table[] = { { .compatible = "nuvoton,npcm750-i2c", }, + { .compatible = "nuvoton,npcm845-i2c", }, {} }; MODULE_DEVICE_TABLE(of, npcm_i2c_bus_of_table);