Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1380383 > unrolled thread

Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Stephen Boyd <sboyd@codeaurora.org> - 2016-04-16 01:50 +0200
    Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Jose Abreu <Jose.Abreu@synopsys.com> - 2016-04-18 12:40 +0200
      Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-04-18 13:50 +0200
        Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Jose Abreu <Jose.Abreu@synopsys.com> - 2016-04-19 11:20 +0200
          Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Stephen Boyd <sboyd@codeaurora.org> - 2016-04-20 04:00 +0200
            Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Jose Abreu <Jose.Abreu@synopsys.com> - 2016-04-20 11:50 +0200
              Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Alexey Brodkin <Alexey.Brodkin@synopsys.com> - 2016-04-20 18:20 +0200
                Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Jose Abreu <Jose.Abreu@synopsys.com> - 2016-04-21 12:00 +0200
                  Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Alexey Brodkin <Alexey.Brodkin@synopsys.com> - 2016-04-21 14:20 +0200
                    Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Jose Abreu <Jose.Abreu@synopsys.com> - 2016-04-21 15:20 +0200
                      Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Alexey Brodkin <Alexey.Brodkin@synopsys.com> - 2016-04-21 16:20 +0200
                    Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-04-22 08:00 +0200

#1380383 — Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver

On 04/11, Jose Abreu wrote:
> new file mode 100644
> index 0000000..3ba4e2f
> --- /dev/null
> +++ b/drivers/clk/axs10x/i2s_pll_clock.c
> @@ -0,0 +1,217 @@
> +
> +static int i2s_pll_clk_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	const char *clk_name;
> +	struct clk *clk;
> +	struct i2s_pll_clk *pll_clk;
> +	struct clk_init_data init;
> +	struct resource *mem;
> +
> +	if (!node)
> +		return -ENODEV;

Does this ever happen? Looks like dead code.

> +
> +	pll_clk = devm_kzalloc(dev, sizeof(*pll_clk), GFP_KERNEL);
> +	if (!pll_clk)
> +		return -ENOMEM;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pll_clk->base = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(pll_clk->base))
> +		return PTR_ERR(pll_clk->base);
> +
> +	clk_name = node->name;
> +	init.name = clk_name;
> +	init.ops = &i2s_pll_ops;
> +	init.num_parents = 0;
> +	pll_clk->hw.init = &init;
> +
> +	clk = clk_register(NULL, &pll_clk->hw);

Pass dev as first argument. Also use devm_clk_register() instead.

> +	if (IS_ERR(clk)) {
> +		dev_err(dev, "failed to register %s div clock (%ld)\n",
> +				clk_name, PTR_ERR(clk));
> +		return PTR_ERR(clk);
> +	}
> +
> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M) {

Please don't readl directly from addresses. I think I mentioned
that before and didn't get back to you when you replied asking
for other solutions. I still think a proper DT is in order
instead of doing this check for ref_clk.

> +		pll_clk->ref_clk = 27000000;
> +		pll_clk->pll_cfg = i2s_pll_cfg_27m;
> +	} else {
> +		pll_clk->ref_clk = 28224000;
> +		pll_clk->pll_cfg = i2s_pll_cfg_28m;
> +	}

We should do this before registering the clk with the framework.

> +
> +	return of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +}
> +
> +static int i2s_pll_clk_remove(struct platform_device *pdev)
> +{
> +	of_clk_del_provider(pdev->dev.of_node);
> +	return 0;
> +}
> +
> +static const struct of_device_id i2s_pll_clk_id[] = {
> +	{ .compatible = "snps,i2s-pll-clock", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, i2s_pll_clk_id);
> +
> +static struct platform_driver i2s_pll_clk_driver = {
> +	.driver = {
> +		.name = "i2s-pll-clock",
> +		.of_match_table = of_match_ptr(i2s_pll_clk_id),

You can drop of_match_ptr(), it doesn't have much use besides
introducing compilation warnings.

> +	},
> +	.probe = i2s_pll_clk_probe,
> +	.remove = i2s_pll_clk_remove,
> +};
> +module_platform_driver(i2s_pll_clk_driver);
> 

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

[toc] | [next] | [standalone]

#1381470

Hi Stephen,


On 16-04-2016 00:46, Stephen Boyd wrote:
> On 04/11, Jose Abreu wrote:
>> new file mode 100644
>> index 0000000..3ba4e2f
>> --- /dev/null
>> +++ b/drivers/clk/axs10x/i2s_pll_clock.c
>> @@ -0,0 +1,217 @@
>> +
>> +static int i2s_pll_clk_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *node = dev->of_node;
>> +	const char *clk_name;
>> +	struct clk *clk;
>> +	struct i2s_pll_clk *pll_clk;
>> +	struct clk_init_data init;
>> +	struct resource *mem;
>> +
>> +	if (!node)
>> +		return -ENODEV;
> Does this ever happen? Looks like dead code.

Will remove.

>> +
>> +	pll_clk = devm_kzalloc(dev, sizeof(*pll_clk), GFP_KERNEL);
>> +	if (!pll_clk)
>> +		return -ENOMEM;
>> +
>> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	pll_clk->base = devm_ioremap_resource(dev, mem);
>> +	if (IS_ERR(pll_clk->base))
>> +		return PTR_ERR(pll_clk->base);
>> +
>> +	clk_name = node->name;
>> +	init.name = clk_name;
>> +	init.ops = &i2s_pll_ops;
>> +	init.num_parents = 0;
>> +	pll_clk->hw.init = &init;
>> +
>> +	clk = clk_register(NULL, &pll_clk->hw);
> Pass dev as first argument. Also use devm_clk_register() instead.

Ok.

>> +	if (IS_ERR(clk)) {
>> +		dev_err(dev, "failed to register %s div clock (%ld)\n",
>> +				clk_name, PTR_ERR(clk));
>> +		return PTR_ERR(clk);
>> +	}
>> +
>> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M) {
> Please don't readl directly from addresses. I think I mentioned
> that before and didn't get back to you when you replied asking
> for other solutions. I still think a proper DT is in order
> instead of doing this check for ref_clk.

I think that the DT approach would be better but I also think that using two DT
files with only one change between them is not viable. I can see some alternatives:
    1) Pass the region of FPGA version in reg field of DT so that writel is not
