[PATCH] i2c-qoriq: modified compatibility for correct prescaler

[PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Valentin Longchamp]

With "fsl-i2c" compatibility the i2c frequency is not set
correctly, because it sets no prescaler. According to the AN2919 from
Freescale and the QorIQ (P2041) documentation, the source clock is 1/2
the platform clock. This implies that a prescaler of 2 must be used.

This changes the compatibility of the qoriq-i2c .dtsi files to pick the
mpc8543, which uses the same driver but sets the correct prescaler.

Signed-off-by: Rainer Boschung <rainer.boschung@keymile.com>
Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
---

 arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi | 4 ++--
 arch/powerpc/boot/dts/fsl/qoriq-i2c-1.dtsi | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
index 5f9bf7d..aa6c366 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
@@ -36,7 +36,7 @@ i2c@118000 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	cell-index = <0>;
-	compatible = "fsl-i2c";
+	compatible = "fsl,mpc8543-i2c", "fsl-i2c";
 	reg = <0x118000 0x100>;
 	interrupts = <38 2 0 0>;
 	dfsrr;
@@ -46,7 +46,7 @@ i2c@118100 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	cell-index = <1>;
-	compatible = "fsl-i2c";
+	compatible = "fsl,mpc8543-i2c", "fsl-i2c";
 	reg = <0x118100 0x100>;
 	interrupts = <38 2 0 0>;
 	dfsrr;
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-i2c-1.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-i2c-1.dtsi
index 7989bf5..b697a3b 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-i2c-1.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-i2c-1.dtsi
@@ -36,7 +36,7 @@ i2c@119000 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	cell-index = <2>;
-	compatible = "fsl-i2c";
+	compatible = "fsl,mpc8543-i2c", "fsl-i2c";
 	reg = <0x119000 0x100>;
 	interrupts = <39 2 0 0>;
 	dfsrr;
@@ -46,7 +46,7 @@ i2c@119100 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	cell-index = <3>;
-	compatible = "fsl-i2c";
+	compatible = "fsl,mpc8543-i2c", "fsl-i2c";
 	reg = <0x119100 0x100>;
 	interrupts = <39 2 0 0>;
 	dfsrr;
-- 
1.8.0.1

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Scott Wood]

