Re: [PATCH V3 0/2] i2c-designware: Baytrail bus locking driver
On 14/12/02 9:09, David E. Box wrote: Select Intel Baytrail platforms support PMIC's whose i2c bus may be controlled exclusively by platform hardware. This patch set adds support for i2c bus locking to the designware core and provides a driver module for managing the lock on these platforms. Since the lock on these systems isn't enumerable outside of the i2c platform driver, the locking functions are assigned at compile time. Have you ever look into the hwspinlock framework? It seems to me that such an exclusive operation between CPUs and external hardware blocks is exactly what hwspinlock is for. Further more hwspinlock takes care of exclusiveness between SMP cores. Ideally I would expect i2c-designware to have hwspinlock lock/unlock API calls on one I2C transaction, but it's not necessarily the case. Introducing such platform hooks (acquire_lock and release_lock) and keeping actual exclusive operataion outside the driver might be good for various usecases/platforms. Shinya -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 0/2] i2c-designware: Baytrail bus locking driver
On 14/12/02 9:09, David E. Box wrote: Select Intel Baytrail platforms support PMIC's whose i2c bus may be controlled exclusively by platform hardware. This patch set adds support for i2c bus locking to the designware core and provides a driver module for managing the lock on these platforms. Since the lock on these systems isn't enumerable outside of the i2c platform driver, the locking functions are assigned at compile time. Have you ever look into the hwspinlock framework? It seems to me that such an exclusive operation between CPUs and external hardware blocks is exactly what hwspinlock is for. Further more hwspinlock takes care of exclusiveness between SMP cores. Ideally I would expect i2c-designware to have hwspinlock lock/unlock API calls on one I2C transaction, but it's not necessarily the case. Introducing such platform hooks (acquire_lock and release_lock) and keeping actual exclusive operataion outside the driver might be good for various usecases/platforms. Shinya -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] panic: Make panic_timeout configurable
On 11/19/13 6:02 PM, Ralf Baechle wrote:> On Mon, Nov 18, 2013 at 09:04:36PM +, Jason Baron wrote: It's more complicated - MIPS was using the global default with five MIPS platforms overriding the default. I propose to kill these overrides for sanity unless somebody comes up with a good argument. Patch below. Ralf Signed-off-by: Ralf Baechle arch/mips/ar7/setup.c | 1 - arch/mips/emma/markeins/setup.c | 3 --- arch/mips/netlogic/xlp/setup.c | 1 - arch/mips/netlogic/xlr/setup.c | 1 - arch/mips/sibyte/swarm/setup.c | 2 -- 5 files changed, 8 deletions(-) [...] diff --git a/arch/mips/emma/markeins/setup.c b/arch/mips/emma/markeins/setup.c index d710058..9100122 100644 --- a/arch/mips/emma/markeins/setup.c +++ b/arch/mips/emma/markeins/setup.c @@ -111,9 +111,6 @@ void __init plat_mem_setup(void) iomem_resource.start = EMMA2RH_IO_BASE; iomem_resource.end = EMMA2RH_ROM_BASE - 1; - /* Reboot on panic */ - panic_timeout = 180; - markeins_sio_setup(); } IIRC we had set it to 180 seconds for some historical reasons, but I'm afraid nobody can recall the reason why it's set so in 2013... Anyway I was thinking it too long and reduced to a few seconds locally when debugging, so there shouldn't be a problem with this change. FWIW, for EMMA2RH portion: Acked-by: Shinya Kuribayashi Thank you always for your help, Ralf. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] panic: Make panic_timeout configurable
On 11/19/13 6:02 PM, Ralf Baechle wrote: On Mon, Nov 18, 2013 at 09:04:36PM +, Jason Baron wrote: It's more complicated - MIPS was using the global default with five MIPS platforms overriding the default. I propose to kill these overrides for sanity unless somebody comes up with a good argument. Patch below. Ralf Signed-off-by: Ralf Baechle r...@linux-mips.org arch/mips/ar7/setup.c | 1 - arch/mips/emma/markeins/setup.c | 3 --- arch/mips/netlogic/xlp/setup.c | 1 - arch/mips/netlogic/xlr/setup.c | 1 - arch/mips/sibyte/swarm/setup.c | 2 -- 5 files changed, 8 deletions(-) [...] diff --git a/arch/mips/emma/markeins/setup.c b/arch/mips/emma/markeins/setup.c index d710058..9100122 100644 --- a/arch/mips/emma/markeins/setup.c +++ b/arch/mips/emma/markeins/setup.c @@ -111,9 +111,6 @@ void __init plat_mem_setup(void) iomem_resource.start = EMMA2RH_IO_BASE; iomem_resource.end = EMMA2RH_ROM_BASE - 1; - /* Reboot on panic */ - panic_timeout = 180; - markeins_sio_setup(); } IIRC we had set it to 180 seconds for some historical reasons, but I'm afraid nobody can recall the reason why it's set so in 2013... Anyway I was thinking it too long and reduced to a few seconds locally when debugging, so there shouldn't be a problem with this change. FWIW, for EMMA2RH portion: Acked-by: Shinya Kuribayashi skuri...@pobox.com Thank you always for your help, Ralf. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] i2c designware add support of I2C standard mode
On 10/9/13 4:56 PM, Mika Westerberg wrote:> On Tue, Oct 08, 2013 at 05:00:55PM
+0200, Romain Baeriswyl wrote:
Some legacy devices support ony I2C standard mode at 100kHz.
This patch allows to select the standard mode through the DTS
with the use of the existing clock-frequency parameter.
When clock-frequency parameter is not set, the fast mode is selected.
Only when the parameter is set at 10, the standard mode is selected.
Signed-off-by: Romain Baeriswyl
Reviewed-by: Christian Ruppert
Reviewed-by: Mika Westerberg
I don't have build environment for these patches right now, but both
changes 1/2 and 2/2 look good.
It's quite reasonable and makes the code readable to convert
{3 => 300} into nsec based one. I should have done so from the
beginning, thanks for doing this.
Shinya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c designware make SCL and SDA falling time configurable
On 10/10/13 9:54 AM, Ryan Mallon wrote: On 09/10/13 18:55, Mika Westerberg wrote: On Tue, Oct 08, 2013 at 05:00:54PM +0200, Romain Baeriswyl wrote: @@ -307,15 +309,25 @@ int i2c_dw_init(struct dw_i2c_dev *dev) /* set standard and fast speed deviders for high/low periods */ + if (dev->sda_falling_time) + sda_falling_time = dev->sda_falling_time; + else + sda_falling_time = 300; /* ns */ I think this looks better: sda_falling_time = dev->sda_falling_time ? dev->sda_falling_time : 300; You can also use the gcc-ism, which is a bit more concise: sda_falling_time = dev->sda_falling_time ?: 300; +1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c designware make SCL and SDA falling time configurable
On 10/10/13 9:54 AM, Ryan Mallon wrote: On 09/10/13 18:55, Mika Westerberg wrote: On Tue, Oct 08, 2013 at 05:00:54PM +0200, Romain Baeriswyl wrote: @@ -307,15 +309,25 @@ int i2c_dw_init(struct dw_i2c_dev *dev) /* set standard and fast speed deviders for high/low periods */ + if (dev-sda_falling_time) + sda_falling_time = dev-sda_falling_time; + else + sda_falling_time = 300; /* ns */ I think this looks better: sda_falling_time = dev-sda_falling_time ? dev-sda_falling_time : 300; You can also use the gcc-ism, which is a bit more concise: sda_falling_time = dev-sda_falling_time ?: 300; +1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] i2c designware add support of I2C standard mode
On 10/9/13 4:56 PM, Mika Westerberg wrote: On Tue, Oct 08, 2013 at 05:00:55PM
+0200, Romain Baeriswyl wrote:
Some legacy devices support ony I2C standard mode at 100kHz.
This patch allows to select the standard mode through the DTS
with the use of the existing clock-frequency parameter.
When clock-frequency parameter is not set, the fast mode is selected.
Only when the parameter is set at 10, the standard mode is selected.
Signed-off-by: Romain Baeriswyl romai...@abilis.com
Reviewed-by: Christian Ruppert christian.rupp...@abilis.com
Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com
I don't have build environment for these patches right now, but both
changes 1/2 and 2/2 look good.
It's quite reasonable and makes the code readable to convert
{3 = 300} into nsec based one. I should have done so from the
beginning, thanks for doing this.
Shinya
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
On 8/21/13 11:39 PM, Christian Ruppert wrote: On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote: On 8/5/13 6:31 PM, Christian Ruppert wrote: On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: As said before, all t_SCL things should go away. Please forget about 100kbps, 400kbps, and so on. Bus/clock speed is totally pointless concept for the I2C bus systems. For example, as long as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a certain I2C bus, it doesn't matter that the resulting clock speed is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. Nobody in the I2C bus doesn't care about actual bus/clock speed. We don't have to care about the resulting bus speed, or rather we should/must not check to see if it's within the proper range. Actually, the I2C specification clearly defines f_SCL;max (and thus implies t_SCL;min), both in the tables and the timing diagrams. Why can we ignore this constraint while having to meet all the others? If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), f_SCL;max will be met by itself. I'm not sure if I agree with this: Standard mode: t_r;min 0ns t_f;min +0ns t_HIGH;min + 4000ns t_LOW;min + 4700ns 1/f_SCL = 8700ns ==> f_SCL = 115kHz==>violation of f_SCL;max=100kHz Fast mode (let's assume V_DD = 5.5V): t_r;min 20ns t_f;min + 20ns t_HIGH;min + 600ns t_LIW;min + 1300ns 1/f_SCL = 1940ns ==> f_SCL = 515kHz==>violation of f_SCL;max=400kHz It's more realistic to calculate with say 50ns < tr,tf < 300ns, than with tt = tf = 0ns or <20ns. Even if with such real tf/tr times, there is cases where f_SCL can be greater than 100/400Hz. I understand what you mean, but that was not my point. See below. And again, all I2C master and slave devices in the bus don't care about f_SCL; what they do care are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying f_SCL is pointless and has no value for HCNT/LCNT calculations. I partially agree: If I2C devices don't care about f_SCL but only about t_r, t_f, t_HIGH and t_LOW there's no need to respect the f_SCL;max constraint. If this is the case, I'm wondering why it is part of the specification, though. With t_r;max and t_f;max, Standard mode: t_r;max 1000ns (max time applied) t_f;max + 300ns (max time applied) t_HIGH;min + 4000ns t_LOW;min + 4700ns 1/f_SCL =1ns ==> f_SCL = 100kHz==> f_SCL;max for Standard-mode Fast mode: t_r;max300ns (max time applied) t_f;max + 300ns (max time applied) t_HIGH;min + 600ns t_LIW;min + 1300ns 1/f_SCL = 2500ns ==> f_SCL = 400kHz==> f_SCL;max for Fast-mode f_SCL;max is defined as a resulting clock frequency with the combination of: (1) the max. conditions of t_r and t_f (2) the min. conditions of t_HIGH and t_LOW We can try to meet t_HIGH;min and t_LOW;min, but we can't meet t_r;min nor t_f;min in the actual systems. The t_r and t_f are _minimum requisites_ for the I/O buffer characteristic of the master and the board designs, hence necessarily contain some time margin. f_SCL is anything more than the resulting speed of (1) + (2), so I don't think we need to adjust HCNT/LCNT values at all. If with t_r < t_r;max and t_f < t_f;max, and you've got a faster clock speed than f_SCL;max, then it's a bonus and we can accept it gratefully. I'd make a compromise proposal; it's fine to make sure whether the resulting f_SCL is within a supported range, but should not make a correction of HCNT/LCNT values. Just report warning messages that some parameters/calculations might be mis-configured an/or wrong. Not sure if this is a good idea: If f_SCL is met by design I'm perfectly happy with dropping the t_HIGH/t_LOW adjustment code, no need to bloat the kernel with confusing warnings. If f_SCL is not automatically met we must either make sure t_HIGH/t_LOW are adjusted or we must take the decision to ignore that constraint and document the reasons behind that decision accordingly. I tried to write my thought down, not sure well-explained, though. Notes: * As long as tHD;SDA issue remains in the DW I2C core, we need to have t_HIGH with a relatively lager value than necessary. In such a case, the resulting f_SCL can never exceed f_SCL;max. * I also wonder which values do you think should be adjusted to meet f_SCL;max, HCNT or LCNT, and why is that? I think it's hard to explain, isn't it? Shinya -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
On 8/21/13 11:39 PM, Christian Ruppert wrote: On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote: On 8/5/13 6:31 PM, Christian Ruppert wrote: On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: As said before, all t_SCL things should go away. Please forget about 100kbps, 400kbps, and so on. Bus/clock speed is totally pointless concept for the I2C bus systems. For example, as long as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a certain I2C bus, it doesn't matter that the resulting clock speed is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. Nobody in the I2C bus doesn't care about actual bus/clock speed. We don't have to care about the resulting bus speed, or rather we should/must not check to see if it's within the proper range. Actually, the I2C specification clearly defines f_SCL;max (and thus implies t_SCL;min), both in the tables and the timing diagrams. Why can we ignore this constraint while having to meet all the others? If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), f_SCL;max will be met by itself. I'm not sure if I agree with this: Standard mode: t_r;min 0ns t_f;min +0ns t_HIGH;min + 4000ns t_LOW;min + 4700ns 1/f_SCL = 8700ns == f_SCL = 115kHz==violation of f_SCL;max=100kHz Fast mode (let's assume V_DD = 5.5V): t_r;min 20ns t_f;min + 20ns t_HIGH;min + 600ns t_LIW;min + 1300ns 1/f_SCL = 1940ns == f_SCL = 515kHz==violation of f_SCL;max=400kHz It's more realistic to calculate with say 50ns tr,tf 300ns, than with tt = tf = 0ns or 20ns. Even if with such real tf/tr times, there is cases where f_SCL can be greater than 100/400Hz. I understand what you mean, but that was not my point. See below. And again, all I2C master and slave devices in the bus don't care about f_SCL; what they do care are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying f_SCL is pointless and has no value for HCNT/LCNT calculations. I partially agree: If I2C devices don't care about f_SCL but only about t_r, t_f, t_HIGH and t_LOW there's no need to respect the f_SCL;max constraint. If this is the case, I'm wondering why it is part of the specification, though. With t_r;max and t_f;max, Standard mode: t_r;max 1000ns (max time applied) t_f;max + 300ns (max time applied) t_HIGH;min + 4000ns t_LOW;min + 4700ns 1/f_SCL =1ns == f_SCL = 100kHz== f_SCL;max for Standard-mode Fast mode: t_r;max300ns (max time applied) t_f;max + 300ns (max time applied) t_HIGH;min + 600ns t_LIW;min + 1300ns 1/f_SCL = 2500ns == f_SCL = 400kHz== f_SCL;max for Fast-mode f_SCL;max is defined as a resulting clock frequency with the combination of: (1) the max. conditions of t_r and t_f (2) the min. conditions of t_HIGH and t_LOW We can try to meet t_HIGH;min and t_LOW;min, but we can't meet t_r;min nor t_f;min in the actual systems. The t_r and t_f are _minimum requisites_ for the I/O buffer characteristic of the master and the board designs, hence necessarily contain some time margin. f_SCL is anything more than the resulting speed of (1) + (2), so I don't think we need to adjust HCNT/LCNT values at all. If with t_r t_r;max and t_f t_f;max, and you've got a faster clock speed than f_SCL;max, then it's a bonus and we can accept it gratefully. I'd make a compromise proposal; it's fine to make sure whether the resulting f_SCL is within a supported range, but should not make a correction of HCNT/LCNT values. Just report warning messages that some parameters/calculations might be mis-configured an/or wrong. Not sure if this is a good idea: If f_SCL is met by design I'm perfectly happy with dropping the t_HIGH/t_LOW adjustment code, no need to bloat the kernel with confusing warnings. If f_SCL is not automatically met we must either make sure t_HIGH/t_LOW are adjusted or we must take the decision to ignore that constraint and document the reasons behind that decision accordingly. I tried to write my thought down, not sure well-explained, though. Notes: * As long as tHD;SDA issue remains in the DW I2C core, we need to have t_HIGH with a relatively lager value than necessary. In such a case, the resulting f_SCL can never exceed f_SCL;max. * I also wonder which values do you think should be adjusted to meet f_SCL;max, HCNT or LCNT, and why is that? I think it's hard to explain, isn't it? Shinya -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] i2c-designware: make HCNT/LCNT values configurable
On 8/19/13 9:07 PM, Mika Westerberg wrote: The DesignWare I2C controller has high count (HCNT) and low count (LCNT) registers for each of the I2C speed modes (standard and fast). These registers are programmed based on the input clock speed in the driver. The current code calculates these values based on the input clock speed and tries hard to meet the I2C bus timing requirements. This could result non-optimal values with regarding to the bus speed. For example on Intel BayTrail we get bus speed of 315.41kHz which is ~20% slower than we would expect (400kHz) in fast mode (even though the timing requirements are met). This patch makes it possible for the platform code to pass more optimal HCNT/LCNT values to the core driver if they are known beforehand. If these are not set we use the calculated and more conservative values. Signed-off-by: Mika Westerberg Looks good, thanks. Acked-by: Shinya Kuribayashi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
Hi, On 8/19/13 8:36 PM, Mika Westerberg wrote: On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote: Actually, the I2C specification clearly defines f_SCL;max (and thus implies t_SCL;min), both in the tables and the timing diagrams. Why can we ignore this constraint while having to meet all the others? If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), f_SCL;max will be met by itself. And again, all I2C master and slave devices in the bus don't care about f_SCL; what they do care are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying f_SCL is pointless and has no value for HCNT/LCNT calculations. One thing that comes to mind regarding the bus speed is that even if we have all the minimal timing requirements met we still prefer resulting bus speeds closer to 400kHz than 315.41kHz for the reasons that we get more data transferred that way, no? That depends I2C slave devices in the bus in your target systems. As long as your slave devices can detect START/STOP conditions and recognize SDA/SCL transitions properly, that should be Ok (you can use HCNT/LCNT settings for 400 kHz without having all the minimal timing requirements met). My comments above was a reply to Christian's snippet code and how to treat f_SCL;mas constraints, and unrelated to your case in question. I'm for having a way to override HCNT/LCNT values as said before, and that should nicely work for you. Shinya -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
Hi, On 8/19/13 8:36 PM, Mika Westerberg wrote: On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote: Actually, the I2C specification clearly defines f_SCL;max (and thus implies t_SCL;min), both in the tables and the timing diagrams. Why can we ignore this constraint while having to meet all the others? If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), f_SCL;max will be met by itself. And again, all I2C master and slave devices in the bus don't care about f_SCL; what they do care are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying f_SCL is pointless and has no value for HCNT/LCNT calculations. One thing that comes to mind regarding the bus speed is that even if we have all the minimal timing requirements met we still prefer resulting bus speeds closer to 400kHz than 315.41kHz for the reasons that we get more data transferred that way, no? That depends I2C slave devices in the bus in your target systems. As long as your slave devices can detect START/STOP conditions and recognize SDA/SCL transitions properly, that should be Ok (you can use HCNT/LCNT settings for 400 kHz without having all the minimal timing requirements met). My comments above was a reply to Christian's snippet code and how to treat f_SCL;mas constraints, and unrelated to your case in question. I'm for having a way to override HCNT/LCNT values as said before, and that should nicely work for you. Shinya -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] i2c-designware: make HCNT/LCNT values configurable
On 8/19/13 9:07 PM, Mika Westerberg wrote: The DesignWare I2C controller has high count (HCNT) and low count (LCNT) registers for each of the I2C speed modes (standard and fast). These registers are programmed based on the input clock speed in the driver. The current code calculates these values based on the input clock speed and tries hard to meet the I2C bus timing requirements. This could result non-optimal values with regarding to the bus speed. For example on Intel BayTrail we get bus speed of 315.41kHz which is ~20% slower than we would expect (400kHz) in fast mode (even though the timing requirements are met). This patch makes it possible for the platform code to pass more optimal HCNT/LCNT values to the core driver if they are known beforehand. If these are not set we use the calculated and more conservative values. Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com Looks good, thanks. Acked-by: Shinya Kuribayashi skuri...@pobox.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
Hi, On 8/5/13 6:31 PM, Christian Ruppert wrote:> On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: As said before, all t_SCL things should go away. Please forget about 100kbps, 400kbps, and so on. Bus/clock speed is totally pointless concept for the I2C bus systems. For example, as long as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a certain I2C bus, it doesn't matter that the resulting clock speed is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. Nobody in the I2C bus doesn't care about actual bus/clock speed. We don't have to care about the resulting bus speed, or rather we should/must not check to see if it's within the proper range. Actually, the I2C specification clearly defines f_SCL;max (and thus implies t_SCL;min), both in the tables and the timing diagrams. Why can we ignore this constraint while having to meet all the others? If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), f_SCL;max will be met by itself. And again, all I2C master and slave devices in the bus don't care about f_SCL; what they do care are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying f_SCL is pointless and has no value for HCNT/LCNT calculations. Is that clear? What is the point to make sure whether f_SCL constraint is met or not? Is there any combination where t_f, t_r, t_HIGH, t_LOW, t_HD;SATA are met, but f_SCL is out of range? I don't think there is. I'd make a compromise proposal; it's fine to make sure whether the resulting f_SCL is within a supported range, but should not make a correction of HCNT/LCNT values. Just report warning messages that some parameters/calculations might be mis-configured an/or wrong. Shinya -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
Hi, On 8/5/13 6:31 PM, Christian Ruppert wrote: On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: As said before, all t_SCL things should go away. Please forget about 100kbps, 400kbps, and so on. Bus/clock speed is totally pointless concept for the I2C bus systems. For example, as long as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a certain I2C bus, it doesn't matter that the resulting clock speed is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. Nobody in the I2C bus doesn't care about actual bus/clock speed. We don't have to care about the resulting bus speed, or rather we should/must not check to see if it's within the proper range. Actually, the I2C specification clearly defines f_SCL;max (and thus implies t_SCL;min), both in the tables and the timing diagrams. Why can we ignore this constraint while having to meet all the others? If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), f_SCL;max will be met by itself. And again, all I2C master and slave devices in the bus don't care about f_SCL; what they do care are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying f_SCL is pointless and has no value for HCNT/LCNT calculations. Is that clear? What is the point to make sure whether f_SCL constraint is met or not? Is there any combination where t_f, t_r, t_HIGH, t_LOW, t_HD;SATA are met, but f_SCL is out of range? I don't think there is. I'd make a compromise proposal; it's fine to make sure whether the resulting f_SCL is within a supported range, but should not make a correction of HCNT/LCNT values. Just report warning messages that some parameters/calculations might be mis-configured an/or wrong. Shinya -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
On 7/22/13 10:17 PM, Christian Ruppert wrote:> On Wed, Jul 17, 2013 at 11:39:58PM +0900, Shinya Kuribayashi wrote: On 7/16/13 8:16 PM, Christian Ruppert wrote:> On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote: Basically, DW I2C core provides a good enough (and quite direct) way to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers. But from my experience (with a slightly old version of DW I2C core around 2005, version 1.06a or so), DW I2C core was apparently lacking in an appropriate hardware mechanism to meet tHD;STA timing spec. We then found that we could meet tHD;STA by increasing *HCNT values, so that's what currently we have in i2c-designware.c I know with that workaround applied, tHIGH is to be configured with a larger value than necessary, but we have no choice. For I2C bus systems, we must meet every timing constraint strictly as required. If DW I2C core cannot meet tHD;STA without the sacrifice of tHIGH, and I would call it a hardware bug. I agree. However, tHD;STA [min] is defined to the same value as tHIGH [min] for all modes in the I2C specification. Do I understand you correctly that the SCL_HCNT parameters control at the same time tHIGH and tHD;STA? *HCNT value does affect both tHIGH and tHD;STA at the same time. But we have to make sure that composition of tHIGH is different from the one of tHD;STA. For tHIGH -- DW I2C core is aware of the SCL rising transition (tr) and can ignore it. It starts counting the HIGH period of the SCL signal (tHIGH) after the SCL input voltage increases at VIH. This is described in the data book as an ideal configuration, and the resulting formula would be: HCNT + (1+4+3) >= IC_CLK * tHIGH ... (a) please refer to the data book for details about (1+4+3) part. For tLOW DW I2C core starts counting the SCL CNTs for the LOW period of the SCL signal (tLOW) as soon as it pulls the SCL line _without_ _confirming_ the SCL input voltage decreases at VIL. In order to meet the tLOW timing spec, we need to take into account the fall time of SCL signal (tf), so the resulting formula should be: LCNT + 1 >= IC_CLK * (tLOW + tf) please refer to the data book for details about '+1' part. For tHD;STA --- There is no explanation about tHD;STA in the data book. We only have (my) experimental result; tHD;STA turned out to be proportional to 'HCNT + 3'. The formula is: HCNT + 3 >= IC_CLK * (tHD;STA + tf) ... (b) DW I2C core seems to start counting the SCL CNTs for the HIGH period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_ line without confirming the SDA input voltage decreases at VIL, so we have to take into account the SDA falling transition (tf) here. As can be expected from (a) and (b), if tHD;STA [min] is defined to the same value as tHIGH [min], we need to have larger HCNT value than necessary for tHIGH, to meet tHD;STA constraint. [...] Hrmmm... This makes my head spin. Do you think the following code snippet represents an accurate method to calculate the register values? If you agree I'll prepare a patch based on that for the DW I2C. The question will be, however, how to obtain the transition times. Please find my comments below. /* *t_f;SDA * |-| * _ _ . . . * \/ * SDA \ / *\/ t_r;SCLt_f;SCL * |-||-| * __ * \ /\ * SCL\ / \ * \_/\___ . . . *|--| |-| || *t_HD;STAt_LOW t_HIGH * * ||---| || * ( HCNT LCNTHCNT ) * 1/f_SYS Composition is: HCNT+3 LCNT+1 HCNT+(1+4+3) The offsets for the HCNT are different in the cases of tHD;STA and t_HIGH. It's based on my experimental result and we can't know how DW I2C core actually counts those period, but it can be easily imagined that for DW I2C core, the way to count t_HD;STA is similar to the way to count t_LOW; it starts counting CNTs as soon as it pulls the SCL/SDA line, then waits for given CNTs. I think your equations and snippet code are based on the assumption that DW I2C core must be counting the number of CNTs for the HIGH period of the SCL signal (that's t_HD;STA and t_HIGH) in the same way, but I don't think so. From my observation and experimental results, it must be different. We couldn't know what actually is, though. * * HCNT = f_SYS * max(t_HD;STA + t_f;SDA , t_HIGH) * = f_SYS * (t_HIGH + t_f;SDA) because T_HD;STA == T_HIGH * LCNT = f_SYS * (t_f;SCL + t_LOW) * HCNT + LCNT + t_r;SCL = 1/f_SCL = t_SCL As said before, all t_SCL things should go away. Please forget about 100kbps, 400kbps, and so on. Bus/clock speed is to
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
On 7/22/13 10:17 PM, Christian Ruppert wrote: On Wed, Jul 17, 2013 at 11:39:58PM +0900, Shinya Kuribayashi wrote: On 7/16/13 8:16 PM, Christian Ruppert wrote: On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote: Basically, DW I2C core provides a good enough (and quite direct) way to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers. But from my experience (with a slightly old version of DW I2C core around 2005, version 1.06a or so), DW I2C core was apparently lacking in an appropriate hardware mechanism to meet tHD;STA timing spec. We then found that we could meet tHD;STA by increasing *HCNT values, so that's what currently we have in i2c-designware.c I know with that workaround applied, tHIGH is to be configured with a larger value than necessary, but we have no choice. For I2C bus systems, we must meet every timing constraint strictly as required. If DW I2C core cannot meet tHD;STA without the sacrifice of tHIGH, and I would call it a hardware bug. I agree. However, tHD;STA [min] is defined to the same value as tHIGH [min] for all modes in the I2C specification. Do I understand you correctly that the SCL_HCNT parameters control at the same time tHIGH and tHD;STA? *HCNT value does affect both tHIGH and tHD;STA at the same time. But we have to make sure that composition of tHIGH is different from the one of tHD;STA. For tHIGH -- DW I2C core is aware of the SCL rising transition (tr) and can ignore it. It starts counting the HIGH period of the SCL signal (tHIGH) after the SCL input voltage increases at VIH. This is described in the data book as an ideal configuration, and the resulting formula would be: HCNT + (1+4+3) = IC_CLK * tHIGH ... (a) please refer to the data book for details about (1+4+3) part. For tLOW DW I2C core starts counting the SCL CNTs for the LOW period of the SCL signal (tLOW) as soon as it pulls the SCL line _without_ _confirming_ the SCL input voltage decreases at VIL. In order to meet the tLOW timing spec, we need to take into account the fall time of SCL signal (tf), so the resulting formula should be: LCNT + 1 = IC_CLK * (tLOW + tf) please refer to the data book for details about '+1' part. For tHD;STA --- There is no explanation about tHD;STA in the data book. We only have (my) experimental result; tHD;STA turned out to be proportional to 'HCNT + 3'. The formula is: HCNT + 3 = IC_CLK * (tHD;STA + tf) ... (b) DW I2C core seems to start counting the SCL CNTs for the HIGH period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_ line without confirming the SDA input voltage decreases at VIL, so we have to take into account the SDA falling transition (tf) here. As can be expected from (a) and (b), if tHD;STA [min] is defined to the same value as tHIGH [min], we need to have larger HCNT value than necessary for tHIGH, to meet tHD;STA constraint. [...] Hrmmm... This makes my head spin. Do you think the following code snippet represents an accurate method to calculate the register values? If you agree I'll prepare a patch based on that for the DW I2C. The question will be, however, how to obtain the transition times. Please find my comments below. /* *t_f;SDA * |-| * _ _ . . . * \/ * SDA \ / *\/ t_r;SCLt_f;SCL * |-||-| * __ * \ /\ * SCL\ / \ * \_/\___ . . . *|--| |-| || *t_HD;STAt_LOW t_HIGH * * ||---| || * ( HCNT LCNTHCNT ) * 1/f_SYS Composition is: HCNT+3 LCNT+1 HCNT+(1+4+3) The offsets for the HCNT are different in the cases of tHD;STA and t_HIGH. It's based on my experimental result and we can't know how DW I2C core actually counts those period, but it can be easily imagined that for DW I2C core, the way to count t_HD;STA is similar to the way to count t_LOW; it starts counting CNTs as soon as it pulls the SCL/SDA line, then waits for given CNTs. I think your equations and snippet code are based on the assumption that DW I2C core must be counting the number of CNTs for the HIGH period of the SCL signal (that's t_HD;STA and t_HIGH) in the same way, but I don't think so. From my observation and experimental results, it must be different. We couldn't know what actually is, though. * * HCNT = f_SYS * max(t_HD;STA + t_f;SDA , t_HIGH) * = f_SYS * (t_HIGH + t_f;SDA) because T_HD;STA == T_HIGH * LCNT = f_SYS * (t_f;SCL + t_LOW) * HCNT + LCNT + t_r;SCL = 1/f_SCL = t_SCL As said before, all t_SCL things should go away. Please forget about 100kbps, 400kbps, and so on. Bus/clock speed is totally pointless concept
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
On 7/16/13 8:16 PM, Christian Ruppert wrote:> On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote: Basically, DW I2C core provides a good enough (and quite direct) way to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers. But from my experience (with a slightly old version of DW I2C core around 2005, version 1.06a or so), DW I2C core was apparently lacking in an appropriate hardware mechanism to meet tHD;STA timing spec. We then found that we could meet tHD;STA by increasing *HCNT values, so that's what currently we have in i2c-designware.c I know with that workaround applied, tHIGH is to be configured with a larger value than necessary, but we have no choice. For I2C bus systems, we must meet every timing constraint strictly as required. If DW I2C core cannot meet tHD;STA without the sacrifice of tHIGH, and I would call it a hardware bug. I agree. However, tHD;STA [min] is defined to the same value as tHIGH [min] for all modes in the I2C specification. Do I understand you correctly that the SCL_HCNT parameters control at the same time tHIGH and tHD;STA? *HCNT value does affect both tHIGH and tHD;STA at the same time. But we have to make sure that composition of tHIGH is different from the one of tHD;STA. For tHIGH -- DW I2C core is aware of the SCL rising transition (tr) and can ignore it. It starts counting the HIGH period of the SCL signal (tHIGH) after the SCL input voltage increases at VIH. This is described in the data book as an ideal configuration, and the resulting formula would be: HCNT + (1+4+3) >= IC_CLK * tHIGH ... (a) please refer to the data book for details about (1+4+3) part. For tLOW DW I2C core starts counting the SCL CNTs for the LOW period of the SCL signal (tLOW) as soon as it pulls the SCL line _without_ _confirming_ the SCL input voltage decreases at VIL. In order to meet the tLOW timing spec, we need to take into account the fall time of SCL signal (tf), so the resulting formula should be: LCNT + 1 >= IC_CLK * (tLOW + tf) please refer to the data book for details about '+1' part. For tHD;STA --- There is no explanation about tHD;STA in the data book. We only have (my) experimental result; tHD;STA turned out to be proportional to 'HCNT + 3'. The formula is: HCNT + 3 >= IC_CLK * (tHD;STA + tf) ... (b) DW I2C core seems to start counting the SCL CNTs for the HIGH period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_ line without confirming the SDA input voltage decreases at VIL, so we have to take into account the SDA falling transition (tf) here. As can be expected from (a) and (b), if tHD;STA [min] is defined to the same value as tHIGH [min], we need to have larger HCNT value than necessary for tHIGH, to meet tHD;STA constraint. Concerning SDA_HOLD_TIME, we have done a few measurements in our lab and it looks like the delay between the falling edge clock of the start condition and the rising edge of the first data bit of the start byte is controlled by this parameter. I conclude that in the drawing below (1) is controlled by SCL_HCNT whereas (2) is controlled by SDA_HOLD_TIME. _ scl \__ ... ___ __ sda \_/ ... |-|---| (1) (2) Thanks for the clarification, understood. Shinya In order to take advantage of those we need some way to pass those values to the i2c designware core. I have two suggestions: - Use the method outlined in this patch and let the interface driver override *CNT values. With *HCNT and *LCNT registers, we can control tHIGH, tLOW, tHD;STA quite accurately. On the other hand, what we can't control with DW I2C core is tr and tf. I assumed that it could never be longer than 300nsec (it's defined as a max. in the I2C specification), so I used it for calculation. I agree that tr and tf are PCB specific, and it would a good first step toward timing optimization to make them configurable through platform data. Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt calculations don't suit with later DW I2C cores, then it would be nice for someone who can access to the data book to update formulas, or provide alternative formulas and make them selectable depending on DW core versions. I'm not having the impression there is a huge difference between the different generations of DW blocks. Probably we can find one formula that suits all blocks. We just have to be careful (in doubt rather conservative) with the transition times. This seems to be currently the case and if I understand Mika correctly, his objective is to remove some of this conservatism. It always helps us to have a way to calculate *HCNT and *LCNT values automatically. As said above, DW I2C core can control tHIGH, tLOW, tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated with proper formulas. It also helps hardware people as wel
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
On 7/16/13 8:16 PM, Christian Ruppert wrote: On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote: Basically, DW I2C core provides a good enough (and quite direct) way to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers. But from my experience (with a slightly old version of DW I2C core around 2005, version 1.06a or so), DW I2C core was apparently lacking in an appropriate hardware mechanism to meet tHD;STA timing spec. We then found that we could meet tHD;STA by increasing *HCNT values, so that's what currently we have in i2c-designware.c I know with that workaround applied, tHIGH is to be configured with a larger value than necessary, but we have no choice. For I2C bus systems, we must meet every timing constraint strictly as required. If DW I2C core cannot meet tHD;STA without the sacrifice of tHIGH, and I would call it a hardware bug. I agree. However, tHD;STA [min] is defined to the same value as tHIGH [min] for all modes in the I2C specification. Do I understand you correctly that the SCL_HCNT parameters control at the same time tHIGH and tHD;STA? *HCNT value does affect both tHIGH and tHD;STA at the same time. But we have to make sure that composition of tHIGH is different from the one of tHD;STA. For tHIGH -- DW I2C core is aware of the SCL rising transition (tr) and can ignore it. It starts counting the HIGH period of the SCL signal (tHIGH) after the SCL input voltage increases at VIH. This is described in the data book as an ideal configuration, and the resulting formula would be: HCNT + (1+4+3) = IC_CLK * tHIGH ... (a) please refer to the data book for details about (1+4+3) part. For tLOW DW I2C core starts counting the SCL CNTs for the LOW period of the SCL signal (tLOW) as soon as it pulls the SCL line _without_ _confirming_ the SCL input voltage decreases at VIL. In order to meet the tLOW timing spec, we need to take into account the fall time of SCL signal (tf), so the resulting formula should be: LCNT + 1 = IC_CLK * (tLOW + tf) please refer to the data book for details about '+1' part. For tHD;STA --- There is no explanation about tHD;STA in the data book. We only have (my) experimental result; tHD;STA turned out to be proportional to 'HCNT + 3'. The formula is: HCNT + 3 = IC_CLK * (tHD;STA + tf) ... (b) DW I2C core seems to start counting the SCL CNTs for the HIGH period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_ line without confirming the SDA input voltage decreases at VIL, so we have to take into account the SDA falling transition (tf) here. As can be expected from (a) and (b), if tHD;STA [min] is defined to the same value as tHIGH [min], we need to have larger HCNT value than necessary for tHIGH, to meet tHD;STA constraint. Concerning SDA_HOLD_TIME, we have done a few measurements in our lab and it looks like the delay between the falling edge clock of the start condition and the rising edge of the first data bit of the start byte is controlled by this parameter. I conclude that in the drawing below (1) is controlled by SCL_HCNT whereas (2) is controlled by SDA_HOLD_TIME. _ scl \__ ... ___ __ sda \_/ ... |-|---| (1) (2) Thanks for the clarification, understood. Shinya In order to take advantage of those we need some way to pass those values to the i2c designware core. I have two suggestions: - Use the method outlined in this patch and let the interface driver override *CNT values. With *HCNT and *LCNT registers, we can control tHIGH, tLOW, tHD;STA quite accurately. On the other hand, what we can't control with DW I2C core is tr and tf. I assumed that it could never be longer than 300nsec (it's defined as a max. in the I2C specification), so I used it for calculation. I agree that tr and tf are PCB specific, and it would a good first step toward timing optimization to make them configurable through platform data. Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt calculations don't suit with later DW I2C cores, then it would be nice for someone who can access to the data book to update formulas, or provide alternative formulas and make them selectable depending on DW core versions. I'm not having the impression there is a huge difference between the different generations of DW blocks. Probably we can find one formula that suits all blocks. We just have to be careful (in doubt rather conservative) with the transition times. This seems to be currently the case and if I understand Mika correctly, his objective is to remove some of this conservatism. It always helps us to have a way to calculate *HCNT and *LCNT values automatically. As said above, DW I2C core can control tHIGH, tLOW, tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated with proper formulas. It also helps hardware people as well to provide reference
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
Hi,
Now I've had a look at the whole discussion.
Basically, DW I2C core provides a good enough (and quite direct) way
to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers.
But from my experience (with a slightly old version of DW I2C core
around 2005, version 1.06a or so), DW I2C core was apparently lacking
in an appropriate hardware mechanism to meet tHD;STA timing spec. We
then found that we could meet tHD;STA by increasing *HCNT values, so
that's what currently we have in i2c-designware.c I know with that
workaround applied, tHIGH is to be configured with a larger value
than necessary, but we have no choice. For I2C bus systems, we must
meet every timing constraint strictly as required. If DW I2C core
cannot meet tHD;STA without the sacrifice of tHIGH, and I would call
it a hardware bug.
On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote:
If I understand the above, it leaves Tf and Tr to be PCB specific and then
these values are passed to the core driver from platform data, right?
That would be the idea: Calculate Th' and Tl' in function of the desired
clock frequency and duty cycle and then adapt these values using
measured transition times.
I think this would be a good solution.
On 7/8/13 10:42 PM, Christian Ruppert wrote:
Anyway, the HCNT, LCNT and SDA hold time values we get from ACPI SSCN/FMCN
methods are measured by our HW guys on a certain board and they have
verified that using those we meet all the I2C timing specs.
In order to take advantage of those we need some way to pass those values
to the i2c designware core. I have two suggestions:
- Use the method outlined in this patch and let the interface driver
override *CNT values.
With *HCNT and *LCNT registers, we can control tHIGH, tLOW, tHD;STA
quite accurately. On the other hand, what we can't control with DW
I2C core is tr and tf. I assumed that it could never be longer than
300nsec (it's defined as a max. in the I2C specification), so I used
it for calculation.
I agree that tr and tf are PCB specific, and it would a good first
step toward timing optimization to make them configurable through
platform data.
Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt
calculations don't suit with later DW I2C cores, then it would be
nice for someone who can access to the data book to update formulas,
or provide alternative formulas and make them selectable depending
on DW core versions.
It always helps us to have a way to calculate *HCNT and *LCNT values
automatically. As said above, DW I2C core can control tHIGH, tLOW,
tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated
with proper formulas. It also helps hardware people as well to
provide reference HCNT/LCNT values.
And as a third step, if we want to use optimized HCNT/LCNT values
which can not be obtained from proper formulas + user-requested
tf/tr, or if we want to use HCNT/LCNT settings verified by vendors
or provided from hardware team, then I'm fine with having a way to
_override_ HCNT/LCNT values. Such direct way might be useful.
- Allow interface drivers to override the function that does calculations
for these values like:
if (dev->std_scl_cnt)
dev->std_scl_cnt(dev, , );
else
/* Fallback to the calculation based solely on iclk */
To make the code having less indentations and be more clear that *CNT
values are being overridden, something like this would be nice (leave
more good comments if necessary I'll leave it to you):
/* set standard and fast speed deviders for high/low periods */
/* Standard-mode */
hcnt = i2c_dw_scl_hcnt(input_clock_khz,
40, /* tHD;STA = tHIGH = 4.0 us */
3, /* tf = 0.3 us */
0, /* 0: DW default, 1: Ideal */
0); /* No offset */
lcnt = i2c_dw_scl_lcnt(input_clock_khz,
47, /* tLOW = 4.7 us */
3, /* tf = 0.3 us */
0); /* No offset */
+ if (dev->ss_hcnt && dev->ss_lcnt) {
+ /* give preference to immediate values over optimal ones */
+ hcnt = dev->ss_hcnt;
+ lcnt = dev->ss_lcnt;
+ }
dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
Shinya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
On 7/11/13 7:13 PM, Mika Westerberg wrote: On Thu, Jul 11, 2013 at 10:36:00AM +0300, Mika Westerberg wrote: On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote: On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote: On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote: What I meant is the following: The clock cycle time Tc is composed of the four components Tc = Th + Tf + Tl + Tr where Th: Time during which the signal is high Tf: Falling edge transition time Tl: Time during which the signal is low Tr: Rising edge transition time The I2C specification specifies a minimum for Tl and Th and a range (or maximum) for Tr and Tf. A maximum frequency is specified as the frequency obtained by adding the minima for Th and Tl to the maxima of Tr ant Tf. Since as you said, transition times are very much PCB dependent, one way to guarantee the max. frequency constraint (and to achieve a constant frequency at its max) is to define the constants Th' = Th + Tf := Th_min + Tf_max Tl' = Tl + Tr := Tl_min + Tr_max and to calculate the variables Th = Th' - Tf Tl = Tl' - Tr in function of Tf and Tr of the given PCB. If I understand the above, it leaves Tf and Tr to be PCB specific and then these values are passed to the core driver from platform data, right? That would be the idea: Calculate Th' and Tl' in function of the desired clock frequency and duty cycle and then adapt these values using measured transition times. What prevented me from implementing this rather academic approach are the following comments in i2c-designware-core.c: When we talk about I2C timing specs, we should not bring up "clock speed" things. All we have to do is to strictly meet timing constraints of tHIGH, tLOW, tHD;SATA, tr, tf, etc. The resulting "clock speed" is not a goal. /* * DesignWare I2C core doesn't seem to have solid strategy to meet * the tHD;STA timing spec. Configuring _HCNT based on tHIGH spec * will result in violation of the tHD;STA spec. */ /* ... * This is just experimental rule; the tHD;STA period * turned out to be proportinal to (_HCNT + 3). With this setting, * we could meet both tHIGH and tHD;STA timing specs. * ... */ If I interpret this right, the slow down of the clock is intentional to meet tHD;STA timing constraints. Correct. Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above comments apply to some earlier version of the IP that didn't have the SDA hold register? If I remember DesignWare APB I2C spec correctly, SDA hold time register doesn't help to meet tHD;STA spec. Could someone confirm it really so with a real hardware, please? Shinya Scratch that. I re-read the spec and tHD;STA is hold time for (repeated) start. There is a constraint that says that the device must internally provide a hold time of at least 300ns for the SDA signal. Maybe that's the constraint the comment above is referring to? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
On 7/11/13 7:13 PM, Mika Westerberg wrote: On Thu, Jul 11, 2013 at 10:36:00AM +0300, Mika Westerberg wrote: On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote: On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote: On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote: What I meant is the following: The clock cycle time Tc is composed of the four components Tc = Th + Tf + Tl + Tr where Th: Time during which the signal is high Tf: Falling edge transition time Tl: Time during which the signal is low Tr: Rising edge transition time The I2C specification specifies a minimum for Tl and Th and a range (or maximum) for Tr and Tf. A maximum frequency is specified as the frequency obtained by adding the minima for Th and Tl to the maxima of Tr ant Tf. Since as you said, transition times are very much PCB dependent, one way to guarantee the max. frequency constraint (and to achieve a constant frequency at its max) is to define the constants Th' = Th + Tf := Th_min + Tf_max Tl' = Tl + Tr := Tl_min + Tr_max and to calculate the variables Th = Th' - Tf Tl = Tl' - Tr in function of Tf and Tr of the given PCB. If I understand the above, it leaves Tf and Tr to be PCB specific and then these values are passed to the core driver from platform data, right? That would be the idea: Calculate Th' and Tl' in function of the desired clock frequency and duty cycle and then adapt these values using measured transition times. What prevented me from implementing this rather academic approach are the following comments in i2c-designware-core.c: When we talk about I2C timing specs, we should not bring up clock speed things. All we have to do is to strictly meet timing constraints of tHIGH, tLOW, tHD;SATA, tr, tf, etc. The resulting clock speed is not a goal. /* * DesignWare I2C core doesn't seem to have solid strategy to meet * the tHD;STA timing spec. Configuring _HCNT based on tHIGH spec * will result in violation of the tHD;STA spec. */ /* ... * This is just experimental rule; the tHD;STA period * turned out to be proportinal to (_HCNT + 3). With this setting, * we could meet both tHIGH and tHD;STA timing specs. * ... */ If I interpret this right, the slow down of the clock is intentional to meet tHD;STA timing constraints. Correct. Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above comments apply to some earlier version of the IP that didn't have the SDA hold register? If I remember DesignWare APB I2C spec correctly, SDA hold time register doesn't help to meet tHD;STA spec. Could someone confirm it really so with a real hardware, please? Shinya Scratch that. I re-read the spec and tHD;STA is hold time for (repeated) start. There is a constraint that says that the device must internally provide a hold time of at least 300ns for the SDA signal. Maybe that's the constraint the comment above is referring to? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
Hi,
Now I've had a look at the whole discussion.
Basically, DW I2C core provides a good enough (and quite direct) way
to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers.
But from my experience (with a slightly old version of DW I2C core
around 2005, version 1.06a or so), DW I2C core was apparently lacking
in an appropriate hardware mechanism to meet tHD;STA timing spec. We
then found that we could meet tHD;STA by increasing *HCNT values, so
that's what currently we have in i2c-designware.c I know with that
workaround applied, tHIGH is to be configured with a larger value
than necessary, but we have no choice. For I2C bus systems, we must
meet every timing constraint strictly as required. If DW I2C core
cannot meet tHD;STA without the sacrifice of tHIGH, and I would call
it a hardware bug.
On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote:
If I understand the above, it leaves Tf and Tr to be PCB specific and then
these values are passed to the core driver from platform data, right?
That would be the idea: Calculate Th' and Tl' in function of the desired
clock frequency and duty cycle and then adapt these values using
measured transition times.
I think this would be a good solution.
On 7/8/13 10:42 PM, Christian Ruppert wrote:
Anyway, the HCNT, LCNT and SDA hold time values we get from ACPI SSCN/FMCN
methods are measured by our HW guys on a certain board and they have
verified that using those we meet all the I2C timing specs.
In order to take advantage of those we need some way to pass those values
to the i2c designware core. I have two suggestions:
- Use the method outlined in this patch and let the interface driver
override *CNT values.
With *HCNT and *LCNT registers, we can control tHIGH, tLOW, tHD;STA
quite accurately. On the other hand, what we can't control with DW
I2C core is tr and tf. I assumed that it could never be longer than
300nsec (it's defined as a max. in the I2C specification), so I used
it for calculation.
I agree that tr and tf are PCB specific, and it would a good first
step toward timing optimization to make them configurable through
platform data.
Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt
calculations don't suit with later DW I2C cores, then it would be
nice for someone who can access to the data book to update formulas,
or provide alternative formulas and make them selectable depending
on DW core versions.
It always helps us to have a way to calculate *HCNT and *LCNT values
automatically. As said above, DW I2C core can control tHIGH, tLOW,
tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated
with proper formulas. It also helps hardware people as well to
provide reference HCNT/LCNT values.
And as a third step, if we want to use optimized HCNT/LCNT values
which can not be obtained from proper formulas + user-requested
tf/tr, or if we want to use HCNT/LCNT settings verified by vendors
or provided from hardware team, then I'm fine with having a way to
_override_ HCNT/LCNT values. Such direct way might be useful.
- Allow interface drivers to override the function that does calculations
for these values like:
if (dev-std_scl_cnt)
dev-std_scl_cnt(dev, hcnt, lcnt);
else
/* Fallback to the calculation based solely on iclk */
To make the code having less indentations and be more clear that *CNT
values are being overridden, something like this would be nice (leave
more good comments if necessary I'll leave it to you):
/* set standard and fast speed deviders for high/low periods */
/* Standard-mode */
hcnt = i2c_dw_scl_hcnt(input_clock_khz,
40, /* tHD;STA = tHIGH = 4.0 us */
3, /* tf = 0.3 us */
0, /* 0: DW default, 1: Ideal */
0); /* No offset */
lcnt = i2c_dw_scl_lcnt(input_clock_khz,
47, /* tLOW = 4.7 us */
3, /* tf = 0.3 us */
0); /* No offset */
+ if (dev-ss_hcnt dev-ss_lcnt) {
+ /* give preference to immediate values over optimal ones */
+ hcnt = dev-ss_hcnt;
+ lcnt = dev-ss_lcnt;
+ }
dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
dev_dbg(dev-dev, Standard-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
Shinya
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: hrtimer: one more expiry time overflow check in hrtimer_interrupt
On 6/28/2013 9:22 PM, Thomas Gleixner wrote: >> On the other hand, we have another call site of tick_program_event() at >> the bottom of hrtimer_interrupt(). The warning this time is triggered >> there, so we need to apply the same fix to it. > > Well, the problem is that you are just papering over the underlying > issue of 32bit systems not being prepared for the year 2038 issue. > > Just blindly silencing the warning is not going to make the system > survive 2038 in any sane way. All timespec/val related time functions > dealing with the clock realtime domain are simply broken in 2038 on > 32bit, so it does not matter whether a warning triggers or not. You're right. With this patch applied, the hrtimer_interrupt /looks/ back to normal, but /proc/timer_list still show that "expires at [in negative range]": active timers: #0: tick_cpu_sched, tick_sched_timer, S:01 # expires at 5081250-5081250 nsecs [in -165398341280 to -165398341280 nsecs] This shouldn't happen and something weird is still going on. > We really need to tackle the underlying problem and not bandaiding a > known to be broken system. Agreed, but a little bit hard task for me. This is 100% reproducible, so I can help debug / verification. Please let me know, if necessary. Thanks for your comments, -- Shinya Kuribayashi Renesas Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: hrtimer: one more expiry time overflow check in hrtimer_interrupt
On 6/28/2013 9:22 PM, Thomas Gleixner wrote: On the other hand, we have another call site of tick_program_event() at the bottom of hrtimer_interrupt(). The warning this time is triggered there, so we need to apply the same fix to it. Well, the problem is that you are just papering over the underlying issue of 32bit systems not being prepared for the year 2038 issue. Just blindly silencing the warning is not going to make the system survive 2038 in any sane way. All timespec/val related time functions dealing with the clock realtime domain are simply broken in 2038 on 32bit, so it does not matter whether a warning triggers or not. You're right. With this patch applied, the hrtimer_interrupt /looks/ back to normal, but /proc/timer_list still show that expires at [in negative range]: active timers: #0: tick_cpu_sched, tick_sched_timer, S:01 # expires at 5081250-5081250 nsecs [in -165398341280 to -165398341280 nsecs] This shouldn't happen and something weird is still going on. We really need to tackle the underlying problem and not bandaiding a known to be broken system. Agreed, but a little bit hard task for me. This is 100% reproducible, so I can help debug / verification. Please let me know, if necessary. Thanks for your comments, -- Shinya Kuribayashi Renesas Electronics -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: hrtimer: one more expiry time overflow check in hrtimer_interrupt
Hello, On 6/12/2013 9:21 PM, Prarit Bhargava wrote: > On 06/11/2013 03:41 AM, Shinya Kuribayashi wrote: >> [ 27.857330] hrtimer: interrupt took 0 ns > > ^^^ see below ... This may be because the frequency of our tick timer source in this system is very slow, it's 32768 Hz. I think it's not suitable for the high resolution timer at all, but we had no choice... And with this patch applied, > diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c > index cdd5607..bc37552 100644 > --- a/kernel/hrtimer.c > +++ b/kernel/hrtimer.c > @@ -1371,6 +1371,8 @@ retry: > tick_program_event(expires_next, 1); > printk_once(KERN_WARNING "hrtimer: interrupt took %llu ns\n", > ktime_to_ns(delta)); > + printk_once(KERN_WARNING "# now=%lld entry_time=%lld\n", > ktime_to_ns(now), ktime_to_ns(entry_time)); > + printk_once(KERN_WARNING "# expires_next=%lld\n", > ktime_to_ns(expires_next)); > } > > /* I've got this: [ 22.857849] hrtimer: interrupt took 0 ns [ 22.861755] # now=-4294967273343634023 entry_time=-4294967273343634023 [ 22.868286] # expires_next=-4294967273343634023 now == entry_time == expires_next, and delta == 0. This means: 1) there haven't been no tick interrupts between 'entry_time' and 'now', 2) delta can be zero, 3) expires_next = ktime_add(now, delta) doesn't move expires_next forward. Ok, it's simply hrtimer is not supposed to work with such a slow timer source. However, this issue happens on other shmobile ARM systems with more faster tick timer source (10--13MHz), see below. >> @@ -1368,6 +1370,8 @@ retry: >> expires_next = ktime_add_ns(now, 100 * NSEC_PER_MSEC); >> else >> expires_next = ktime_add(now, delta); >> +if (expires_next.tv64 < 0) >> +expires_next.tv64 = KTIME_MAX; > > Even with this change you will still see the warning below if delta = 0. Correct. >> tick_program_event(expires_next, 1); >> printk_once(KERN_WARNING "hrtimer: interrupt took %llu ns\n", >> ktime_to_ns(delta)); > > So I'm not sure that this is the correct thing to do. > > Is this reproducible on any ARM system? I'll grab an x86_64 box and give it a > shot there too. Can you dump the values of now, delta, and expires_next when > the printk_once triggers? It's 100% reproducible in our shmobile ARM kernels, v3.4 and v3.10-rc2. And it doesn't reproduce with v2.6.35-based kernels so far. Here're logs from Cortex-A15 UP/SMP systems. They're having ARM architected tiemrs (the Generic Timers, running at 10--13MHz) using as clocksource, clockevents, sched_clock, and read_current_timer (udelay) sources. - machine: R-Mobile APE6 (Cortex-A15 UP system - it's octo-core A15/A7 SoC, but not yet available in mainline) kernel : v3.10-rc2 mainline timer : the Generic Timers (arch/arm/kernel/arch_timer.c) running at 13MHz - |# uname -a |Linux arm 3.10.0-rc2-00966-gd658f9e #1284 Tue Jun 11 13:36:10 JST 2013 armv7l GNU/Linux |# cat /proc/timer_list |egrep "Clock Event Device|event_handler" |Clock Event Device: arch_sys_timer | event_handler: hrtimer_interrupt |# date -s "2038-1-19 3:14:00" |Tue Jan 19 03:14:00 UTC 2038 |# [ 138.942325] [ cut here ] |[ 138.946910] WARNING: at kernel/time/clockevents.c:209 clockevents_program_event+0x130/0x148() |[ 138.955315] Modules linked in: |[ 138.958339] CPU: 0 PID: 0 Comm: swapper Tainted: GW 3.10.0-rc2-00966-gd658f9e #1284 |[...] |[ 139.131970] ---[ end trace f879bec39a6bf85c ]--- |[ 139.136521] hrtimer: interrupt took 2385 ns - machine: R-Car H2 (quad-core Cortex-A15 SMP system) kernel : v3.4-based custom Android kernel timer : the Generic Timers (arch/arm/kernel/arch_timer.c) running at 10MHz - |root@renesas:~# date -s "2038-1-19 3:14:00" |Tue Jan 19 03:14:00 GMT 2038 |root@renesas:~# [ 26.458377] [ cut here ] |[ 26.472214] WARNING: at kernel/time/clockevents.c:209 clockevents_program_event+0x3c/0x138() |[...] |[ 26.980342] ---[ end trace 5218f4fd6603f493 ]--- |[ 26.994166] hrtimer: interrupt took 1900 ns |[ 27.006687] # now=-4294967269549995552 entry_time=-4294967269549997452 |[ 27.026240] # expires_next=-4294967269549993652 I also gave a try on my Ubuntu box, but it seems to work without printk_once() at the bottome of hrtimer_interrupt() printed, which means that hrtimer_update_base()-and-retries mechanism works as expected. $ uname -a Linux RI106878 3.2.0-45-generic #70-
Re: hrtimer: one more expiry time overflow check in hrtimer_interrupt
Hello, On 6/12/2013 9:21 PM, Prarit Bhargava wrote: On 06/11/2013 03:41 AM, Shinya Kuribayashi wrote: [ 27.857330] hrtimer: interrupt took 0 ns ^^^ see below ... This may be because the frequency of our tick timer source in this system is very slow, it's 32768 Hz. I think it's not suitable for the high resolution timer at all, but we had no choice... And with this patch applied, diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index cdd5607..bc37552 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -1371,6 +1371,8 @@ retry: tick_program_event(expires_next, 1); printk_once(KERN_WARNING hrtimer: interrupt took %llu ns\n, ktime_to_ns(delta)); + printk_once(KERN_WARNING # now=%lld entry_time=%lld\n, ktime_to_ns(now), ktime_to_ns(entry_time)); + printk_once(KERN_WARNING # expires_next=%lld\n, ktime_to_ns(expires_next)); } /* I've got this: [ 22.857849] hrtimer: interrupt took 0 ns [ 22.861755] # now=-4294967273343634023 entry_time=-4294967273343634023 [ 22.868286] # expires_next=-4294967273343634023 now == entry_time == expires_next, and delta == 0. This means: 1) there haven't been no tick interrupts between 'entry_time' and 'now', 2) delta can be zero, 3) expires_next = ktime_add(now, delta) doesn't move expires_next forward. Ok, it's simply hrtimer is not supposed to work with such a slow timer source. However, this issue happens on other shmobile ARM systems with more faster tick timer source (10--13MHz), see below. @@ -1368,6 +1370,8 @@ retry: expires_next = ktime_add_ns(now, 100 * NSEC_PER_MSEC); else expires_next = ktime_add(now, delta); +if (expires_next.tv64 0) +expires_next.tv64 = KTIME_MAX; Even with this change you will still see the warning below if delta = 0. Correct. tick_program_event(expires_next, 1); printk_once(KERN_WARNING hrtimer: interrupt took %llu ns\n, ktime_to_ns(delta)); So I'm not sure that this is the correct thing to do. Is this reproducible on any ARM system? I'll grab an x86_64 box and give it a shot there too. Can you dump the values of now, delta, and expires_next when the printk_once triggers? It's 100% reproducible in our shmobile ARM kernels, v3.4 and v3.10-rc2. And it doesn't reproduce with v2.6.35-based kernels so far. Here're logs from Cortex-A15 UP/SMP systems. They're having ARM architected tiemrs (the Generic Timers, running at 10--13MHz) using as clocksource, clockevents, sched_clock, and read_current_timer (udelay) sources. - machine: R-Mobile APE6 (Cortex-A15 UP system - it's octo-core A15/A7 SoC, but not yet available in mainline) kernel : v3.10-rc2 mainline timer : the Generic Timers (arch/arm/kernel/arch_timer.c) running at 13MHz - |# uname -a |Linux arm 3.10.0-rc2-00966-gd658f9e #1284 Tue Jun 11 13:36:10 JST 2013 armv7l GNU/Linux |# cat /proc/timer_list |egrep Clock Event Device|event_handler |Clock Event Device: arch_sys_timer | event_handler: hrtimer_interrupt |# date -s 2038-1-19 3:14:00 |Tue Jan 19 03:14:00 UTC 2038 |# [ 138.942325] [ cut here ] |[ 138.946910] WARNING: at kernel/time/clockevents.c:209 clockevents_program_event+0x130/0x148() |[ 138.955315] Modules linked in: |[ 138.958339] CPU: 0 PID: 0 Comm: swapper Tainted: GW 3.10.0-rc2-00966-gd658f9e #1284 |[...] |[ 139.131970] ---[ end trace f879bec39a6bf85c ]--- |[ 139.136521] hrtimer: interrupt took 2385 ns - machine: R-Car H2 (quad-core Cortex-A15 SMP system) kernel : v3.4-based custom Android kernel timer : the Generic Timers (arch/arm/kernel/arch_timer.c) running at 10MHz - |root@renesas:~# date -s 2038-1-19 3:14:00 |Tue Jan 19 03:14:00 GMT 2038 |root@renesas:~# [ 26.458377] [ cut here ] |[ 26.472214] WARNING: at kernel/time/clockevents.c:209 clockevents_program_event+0x3c/0x138() |[...] |[ 26.980342] ---[ end trace 5218f4fd6603f493 ]--- |[ 26.994166] hrtimer: interrupt took 1900 ns |[ 27.006687] # now=-4294967269549995552 entry_time=-4294967269549997452 |[ 27.026240] # expires_next=-4294967269549993652 I also gave a try on my Ubuntu box, but it seems to work without printk_once() at the bottome of hrtimer_interrupt() printed, which means that hrtimer_update_base()-and-retries mechanism works as expected. $ uname -a Linux RI106878 3.2.0-45-generic #70-Ubuntu SMP Wed May 29 20:12:06 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux I'd like to see whether this happens on other ARM machines excpet for shmobile. Could someone help, please? -- Shinya Kuribayashi Renesas Electronics -- To unsubscribe from this list: send the line
hrtimer: one more expiry time overflow check in hrtimer_interrupt
When executing a date command to set the system date and time to a few seconds before the 2038 problem expiration time, we got a WARN_ON_ONCE() like this: root@renesas:~# date -s "2038-1-19 3:14:00" Tue Jan 19 03:14:00 GMT 2038 (then wait for 7-8 seconds) root@renesas:~# [ 27.662658] [ cut here ] [ 27.667297] WARNING: at kernel/time/clockevents.c:209 clockevents_program_event+0x3c/0x138() [ 27.675720] Modules linked in: [ 27.678802] [] (unwind_backtrace+0x0/0xe0) from [] (warn_slowpath_common+0x4c/0x64) [ 27.688201] [] (warn_slowpath_common+0x4c/0x64) from [] (warn_slowpath_null+0x18/0x1c) [ 27.697845] [] (warn_slowpath_null+0x18/0x1c) from [] (clockevents_program_event+0x3c/0x138) [ 27.708007] [] (clockevents_program_event+0x3c/0x138) from [] (tick_program_event+0x2c/0x34) [ 27.718170] [] (tick_program_event+0x2c/0x34) from [] (hrtimer_interrupt+0x268/0x2a8) [ 27.727752] [] (hrtimer_interrupt+0x268/0x2a8) from [] (cmt_timer_interrupt+0x2c/0x34) [ 27.737396] [] (cmt_timer_interrupt+0x2c/0x34) from [] (handle_irq_event_percpu+0xb0/0x2a8) [ 27.747467] [] (handle_irq_event_percpu+0xb0/0x2a8) from [] (handle_irq_event+0x58/0x74) [ 27.757293] [] (handle_irq_event+0x58/0x74) from [] (handle_fasteoi_irq+0xc0/0x148) [ 27.72] [] (handle_fasteoi_irq+0xc0/0x148) from [] (generic_handle_irq+0x20/0x30) [ 27.776245] [] (generic_handle_irq+0x20/0x30) from [] (handle_IRQ+0x60/0x84) [ 27.785003] [] (handle_IRQ+0x60/0x84) from [] (gic_handle_irq+0x34/0x4c) [ 27.793426] [] (gic_handle_irq+0x34/0x4c) from [] (__irq_svc+0x40/0x70) [ 27.801788] Exception stack(0xc04aff68 to 0xc04affb0) [ 27.806823] ff60: f010 0001 c04ae000 c04ec388 [ 27.815002] ff80: c04b604c c0840d80 40004059 412fc093 c04ce140 c04affb0 [ 27.823150] ffa0: c000f064 c000f068 6013 [ 27.828216] [] (__irq_svc+0x40/0x70) from [] (default_idle+0x24/0x2c) [ 27.836395] [] (default_idle+0x24/0x2c) from [] (cpu_idle+0x74/0xc8) [ 27.844451] [] (cpu_idle+0x74/0xc8) from [] (start_kernel+0x248/0x288) [ 27.852722] ---[ end trace 9d8ad385bde80fd3 ]--- [ 27.857330] hrtimer: interrupt took 0 ns This is triggered with our v3.4-based custom ARM kernel, but we confirmed that v3.10-rc can still have the same problem. I found a similar issue fixed in v3.9 by Prarit Bhargava in commit 8f294b5a13 (hrtimer: Add expiry time overflow check in hrtimer_interrupt, 2013-04-08). It tried to resolve a overflow issue detected around 1970 + 100 seconds. On the other hand, we have another call site of tick_program_event() at the bottom of hrtimer_interrupt(). The warning this time is triggered there, so we need to apply the same fix to it. Reported-by: Hiroyuki Yokoyama Signed-off-by: Shinya Kuribayashi --- Hi Prarit-san and John-san, http://git.kernel.org/linus/8f294b5a139ee4b75e890ad5b443c93d1e558a8b hrtimer: Add expiry time overflow check in hrtimer_interrupt I tried to fix the other case of overflow issues in hrtimer_interrupt(), but not sure it should be worked around like this in the first place. Any comments are appreciated, thanks in advance. diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index cdd5607..a42d712 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -1368,6 +1370,8 @@ retry: expires_next = ktime_add_ns(now, 100 * NSEC_PER_MSEC); else expires_next = ktime_add(now, delta); + if (expires_next.tv64 < 0) + expires_next.tv64 = KTIME_MAX; tick_program_event(expires_next, 1); printk_once(KERN_WARNING "hrtimer: interrupt took %llu ns\n", ktime_to_ns(delta)); -- Shinya Kuribayashi Renesas Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
hrtimer: one more expiry time overflow check in hrtimer_interrupt
When executing a date command to set the system date and time to a few seconds before the 2038 problem expiration time, we got a WARN_ON_ONCE() like this: root@renesas:~# date -s 2038-1-19 3:14:00 Tue Jan 19 03:14:00 GMT 2038 (then wait for 7-8 seconds) root@renesas:~# [ 27.662658] [ cut here ] [ 27.667297] WARNING: at kernel/time/clockevents.c:209 clockevents_program_event+0x3c/0x138() [ 27.675720] Modules linked in: [ 27.678802] [c00130ec] (unwind_backtrace+0x0/0xe0) from [c001f4d8] (warn_slowpath_common+0x4c/0x64) [ 27.688201] [c001f4d8] (warn_slowpath_common+0x4c/0x64) from [c001f508] (warn_slowpath_null+0x18/0x1c) [ 27.697845] [c001f508] (warn_slowpath_null+0x18/0x1c) from [c00549bc] (clockevents_program_event+0x3c/0x138) [ 27.708007] [c00549bc] (clockevents_program_event+0x3c/0x138) from [c005510c] (tick_program_event+0x2c/0x34) [ 27.718170] [c005510c] (tick_program_event+0x2c/0x34) from [c003fa98] (hrtimer_interrupt+0x268/0x2a8) [ 27.727752] [c003fa98] (hrtimer_interrupt+0x268/0x2a8) from [c00180c8] (cmt_timer_interrupt+0x2c/0x34) [ 27.737396] [c00180c8] (cmt_timer_interrupt+0x2c/0x34) from [c0066748] (handle_irq_event_percpu+0xb0/0x2a8) [ 27.747467] [c0066748] (handle_irq_event_percpu+0xb0/0x2a8) from [c0066998] (handle_irq_event+0x58/0x74) [ 27.757293] [c0066998] (handle_irq_event+0x58/0x74) from [c0068f24] (handle_fasteoi_irq+0xc0/0x148) [ 27.72] [c0068f24] (handle_fasteoi_irq+0xc0/0x148) from [c0066014] (generic_handle_irq+0x20/0x30) [ 27.776245] [c0066014] (generic_handle_irq+0x20/0x30) from [c000ef54] (handle_IRQ+0x60/0x84) [ 27.785003] [c000ef54] (handle_IRQ+0x60/0x84) from [c0009334] (gic_handle_irq+0x34/0x4c) [ 27.793426] [c0009334] (gic_handle_irq+0x34/0x4c) from [c000e2c0] (__irq_svc+0x40/0x70) [ 27.801788] Exception stack(0xc04aff68 to 0xc04affb0) [ 27.806823] ff60: f010 0001 c04ae000 c04ec388 [ 27.815002] ff80: c04b604c c0840d80 40004059 412fc093 c04ce140 c04affb0 [ 27.823150] ffa0: c000f064 c000f068 6013 [ 27.828216] [c000e2c0] (__irq_svc+0x40/0x70) from [c000f068] (default_idle+0x24/0x2c) [ 27.836395] [c000f068] (default_idle+0x24/0x2c) from [c000f338] (cpu_idle+0x74/0xc8) [ 27.844451] [c000f338] (cpu_idle+0x74/0xc8) from [c048c6d4] (start_kernel+0x248/0x288) [ 27.852722] ---[ end trace 9d8ad385bde80fd3 ]--- [ 27.857330] hrtimer: interrupt took 0 ns This is triggered with our v3.4-based custom ARM kernel, but we confirmed that v3.10-rc can still have the same problem. I found a similar issue fixed in v3.9 by Prarit Bhargava in commit 8f294b5a13 (hrtimer: Add expiry time overflow check in hrtimer_interrupt, 2013-04-08). It tried to resolve a overflow issue detected around 1970 + 100 seconds. On the other hand, we have another call site of tick_program_event() at the bottom of hrtimer_interrupt(). The warning this time is triggered there, so we need to apply the same fix to it. Reported-by: Hiroyuki Yokoyama hiroyuki.yokoyama...@renesas.com Signed-off-by: Shinya Kuribayashi shinya.kuribayashi...@renesas.com --- Hi Prarit-san and John-san, http://git.kernel.org/linus/8f294b5a139ee4b75e890ad5b443c93d1e558a8b hrtimer: Add expiry time overflow check in hrtimer_interrupt I tried to fix the other case of overflow issues in hrtimer_interrupt(), but not sure it should be worked around like this in the first place. Any comments are appreciated, thanks in advance. diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index cdd5607..a42d712 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -1368,6 +1370,8 @@ retry: expires_next = ktime_add_ns(now, 100 * NSEC_PER_MSEC); else expires_next = ktime_add(now, delta); + if (expires_next.tv64 0) + expires_next.tv64 = KTIME_MAX; tick_program_event(expires_next, 1); printk_once(KERN_WARNING hrtimer: interrupt took %llu ns\n, ktime_to_ns(delta)); -- Shinya Kuribayashi Renesas Electronics -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] ARM: export read_current_timer
On 8/23/2012 2:58 AM, Will Deacon wrote: > On Wed, Aug 22, 2012 at 06:57:20PM +0100, Stephen Boyd wrote: >> On 08/22/12 10:49, Will Deacon wrote: >>> On the topic of the timer stuff: Shinya/Stephen, did you have a chance to >>> look at the registration stuff that was proposed? I'm happy to push it if >>> people will actually use it. >> >> Yes I have tested it on our internal trees and it looks good. I plan to >> send a patch to move MSM's timers over to it later this week so that we >> have at least two users upstream. And I think other A9 MPcore platforms, namely OMAP and EXYNOS, would also be candidates, who tried to skip calibrate_delay() in the past (OMAP) or currently provide non-smp_twd timers as localtimers (EXYNOS). I may miss the latest status of those BSPs, but believe that we would have more users (>2) in the future. > Awesome, I'll dust that series off at -rc3 then. It works for me for weeks without troubles, looking forward to it. -- Shinya Kuribayashi Renesas Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] ARM: export read_current_timer
On 8/23/2012 2:49 AM, Will Deacon wrote: > On Wed, Aug 22, 2012 at 06:15:14PM +0100, Stephen Boyd wrote: >> On 08/22/12 07:29, Arnd Bergmann wrote: >>> read_current_timer is used in the get_cycles() function when >>> ARM_ARCH_TIMER is set, and that function can be inlined into >>> driver modules, so we should export the function to avoid >>> errors like >>> >>> ERROR: "read_current_timer" [drivers/video/udlfb.ko] undefined! >>> ERROR: "read_current_timer" [crypto/tcrypt.ko] undefined! >>> >>> Signed-off-by: Arnd Bergmann >>> Cc: Shinya Kuribayashi >> >> Acked-by: Stephen Boyd >> >> I ran into this last week but forgot to send the patch. Thanks. > > Looks good to me, thanks Arnd: > > Acked-by: Will Deacon I haven't hit with this so far with our configs though, but why not? Acked-by: Shinya Kuribayashi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] ARM: export read_current_timer
On 8/23/2012 2:49 AM, Will Deacon wrote: On Wed, Aug 22, 2012 at 06:15:14PM +0100, Stephen Boyd wrote: On 08/22/12 07:29, Arnd Bergmann wrote: read_current_timer is used in the get_cycles() function when ARM_ARCH_TIMER is set, and that function can be inlined into driver modules, so we should export the function to avoid errors like ERROR: read_current_timer [drivers/video/udlfb.ko] undefined! ERROR: read_current_timer [crypto/tcrypt.ko] undefined! Signed-off-by: Arnd Bergmann a...@arndb.de Cc: Shinya Kuribayashi shinya.kuribayashi...@renesas.com Acked-by: Stephen Boyd sb...@codeaurora.org I ran into this last week but forgot to send the patch. Thanks. Looks good to me, thanks Arnd: Acked-by: Will Deacon will.dea...@arm.com I haven't hit with this so far with our configs though, but why not? Acked-by: Shinya Kuribayashi shinya.kuribayashi...@renesas.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] ARM: export read_current_timer
On 8/23/2012 2:58 AM, Will Deacon wrote: On Wed, Aug 22, 2012 at 06:57:20PM +0100, Stephen Boyd wrote: On 08/22/12 10:49, Will Deacon wrote: On the topic of the timer stuff: Shinya/Stephen, did you have a chance to look at the registration stuff that was proposed? I'm happy to push it if people will actually use it. Yes I have tested it on our internal trees and it looks good. I plan to send a patch to move MSM's timers over to it later this week so that we have at least two users upstream. And I think other A9 MPcore platforms, namely OMAP and EXYNOS, would also be candidates, who tried to skip calibrate_delay() in the past (OMAP) or currently provide non-smp_twd timers as localtimers (EXYNOS). I may miss the latest status of those BSPs, but believe that we would have more users (2) in the future. Awesome, I'll dust that series off at -rc3 then. It works for me for weeks without troubles, looking forward to it. -- Shinya Kuribayashi Renesas Electronics -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
