| Message ID | 20180803030237.3366-15-songjun.wu@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | MIPS: intel: add initial support for Intel MIPS SoCs | expand |
On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote: > Previous implementation uses platform-dependent API to get the clock. > Those functions are not available for other SoC which uses the same IP. > The CCF (Common Clock Framework) have an abstraction based APIs for > clock. In future, the platform specific code will be removed when the > legacy soc use CCF as well. > Change to use CCF APIs to get clock and rate. So that different SoCs > can use the same driver. > > Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com> > --- > > Changes in v2: None > > drivers/tty/serial/lantiq.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c > index 36479d66fb7c..35518ab3a80d 100644 > --- a/drivers/tty/serial/lantiq.c > +++ b/drivers/tty/serial/lantiq.c > @@ -26,7 +26,9 @@ > #include <linux/clk.h> > #include <linux/gpio.h> > > +#ifdef CONFIG_LANTIQ > #include <lantiq_soc.h> > +#endif That is never how you do this in Linux, you know better. Please go and get this patchset reviewed and signed-off-by from other internal Intel kernel developers before resending it next time. It is their job to find and fix your basic errors like this, not ours. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote: > On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote: >> Previous implementation uses platform-dependent API to get the clock. >> Those functions are not available for other SoC which uses the same IP. >> The CCF (Common Clock Framework) have an abstraction based APIs for >> clock. In future, the platform specific code will be removed when the >> legacy soc use CCF as well. >> Change to use CCF APIs to get clock and rate. So that different SoCs >> can use the same driver. >> >> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com> >> --- >> >> Changes in v2: None >> >> drivers/tty/serial/lantiq.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c >> index 36479d66fb7c..35518ab3a80d 100644 >> --- a/drivers/tty/serial/lantiq.c >> +++ b/drivers/tty/serial/lantiq.c >> @@ -26,7 +26,9 @@ >> #include <linux/clk.h> >> #include <linux/gpio.h> >> >> +#ifdef CONFIG_LANTIQ >> #include <lantiq_soc.h> >> +#endif > That is never how you do this in Linux, you know better. > > Please go and get this patchset reviewed and signed-off-by from other > internal Intel kernel developers before resending it next time. It is > their job to find and fix your basic errors like this, not ours. Thank you for your comment. Actually, we have discussed this issue internally. We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit message in "[PATCH v2 08/18] serial: intel: Get serial id from dts". Please refer the commit message below. "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC macro is defined in lantiq_soc.h. lantiq_soc.h is in arch path for legacy product support. arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h If "#ifdef preprocessor" is changed to "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled, code using LTQ_EARLY_ASC is compiled. Compilation will fail for no LTQ_EARLY_ASC defined. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote: > > > On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote: > > On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote: > > > Previous implementation uses platform-dependent API to get the clock. > > > Those functions are not available for other SoC which uses the same IP. > > > The CCF (Common Clock Framework) have an abstraction based APIs for > > > clock. In future, the platform specific code will be removed when the > > > legacy soc use CCF as well. > > > Change to use CCF APIs to get clock and rate. So that different SoCs > > > can use the same driver. > > > > > > Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com> > > > --- > > > > > > Changes in v2: None > > > > > > drivers/tty/serial/lantiq.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c > > > index 36479d66fb7c..35518ab3a80d 100644 > > > --- a/drivers/tty/serial/lantiq.c > > > +++ b/drivers/tty/serial/lantiq.c > > > @@ -26,7 +26,9 @@ > > > #include <linux/clk.h> > > > #include <linux/gpio.h> > > > +#ifdef CONFIG_LANTIQ > > > #include <lantiq_soc.h> > > > +#endif > > That is never how you do this in Linux, you know better. > > > > Please go and get this patchset reviewed and signed-off-by from other > > internal Intel kernel developers before resending it next time. It is > > their job to find and fix your basic errors like this, not ours. > Thank you for your comment. > Actually, we have discussed this issue internally. > We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit > message in "[PATCH v2 08/18] serial: intel: Get serial id from dts". > Please refer the commit message below. > > "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC > macro is defined in lantiq_soc.h. > lantiq_soc.h is in arch path for legacy product support. > > arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h > > If "#ifdef preprocessor" is changed to > "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled, > code using LTQ_EARLY_ASC is compiled. > Compilation will fail for no LTQ_EARLY_ASC defined. Sorry, but no. Why is this one tiny driver/chip somehow more "special" than all of the tens of thousands of other devices we support to warrent it getting some sort of special exception to do things differently? What happens to the next device that wants to do it this way? Our coding style and rules are there for a reason, do not violate them thinking your device is the only one that matters. Do it properly, again, you all know better than this. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote: > On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote: >> >> >> On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote: >>> On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote: >>>> Previous implementation uses platform-dependent API to get the clock. >>>> Those functions are not available for other SoC which uses the same IP. >>>> The CCF (Common Clock Framework) have an abstraction based APIs for >>>> clock. In future, the platform specific code will be removed when the >>>> legacy soc use CCF as well. >>>> Change to use CCF APIs to get clock and rate. So that different SoCs >>>> can use the same driver. >>>> >>>> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com> >>>> --- >>>> >>>> Changes in v2: None >>>> >>>> drivers/tty/serial/lantiq.c | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c >>>> index 36479d66fb7c..35518ab3a80d 100644 >>>> --- a/drivers/tty/serial/lantiq.c >>>> +++ b/drivers/tty/serial/lantiq.c >>>> @@ -26,7 +26,9 @@ >>>> #include <linux/clk.h> >>>> #include <linux/gpio.h> >>>> +#ifdef CONFIG_LANTIQ >>>> #include <lantiq_soc.h> >>>> +#endif >>> That is never how you do this in Linux, you know better. >>> >>> Please go and get this patchset reviewed and signed-off-by from other >>> internal Intel kernel developers before resending it next time. It is >>> their job to find and fix your basic errors like this, not ours. >> Thank you for your comment. >> Actually, we have discussed this issue internally. >> We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit >> message in "[PATCH v2 08/18] serial: intel: Get serial id from dts". >> Please refer the commit message below. >> >> "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC >> macro is defined in lantiq_soc.h. >> lantiq_soc.h is in arch path for legacy product support. >> >> arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h >> >> If "#ifdef preprocessor" is changed to >> "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled, >> code using LTQ_EARLY_ASC is compiled. >> Compilation will fail for no LTQ_EARLY_ASC defined. > > Sorry, but no. Why is this one tiny driver/chip somehow more "special" > than all of the tens of thousands of other devices we support to warrent > it getting some sort of special exception to do things differently? > What happens to the next device that wants to do it this way? > > Our coding style and rules are there for a reason, do not violate them > thinking your device is the only one that matters. > > Do it properly, again, you all know better than this. > > greg k-h > Hi Greg, The problem is that the Lantiq SoC code in arch/mips/lantiq does not use the common clock framework, but it uses the clk framework directly. It defines CONFIG_HAVE_CLK and CONFIG_CLKDEV_LOOKUP, but not CONFIG_COMMON_CLK. The xRX500 SoC which is being added here is about 2 generations more recent than the VR9/xRX200 SoC which is the latest which is supported by the code in arch/mips/lantiq. With this new SoC we switched to the common clock framework. This driver is used by the older SoC and also by the new ones because this IP core is pretty similar in all the SoCs. This patch makes it possible to use it with the legacy lantiq code and also with the common clock framework. I see multiple options to fix this problem. 1. The current approach to have it as a compile variant for a) legacy lantiq arch code without common clock framework and b) support for SoCs using the common clock framework. 2. Convert the lantiq arch code to the common clock framework. This would be a good approach, but it need some efforts. 3. Remove the arch/mips/lantiq code. There are still users of this code. 4. Use the old APIs also for the new xRX500 SoC, I do not like this approach. 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally available and provide some better wrapper code. Hauke
On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote: > On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote: > > On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote: > >> > >> > >> On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote: > >>> On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote: > >>>> Previous implementation uses platform-dependent API to get the clock. > >>>> Those functions are not available for other SoC which uses the same IP. > >>>> The CCF (Common Clock Framework) have an abstraction based APIs for > >>>> clock. In future, the platform specific code will be removed when the > >>>> legacy soc use CCF as well. > >>>> Change to use CCF APIs to get clock and rate. So that different SoCs > >>>> can use the same driver. > >>>> > >>>> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com> > >>>> --- > >>>> > >>>> Changes in v2: None > >>>> > >>>> drivers/tty/serial/lantiq.c | 11 +++++++++++ > >>>> 1 file changed, 11 insertions(+) > >>>> > >>>> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c > >>>> index 36479d66fb7c..35518ab3a80d 100644 > >>>> --- a/drivers/tty/serial/lantiq.c > >>>> +++ b/drivers/tty/serial/lantiq.c > >>>> @@ -26,7 +26,9 @@ > >>>> #include <linux/clk.h> > >>>> #include <linux/gpio.h> > >>>> +#ifdef CONFIG_LANTIQ > >>>> #include <lantiq_soc.h> > >>>> +#endif > >>> That is never how you do this in Linux, you know better. > >>> > >>> Please go and get this patchset reviewed and signed-off-by from other > >>> internal Intel kernel developers before resending it next time. It is > >>> their job to find and fix your basic errors like this, not ours. > >> Thank you for your comment. > >> Actually, we have discussed this issue internally. > >> We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit > >> message in "[PATCH v2 08/18] serial: intel: Get serial id from dts". > >> Please refer the commit message below. > >> > >> "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC > >> macro is defined in lantiq_soc.h. > >> lantiq_soc.h is in arch path for legacy product support. > >> > >> arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h > >> > >> If "#ifdef preprocessor" is changed to > >> "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled, > >> code using LTQ_EARLY_ASC is compiled. > >> Compilation will fail for no LTQ_EARLY_ASC defined. > > > > Sorry, but no. Why is this one tiny driver/chip somehow more "special" > > than all of the tens of thousands of other devices we support to warrent > > it getting some sort of special exception to do things differently? > > What happens to the next device that wants to do it this way? > > > > Our coding style and rules are there for a reason, do not violate them > > thinking your device is the only one that matters. > > > > Do it properly, again, you all know better than this. > > > > greg k-h > > > Hi Greg, > > The problem is that the Lantiq SoC code in arch/mips/lantiq does not use > the common clock framework, but it uses the clk framework directly. It > defines CONFIG_HAVE_CLK and CONFIG_CLKDEV_LOOKUP, but not > CONFIG_COMMON_CLK. The xRX500 SoC which is being added here is about 2 > generations more recent than the VR9/xRX200 SoC which is the latest > which is supported by the code in arch/mips/lantiq. With this new SoC we > switched to the common clock framework. This driver is used by the older > SoC and also by the new ones because this IP core is pretty similar in > all the SoCs. > This patch makes it possible to use it with the legacy lantiq code and > also with the common clock framework. I see multiple options to fix this > problem. > > 1. The current approach to have it as a compile variant for a) legacy > lantiq arch code without common clock framework and b) support for SoCs > using the common clock framework. > 2. Convert the lantiq arch code to the common clock framework. This > would be a good approach, but it need some efforts. > 3. Remove the arch/mips/lantiq code. There are still users of this code. > 4. Use the old APIs also for the new xRX500 SoC, I do not like this > approach. > 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally > available and provide some better wrapper code. I don't really care what you do at this point in time, but you all should know better than the crazy #ifdef is not allowed to try to prevent/allow the inclusion of a .h file. Checkpatch might have even warned you about it, right? So do it correctly, odds are #5 is correct, as that makes it work like any other device in the kernel. You are not unique here. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote: >> On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote: >> > On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote: >> This patch makes it possible to use it with the legacy lantiq code and >> also with the common clock framework. I see multiple options to fix this >> problem. >> >> 1. The current approach to have it as a compile variant for a) legacy >> lantiq arch code without common clock framework and b) support for SoCs >> using the common clock framework. >> 2. Convert the lantiq arch code to the common clock framework. This >> would be a good approach, but it need some efforts. >> 3. Remove the arch/mips/lantiq code. There are still users of this code. >> 4. Use the old APIs also for the new xRX500 SoC, I do not like this >> approach. >> 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally >> available and provide some better wrapper code. > > I don't really care what you do at this point in time, but you all > should know better than the crazy #ifdef is not allowed to try to > prevent/allow the inclusion of a .h file. Checkpatch might have even > warned you about it, right? > > So do it correctly, odds are #5 is correct, as that makes it work like > any other device in the kernel. You are not unique here. The best approach here would clearly be 2. We don't want platform specific header files for doing things that should be completely generic. Converting lantiq to the common-clk framework obviously requires some work, but then again the whole arch/mips/lantiq/clk.c file is fairly short and maybe not that hard to convert. From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you already use the clkdev lookup mechanism for some devices without using COMMON_CLK, so I would assume that you can also use those for the remaining clks, which would be much simpler. It registers one anonymous clk there as clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1); so why not add replace that with two named clocks and just use the same names in the DT for the newer chip? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/5/2018 5:03 AM, Arnd Bergmann wrote: > On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: >> On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote: >>> On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote: >>>> On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote: >>> This patch makes it possible to use it with the legacy lantiq code and >>> also with the common clock framework. I see multiple options to fix this >>> problem. >>> >>> 1. The current approach to have it as a compile variant for a) legacy >>> lantiq arch code without common clock framework and b) support for SoCs >>> using the common clock framework. >>> 2. Convert the lantiq arch code to the common clock framework. This >>> would be a good approach, but it need some efforts. >>> 3. Remove the arch/mips/lantiq code. There are still users of this code. >>> 4. Use the old APIs also for the new xRX500 SoC, I do not like this >>> approach. >>> 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally >>> available and provide some better wrapper code. >> I don't really care what you do at this point in time, but you all >> should know better than the crazy #ifdef is not allowed to try to >> prevent/allow the inclusion of a .h file. Checkpatch might have even >> warned you about it, right? >> >> So do it correctly, odds are #5 is correct, as that makes it work like >> any other device in the kernel. You are not unique here. > The best approach here would clearly be 2. We don't want platform > specific header files for doing things that should be completely generic. > > Converting lantiq to the common-clk framework obviously requires > some work, but then again the whole arch/mips/lantiq/clk.c file > is fairly short and maybe not that hard to convert. > > >From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you > already use the clkdev lookup mechanism for some devices without > using COMMON_CLK, so I would assume that you can also use those > for the remaining clks, which would be much simpler. It registers > one anonymous clk there as > > clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1); > > so why not add replace that with two named clocks and just use > the same names in the DT for the newer chip? > > Arnd We discussed internally and have another solution for this issue. Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in lantiq.h, also providing no-op stub functions in the #else case, then call those functions unconditionally from lantiq.c to avoid #ifdef in C file. To support CCF in legacy product is another topic, is not included in this patch. The implementation is as following： #ifdef CONFIG_LANTIQ #include <lantiq_soc.h> #else #define LTQ_EARLY_ASC 0 #define CPHYSADDR(_val) 0 static inline struct clk *clk_get_fpi(void) { return NULL; } #endif -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Songjun, On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote: > On 8/5/2018 5:03 AM, Arnd Bergmann wrote: > > On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > >> On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote: > >>> On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote: > >>>> On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote: > >>> This patch makes it possible to use it with the legacy lantiq code and > >>> also with the common clock framework. I see multiple options to fix this > >>> problem. > >>> > >>> 1. The current approach to have it as a compile variant for a) legacy > >>> lantiq arch code without common clock framework and b) support for SoCs > >>> using the common clock framework. > >>> 2. Convert the lantiq arch code to the common clock framework. This > >>> would be a good approach, but it need some efforts. > >>> 3. Remove the arch/mips/lantiq code. There are still users of this code. > >>> 4. Use the old APIs also for the new xRX500 SoC, I do not like this > >>> approach. > >>> 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally > >>> available and provide some better wrapper code. > >> I don't really care what you do at this point in time, but you all > >> should know better than the crazy #ifdef is not allowed to try to > >> prevent/allow the inclusion of a .h file. Checkpatch might have even > >> warned you about it, right? > >> > >> So do it correctly, odds are #5 is correct, as that makes it work like > >> any other device in the kernel. You are not unique here. > > The best approach here would clearly be 2. We don't want platform > > specific header files for doing things that should be completely generic. > > > > Converting lantiq to the common-clk framework obviously requires > > some work, but then again the whole arch/mips/lantiq/clk.c file > > is fairly short and maybe not that hard to convert. > > > > >From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you > > already use the clkdev lookup mechanism for some devices without > > using COMMON_CLK, so I would assume that you can also use those > > for the remaining clks, which would be much simpler. It registers > > one anonymous clk there as > > > > clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1); > > > > so why not add replace that with two named clocks and just use > > the same names in the DT for the newer chip? > > > > Arnd > We discussed internally and have another solution for this issue. > Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in > lantiq.h, > also providing no-op stub functions in the #else case, then call those > functions > unconditionally from lantiq.c to avoid #ifdef in C file. > > To support CCF in legacy product is another topic, is not included in > this patch. > > The implementation is as following： > #ifdef CONFIG_LANTIQ > #include <lantiq_soc.h> > #else > #define LTQ_EARLY_ASC 0 > #define CPHYSADDR(_val) 0 > > static inline struct clk *clk_get_fpi(void) > { > return NULL; > } > #endif Why not use clkdev_add(), as Arnd suggested? That would be a 3-line patch without introducing a new header file and an ugly #ifdef, which complicates compile coverage testing? Gr{oetje,eeting}s, Geert
On 8/6/2018 3:20 PM, Geert Uytterhoeven wrote: > Hi Songjun, > > On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote: >> On 8/5/2018 5:03 AM, Arnd Bergmann wrote: >>> On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman >>> <gregkh@linuxfoundation.org> wrote: >>>> On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote: >>>>> On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote: >>>>>> On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote: >>>>> This patch makes it possible to use it with the legacy lantiq code and >>>>> also with the common clock framework. I see multiple options to fix this >>>>> problem. >>>>> >>>>> 1. The current approach to have it as a compile variant for a) legacy >>>>> lantiq arch code without common clock framework and b) support for SoCs >>>>> using the common clock framework. >>>>> 2. Convert the lantiq arch code to the common clock framework. This >>>>> would be a good approach, but it need some efforts. >>>>> 3. Remove the arch/mips/lantiq code. There are still users of this code. >>>>> 4. Use the old APIs also for the new xRX500 SoC, I do not like this >>>>> approach. >>>>> 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally >>>>> available and provide some better wrapper code. >>>> I don't really care what you do at this point in time, but you all >>>> should know better than the crazy #ifdef is not allowed to try to >>>> prevent/allow the inclusion of a .h file. Checkpatch might have even >>>> warned you about it, right? >>>> >>>> So do it correctly, odds are #5 is correct, as that makes it work like >>>> any other device in the kernel. You are not unique here. >>> The best approach here would clearly be 2. We don't want platform >>> specific header files for doing things that should be completely generic. >>> >>> Converting lantiq to the common-clk framework obviously requires >>> some work, but then again the whole arch/mips/lantiq/clk.c file >>> is fairly short and maybe not that hard to convert. >>> >>> >From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you >>> already use the clkdev lookup mechanism for some devices without >>> using COMMON_CLK, so I would assume that you can also use those >>> for the remaining clks, which would be much simpler. It registers >>> one anonymous clk there as >>> >>> clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1); >>> >>> so why not add replace that with two named clocks and just use >>> the same names in the DT for the newer chip? >>> >>> Arnd >> We discussed internally and have another solution for this issue. >> Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in >> lantiq.h, >> also providing no-op stub functions in the #else case, then call those >> functions >> unconditionally from lantiq.c to avoid #ifdef in C file. >> >> To support CCF in legacy product is another topic, is not included in >> this patch. >> >> The implementation is as following： >> #ifdef CONFIG_LANTIQ >> #include <lantiq_soc.h> >> #else >> #define LTQ_EARLY_ASC 0 >> #define CPHYSADDR(_val) 0 >> >> static inline struct clk *clk_get_fpi(void) >> { >> return NULL; >> } >> #endif > Why not use clkdev_add(), as Arnd suggested? > That would be a 3-line patch without introducing a new header file and an ugly > #ifdef, which complicates compile coverage testing? > > Gr{oetje,eeting}s, > > Geert The reason we add a new head file is also for two macros(LTQ_EARLY_ASC and CPHYSADDR) used by legacy product. We need to provide the no-op stub for these two macro for new product. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Songjun, On Mon, Aug 6, 2018 at 10:58 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote: > On 8/6/2018 3:20 PM, Geert Uytterhoeven wrote: > > On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote: > >> On 8/5/2018 5:03 AM, Arnd Bergmann wrote: > >>> On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman > >>> <gregkh@linuxfoundation.org> wrote: > >>>> On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote: > >>>>> On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote: > >>>>>> On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote: > >>>>> This patch makes it possible to use it with the legacy lantiq code and > >>>>> also with the common clock framework. I see multiple options to fix this > >>>>> problem. > >>>>> > >>>>> 1. The current approach to have it as a compile variant for a) legacy > >>>>> lantiq arch code without common clock framework and b) support for SoCs > >>>>> using the common clock framework. > >>>>> 2. Convert the lantiq arch code to the common clock framework. This > >>>>> would be a good approach, but it need some efforts. > >>>>> 3. Remove the arch/mips/lantiq code. There are still users of this code. > >>>>> 4. Use the old APIs also for the new xRX500 SoC, I do not like this > >>>>> approach. > >>>>> 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally > >>>>> available and provide some better wrapper code. > >>>> I don't really care what you do at this point in time, but you all > >>>> should know better than the crazy #ifdef is not allowed to try to > >>>> prevent/allow the inclusion of a .h file. Checkpatch might have even > >>>> warned you about it, right? > >>>> > >>>> So do it correctly, odds are #5 is correct, as that makes it work like > >>>> any other device in the kernel. You are not unique here. > >>> The best approach here would clearly be 2. We don't want platform > >>> specific header files for doing things that should be completely generic. > >>> > >>> Converting lantiq to the common-clk framework obviously requires > >>> some work, but then again the whole arch/mips/lantiq/clk.c file > >>> is fairly short and maybe not that hard to convert. > >>> > >>> >From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you > >>> already use the clkdev lookup mechanism for some devices without > >>> using COMMON_CLK, so I would assume that you can also use those > >>> for the remaining clks, which would be much simpler. It registers > >>> one anonymous clk there as > >>> > >>> clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1); > >>> > >>> so why not add replace that with two named clocks and just use > >>> the same names in the DT for the newer chip? > >>> > >>> Arnd > >> We discussed internally and have another solution for this issue. > >> Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in > >> lantiq.h, > >> also providing no-op stub functions in the #else case, then call those > >> functions > >> unconditionally from lantiq.c to avoid #ifdef in C file. > >> > >> To support CCF in legacy product is another topic, is not included in > >> this patch. > >> > >> The implementation is as following： > >> #ifdef CONFIG_LANTIQ > >> #include <lantiq_soc.h> > >> #else > >> #define LTQ_EARLY_ASC 0 > >> #define CPHYSADDR(_val) 0 > >> > >> static inline struct clk *clk_get_fpi(void) > >> { > >> return NULL; > >> } > >> #endif > > Why not use clkdev_add(), as Arnd suggested? > > That would be a 3-line patch without introducing a new header file and an ugly > > #ifdef, which complicates compile coverage testing? > > > The reason we add a new head file is also for two macros(LTQ_EARLY_ASC > and CPHYSADDR) > used by legacy product. We need to provide the no-op stub for these two > macro for new product. No you don't. The line number should not be obtained by comparing the resource address with a hardcoded base address. Perhaps the override of port->line should just be removed, as IIRC, the serial core has already filled in that field with the (next available) line number? Gr{oetje,eeting}s, Geert
On 8/6/2018 5:29 PM, Geert Uytterhoeven wrote: > Hi Songjun, > > On Mon, Aug 6, 2018 at 10:58 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote: >> On 8/6/2018 3:20 PM, Geert Uytterhoeven wrote: >>> On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote: >>>> On 8/5/2018 5:03 AM, Arnd Bergmann wrote: >>>>> On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman >>>>> <gregkh@linuxfoundation.org> wrote: >>>>>> On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote: >>>>>>> On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote: >>>>>>>> On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote: >>>>>>> This patch makes it possible to use it with the legacy lantiq code and >>>>>>> also with the common clock framework. I see multiple options to fix this >>>>>>> problem. >>>>>>> >>>>>>> 1. The current approach to have it as a compile variant for a) legacy >>>>>>> lantiq arch code without common clock framework and b) support for SoCs >>>>>>> using the common clock framework. >>>>>>> 2. Convert the lantiq arch code to the common clock framework. This >>>>>>> would be a good approach, but it need some efforts. >>>>>>> 3. Remove the arch/mips/lantiq code. There are still users of this code. >>>>>>> 4. Use the old APIs also for the new xRX500 SoC, I do not like this >>>>>>> approach. >>>>>>> 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally >>>>>>> available and provide some better wrapper code. >>>>>> I don't really care what you do at this point in time, but you all >>>>>> should know better than the crazy #ifdef is not allowed to try to >>>>>> prevent/allow the inclusion of a .h file. Checkpatch might have even >>>>>> warned you about it, right? >>>>>> >>>>>> So do it correctly, odds are #5 is correct, as that makes it work like >>>>>> any other device in the kernel. You are not unique here. >>>>> The best approach here would clearly be 2. We don't want platform >>>>> specific header files for doing things that should be completely generic. >>>>> >>>>> Converting lantiq to the common-clk framework obviously requires >>>>> some work, but then again the whole arch/mips/lantiq/clk.c file >>>>> is fairly short and maybe not that hard to convert. >>>>> >>>>> >From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you >>>>> already use the clkdev lookup mechanism for some devices without >>>>> using COMMON_CLK, so I would assume that you can also use those >>>>> for the remaining clks, which would be much simpler. It registers >>>>> one anonymous clk there as >>>>> >>>>> clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1); >>>>> >>>>> so why not add replace that with two named clocks and just use >>>>> the same names in the DT for the newer chip? >>>>> >>>>> Arnd >>>> We discussed internally and have another solution for this issue. >>>> Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in >>>> lantiq.h, >>>> also providing no-op stub functions in the #else case, then call those >>>> functions >>>> unconditionally from lantiq.c to avoid #ifdef in C file. >>>> >>>> To support CCF in legacy product is another topic, is not included in >>>> this patch. >>>> >>>> The implementation is as following： >>>> #ifdef CONFIG_LANTIQ >>>> #include <lantiq_soc.h> >>>> #else >>>> #define LTQ_EARLY_ASC 0 >>>> #define CPHYSADDR(_val) 0 >>>> >>>> static inline struct clk *clk_get_fpi(void) >>>> { >>>> return NULL; >>>> } >>>> #endif >>> Why not use clkdev_add(), as Arnd suggested? >>> That would be a 3-line patch without introducing a new header file and an ugly >>> #ifdef, which complicates compile coverage testing? >>> >> The reason we add a new head file is also for two macros(LTQ_EARLY_ASC >> and CPHYSADDR) >> used by legacy product. We need to provide the no-op stub for these two >> macro for new product. > No you don't. The line number should not be obtained by comparing the > resource address with a hardcoded base address. This is the previous code. Now the line number is obtained from dts. We keep this code for the compatibility. Referring to the conditional-compilation part in coding-style, We add a header file to avoid using “#ifdef” in C file. > Perhaps the override of port->line should just be removed, as IIRC, the serial > core has already filled in that field with the (next available) line number? > > Gr{oetje,eeting}s, > > Geert -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Songjun, On Tue, Aug 7, 2018 at 9:18 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote: > On 8/6/2018 5:29 PM, Geert Uytterhoeven wrote: > > On Mon, Aug 6, 2018 at 10:58 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote: > >> The reason we add a new head file is also for two macros(LTQ_EARLY_ASC > >> and CPHYSADDR) > >> used by legacy product. We need to provide the no-op stub for these two > >> macro for new product. > > No you don't. The line number should not be obtained by comparing the > > resource address with a hardcoded base address. > This is the previous code. Now the line number is obtained from dts. Note that obtaining line numbers from DTS has its own share of problems, when considering DT overlays. I've replied to the patch adding the call to of_alias_get_id(). > We keep this code for the compatibility. > > Referring to the conditional-compilation part in coding-style, > We add a header file to avoid using “#ifdef” in C file. > > Perhaps the override of port->line should just be removed, as IIRC, the serial > > core has already filled in that field with the (next available) line number? Gr{oetje,eeting}s, Geert-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c index 36479d66fb7c..35518ab3a80d 100644 --- a/drivers/tty/serial/lantiq.c +++ b/drivers/tty/serial/lantiq.c @@ -26,7 +26,9 @@ #include <linux/clk.h> #include <linux/gpio.h> +#ifdef CONFIG_LANTIQ #include <lantiq_soc.h> +#endif #define PORT_LTQ_ASC 111 #define MAXPORTS 2 @@ -744,14 +746,23 @@ lqasc_probe(struct platform_device *pdev) port->irq = irqres[0].start; port->mapbase = mmres->start; +#if (IS_ENABLED(CONFIG_LANTIQ) && !IS_ENABLED(CONFIG_COMMON_CLK)) ltq_port->freqclk = clk_get_fpi(); +#else + ltq_port->freqclk = devm_clk_get(&pdev->dev, "freq"); +#endif + if (IS_ERR(ltq_port->freqclk)) { pr_err("failed to get fpi clk\n"); return -ENOENT; } /* not all asc ports have clock gates, lets ignore the return code */ +#if (IS_ENABLED(CONFIG_LANTIQ) && !IS_ENABLED(CONFIG_COMMON_CLK)) ltq_port->clk = clk_get(&pdev->dev, NULL); +#else + ltq_port->clk = devm_clk_get(&pdev->dev, "asc"); +#endif ltq_port->tx_irq = irqres[0].start; ltq_port->rx_irq = irqres[1].start;
Previous implementation uses platform-dependent API to get the clock. Those functions are not available for other SoC which uses the same IP. The CCF (Common Clock Framework) have an abstraction based APIs for clock. In future, the platform specific code will be removed when the legacy soc use CCF as well. Change to use CCF APIs to get clock and rate. So that different SoCs can use the same driver. Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com> --- Changes in v2: None drivers/tty/serial/lantiq.c | 11 +++++++++++ 1 file changed, 11 insertions(+)