From: Peng Fan <[email protected]>
With the clk names specified in clk_bypassed module parameter, give
user an option to bypass the clk from managing them by Linux kernel.
This is useful when Cortex-M33 and Cortex-A55 both running, A55 may
shutdown the clocks used by M33. Although we have mcore_booted module
parameter, but it is only for composite gate, the CCGR gate could still
be shutdown by Linux.
So let's use clk_bypassed parameter to handle clk root and clk ccgr both.
Signed-off-by: Peng Fan <[email protected]>
---
V1:
Depends on: https://lore.kernel.org/all/[email protected]/
drivers/clk/imx/clk-imx93.c | 55 +++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/drivers/clk/imx/clk-imx93.c b/drivers/clk/imx/clk-imx93.c
index 07b4a043e449..88ed824ca2ff 100644
--- a/drivers/clk/imx/clk-imx93.c
+++ b/drivers/clk/imx/clk-imx93.c
@@ -255,6 +255,51 @@ static const struct imx93_clk_ccgr {
static struct clk_hw_onecell_data *clk_hw_data;
static struct clk_hw **clks;
+#define IMX_NUM_CLK_ENTRY 64
+
+static char clk_bypassed_names[SZ_512];
+static char *clk_names[IMX_NUM_CLK_ENTRY];
+
+/* The module args format as this: clk-imx93.clk_bypassed="lpi2c8_root,spdif" */
+static int imx_clk_bypass_setup(struct device *dev, char *str)
+{
+ char *pos = str, *next;
+ int i = 0;
+
+ while (pos != NULL && (next = strchr(pos, ',')) != NULL) {
+ clk_names[i++] = devm_kstrndup(dev, pos, next - pos, GFP_KERNEL);
+ pos = ++next;
+ }
+
+ clk_names[i] = devm_kstrdup(dev, pos, GFP_KERNEL);
+
+ return 0;
+}
+
+static bool imx_clk_bypass_check(char *name)
+{
+ int i;
+
+ for (i = 0; i < IMX_NUM_CLK_ENTRY; i++) {
+ if (!clk_names[i])
+ return false;
+ if (!strcmp(name, clk_names[i]))
+ return true;
+ }
+
+ return false;
+}
+
+static int imx_clk_bypass_free(struct device *dev)
+{
+ int i;
+
+ for (i = 0; i < IMX_NUM_CLK_ENTRY; i++)
+ devm_kfree(dev, clk_names[i]);
+
+ return 0;
+}
+
static int imx93_clocks_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -264,6 +309,8 @@ static int imx93_clocks_probe(struct platform_device *pdev)
void __iomem *base, *anatop_base;
int i, ret;
+ imx_clk_bypass_setup(dev, clk_bypassed_names);
+
clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
IMX93_CLK_END), GFP_KERNEL);
if (!clk_hw_data)
@@ -310,6 +357,8 @@ static int imx93_clocks_probe(struct platform_device *pdev)
for (i = 0; i < ARRAY_SIZE(root_array); i++) {
root = &root_array[i];
+ if (unlikely(imx_clk_bypass_check(root->name)))
+ continue;
clks[root->clk] = imx93_clk_composite_flags(root->name,
parent_names[root->sel],
4, base + root->off, 3,
@@ -318,6 +367,8 @@ static int imx93_clocks_probe(struct platform_device *pdev)
for (i = 0; i < ARRAY_SIZE(ccgr_array); i++) {
ccgr = &ccgr_array[i];
+ if (unlikely(imx_clk_bypass_check(ccgr->name)))
+ continue;
clks[ccgr->clk] = imx93_clk_gate(NULL, ccgr->name, ccgr->parent_name,
ccgr->flags, base + ccgr->off, 0, 1, 1, 3,
ccgr->shared_count);
@@ -341,6 +392,8 @@ static int imx93_clocks_probe(struct platform_device *pdev)
imx_register_uart_clocks();
+ imx_clk_bypass_free(dev);
+
return 0;
unregister_hws:
@@ -367,6 +420,8 @@ static struct platform_driver imx93_clk_driver = {
module_platform_driver(imx93_clk_driver);
module_param(mcore_booted, bool, 0444);
MODULE_PARM_DESC(mcore_booted, "See Cortex-M core is booted or not");
+module_param_string(clk_bypassed, clk_bypassed_names, SZ_512, 0444);
+MODULE_PARM_DESC(clk_bypassed, "The clks will not be managed by Linux");
MODULE_DESCRIPTION("NXP i.MX93 clock driver");
MODULE_LICENSE("GPL v2");
--
2.37.1
On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> With the clk names specified in clk_bypassed module parameter, give
> user an option to bypass the clk from managing them by Linux kernel.
As I said on another email, no, please do not add new module parameters
for drivers, this is not the 1990s
Also, another comment below:
> @@ -310,6 +357,8 @@ static int imx93_clocks_probe(struct platform_device *pdev)
>
> for (i = 0; i < ARRAY_SIZE(root_array); i++) {
> root = &root_array[i];
> + if (unlikely(imx_clk_bypass_check(root->name)))
> + continue;
Only ever use likely/unlikely if you can measure the difference. Here
on a probe function, you can not, this is not needed at all, the
compiler and CPU will do a better job over time than you can guess at
this.
But as this change isn't needed, this shouldn't be an issue either.
thanks,
greg k-h
+ DT maintainers.
> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> parameter
>
> On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > With the clk names specified in clk_bypassed module parameter, give
> > user an option to bypass the clk from managing them by Linux kernel.
>
> As I said on another email, no, please do not add new module parameters
> for drivers, this is not the 1990s
ok, but this is for boot, so only DT could be considered.
But DT is to describe hardware, here I just wanna give user
an option to bypass some clocks. Is it ok to add a DT property
saying "fsl,imx93-bypass-clks = <IMX93_CLK_X>, <IMX93_CLK_Y>" ?
Thanks,
Peng.
>
> Also, another comment below:
>
> > @@ -310,6 +357,8 @@ static int imx93_clocks_probe(struct
> > platform_device *pdev)
> >
> > for (i = 0; i < ARRAY_SIZE(root_array); i++) {
> > root = &root_array[i];
> > + if (unlikely(imx_clk_bypass_check(root->name)))
> > + continue;
>
> Only ever use likely/unlikely if you can measure the difference. Here on a
> probe function, you can not, this is not needed at all, the compiler and CPU
> will do a better job over time than you can guess at this.
>
> But as this change isn't needed, this shouldn't be an issue either.
>
> thanks,
>
> greg k-h
On 04/05/2023 11:17, Peng Fan wrote:
> + DT maintainers.
>
>> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
>> parameter
>>
>> On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
>>> From: Peng Fan <[email protected]>
>>>
>>> With the clk names specified in clk_bypassed module parameter, give
>>> user an option to bypass the clk from managing them by Linux kernel.
>>
>> As I said on another email, no, please do not add new module parameters
>> for drivers, this is not the 1990s
>
> ok, but this is for boot, so only DT could be considered.
>
> But DT is to describe hardware, here I just wanna give user
> an option to bypass some clocks. Is it ok to add a DT property
> saying "fsl,imx93-bypass-clks = <IMX93_CLK_X>, <IMX93_CLK_Y>" ?
>
I don't know what it is to "bypass some clocks". This does not look like
parameter for system at all.
Best regards,
Krzysztof
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: 2023年5月4日 17:31
> To: Peng Fan <[email protected]>; Greg KH <[email protected]>; Peng Fan
> (OSS) <[email protected]>; Rob Herring <[email protected]>;
> [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> parameter
>
> On 04/05/2023 11:17, Peng Fan wrote:
> > + DT maintainers.
> >
> >> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> >> parameter
> >>
> >> On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
> >>> From: Peng Fan <[email protected]>
> >>>
> >>> With the clk names specified in clk_bypassed module parameter, give
> >>> user an option to bypass the clk from managing them by Linux kernel.
> >>
> >> As I said on another email, no, please do not add new module
> >> parameters for drivers, this is not the 1990s
> >
> > ok, but this is for boot, so only DT could be considered.
> >
> > But DT is to describe hardware, here I just wanna give user an option
> > to bypass some clocks. Is it ok to add a DT property saying
> > "fsl,imx93-bypass-clks = <IMX93_CLK_X>, <IMX93_CLK_Y>" ?
> >
>
> I don't know what it is to "bypass some clocks". This does not look like
> parameter for system at all.
Currently the linux clk driver registers all the clocks and manage them.
But when M33 is running, M33 may not wanna linux to manage
some clocks M33 is using. So I wanna linux not register those clocks
that M33 will use.
Thanks,
Peng.
>
> Best regards,
> Krzysztof
Hello Peng,
On 04.05.23 11:34, Peng Fan wrote:
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[email protected]>
>> Sent: 2023年5月4日 17:31
>> To: Peng Fan <[email protected]>; Greg KH <[email protected]>; Peng Fan
>> (OSS) <[email protected]>; Rob Herring <[email protected]>;
>> [email protected]
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; dl-linux-imx <[email protected]>; linux-
>> [email protected]; [email protected]; linux-
>> [email protected]
>> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
>> parameter
>>
>> On 04/05/2023 11:17, Peng Fan wrote:
>>> + DT maintainers.
>>>
>>>> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
>>>> parameter
>>>>
>>>> On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
>>>>> From: Peng Fan <[email protected]>
>>>>>
>>>>> With the clk names specified in clk_bypassed module parameter, give
>>>>> user an option to bypass the clk from managing them by Linux kernel.
>>>>
>>>> As I said on another email, no, please do not add new module
>>>> parameters for drivers, this is not the 1990s
>>>
>>> ok, but this is for boot, so only DT could be considered.
>>>
>>> But DT is to describe hardware, here I just wanna give user an option
>>> to bypass some clocks. Is it ok to add a DT property saying
>>> "fsl,imx93-bypass-clks = <IMX93_CLK_X>, <IMX93_CLK_Y>" ?
>>>
>>
>> I don't know what it is to "bypass some clocks". This does not look like
>> parameter for system at all.
>
> Currently the linux clk driver registers all the clocks and manage them.
> But when M33 is running, M33 may not wanna linux to manage
> some clocks M33 is using. So I wanna linux not register those clocks
> that M33 will use.
Describe the M33 in the DT, assign it the clocks it depends on and have
a driver bind to it that just claims the clocks in question to ensure
nothing else messes with it?
Cheers,
Ahmad
>
> Thanks,
> Peng.
>
>>
>> Best regards,
>> Krzysztof
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 04/05/2023 11:34, Peng Fan wrote:
>
>
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[email protected]>
>> Sent: 2023年5月4日 17:31
>> To: Peng Fan <[email protected]>; Greg KH <[email protected]>; Peng Fan
>> (OSS) <[email protected]>; Rob Herring <[email protected]>;
>> [email protected]
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; dl-linux-imx <[email protected]>; linux-
>> [email protected]; [email protected]; linux-
>> [email protected]
>> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
>> parameter
>>
>> On 04/05/2023 11:17, Peng Fan wrote:
>>> + DT maintainers.
>>>
>>>> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
>>>> parameter
>>>>
>>>> On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
>>>>> From: Peng Fan <[email protected]>
>>>>>
>>>>> With the clk names specified in clk_bypassed module parameter, give
>>>>> user an option to bypass the clk from managing them by Linux kernel.
>>>>
>>>> As I said on another email, no, please do not add new module
>>>> parameters for drivers, this is not the 1990s
>>>
>>> ok, but this is for boot, so only DT could be considered.
>>>
>>> But DT is to describe hardware, here I just wanna give user an option
>>> to bypass some clocks. Is it ok to add a DT property saying
>>> "fsl,imx93-bypass-clks = <IMX93_CLK_X>, <IMX93_CLK_Y>" ?
>>>
>>
>> I don't know what it is to "bypass some clocks". This does not look like
>> parameter for system at all.
>
> Currently the linux clk driver registers all the clocks and manage them.
> But when M33 is running, M33 may not wanna linux to manage
> some clocks M33 is using. So I wanna linux not register those clocks
> that M33 will use.
Ask the one who designed such system that second processor pokes parts
of first processor... I assume if the clock controller is enabled in DTS
for Linux, then the Linux owns it. Otherwise how do you expect to handle
concurrent access to same registers from different processors?
And how are you going to decide which clocks should be managed by M33?
One firmware could want to play with one clock, other with everything...
Module parameter is not the way to deal with it.
Probably Ahmad's idea is the only one reasonable in your case, if you do
not have hypervisor.
Best regards,
Krzysztof
> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> parameter
>
> Hello Peng,
>
> On 04.05.23 11:34, Peng Fan wrote:
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[email protected]>
> >> Sent: 2023??5??4?? 17:31
> >> To: Peng Fan <[email protected]>; Greg KH <[email protected]>; Peng
> Fan
> >> (OSS) <[email protected]>; Rob Herring <[email protected]>;
> >> [email protected]
> >> Cc: [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> [email protected];
> >> [email protected]; dl-linux-imx <[email protected]>; linux-
> >> [email protected]; [email protected]; linux-
> >> [email protected]
> >> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> >> parameter
> >>
> >> On 04/05/2023 11:17, Peng Fan wrote:
> >>> + DT maintainers.
> >>>
> >>>> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> >>>> parameter
> >>>>
> >>>> On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
> >>>>> From: Peng Fan <[email protected]>
> >>>>>
> >>>>> With the clk names specified in clk_bypassed module parameter,
> >>>>> give user an option to bypass the clk from managing them by Linux
> kernel.
> >>>>
> >>>> As I said on another email, no, please do not add new module
> >>>> parameters for drivers, this is not the 1990s
> >>>
> >>> ok, but this is for boot, so only DT could be considered.
> >>>
> >>> But DT is to describe hardware, here I just wanna give user an
> >>> option to bypass some clocks. Is it ok to add a DT property saying
> >>> "fsl,imx93-bypass-clks = <IMX93_CLK_X>, <IMX93_CLK_Y>" ?
> >>>
> >>
> >> I don't know what it is to "bypass some clocks". This does not look
> >> like parameter for system at all.
> >
> > Currently the linux clk driver registers all the clocks and manage them.
> > But when M33 is running, M33 may not wanna linux to manage some
> clocks
> > M33 is using. So I wanna linux not register those clocks that M33 will
> > use.
>
> Describe the M33 in the DT, assign it the clocks it depends on and have a
> driver bind to it that just claims the clocks in question to ensure nothing else
> messes with it?
Hmm. When M33 is booted by ROM or U-Boot, just claim the clocks
may not help, but it may help if parse the m33 clock from clock driver and
enable them.
M33 clocks may vary based on different usecase (:
Regards,
Peng.
>
> Cheers,
> Ahmad
>
> >
> > Thanks,
> > Peng.
> >
> >>
> >> Best regards,
> >> Krzysztof
> >
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C8fcef3d1c
> 5834cda284d08db4c86ef75%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> 7C0%7C638187914647192693%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
> %7C%7C%7C&sdata=FTOeNVRtLQmt9eASlbKlEyvw6mXydDap4i3vYpH%2FfR
> k%3D&reserved=0 |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: 2023年5月4日 19:01
> To: Peng Fan <[email protected]>; Greg KH <[email protected]>; Peng Fan
> (OSS) <[email protected]>; Rob Herring <[email protected]>;
> [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> parameter
>
> On 04/05/2023 11:34, Peng Fan wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[email protected]>
> >> Sent: 2023年5月4日 17:31
> >> To: Peng Fan <[email protected]>; Greg KH <[email protected]>; Peng
> Fan
> >> (OSS) <[email protected]>; Rob Herring <[email protected]>;
> >> [email protected]
> >> Cc: [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> [email protected];
> >> [email protected]; dl-linux-imx <[email protected]>; linux-
> >> [email protected]; [email protected]; linux-
> >> [email protected]
> >> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> >> parameter
> >>
> >> On 04/05/2023 11:17, Peng Fan wrote:
> >>> + DT maintainers.
> >>>
> >>>> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> >>>> parameter
> >>>>
> >>>> On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
> >>>>> From: Peng Fan <[email protected]>
> >>>>>
> >>>>> With the clk names specified in clk_bypassed module parameter,
> >>>>> give user an option to bypass the clk from managing them by Linux
> kernel.
> >>>>
> >>>> As I said on another email, no, please do not add new module
> >>>> parameters for drivers, this is not the 1990s
> >>>
> >>> ok, but this is for boot, so only DT could be considered.
> >>>
> >>> But DT is to describe hardware, here I just wanna give user an
> >>> option to bypass some clocks. Is it ok to add a DT property saying
> >>> "fsl,imx93-bypass-clks = <IMX93_CLK_X>, <IMX93_CLK_Y>" ?
> >>>
> >>
> >> I don't know what it is to "bypass some clocks". This does not look
> >> like parameter for system at all.
> >
> > Currently the linux clk driver registers all the clocks and manage them.
> > But when M33 is running, M33 may not wanna linux to manage some
> clocks
> > M33 is using. So I wanna linux not register those clocks that M33 will
> > use.
>
> Ask the one who designed such system that second processor pokes parts
> of first processor... I assume if the clock controller is enabled in DTS for
> Linux, then the Linux owns it. Otherwise how do you expect to handle
> concurrent access to same registers from different processors?
Each clock has a register, we suppose M33 SW and Linux SW not concurrent
access to same register.
>
> And how are you going to decide which clocks should be managed by M33?
> One firmware could want to play with one clock, other with everything...
> Module parameter is not the way to deal with it.
Actually I have no good idea.
>
> Probably Ahmad's idea is the only one reasonable in your case, if you do not
> have hypervisor.
No hypervisor here. Anyway let me think about more.
Thanks,
Peng.
>
>
> Best regards,
> Krzysztof
> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> parameter
>
> On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > With the clk names specified in clk_bypassed module parameter, give
> > user an option to bypass the clk from managing them by Linux kernel.
>
> As I said on another email, no, please do not add new module parameters
> for drivers, this is not the 1990s
After a search of the list,
https://lore.kernel.org/all/?q=module_param
I still see many drivers are adding module_param.
Is this is strict ban that new platform driver should not add
module_param?
Thanks,
Peng.
>
> Also, another comment below:
>
> > @@ -310,6 +357,8 @@ static int imx93_clocks_probe(struct
> > platform_device *pdev)
> >
> > for (i = 0; i < ARRAY_SIZE(root_array); i++) {
> > root = &root_array[i];
> > + if (unlikely(imx_clk_bypass_check(root->name)))
> > + continue;
>
> Only ever use likely/unlikely if you can measure the difference. Here on a
> probe function, you can not, this is not needed at all, the compiler and CPU
> will do a better job over time than you can guess at this.
>
> But as this change isn't needed, this shouldn't be an issue either.
>
> thanks,
>
> greg k-h
On Wed, May 10, 2023 at 07:49:20AM +0000, Peng Fan wrote:
>
> > Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> > parameter
> >
> > On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <[email protected]>
> > >
> > > With the clk names specified in clk_bypassed module parameter, give
> > > user an option to bypass the clk from managing them by Linux kernel.
> >
> > As I said on another email, no, please do not add new module parameters
> > for drivers, this is not the 1990s
>
> After a search of the list,
> https://lore.kernel.org/all/?q=module_param
>
> I still see many drivers are adding module_param.
And they should not be doing so as it is almost always not a good idea
(note, some subsystems, like sound, do require it, as that's the api
they use, so this is not a blanket statement.)
> Is this is strict ban that new platform driver should not add
> module_param?
You need to really really really justify, and document in the changelog
text, why all of the other methods of configuring a platform driver will
not work in order to have it considered.
thanks,
greg k-h
On 5/10/2023 5:13 PM, Greg KH wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
>
>
> On Wed, May 10, 2023 at 07:49:20AM +0000, Peng Fan wrote:
>>
>>> Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
>>> parameter
>>>
>>> On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
>>>> From: Peng Fan <[email protected]>
>>>>
>>>> With the clk names specified in clk_bypassed module parameter, give
>>>> user an option to bypass the clk from managing them by Linux kernel.
>>>
>>> As I said on another email, no, please do not add new module parameters
>>> for drivers, this is not the 1990s
>>
>> After a search of the list,
>> https://lore.kernel.org/all/?q=module_param
>>
>> I still see many drivers are adding module_param.
>
> And they should not be doing so as it is almost always not a good idea
> (note, some subsystems, like sound, do require it, as that's the api
> they use, so this is not a blanket statement.)
>
>> Is this is strict ban that new platform driver should not add
>> module_param?
>
> You need to really really really justify, and document in the changelog
> text, why all of the other methods of configuring a platform driver will
> not work in order to have it considered.
I just wanna use the module parateter to give user a choice to choose
to bypass some clocks. There are 100+ clocks in the driver. Different
user may wanna different configuration. With device tree, it is
not flexible.Such as user A may wanna bypass clock X, Y; user B may
wanna bypass clock Z.
With module parameter, I could easily set it in bootargs.
But anyway if this is not preferred, I need to find other way.
Thanks,
Peng.
>
> thanks,
>
> greg k-h
On Thu, May 11, 2023 at 04:54:23PM +0800, Peng Fan wrote:
>
>
> On 5/10/2023 5:13 PM, Greg KH wrote:
> > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> >
> >
> > On Wed, May 10, 2023 at 07:49:20AM +0000, Peng Fan wrote:
> > >
> > > > Subject: Re: [PATCH] clk: imx: imx93: introduce clk_bypassed module
> > > > parameter
> > > >
> > > > On Thu, May 04, 2023 at 04:55:06PM +0800, Peng Fan (OSS) wrote:
> > > > > From: Peng Fan <[email protected]>
> > > > >
> > > > > With the clk names specified in clk_bypassed module parameter, give
> > > > > user an option to bypass the clk from managing them by Linux kernel.
> > > >
> > > > As I said on another email, no, please do not add new module parameters
> > > > for drivers, this is not the 1990s
> > >
> > > After a search of the list,
> > > https://lore.kernel.org/all/?q=module_param
> > >
> > > I still see many drivers are adding module_param.
> >
> > And they should not be doing so as it is almost always not a good idea
> > (note, some subsystems, like sound, do require it, as that's the api
> > they use, so this is not a blanket statement.)
> >
> > > Is this is strict ban that new platform driver should not add
> > > module_param?
> >
> > You need to really really really justify, and document in the changelog
> > text, why all of the other methods of configuring a platform driver will
> > not work in order to have it considered.
>
> I just wanna use the module parateter to give user a choice to choose
> to bypass some clocks.
That is not what a module paramter is for, a "user" does not use them,
that would be required to be set at boot time.
> There are 100+ clocks in the driver. Different user
> may wanna different configuration. With device tree, it is
> not flexible.Such as user A may wanna bypass clock X, Y; user B may
> wanna bypass clock Z.
Device tree is supposed to be flexible to handle this. If not, please
rework the way your driver handles the device tree information about the
clocks.
Again, module parameters is not the way to handle this issue, sorry.
Think about what would happen if all drivers were to attempt to have a
module parameter like this. That would be unmaintainable.
> With module parameter, I could easily set it in bootargs.
Users do not set bootargs :)
> But anyway if this is not preferred, I need to find other way.
Use device tree please, that is what it is there for.
thanks,
greg k-h