[PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Alexander Sverdlin]

According to KeyStone Architecture I2C User Guide,

                         module clock frequency
master clock frequency = ----------------------
                         (ICCL + 6) + (ICCH + 6)

i.e. "d" in i2c_davinci_calc_clk_dividers() should be fixed and
not dependent from module clock prescaler PSC on these SoCs.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---

RFC: If someone from TI has an idea how to improve the coverage of future Keystone
revisions -- hints/patches are welcome. The current ID check is based on
Davinci/Keystone datasheets and is at least working on real Keystone II.

 drivers/i2c/busses/i2c-davinci.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4a110af..3d78f6a 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -60,6 +60,8 @@
 #define DAVINCI_I2C_IVR_REG	0x28
 #define DAVINCI_I2C_EMDR_REG	0x2c
 #define DAVINCI_I2C_PSC_REG	0x30
+#define DAVINCI_I2C_ICPID1_REG	0x34
+#define DAVINCI_I2C_ICPID2_REG	0x38
 #define DAVINCI_I2C_FUNC_REG	0x48
 #define DAVINCI_I2C_DIR_REG	0x4c
 #define DAVINCI_I2C_DIN_REG	0x50
@@ -203,6 +205,9 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
 	 * where if PSC == 0, d = 7,
 	 *       if PSC == 1, d = 6
 	 *       if PSC > 1 , d = 5
+	 *
+	 * Note:
+	 * d is always 6 on Keystone I2C controller
 	 */

 	/* get minimum of 7 MHz clock, but max of 12 MHz */
@@ -211,6 +216,11 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
 		psc++;	/* better to run under spec than over */
 	d = (psc >= 2) ? 5 : 7 - psc;

+	if (davinci_i2c_read_reg(dev, DAVINCI_I2C_ICPID2_REG) == 0x2206) {
+		dev_dbg(dev->dev, "Keystone SoC detected\n");
+		d = 6;
+	}
+
 	clk = ((input_clock / (psc + 1)) / (pdata->bus_freq * 1000));
 	/* Avoid driving the bus too fast because of rounding errors above */
 	if (input_clock / (psc + 1) / clk > pdata->bus_freq * 1000)

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Sekhar Nori]

On Thursday 18 June 2015 02:23 PM, Alexander Sverdlin wrote:
> According to KeyStone Architecture I2C User Guide,
> 
>                          module clock frequency
> master clock frequency = ----------------------
>                          (ICCL + 6) + (ICCH + 6)
> 
> i.e. "d" in i2c_davinci_calc_clk_dividers() should be fixed and
> not dependent from module clock prescaler PSC on these SoCs.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
> 
> RFC: If someone from TI has an idea how to improve the coverage of future Keystone
> revisions -- hints/patches are welcome. The current ID check is based on
> Davinci/Keystone datasheets and is at least working on real Keystone II.

+ Murali who works on Keystone devices in TI.

> 
>  drivers/i2c/busses/i2c-davinci.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 4a110af..3d78f6a 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -60,6 +60,8 @@
>  #define DAVINCI_I2C_IVR_REG	0x28
>  #define DAVINCI_I2C_EMDR_REG	0x2c
>  #define DAVINCI_I2C_PSC_REG	0x30
> +#define DAVINCI_I2C_ICPID1_REG	0x34
> +#define DAVINCI_I2C_ICPID2_REG	0x38
>  #define DAVINCI_I2C_FUNC_REG	0x48
>  #define DAVINCI_I2C_DIR_REG	0x4c
>  #define DAVINCI_I2C_DIN_REG	0x50
> @@ -203,6 +205,9 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
>  	 * where if PSC == 0, d = 7,
>  	 *       if PSC == 1, d = 6
>  	 *       if PSC > 1 , d = 5
> +	 *
> +	 * Note:
> +	 * d is always 6 on Keystone I2C controller
>  	 */
> 
>  	/* get minimum of 7 MHz clock, but max of 12 MHz */
> @@ -211,6 +216,11 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
>  		psc++;	/* better to run under spec than over */
>  	d = (psc >= 2) ? 5 : 7 - psc;
> 
> +	if (davinci_i2c_read_reg(dev, DAVINCI_I2C_ICPID2_REG) == 0x2206) {
> +		dev_dbg(dev->dev, "Keystone SoC detected\n");
> +		d = 6;
> +	}