directly used;
    2) Create a dummy parent clock driver that reads from FPGA version register
and returns the rate;
    3) Last resort: Use two DT files for each FPGA version.

@Vineet, @Alexey: Can you give some suggestions?

Some background:
We are expecting a new firmware release for the AXS board that will change the
reference clock value of the I2S PLL from 27MHz to 28.224MHz. Due to this change
the dividers of this PLL will change. Right now I am directly reading from the
FPGA version register but Stephen suggested to use a DT approach so that this
rate is declared as parent clock. This would be a good solution but would
require the usage of two different DT files (one for the current firmware and
another for the new firmware), which I think is not ideal. What is your opinion?
Some other solutions are listed above.

>> +		pll_clk->ref_clk = 27000000;
>> +		pll_clk->pll_cfg = i2s_pll_cfg_27m;
>> +	} else {
>> +		pll_clk->ref_clk = 28224000;
>> +		pll_clk->pll_cfg = i2s_pll_cfg_28m;
>> +	}
> We should do this before registering the clk with the framework.

Ok.

>> +
>> +	return of_clk_add_provider(node, of_clk_src_simple_get, clk);
>> +}
>> +
>> +static int i2s_pll_clk_remove(struct platform_device *pdev)
>> +{
>> +	of_clk_del_provider(pdev->dev.of_node);
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id i2s_pll_clk_id[] = {
>> +	{ .compatible = "snps,i2s-pll-clock", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, i2s_pll_clk_id);
>> +
>> +static struct platform_driver i2s_pll_clk_driver = {
>> +	.driver = {
>> +		.name = "i2s-pll-clock",
>> +		.of_match_table = of_match_ptr(i2s_pll_clk_id),
> You can drop of_match_ptr(), it doesn't have much use besides
> introducing compilation warnings.

Ok.

>> +	},
>> +	.probe = i2s_pll_clk_probe,
>> +	.remove = i2s_pll_clk_remove,
>> +};
>> +module_platform_driver(i2s_pll_clk_driver);
>>

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1381630

On Monday 18 April 2016 04:00 PM, Jose Abreu wrote:
>>> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M) {
>> > Please don't readl directly from addresses. I think I mentioned
>> > that before and didn't get back to you when you replied asking
>> > for other solutions. I still think a proper DT is in order
>> > instead of doing this check for ref_clk.
> I think that the DT approach would be better but I also think that using two DT
> files with only one change between them is not viable. I can see some alternatives:
>     1) Pass the region of FPGA version in reg field of DT so that writel is not
> directly used;
>     2) Create a dummy parent clock driver that reads from FPGA version register
> and returns the rate;
>     3) Last resort: Use two DT files for each FPGA version.
> 
> @Vineet, @Alexey: Can you give some suggestions?
> 
> Some background:
> We are expecting a new firmware release for the AXS board that will change the
> reference clock value of the I2S PLL from 27MHz to 28.224MHz. Due to this change
> the dividers of this PLL will change. Right now I am directly reading from the
> FPGA version register but Stephen suggested to use a DT approach so that this
> rate is declared as parent clock. This would be a good solution but would
> require the usage of two different DT files (one for the current firmware and
> another for the new firmware), which I think is not ideal. What is your opinion?
> Some other solutions are listed above.

Consider this my ignorance of clk drivers, what exactly is the problem with that
readl() for FPGA ver. Having 2 versions of DT is annoyance for sure, but the
bigger headache is that it still won't help cases of users mixing and matching
boards and DT. IMO this runtime check is pretty nice and will support both types
of boards with exact same code/DT !

FWIW, both solutions #1 and #3 seem to imply a different DT - no ?

And I really don't see how #2 makes things more elegant/abstracted w.r.t clk
framework ?