On Fri, 2014-10-17 at 11:27 +0200, Valentin Longchamp wrote:
> With "fsl-i2c" compatibility the i2c frequency is not set
> correctly, because it sets no prescaler. According to the AN2919 from
> Freescale and the QorIQ (P2041) documentation, the source clock is 1/2
> the platform clock. This implies that a prescaler of 2 must be used.
> 
> This changes the compatibility of the qoriq-i2c .dtsi files to pick the
> mpc8543, which uses the same driver but sets the correct prescaler.
> 
> Signed-off-by: Rainer Boschung <rainer.boschung@keymile.com>
> Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
> ---
> 
>  arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi | 4 ++--
>  arch/powerpc/boot/dts/fsl/qoriq-i2c-1.dtsi | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
> index 5f9bf7d..aa6c366 100644
> --- a/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
> +++ b/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
> @@ -36,7 +36,7 @@ i2c@118000 {
>  	#address-cells = <1>;
>  	#size-cells = <0>;
>  	cell-index = <0>;
> -	compatible = "fsl-i2c";
> +	compatible = "fsl,mpc8543-i2c", "fsl-i2c";
>  	reg = <0x118000 0x100>;
>  	interrupts = <38 2 0 0>;
>  	dfsrr;
> @@ -46,7 +46,7 @@ i2c@118100 {
>  	#address-cells = <1>;
>  	#size-cells = <0>;
>  	cell-index = <1>;
> -	compatible = "fsl-i2c";
> +	compatible = "fsl,mpc8543-i2c", "fsl-i2c";
>  	reg = <0x118100 0x100>;
>  	interrupts = <38 2 0 0>;
>  	dfsrr;

Are all chips that use this dtsi 100% compatible with mpc8543's i2c, or
just in ways the Linux driver cares about?

What about fsl,mpc8544-i2c, which has additional special handling in the
driver, but is only used in socrates.dts (not mpc8544ds.dts)?

What about pq3-i2c-*.dtsi?

-Scott

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Valentin Longchamp]

On 10/29/2014 12:08 AM, Scott Wood wrote:
> On Fri, 2014-10-17 at 11:27 +0200, Valentin Longchamp wrote:
>> With "fsl-i2c" compatibility the i2c frequency is not set
>> correctly, because it sets no prescaler. According to the AN2919 from
>> Freescale and the QorIQ (P2041) documentation, the source clock is 1/2
>> the platform clock. This implies that a prescaler of 2 must be used.
>>
>> This changes the compatibility of the qoriq-i2c .dtsi files to pick the
>> mpc8543, which uses the same driver but sets the correct prescaler.
>>
>> Signed-off-by: Rainer Boschung <rainer.boschung@keymile.com>
>> Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
>> ---
>>
>>  arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi | 4 ++--
>>  arch/powerpc/boot/dts/fsl/qoriq-i2c-1.dtsi | 4 ++--
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
>> index 5f9bf7d..aa6c366 100644
>> --- a/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
>> +++ b/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
>> @@ -36,7 +36,7 @@ i2c@118000 {
>>  	#address-cells = <1>;
>>  	#size-cells = <0>;
>>  	cell-index = <0>;
>> -	compatible = "fsl-i2c";
>> +	compatible = "fsl,mpc8543-i2c", "fsl-i2c";
>>  	reg = <0x118000 0x100>;
>>  	interrupts = <38 2 0 0>;
>>  	dfsrr;
>> @@ -46,7 +46,7 @@ i2c@118100 {
>>  	#address-cells = <1>;
>>  	#size-cells = <0>;
>>  	cell-index = <1>;
>> -	compatible = "fsl-i2c";
>> +	compatible = "fsl,mpc8543-i2c", "fsl-i2c";
>>  	reg = <0x118100 0x100>;
>>  	interrupts = <38 2 0 0>;
>>  	dfsrr;
> 
> Are all chips that use this dtsi 100% compatible with mpc8543's i2c, or
> just in ways the Linux driver cares about?

I have just looked briefly at the mpc8548 RM (covers mpc8543) and its i2c
controller looks the same as the qoriq's. I cannot however state if they are
100% compatible.

If we wanted to be on the safe side and strict (since we are not sure that the
hardware is 100% compatible), we maybe should add a fsl,qoriq-i2c compatible to
the driver that does the same as mpc8543-i2c.

> 
> What about fsl,mpc8544-i2c, which has additional special handling in the
> driver, but is only used in socrates.dts (not mpc8544ds.dts)?

>From the mpc8544 RM, this controller looks the same as the above 2, except for
the prescaler from the driver which is set to 3. As to why it is only used in
the socrates.dts, I cannot comment about it.

The prescaler is confirmed to be 3 by default by the Table 3 of the AN-219 for
the mpc8544.

> 
> What about pq3-i2c-*.dtsi?
> 

This is also interesting: from the AN-219 Table 4, some pq3 have a 2:1
(mcpc8536/43/45/47/48/67/68/72, plus p2020) prescaler where some don't
(mpc8533/44, where it can be 3:1 -default- or 2:1). However pq3-i2c-*.dtsi
defines no prescaler.

Now if I look at what files include these pq3-i2c-*.dtsi, I see some that are in
the the 2:1 list:
arch/powerpc/boot/dts/fsl/mpc8536si-post.dtsi
arch/powerpc/boot/dts/fsl/mpc8548si-post.dtsi
arch/powerpc/boot/dts/fsl/mpc8568si-post.dtsi
arch/powerpc/boot/dts/fsl/mpc8572si-post.dtsi
arch/powerpc/boot/dts/fsl/p2020si-post.dtsi

I don't have any hardware to do some tests with these, but from my measurements
on our qoriq based system (P2041 SoC) I think that the generated I2C clocks for
the above SoC currently are not correct because of the ignored prescaler.

Valentin

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Scott Wood]

On Wed, 2014-10-29 at 09:59 +0100, Valentin Longchamp wrote:
> On 10/29/2014 12:08 AM, Scott Wood wrote:
> > On Fri, 2014-10-17 at 11:27 +0200, Valentin Longchamp wrote:
> >> With "fsl-i2c" compatibility the i2c frequency is not set
> >> correctly, because it sets no prescaler. According to the AN2919 from
> >> Freescale and the QorIQ (P2041) documentation, the source clock is 1/2
> >> the platform clock. This implies that a prescaler of 2 must be used.
> >>
> >> This changes the compatibility of the qoriq-i2c .dtsi files to pick the
> >> mpc8543, which uses the same driver but sets the correct prescaler.
> >>
> >> Signed-off-by: Rainer Boschung <rainer.boschung@keymile.com>
> >> Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
> >> ---
> >>
> >>  arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi | 4 ++--
> >>  arch/powerpc/boot/dts/fsl/qoriq-i2c-1.dtsi | 4 ++--
> >>  2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
> >> index 5f9bf7d..aa6c366 100644
> >> --- a/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
> >> +++ b/arch/powerpc/boot/dts/fsl/qoriq-i2c-0.dtsi
> >> @@ -36,7 +36,7 @@ i2c@118000 {
> >>  	#address-cells = <1>;
> >>  	#size-cells = <0>;
> >>  	cell-index = <0>;
> >> -	compatible = "fsl-i2c";
> >> +	compatible = "fsl,mpc8543-i2c", "fsl-i2c";
> >>  	reg = <0x118000 0x100>;
> >>  	interrupts = <38 2 0 0>;
> >>  	dfsrr;
> >> @@ -46,7 +46,7 @@ i2c@118100 {
> >>  	#address-cells = <1>;
> >>  	#size-cells = <0>;
> >>  	cell-index = <1>;
> >> -	compatible = "fsl-i2c";
> >> +	compatible = "fsl,mpc8543-i2c", "fsl-i2c";
> >>  	reg = <0x118100 0x100>;
> >>  	interrupts = <38 2 0 0>;
> >>  	dfsrr;
> > 
> > Are all chips that use this dtsi 100% compatible with mpc8543's i2c, or
> > just in ways the Linux driver cares about?
> 
> I have just looked briefly at the mpc8548 RM (covers mpc8543) and its i2c
> controller looks the same as the qoriq's. I cannot however state if they are
> 100% compatible.
> 
> If we wanted to be on the safe side and strict (since we are not sure that the
> hardware is 100% compatible), we maybe should add a fsl,qoriq-i2c compatible to
> the driver that does the same as mpc8543-i2c.

If we're going to change the device tree I'd rather just add a property
to say what the prescaler is.

-Scott

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Wolfram Sang]

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


> If we wanted to be on the safe side and strict (since we are not sure that the
> hardware is 100% compatible), we maybe should add a fsl,qoriq-i2c compatible to
> the driver that does the same as mpc8543-i2c.

Or you leave the driver as is and use both compatibles:

compatible = "fsl,qoriq-i2c", "fsl,mpc8543-i2c", "fsl-i2c";

?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Valentin Longchamp]

On 11/13/2014 01:34 AM, Wolfram Sang wrote:
> 
>> If we wanted to be on the safe side and strict (since we are not sure that the
>> hardware is 100% compatible), we maybe should add a fsl,qoriq-i2c compatible to
>> the driver that does the same as mpc8543-i2c.
> 
> Or you leave the driver as is and use both compatibles:
> 
> compatible = "fsl,qoriq-i2c", "fsl,mpc8543-i2c", "fsl-i2c";
> 
> ?
> 

I like Scott's proposition to add the prescaler in the device tree more. From
the hardware description point of view, it makes more sense: the devices are all
just fsl-i2c, with a different prescaler. I just quote it below as a reminder.

> 
> If we're going to change the device tree I'd rather just add a property
> to say what the prescaler is.

 We would however, leave the boards' device trees that use things like
"fsl,mpc8543-i2c" as is and introduce the prescaler for the others requiring it.


Now the drawback is that the driver would require a change, to parse this
prescaler new prescaler property. Would this be OK from your point of view
Wolfram ? If yes, I will send the patches for it.

Valentin

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Wolfram Sang]

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


> > 
> > If we're going to change the device tree I'd rather just add a property
> > to say what the prescaler is.
> 
>  We would however, leave the boards' device trees that use things like
> "fsl,mpc8543-i2c" as is and introduce the prescaler for the others requiring it.
> 
> 
> Now the drawback is that the driver would require a change, to parse this
> prescaler new prescaler property. Would this be OK from your point of view
> Wolfram ? If yes, I will send the patches for it.

I don't think it is OK. I'd think it can be deduced from the compatible
property. Let's ask the DT experts here. I'll follow their suggestion.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Scott Wood]