I think its better to use a different compatible string for i2c on
keystone devices rather than using a fixed hardcoded IP version.

Thanks,
Sekhar

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Alexander Sverdlin]

Hi!

On 18/06/15 11:00, ext Sekhar Nori wrote:
>> +	if (davinci_i2c_read_reg(dev, DAVINCI_I2C_ICPID2_REG) == 0x2206) {
>> > +		dev_dbg(dev->dev, "Keystone SoC detected\n");
>> > +		d = 6;
>> > +	}
> I think its better to use a different compatible string for i2c on
> keystone devices rather than using a fixed hardcoded IP version.

Yeah, this should have been done from the beginning, when the driver has been
re-used for Keystone, but this time is already missed, so I don't want
to introduce huge incompatibility with the existing device-trees.

And from the other PoV, device-trees are for something one cannot probe. We
can probe for Keystone revisions and can free the end-user from this headache
completely.

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Sekhar Nori]

Hi Alexander,

On Thursday 18 June 2015 02:39 PM, Alexander Sverdlin wrote:
> Hi!
> 
> On 18/06/15 11:00, ext Sekhar Nori wrote:
>>> +	if (davinci_i2c_read_reg(dev, DAVINCI_I2C_ICPID2_REG) == 0x2206) {
>>>> +		dev_dbg(dev->dev, "Keystone SoC detected\n");
>>>> +		d = 6;
>>>> +	}
>> I think its better to use a different compatible string for i2c on
>> keystone devices rather than using a fixed hardcoded IP version.
> 
> Yeah, this should have been done from the beginning, when the driver has been
> re-used for Keystone, but this time is already missed, so I don't want
> to introduce huge incompatibility with the existing device-trees.

How is backward compatibility broken? compatible property is a list, so
you would do something like:

	compatible = "ti,keystone-i2c", "ti,davinci-i2c";

Newer kernels would keep working with older device tree blobs. Older
kernels wont have the fix, but thats true even now.

> And from the other PoV, device-trees are for something one cannot probe. We
> can probe for Keystone revisions and can free the end-user from this headache
> completely.

Keep in mind that this can invite driver patching whenever version
number is tinkered with in hardware - even for otherwise
software-invsible changes.

Thanks,
Sekhar

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Alexander Sverdlin]

Hello!

On 18/06/15 11:25, ext Sekhar Nori wrote:
>>>> +	if (davinci_i2c_read_reg(dev, DAVINCI_I2C_ICPID2_REG) == 0x2206) {
>>>>> >>>> +		dev_dbg(dev->dev, "Keystone SoC detected\n");
>>>>> >>>> +		d = 6;
>>>>> >>>> +	}
>>> >> I think its better to use a different compatible string for i2c on
>>> >> keystone devices rather than using a fixed hardcoded IP version.
>> > 
>> > Yeah, this should have been done from the beginning, when the driver has been
>> > re-used for Keystone, but this time is already missed, so I don't want
>> > to introduce huge incompatibility with the existing device-trees.
> How is backward compatibility broken? compatible property is a list, so
> you would do something like:
> 
> 	compatible = "ti,keystone-i2c", "ti,davinci-i2c";
> 
> Newer kernels would keep working with older device tree blobs. Older
> kernels wont have the fix, but thats true even now.

Ah, beyond the evalboards, there are device-trees not linked into the kernel,
but flashed into the boards, as originally in OF. They are part of the HW, its
description. Not part or description of the Kernel. And you have no way to
introduce this fix any more without updating this OF part if you go with
new compatible property.

