Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

2013-06-20 Thread Christian Ruppert 
Hello Wolfram,

On Wed, Jun 19, 2013 at 05:20:59PM +0200, Wolfram Sang wrote:
 [...]
  
   - It should not be encoded in the devicetree, since the flaw is implicit
 to the device, so only the driver needs to know about it. I wonder
 about something like this in the i2c slave driver:
   
 ret = i2c_request_sda_hold_time(client);
   
 The core then can collect the requests and forward them to the host
 driver. This driver then can set up the hardware or return -EOPNOTSUPP
 and we can even warn the user that there might be problems ahead.
  
  This might be a solution but given that many I2C drivers are written as
  an afterthought by device manufacturers and are released under more or
  less open terms of licensing into the wild I doubt this would work very
  well in practise.
 
 Hrmgl, the design looks much better to me, though. Once a driver is
 identified to need this, the core is able to report this requirement to
 a user who might even be unaware of the issue. The dt property has a bit
 of try this if things don't work, you may be lucky taste to it. Need
 to think about it. If PCB design also has an influence, then my idea
 won't work, though.

I agree with that in theory. We would need to define more constraints in
this case, however: Both min and max setup and hold times would be
required for data and start/stop conditions. We have seen cases in which
the hold time we configured was too big for some devices on the bus
which in turn seemed to get confused between data and start/stop
conditions. I don't remember the exact values causing this issue but it
was definitely less than the max of 650ns I have grepped out yesterday.

