| Message ID | 1394201571-11681-3-git-send-email-chiau.ee.chew@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Mar 07, 2014 at 10:12:51PM +0800, Chew Chiau Ee wrote: > From: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > On Intel BayTrail, there was case whereby the resulting fast mode > bus speed becomes slower (~20% slower compared to expected speed) > if using the HCNT/LCNT calculated in the core layer. Thus, this > patch is added to allow pci glue layer to pass in optimal > HCNT/LCNT/SDA hold time values to core layer since the core > layer supports cofigurable HCNT/LCNT/SDA hold time values now. > > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com> Can you make use of those instead? u32 sda_falling_time; u32 scl_falling_time; This is more consistent with using sda_hold_time and lets them have a common (and more readable) unit.
> -----Original Message----- > From: Wolfram Sang [mailto:wsa@the-dreams.de] > Sent: Sunday, March 09, 2014 5:04 PM > To: Chew, Chiau Ee > Cc: Mika Westerberg; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 2/2] i2c: designware-pci: set ideal HCNT, LCNT and SDA > hold time value > > On Fri, Mar 07, 2014 at 10:12:51PM +0800, Chew Chiau Ee wrote: > > From: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > > > On Intel BayTrail, there was case whereby the resulting fast mode bus > > speed becomes slower (~20% slower compared to expected speed) if using > > the HCNT/LCNT calculated in the core layer. Thus, this patch is added > > to allow pci glue layer to pass in optimal HCNT/LCNT/SDA hold time > > values to core layer since the core layer supports cofigurable > > HCNT/LCNT/SDA hold time values now. > > > > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > Can you make use of those instead? > > u32 sda_falling_time; > u32 scl_falling_time; > > This is more consistent with using sda_hold_time and lets them have a common > (and more readable) unit. Would like to clarify on your statement above. So you are suggesting to change the following variable name in the code? As in: "u32 ss_hcnt;" to "u32 ss_scl_rising_time" "u32 fs_hcnt;" to "u32 fs_scl_rising_time" "u32 ss_lcnt;" to "u32 ss_scl_falling_time" "u32 fs_lcnt;" to " u32 fs_scl_falling_time" "u32 sda_hold;" to "u32 sda_hold_time" -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 10, 2014 at 11:34:49AM +0000, Chew, Chiau Ee wrote: > > > > -----Original Message----- > > From: Wolfram Sang [mailto:wsa@the-dreams.de] > > Sent: Sunday, March 09, 2014 5:04 PM > > To: Chew, Chiau Ee > > Cc: Mika Westerberg; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH 2/2] i2c: designware-pci: set ideal HCNT, LCNT and SDA > > hold time value > > > > On Fri, Mar 07, 2014 at 10:12:51PM +0800, Chew Chiau Ee wrote: > > > From: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > > > > > On Intel BayTrail, there was case whereby the resulting fast mode bus > > > speed becomes slower (~20% slower compared to expected speed) if using > > > the HCNT/LCNT calculated in the core layer. Thus, this patch is added > > > to allow pci glue layer to pass in optimal HCNT/LCNT/SDA hold time > > > values to core layer since the core layer supports cofigurable > > > HCNT/LCNT/SDA hold time values now. > > > > > > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > > > Can you make use of those instead? > > > > u32 sda_falling_time; > > u32 scl_falling_time; > > > > This is more consistent with using sda_hold_time and lets them have a common > > (and more readable) unit. > > Would like to clarify on your statement above. So you are suggesting to change the following > variable name in the code? As in: Nope. But I had a look and it doesn't make sense to use the above. So, just forget it :) Will send a second review.
On Fri, Mar 07, 2014 at 10:12:51PM +0800, Chew Chiau Ee wrote: > From: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > On Intel BayTrail, there was case whereby the resulting fast mode > bus speed becomes slower (~20% slower compared to expected speed) > if using the HCNT/LCNT calculated in the core layer. Thus, this > patch is added to allow pci glue layer to pass in optimal > HCNT/LCNT/SDA hold time values to core layer since the core > layer supports cofigurable HCNT/LCNT/SDA hold time values now. > > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com> > --- > drivers/i2c/busses/i2c-designware-pcidrv.c | 34 ++++++++++++++++++++++++++++ > 1 files changed, 34 insertions(+), 0 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c > index 87f2fc4..96417ca 100644 > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c > @@ -58,6 +58,14 @@ enum dw_pci_ctl_id_t { > baytrail, > }; > > +struct scl_sda_cfg { > + u32 ss_hcnt; > + u32 fs_hcnt; > + u32 ss_lcnt; > + u32 fs_lcnt; > + u32 sda_hold; > +}; 'dw_' prefix for this struct name, please. > #define INTEL_MID_STD_CFG (DW_IC_CON_MASTER | \ > @@ -77,6 +86,21 @@ struct dw_pci_controller { > I2C_FUNC_SMBUS_WORD_DATA | \ > I2C_FUNC_SMBUS_I2C_BLOCK) > > +/* BayTrail HCNT/LCNT/SDA_HOLD */ > +#define BYT_STD_MODE_HCNT 0x200 > +#define BYT_STD_MODE_LCNT BYT_STD_MODE_HCNT > +#define BYT_FAST_MODE_HCNT 0x55 > +#define BYT_FAST_MODE_LCNT 0x99 > +#define BYT_SDA_HOLD 0x6 I'd think you can use these values directly in the struct. They are not supposed to be used outside anyhow. > +static struct scl_sda_cfg byt_config = { > + .ss_hcnt = BYT_STD_MODE_HCNT, > + .fs_hcnt = BYT_FAST_MODE_HCNT, > + .ss_lcnt = BYT_STD_MODE_LCNT, > + .fs_lcnt = BYT_FAST_MODE_LCNT, > + .sda_hold = BYT_SDA_HOLD, > +}; I'd prefer no tabs before "=", just one space. Thanks, Wolfram
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index 87f2fc4..96417ca 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -58,6 +58,14 @@ enum dw_pci_ctl_id_t { baytrail, }; +struct scl_sda_cfg { + u32 ss_hcnt; + u32 fs_hcnt; + u32 ss_lcnt; + u32 fs_lcnt; + u32 sda_hold; +}; + struct dw_pci_controller { u32 bus_num; u32 bus_cfg; @@ -65,6 +73,7 @@ struct dw_pci_controller { u32 rx_fifo_depth; u32 clk_khz; u32 functionality; + struct scl_sda_cfg *scl_sda_cfg; }; #define INTEL_MID_STD_CFG (DW_IC_CON_MASTER | \ @@ -77,6 +86,21 @@ struct dw_pci_controller { I2C_FUNC_SMBUS_WORD_DATA | \ I2C_FUNC_SMBUS_I2C_BLOCK) +/* BayTrail HCNT/LCNT/SDA_HOLD */ +#define BYT_STD_MODE_HCNT 0x200 +#define BYT_STD_MODE_LCNT BYT_STD_MODE_HCNT +#define BYT_FAST_MODE_HCNT 0x55 +#define BYT_FAST_MODE_LCNT 0x99 +#define BYT_SDA_HOLD 0x6 + +static struct scl_sda_cfg byt_config = { + .ss_hcnt = BYT_STD_MODE_HCNT, + .fs_hcnt = BYT_FAST_MODE_HCNT, + .ss_lcnt = BYT_STD_MODE_LCNT, + .fs_lcnt = BYT_FAST_MODE_LCNT, + .sda_hold = BYT_SDA_HOLD, +}; + static struct dw_pci_controller dw_pci_controllers[] = { [moorestown_0] = { .bus_num = 0, @@ -148,6 +172,7 @@ static struct dw_pci_controller dw_pci_controllers[] = { .rx_fifo_depth = 32, .clk_khz = 100000, .functionality = I2C_FUNC_10BIT_ADDR, + .scl_sda_cfg = &byt_config, }, }; static struct i2c_algorithm i2c_dw_algo = { @@ -231,6 +256,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, struct i2c_adapter *adap; int r; struct dw_pci_controller *controller; + struct scl_sda_cfg *cfg; if (id->driver_data >= ARRAY_SIZE(dw_pci_controllers)) { dev_err(&pdev->dev, "%s: invalid driver data %ld\n", __func__, @@ -268,6 +294,14 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, DW_DEFAULT_FUNCTIONALITY; dev->master_cfg = controller->bus_cfg; + if (controller->scl_sda_cfg) { + cfg = controller->scl_sda_cfg; + dev->ss_hcnt = cfg->ss_hcnt; + dev->fs_hcnt = cfg->fs_hcnt; + dev->ss_lcnt = cfg->ss_lcnt; + dev->fs_lcnt = cfg->fs_lcnt; + dev->sda_hold_time = cfg->sda_hold; + } pci_set_drvdata(pdev, dev);