So I prefer what you had before.
-Vineet

[toc] | [prev] | [next] | [standalone]

#1382309

Hi Vineet,


On 18-04-2016 12:49, Vineet Gupta wrote:
> On Monday 18 April 2016 04:00 PM, Jose Abreu wrote:
>>>> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M) {
>>>> Please don't readl directly from addresses. I think I mentioned
>>>> that before and didn't get back to you when you replied asking
>>>> for other solutions. I still think a proper DT is in order
>>>> instead of doing this check for ref_clk.
>> I think that the DT approach would be better but I also think that using two DT
>> files with only one change between them is not viable. I can see some alternatives:
>>     1) Pass the region of FPGA version in reg field of DT so that writel is not
>> directly used;
>>     2) Create a dummy parent clock driver that reads from FPGA version register
>> and returns the rate;
>>     3) Last resort: Use two DT files for each FPGA version.
>>
>> @Vineet, @Alexey: Can you give some suggestions?
>>
>> Some background:
>> We are expecting a new firmware release for the AXS board that will change the
>> reference clock value of the I2S PLL from 27MHz to 28.224MHz. Due to this change
>> the dividers of this PLL will change. Right now I am directly reading from the
>> FPGA version register but Stephen suggested to use a DT approach so that this
>> rate is declared as parent clock. This would be a good solution but would
>> require the usage of two different DT files (one for the current firmware and
>> another for the new firmware), which I think is not ideal. What is your opinion?
>> Some other solutions are listed above.
> Consider this my ignorance of clk drivers, what exactly is the problem with that
> readl() for FPGA ver. Having 2 versions of DT is annoyance for sure, but the
> bigger headache is that it still won't help cases of users mixing and matching
> boards and DT. IMO this runtime check is pretty nice and will support both types
> of boards with exact same code/DT !
>
> FWIW, both solutions #1 and #3 seem to imply a different DT - no ?

Solution 1 only requires that the FPGA version register is declared in the DT,
something like this:
    i2s_clock@100a0 {
        compatible = "snps,axs10x-i2s-pll-clock";
        reg = <0x100a0 0x10 0x11230 0x04>;
        #clock-cells = <0>;
    };

And then the region is io-remapped. This solution would discard the direct readl
from the address and would still be compatible with the different firmwares
using the same DT.

Solution 3 is the alternative that Stephen suggested which requires two
different DT's.

>
> And I really don't see how #2 makes things more elegant/abstracted w.r.t clk
> framework ?

Yes, solution 2 is more of a workaround and is not the best by far.

>
> So I prefer what you had before.
> -Vineet

@Stephen: can you give some input so that I can submit a v6?

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1382992

On 04/19, Jose Abreu wrote:
> 
> @Stephen: can you give some input so that I can submit a v6?
> 

I don't prefer putting the second register in the same DT node,
but that's really up to the DT reviewers to approve such a
design. The current binding has been acked by Rob right?

Assuming the new binding is acked/reviewed then that solution is
fine.

Otherwise, I still prefer two DTS files for the two different FPGA
versions. At the least, please use ioremap for any pointers that
you readl/writel here.

Beyond that, we should have a fixed rate source clk somewhere in
the software view of the clk tree, because that reflects reality.
Hardcoding the parent rate in the structure works, but doesn't
properly express the clk tree.

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

[toc] | [prev] | [next] | [standalone]

#1383224

Hi Stephen,


On 20-04-2016 02:54, Stephen Boyd wrote:
> On 04/19, Jose Abreu wrote:
>> @Stephen: can you give some input so that I can submit a v6?
>>
> I don't prefer putting the second register in the same DT node,
> but that's really up to the DT reviewers to approve such a
> design. The current binding has been acked by Rob right?

Yes.

> Assuming the new binding is acked/reviewed then that solution is
> fine.

Ok, will then use the DT to pass the FPGA version register.

> Otherwise, I still prefer two DTS files for the two different FPGA
> versions. At the least, please use ioremap for any pointers that
> you readl/writel here.
>
> Beyond that, we should have a fixed rate source clk somewhere in
> the software view of the clk tree, because that reflects reality.
> Hardcoding the parent rate in the structure works, but doesn't
> properly express the clk tree.
>

Can I use a property in the DT to pass this reference clock? something like this:
    snps,parent-freq = <0xFBED9 27000000>, <0x0 28224000>; /* Tuple
<fpga-version reference-clock-freq>, fpga-version = 0 is default */

Or use a parent clock? like:
    clk {
        compatible = "fixed-clock";
        clock-frequency = <27000000>;
        #clock-cells = <0>;
        snps,fpga-version = <0xFBED9>;
    }

It is important to distinguish between the different versions automatically, is
any of these solutions ok?

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1383547

Hi Jose, Stephen,

