Message-ID: <20130619152058.GA2951@katana>
Date:	Wed, 19 Jun 2013 17:20:59 +0200
From:	Wolfram Sang <wsa@...-dreams.de>
To:	Christian Ruppert <christian.ruppert@...lis.com>
Cc:	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	linux-i2c@...r.kernel.org,
	"Ben Dooks (embedded platforms)" <ben-linux@...ff.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Rob Landley <rob@...dley.net>,
	devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Vineet Gupta <Vineet.Gupta1@...opsys.com>,
	Pierrick Hascoet <pierrick.hascoet@...lis.com>
Subject: Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable

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


Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists