[PATCH] clk: x86: add "mclk" alias for Baytrail/Cherrytrail

[PATCH] clk: x86: add "mclk" alias for Baytrail/Cherrytrail

[Pierre-Louis Bossart]

Due to timing requirements, TI and Conexant manage the audio
reference clock from their ASoC codec drivers using the "mclk"
string. This patch adds a default alias to "pmc_plt_clk_3" to
avoid Intel-specific tests in those codec drivers and use code
as-is.
"pmc_plt_clk_3" is used exclusively for audio on all known
Baytrail/CherryTrail designs and is e.g. routed on the MCLK
(pin 26) of the MinnowBoardMAX Turbot LSE connector.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---

Notes:
    This patch only applies on top of linux-next or clk-next, the
    previous clk-related patches are not yet merged in the audio
    trees

 drivers/clk/x86/clk-pmc-atom.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
index 2b60577..ba00d27 100644
--- a/drivers/clk/x86/clk-pmc-atom.c
+++ b/drivers/clk/x86/clk-pmc-atom.c
@@ -340,6 +340,8 @@ static int plt_clk_probe(struct platform_device *pdev)
 
 	plt_clk_free_parent_names_loop(parent_names, data->nparents);
 
+	clk_add_alias("mclk", dev_name(&pdev->dev), "pmc_plt_clk_3", NULL);
+
 	platform_set_drvdata(pdev, data);
 	return 0;
 
-- 
2.7.4

Re: [PATCH] clk: x86: add "mclk" alias for Baytrail/Cherrytrail

[Stephen Boyd]

On 02/12, Pierre-Louis Bossart wrote:
> Due to timing requirements, TI and Conexant manage the audio
> reference clock from their ASoC codec drivers using the "mclk"
> string. This patch adds a default alias to "pmc_plt_clk_3" to
> avoid Intel-specific tests in those codec drivers and use code
> as-is.
> "pmc_plt_clk_3" is used exclusively for audio on all known
> Baytrail/CherryTrail designs and is e.g. routed on the MCLK
> (pin 26) of the MinnowBoardMAX Turbot LSE connector.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
> 
> Notes:
>     This patch only applies on top of linux-next or clk-next, the
>     previous clk-related patches are not yet merged in the audio
>     trees
> 
>  drivers/clk/x86/clk-pmc-atom.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
> index 2b60577..ba00d27 100644
> --- a/drivers/clk/x86/clk-pmc-atom.c
> +++ b/drivers/clk/x86/clk-pmc-atom.c
> @@ -340,6 +340,8 @@ static int plt_clk_probe(struct platform_device *pdev)
>  
>  	plt_clk_free_parent_names_loop(parent_names, data->nparents);
>  
> +	clk_add_alias("mclk", dev_name(&pdev->dev), "pmc_plt_clk_3", NULL);
> +

This leaks an alias when the driver is removed. Honestly,
clk_add_alias() doesn't work well because of that problem. Can
you just add another lookup with the pointer you already have
instead of passing a NULL device to do a global lookup?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: x86: add "mclk" alias for Baytrail/Cherrytrail

[Pierre-Louis Bossart]

On 2/14/17 1:04 PM, Stephen Boyd wrote:
> On 02/12, Pierre-Louis Bossart wrote:
>> Due to timing requirements, TI and Conexant manage the audio
>> reference clock from their ASoC codec drivers using the "mclk"
>> string. This patch adds a default alias to "pmc_plt_clk_3" to
>> avoid Intel-specific tests in those codec drivers and use code
>> as-is.
>> "pmc_plt_clk_3" is used exclusively for audio on all known
>> Baytrail/CherryTrail designs and is e.g. routed on the MCLK
>> (pin 26) of the MinnowBoardMAX Turbot LSE connector.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>
>> Notes:
>>     This patch only applies on top of linux-next or clk-next, the
>>     previous clk-related patches are not yet merged in the audio
>>     trees
>>
>>  drivers/clk/x86/clk-pmc-atom.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
>> index 2b60577..ba00d27 100644
>> --- a/drivers/clk/x86/clk-pmc-atom.c
>> +++ b/drivers/clk/x86/clk-pmc-atom.c
>> @@ -340,6 +340,8 @@ static int plt_clk_probe(struct platform_device *pdev)
>>
>>  	plt_clk_free_parent_names_loop(parent_names, data->nparents);
>>
>> +	clk_add_alias("mclk", dev_name(&pdev->dev), "pmc_plt_clk_3", NULL);
>> +
>
> This leaks an alias when the driver is removed. Honestly,
> clk_add_alias() doesn't work well because of that problem. Can
> you just add another lookup with the pointer you already have
> instead of passing a NULL device to do a global lookup?