>> > And from the other PoV, device-trees are for something one cannot probe. We
>> > can probe for Keystone revisions and can free the end-user from this headache
>> > completely.
> Keep in mind that this can invite driver patching whenever version
> number is tinkered with in hardware - even for otherwise
> software-invsible changes.

That's true. But I do not have an overview, how many IP versions do you actually have?
I've found one revision in Davinci manual, one revision in Keystone manual, even
including minor revision. Checking only major revision now can survive couple of minor
changes in IP.

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Sekhar Nori]

On Thursday 18 June 2015 03:07 PM, Alexander Sverdlin wrote:
> Hello!
> 
> On 18/06/15 11:25, ext Sekhar Nori wrote:
>>>>> +	if (davinci_i2c_read_reg(dev, DAVINCI_I2C_ICPID2_REG) == 0x2206) {
>>>>>>>>>> +		dev_dbg(dev->dev, "Keystone SoC detected\n");
>>>>>>>>>> +		d = 6;
>>>>>>>>>> +	}
>>>>>> I think its better to use a different compatible string for i2c on
>>>>>> keystone devices rather than using a fixed hardcoded IP version.
>>>>
>>>> Yeah, this should have been done from the beginning, when the driver has been
>>>> re-used for Keystone, but this time is already missed, so I don't want
>>>> to introduce huge incompatibility with the existing device-trees.
>> How is backward compatibility broken? compatible property is a list, so
>> you would do something like:
>>
>> 	compatible = "ti,keystone-i2c", "ti,davinci-i2c";
>>
>> Newer kernels would keep working with older device tree blobs. Older
>> kernels wont have the fix, but thats true even now.
> 
> Ah, beyond the evalboards, there are device-trees not linked into the kernel,
> but flashed into the boards, as originally in OF. They are part of the HW, its
> description. Not part or description of the Kernel. And you have no way to
> introduce this fix any more without updating this OF part if you go with
> new compatible property.

I see. So how critical is this fix? That should be described in the
commit description. And if its really critical, stable kernel should be
CCed too.

> 
>>>> And from the other PoV, device-trees are for something one cannot probe. We
>>>> can probe for Keystone revisions and can free the end-user from this headache
>>>> completely.
>> Keep in mind that this can invite driver patching whenever version
>> number is tinkered with in hardware - even for otherwise
>> software-invsible changes.
> 
> That's true. But I do not have an overview, how many IP versions do you actually have?
> I've found one revision in Davinci manual, one revision in Keystone manual, even
> including minor revision. Checking only major revision now can survive couple of minor
> changes in IP.

Yeah, sticking to major version should help. What I am worried about are
versions coming in future, not those existing. And development on
keystone architecture is ongoing in TI.

Thanks,
Sekhar

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Alexander Sverdlin]

Hi!

On 18/06/15 11:47, ext Sekhar Nori wrote:
>> Ah, beyond the evalboards, there are device-trees not linked into the kernel,
>> > but flashed into the boards, as originally in OF. They are part of the HW, its
>> > description. Not part or description of the Kernel. And you have no way to
>> > introduce this fix any more without updating this OF part if you go with
>> > new compatible property.
> I see. So how critical is this fix? That should be described in the
> commit description. And if its really critical, stable kernel should be
> CCed too.

Now we got to the point, see below...

>>>>> >>>> And from the other PoV, device-trees are for something one cannot probe. We
>>>>> >>>> can probe for Keystone revisions and can free the end-user from this headache
>>>>> >>>> completely.
>>> >> Keep in mind that this can invite driver patching whenever version
>>> >> number is tinkered with in hardware - even for otherwise
>>> >> software-invsible changes.
>> > 
>> > That's true. But I do not have an overview, how many IP versions do you actually have?
>> > I've found one revision in Davinci manual, one revision in Keystone manual, even
>> > including minor revision. Checking only major revision now can survive couple of minor
>> > changes in IP.
> Yeah, sticking to major version should help. What I am worried about are
> versions coming in future, not those existing. And development on
> keystone architecture is ongoing in TI.