On Wed, 2016-04-20 at 10:47 +-0100, Jose Abreu wrote:
+AD4- Hi Stephen,
+AD4- 
+AD4- 
+AD4- On 20-04-2016 02:54, Stephen Boyd wrote:
+AD4- +AD4- 
+AD4- +AD4- On 04/19, Jose Abreu wrote:
+AD4- +AD4- +AD4- 
+AD4- +AD4- +AD4- +AEA-Stephen: can you give some input so that I can submit a v6?
+AD4- +AD4- +AD4- 
+AD4- +AD4- I don't prefer putting the second register in the same DT node,
+AD4- +AD4- but that's really up to the DT reviewers to approve such a
+AD4- +AD4- design. The current binding has been acked by Rob right?
+AD4- Yes.
+AD4- 
+AD4- +AD4- 
+AD4- +AD4- Assuming the new binding is acked/reviewed then that solution is
+AD4- +AD4- fine.
+AD4- Ok, will then use the DT to pass the FPGA version register.

We won't need to know FPGA version at all I think.
Read my comment below.

+AD4- +AD4- 
+AD4- +AD4- Otherwise, I still prefer two DTS files for the two different FPGA
+AD4- +AD4- versions. At the least, please use ioremap for any pointers that
+AD4- +AD4- you readl/writel here.
+AD4- +AD4- 
+AD4- +AD4- Beyond that, we should have a fixed rate source clk somewhere in
+AD4- +AD4- the software view of the clk tree, because that reflects reality.
+AD4- +AD4- Hardcoding the parent rate in the structure works, but doesn't
+AD4- +AD4- properly express the clk tree.
+AD4- +AD4- 
+AD4- Can I use a property in the DT to pass this reference clock? something like this:
+AD4- +AKAAoACgAKA-snps,parent-freq +AD0- +ADw-0xFBED9 27000000+AD4-, +ADw-0x0 28224000+AD4AOw- /+ACo- Tuple
+AD4- +ADw-fpga-version reference-clock-freq+AD4-, fpga-version +AD0- 0 is default +ACo-/
+AD4- 
+AD4- Or use a parent clock? like:
+AD4- +AKAAoACgAKA-clk +AHs-
+AD4- +AKAAoACgAKAAoACgAKAAoA-compatible +AD0- +ACI-fixed-clock+ACIAOw-
+AD4- +AKAAoACgAKAAoACgAKAAoA-clock-frequency +AD0- +ADw-27000000+AD4AOw-
+AD4- +AKAAoACgAKAAoACgAKAAoAAj-clock-cells +AD0- +ADw-0+AD4AOw-
+AD4- +AKAAoACgAKAAoACgAKAAoA-snps,fpga-version +AD0- +ADw-0xFBED9+AD4AOw-
+AD4- +AKAAoACgAKAAfQ-
+AD4- 
+AD4- It is important to distinguish between the different versions automatically, is
+AD4- any of these solutions ok?

I do like that solution with a master clock but with some fine-tuning
for simplification.

We'll add master clock node for I2S as a fixed clock like that:
-------------------+AD4-8------------------
	i2s+AF8-master+AF8-clock: clk +AHs-
		+ACM-clock-cells +AD0- +ADw-0+AD4AOw-
		compatible +AD0- +ACI-fixed-clock+ACIAOw-
		clock-frequency +AD0- +ADw-27000000+AD4AOw-
	+AH0AOw-
-------------------+AD4-8------------------

Note there's no mention of MB version, just a value of the frequency.
And in the driver itself value of that master clock will be used for
population of +ACI-pll+AF8-clk-+AD4-ref+AF8-clk+ACI- directly.

These are benefits we'll get with that approach:
+AKAAWw-1+AF0- We escape any IOs not related to our clock device (I mean
+AKA- +AKA- +AKAAIg-snps,i2s-pll-clock+ACI-) itself.
+AKAAWw-2+AF0- We'll use whatever reference clock value is given.
+AKA- +AKA- +AKA-I.e. we'll be able to do a fix-up of that reference clock
+AKA- +AKA- +AKA-value early in platform code depending on HW we're running on.
+AKA- +AKA- +AKA-That's what people do here and there.
+AKAAWw-3+AF0- Remember another clock driver for AXS10x board is right around
+AKA- +AKA- +AKA-the corner. I mean the one for ARC PGU which uses exactly the same
+AKA- +AKA- +AKA-master clock. So one fixup as mentioned above will work
+AKA- +AKA- +AKA-at once for 2 clock drivers.

Let me know if above makes sense.

-Alexey

[toc] | [prev] | [next] | [standalone]

#1384028

Hi Alexey,