This is a builtin driver that cannot be configured as a module, is the 
leaked alias problematic?
I don't mind trying something different but I am not familiar enough 
with the framework to understand what you are hinting at. Are you 
suggesting a change on the last parameter such as:

clk_add_alias("mclk", dev_name(&pdev->dev), "pmc_plt_clk_3", &pdev->dev)

Thanks!
-Pierre

Re: [PATCH] clk: x86: add "mclk" alias for Baytrail/Cherrytrail

[Stephen Boyd]

On 02/14, Pierre-Louis Bossart wrote:
> On 2/14/17 1:04 PM, Stephen Boyd wrote:
> >
> >This leaks an alias when the driver is removed. Honestly,
> >clk_add_alias() doesn't work well because of that problem. Can
> >you just add another lookup with the pointer you already have
> >instead of passing a NULL device to do a global lookup?
> 
> This is a builtin driver that cannot be configured as a module, is
> the leaked alias problematic?

Well we don't suppress driver unbinding via sysfs here, unless I
missed something, so the leak could be triggered that way.

> I don't mind trying something different but I am not familiar enough
> with the framework to understand what you are hinting at. Are you
> suggesting a change on the last parameter such as:
> 
> clk_add_alias("mclk", dev_name(&pdev->dev), "pmc_plt_clk_3", &pdev->dev)
> 

I mean:

 lookup = clkdev_hw_create(data->clks[3].hw, "mclk", NULL);

and then freeing that lookup with clkdev_drop in the remove of
the driver. "mclk" is really generic for a connection name
without an associated device id, so you may want to pass some
device as the last argument here, but dev_name(&pdev->dev) seems
odd because that's the clock controller device, not whatever
device would be calling clk_get() with this created lookup in
mind.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: x86: add "mclk" alias for Baytrail/Cherrytrail

[Andy Shevchenko]

On Thu, Feb 16, 2017 at 2:56 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 02/14, Pierre-Louis Bossart wrote:
>> On 2/14/17 1:04 PM, Stephen Boyd wrote:

>> This is a builtin driver that cannot be configured as a module, is
>> the leaked alias problematic?
>
> Well we don't suppress driver unbinding via sysfs here, unless I
> missed something, so the leak could be triggered that way.

It has ->remove() hook, so, definitely it's possible to unbind.

>  lookup = clkdev_hw_create(data->clks[3].hw, "mclk", NULL);
>
> and then freeing that lookup with clkdev_drop in the remove of
> the driver. "mclk" is really generic for a connection name
> without an associated device id, so you may want to pass some
> device as the last argument here, but dev_name(&pdev->dev) seems
> odd because that's the clock controller device, not whatever
> device would be calling clk_get() with this created lookup in
> mind.

Here is the problem with CLK framework since it's (was?) too oriented
to ARM and clock trees.

The best option for x86 is to place it in some platform code, which is
bad idea per se.
For example, GPIO, PWM, etc have static look up tables and glue
drivers (like PCI part of the driver, or I2C, or similar) can fill
that table based on ID.

My personal opinion that the best place is not the clock driver for
that, rather converting existing drivers to take clock name based on
some (platform related) ID.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] clk: x86: add "mclk" alias for Baytrail/Cherrytrail

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]

On Thu, Feb 16, 2017 at 04:05:19AM +0200, Andy Shevchenko wrote:
> On Thu, Feb 16, 2017 at 2:56 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> > and then freeing that lookup with clkdev_drop in the remove of
> > the driver. "mclk" is really generic for a connection name
> > without an associated device id, so you may want to pass some
> > device as the last argument here, but dev_name(&pdev->dev) seems
> > odd because that's the clock controller device, not whatever
> > device would be calling clk_get() with this created lookup in
> > mind.

> Here is the problem with CLK framework since it's (was?) too oriented
> to ARM and clock trees.

That's not really much more of a problem for x86 than it is for ARM,
things got easier for ARM with the introduction of device tree but this
stuff all predates device tree.

> The best option for x86 is to place it in some platform code, which is
> bad idea per se.
> For example, GPIO, PWM, etc have static look up tables and glue
> drivers (like PCI part of the driver, or I2C, or similar) can fill
> that table based on ID.

Platform code seems fine?

> My personal opinion that the best place is not the clock driver for
> that, rather converting existing drivers to take clock name based on
> some (platform related) ID.

That doesn't scale so well, you end up needing the platform code anyway
and it's one more thing to go wrong when configuring the platform code
so you may as well just do something idiomatic with the platform code.
Some systems did historically do things like this, that's where the use
of the device pointer for namespacing came from.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]