i2c: davinci: Fix bus rate calculation on Keystone SoC
Subsystems (2)
PATCH 1/1 i2c: davinci: Fix bus rate calculation on Keystone SoC
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.
---
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(-)
drivers/i2c/busses/i2c-davinci.c +10 -0
@@ -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 )
Quoted 9 lines · 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.
Quoted 6 lines
Signed-off-by Alexander Sverdlin <[email protected]>---
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.
Quoted 28 lines
drivers/i2c/busses/i2c-davinci.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)drivers/i2c/busses/i2c-davinci.c +5
-0@@ -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 */
drivers/i2c/busses/i2c-davinci.c static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev) · line 222
Quoted 7 lines
drivers/i2c/busses/i2c-davinci.c +4
-0@@ -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
Hi!
Quoted 6 lines · ext Sekhar Nori
Quoted 4 lines
drivers/i2c/busses/i2c-davinci.c +1
-0+if (davinci_i2c_read_reg (dev,DAVINCI_I2C_ICPID2_REG ) ==0x2206 ) {drivers/i2c/busses/i2c-davinci.c static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev) · line 222
Quoted 3 lines
drivers/i2c/busses/i2c-davinci.c +3
-0+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.
Signature · 2 lines
Best regards, Alexander Sverdlin.
Hi Alexander,
Quoted 10 lines · Alexander Sverdlin
Hi!
Quoted 6 lines · ext Sekhar Nori
Quoted 4 lines
drivers/i2c/busses/i2c-davinci.c +1
-0+if (davinci_i2c_read_reg (dev,DAVINCI_I2C_ICPID2_REG ) ==0x2206 ) {drivers/i2c/busses/i2c-davinci.c static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev) · line 222
Quoted 3 lines
drivers/i2c/busses/i2c-davinci.c +3
-0+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.
Quoted 3 lines
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.
Quoted 3 lines
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
Hello!
Quoted 10 lines · ext Sekhar Nori
Quoted 10 lines
Quoted 10 lines
Quoted 6 lines
(quoted diff) +1
-0+ if (davinci_i2c_read_reg(dev, DAVINCI_I2C_ICPID2_REG) == 0x2206) {Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ 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.
Quoted 7 lines
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.
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
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.
Signature · 2 lines
Best regards, Alexander Sverdlin.
Quoted 18 lines · Alexander Sverdlin
Hello!
Quoted 15 lines · ext Sekhar Nori
Quoted 10 lines
Quoted 10 lines
Quoted 6 lines
(quoted diff) +1
-0+ if (davinci_i2c_read_reg(dev, DAVINCI_I2C_ICPID2_REG) == 0x2206) {Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ 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";
Quoted 8 lines
Quoted 2 lines
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.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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.
Quoted 8 lines
Quoted 3 lines
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
Hi!
Quoted 8 lines · ext Sekhar Nori
Quoted 5 lines
Ah, beyond the evalboards, there are device-trees not linked into the kernel,
Quoted 4 lines
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...
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
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.
Signature · 2 lines
Best regards, Alexander Sverdlin.
Quoted 25 lines · Alexander Sverdlin
Hi!
Quoted 8 lines · ext Sekhar Nori
Quoted 5 lines
Ah, beyond the evalboards, there are device-trees not linked into the kernel,
Quoted 4 lines
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...
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 3 lines
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
Hello!
Quoted 26 lines · ext Sekhar Nori
Quoted 26 lines
Quoted 26 lines
Quoted 8 lines
Ah, beyond the evalboards, there are device-trees not linked into the kernel,
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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...
Quoted 14 lines
Quoted 14 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
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...
Signature · 2 lines
Best regards, Alexander Sverdlin.
Quoted 10 lines · Sekhar Nori
Quoted 9 lines · 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.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Alexander Sverdlin <[email protected]>---
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
Quoted 43 lines
Quoted 29 lines
drivers/i2c/busses/i2c-davinci.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)drivers/i2c/busses/i2c-davinci.c +5
-0@@ -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 */ drivers/i2c/busses/i2c-davinci.c static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev) · line 222
Quoted 7 lines
drivers/i2c/busses/i2c-davinci.c +4
-0@@ -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
Signature · 2 lines
Murali Karicheri Linux Kernel, Keystone
Quoted 18 lines · Murali Karicheri
Quoted 17 lines · Sekhar Nori
Quoted 15 lines · 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 <[email protected]>---
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.
Quoted 6 lines
Quoted 1 line
+ 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?
Quoted 50 lines
Murali
Quoted 43 lines
Quoted 29 lines
drivers/i2c/busses/i2c-davinci.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)drivers/i2c/busses/i2c-davinci.c +5
-0@@ -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 */ drivers/i2c/busses/i2c-davinci.c static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev) · line 222
Quoted 7 lines
drivers/i2c/busses/i2c-davinci.c +4
-0@@ -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,
SekharSignature · 2 lines
Murali Karicheri Linux Kernel, Keystone
Quoted 19 lines · Wolfram Sang
Quoted 18 lines · Murali Karicheri
Quoted 17 lines · Sekhar Nori
Quoted 15 lines · 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 <[email protected]>---
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.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
+ 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
Hi,
Quoted 24 lines · Sekhar Nori
Quoted 23 lines · Wolfram Sang
Quoted 22 lines · Murali Karicheri
Quoted 10 lines · Sekhar Nori
Quoted 9 lines · 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.Quoted 8 lines
Quoted 6 lines
Signed-off-by Alexander Sverdlin <[email protected]>---
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
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
+ 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.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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.
Signature · 2 lines
regards, -grygorii
Quoted 52 lines · Grygorii Strashko
Hi,
Quoted 24 lines · Sekhar Nori
Quoted 23 lines · Wolfram Sang
Quoted 22 lines · Murali Karicheri
Quoted 10 lines · Sekhar Nori
Quoted 9 lines · 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.Quoted 8 lines
Quoted 6 lines
Signed-off-by Alexander Sverdlin <[email protected]>---
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
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
+ 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.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 7 lines
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,
Quoted 15 lines · ext Wolfram Sang
Quoted 15 lines
Quoted 15 lines
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.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
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!
Signature · 2 lines
Best regards, Alexander Sverdlin.