On 20-04-2016 17:12, Alexey Brodkin wrote:
> Hi Jose, Stephen,
>
> On Wed, 2016-04-20 at 10:47 +0100, Jose Abreu wrote:
>> Hi Stephen,
>>
>>
>> On 20-04-2016 02:54, Stephen Boyd wrote:
>>> On 04/19, Jose Abreu wrote:
>>>> @Stephen: can you give some input so that I can submit a v6?
>>>>
>>> I don't prefer putting the second register in the same DT node,
>>> but that's really up to the DT reviewers to approve such a
>>> design. The current binding has been acked by Rob right?
>> Yes.
>>
>>> Assuming the new binding is acked/reviewed then that solution is
>>> fine.
>> Ok, will then use the DT to pass the FPGA version register.
> We won't need to know FPGA version at all I think.
> Read my comment below.
>
>>> Otherwise, I still prefer two DTS files for the two different FPGA
>>> versions. At the least, please use ioremap for any pointers that
>>> you readl/writel here.
>>>
>>> Beyond that, we should have a fixed rate source clk somewhere in
>>> the software view of the clk tree, because that reflects reality.
>>> Hardcoding the parent rate in the structure works, but doesn't
>>> properly express the clk tree.
>>>
>> Can I use a property in the DT to pass this reference clock? something like this:
>>     snps,parent-freq = <0xFBED9 27000000>, <0x0 28224000>; /* Tuple
>> <fpga-version reference-clock-freq>, fpga-version = 0 is default */
>>
>> Or use a parent clock? like:
>>     clk {
>>         compatible = "fixed-clock";
>>         clock-frequency = <27000000>;
>>         #clock-cells = <0>;
>>         snps,fpga-version = <0xFBED9>;
>>     }
>>
>> It is important to distinguish between the different versions automatically, is
>> any of these solutions ok?
> I do like that solution with a master clock but with some fine-tuning
> for simplification.
>
> We'll add master clock node for I2S as a fixed clock like that:
> ------------------->8------------------
> 	i2s_master_clock: clk {
> 		#clock-cells = <0>;
> 		compatible = "fixed-clock";
> 		clock-frequency = <27000000>;
> 	};
> ------------------->8------------------
>
> Note there's no mention of MB version, just a value of the frequency.
> And in the driver itself value of that master clock will be used for
> population of "pll_clk->ref_clk" directly.
>
> These are benefits we'll get with that approach:
>  [1] We escape any IOs not related to our clock device (I mean
>      "snps,i2s-pll-clock") itself.
>  [2] We'll use whatever reference clock value is given.
>      I.e. we'll be able to do a fix-up of that reference clock
>      value early in platform code depending on HW we're running on.
>      That's what people do here and there.
>  [3] Remember another clock driver for AXS10x board is right around
>      the corner. I mean the one for ARC PGU which uses exactly the same
>      master clock. So one fixup as mentioned above will work
>      at once for 2 clock drivers.
>
> Let me know if above makes sense.

That approach can't be used because the reference clock value will change in the
next firmware release.  The new release will have a reference clock of 28224000
Hz instead of the usual 27000000 Hz, so we need to have a way to distinguish
between them. Because of that we can't have only one master clock unless you
state to users that they have to change the reference clock value when using the
new firmware release. Stephen suggested to use two DT files (one for each
firmware release), but as Vineet said this would be annoying to the user so I am
trying to use another solution so that only one DT file is required.

>
> -Alexey

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1384137

Hi Jose,

On Thu, 2016-04-21 at 10:51 +-0100, Jose Abreu wrote:
+AD4- Hi Alexey,