This is not really critical fix. Currently bus rate is lower than expected because of these
calculation errors. The fix maximizes the bus rate. So newer SoCs will run little bit slower
until support is added to this part of the code. Not really critical. So no point in CCing
stable maintainers also.

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Sekhar Nori]

On Thursday 18 June 2015 03:34 PM, Alexander Sverdlin wrote:
> Hi!
> 
> On 18/06/15 11:47, ext Sekhar Nori wrote:
>>> Ah, beyond the evalboards, there are device-trees not linked into the kernel,
>>>> but flashed into the boards, as originally in OF. They are part of the HW, its
>>>> description. Not part or description of the Kernel. And you have no way to
>>>> introduce this fix any more without updating this OF part if you go with
>>>> new compatible property.
>> I see. So how critical is this fix? That should be described in the
>> commit description. And if its really critical, stable kernel should be
>> CCed too.
> 
> Now we got to the point, see below...
> 
>>>>>>>>>> And from the other PoV, device-trees are for something one cannot probe. We
>>>>>>>>>> can probe for Keystone revisions and can free the end-user from this headache
>>>>>>>>>> completely.
>>>>>> Keep in mind that this can invite driver patching whenever version
>>>>>> number is tinkered with in hardware - even for otherwise
>>>>>> software-invsible changes.
>>>>
>>>> That's true. But I do not have an overview, how many IP versions do you actually have?
>>>> I've found one revision in Davinci manual, one revision in Keystone manual, even
>>>> including minor revision. Checking only major revision now can survive couple of minor
>>>> changes in IP.
>> Yeah, sticking to major version should help. What I am worried about are
>> versions coming in future, not those existing. And development on
>> keystone architecture is ongoing in TI.
> 
> This is not really critical fix. Currently bus rate is lower than expected because of these
> calculation errors. The fix maximizes the bus rate. So newer SoCs will run little bit slower
> until support is added to this part of the code. Not really critical. So no point in CCing
> stable maintainers also.

If its not a critical fix, do we really need to care about older DTBs
which have been ROM'ed into production?

Thanks,
Sekhar

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Alexander Sverdlin]

Hello!

On 18/06/15 13:12, ext Sekhar Nori wrote:
>>>> Ah, beyond the evalboards, there are device-trees not linked into the kernel,
>>>>> >>>> but flashed into the boards, as originally in OF. They are part of the HW, its
>>>>> >>>> description. Not part or description of the Kernel. And you have no way to
>>>>> >>>> introduce this fix any more without updating this OF part if you go with
>>>>> >>>> new compatible property.
>>> >> I see. So how critical is this fix? That should be described in the
>>> >> commit description. And if its really critical, stable kernel should be
>>> >> CCed too.
>> > 
>> > Now we got to the point, see below...
>> > 
>>>>>>>>>>> >>>>>>>>>> And from the other PoV, device-trees are for something one cannot probe. We
>>>>>>>>>>> >>>>>>>>>> can probe for Keystone revisions and can free the end-user from this headache
>>>>>>>>>>> >>>>>>>>>> completely.
>>>>>>> >>>>>> Keep in mind that this can invite driver patching whenever version
>>>>>>> >>>>>> number is tinkered with in hardware - even for otherwise
>>>>>>> >>>>>> software-invsible changes.
>>>>> >>>>
>>>>> >>>> That's true. But I do not have an overview, how many IP versions do you actually have?
>>>>> >>>> I've found one revision in Davinci manual, one revision in Keystone manual, even
>>>>> >>>> including minor revision. Checking only major revision now can survive couple of minor
>>>>> >>>> changes in IP.
>>> >> Yeah, sticking to major version should help. What I am worried about are
>>> >> versions coming in future, not those existing. And development on
>>> >> keystone architecture is ongoing in TI.
>> > 
>> > This is not really critical fix. Currently bus rate is lower than expected because of these
>> > calculation errors. The fix maximizes the bus rate. So newer SoCs will run little bit slower
>> > until support is added to this part of the code. Not really critical. So no point in CCing
>> > stable maintainers also.
> If its not a critical fix, do we really need to care about older DTBs
> which have been ROM'ed into production?