On Fri, 2014-11-14 at 09:28 +0100, Wolfram Sang wrote:
> > > 
> > > If we're going to change the device tree I'd rather just add a property
> > > to say what the prescaler is.
> > 
> >  We would however, leave the boards' device trees that use things like
> > "fsl,mpc8543-i2c" as is and introduce the prescaler for the others requiring it.
> > 
> > 
> > Now the drawback is that the driver would require a change, to parse this
> > prescaler new prescaler property. Would this be OK from your point of view
> > Wolfram ? If yes, I will send the patches for it.
> 
> I don't think it is OK.

Why?

>  I'd think it can be deduced from the compatible property.

For almost all existing device trees it cannot be.

If you want something that will work without changing device trees,
you'll need to use SVR to identify the SoC.

-Scott

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Wolfram Sang]

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

On Mon, Nov 17, 2014 at 07:28:03PM -0600, Scott Wood wrote:
> On Fri, 2014-11-14 at 09:28 +0100, Wolfram Sang wrote:
> > > > 
> > > > If we're going to change the device tree I'd rather just add a property
> > > > to say what the prescaler is.
> > > 
> > >  We would however, leave the boards' device trees that use things like
> > > "fsl,mpc8543-i2c" as is and introduce the prescaler for the others requiring it.
> > > 
> > > 
> > > Now the drawback is that the driver would require a change, to parse this
> > > prescaler new prescaler property. Would this be OK from your point of view
> > > Wolfram ? If yes, I will send the patches for it.
> > 
> > I don't think it is OK.
> 
> Why?

