i2c-sh_mobile non-urgent changes
Subsystems (3)
cover i2c-sh_mobile non-urgent changes
Hello,
This is the first batch to fix the i2c-sh_mobile driver. As a first step, this comprises of the SCL optimization work and a simple fix to annoying spurious WAIT interrupt issue; in other words, this does not change tx/rx data handling logic.
This driver has an architectural problem with send/receive procedures and needs a fundamental overhaul. I've been working on splitting into logical chunks, but not yet completed.
Patches are prepared against the vanilla v3.6. I don't have a chance to give it a try with v3.6+ kernels myself, but tend to be optimistic about that. I have been cooking these patches over a year, and they work perfectly fine with older kernels ..v3.4 so far.
Shinya Kuribayashi (5):
i2c: i2c-sh_mobile: calculate clock parameters at driver probing time
i2c: i2c-sh_mobile: optimize ICCH/ICCL values according to I2C bus speed
i2c: i2c-sh_mobile: fix ICCH to avoid violation of the tHD;STA timing spec
i2c: i2c-sh_mobile: support I2C hardware block with a faster operating clock
i2c: i2c-sh_mobile: fix spurious transfer request timed out
drivers/i2c/busses/i2c-sh_mobile.c | 150 ++++++++++++++++++++++++------------- include/linux/i2c/i2c-sh_mobile.h | 1 + 2 files changed, 98 insertions(+), 53 deletions(-)
--
Shinya Kuribayashi
Renesas Electronics
PATCH 1/5 i2c: i2c-sh_mobile: calculate clock parameters at driver probing time
Currently SCL clock parameters (ICCH/ICCL) are calculated in activate_ch(), which gets called every time sh_mobile_i2c_xfer() is processed, while each I2C bus speed is system-defined and in general those parameters do not have to be updated over I2C transactions.
The only reason I could see having it transaction-time is to adjust ICCH/ICCL values according to the operating frequency of the I2C hardware block, in the face of DFS (Dynamic Frequency Scaling).
However, this won't be necessary.
The operating frequency of the I2C hardware block can change _even_ in the middle of I2C transactions. There is no way to prevent it from happening, and I2C hardware block can work with such dynamic frequency change, of course.
Another is that ICCH/ICCL clock parameters optimized for the faster operating frequency, can also be applied to the slower operating frequency, as long as slave devices work. However, the converse is not true. It would violate SCL timing specs of the I2C standard.
What we can do now is to calculate the ICCH/ICCL clock parameters according to the fastest operating clock of the I2C hardware block. And if that's the case, that calculation should be done just once at driver-module-init time.
This patch moves ICCH/ICCL calculating part from activate_ch() into sh_mobile_i2c_init(), and call it from sh_mobile_i2c_probe().
Note that sh_mobile_i2c_init() just prepares clock parameters using the clock rate and platform data provided, but does _not_ make any hardware I/O accesses. We don't have to care about run-time PM maintenance here.
--- drivers/i2c/busses/i2c-sh_mobile.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-sh_mobile.c +13 -5
@@ -187,18 +187,15 @@ static void iic_set_clr(struct sh_mobile_i2c_data *pd, int offs,iic_wr (pd, offs, (iic_rd (pd, offs) | set) & ~clr); } -static void activate_ch (struct sh_mobile_i2c_data *pd) +static void sh_mobile_i2c_init (struct sh_mobile_i2c_data *pd) {unsigned long i2c_clk;u_int32_t num;u_int32_t denom;u_int32_t tmp; -/* Wake up device and enable clock */ -pm_runtime_get_sync (pd->dev); -clk_enable (pd->clk); -/* Get clock rate after clock is enabled */ +clk_enable (pd->clk); i2c_clk =clk_get_rate (pd->clk);/* Calculate the value for iccl. From the data sheet:
@@ -239,6 +236,15 @@ static void activate_ch(struct sh_mobile_i2c_data *pd) pd->icic &= ~ICIC_ICCHB8 ; } +clk_disable (pd->clk); +} + +static void activate_ch (struct sh_mobile_i2c_data *pd) +{ +/* Wake up device and enable clock */ +pm_runtime_get_sync (pd->dev); +clk_enable (pd->clk); +/* Enable channel and configure rx ack */ iic_set_clr (pd,ICCR ,ICCR_ICE ,0 );
@@ -632,6 +638,8 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)if (size >0x17 ) pd->flags |=IIC_FLAG_HAS_ICIC67 ; +sh_mobile_i2c_init (pd); +/* Enable Runtime PM for this device. * * Also tell the Runtime PM core to ignore children
Signature · 1 line
1.7.12.4
PATCH 2/5 i2c: i2c-sh_mobile: optimize ICCH/ICCL values according to I2C bus speed
In reply to Shinya KuribayashiICCH/ICCL values is supposed to be calculated/optimized to strictly meet the timing specs required by the I2C standard. The resulting I2C bus speed does not matter at all, if it's less than 100 or 400 kHz.
Also fix a typo in the comment, print icch/iccl values at probe, etc.
--- drivers/i2c/busses/i2c-sh_mobile.c | 121 ++++++++++++++++++++++--------------- 1 file changed, 72 insertions(+), 49 deletions(-)
drivers/i2c/busses/i2c-sh_mobile.c +72 -49
@@ -122,9 +122,9 @@ struct sh_mobile_i2c_data {unsigned long bus_speed;struct clk *clk;u_int8_t icic; -u_int8_t iccl; -u_int8_t icch;u_int8_t flags; +u_int16_t iccl; +u_int16_t icch;spinlock_t lock;wait_queue_head_t wait;
@@ -135,7 +135,8 @@ struct sh_mobile_i2c_data {#define IIC_FLAG_HAS_ICIC67 (1 << 0) -#define NORMAL_SPEED 100000/* FAST_SPEED 400000 */ +#define STANDARD_MODE 100000 +#define FAST_MODE 400000/* Register offsets */ #define ICDR 0x00
@@ -187,55 +188,76 @@ static void iic_set_clr(struct sh_mobile_i2c_data *pd, int offs,iic_wr (pd, offs, (iic_rd (pd, offs) | set) & ~clr); } +static u32 sh_mobile_i2c_iccl (unsigned long count_khz,u32 tLOW,u32 tf,int offset) +{ +/* +* Conditional expression: +* ICCL >= COUNT_CLK * (tLOW + tf) +* +* SH-Mobile IIC hardware starts counting the LOW period of +* the SCL signal (tLOW) as soon as it pulls the SCL line. +* In order to meet the tLOW timing spec, we need to take into +* account the fall time of SCL signal (tf). Default tf value +* should be 0.3 us, for safety. +*/ +return (((count_khz * (tLOW + tf)) +5000 ) /10000 ) + offset; +} + +static u32 sh_mobile_i2c_icch (unsigned long count_khz,u32 tHIGH,int offset) +{ +/* +* Conditional expression: +* ICCH >= COUNT_CLK * tHIGH +* +* SH-Mobile IIC hardware is aware of SCL transition period 'tr', +* and can ignore it. SH-Mobile IIC controller starts counting +* the HIGH period of the SCL signal (tHIGH) after the SCL input +* voltage increases at VIH. +*/ +return ((count_khz * tHIGH) +5000 ) /10000 + offset; +} +static void sh_mobile_i2c_init (struct sh_mobile_i2c_data *pd) { -unsigned long i2c_clk; -u_int32_t num; -u_int32_t denom; -u_int32_t tmp; +unsigned long i2c_clk_khz; +u32 tHIGH, tLOW, tf; +int offset;/* Get clock rate after clock is enabled */ clk_enable (pd->clk); - i2c_clk =clk_get_rate (pd->clk); - -/* Calculate the value for iccl. From the data sheet: -* iccl = (p clock / transfer rate) * (L / (L + H)) -* where L and H are the SCL low/high ratio (5/4 in this case). -* We also round off the result. -*/ - num = i2c_clk *5 ; - denom = pd->bus_speed *9 ; - tmp = num *10 / denom; -if (tmp %10 >=5 ) - pd->iccl = (u_int8_t)((num/denom) +1 ); -else - pd->iccl = (u_int8_t)(num/denom); - -/* one more bit of ICCL in ICIC */ -if (pd->flags &IIC_FLAG_HAS_ICIC67 ) { -if ((num/denom) >0xff ) - pd->icic |=ICIC_ICCLB8 ; -else - pd->icic &= ~ICIC_ICCLB8 ; + i2c_clk_khz =clk_get_rate (pd->clk) /1000 ; + +if (pd->bus_speed ==STANDARD_MODE ) { + tLOW =47 ;/* tLOW = 4.7 us */ + tHIGH =40 ;/* tHD;STA = tHIGH = 4.0 us */ + tf =3 ;/* tf = 0.3 us */ + offset =0 ;/* No offset */ + }else if (pd->bus_speed ==FAST_MODE ) { + tLOW =13 ;/* tLOW = 1.3 us */ + tHIGH =6 ;/* tHD;STA = tHIGH = 0.6 us */ + tf =3 ;/* tf = 0.3 us */ + offset =0 ;/* No offset */ + }else { +dev_err (pd->dev,"unrecognized bus speed %lu Hz\n" , + pd->bus_speed); + goto out; } -/* Calculate the value for icch. From the data sheet: -icch = (p clock / transfer rate) * (H / (L + H)) */ - num = i2c_clk *4 ; - tmp = num *10 / denom; -if (tmp %10 >=5 ) - pd->icch = (u_int8_t)((num/denom) +1 ); + pd->iccl =sh_mobile_i2c_iccl (i2c_clk_khz, tLOW, tf, offset); +/* one more bit of ICCL in ICIC */ +if ((pd->iccl >0xff ) && (pd->flags &IIC_FLAG_HAS_ICIC67 )) + pd->icic |=ICIC_ICCLB8 ;else - pd->icch = (u_int8_t)(num/denom); + pd->icic &= ~ICIC_ICCLB8 ; + pd->icch =sh_mobile_i2c_icch (i2c_clk_khz, tHIGH, offset);/* one more bit of ICCH in ICIC */ -if (pd->flags &IIC_FLAG_HAS_ICIC67 ) { -if ((num/denom) >0xff ) - pd->icic |=ICIC_ICCHB8 ; -else - pd->icic &= ~ICIC_ICCHB8 ; - } +if ((pd->icch >0xff ) && (pd->flags &IIC_FLAG_HAS_ICIC67 )) + pd->icic |=ICIC_ICCHB8 ; +else + pd->icic &= ~ICIC_ICCHB8 ; +out:clk_disable (pd->clk); }
@@ -252,8 +274,8 @@ static void activate_ch(struct sh_mobile_i2c_data *pd)iic_wr (pd,ICIC ,0 );/* Set the clock */ -iic_wr (pd,ICCL , pd->iccl); -iic_wr (pd,ICCH , pd->icch); +iic_wr (pd,ICCL , pd->iccl &0xff ); +iic_wr (pd,ICCH , pd->icch &0xff ); }static void deactivate_ch (struct sh_mobile_i2c_data *pd)
@@ -457,8 +479,8 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg)iic_set_clr (pd,ICCR ,ICCR_ICE ,0 );/* Set the clock */ -iic_wr (pd,ICCL , pd->iccl); -iic_wr (pd,ICCH , pd->icch); +iic_wr (pd,ICCL , pd->iccl &0xff ); +iic_wr (pd,ICCH , pd->icch &0xff ); pd->msg = usr_msg; pd->pos =-1 ;
@@ -627,8 +649,8 @@ static int sh_mobile_i2c_probe(struct platform_device *dev) goto err_irq; } -/* Use platformd data bus speed or NORMAL_SPEED */ - pd->bus_speed =NORMAL_SPEED ; +/* Use platform data bus speed or STANDARD_MODE */ + pd->bus_speed =STANDARD_MODE ;if (pdata && pdata->bus_speed) pd->bus_speed = pdata->bus_speed;
@@ -675,8 +697,9 @@ static int sh_mobile_i2c_probe(struct platform_device *dev) goto err_all; } -dev_info (&dev->dev,"I2C adapter %d with bus speed %lu Hz\n" , - adap->nr, pd->bus_speed); +dev_info (&dev->dev, +"I2C adapter %d with bus speed %lu Hz (L/H=%x/%x)\n" , + adap->nr, pd->bus_speed, pd->iccl, pd->icch);of_i2c_register_devices (adap);return 0 ;
Signature · 1 line
1.7.12.4
PATCH 3/5 i2c: i2c-sh_mobile: fix ICCH to avoid violation of the tHD;STA timing spec
In reply to Shinya KuribayashiThe optimized ICCH/ICCL values in the previous commit afterward turned out to violate tHD;STA timing spec. We had to take into account the fall time of SDA signal (tf) at START condition. Fix it accordingly.
With this change, sh_mobile_i2c_icch() is virtually identical to sh_mobile_i2c_iccl(), but they're providing good descriptions of SH-/R-Mobile I2C hardware spec, and I'd leave them as separated.
--- drivers/i2c/busses/i2c-sh_mobile.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-sh_mobile.c +9 -4
@@ -203,18 +203,23 @@ static u32 sh_mobile_i2c_iccl(unsigned long count_khz, u32 tLOW, u32 tf, int offreturn (((count_khz * (tLOW + tf)) +5000 ) /10000 ) + offset; } -static u32 sh_mobile_i2c_icch (unsigned long count_khz,u32 tHIGH,int offset) +static u32 sh_mobile_i2c_icch (unsigned long count_khz,u32 tHIGH,u32 tf,int offset) {/* * Conditional expression: -* ICCH >= COUNT_CLK * tHIGH +* ICCH >= COUNT_CLK * (tHIGH + tf) * * SH-Mobile IIC hardware is aware of SCL transition period 'tr', * and can ignore it. SH-Mobile IIC controller starts counting * the HIGH period of the SCL signal (tHIGH) after the SCL input * voltage increases at VIH. +* +* Afterward it turned out calculating ICCH using only tHIGH spec +* will result in violation of the tHD;STA timing spec. We need +* to take into account the fall time of SDA signal (tf) at START +* condition, in order to meet both tHIGH and tHD;STA specs. */ -return ((count_khz * tHIGH) +5000 ) /10000 + offset; +return (((count_khz * (tHIGH + tf)) +5000 ) /10000 ) + offset; }static void sh_mobile_i2c_init (struct sh_mobile_i2c_data *pd)
@@ -250,7 +255,7 @@ static void sh_mobile_i2c_init(struct sh_mobile_i2c_data *pd)else pd->icic &= ~ICIC_ICCLB8 ; - pd->icch =sh_mobile_i2c_icch (i2c_clk_khz, tHIGH, offset); + pd->icch =sh_mobile_i2c_icch (i2c_clk_khz, tHIGH, tf, offset);/* one more bit of ICCH in ICIC */ if ((pd->icch >0xff ) && (pd->flags &IIC_FLAG_HAS_ICIC67 )) pd->icic |=ICIC_ICCHB8 ;
Signature · 1 line
1.7.12.4
PATCH 4/5 i2c: i2c-sh_mobile: support I2C hardware block with a faster operating clock
In reply to Shinya KuribayashiOn newer SH-/R-Mobile SoCs, a clock supply to the I2C hardware block, which is used to generate the SCL clock output, is getting faster than before, while on the other hand, the SCL clock control registers, ICCH and ICCL, stay unchanged in 9-bit-wide (8+1).
On such silicons, the internal SCL clock counter gets incremented every 2 clocks of the operating clock.
This patch makes it configurable through platform data.
--- drivers/i2c/busses/i2c-sh_mobile.c | 5 +++++ include/linux/i2c/i2c-sh_mobile.h | 1 + 2 files changed, 6 insertions(+)
drivers/i2c/busses/i2c-sh_mobile.c +5 -0
@@ -120,6 +120,7 @@ struct sh_mobile_i2c_data {void __iomem *reg;struct i2c_adapter adap;unsigned long bus_speed; +unsigned int clks_per_count;struct clk *clk;u_int8_t icic;u_int8_t flags;
@@ -231,6 +232,7 @@ static void sh_mobile_i2c_init(struct sh_mobile_i2c_data *pd)/* Get clock rate after clock is enabled */ clk_enable (pd->clk); i2c_clk_khz =clk_get_rate (pd->clk) /1000 ; + i2c_clk_khz /= pd->clks_per_count;if (pd->bus_speed ==STANDARD_MODE ) { tLOW =47 ;/* tLOW = 4.7 us */
@@ -658,6 +660,9 @@ static int sh_mobile_i2c_probe(struct platform_device *dev) pd->bus_speed =STANDARD_MODE ;if (pdata && pdata->bus_speed) pd->bus_speed = pdata->bus_speed; + pd->clks_per_count =1 ; +if (pdata && pdata->clks_per_count) + pd->clks_per_count = pdata->clks_per_count;/* The IIC blocks on SH-Mobile ARM processors * come with two new bits in ICIC.
include/linux/i2c/i2c-sh_mobile.h +1 -0
@@ -5,6 +5,7 @@struct i2c_sh_mobile_platform_data {unsigned long bus_speed; +unsigned int clks_per_count; };#endif /* __I2C_SH_MOBILE_H__ */
Signature · 1 line
1.7.12.4
PATCH 5/5 i2c: i2c-sh_mobile: fix spurious transfer request timed out
In reply to Shinya KuribayashiEnsure that any of preceding register write operations to the I2C hardware block reached the module, and the write data is reflected in the registers, before leaving the interrupt handler.
Otherwise, we'll suffer from spurious WAIT interrupts that lead to 'Transfer request timed out' message, and the transaction failed.
--- drivers/i2c/busses/i2c-sh_mobile.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/i2c/busses/i2c-sh_mobile.c +3 -0
@@ -469,6 +469,9 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id)wake_up (&pd->wait); } +/* defeat write posting to avoid spurious WAIT interrupts */ +iic_rd (pd,ICSR ); +return IRQ_HANDLED ; }
Signature · 1 line
1.7.12.4
cover i2c-sh_mobile non-urgent changes
In reply to Shinya KuribayashiQuoted 7 lines · Shinya Kuribayashi
Hello,
This is the first batch to fix the i2c-sh_mobile driver. As a first step, this comprises of the SCL optimization work and a simple fix to annoying spurious WAIT interrupt issue; in other words, this does not change tx/rx data handling logic.
Quoted 8 lines
This driver has an architectural problem with send/receive procedures and needs a fundamental overhaul. I've been working on splitting into logical chunks, but not yet completed.
Patches are prepared against the vanilla v3.6. I don't have a chance to give it a try with v3.6+ kernels myself, but tend to be optimistic about that. I have been cooking these patches over a year, and they work perfectly fine with older kernels ..v3.4 so far.
Looks good to me, like the extensive patch descriptions. I am going to apply it to for-next with patches 2+3 squashed, because after patch 2 we would have a buggy state otherwise. Let me know if you are not okay with that.
Thanks,
Wolfram
Signature · 9 lines
Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 198 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/3d2de104/attachment-0001.sig>
cover i2c-sh_mobile non-urgent changes
Quoted 4 lines · Wolfram Sang
Looks good to me, like the extensive patch descriptions. I am going to apply it to for-next with patches 2+3 squashed, because after patch 2 we would have a buggy state otherwise. Let me know if you are not okay with that.
It's not that buggy as you/we think, in most cases it work without problem. What's more important for me is to record the issue and how to solve it. So I'd like to have patch 2 and 3 separately.
Thank you for your offer anyway,
--
Shinya Kuribayashi
Renesas Electronics
cover i2c-sh_mobile non-urgent changes
Quoted 9 lines · Shinya Kuribayashi
Quoted 4 lines · Wolfram Sang
Looks good to me, like the extensive patch descriptions. I am going to apply it to for-next with patches 2+3 squashed, because after patch 2 we would have a buggy state otherwise. Let me know if you are not okay with that.
It's not that buggy as you/we think, in most cases it work without problem. What's more important for me is to record the issue and how to solve it. So I'd like to have patch 2 and 3 separately.
I agree with Wolfram. For developers it can be important to know their own development process, that's true. But for upstream it is an absolute preference to avoid breakages. So, we should never commit patches, that are known to contain problems. In this case it means, that patches 2 and 3 should be merged. It is good, that the problem has been detected and fixed during development or testing, this gives us a chance to avoid breaking the mainline, which we should certainly use.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/