I tend not to change the DT binding, but if majority will decide it's the way to go,
I'll prepare another patch. Let's wait for other opinions...

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Murali Karicheri]

On 06/18/2015 05:00 AM, Sekhar Nori wrote:
> On Thursday 18 June 2015 02:23 PM, Alexander Sverdlin wrote:
>> According to KeyStone Architecture I2C User Guide,
>>
>>                           module clock frequency
>> master clock frequency = ----------------------
>>                           (ICCL + 6) + (ICCH + 6)
>>
>> i.e. "d" in i2c_davinci_calc_clk_dividers() should be fixed and
>> not dependent from module clock prescaler PSC on these SoCs.
>>
>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
>> ---
>>
>> RFC: If someone from TI has an idea how to improve the coverage of future Keystone
>> revisions -- hints/patches are welcome. The current ID check is based on
>> Davinci/Keystone datasheets and is at least working on real Keystone II.
>
> + Murali who works on Keystone devices in TI.

+ Grygorii

+ Grygorii has been involved in the Keystone related enhancement and 
reviewing prior patches. Need to have his ack for this change

Murali
>
>>
>>   drivers/i2c/busses/i2c-davinci.c |   10 ++++++++++
>>   1 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index 4a110af..3d78f6a 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -60,6 +60,8 @@
>>   #define DAVINCI_I2C_IVR_REG	0x28
>>   #define DAVINCI_I2C_EMDR_REG	0x2c
>>   #define DAVINCI_I2C_PSC_REG	0x30
>> +#define DAVINCI_I2C_ICPID1_REG	0x34
>> +#define DAVINCI_I2C_ICPID2_REG	0x38
>>   #define DAVINCI_I2C_FUNC_REG	0x48
>>   #define DAVINCI_I2C_DIR_REG	0x4c
>>   #define DAVINCI_I2C_DIN_REG	0x50
>> @@ -203,6 +205,9 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
>>   	 * where if PSC == 0, d = 7,
>>   	 *       if PSC == 1, d = 6
>>   	 *       if PSC > 1 , d = 5
>> +	 *
>> +	 * Note:
>> +	 * d is always 6 on Keystone I2C controller
>>   	 */
>>
>>   	/* get minimum of 7 MHz clock, but max of 12 MHz */
>> @@ -211,6 +216,11 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
>>   		psc++;	/* better to run under spec than over */
>>   	d = (psc >= 2) ? 5 : 7 - psc;
>>
>> +	if (davinci_i2c_read_reg(dev, DAVINCI_I2C_ICPID2_REG) == 0x2206) {
>> +		dev_dbg(dev->dev, "Keystone SoC detected\n");
>> +		d = 6;
>> +	}
>
> I think its better to use a different compatible string for i2c on
> keystone devices rather than using a fixed hardcoded IP version.
>
> Thanks,
> Sekhar
>


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Wolfram Sang]

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

On Thu, Jun 18, 2015 at 12:22:33PM -0400, Murali Karicheri wrote:
> On 06/18/2015 05:00 AM, Sekhar Nori wrote:
> >On Thursday 18 June 2015 02:23 PM, Alexander Sverdlin wrote:
> >>According to KeyStone Architecture I2C User Guide,
> >>
> >>                          module clock frequency
> >>master clock frequency = ----------------------
> >>                          (ICCL + 6) + (ICCH + 6)
> >>
> >>i.e. "d" in i2c_davinci_calc_clk_dividers() should be fixed and
> >>not dependent from module clock prescaler PSC on these SoCs.
> >>
> >>Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> >>---
> >>
> >>RFC: If someone from TI has an idea how to improve the coverage of future Keystone
> >>revisions -- hints/patches are welcome. The current ID check is based on
> >>Davinci/Keystone datasheets and is at least working on real Keystone II.
> >
> >+ Murali who works on Keystone devices in TI.
> 
> + Grygorii
> 
> + Grygorii has been involved in the Keystone related enhancement and
> reviewing prior patches. Need to have his ack for this change