Because I thought it could be deduced. Then, a seperate property would
not be OK.

> >  I'd think it can be deduced from the compatible property.
> 
> For almost all existing device trees it cannot be.

Pity :( If we do introduce a new property, it should probably be
"clock-div". Grepping through binding documentation, that seems
accepted. We should ask DT maintainers, too, to be safe.

> If you want something that will work without changing device trees,
> you'll need to use SVR to identify the SoC.

The driver is doing that already, see mpc_i2c_get_sec_cfg_8xxx(). Dunno
if it makes sense to add to it for consistency reasons?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Scott Wood]

On Tue, 2014-11-25 at 19:13 +0100, Wolfram Sang wrote:
> On Mon, Nov 17, 2014 at 07:28:03PM -0600, Scott Wood wrote:
> > On Fri, 2014-11-14 at 09:28 +0100, Wolfram Sang wrote:
> > > > > 
> > > > > If we're going to change the device tree I'd rather just add a property
> > > > > to say what the prescaler is.
> > > > 
> > > >  We would however, leave the boards' device trees that use things like
> > > > "fsl,mpc8543-i2c" as is and introduce the prescaler for the others requiring it.
> > > > 
> > > > 
> > > > Now the drawback is that the driver would require a change, to parse this
> > > > prescaler new prescaler property. Would this be OK from your point of view
> > > > Wolfram ? If yes, I will send the patches for it.
> > > 
> > > I don't think it is OK.
> > 
> > Why?
> 
> Because I thought it could be deduced. Then, a seperate property would
> not be OK.
> 
> > >  I'd think it can be deduced from the compatible property.
> > 
> > For almost all existing device trees it cannot be.
> 
> Pity :( If we do introduce a new property, it should probably be
> "clock-div". Grepping through binding documentation, that seems
> accepted. We should ask DT maintainers, too, to be safe.
> 
> > If you want something that will work without changing device trees,
> > you'll need to use SVR to identify the SoC.
> 
> The driver is doing that already, see mpc_i2c_get_sec_cfg_8xxx(). Dunno
> if it makes sense to add to it for consistency reasons?

That's not SVR, but sure.  Better to avoid messing with existing device
trees.

-Scott

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Danielle Costantino]

