| Message ID | 1352118223-3796-1-git-send-email-shubhrajyoti@ti.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 05, 2012 at 05:53:36PM +0530, Shubhrajyoti D wrote: > The revision register on OMAP4 is a 16-bit lo and a 16-bit > hi. Currently the driver reads only the lower 8-bits. > Fix the same by preventing the truncating of the rev register > for OMAP4. > > Also use the scheme bit ie bit-14 of the hi register to know if it > is OMAP_I2C_IP_VERSION_2. > > On platforms previous to OMAP4 the offset 0x04 is IE register whose > bit-14 reset value is 0, the code uses the same to its advantage. > > Also since the omap_i2c_read_reg uses reg_map_ip_* a raw_readw is done > to fetch the revision register. > > The dev->regs is populated after reading the rev_hi. A NULL check > has been added in the resume handler to prevent the access before > the setting of the regs. > Reviewed-by: Felipe Balbi <balbi@ti.com> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > v3: Fix the comments. > > drivers/i2c/busses/i2c-omap.c | 61 ++++++++++++++++++++++++++++++++--------- > 1 files changed, 48 insertions(+), 13 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index db31eae..5c6f538 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -49,9 +49,10 @@ > #define OMAP_I2C_OMAP1_REV_2 0x20 > > /* I2C controller revisions present on specific hardware */ > -#define OMAP_I2C_REV_ON_2430 0x36 > -#define OMAP_I2C_REV_ON_3430_3530 0x3C > -#define OMAP_I2C_REV_ON_3630_4430 0x40 > +#define OMAP_I2C_REV_ON_2430 0x00000036 > +#define OMAP_I2C_REV_ON_3430_3530 0x0000003C > +#define OMAP_I2C_REV_ON_3630 0x00000040 > +#define OMAP_I2C_REV_ON_4430_PLUS 0x50400002 > > /* timeout waiting for the controller to respond */ > #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000)) > @@ -202,7 +203,7 @@ struct omap_i2c_dev { > * fifo_size==0 implies no fifo > * if set, should be trsh+1 > */ > - u8 rev; > + u32 rev; > unsigned b_hw:1; /* bad h/w fixes */ > unsigned receiver:1; /* true when we're in receiver mode */ > u16 iestate; /* Saved interrupt register */ > @@ -490,7 +491,7 @@ static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx) > > omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, buf); > > - if (dev->rev < OMAP_I2C_REV_ON_3630_4430) > + if (dev->rev < OMAP_I2C_REV_ON_3630) > dev->b_hw = 1; /* Enable hardware fixes */ > > /* calculate wakeup latency constraint for MPU */ > @@ -1052,6 +1053,16 @@ static const struct of_device_id omap_i2c_of_match[] = { > MODULE_DEVICE_TABLE(of, omap_i2c_of_match); > #endif > > +#define OMAP_I2C_SCHEME(rev) ((rev & 0xc000) >> 14) > + > +#define OMAP_I2C_REV_SCHEME_0_MAJOR(rev) (rev >> 4) > +#define OMAP_I2C_REV_SCHEME_0_MINOR(rev) (rev & 0xf) > + > +#define OMAP_I2C_REV_SCHEME_1_MAJOR(rev) ((rev & 0x0700) >> 7) > +#define OMAP_I2C_REV_SCHEME_1_MINOR(rev) (rev & 0x1f) > +#define OMAP_I2C_SCHEME_0 0 > +#define OMAP_I2C_SCHEME_1 1 > + > static int __devinit > omap_i2c_probe(struct platform_device *pdev) > { > @@ -1064,6 +1075,8 @@ omap_i2c_probe(struct platform_device *pdev) > const struct of_device_id *match; > int irq; > int r; > + u32 rev; > + u16 minor, major; > > /* NOTE: driver uses the static register mapping */ > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > @@ -1117,11 +1130,6 @@ omap_i2c_probe(struct platform_device *pdev) > > dev->reg_shift = (dev->flags >> OMAP_I2C_FLAG_BUS_SHIFT__SHIFT) & 3; > > - if (dev->dtrev == OMAP_I2C_IP_VERSION_2) > - dev->regs = (u8 *)reg_map_ip_v2; > - else > - dev->regs = (u8 *)reg_map_ip_v1; > - > pm_runtime_enable(dev->dev); > pm_runtime_set_autosuspend_delay(dev->dev, OMAP_I2C_PM_TIMEOUT); > pm_runtime_use_autosuspend(dev->dev); > @@ -1130,7 +1138,31 @@ omap_i2c_probe(struct platform_device *pdev) > if (IS_ERR_VALUE(r)) > goto err_free_mem; > > - dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff; > + /* > + * Read the Rev hi bit-[15:14] ie scheme this is 1 indicates ver2. > + * On omap1/3/2 Offset 4 is IE Reg the bit [15:14] is 0 at reset. > + * Also since the omap_i2c_read_reg uses reg_map_ip_* a > + * raw_readw is done. > + */ > + rev = __raw_readw(dev->base + 0x04); > + > + switch (OMAP_I2C_SCHEME(rev)) { > + case OMAP_I2C_SCHEME_0: > + dev->regs = (u8 *)reg_map_ip_v1; > + dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG); > + minor = OMAP_I2C_REV_SCHEME_0_MAJOR(dev->rev); > + major = OMAP_I2C_REV_SCHEME_0_MAJOR(dev->rev); > + break; > + case OMAP_I2C_SCHEME_1: > + /* FALLTHROUGH */ > + default: > + dev->regs = (u8 *)reg_map_ip_v2; > + rev = (rev << 16) | > + omap_i2c_read_reg(dev, OMAP_I2C_IP_V2_REVNB_LO); > + minor = OMAP_I2C_REV_SCHEME_1_MINOR(rev); > + major = OMAP_I2C_REV_SCHEME_1_MAJOR(rev); > + dev->rev = rev; > + } > > dev->errata = 0; > > @@ -1155,7 +1187,7 @@ omap_i2c_probe(struct platform_device *pdev) > > dev->fifo_size = (dev->fifo_size / 2); > > - if (dev->rev < OMAP_I2C_REV_ON_3630_4430) > + if (dev->rev < OMAP_I2C_REV_ON_3630) > dev->b_hw = 1; /* Enable hardware fixes */ > > /* calculate wakeup latency constraint for MPU */ > @@ -1198,7 +1230,7 @@ omap_i2c_probe(struct platform_device *pdev) > } > > dev_info(dev->dev, "bus %d rev%d.%d.%d at %d kHz\n", adap->nr, > - dev->dtrev, dev->rev >> 4, dev->rev & 0xf, dev->speed); > + dev->dtrev, major, minor, dev->speed); > > of_i2c_register_devices(adap); > > @@ -1264,6 +1296,9 @@ static int omap_i2c_runtime_resume(struct device *dev) > struct platform_device *pdev = to_platform_device(dev); > struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); > > + if (!_dev->regs) > + return 0; > + > if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) { > omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0); > omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate); > -- > 1.7.5.4 >
On Mon, Nov 05, 2012 at 05:53:38PM +0530, Shubhrajyoti D wrote: > The dtrev is used only for the comments. Remove the same and use > the scheme instead to know if it is version2. > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> Reviewed-by: Felipe Balbi <balbi@ti.com> > --- > v3: remove the scheme from the commments. > todo: remove the dtrev from hwmod etc. > > drivers/i2c/busses/i2c-omap.c | 12 +++++------- > 1 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 737d843..5f0c06c 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -191,7 +191,6 @@ struct omap_i2c_dev { > u32 latency; /* maximum MPU wkup latency */ > struct pm_qos_request pm_qos_request; > u32 speed; /* Speed of bus in kHz */ > - u32 dtrev; /* extra revision from DT */ > u32 flags; > u16 cmd_err; > u8 *buf; > @@ -1076,7 +1075,7 @@ omap_i2c_probe(struct platform_device *pdev) > int irq; > int r; > u32 rev; > - u16 minor, major; > + u16 minor, major, scheme; > > /* NOTE: driver uses the static register mapping */ > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > @@ -1108,7 +1107,6 @@ omap_i2c_probe(struct platform_device *pdev) > u32 freq = 100000; /* default to 100000 Hz */ > > pdata = match->data; > - dev->dtrev = pdata->rev; > dev->flags = pdata->flags; > > of_property_read_u32(node, "clock-frequency", &freq); > @@ -1117,7 +1115,6 @@ omap_i2c_probe(struct platform_device *pdev) > } else if (pdata != NULL) { > dev->speed = pdata->clkrate; > dev->flags = pdata->flags; > - dev->dtrev = pdata->rev; > } > > dev->dev = &pdev->dev; > @@ -1146,7 +1143,8 @@ omap_i2c_probe(struct platform_device *pdev) > */ > rev = __raw_readw(dev->base + 0x04); > > - switch (OMAP_I2C_SCHEME(rev)) { > + scheme = OMAP_I2C_SCHEME(rev); > + switch (scheme) { > case OMAP_I2C_SCHEME_0: > dev->regs = (u8 *)reg_map_ip_v1; > dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG); > @@ -1230,8 +1228,8 @@ omap_i2c_probe(struct platform_device *pdev) > goto err_unuse_clocks; > } > > - dev_info(dev->dev, "bus %d rev%d.%d.%d at %d kHz\n", adap->nr, > - dev->dtrev, major, minor, dev->speed); > + dev_info(dev->dev, "bus %d rev%d.%d at %d kHz\n", adap->nr, > + major, minor, dev->speed); > > of_i2c_register_devices(adap); > > -- > 1.7.5.4 >
Hi, On Mon, Nov 05, 2012 at 05:53:43PM +0530, Shubhrajyoti D wrote: > Currently after the reset the sysc is written with hardcoded values. > The patch reads the sysc register and writes back the same value > after reset. > > - Some unnecessary rev checks can be optimised. > - Also due to whatever reason the hwmod flags are changed > we will not reset the values. > - In some of the cases the minor values of the 2430 register > is different(0x37) in that case the autoidle setting may be missed. > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 20 +++++--------------- > 1 files changed, 5 insertions(+), 15 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 25f1564..a09acdc 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -302,7 +302,11 @@ static void __omap_i2c_init(struct omap_i2c_dev *dev) > static int omap_i2c_reset(struct omap_i2c_dev *dev) > { > unsigned long timeout; > + u16 sysc; > + > if (dev->rev >= OMAP_I2C_OMAP1_REV_2) { > + sysc = omap_i2c_read_reg(dev, OMAP_I2C_SYSC_REG); > + > /* Disable I2C controller before soft reset */ > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, > omap_i2c_read_reg(dev, OMAP_I2C_CON_REG) & > @@ -324,22 +328,8 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev) > } > > /* SYSC register is cleared by the reset; rewrite it */ > - if (dev->rev == OMAP_I2C_REV_ON_2430) { > - > - omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, > - SYSC_AUTOIDLE_MASK); > + omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, sysc); > > - } else if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) { > - dev->syscstate = SYSC_AUTOIDLE_MASK; > - dev->syscstate |= SYSC_ENAWAKEUP_MASK; > - dev->syscstate |= (SYSC_IDLEMODE_SMART << > - __ffs(SYSC_SIDLEMODE_MASK)); > - dev->syscstate |= (SYSC_CLOCKACTIVITY_FCLK << > - __ffs(SYSC_CLOCKACTIVITY_MASK)); > - > - omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, > - dev->syscstate); > - } not sure if this will work. What about the first time you call reset() ? won't SYSC just contain the reset values ?
On Monday 05 November 2012 07:44 PM, Felipe Balbi wrote: >> - dev->syscstate); >> > - } > not sure if this will work. What about the first time you call reset() ? > won't SYSC just contain the reset values ? No actually the hwmod sets the value.
Hi, On Mon, Nov 05, 2012 at 07:53:45PM +0530, Shubhrajyoti wrote: > On Monday 05 November 2012 07:44 PM, Felipe Balbi wrote: > >> - dev->syscstate); > >> > - } > > not sure if this will work. What about the first time you call reset() ? > > won't SYSC just contain the reset values ? > No actually the hwmod sets the value. ahaaa, ok good. Let's get an Ack from Benoit, then.
On 11/5/2012 3:25 PM, Felipe Balbi wrote: > Hi, > > On Mon, Nov 05, 2012 at 07:53:45PM +0530, Shubhrajyoti wrote: >> On Monday 05 November 2012 07:44 PM, Felipe Balbi wrote: >>>> - dev->syscstate); >>>>> - } >>> not sure if this will work. What about the first time you call reset() ? >>> won't SYSC just contain the reset values ? >> No actually the hwmod sets the value. > > ahaaa, ok good. Let's get an Ack from Benoit, then. Well, in fact, I'm a little bit surprised that we still have to hack the SYSC directly without using an omap_device API. I know that we have to do some weird stuff for reseting that IP, but didn't we already exposed something to allow that? Regards, Benoit
On 11/5/12, Cousson, Benoit <b-cousson@ti.com> wrote: > On 11/5/2012 3:25 PM, Felipe Balbi wrote: >> Hi, >> >> On Mon, Nov 05, 2012 at 07:53:45PM +0530, Shubhrajyoti wrote: >>> On Monday 05 November 2012 07:44 PM, Felipe Balbi wrote: >>>>> - dev->syscstate); >>>>>> - } >>>> not sure if this will work. What about the first time you call reset() >>>> ? >>>> won't SYSC just contain the reset values ? >>> No actually the hwmod sets the value. >> >> ahaaa, ok good. Let's get an Ack from Benoit, then. > > Well, in fact, I'm a little bit surprised that we still have to hack the there was an attempt [1] the pdata stuff may have issues with dt having to deal with more pdata [2] > SYSC directly without using an omap_device API. Paul was not happy with omap device api [3] As far as the patch is concerned it is only getting rid of the hard coded flags and the rev check. > I know that we have to > do some weird stuff for reseting that IP, but didn't we already exposed > something to allow that? > > Regards, > Benoit [1] http://www.spinics.net/lists/linux-i2c/msg06810.html [2] http://www.spinics.net/lists/linux-i2c/msg06937.html [3] http://www.spinics.net/lists/linux-i2c/msg06943.html
On Mon, Nov 5, 2012 at 5:53 PM, Shubhrajyoti D <shubhrajyoti@ti.com> wrote: > > Does the followiing > - Make the revision a 32- bit consisting of rev_lo amd rev_hi each > of 16 bits. > > - Also use the revision register for the erratum i207. > - Refactor the i2c_omap_init code. > > Adds a patch to remove the hardcoding sysc register. Instead > read register ,reset and then writeback the read value. > > Also more cleanup is possible will check on that subsequently. > > Previous discussions can be found > http://www.spinics.net/lists/linux-omap/msg81265.html > > > Tested on OMAP4430sdp ,4460 ,omap3630 ,3430 and omap2430. > > For omap2 testing the below patch was used > [PATCH] ARM: vfp: fix save and restore when running on pre-VFPv3 and CONFIG_VFPv3 set > If there are no further comments can this be considered for next. Thanks and Regards,
On Mon, Nov 05, 2012 at 05:53:35PM +0530, Shubhrajyoti D wrote: > Shubhrajyoti D (8): > i2c: omap: Fix the revision register read > i2c: omap: use revision check for OMAP_I2C_FLAG_APPLY_ERRATA_I207 > i2c: omap: remove the dtrev > ARM: i2c: omap: Remove the i207 errata flag > i2c: omap: re-factor omap_i2c_init function > i2c: omap: make reset a seperate function > i2c: omap: Restore i2c context always > i2c: omap: cleanup the sysc write Pushed the series to for-next, after fixing a trivial merge conflict caused by reverting the QoS patch.
On Wednesday 14 November 2012 05:34 PM, Wolfram Sang wrote: > On Mon, Nov 05, 2012 at 05:53:35PM +0530, Shubhrajyoti D wrote: > >> Shubhrajyoti D (8): >> i2c: omap: Fix the revision register read >> i2c: omap: use revision check for OMAP_I2C_FLAG_APPLY_ERRATA_I207 >> i2c: omap: remove the dtrev >> ARM: i2c: omap: Remove the i207 errata flag >> i2c: omap: re-factor omap_i2c_init function >> i2c: omap: make reset a seperate function >> i2c: omap: Restore i2c context always >> i2c: omap: cleanup the sysc write > Pushed the series to for-next, after fixing a trivial merge conflict > caused by reverting the QoS patch. Thanks. >
Does the followiing - Make the revision a 32- bit consisting of rev_lo amd rev_hi each of 16 bits. - Also use the revision register for the erratum i207. - Refactor the i2c_omap_init code. Adds a patch to remove the hardcoding sysc register. Instead read register ,reset and then writeback the read value. Also more cleanup is possible will check on that subsequently. Previous discussions can be found http://www.spinics.net/lists/linux-omap/msg81265.html Tested on OMAP4430sdp ,4460 ,omap3630 ,3430 and omap2430. For omap2 testing the below patch was used [PATCH] ARM: vfp: fix save and restore when running on pre-VFPv3 and CONFIG_VFPv3 set Also for using the pm testing below patches are used. arm: sched: stop sched_clock() during suspend ARM: OMAP: hwmod: wait for sysreset complete after enabling hwmod The following changes since commit 3d70f8c617a436c7146ecb81df2265b4626dfe89: Linux 3.7-rc4 (2012-11-04 11:07:39 -0800) are available in the git repository at: git://gitorious.org/linus-tree/linus-tree.git i2c_omap/for_3.8 Shubhrajyoti D (8): i2c: omap: Fix the revision register read i2c: omap: use revision check for OMAP_I2C_FLAG_APPLY_ERRATA_I207 i2c: omap: remove the dtrev ARM: i2c: omap: Remove the i207 errata flag i2c: omap: re-factor omap_i2c_init function i2c: omap: make reset a seperate function i2c: omap: Restore i2c context always i2c: omap: cleanup the sysc write arch/arm/mach-omap2/omap_hwmod_2430_data.c | 3 +- arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 9 +- drivers/i2c/busses/i2c-omap.c | 202 ++++++++++++++++------------ include/linux/i2c-omap.h | 1 - 4 files changed, 118 insertions(+), 97 deletions(-)