Any news?

> 
> Murali
> >
> >>
> >>  drivers/i2c/busses/i2c-davinci.c |   10 ++++++++++
> >>  1 files changed, 10 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> >>index 4a110af..3d78f6a 100644
> >>--- a/drivers/i2c/busses/i2c-davinci.c
> >>+++ b/drivers/i2c/busses/i2c-davinci.c
> >>@@ -60,6 +60,8 @@
> >>  #define DAVINCI_I2C_IVR_REG	0x28
> >>  #define DAVINCI_I2C_EMDR_REG	0x2c
> >>  #define DAVINCI_I2C_PSC_REG	0x30
> >>+#define DAVINCI_I2C_ICPID1_REG	0x34
> >>+#define DAVINCI_I2C_ICPID2_REG	0x38
> >>  #define DAVINCI_I2C_FUNC_REG	0x48
> >>  #define DAVINCI_I2C_DIR_REG	0x4c
> >>  #define DAVINCI_I2C_DIN_REG	0x50
> >>@@ -203,6 +205,9 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
> >>  	 * where if PSC == 0, d = 7,
> >>  	 *       if PSC == 1, d = 6
> >>  	 *       if PSC > 1 , d = 5
> >>+	 *
> >>+	 * Note:
> >>+	 * d is always 6 on Keystone I2C controller
> >>  	 */
> >>
> >>  	/* get minimum of 7 MHz clock, but max of 12 MHz */
> >>@@ -211,6 +216,11 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
> >>  		psc++;	/* better to run under spec than over */
> >>  	d = (psc >= 2) ? 5 : 7 - psc;
> >>
> >>+	if (davinci_i2c_read_reg(dev, DAVINCI_I2C_ICPID2_REG) == 0x2206) {
> >>+		dev_dbg(dev->dev, "Keystone SoC detected\n");
> >>+		d = 6;
> >>+	}
> >
> >I think its better to use a different compatible string for i2c on
> >keystone devices rather than using a fixed hardcoded IP version.
> >
> >Thanks,
> >Sekhar
> >
> 
> 
> -- 
> Murali Karicheri
> Linux Kernel, Keystone

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

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Sekhar Nori]

On Friday 10 July 2015 01:23 AM, Wolfram Sang wrote:
> On Thu, Jun 18, 2015 at 12:22:33PM -0400, Murali Karicheri wrote:
>> On 06/18/2015 05:00 AM, Sekhar Nori wrote:
>>> On Thursday 18 June 2015 02:23 PM, Alexander Sverdlin wrote:
>>>> According to KeyStone Architecture I2C User Guide,
>>>>
>>>>                          module clock frequency
>>>> master clock frequency = ----------------------
>>>>                          (ICCL + 6) + (ICCH + 6)
>>>>
>>>> i.e. "d" in i2c_davinci_calc_clk_dividers() should be fixed and
>>>> not dependent from module clock prescaler PSC on these SoCs.
>>>>
>>>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>
>>>> RFC: If someone from TI has an idea how to improve the coverage of future Keystone
>>>> revisions -- hints/patches are welcome. The current ID check is based on
>>>> Davinci/Keystone datasheets and is at least working on real Keystone II.
>>>
>>> + Murali who works on Keystone devices in TI.
>>
>> + Grygorii
>>
>> + Grygorii has been involved in the Keystone related enhancement and
>> reviewing prior patches. Need to have his ack for this change
> 
> Any news?

Fixing Grygorii's e-mail id.

Grygorii, let me know if you don't have the thread. I can forward.