I saw that this patch was marked as not applicable, but on most qoriq
devices the pre-scaler is 2 especially for p2020/p2010 devices
arch/powerpc/boot/dts/fsl/p2020si-post.dtsi

from the P2020 RM: p. 477
"Frequency divider ratio. Used to prescale the clock for bit rate
selection. The serial bit clock frequency of
SCL is equal to one half the platform ( CCB ) clock divided by the
designated divider ."

This means that the current dts for these devices are providing false
clock settings. I have a p2020 board and can take some scope
measurements next week to prove this. I think something should be
modified to address this.


On Tue, Nov 25, 2014 at 5:41 PM, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, 2014-11-25 at 19:13 +0100, Wolfram Sang wrote:
>> On Mon, Nov 17, 2014 at 07:28:03PM -0600, Scott Wood wrote:
>> > On Fri, 2014-11-14 at 09:28 +0100, Wolfram Sang wrote:
>> > > > >
>> > > > > If we're going to change the device tree I'd rather just add a property
>> > > > > to say what the prescaler is.
>> > > >
>> > > >  We would however, leave the boards' device trees that use things like
>> > > > "fsl,mpc8543-i2c" as is and introduce the prescaler for the others requiring it.
>> > > >
>> > > >
>> > > > Now the drawback is that the driver would require a change, to parse this
>> > > > prescaler new prescaler property. Would this be OK from your point of view
>> > > > Wolfram ? If yes, I will send the patches for it.
>> > >
>> > > I don't think it is OK.
>> >
>> > Why?
>>
>> Because I thought it could be deduced. Then, a seperate property would
>> not be OK.
>>
>> > >  I'd think it can be deduced from the compatible property.
>> >
>> > For almost all existing device trees it cannot be.
>>
>> Pity :( If we do introduce a new property, it should probably be
>> "clock-div". Grepping through binding documentation, that seems
>> accepted. We should ask DT maintainers, too, to be safe.
>>
>> > If you want something that will work without changing device trees,
>> > you'll need to use SVR to identify the SoC.
>>
>> The driver is doing that already, see mpc_i2c_get_sec_cfg_8xxx(). Dunno
>> if it makes sense to add to it for consistency reasons?
>
> That's not SVR, but sure.  Better to avoid messing with existing device
> trees.
>
> -Scott
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
- Danielle Costantino

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Wolfram Sang]

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


> I saw that this patch was marked as not applicable, but on most qoriq
> devices the pre-scaler is 2 especially for p2020/p2010 devices
> arch/powerpc/boot/dts/fsl/p2020si-post.dtsi

Just for completeness: "Not applicable" given from patchwork of the i2c
subsystem means this patch is not for the i2c subsystem. In this case,
it is for powerpc because it was modifying powerpc dts files only. That
doesn't say anything about the patch itself.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Valentin Longchamp]

Hi all,

Picking up this issue again.

