| Message ID | 5582870B.7030304@nokia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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@nokia.com> > --- > > 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 -- 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
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.
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 -- 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
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.
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 -- 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
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.
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 -- 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
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...
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@nokia.com> >> --- >> >> 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 >
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@nokia.com> > >>--- > >> > >>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
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@nokia.com> >>>> --- >>>> >>>> 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 -- 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
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@nokia.com> >>>>> --- >>>>> >>>>> 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.
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@nokia.com> > >>>>> --- > >>>>> > >>>>> 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?
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!
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)
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@nokia.com> --- 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(-) -- 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