+AD4- +AD4- +AD4- +AD4- Otherwise, I still prefer two DTS files for the two different FPGA
+AD4- +AD4- +AD4- +AD4- versions. At the least, please use ioremap for any pointers that
+AD4- +AD4- +AD4- +AD4- you readl/writel here.
+AD4- +AD4- +AD4- +AD4- 
+AD4- +AD4- +AD4- +AD4- Beyond that, we should have a fixed rate source clk somewhere in
+AD4- +AD4- +AD4- +AD4- the software view of the clk tree, because that reflects reality.
+AD4- +AD4- +AD4- +AD4- Hardcoding the parent rate in the structure works, but doesn't
+AD4- +AD4- +AD4- +AD4- properly express the clk tree.
+AD4- +AD4- +AD4- +AD4- 
+AD4- +AD4- +AD4- Can I use a property in the DT to pass this reference clock? something like this:
+AD4- +AD4- +AD4- +AKAAoACgAKA-snps,parent-freq +AD0- +ADw-0xFBED9 27000000+AD4-, +ADw-0x0 28224000+AD4AOw- /+ACo- Tuple
+AD4- +AD4- +AD4- +ADw-fpga-version reference-clock-freq+AD4-, fpga-version +AD0- 0 is default +ACo-/
+AD4- +AD4- +AD4- 
+AD4- +AD4- +AD4- Or use a parent clock? like:
+AD4- +AD4- +AD4- +AKAAoACgAKA-clk +AHs-
+AD4- +AD4- +AD4- +AKAAoACgAKAAoACgAKAAoA-compatible +AD0- +ACI-fixed-clock+ACIAOw-
+AD4- +AD4- +AD4- +AKAAoACgAKAAoACgAKAAoA-clock-frequency +AD0- +ADw-27000000+AD4AOw-
+AD4- +AD4- +AD4- +AKAAoACgAKAAoACgAKAAoAAj-clock-cells +AD0- +ADw-0+AD4AOw-
+AD4- +AD4- +AD4- +AKAAoACgAKAAoACgAKAAoA-snps,fpga-version +AD0- +ADw-0xFBED9+AD4AOw-
+AD4- +AD4- +AD4- +AKAAoACgAKAAfQ-
+AD4- +AD4- +AD4- 
+AD4- +AD4- +AD4- It is important to distinguish between the different versions automatically, is
+AD4- +AD4- +AD4- any of these solutions ok?
+AD4- +AD4- I do like that solution with a master clock but with some fine-tuning
+AD4- +AD4- for simplification.
+AD4- +AD4- 
+AD4- +AD4- We'll add master clock node for I2S as a fixed clock like that:
+AD4- +AD4- -------------------+AD4-8------------------
+AD4- +AD4- 	i2s+AF8-master+AF8-clock: clk +AHs-
+AD4- +AD4- 		+ACM-clock-cells +AD0- +ADw-0+AD4AOw-
+AD4- +AD4- 		compatible +AD0- +ACI-fixed-clock+ACIAOw-
+AD4- +AD4- 		clock-frequency +AD0- +ADw-27000000+AD4AOw-
+AD4- +AD4- 	+AH0AOw-
+AD4- +AD4- -------------------+AD4-8------------------
+AD4- +AD4- 
+AD4- +AD4- Note there's no mention of MB version, just a value of the frequency.
+AD4- +AD4- And in the driver itself value of that master clock will be used for
+AD4- +AD4- population of +ACI-pll+AF8-clk-+AD4-ref+AF8-clk+ACI- directly.
+AD4- +AD4- 
+AD4- +AD4- These are benefits we'll get with that approach:
+AD4- +AD4- +AKAAWw-1+AF0- We escape any IOs not related to our clock device (I mean
+AD4- +AD4- +AKAAoACgAKAAoAAi-snps,i2s-pll-clock+ACI-) itself.
+AD4- +AD4- +AKAAWw-2+AF0- We'll use whatever reference clock value is given.
+AD4- +AD4- +AKAAoACgAKAAoA-I.e. we'll be able to do a fix-up of that reference clock
+AD4- +AD4- +AKAAoACgAKAAoA-value early in platform code depending on HW we're running on.
+AD4- +AD4- +AKAAoACgAKAAoA-That's what people do here and there.
+AD4- +AD4- +AKAAWw-3+AF0- Remember another clock driver for AXS10x board is right around
+AD4- +AD4- +AKAAoACgAKAAoA-the corner. I mean the one for ARC PGU which uses exactly the same
+AD4- +AD4- +AKAAoACgAKAAoA-master clock. So one fixup as mentioned above will work
+AD4- +AD4- +AKAAoACgAKAAoA-at once for 2 clock drivers.
+AD4- +AD4- 
+AD4- +AD4- Let me know if above makes sense.
+AD4- That approach can't be used because the reference clock value will change in the
+AD4- next firmware release.+AKAAoA-The new release will have a reference clock of 28224000
+AD4- Hz instead of the usual 27000000 Hz, so we need to have a way to distinguish
+AD4- between them. Because of that we can't have only one master clock unless you
+AD4- state to users that they have to change the reference clock value when using the
+AD4- new firmware release. Stephen suggested to use two DT files (one for each
+AD4- firmware release), but as Vineet said this would be annoying to the user so I am
+AD4- trying to use another solution so that only one DT file is required.

Ok reference clock will change.
But I may guess we'll still be able to determine at least that new
firmware version in run-time, right? If so we'll update a fix-up in
early axs10x platform code so that reference clock will be set as+AKA-28224000 Hz.

And indeed 2 DT files is a no go - we want to run the same one binary
(with built-in .dtb) on all flavors of AXS boards. And fix-up I'm talking about
will actually do transformation of .dtb early on kernel boot process so that will
be a complete equivalent of different DT files.

-Alexey

[toc] | [prev] | [next] | [standalone]

#1384199

Hi Alexey,