Addressing this issue properly would mean implementing some simple form
of STA. Not sure if an operating system kernel is the right place for
this, though (and pretty sure that individual device drivers aren't).
These questions are probably better addressed at the PCB design stage.

 [...]
  
In the case of the Designware block, the parameter both changes SDA and
START hold times, however, and you'll find lots of data sheets for
hardware with START hold time requirements on the net, e.g.
http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf
   
   What I couldn't find is a reference manual for a designware IP that
   supports sda hold time? I found some spear SoC which do not have that
   register, so that should surely be reflected in the patchset, too.
  
  If you have access to DesignWare documentation, check out the
  DesignWare DW_apb_i2c Databook Version 1.17a from March 2012.
 
 I don't have, and I do have a hard time finding information about it
 otherwise :(
 
  Unluckily, I clearly don't have the right to share this document with
  you. Do you know the version of the blocks in the spear SoC which do not
  support this register?
 
 ST Spear300 and 600 have 0x3130352a in the version reg according to the
 refman.

Hrmmm... Going through the release notes it looks like this feature was
added with module version 1.11a, released in 2010 (the initial version
of the IP was released in 2003).

The empirical solution in the function i2c_dw_scl_hcnt does not seem to
work in all cases: Our lab guys confirmed that we have several PCB
designs which do not work without adjusting the sda-hold-time parameter
to an appropriate value. The value seems to be different for different
PCBs.
   
   I'd hope that 300ns is a safe value for all PCBs?
  
  Not according to our PCB guys. The highest value I have found in a quick
  check of our device trees is 650ns with others being just slightly above
  300ns.
 
 Thanks for sharing your results \o/ This helps me a lot. Can you find
 out/guess if this value is solely dependend on a slave or is it also
 dependend on PCB layout?

In the simplest form of STA, timings are generally considered a function
of three factors:
  . Drive strength
  . Bus load (Fan out)
  . Internal timings of all devices on the bus (setup/hold times etc.)

This applies to ASIC internal timing. My PCB experience is quite limited
and I'm not sure if this can be applied as such. That said, the I2C
specification does define all three of these factors and I deduce they
all have their importance. We know that the specified internal device
timings are sometimes buggy, I don't know about the other two.

Greetings,
  Christian

-- 
  Christian Ruppert  ,  christian.rupp...@abilis.com
/|
  Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri
 _// | bilis Systems   CH-1228 Plan-les-Ouates


pgpyGsxOFvhEv.pgp
Description: PGP signature

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

2013-06-19 Thread Wolfram Sang 
Hi,

On Wed, Jun 12, 2013 at 04:47:45PM +0200, Christian Ruppert wrote:
 On Mon, Jun 10, 2013 at 05:29:55PM +0200, Wolfram Sang wrote:
  On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote:
   This patch makes the SDA hold time configurable through device tree.
   
   [rebased to i2c-current/i2c-next/mainline-3.10-rc1]
   
   Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
   Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
  
  Hmm, I really have problems adding a generic property. I need to better
  understand why this is needed? What is the usecase? Can't a safe value
  be calculated depending on the bus-speed? Is there a public datasheet
  available somewhere?
 
 I checked with our PCB/Applications team and the data sheets for the
 peripherals in question (DVB demodulator front ends) are under NDA.
 Mika, you seem to be interested in this patch as well. Do you know of
 any publicly available data sheets for hardware requiring this
 adjustment?

So, I looked around and found:
http://www.maximintegrated.com/app-notes/index.mvp/id/3268

which after thinking further about it gives me the following
conclusions:

- sda-hold-time is a property/requirement of a device not following
  the I2C spec. It is not a property of the master!

- It should not be encoded in the devicetree, since the flaw is implicit
  to the device, so only the driver needs to know about it. I wonder
  about something like this in the i2c slave driver:

ret = i2c_request_sda_hold_time(client);

  The core then can collect the requests and forward them to the host
  driver. This driver then can set up the hardware or return -EOPNOTSUPP
  and we can even warn the user that there might be problems ahead.

- I wonder if we really need to have a parameter time-in-ns? The
  specs cleary say 300ns, so I'd think this is the value we should
  always use. This is from a theorhetical pov though, maybe your
  practical experience is different. What values do you need?


 In the case of the Designware block, the parameter both changes SDA and
 START hold times, however, and you'll find lots of data sheets for
 hardware with START hold time requirements on the net, e.g.
 http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf

What I couldn't find is a reference manual for a designware IP that
supports sda hold time? I found some spear SoC which do not have that
register, so that should surely be reflected in the patchset, too.

 The empirical solution in the function i2c_dw_scl_hcnt does not seem to
 work in all cases: Our lab guys confirmed that we have several PCB
 designs which do not work without adjusting the sda-hold-time parameter
 to an appropriate value. The value seems to be different for different
 PCBs.

I'd hope that 300ns is a safe value for all PCBs?

 I suspect that this kind of configurability is not the same for all i2c
 bus master hardware.

Yeah, maybe some do sda-holding by default? Dunno, never checked for
that detail.

Regards,

   Wolfram



signature.asc
Description: Digital signature

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

2013-06-19 Thread Christian Ruppert 
On Wed, Jun 19, 2013 at 11:45:40AM +0200, Wolfram Sang wrote:
 Hi,
 
 On Wed, Jun 12, 2013 at 04:47:45PM +0200, Christian Ruppert wrote:
  On Mon, Jun 10, 2013 at 05:29:55PM +0200, Wolfram Sang wrote:
   On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote:
This patch makes the SDA hold time configurable through device tree.

[rebased to i2c-current/i2c-next/mainline-3.10-rc1]

Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
   
   Hmm, I really have problems adding a generic property. I need to better
   understand why this is needed? What is the usecase? Can't a safe value
   be calculated depending on the bus-speed? Is there a public datasheet
   available somewhere?
  
  I checked with our PCB/Applications team and the data sheets for the
  peripherals in question (DVB demodulator front ends) are under NDA.
  Mika, you seem to be interested in this patch as well. Do you know of
  any publicly available data sheets for hardware requiring this
  adjustment?
 
 So, I looked around and found:
 http://www.maximintegrated.com/app-notes/index.mvp/id/3268
 
 which after thinking further about it gives me the following
 conclusions:
 
 - sda-hold-time is a property/requirement of a device not following
   the I2C spec. It is not a property of the master!

Actually, in a protocol like I2C, every device on the bus must respect
timing constraints like hold time etc. These parameters apply at the
same time to the master and to all slaves.

 - It should not be encoded in the devicetree, since the flaw is implicit
   to the device, so only the driver needs to know about it. I wonder
   about something like this in the i2c slave driver:
 
   ret = i2c_request_sda_hold_time(client);
 
   The core then can collect the requests and forward them to the host
   driver. This driver then can set up the hardware or return -EOPNOTSUPP
   and we can even warn the user that there might be problems ahead.

This might be a solution but given that many I2C drivers are written as
an afterthought by device manufacturers and are released under more or
less open terms of licensing into the wild I doubt this would work very
well in practise.

 - I wonder if we really need to have a parameter time-in-ns? The
   specs cleary say 300ns, so I'd think this is the value we should
   always use. This is from a theorhetical pov though, maybe your
   practical experience is different. What values do you need?

In reality, the I2C specification is more subtle than that: The data
hold time is specified as 0ns with a footnote [3] stating that devices
must internally provide a hold time of at least 300ns for the SDA
signal

Revision 5 contains a relatively understandable explanation about how to
interpret this but earlier versions are less helpful. I think this
confusion is at the root of many timing issues encountered with I2C (and
the reason why Synopsys made this configurable). In fact, especially
earlier specs are _all but_ clear in this point and we cannot assume
that all peripherals were designed after Revision 5 was released in
October 2012.

  In the case of the Designware block, the parameter both changes SDA and
  START hold times, however, and you'll find lots of data sheets for
  hardware with START hold time requirements on the net, e.g.
  http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf
 
 What I couldn't find is a reference manual for a designware IP that
 supports sda hold time? I found some spear SoC which do not have that
 register, so that should surely be reflected in the patchset, too.

If you have access to DesignWare documentation, check out the
DesignWare DW_apb_i2c Databook Version 1.17a from March 2012.
Unluckily, I clearly don't have the right to share this document with
you. Do you know the version of the blocks in the spear SoC which do not
support this register?

  The empirical solution in the function i2c_dw_scl_hcnt does not seem to
  work in all cases: Our lab guys confirmed that we have several PCB
  designs which do not work without adjusting the sda-hold-time parameter
  to an appropriate value. The value seems to be different for different
  PCBs.
 
 I'd hope that 300ns is a safe value for all PCBs?

Not according to our PCB guys. The highest value I have found in a quick
check of our device trees is 650ns with others being just slightly above
300ns.

  I suspect that this kind of configurability is not the same for all i2c
  bus master hardware.
 
 Yeah, maybe some do sda-holding by default? Dunno, never checked for
 that detail.
 
 Regards,
 
Wolfram
 



-- 
  Christian Ruppert  ,  christian.rupp...@abilis.com
/|
  Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri
 _// | bilis Systems   CH-1228 Plan-les-Ouates
--
To unsubscribe from this list: send the line unsubscribe

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

2013-06-19 Thread Wolfram Sang 
Hi Christian,

  So, I looked around and found:
  http://www.maximintegrated.com/app-notes/index.mvp/id/3268
  
  which after thinking further about it gives me the following
  conclusions:
  
  - sda-hold-time is a property/requirement of a device not following
the I2C spec. It is not a property of the master!
 
 Actually, in a protocol like I2C, every device on the bus must respect
 timing constraints like hold time etc. These parameters apply at the
 same time to the master and to all slaves.

Yes. What I meant is: the flaw of one a specific device on the bus
imposes the timing on the whole bus.

 
  - It should not be encoded in the devicetree, since the flaw is implicit
to the device, so only the driver needs to know about it. I wonder
about something like this in the i2c slave driver:
  
  ret = i2c_request_sda_hold_time(client);
  
The core then can collect the requests and forward them to the host
driver. This driver then can set up the hardware or return -EOPNOTSUPP
and we can even warn the user that there might be problems ahead.
 
 This might be a solution but given that many I2C drivers are written as
 an afterthought by device manufacturers and are released under more or
 less open terms of licensing into the wild I doubt this would work very
 well in practise.

Hrmgl, the design looks much better to me, though. Once a driver is
identified to need this, the core is able to report this requirement to
a user who might even be unaware of the issue. The dt property has a bit
of try this if things don't work, you may be lucky taste to it. Need
to think about it. If PCB design also has an influence, then my idea
won't work, though.

  - I wonder if we really need to have a parameter time-in-ns? The
specs cleary say 300ns, so I'd think this is the value we should
always use. This is from a theorhetical pov though, maybe your
practical experience is different. What values do you need?
 
 In reality, the I2C specification is more subtle than that: The data
 hold time is specified as 0ns with a footnote [3] stating that devices
 must internally provide a hold time of at least 300ns for the SDA
 signal
 
 Revision 5 contains a relatively understandable explanation about how to
 interpret this but earlier versions are less helpful. I think this
 confusion is at the root of many timing issues encountered with I2C (and
 the reason why Synopsys made this configurable). In fact, especially
 earlier specs are _all but_ clear in this point and we cannot assume
 that all peripherals were designed after Revision 5 was released in
 October 2012.

OK, agreed. Still surprised that I never encountered such a device,
probably I was just lucky.

 
   In the case of the Designware block, the parameter both changes SDA and
   START hold times, however, and you'll find lots of data sheets for
   hardware with START hold time requirements on the net, e.g.
   http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf
  
  What I couldn't find is a reference manual for a designware IP that
  supports sda hold time? I found some spear SoC which do not have that
  register, so that should surely be reflected in the patchset, too.
 
 If you have access to DesignWare documentation, check out the
 DesignWare DW_apb_i2c Databook Version 1.17a from March 2012.

I don't have, and I do have a hard time finding information about it
otherwise :(

 Unluckily, I clearly don't have the right to share this document with
 you. Do you know the version of the blocks in the spear SoC which do not
 support this register?

ST Spear300 and 600 have 0x3130352a in the version reg according to the
refman.

   The empirical solution in the function i2c_dw_scl_hcnt does not seem to
   work in all cases: Our lab guys confirmed that we have several PCB
   designs which do not work without adjusting the sda-hold-time parameter
   to an appropriate value. The value seems to be different for different
   PCBs.
  
  I'd hope that 300ns is a safe value for all PCBs?
 
 Not according to our PCB guys. The highest value I have found in a quick
 check of our device trees is 650ns with others being just slightly above
 300ns.

Thanks for sharing your results \o/ This helps me a lot. Can you find
out/guess if this value is solely dependend on a slave or is it also
dependend on PCB layout?

Thanks,

   Wolfram



signature.asc
Description: Digital signature

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

2013-06-18 Thread Andy Shevchenko 
On Mon, Jun 17, 2013 at 11:55 PM, Rob Herring robherri...@gmail.com wrote:

 +Optional properties :
 + - sda-hold-time : should contain the SDA hold time in nanoseconds.

 Please specify time units in the property name. Perhaps
 i2c-sda-hold-time-ns.

 Based on reading the discussion, there is one similar property I have
 found: samsung,i2c-sda-delay = 100. I wouldn't copy it as it doesn't
 tell the units or what the delay is.

Often on of the painful issues with DT models is the absence of the properties.
Should someone create the property definition first under
Documentation and _then_ patch everything around?
(Though I think better to think and create documents first and after
program accordingly)

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

2013-06-17 Thread Mika Westerberg 
On Wed, Jun 12, 2013 at 04:47:45PM +0200, Christian Ruppert wrote:
 On Mon, Jun 10, 2013 at 05:29:55PM +0200, Wolfram Sang wrote:
  On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote:
   This patch makes the SDA hold time configurable through device tree.
   
   [rebased to i2c-current/i2c-next/mainline-3.10-rc1]
   
   Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
   Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
  
  Hmm, I really have problems adding a generic property. I need to better
  understand why this is needed? What is the usecase? Can't a safe value
  be calculated depending on the bus-speed? Is there a public datasheet
  available somewhere?
 
 I checked with our PCB/Applications team and the data sheets for the
 peripherals in question (DVB demodulator front ends) are under NDA.
 Mika, you seem to be interested in this patch as well. Do you know of
 any publicly available data sheets for hardware requiring this
 adjustment?

Sorry for the delay, I was on vacation.

I'm not aware of any publicly available datasheets for this block,
unfortunately.

 In the case of the Designware block, the parameter both changes SDA and
 START hold times, however, and you'll find lots of data sheets for
 hardware with START hold time requirements on the net, e.g.
 http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf
 
 The empirical solution in the function i2c_dw_scl_hcnt does not seem to
 work in all cases: Our lab guys confirmed that we have several PCB
 designs which do not work without adjusting the sda-hold-time parameter
 to an appropriate value. The value seems to be different for different
 PCBs.

Yeah, we see the same problem. Our platform guys have solved this so that
they made the SDA hold time available from ACPI to the device driver.

 I suspect that this kind of configurability is not the same for all i2c
 bus master hardware. If you don't want to add a generic property, would
 you accept the patch with the property renamed to dwi2c,sda-hold-time?
 
 Greetings,
   Christian
 
 
 -- 
   Christian Ruppert  ,  christian.rupp...@abilis.com
 /|
   Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri
  _// | bilis Systems   CH-1228 Plan-les-Ouates


--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

2013-06-17 Thread Rob Herring 
On 05/14/2013 08:04 AM, Christian Ruppert wrote:
 This patch makes the SDA hold time configurable through device tree.
 
 [rebased to i2c-current/i2c-next/mainline-3.10-rc1]
 
 Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
 Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
 ---
  .../devicetree/bindings/i2c/i2c-designware.txt |   14 ++
  drivers/i2c/busses/i2c-designware-core.c   |5 +
  drivers/i2c/busses/i2c-designware-core.h   |1 +
  drivers/i2c/busses/i2c-designware-platdrv.c|9 +
  4 files changed, 29 insertions(+), 0 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt 
 b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
 index e42a2ee..21fabe7 100644
 --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
 +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
 @@ -10,6 +10,9 @@ Recommended properties :
  
   - clock-frequency : desired I2C bus clock frequency in Hz.
  
 +Optional properties :
 + - sda-hold-time : should contain the SDA hold time in nanoseconds.

Please specify time units in the property name. Perhaps
i2c-sda-hold-time-ns.

Based on reading the discussion, there is one similar property I have
found: samsung,i2c-sda-delay = 100. I wouldn't copy it as it doesn't
tell the units or what the delay is.

Otherwise, looks fine to me.

Rob

 +
  Example :
  
   i2c@f {
 @@ -20,3 +23,14 @@ Example :
   interrupts = 11;
   clock-frequency = 40;
   };
 +
 + i2c@112 {
 + #address-cells = 1;
 + #size-cells = 0;
 + compatible = snps,designware-i2c;
 + reg = 0x112 0x1000;
 + interrupt-parent = ictl;
 + interrupts = 12 1;
 + clock-frequency = 40;
 + sda-hold-time = 300;
 + };
 diff --git a/drivers/i2c/busses/i2c-designware-core.c 
 b/drivers/i2c/busses/i2c-designware-core.c
 index 21fbb34..91d422c 100644
 --- a/drivers/i2c/busses/i2c-designware-core.c
 +++ b/drivers/i2c/busses/i2c-designware-core.c
 @@ -67,6 +67,7 @@
  #define DW_IC_STATUS 0x70
  #define DW_IC_TXFLR  0x74
  #define DW_IC_RXFLR  0x78
 +#define DW_IC_SDA_HOLD   0x7c
  #define DW_IC_TX_ABRT_SOURCE 0x80
  #define DW_IC_ENABLE_STATUS  0x9c
  #define DW_IC_COMP_PARAM_1   0xf4
 @@ -332,6 +333,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
   dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
   dev_dbg(dev-dev, Fast-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
  
 + /* Configure SDA Hold Time if required */
 + if (dev-sda_hold_time)
 + dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
 +
   /* Configure Tx/Rx FIFO threshold levels */
   dw_writel(dev, dev-tx_fifo_depth - 1, DW_IC_TX_TL);
   dw_writel(dev, 0, DW_IC_RX_TL);
 diff --git a/drivers/i2c/busses/i2c-designware-core.h 
 b/drivers/i2c/busses/i2c-designware-core.h
 index 9c1840e..33dfec3 100644
 --- a/drivers/i2c/busses/i2c-designware-core.h
 +++ b/drivers/i2c/busses/i2c-designware-core.h
 @@ -88,6 +88,7 @@ struct dw_i2c_dev {
   u32 master_cfg;
   unsigned inttx_fifo_depth;
   unsigned intrx_fifo_depth;
 + u32 sda_hold_time;
  };
  
  #define ACCESS_SWAP  0x0001
 diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
 b/drivers/i2c/busses/i2c-designware-platdrv.c
 index 8ec9133..589f414 100644
 --- a/drivers/i2c/busses/i2c-designware-platdrv.c
 +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
 @@ -34,6 +34,7 @@
  #include linux/sched.h
  #include linux/err.h
  #include linux/interrupt.h
 +#include linux/of.h
  #include linux/of_i2c.h
  #include linux/platform_device.h
  #include linux/pm.h
 @@ -120,6 +121,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
   return PTR_ERR(dev-clk);
   clk_prepare_enable(dev-clk);
  
 + if (pdev-dev.of_node) {
 + u32 ht;
 + u32 ic_clk = dev-get_clk_rate_khz(dev);
 +
 + of_property_read_u32(pdev-dev.of_node, sda-hold-time, ht);
 + dev-sda_hold_time = ((u64)ic_clk * ht + 50) / 100;
 + }
 +
   dev-functionality =
   I2C_FUNC_I2C |
   I2C_FUNC_10BIT_ADDR |
 

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

2013-06-12 Thread Christian Ruppert 
On Mon, Jun 10, 2013 at 05:29:55PM +0200, Wolfram Sang wrote:
 On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote:
  This patch makes the SDA hold time configurable through device tree.
  
  [rebased to i2c-current/i2c-next/mainline-3.10-rc1]
  
  Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
  Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
 
 Hmm, I really have problems adding a generic property. I need to better
 understand why this is needed? What is the usecase? Can't a safe value
 be calculated depending on the bus-speed? Is there a public datasheet
 available somewhere?

I checked with our PCB/Applications team and the data sheets for the
peripherals in question (DVB demodulator front ends) are under NDA.
Mika, you seem to be interested in this patch as well. Do you know of
any publicly available data sheets for hardware requiring this
adjustment?

In the case of the Designware block, the parameter both changes SDA and
START hold times, however, and you'll find lots of data sheets for
hardware with START hold time requirements on the net, e.g.
http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf

The empirical solution in the function i2c_dw_scl_hcnt does not seem to
work in all cases: Our lab guys confirmed that we have several PCB
designs which do not work without adjusting the sda-hold-time parameter
to an appropriate value. The value seems to be different for different
PCBs.

I suspect that this kind of configurability is not the same for all i2c
bus master hardware. If you don't want to add a generic property, would
you accept the patch with the property renamed to dwi2c,sda-hold-time?

Greetings,
  Christian


-- 
  Christian Ruppert  ,  christian.rupp...@abilis.com
/|
  Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri
 _// | bilis Systems   CH-1228 Plan-les-Ouates


pgpJq0LvhUoGt.pgp
Description: PGP signature

Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

2013-06-10 Thread Wolfram Sang 
On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote:
 This patch makes the SDA hold time configurable through device tree.
 
 [rebased to i2c-current/i2c-next/mainline-3.10-rc1]
 
 Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
 Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com

Hmm, I really have problems adding a generic property. I need to better
understand why this is needed? What is the usecase? Can't a safe value
be calculated depending on the bus-speed? Is there a public datasheet
available somewhere?

Thanks,

   Wolfram



signature.asc
Description: Digital signature

[PATCH REBASE] i2c-designware: make SDA hold time configurable

2013-05-14 Thread Christian Ruppert 
This patch makes the SDA hold time configurable through device tree.

[rebased to i2c-current/i2c-next/mainline-3.10-rc1]

Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
---
 .../devicetree/bindings/i2c/i2c-designware.txt |   14 ++
 drivers/i2c/busses/i2c-designware-core.c   |5 +
 drivers/i2c/busses/i2c-designware-core.h   |1 +
 drivers/i2c/busses/i2c-designware-platdrv.c|9 +
 4 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt 
b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..21fabe7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,9 @@ Recommended properties :
 
  - clock-frequency : desired I2C bus clock frequency in Hz.
 
+Optional properties :
+ - sda-hold-time : should contain the SDA hold time in nanoseconds.
+
 Example :
 
i2c@f {
@@ -20,3 +23,14 @@ Example :
interrupts = 11;
clock-frequency = 40;
};
+
+   i2c@112 {
+   #address-cells = 1;
+   #size-cells = 0;
+   compatible = snps,designware-i2c;
+   reg = 0x112 0x1000;
+   interrupt-parent = ictl;
+   interrupts = 12 1;
+   clock-frequency = 40;
+   sda-hold-time = 300;
+   };
diff --git a/drivers/i2c/busses/i2c-designware-core.c 
b/drivers/i2c/busses/i2c-designware-core.c
index 21fbb34..91d422c 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,6 +67,7 @@
 #define DW_IC_STATUS   0x70
 #define DW_IC_TXFLR0x74
 #define DW_IC_RXFLR0x78
+#define DW_IC_SDA_HOLD 0x7c
 #define DW_IC_TX_ABRT_SOURCE   0x80
 #define DW_IC_ENABLE_STATUS0x9c
 #define DW_IC_COMP_PARAM_1 0xf4
@@ -332,6 +333,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
dev_dbg(dev-dev, Fast-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
 
+   /* Configure SDA Hold Time if required */
+   if (dev-sda_hold_time)
+   dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
+
/* Configure Tx/Rx FIFO threshold levels */
dw_writel(dev, dev-tx_fifo_depth - 1, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h 
b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..33dfec3 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,7 @@ struct dw_i2c_dev {
u32 master_cfg;
unsigned inttx_fifo_depth;
unsigned intrx_fifo_depth;
+   u32 sda_hold_time;
 };
 
 #define ACCESS_SWAP0x0001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
b/drivers/i2c/busses/i2c-designware-platdrv.c
index 8ec9133..589f414 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
 #include linux/sched.h
 #include linux/err.h
 #include linux/interrupt.h
+#include linux/of.h
 #include linux/of_i2c.h
 #include linux/platform_device.h
 #include linux/pm.h
@@ -120,6 +121,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
return PTR_ERR(dev-clk);
clk_prepare_enable(dev-clk);
 
+   if (pdev-dev.of_node) {
+   u32 ht;
+   u32 ic_clk = dev-get_clk_rate_khz(dev);
+
+   of_property_read_u32(pdev-dev.of_node, sda-hold-time, ht);
+   dev-sda_hold_time = ((u64)ic_clk * ht + 50) / 100;
+   }
+
dev-functionality =
I2C_FUNC_I2C |
I2C_FUNC_10BIT_ADDR |
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html