| Message ID | 201305221219.43558.jbe@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Juergen Beisert, > Hi Fabio, > > > i2c slowness is a different issue. > > Same happens here for my i.M23 based platform. It seems the PIO mode does > not work, or at least not like it works on a i.MX28. Each short transfer > needs about one second (without an error message) but does not send > anything on the I2C lines. > > I need the following patches to make I2C master work within a 3.10-rc2 > kernel: > > Subject: [PATCH] I2C/MXS: distinguish i.MX23 and i.MX28 based I2C I'm all for it, but then ... won't it be better if you actually fixed the PIO and mixed-mode on MX23 instead of implementing such hack? It might actually turn out to be beneficial for MX28 too. Best regards, Marek Vasut
Hi Juergen, On Wed, May 22, 2013 at 7:19 AM, Juergen Beisert <jbe@pengutronix.de> wrote: > Hi Fabio, > >> i2c slowness is a different issue. > > Same happens here for my i.M23 based platform. It seems the PIO mode does not > work, or at least not like it works on a i.MX28. Each short transfer needs > about one second (without an error message) but does not send anything on the > I2C lines. > > I need the following patches to make I2C master work within a 3.10-rc2 kernel: Thanks, I tried forcing dma mode and I got the following probe issue on a mx28evk running today's linux-next: [ 6.403193] mxs-sgtl5000 sound.12: ASoC: CODEC (null) not registered [ 6.412175] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-517) [ 6.425582] platform sound.12: Driver mxs-sgtl5000 requests probe deferral [ 6.434043] mmcblk0: p1 p2 p3 [ 6.439586] stmp3xxx-rtc 80056000.rtc: setting system clock to 2013-04-23 21:00:42 UTC (1366750842) [ 6.458141] mxs-sgtl5000 sound.12: ASoC: CODEC (null) not registered [ 6.464773] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-517) If I run linux-next with 2f769432d8554 reverted then I get a 23 seconds delay in the probe (1 second for each sgtl5000 register write). Regards, Fabio Estevam
Hi Marek, Marek Vasut wrote: > > > i2c slowness is a different issue. > > > > Same happens here for my i.M23 based platform. It seems the PIO mode does > > not work, or at least not like it works on a i.MX28. Each short transfer > > needs about one second (without an error message) but does not send > > anything on the I2C lines. > > > > I need the following patches to make I2C master work within a 3.10-rc2 > > kernel: > > > > Subject: [PATCH] I2C/MXS: distinguish i.MX23 and i.MX28 based I2C > > I'm all for it, but then ... won't it be better if you actually fixed the > PIO and mixed-mode on MX23 instead of implementing such hack? If the PIO mode or my patch is a hack depends on the point of view: Lucas told me the PIO mode is *mentioned* but *not specified* in the i.MX23/i.MX28 datasheets. So, the PIO mode seems to depend on some undocumented status bits in the i.MX28 I2C controller implementation. Regards, Juergen
Dear Juergen Beisert, > Hi Marek, > > Marek Vasut wrote: > > > > i2c slowness is a different issue. > > > > > > Same happens here for my i.M23 based platform. It seems the PIO mode > > > does not work, or at least not like it works on a i.MX28. Each short > > > transfer needs about one second (without an error message) but does > > > not send anything on the I2C lines. > > > > > > I need the following patches to make I2C master work within a 3.10-rc2 > > > kernel: > > > > > > Subject: [PATCH] I2C/MXS: distinguish i.MX23 and i.MX28 based I2C > > > > I'm all for it, but then ... won't it be better if you actually fixed the > > PIO and mixed-mode on MX23 instead of implementing such hack? > > If the PIO mode or my patch is a hack depends on the point of view: Lucas > told me the PIO mode is *mentioned* but *not specified* in the > i.MX23/i.MX28 datasheets. The PIO works the same way DMA does -- set up bits and then pump data into the DATA register. > So, the PIO mode seems to depend on some undocumented status bits in the > i.MX28 I2C controller implementation. How would DMA work then if it used undocumented registers ? It's in the documentation, just read it or ask FSL ;-) Best regards, Marek Vasut
Am Donnerstag, den 23.05.2013, 16:48 +0200 schrieb Marek Vasut: > Dear Juergen Beisert, > > > Hi Marek, > > > > Marek Vasut wrote: > > > > > i2c slowness is a different issue. > > > > > > > > Same happens here for my i.M23 based platform. It seems the PIO mode > > > > does not work, or at least not like it works on a i.MX28. Each short > > > > transfer needs about one second (without an error message) but does > > > > not send anything on the I2C lines. > > > > > > > > I need the following patches to make I2C master work within a 3.10-rc2 > > > > kernel: > > > > > > > > Subject: [PATCH] I2C/MXS: distinguish i.MX23 and i.MX28 based I2C > > > > > > I'm all for it, but then ... won't it be better if you actually fixed the > > > PIO and mixed-mode on MX23 instead of implementing such hack? > > > > If the PIO mode or my patch is a hack depends on the point of view: Lucas > > told me the PIO mode is *mentioned* but *not specified* in the > > i.MX23/i.MX28 datasheets. > > The PIO works the same way DMA does -- set up bits and then pump data into the > DATA register. > > So, the PIO mode seems to depend on some undocumented status bits in the > > i.MX28 I2C controller implementation. > > How would DMA work then if it used undocumented registers ? It's in the > documentation, just read it or ask FSL ;-) > While the PIO mode might use the same controller mechanisms as the DMA mode, PIO mode is _not_ a documented mode of operation for the i.MX23. To quote the i.MX28 RM: "The I2C block on the i.MX28 supports a new PIO mode or soft-DMA mode.", which implies the PIO mode to be a new mode of operation not found on earlier i.MX SoCs. The doc is slightly fuzzy here as the i.MX23 RM in contrary states: "Short transmission (up to three bytes plus address) can be easily triggered using only PIO operations, i.e., no DMA setup required." But again it's not a documented mode of operation, i.MX23 doc only describes the DMA mode. So while we _might_ be able to get the PIO mode to work on the i.MX23 there is nothing in the doc stating that it's even meant to work. Even while PIO and DMA mode use the same internal mechanisms, there's still plenty of opportunities of fail in there. After all PIO mode relies on reading a debug register in the course of normal operation. Only more extensive experimentation could show if we are in fact able to make it work, a first shot of using PIO mode on MX23 failed, so it might as well be that Juergens quick fix is correct and we have to disable PIO mode on MX23 altogether. That said please stop slapping the word "hack" over this patch until proven otherwise. Regards, Lucas
Hi, On 23/05/2013 17:28, Lucas Stach wrote: > Am Donnerstag, den 23.05.2013, 16:48 +0200 schrieb Marek Vasut: >> Dear Juergen Beisert, >> >>> Hi Marek, >>> >>> Marek Vasut wrote: >>>>>> i2c slowness is a different issue. >>>>> Same happens here for my i.M23 based platform. It seems the PIO mode >>>>> does not work, or at least not like it works on a i.MX28. Each short >>>>> transfer needs about one second (without an error message) but does >>>>> not send anything on the I2C lines. >>>>> >>>>> I need the following patches to make I2C master work within a 3.10-rc2 >>>>> kernel: >>>>> >>>>> Subject: [PATCH] I2C/MXS: distinguish i.MX23 and i.MX28 based I2C >>>> I'm all for it, but then ... won't it be better if you actually fixed the >>>> PIO and mixed-mode on MX23 instead of implementing such hack? >>> If the PIO mode or my patch is a hack depends on the point of view: Lucas >>> told me the PIO mode is *mentioned* but *not specified* in the >>> i.MX23/i.MX28 datasheets. >> The PIO works the same way DMA does -- set up bits and then pump data into the >> DATA register. >>> So, the PIO mode seems to depend on some undocumented status bits in the >>> i.MX28 I2C controller implementation. >> How would DMA work then if it used undocumented registers ? It's in the >> documentation, just read it or ask FSL ;-) >> > While the PIO mode might use the same controller mechanisms as the DMA > mode, PIO mode is _not_ a documented mode of operation for the i.MX23. > > To quote the i.MX28 RM: "The I2C block on the i.MX28 supports a new PIO > mode or soft-DMA mode.", which implies the PIO mode to be a new mode of > operation not found on earlier i.MX SoCs. > The doc is slightly fuzzy here as the i.MX23 RM in contrary states: > "Short transmission (up to three bytes plus address) can be easily > triggered using only PIO operations, i.e., no DMA setup required." But > again it's not a documented mode of operation, i.MX23 doc only describes > the DMA mode. > > So while we _might_ be able to get the PIO mode to work on the i.MX23 > there is nothing in the doc stating that it's even meant to work. Even > while PIO and DMA mode use the same internal mechanisms, there's still > plenty of opportunities of fail in there. After all PIO mode relies on > reading a debug register in the course of normal operation. > > Only more extensive experimentation could show if we are in fact able to > make it work, a first shot of using PIO mode on MX23 failed, so it might > as well be that Juergens quick fix is correct and we have to disable PIO > mode on MX23 altogether. That said please stop slapping the word "hack" > over this patch until proven otherwise. Still, Fabio and I have been experiencing the one second delay on i.mx28. From my experience, it appeared between 3.7 and 3.9, see: http://www.spinics.net/lists/linux-i2c/msg11985.html. I still didn't take the time to investigate more yet but it confirms that PIO mode is working on i.mx28 though with an annoying delay. Regards,
Dear Lucas Stach, > Am Donnerstag, den 23.05.2013, 16:48 +0200 schrieb Marek Vasut: > > Dear Juergen Beisert, > > > > > Hi Marek, > > > > > > Marek Vasut wrote: > > > > > > i2c slowness is a different issue. > > > > > > > > > > Same happens here for my i.M23 based platform. It seems the PIO > > > > > mode does not work, or at least not like it works on a i.MX28. > > > > > Each short transfer needs about one second (without an error > > > > > message) but does not send anything on the I2C lines. > > > > > > > > > > I need the following patches to make I2C master work within a > > > > > 3.10-rc2 kernel: > > > > > > > > > > Subject: [PATCH] I2C/MXS: distinguish i.MX23 and i.MX28 based I2C > > > > > > > > I'm all for it, but then ... won't it be better if you actually fixed > > > > the PIO and mixed-mode on MX23 instead of implementing such hack? > > > > > > If the PIO mode or my patch is a hack depends on the point of view: > > > Lucas told me the PIO mode is *mentioned* but *not specified* in the > > > i.MX23/i.MX28 datasheets. > > > > The PIO works the same way DMA does -- set up bits and then pump data > > into the DATA register. > > > > > So, the PIO mode seems to depend on some undocumented status bits in > > > the i.MX28 I2C controller implementation. > > > > How would DMA work then if it used undocumented registers ? It's in the > > documentation, just read it or ask FSL ;-) > > While the PIO mode might use the same controller mechanisms as the DMA > mode, PIO mode is _not_ a documented mode of operation for the i.MX23. > > To quote the i.MX28 RM: "The I2C block on the i.MX28 supports a new PIO > mode or soft-DMA mode.", which implies the PIO mode to be a new mode of > operation not found on earlier i.MX SoCs. > The doc is slightly fuzzy here as the i.MX23 RM in contrary states: > "Short transmission (up to three bytes plus address) can be easily > triggered using only PIO operations, i.e., no DMA setup required." But > again it's not a documented mode of operation, i.MX23 doc only describes > the DMA mode. I'm CCing a Amaury Pouly so he can comment on this stuff. He did some hardware digging. > So while we _might_ be able to get the PIO mode to work on the i.MX23 > there is nothing in the doc stating that it's even meant to work. Even > while PIO and DMA mode use the same internal mechanisms, there's still > plenty of opportunities of fail in there. After all PIO mode relies on > reading a debug register in the course of normal operation. What's the problem with that ? The debug register contains status bits. > Only more extensive experimentation could show if we are in fact able to > make it work, a first shot of using PIO mode on MX23 failed, so it might > as well be that Juergens quick fix is correct and we have to disable PIO > mode on MX23 altogether. That said please stop slapping the word "hack" > over this patch until proven otherwise. > > Regards, > Lucas Best regards, Marek Vasut
Am Donnerstag, den 23.05.2013, 19:51 +0200 schrieb Alexandre Belloni: > Hi, > [...] > > > > So while we _might_ be able to get the PIO mode to work on the i.MX23 > > there is nothing in the doc stating that it's even meant to work. Even > > while PIO and DMA mode use the same internal mechanisms, there's still > > plenty of opportunities of fail in there. After all PIO mode relies on > > reading a debug register in the course of normal operation. > > > > Only more extensive experimentation could show if we are in fact able to > > make it work, a first shot of using PIO mode on MX23 failed, so it might > > as well be that Juergens quick fix is correct and we have to disable PIO > > mode on MX23 altogether. That said please stop slapping the word "hack" > > over this patch until proven otherwise. > > Still, Fabio and I have been experiencing the one second delay on > i.mx28. From my experience, it appeared between 3.7 and 3.9, see: > http://www.spinics.net/lists/linux-i2c/msg11985.html. I still didn't > take the time to investigate more yet but it confirms that PIO mode is > working on i.mx28 though with an annoying delay. > Just for the record: I know that PIO mode is functional on i.MX28 because I have it working on one of our customer boards. I am rather much surprised it's still not working well for you, but haven't had a chance to investigate yet. The i.MX28 RM explicitly describes how to work with the PIO mode, so if it's not working this is bug somewhere. This thread/patch is about a whole different problem, namely PIO mode being non-functional on i.MX23. As the i.MX23 RM doesn't describe the PIO mode, there is no indication that it's even meant to work on this chip. Regards, Lucas
Dear Lucas Stach, > Am Donnerstag, den 23.05.2013, 19:51 +0200 schrieb Alexandre Belloni: > > Hi, > > [...] > > > > So while we _might_ be able to get the PIO mode to work on the i.MX23 > > > there is nothing in the doc stating that it's even meant to work. Even > > > while PIO and DMA mode use the same internal mechanisms, there's still > > > plenty of opportunities of fail in there. After all PIO mode relies on > > > reading a debug register in the course of normal operation. > > > > > > Only more extensive experimentation could show if we are in fact able > > > to make it work, a first shot of using PIO mode on MX23 failed, so it > > > might as well be that Juergens quick fix is correct and we have to > > > disable PIO mode on MX23 altogether. That said please stop slapping > > > the word "hack" over this patch until proven otherwise. > > > > Still, Fabio and I have been experiencing the one second delay on > > i.mx28. From my experience, it appeared between 3.7 and 3.9, see: > > http://www.spinics.net/lists/linux-i2c/msg11985.html. I still didn't > > take the time to investigate more yet but it confirms that PIO mode is > > working on i.mx28 though with an annoying delay. > > Just for the record: I know that PIO mode is functional on i.MX28 > because I have it working on one of our customer boards. I am rather > much surprised it's still not working well for you, but haven't had a > chance to investigate yet. The i.MX28 RM explicitly describes how to > work with the PIO mode, so if it's not working this is bug somewhere. I won't be surprised if there was more than one. The RM describes something , but has many flaws. I'm sure you noticed that yourself. > This thread/patch is about a whole different problem, namely PIO mode > being non-functional on i.MX23. As the i.MX23 RM doesn't describe the > PIO mode, there is no indication that it's even meant to work on this > chip. I'm not against the patch per-se, but can we not get the best of this and instead try some more to fix the PIO on MX23 so the MX23 users won't suffer performance degradation on already weak CPU? Best regards, Marek Vasut
Dear Juergen Beisert, > Hi Fabio, > > > i2c slowness is a different issue. > > Same happens here for my i.M23 based platform. It seems the PIO mode does > not work, or at least not like it works on a i.MX28. Each short transfer > needs about one second (without an error message) but does not send > anything on the I2C lines. > > I need the following patches to make I2C master work within a 3.10-rc2 > kernel: > > Subject: [PATCH] I2C/MXS: distinguish i.MX23 and i.MX28 based I2C > controller Signed-off-by: Juergen Beisert <jbe@pengutronix.de> Can you please properly submit this patch ? Best regards, Marek Vasut
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c index 2039f23..5e69d8c 100644 --- a/drivers/i2c/busses/i2c-mxs.c +++ b/drivers/i2c/busses/i2c-mxs.c @@ -97,10 +97,17 @@ #define MXS_CMD_I2C_READ (MXS_I2C_CTRL0_SEND_NAK_ON_LAST | \ MXS_I2C_CTRL0_MASTER_MODE) +enum mxs_i2c_devtype { + MXS_I2C_UNKNOWN = 0, + MXS_I2C_V1, + MXS_I2C_V2, +}; + /** * struct mxs_i2c_dev - per device, private MXS-I2C data * * @dev: driver model device node + * @dev_type: distinguish i.MX23/i.MX28 features * @regs: IO registers pointer * @cmd_complete: completion object for transaction wait * @cmd_err: error code for last transaction @@ -108,6 +115,7 @@ */ struct mxs_i2c_dev { struct device *dev; + enum mxs_i2c_devtype dev_type; void __iomem *regs; struct completion cmd_complete; int cmd_err; @@ -633,8 +641,28 @@ static int mxs_i2c_get_ofdata(struct mxs_i2c_dev *i2c) return 0; } +static struct platform_device_id mxs_i2c_devtype[] = { + { + .name = "imx23-i2c", + .driver_data = MXS_I2C_V1, + }, { + .name = "imx28-i2c", + .driver_data = MXS_I2C_V2, + }, { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(platform, mxs_i2c_devtype); + +static const struct of_device_id mxs_i2c_dt_ids[] = { + { .compatible = "fsl,imx23-i2c", .data = &mxs_i2c_devtype[0], }, + { .compatible = "fsl,imx28-i2c", .data = &mxs_i2c_devtype[1], }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, mxs_i2c_dt_ids); + static int mxs_i2c_probe(struct platform_device *pdev) { + const struct of_device_id *of_id = + of_match_device(mxs_i2c_dt_ids, &pdev->dev); struct device *dev = &pdev->dev; struct mxs_i2c_dev *i2c; struct i2c_adapter *adap; @@ -651,6 +679,11 @@ static int mxs_i2c_probe(struct platform_device *pdev) if (!i2c) return -ENOMEM; + if (of_id) { + const struct platform_device_id *device_id = of_id->data; + i2c->dev_type = device_id->driver_data; + } + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); irq = platform_get_irq(pdev, 0); @@ -726,12 +759,6 @@ static int mxs_i2c_remove(struct platform_device *pdev) return 0; } -static const struct of_device_id mxs_i2c_dt_ids[] = { - { .compatible = "fsl,imx28-i2c", }, - { /* sentinel */ } -}; -MODULE_DEVICE_TABLE(of, mxs_i2c_dt_ids); - static struct platform_driver mxs_i2c_driver = { .driver = { .name = DRIVER_NAME, --------------------------------------------------------------------------------------------- Subject: [PATCH] I2C/MXS: on i.MX23 only do DMA based I2C transfers Signed-off-by: Juergen Beisert <jbe@pengutronix.de> --- drivers/i2c/busses/i2c-mxs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c index 5e69d8c..90cbac5 100644 --- a/drivers/i2c/busses/i2c-mxs.c +++ b/drivers/i2c/busses/i2c-mxs.c @@ -500,9 +500,10 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, * is set to 8 bytes, transfers shorter than 8 bytes are transfered * using PIO mode while longer transfers use DMA. The 8 byte border is * based on this empirical measurement and a lot of previous frobbing. + * Note: this special feature only works on i.MX28 SoC */ i2c->cmd_err = 0; - if (msg->len < 8) { + if (i2c->dev_type == MXS_I2C_V2 && msg->len < 8) { ret = mxs_i2c_pio_setup_xfer(adap, msg, flags); if (ret) mxs_i2c_reset(i2c);