On 11/26/2014 02:41 AM, Scott Wood wrote:
> On Tue, 2014-11-25 at 19:13 +0100, Wolfram Sang wrote:
>> On Mon, Nov 17, 2014 at 07:28:03PM -0600, Scott Wood wrote:
>>> On Fri, 2014-11-14 at 09:28 +0100, Wolfram Sang wrote:
>>>>>>
>>>>>> If we're going to change the device tree I'd rather just add a property
>>>>>> to say what the prescaler is.
>>>>>
>>>>>  We would however, leave the boards' device trees that use things like
>>>>> "fsl,mpc8543-i2c" as is and introduce the prescaler for the others requiring it.
>>>>>
>>>>>
>>>>> Now the drawback is that the driver would require a change, to parse this
>>>>> prescaler new prescaler property. Would this be OK from your point of view
>>>>> Wolfram ? If yes, I will send the patches for it.
>>>>
>>>> I don't think it is OK.
>>>
>>> Why?
>>
>> Because I thought it could be deduced. Then, a seperate property would
>> not be OK.
>>
>>>>  I'd think it can be deduced from the compatible property.
>>>
>>> For almost all existing device trees it cannot be.
>>
>> Pity :( If we do introduce a new property, it should probably be
>> "clock-div". Grepping through binding documentation, that seems
>> accepted. We should ask DT maintainers, too, to be safe.
>>
>>> If you want something that will work without changing device trees,
>>> you'll need to use SVR to identify the SoC.
>>
>> The driver is doing that already, see mpc_i2c_get_sec_cfg_8xxx(). Dunno
>> if it makes sense to add to it for consistency reasons?
> 
> That's not SVR, but sure.  Better to avoid messing with existing device
> trees.
> 

What is then the agreement here ? Add a clock-div to the device trees ? Or do
something similar to  mpc_i2c_get_sec_cfg_8xxx() ?

I think the clock-div property is better according to Freescale's AN 2919
section 3.1 Source clock. All the source clocks are fixed (with a clock-div of 2
in case of mpc8536/43/45/47/48/67/68/72, plus p2020) except for the mpc8533/44
where it can be 2 or 3, and that's what mpc_i2c_get_sec_cfg_8xxx() determines.

So mpc_i2c_get_sec_cfg_8xxx() should remain the exception and the other
prescaler values should be derived from an additional clock-div that must be
added in the respective device trees (at least for the qoriq devices, because
for instance mpc8543 already has the correct prescaler thanks to
mpc_i2c_data_8543 from i2c-mpc.c).

Valentin

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Valentin Longchamp]

Wolfgang, Scott,

On 12/11/2014 02:44 PM, Valentin Longchamp wrote:
> Hi all,
> 
> Picking up this issue again.
> 
> On 11/26/2014 02:41 AM, Scott Wood wrote:
>> On Tue, 2014-11-25 at 19:13 +0100, Wolfram Sang wrote:
>>> On Mon, Nov 17, 2014 at 07:28:03PM -0600, Scott Wood wrote:
>>>> On Fri, 2014-11-14 at 09:28 +0100, Wolfram Sang wrote:
>>>>>>>
>>>>>>> If we're going to change the device tree I'd rather just add a property
>>>>>>> to say what the prescaler is.
>>>>>>
>>>>>>  We would however, leave the boards' device trees that use things like
>>>>>> "fsl,mpc8543-i2c" as is and introduce the prescaler for the others requiring it.
>>>>>>
>>>>>>
>>>>>> Now the drawback is that the driver would require a change, to parse this
>>>>>> prescaler new prescaler property. Would this be OK from your point of view
>>>>>> Wolfram ? If yes, I will send the patches for it.
>>>>>
>>>>> I don't think it is OK.
>>>>
>>>> Why?
>>>
>>> Because I thought it could be deduced. Then, a seperate property would
>>> not be OK.
>>>
>>>>>  I'd think it can be deduced from the compatible property.
>>>>
>>>> For almost all existing device trees it cannot be.
>>>
>>> Pity :( If we do introduce a new property, it should probably be
>>> "clock-div". Grepping through binding documentation, that seems
>>> accepted. We should ask DT maintainers, too, to be safe.
>>>
>>>> If you want something that will work without changing device trees,
>>>> you'll need to use SVR to identify the SoC.
>>>
>>> The driver is doing that already, see mpc_i2c_get_sec_cfg_8xxx(). Dunno
>>> if it makes sense to add to it for consistency reasons?
>>
>> That's not SVR, but sure.  Better to avoid messing with existing device
>> trees.
>>
> 
> What is then the agreement here ? Add a clock-div to the device trees ? Or do
> something similar to  mpc_i2c_get_sec_cfg_8xxx() ?
> 
> I think the clock-div property is better according to Freescale's AN 2919
> section 3.1 Source clock. All the source clocks are fixed (with a clock-div of 2
> in case of mpc8536/43/45/47/48/67/68/72, plus p2020) except for the mpc8533/44
> where it can be 2 or 3, and that's what mpc_i2c_get_sec_cfg_8xxx() determines.
> 
> So mpc_i2c_get_sec_cfg_8xxx() should remain the exception and the other
> prescaler values should be derived from an additional clock-div that must be
> added in the respective device trees (at least for the qoriq devices, because
> for instance mpc8543 already has the correct prescaler thanks to
> mpc_i2c_data_8543 from i2c-mpc.c).
> 

Do you have an opinion on the above ?

Valentin

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Wolfram Sang]

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

