Re: [PATCH v2] i2c: designware: Do not require clock when SSCN and FFCN are provided

2015-12-18 Thread Mika Westerberg 
On Wed, Dec 16, 2015 at 08:23:45PM -0600, Suravee Suthikulpanit wrote:
> The current driver uses input clock source frequency to calculate
> values for [SS|FS]_[HC|LC] registers. However, when booting ACPI, we do not
> currently have a good way to provide the frequency information.
> Instead, we can leverage the SSCN and FFCN ACPI methods, which can be used
> to directly provide these values. So, the clock information should
> no longer be required during probing.
> 
> However, since clk can be invalid, additional checks must be done where
> we are making use of it.
> 
> Signed-off-by: Suravee Suthikulpanit 

This works fine on Intel Baytrail and Skylake. However, I think we could
do this a bit better still ;-)

> ---
> 
> Note: This has been tested on AMD Seattle RevB for both DT and ACPI.
> 
> Changes in V2:
> In v1, I disregarded the clock if SSCN and FMCN are provided,
> assuming that it was not needed. That was incorrect assumption,
> and is now fixed in v2.
> 
>  drivers/i2c/busses/i2c-designware-platdrv.c | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 8ffc36b..4615fe3 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -44,6 +44,9 @@
>  
>  static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
>  {
> + if (IS_ERR(dev->clk))
> + return 0;
> +
>   return clk_get_rate(dev->clk)/1000;
>  }

So instead of this, what if we do not assign dev->get_clk_rate_khz at
all and then do something like below in the core driver?

Of course we still need the other changes you did in this patch to cope
with the missing clock.

diff --git a/drivers/i2c/busses/i2c-designware-core.c 
b/drivers/i2c/busses/i2c-designware-core.c
index 8c48b27ba059..25dccd8df772 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -271,6 +271,17 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool 
enable)
 enable ? "en" : "dis");
 }
 
+static unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
+{
+   /*
+* Clock is not necessary if we got LCNT/HCNT values directly from
+* the platform code.
+*/
+   if (WARN_ON_ONCE(!dev->get_clk_rate_khz))
+   return 0;
+   return dev->get_clk_rate_khz(dev);
+}
+
 /**
  * i2c_dw_init() - initialize the designware i2c master hardware
  * @dev: device private data
@@ -281,7 +292,6 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool 
enable)
  */
 int i2c_dw_init(struct dw_i2c_dev *dev)
 {
-   u32 input_clock_khz;
u32 hcnt, lcnt;
u32 reg;
u32 sda_falling_time, scl_falling_time;
@@ -295,8 +305,6 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
}
}
 
-   input_clock_khz = dev->get_clk_rate_khz(dev);
-
reg = dw_readl(dev, DW_IC_COMP_TYPE);
if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
/* Configure register endianess access */
@@ -325,12 +333,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
hcnt = dev->ss_hcnt;
lcnt = dev->ss_lcnt;
} else {
-   hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+   hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
4000,   /* tHD;STA = tHIGH = 4.0 us */
sda_falling_time,
0,  /* 0: DW default, 1: Ideal */
0); /* No offset */
-   lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+   lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
4700,   /* tLOW = 4.7 us */
scl_falling_time,
0); /* No offset */
@@ -344,12 +352,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
hcnt = dev->fs_hcnt;
lcnt = dev->fs_lcnt;
} else {
-   hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+   hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
600,/* tHD;STA = tHIGH = 0.6 us */
sda_falling_time,
0,  /* 0: DW default, 1: Ideal */
0); /* No offset */
-   lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+   lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
1300,   /* tLOW = 1.3 us */
scl_falling_time,
0); /* No offset */
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to

Re: [PATCH v6] at24: Support SMBus read/write of 16-bit devices