On 21-04-2016 13:18, Alexey Brodkin wrote:
> Hi Jose,
>
> On Thu, 2016-04-21 at 10:51 +0100, Jose Abreu wrote:
>> Hi Alexey,
>
>>>>> Otherwise, I still prefer two DTS files for the two different FPGA
>>>>> versions. At the least, please use ioremap for any pointers that
>>>>> you readl/writel here.
>>>>>
>>>>> Beyond that, we should have a fixed rate source clk somewhere in
>>>>> the software view of the clk tree, because that reflects reality.
>>>>> Hardcoding the parent rate in the structure works, but doesn't
>>>>> properly express the clk tree.
>>>>>
>>>> Can I use a property in the DT to pass this reference clock? something like this:
>>>>     snps,parent-freq = <0xFBED9 27000000>, <0x0 28224000>; /* Tuple
>>>> <fpga-version reference-clock-freq>, fpga-version = 0 is default */
>>>>
>>>> Or use a parent clock? like:
>>>>     clk {
>>>>         compatible = "fixed-clock";
>>>>         clock-frequency = <27000000>;
>>>>         #clock-cells = <0>;
>>>>         snps,fpga-version = <0xFBED9>;
>>>>     }
>>>>
>>>> It is important to distinguish between the different versions automatically, is
>>>> any of these solutions ok?
>>> I do like that solution with a master clock but with some fine-tuning
>>> for simplification.
>>>
>>> We'll add master clock node for I2S as a fixed clock like that:
>>> ------------------->8------------------
>>> 	i2s_master_clock: clk {
>>> 		#clock-cells = <0>;
>>> 		compatible = "fixed-clock";
>>> 		clock-frequency = <27000000>;
>>> 	};
>>> ------------------->8------------------
>>>
>>> Note there's no mention of MB version, just a value of the frequency.
>>> And in the driver itself value of that master clock will be used for
>>> population of "pll_clk->ref_clk" directly.
>>>
>>> These are benefits we'll get with that approach:
>>>  [1] We escape any IOs not related to our clock device (I mean
>>>      "snps,i2s-pll-clock") itself.
>>>  [2] We'll use whatever reference clock value is given.
>>>      I.e. we'll be able to do a fix-up of that reference clock
>>>      value early in platform code depending on HW we're running on.
>>>      That's what people do here and there.
>>>  [3] Remember another clock driver for AXS10x board is right around
>>>      the corner. I mean the one for ARC PGU which uses exactly the same
>>>      master clock. So one fixup as mentioned above will work
>>>      at once for 2 clock drivers.
>>>
>>> Let me know if above makes sense.
>> That approach can't be used because the reference clock value will change in the
>> next firmware release.  The new release will have a reference clock of 28224000
>> Hz instead of the usual 27000000 Hz, so we need to have a way to distinguish
>> between them. Because of that we can't have only one master clock unless you
>> state to users that they have to change the reference clock value when using the
>> new firmware release. Stephen suggested to use two DT files (one for each
>> firmware release), but as Vineet said this would be annoying to the user so I am
>> trying to use another solution so that only one DT file is required.
> Ok reference clock will change.
> But I may guess we'll still be able to determine at least that new
> firmware version in run-time, right? If so we'll update a fix-up in
> early axs10x platform code so that reference clock will be set as 28224000 Hz.

Yes, there is a register where the FPGA version date is encoded, we can use that
to check which firmware is used (if date <= old_firmware_date then
clock=27000000; else clock=28224000). If that fix is acceptable it could be a
good solution without having to use custom parameters in the DT (no need to
encode the different clocks and we would only use one master clock) but I am not
sure where and how this can be encoded and I don't know how to change the DT on
runtime. Can you give me some guidelines?

>
> And indeed 2 DT files is a no go - we want to run the same one binary
> (with built-in .dtb) on all flavors of AXS boards. And fix-up I'm talking about
> will actually do transformation of .dtb early on kernel boot process so that will
> be a complete equivalent of different DT files.

And doing modifications on the DT can cause some misdirections to users.
Besides, we would have clock specific functions in init procedures which is
precisely what we are trying to avoid by submitting this driver.

>
> -Alexey

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1384272

Hi Jose,

On Thu, 2016-04-21 at 14:10 +-0100, Jose Abreu wrote:
+AD4- Hi Alexey,
+AD4- 
+AD4- 
+AD4- On 21-04-2016 13:18, Alexey Brodkin wrote:
+AD4- +AD4- 
+AD4- +AD4- Hi Jose,
+AD4- +AD4- 
+AD4- +AD4- On Thu, 2016-04-21 at 10:51 +-0100, Jose Abreu wrote:
+AD4- +AD4- +AD4- 
+AD4- +AD4- +AD4- Hi Alexey,
+AD4- +AD4- +AD4AoA-
+AD4- +AD4- Ok reference clock will change.
+AD4- +AD4- But I may guess we'll still be able to determine at least that new
+AD4- +AD4- firmware version in run-time, right? If so we'll update a fix-up in
+AD4- +AD4- early axs10x platform code so that reference clock will be set as 28224000 Hz.
+AD4- Yes, there is a register where the FPGA version date is encoded, we can use that
+AD4- to check which firmware is used (if date +ADwAPQ- old+AF8-firmware+AF8-date then
+AD4- clock+AD0-27000000+ADs- else clock+AD0-28224000). If that fix is acceptable it could be a
+AD4- good solution without having to use custom parameters in the DT (no need to
+AD4- encode the different clocks and we would only use one master clock) but I am not
+AD4- sure where and how this can be encoded and I don't know how to change the DT on
+AD4- runtime. Can you give me some guidelines?

Take a look here -+AKA-http://git.kernel.org/cgit/linux/kernel/git/vgupta/arc.git/commit/arch/arc/plat-axs10x/axs10x.c?h+AD0-for
-next+ACY-id+AD0-5cd0f5102753a7405548d0c66c11a2a0a05bbf2e

We do something very similar here - we're patching in run-time
core frequency that was specified in .dts.

And in the very same way one will be able to do fix-ups for other
clocks.