Thanks,
Sekhar

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Grygorii Strashko]

Hi,
On 07/10/2015 07:02 PM, Sekhar Nori wrote:
> On Friday 10 July 2015 01:23 AM, Wolfram Sang wrote:
>> On Thu, Jun 18, 2015 at 12:22:33PM -0400, Murali Karicheri wrote:
>>> On 06/18/2015 05:00 AM, Sekhar Nori wrote:
>>>> On Thursday 18 June 2015 02:23 PM, Alexander Sverdlin wrote:
>>>>> According to KeyStone Architecture I2C User Guide,
>>>>>
>>>>>                           module clock frequency
>>>>> master clock frequency = ----------------------
>>>>>                           (ICCL + 6) + (ICCH + 6)
>>>>>
>>>>> i.e. "d" in i2c_davinci_calc_clk_dividers() should be fixed and
>>>>> not dependent from module clock prescaler PSC on these SoCs.
>>>>>
>>>>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
>>>>> ---
>>>>>
>>>>> RFC: If someone from TI has an idea how to improve the coverage of future Keystone
>>>>> revisions -- hints/patches are welcome. The current ID check is based on
>>>>> Davinci/Keystone datasheets and is at least working on real Keystone II.
>>>>
>>>> + Murali who works on Keystone devices in TI.
>>>
>>> + Grygorii
>>>
>>> + Grygorii has been involved in the Keystone related enhancement and
>>> reviewing prior patches. Need to have his ack for this change
>>
>> Any news?
> 
> Fixing Grygorii's e-mail id.
> 
> Grygorii, let me know if you don't have the thread. I can forward.

Thanks Sekhar.

My opinion - it's time for compatible string :) "ti,keystone-i2c". 
Especially taking int account two things:
1) In Datasheet SPRS893B TCI6630K2L Multicore DSP+ARM KeyStone II System-on-Chip (SoC) (Rev. E) 
   values for those registers specified as:
   0x0034 ICPID1 I2C Peripheral Identification Register 1 [value: 0x0000 0105]
   0x0038 ICPID2 I2C Peripheral Identification Register 2 [value: 0x0000 0005]
   (actually the same is in k2h, k2e Datasheets).

2) This is not the first time such discussion has been raised.


>> > This is not really critical fix. Currently bus rate is lower than expected because of these
>> > calculation errors. The fix maximizes the bus rate. So newer SoCs will run little bit slower
>> > until support is added to this part of the code. Not really critical. 

Regarding the patch itself:
- Seems the "d" value is fixed to 6 as per User Guide SPRUGV3
  "KeyStone Architecture 2 Inter-IC Control Bus (I2C)" and this change is correct. 
  It would be nice to have ref on document in commit message as above.

- I think, it will be very useful to have same real digits/calculation mentioned in
  commit message which can show how valuable is the improvement. 

-- 
regards,
-grygorii

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Wolfram Sang]

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