2015-12-18 Thread Aaron Sierra 
- Original Message -
> From: "Wolfram Sang" 
> Sent: Friday, December 11, 2015 7:19:40 AM
> 
> On Mon, Nov 16, 2015 at 06:02:19PM -0600, Aaron Sierra wrote:
> > Previously, the at24 driver would bail out in the case of a 16-bit
> > addressable EEPROM attached to an SMBus controller. This is because
> > SMBus block reads and writes don't map to I2C multi-byte reads and
> > writes when the offset portion is 2 bytes.
> > 
> > Instead of bailing out, this patch settles for functioning with single
> > byte read SMBus cycles. Writes can be block or single-byte, depending
> > on SMBus controller features.
> > 
> > Read access is not without some risk. Multiple SMBus cycles are
> > required to read even one byte. If the SMBus has multiple masters and
> > one accesses this EEPROM between the dummy address write and the
> > subsequent current-address-read cycle(s), this driver will receive
> > data from the wrong address.
> > 
> > Functionality has been tested with the following devices:
> > 
> > AT24CM01 attached to Intel ISCH SMBus
> > AT24C512 attached to Intel I801 SMBus
> > 
> > Read performance:
> > 3.6 KB/s with 32-byte* access
> > 
> > *limited to 32-bytes by I2C_SMBUS_BLOCK_MAX.
> > 
> > Write performance:
> > 248 B/s with 1-byte page (default)
> > 3.9 KB/s with 128-byte* page (via platform data)
> > 
> > *limited to 31-bytes by I2C_SMBUS_BLOCK_MAX - 1.
> > 
> > Signed-off-by: Nate Case 
> > Signed-off-by: Aaron Sierra 
> > Reviewed-by: Jean Delvare 
> > ---
> >  v2 - Account for changes related to introduction of
> >   i2c_smbus_read_i2c_block_data_or_emulated()
> >  v3 - Consolidate three patches into one
> > - Expand comments regarding SMBus multi-master read risks.
> > - Rely on current-address-read for improved read performance (i.e. one
> >   dummy address write followed by multiple individual byte reads).
> >   This improves performance from 1.4 KiB/s to 3.6 KiB/s.
> > - Use struct at24_data's writebuf instead of kzalloc-ing
> > - Only limit write_max by 1-byte when accessing a 16-bit device with
> >   block writes instead of attempting to preserve a power-of-two.
> > - Style fixes (indentation, parentheses, unnecessary masking, etc.)
> >  v4 - Address 16-bit safety in Kconfig
> > - Set "count" to zero later in at24_smbus_read_block_data()
> > - Fix over-80-columns issues in at24_eeprom_read()
> > - Fix write_max off-by-one in at24_probe()
> > - Check SMBus functionality needed for 16-bit device reads
> > - Homogenize indentation of SMBus functionality checks for SMBus write
> >  v5 - 16-bit device read needs READ_BYTE not READ_BYTE_DATA
> > - Clarify write_max limiting with smbus_max
> > - Add X-ES copyright
> >  v6 - Update comment associated with SMBus functionality testing for 16-bit
> >   device read (READ_BYTE not READ_BYTE_DATA)
> > 
> >  drivers/misc/eeprom/Kconfig |   5 +-
> >  drivers/misc/eeprom/at24.c  | 132
> >  +++-
> >  2 files changed, 122 insertions(+), 15 deletions(-)
> 
> I remember I had a lengthy discussion with Guenter Roeck about 16 bit
> support via smbus. I don't have the bandwidth right now to recap the
> discussion and check if the concerns apply to your implementation as
> well. Could you check this thread
> 
> https://lkml.org/lkml/2015/2/4/436
> 
> and report back if it applies to your patch as well? This would speed up
> the review significantly.

Wolfram,
The real bus-level multi-master issue is still present. However, it is
documented via comments in the driver and warnings in the Kconfig.

The other (and apparently bigger) issue of a single master experiencing
corruption due to simultaneous access through i2c-dev and this driver
appears to have been resolved. The i2cdump, i2cget, and i2cset utilities
refuse to access any device that has a driver bound to it. My attempts
to cause corruption using i2cdump were met with the following message
until I unregistered the at24 driver from the device:

   # i2cdump 8 0x54
   No size specified (using byte-data access)
   Error: Could not set address to 0x54: Device or resource busy

-Aaron S.
--
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 v6] at24: Support SMBus read/write of 16-bit devices

2015-12-18 Thread Jean Delvare 
On Fri, 18 Dec 2015 13:47:50 -0600 (CST), Aaron Sierra wrote:
> - Original Message -
> > From: "Wolfram Sang" 
> > Sent: Friday, December 11, 2015 7:19:40 AM
> >
> > I remember I had a lengthy discussion with Guenter Roeck about 16 bit
> > support via smbus. I don't have the bandwidth right now to recap the
> > discussion and check if the concerns apply to your implementation as
> > well. Could you check this thread
> > 
> > https://lkml.org/lkml/2015/2/4/436
> > 
> > and report back if it applies to your patch as well? This would speed up
> > the review significantly.
> 
> The real bus-level multi-master issue is still present. However, it is
> documented via comments in the driver and warnings in the Kconfig.
> 
> The other (and apparently bigger) issue of a single master experiencing
> corruption due to simultaneous access through i2c-dev and this driver
> appears to have been resolved. The i2cdump, i2cget, and i2cset utilities
> refuse to access any device that has a driver bound to it. My attempts
> to cause corruption using i2cdump were met with the following message
> until I unregistered the at24 driver from the device:
> 
># i2cdump 8 0x54
>No size specified (using byte-data access)
>Error: Could not set address to 0x54: Device or resource busy

You could always bypass the security check and force the access with
option -f. But then there are plenty of ways to corrupt any device that
way, regardless of the driver (or even without any driver bound to the
device, if you run multiple i2c-dev-based user-space tools in
parallel.) So I'd say this is not relevant.

-- 
Jean Delvare
SUSE L3 Support
--
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: [RFC 1/3] i2c: document binding for multi-master case

2015-12-18 Thread Rob Herring 
On Wed, Dec 16, 2015 at 07:44:18PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> We need this binding because some I2C master drivers will need to adapt
> their PM settings for the arbitration circuitry. I skipped the "i2c-"
> prefix because I can imagine to be useful outside of i2c.
> 
> Signed-off-by: Wolfram Sang 
> Cc: devicet...@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/i2c/i2c.txt | 5 +
>  1 file changed, 5 insertions(+)

Acked-by: Rob Herring 
--
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