On Tue, Dec 23, 2014 at 02:23:01PM +0100, Valentin Longchamp wrote:
> Wolfgang, Scott,

Wolfram, please.

> > What is then the agreement here ? Add a clock-div to the device trees ? Or do
> > something similar to  mpc_i2c_get_sec_cfg_8xxx() ?
> > 
> > I think the clock-div property is better according to Freescale's AN 2919
> > section 3.1 Source clock. All the source clocks are fixed (with a clock-div of 2
> > in case of mpc8536/43/45/47/48/67/68/72, plus p2020) except for the mpc8533/44
> > where it can be 2 or 3, and that's what mpc_i2c_get_sec_cfg_8xxx() determines.
> > 
> > So mpc_i2c_get_sec_cfg_8xxx() should remain the exception and the other
> > prescaler values should be derived from an additional clock-div that must be
> > added in the respective device trees (at least for the qoriq devices, because
> > for instance mpc8543 already has the correct prescaler thanks to
> > mpc_i2c_data_8543 from i2c-mpc.c).
> > 
> 
> Do you have an opinion on the above ?

I don't mind. I'll leave it to PowerPC experts to judge if a new binding
is apropriate or reading SVR is the way to go. If it is going to be a
new binding, then please look around before if there is already
something similar around...


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler

[Scott Wood]

On Tue, 2014-12-23 at 14:49 +0100, Wolfram Sang wrote:
> On Tue, Dec 23, 2014 at 02:23:01PM +0100, Valentin Longchamp wrote:
> > Wolfgang, Scott,
> 
> Wolfram, please.
> 
> > > What is then the agreement here ? Add a clock-div to the device trees ? Or do
> > > something similar to  mpc_i2c_get_sec_cfg_8xxx() ?
> > > 
> > > I think the clock-div property is better according to Freescale's AN 2919
> > > section 3.1 Source clock. All the source clocks are fixed (with a clock-div of 2
> > > in case of mpc8536/43/45/47/48/67/68/72, plus p2020) except for the mpc8533/44
> > > where it can be 2 or 3, and that's what mpc_i2c_get_sec_cfg_8xxx() determines.
> > > 
> > > So mpc_i2c_get_sec_cfg_8xxx() should remain the exception and the other
> > > prescaler values should be derived from an additional clock-div that must be
> > > added in the respective device trees (at least for the qoriq devices, because
> > > for instance mpc8543 already has the correct prescaler thanks to
> > > mpc_i2c_data_8543 from i2c-mpc.c).
> > > 
> > 
> > Do you have an opinion on the above ?
> 
> I don't mind. I'll leave it to PowerPC experts to judge if a new binding
> is apropriate or reading SVR is the way to go. If it is going to be a
> new binding, then please look around before if there is already
> something similar around...

I'd rather use SVR so things work with existing device trees.

-Scott