On Fri, Jul 10, 2015 at 09:26:30PM +0300, Grygorii Strashko wrote:
> Hi,
> On 07/10/2015 07:02 PM, Sekhar Nori wrote:
> > On Friday 10 July 2015 01:23 AM, Wolfram Sang wrote:
> >> On Thu, Jun 18, 2015 at 12:22:33PM -0400, Murali Karicheri wrote:
> >>> On 06/18/2015 05:00 AM, Sekhar Nori wrote:
> >>>> On Thursday 18 June 2015 02:23 PM, Alexander Sverdlin wrote:
> >>>>> According to KeyStone Architecture I2C User Guide,
> >>>>>
> >>>>>                           module clock frequency
> >>>>> master clock frequency = ----------------------
> >>>>>                           (ICCL + 6) + (ICCH + 6)
> >>>>>
> >>>>> i.e. "d" in i2c_davinci_calc_clk_dividers() should be fixed and
> >>>>> not dependent from module clock prescaler PSC on these SoCs.
> >>>>>
> >>>>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> >>>>> ---
> >>>>>
> >>>>> RFC: If someone from TI has an idea how to improve the coverage of future Keystone
> >>>>> revisions -- hints/patches are welcome. The current ID check is based on
> >>>>> Davinci/Keystone datasheets and is at least working on real Keystone II.
> >>>>
> >>>> + Murali who works on Keystone devices in TI.
> >>>
> >>> + Grygorii
> >>>
> >>> + Grygorii has been involved in the Keystone related enhancement and
> >>> reviewing prior patches. Need to have his ack for this change
> >>
> >> Any news?
> > 
> > Fixing Grygorii's e-mail id.
> > 
> > Grygorii, let me know if you don't have the thread. I can forward.
> 
> Thanks Sekhar.
> 
> My opinion - it's time for compatible string :) "ti,keystone-i2c". 
> Especially taking int account two things:
> 1) In Datasheet SPRS893B TCI6630K2L Multicore DSP+ARM KeyStone II System-on-Chip (SoC) (Rev. E) 
>    values for those registers specified as:
>    0x0034 ICPID1 I2C Peripheral Identification Register 1 [value: 0x0000 0105]
>    0x0038 ICPID2 I2C Peripheral Identification Register 2 [value: 0x0000 0005]
>    (actually the same is in k2h, k2e Datasheets).
> 
> 2) This is not the first time such discussion has been raised.
> 
> 
> >> > This is not really critical fix. Currently bus rate is lower than expected because of these
> >> > calculation errors. The fix maximizes the bus rate. So newer SoCs will run little bit slower
> >> > until support is added to this part of the code. Not really critical. 
> 
> Regarding the patch itself:
> - Seems the "d" value is fixed to 6 as per User Guide SPRUGV3
>   "KeyStone Architecture 2 Inter-IC Control Bus (I2C)" and this change is correct. 
>   It would be nice to have ref on document in commit message as above.
> 
> - I think, it will be very useful to have same real digits/calculation mentioned in
>   commit message which can show how valuable is the improvement. 

Alexander, any comments to this feedback?


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

Re: [PATCH] i2c: davinci: Fix bus rate calculation on Keystone SoC

[Alexander Sverdlin]

Hello Wolfram,

On 12/08/15 10:43, ext Wolfram Sang wrote:
>> > My opinion - it's time for compatible string :) "ti,keystone-i2c". 
>> > Especially taking int account two things:
>> > 1) In Datasheet SPRS893B TCI6630K2L Multicore DSP+ARM KeyStone II System-on-Chip (SoC) (Rev. E) 
>> >    values for those registers specified as:
>> >    0x0034 ICPID1 I2C Peripheral Identification Register 1 [value: 0x0000 0105]
>> >    0x0038 ICPID2 I2C Peripheral Identification Register 2 [value: 0x0000 0005]
>> >    (actually the same is in k2h, k2e Datasheets).
>> > 
>> > 2) This is not the first time such discussion has been raised.
>> > 
>> > 
>>>>> > >> > This is not really critical fix. Currently bus rate is lower than expected because of these
>>>>> > >> > calculation errors. The fix maximizes the bus rate. So newer SoCs will run little bit slower
>>>>> > >> > until support is added to this part of the code. Not really critical. 
>> > 
>> > Regarding the patch itself:
>> > - Seems the "d" value is fixed to 6 as per User Guide SPRUGV3
>> >   "KeyStone Architecture 2 Inter-IC Control Bus (I2C)" and this change is correct. 
>> >   It would be nice to have ref on document in commit message as above.
>> > 
>> > - I think, it will be very useful to have same real digits/calculation mentioned in
>> >   commit message which can show how valuable is the improvement. 
> Alexander, any comments to this feedback?

I would need to rework the patch to distinguish Keystones basing on DT compatible property.
I will re-send another version along with actual bofore-after real world numbers.
Thanks for feedback!

-- 
Best regards,
Alexander Sverdlin.