Moreover I would propose to think about that fix-up as of completely
separate topic. I.e. in your driver for AXS' I2S clock just use a new
reference +ACI-fixed-clock+ACI- (that you'll add in +ACI-axs10x+AF8-mb.dtsi+ACI- as a part of
your driver submission). And once your driver gets accepted we'll work on
fix-up in axs10x platform.

This way we'll move with smaller steps and hopefully will get things done
sooner.

+AD4- +AD4- And indeed 2 DT files is a no go - we want to run the same one binary
+AD4- +AD4- (with built-in .dtb) on all flavors of AXS boards. And fix-up I'm talking about
+AD4- +AD4- will actually do transformation of .dtb early on kernel boot process so that will
+AD4- +AD4- be a complete equivalent of different DT files.
+AD4- And doing modifications on the DT can cause some misdirections to users.

What do you mean here? What kind of problems do you expect to face?

+AD4- Besides, we would have clock specific functions in init procedures which is
+AD4- precisely what we are trying to avoid by submitting this driver.

You're talking about fixups above here?

-Alexey

[toc] | [prev] | [next] | [standalone]

#1384711

On Thursday 21 April 2016 05:48 PM, Alexey Brodkin wrote:
> Hi Jose,
> 
> On Thu, 2016-04-21 at 10:51 +0100, Jose Abreu wrote:
>> Hi Alexey,
> 
> 
>>>>> Otherwise, I still prefer two DTS files for the two different FPGA
>>>>> versions. At the least, please use ioremap for any pointers that
>>>>> you readl/writel here.
>>>>>
>>>>> Beyond that, we should have a fixed rate source clk somewhere in
>>>>> the software view of the clk tree, because that reflects reality.
>>>>> Hardcoding the parent rate in the structure works, but doesn't
>>>>> properly express the clk tree.
>>>>>
>>>> Can I use a property in the DT to pass this reference clock? something like this:
>>>>     snps,parent-freq = <0xFBED9 27000000>, <0x0 28224000>; /* Tuple
>>>> <fpga-version reference-clock-freq>, fpga-version = 0 is default */
>>>>
>>>> Or use a parent clock? like:
>>>>     clk {
>>>>         compatible = "fixed-clock";
>>>>         clock-frequency = <27000000>;
>>>>         #clock-cells = <0>;
>>>>         snps,fpga-version = <0xFBED9>;
>>>>     }
>>>>
>>>> It is important to distinguish between the different versions automatically, is
>>>> any of these solutions ok?
>>> I do like that solution with a master clock but with some fine-tuning
>>> for simplification.
>>>
>>> We'll add master clock node for I2S as a fixed clock like that:
>>> ------------------->8------------------
>>> 	i2s_master_clock: clk {
>>> 		#clock-cells = <0>;
>>> 		compatible = "fixed-clock";
>>> 		clock-frequency = <27000000>;
>>> 	};
>>> ------------------->8------------------
>>>
>>> Note there's no mention of MB version, just a value of the frequency.
>>> And in the driver itself value of that master clock will be used for
>>> population of "pll_clk->ref_clk" directly.
>>>
>>> These are benefits we'll get with that approach:
>>>  [1] We escape any IOs not related to our clock device (I mean
>>>      "snps,i2s-pll-clock") itself.
>>>  [2] We'll use whatever reference clock value is given.
>>>      I.e. we'll be able to do a fix-up of that reference clock
>>>      value early in platform code depending on HW we're running on.
>>>      That's what people do here and there.
>>>  [3] Remember another clock driver for AXS10x board is right around
>>>      the corner. I mean the one for ARC PGU which uses exactly the same
>>>      master clock. So one fixup as mentioned above will work
>>>      at once for 2 clock drivers.
>>>
>>> Let me know if above makes sense.
>> That approach can't be used because the reference clock value will change in the
>> next firmware release.  The new release will have a reference clock of 28224000
>> Hz instead of the usual 27000000 Hz, so we need to have a way to distinguish
>> between them. Because of that we can't have only one master clock unless you
>> state to users that they have to change the reference clock value when using the
>> new firmware release. Stephen suggested to use two DT files (one for each
>> firmware release), but as Vineet said this would be annoying to the user so I am
>> trying to use another solution so that only one DT file is required.
> 
> Ok reference clock will change.
> But I may guess we'll still be able to determine at least that new
> firmware version in run-time, right? If so we'll update a fix-up in
> early axs10x platform code so that reference clock will be set as 28224000 Hz.


Please no - lets not bolt-in more hacks and instead try do this cleanly if
possible. And from other discusions it seems there might be a way. The readl
approach seems fine to me (with ioremap) if that is what it takes.


> And indeed 2 DT files is a no go - we want to run the same one binary
> (with built-in .dtb) on all flavors of AXS boards.

Right - 2 DT is not acceptable unless we are feeling bored and want more emails
for AXS board support :-)

 And fix-up I'm talking about
> will actually do transformation of .dtb early on kernel boot process so that will
> be a complete equivalent of different DT files.
> 
> -Alexey--
> 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
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web