| Message ID | 20210317065359.3109394-1-xiaoning.wang@nxp.com |
|---|---|
| Headers | show |
| Series | i2c: imx-lpi2c: New features and bug fixes | expand |
> From: Clark Wang <xiaoning.wang@nxp.com> > Sent: Wednesday, March 17, 2021 2:54 PM > > A NACK flag in ISR means i2c bus error. In such codition, there is no need to do > read/write operation. It's better to return ISR directly and then stop i2c > transfer. > > Signed-off-by: Gao Pan <pandy.gao@nxp.com> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com> Regards Aisheng > --- > drivers/i2c/busses/i2c-imx-lpi2c.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > b/drivers/i2c/busses/i2c-imx-lpi2c.c > index 9db6ccded5e9..bbf44ac95021 100644 > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > @@ -507,15 +507,17 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id) > lpi2c_imx_intctrl(lpi2c_imx, 0); > temp = readl(lpi2c_imx->base + LPI2C_MSR); > > + if (temp & MSR_NDF) { > + complete(&lpi2c_imx->complete); > + goto ret; > + } > + > if (temp & MSR_RDF) > lpi2c_imx_read_rxfifo(lpi2c_imx); > - > - if (temp & MSR_TDF) > + else if (temp & MSR_TDF) > lpi2c_imx_write_txfifo(lpi2c_imx); > > - if (temp & MSR_NDF) > - complete(&lpi2c_imx->complete); > - > +ret: > return IRQ_HANDLED; > } > > -- > 2.25.1
> From: Clark Wang <xiaoning.wang@nxp.com> > Sent: Wednesday, March 17, 2021 2:54 PM > Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support > > - Add runtime pm support to dynamicly manage the clock. > - Put the suspend to suspend_noirq. > - Call .pm_runtime_force_suspend() to force runtime pm suspended > in .suspend_noirq(). > The patch title needs to be improved as the driver already supports rpm. And do one thing in one patch. > Signed-off-by: Fugang Duan <fugang.duan@nxp.com> > Signed-off-by: Gao Pan <pandy.gao@nxp.com> > Reviewed-by: Anson Huang <Anson.Huang@nxp.com> Please add your sign-off. > --- > drivers/i2c/busses/i2c-imx-lpi2c.c | 50 ++++++++++++++++++++---------- > 1 file changed, 33 insertions(+), 17 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > b/drivers/i2c/busses/i2c-imx-lpi2c.c > index bbf44ac95021..1e920e7ac7c1 100644 > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > @@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device > *pdev) > if (ret) > lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ; > > - ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0, > + ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, > + IRQF_NO_SUSPEND, This belongs to a separate patch > pdev->name, lpi2c_imx); > if (ret) { > dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ -584,35 > +585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev) > i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx); > platform_set_drvdata(pdev, lpi2c_imx); > > - ret = clk_prepare_enable(lpi2c_imx->clk); > - if (ret) { > - dev_err(&pdev->dev, "clk enable failed %d\n", ret); > - return ret; > - } > - > pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT); > pm_runtime_use_autosuspend(&pdev->dev); > - pm_runtime_get_noresume(&pdev->dev); > - pm_runtime_set_active(&pdev->dev); > pm_runtime_enable(&pdev->dev); > > + ret = pm_runtime_get_sync(&pdev->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&pdev->dev); > + dev_err(&pdev->dev, "failed to enable clock\n"); > + return ret; > + } Can't current clk control via rpm work well? Please describe why need change. > + > temp = readl(lpi2c_imx->base + LPI2C_PARAM); > lpi2c_imx->txfifosize = 1 << (temp & 0x0f); > lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f); > > + pm_runtime_put(&pdev->dev); > + > ret = i2c_add_adapter(&lpi2c_imx->adapter); > if (ret) > goto rpm_disable; > > - pm_runtime_mark_last_busy(&pdev->dev); > - pm_runtime_put_autosuspend(&pdev->dev); > - > dev_info(&lpi2c_imx->adapter.dev, "LPI2C adapter registered\n"); > > return 0; > > rpm_disable: > - pm_runtime_put(&pdev->dev); > pm_runtime_disable(&pdev->dev); > pm_runtime_dont_use_autosuspend(&pdev->dev); > > @@ -636,7 +634,7 @@ static int __maybe_unused > lpi2c_runtime_suspend(struct device *dev) > struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev); > > clk_disable_unprepare(lpi2c_imx->clk); > - pinctrl_pm_select_sleep_state(dev); > + pinctrl_pm_select_idle_state(dev); This belongs to a separate patch > > return 0; > } > @@ -649,16 +647,34 @@ static int __maybe_unused > lpi2c_runtime_resume(struct device *dev) > pinctrl_pm_select_default_state(dev); > ret = clk_prepare_enable(lpi2c_imx->clk); > if (ret) { > - dev_err(dev, "failed to enable I2C clock, ret=%d\n", ret); > + dev_err(dev, "can't enable I2C clock, ret=%d\n", ret); Probably unnecessary change > return ret; > } > > + return ret; > +} > + > +static int lpi2c_suspend_noirq(struct device *dev) { > + int ret; > + > + ret = pm_runtime_force_suspend(dev); > + if (ret) > + return ret; > + > + pinctrl_pm_select_sleep_state(dev); > + > return 0; > } > > +static int lpi2c_resume_noirq(struct device *dev) { > + return pm_runtime_force_resume(dev); > +} > + > static const struct dev_pm_ops lpi2c_pm_ops = { > - SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > - pm_runtime_force_resume) > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpi2c_suspend_noirq, > + lpi2c_resume_noirq) Belongs to separate change and explain why need do this > SET_RUNTIME_PM_OPS(lpi2c_runtime_suspend, > lpi2c_runtime_resume, NULL) > }; > -- > 2.25.1
> From: Clark Wang <xiaoning.wang@nxp.com> > Sent: Wednesday, March 17, 2021 2:54 PM > > The lpi2c IP needs two clks: ipg clk and per clk. The old lpi2c driver missed ipg > clk. This patch adds ipg clk for lpi2c driver. > Pleas also update dt-binding and sent along with this patchset.(before this one) > Signed-off-by: Gao Pan <pandy.gao@nxp.com> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > Acked-by: Fugang Duan <fugang.duan@nxp.com> You can drop the Ack tag if the patch was changed > --- > drivers/i2c/busses/i2c-imx-lpi2c.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > b/drivers/i2c/busses/i2c-imx-lpi2c.c > index 1e920e7ac7c1..664fcc0dba51 100644 > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > @@ -94,7 +94,8 @@ enum lpi2c_imx_pincfg { > > struct lpi2c_imx_struct { > struct i2c_adapter adapter; > - struct clk *clk; > + struct clk *clk_per; > + struct clk *clk_ipg; > void __iomem *base; > __u8 *rx_buf; > __u8 *tx_buf; > @@ -563,10 +564,16 @@ static int lpi2c_imx_probe(struct platform_device > *pdev) > strlcpy(lpi2c_imx->adapter.name, pdev->name, > sizeof(lpi2c_imx->adapter.name)); > > - lpi2c_imx->clk = devm_clk_get(&pdev->dev, NULL); > - if (IS_ERR(lpi2c_imx->clk)) { > + lpi2c_imx->clk_per = devm_clk_get(&pdev->dev, "per"); > + if (IS_ERR(lpi2c_imx->clk_per)) { > dev_err(&pdev->dev, "can't get I2C peripheral clock\n"); > - return PTR_ERR(lpi2c_imx->clk); > + return PTR_ERR(lpi2c_imx->clk_per); > + } > + > + lpi2c_imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg"); > + if (IS_ERR(lpi2c_imx->clk_ipg)) { > + dev_err(&pdev->dev, "can't get I2C ipg clock\n"); > + return PTR_ERR(lpi2c_imx->clk_ipg); > } Will this break exist dts? Regards Aisheng > > ret = of_property_read_u32(pdev->dev.of_node, > @@ -633,7 +640,8 @@ static int __maybe_unused > lpi2c_runtime_suspend(struct device *dev) { > struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev); > > - clk_disable_unprepare(lpi2c_imx->clk); > + clk_disable_unprepare(lpi2c_imx->clk_ipg); > + clk_disable_unprepare(lpi2c_imx->clk_per); > pinctrl_pm_select_idle_state(dev); > > return 0; > @@ -645,12 +653,18 @@ static int __maybe_unused > lpi2c_runtime_resume(struct device *dev) > int ret; > > pinctrl_pm_select_default_state(dev); > - ret = clk_prepare_enable(lpi2c_imx->clk); > + ret = clk_prepare_enable(lpi2c_imx->clk_per); > if (ret) { > - dev_err(dev, "can't enable I2C clock, ret=%d\n", ret); > + dev_err(dev, "can't enable I2C per clock, ret=%d\n", ret); > return ret; > } > > + ret = clk_prepare_enable(lpi2c_imx->clk_ipg); > + if (ret) { > + clk_disable_unprepare(lpi2c_imx->clk_per); > + dev_err(dev, "can't enable I2C ipg clock, ret=%d\n", ret); > + } > + > return ret; > } > > -- > 2.25.1
> From: Clark Wang <xiaoning.wang@nxp.com> > Sent: Wednesday, March 17, 2021 2:54 PM > > Switching the clock frequently will affect the data transmission efficiency, and > prolong the timeout to reduce autosuspend times for lpi2c. > > Acked-by: Fugang Duan <fugang.duan@nxp.com> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com> Regards Aisheng > --- > drivers/i2c/busses/i2c-imx-lpi2c.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > b/drivers/i2c/busses/i2c-imx-lpi2c.c > index 86b69852f7be..c0cb77c66090 100644 > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > @@ -75,7 +75,7 @@ > #define I2C_CLK_RATIO 2 > #define CHUNK_DATA 256 > > -#define I2C_PM_TIMEOUT 10 /* ms */ > +#define I2C_PM_TIMEOUT 1000 /* ms */ > > enum lpi2c_imx_mode { > STANDARD, /* 100+Kbps */ > -- > 2.25.1
> From: Clark Wang <xiaoning.wang@nxp.com> > Sent: Wednesday, March 17, 2021 2:54 PM > > The clkhi and clklo ratio was not very precise before that can make the time of > START/STOP/HIGH LEVEL out of specification. > > Therefore, the calculation of these times has been modified in this patch. > At the same time, the mode rate definition of i2c is corrected. > > Reviewed-by: Fugang Duan <fugang.duan@nxp.com> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > --- > drivers/i2c/busses/i2c-imx-lpi2c.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > b/drivers/i2c/busses/i2c-imx-lpi2c.c > index 7216a393095d..5dbe85126f24 100644 > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > @@ -73,17 +73,17 @@ > #define MCFGR1_IGNACK BIT(9) > #define MRDR_RXEMPTY BIT(14) > > -#define I2C_CLK_RATIO 2 > +#define I2C_CLK_RATIO 24 / 59 Where is this ratio coming from? Can you describe why use it in commit message? Regards Aisheng > #define CHUNK_DATA 256 > > #define I2C_PM_TIMEOUT 1000 /* ms */ > > enum lpi2c_imx_mode { > - STANDARD, /* 100+Kbps */ > - FAST, /* 400+Kbps */ > - FAST_PLUS, /* 1.0+Mbps */ > - HS, /* 3.4+Mbps */ > - ULTRA_FAST, /* 5.0+Mbps */ > + STANDARD, /* <=100Kbps */ > + FAST, /* <=400Kbps */ > + FAST_PLUS, /* <=1.0Mbps */ > + HS, /* <=3.4Mbps */ > + ULTRA_FAST, /* <=5.0Mbps */ > }; > > enum lpi2c_imx_pincfg { > @@ -156,13 +156,13 @@ static void lpi2c_imx_set_mode(struct > lpi2c_imx_struct *lpi2c_imx) > unsigned int bitrate = lpi2c_imx->bitrate; > enum lpi2c_imx_mode mode; > > - if (bitrate < I2C_MAX_FAST_MODE_FREQ) > + if (bitrate <= I2C_MAX_STANDARD_MODE_FREQ) > mode = STANDARD; > - else if (bitrate < I2C_MAX_FAST_MODE_PLUS_FREQ) > + else if (bitrate <= I2C_MAX_FAST_MODE_FREQ) > mode = FAST; > - else if (bitrate < I2C_MAX_HIGH_SPEED_MODE_FREQ) > + else if (bitrate <= I2C_MAX_FAST_MODE_PLUS_FREQ) > mode = FAST_PLUS; > - else if (bitrate < I2C_MAX_ULTRA_FAST_MODE_FREQ) > + else if (bitrate <= I2C_MAX_HIGH_SPEED_MODE_FREQ) > mode = HS; > else > mode = ULTRA_FAST; > @@ -209,7 +209,8 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct > *lpi2c_imx) > } while (1); > } > > -/* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */ > +/* CLKLO = (1 - I2C_CLK_RATIO) * clk_cycle, SETHOLD = CLKHI, DATAVD = > CLKHI/2 > + CLKHI = I2C_CLK_RATIO * clk_cycle */ > static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx) { > u8 prescale, filt, sethold, clkhi, clklo, datavd; @@ -232,8 +233,8 @@ > static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx) > > for (prescale = 0; prescale <= 7; prescale++) { > clk_cycle = clk_rate / ((1 << prescale) * lpi2c_imx->bitrate) > - - 3 - (filt >> 1); > - clkhi = (clk_cycle + I2C_CLK_RATIO) / (I2C_CLK_RATIO + 1); > + - (2 + filt) / (1 << prescale); > + clkhi = clk_cycle * I2C_CLK_RATIO; > clklo = clk_cycle - clkhi; > if (clklo < 64) > break; > -- > 2.25.1
> From: Clark Wang <xiaoning.wang@nxp.com> > Sent: Wednesday, March 17, 2021 2:54 PM > > Add eDMA receive and send mode support. > Support to read and write data larger than 256 bytes in one frame. > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > Reviewed-by: Li Jun <jun.li@nxp.com> > --- > drivers/i2c/busses/i2c-imx-lpi2c.c | 291 ++++++++++++++++++++++++++++- Pease update dt-binding first > 1 file changed, 289 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > b/drivers/i2c/busses/i2c-imx-lpi2c.c > index 1e26672d47bf..6d920bf0dbd4 100644 > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > @@ -8,6 +8,8 @@ > #include <linux/clk.h> > #include <linux/completion.h> > #include <linux/delay.h> > +#include <linux/dmaengine.h> > +#include <linux/dma-mapping.h> > #include <linux/err.h> > #include <linux/errno.h> > #include <linux/i2c.h> > @@ -31,6 +33,7 @@ > #define LPI2C_MCR 0x10 /* i2c contrl register */ > #define LPI2C_MSR 0x14 /* i2c status register */ > #define LPI2C_MIER 0x18 /* i2c interrupt enable */ > +#define LPI2C_MDER 0x1C /* i2c DMA enable */ > #define LPI2C_MCFGR0 0x20 /* i2c master configuration */ > #define LPI2C_MCFGR1 0x24 /* i2c master configuration */ > #define LPI2C_MCFGR2 0x28 /* i2c master configuration */ > @@ -72,11 +75,15 @@ > #define MCFGR1_AUTOSTOP BIT(8) > #define MCFGR1_IGNACK BIT(9) > #define MRDR_RXEMPTY BIT(14) > +#define MDER_TDDE BIT(0) > +#define MDER_RDDE BIT(1) > > #define I2C_CLK_RATIO 24 / 59 > #define CHUNK_DATA 256 > > #define I2C_PM_TIMEOUT 1000 /* ms */ > +#define I2C_DMA_THRESHOLD 16 /* bytes */ > +#define I2C_USE_PIO (-150) Can you clarify a bit why need using this strange value? > > enum lpi2c_imx_mode { > STANDARD, /* <=100Kbps */ > @@ -95,6 +102,7 @@ enum lpi2c_imx_pincfg { > > struct lpi2c_imx_struct { > struct i2c_adapter adapter; > + resource_size_t phy_addr; > int irq; > struct clk *clk_per; > struct clk *clk_ipg; > @@ -114,6 +122,17 @@ struct lpi2c_imx_struct { > struct pinctrl *pinctrl; > struct pinctrl_state *pinctrl_pins_default; > struct pinctrl_state *pinctrl_pins_gpio; > + > + bool can_use_dma; > + bool using_dma; > + bool xferred; > + struct i2c_msg *msg; > + dma_addr_t dma_addr; > + struct dma_chan *dma_tx; > + struct dma_chan *dma_rx; > + enum dma_data_direction dma_direction; > + u8 *dma_buf; > + unsigned int dma_len; > }; > > static void lpi2c_imx_intctrl(struct lpi2c_imx_struct *lpi2c_imx, @@ -289,6 > +308,9 @@ static int lpi2c_imx_master_enable(struct lpi2c_imx_struct > *lpi2c_imx) > if (ret) > goto rpm_put; > > + if (lpi2c_imx->can_use_dma) > + writel(MDER_TDDE | MDER_RDDE, lpi2c_imx->base + LPI2C_MDER); > + > temp = readl(lpi2c_imx->base + LPI2C_MCR); > temp |= MCR_MEN; > writel(temp, lpi2c_imx->base + LPI2C_MCR); @@ -462,6 +484,154 @@ > static void lpi2c_imx_read(struct lpi2c_imx_struct *lpi2c_imx, > lpi2c_imx_intctrl(lpi2c_imx, MIER_RDIE | MIER_NDIE); } > > +static void lpi2c_dma_unmap(struct lpi2c_imx_struct *lpi2c_imx) { > + struct dma_chan *chan = lpi2c_imx->dma_direction == > DMA_FROM_DEVICE > + ? lpi2c_imx->dma_rx : lpi2c_imx->dma_tx; > + > + dma_unmap_single(chan->device->dev, lpi2c_imx->dma_addr, > + lpi2c_imx->dma_len, lpi2c_imx->dma_direction); > + > + lpi2c_imx->dma_direction = DMA_NONE; > +} > + > +static void lpi2c_cleanup_dma(struct lpi2c_imx_struct *lpi2c_imx) { > + if (lpi2c_imx->dma_direction == DMA_NONE) > + return; > + else if (lpi2c_imx->dma_direction == DMA_FROM_DEVICE) > + dmaengine_terminate_all(lpi2c_imx->dma_rx); > + else if (lpi2c_imx->dma_direction == DMA_TO_DEVICE) > + dmaengine_terminate_all(lpi2c_imx->dma_tx); > + > + lpi2c_dma_unmap(lpi2c_imx); > +} > + > +static void lpi2c_dma_callback(void *data) { > + struct lpi2c_imx_struct *lpi2c_imx = (struct lpi2c_imx_struct *)data; > + > + lpi2c_dma_unmap(lpi2c_imx); > + writel(GEN_STOP << 8, lpi2c_imx->base + LPI2C_MTDR); > + lpi2c_imx->xferred = true; > + > + complete(&lpi2c_imx->complete); > +} > + > +static int lpi2c_dma_submit(struct lpi2c_imx_struct *lpi2c_imx, > + struct i2c_msg *msg) > +{ > + bool read = msg->flags & I2C_M_RD; > + enum dma_data_direction dir = read ? DMA_FROM_DEVICE : > DMA_TO_DEVICE; > + struct dma_chan *chan = read ? lpi2c_imx->dma_rx : lpi2c_imx->dma_tx; > + struct dma_async_tx_descriptor *txdesc; > + dma_cookie_t cookie; > + > + lpi2c_imx->dma_len = read ? msg->len - 1 : msg->len; > + lpi2c_imx->msg = msg; > + lpi2c_imx->dma_direction = dir; > + > + if (IS_ERR(chan)) > + return PTR_ERR(chan); > + > + lpi2c_imx->dma_addr = dma_map_single(chan->device->dev, > + lpi2c_imx->dma_buf, > + lpi2c_imx->dma_len, dir); > + if (dma_mapping_error(chan->device->dev, lpi2c_imx->dma_addr)) { > + dev_err(&lpi2c_imx->adapter.dev, "dma map failed, use pio\n"); > + return -EINVAL; > + } > + > + txdesc = dmaengine_prep_slave_single(chan, lpi2c_imx->dma_addr, > + lpi2c_imx->dma_len, read ? > + DMA_DEV_TO_MEM : DMA_MEM_TO_DEV, > + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > + if (!txdesc) { > + dev_err(&lpi2c_imx->adapter.dev, "dma prep slave sg failed, use > pio\n"); > + lpi2c_cleanup_dma(lpi2c_imx); > + return -EINVAL; > + } > + > + reinit_completion(&lpi2c_imx->complete); > + txdesc->callback = lpi2c_dma_callback; > + txdesc->callback_param = (void *)lpi2c_imx; > + > + cookie = dmaengine_submit(txdesc); > + if (dma_submit_error(cookie)) { > + dev_err(&lpi2c_imx->adapter.dev, "submitting dma failed, use > pio\n"); > + lpi2c_cleanup_dma(lpi2c_imx); > + return -EINVAL; > + } > + > + lpi2c_imx_intctrl(lpi2c_imx, MIER_NDIE); > + > + dma_async_issue_pending(chan); > + > + return 0; > +} > + > +static bool is_use_dma(struct lpi2c_imx_struct *lpi2c_imx, struct > +i2c_msg *msg) { > + if (!lpi2c_imx->can_use_dma) > + return false; > + > + if (msg->len < I2C_DMA_THRESHOLD) > + return false; > + > + return true; > +} > + > +static int lpi2c_imx_push_rx_cmd(struct lpi2c_imx_struct *lpi2c_imx, > + struct i2c_msg *msg) > +{ > + unsigned int temp, rx_remain; > + unsigned long orig_jiffies = jiffies; > + > + if ((msg->flags & I2C_M_RD)) { > + rx_remain = msg->len; > + do { > + temp = rx_remain > CHUNK_DATA ? > + CHUNK_DATA - 1 : rx_remain - 1; > + temp |= (RECV_DATA << 8); > + while ((readl(lpi2c_imx->base + LPI2C_MFSR) & 0xff) > 2) { > + if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(1000))) { > + dev_dbg(&lpi2c_imx->adapter.dev, "txfifo empty > timeout\n"); > + if (lpi2c_imx->adapter.bus_recovery_info) > + i2c_recover_bus(&lpi2c_imx->adapter); > + return -ETIMEDOUT; > + } > + schedule(); > + } > + writel(temp, lpi2c_imx->base + LPI2C_MTDR); > + rx_remain = rx_remain - (temp & 0xff) - 1; > + } while (rx_remain > 0); > + } > + > + return 0; > +} > + > +static int lpi2c_dma_xfer(struct lpi2c_imx_struct *lpi2c_imx, > + struct i2c_msg *msg) > +{ > + int result; > + > + result = lpi2c_dma_submit(lpi2c_imx, msg); > + if (!result) { > + result = lpi2c_imx_push_rx_cmd(lpi2c_imx, msg); > + if (result) > + return result; > + result = lpi2c_imx_msg_complete(lpi2c_imx); > + return result; > + } > + > + /* DMA xfer failed, try to use PIO, clean up dma things */ > + i2c_put_dma_safe_msg_buf(lpi2c_imx->dma_buf, lpi2c_imx->msg, > + lpi2c_imx->xferred); > + lpi2c_cleanup_dma(lpi2c_imx); > + > + return I2C_USE_PIO; > +} > + > static int lpi2c_imx_xfer(struct i2c_adapter *adapter, > struct i2c_msg *msgs, int num) > { > @@ -474,6 +644,9 @@ static int lpi2c_imx_xfer(struct i2c_adapter *adapter, > return result; > > for (i = 0; i < num; i++) { > + lpi2c_imx->xferred = false; > + lpi2c_imx->using_dma = false; > + > result = lpi2c_imx_start(lpi2c_imx, &msgs[i]); > if (result) > goto disable; > @@ -482,9 +655,24 @@ static int lpi2c_imx_xfer(struct i2c_adapter *adapter, > if (num == 1 && msgs[0].len == 0) > goto stop; > > + if (is_use_dma(lpi2c_imx, &msgs[i])) { > + lpi2c_imx->using_dma = true; > + > + writel(0x1, lpi2c_imx->base + LPI2C_MFCR); > + > + lpi2c_imx->dma_buf = i2c_get_dma_safe_msg_buf(&msgs[i], > + I2C_DMA_THRESHOLD); > + if (lpi2c_imx->dma_buf) { > + result = lpi2c_dma_xfer(lpi2c_imx, &msgs[i]); > + if (result != I2C_USE_PIO) > + goto stop; > + } > + } > + > + lpi2c_imx->using_dma = false; > lpi2c_imx->delivered = 0; > lpi2c_imx->msglen = msgs[i].len; > - init_completion(&lpi2c_imx->complete); > + reinit_completion(&lpi2c_imx->complete); > > if (msgs[i].flags & I2C_M_RD) > lpi2c_imx_read(lpi2c_imx, &msgs[i]); @@ -503,7 +691,16 @@ > static int lpi2c_imx_xfer(struct i2c_adapter *adapter, > } > > stop: > - lpi2c_imx_stop(lpi2c_imx); > + if (!lpi2c_imx->using_dma) > + lpi2c_imx_stop(lpi2c_imx); > + else { > + i2c_put_dma_safe_msg_buf(lpi2c_imx->dma_buf, lpi2c_imx->msg, > + lpi2c_imx->xferred); > + if (result) { > + lpi2c_cleanup_dma(lpi2c_imx); > + writel(GEN_STOP << 8, lpi2c_imx->base + LPI2C_MTDR); > + } > + } > > temp = readl(lpi2c_imx->base + LPI2C_MSR); > if ((temp & MSR_NDF) && !result) > @@ -528,6 +725,10 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id) > temp = readl(lpi2c_imx->base + LPI2C_MSR); > > if (temp & MSR_NDF) { > + if (lpi2c_imx->using_dma) { > + lpi2c_cleanup_dma(lpi2c_imx); > + writel(GEN_STOP << 8, lpi2c_imx->base + LPI2C_MTDR); > + } > complete(&lpi2c_imx->complete); > goto ret; > } > @@ -623,20 +824,94 @@ static const struct of_device_id > lpi2c_imx_of_match[] = { }; MODULE_DEVICE_TABLE(of, > lpi2c_imx_of_match); > > +static void lpi2c_dma_exit(struct lpi2c_imx_struct *lpi2c_imx) { > + if (lpi2c_imx->dma_rx) { > + dma_release_channel(lpi2c_imx->dma_rx); > + lpi2c_imx->dma_rx = NULL; > + } > + > + if (lpi2c_imx->dma_tx) { > + dma_release_channel(lpi2c_imx->dma_tx); > + lpi2c_imx->dma_tx = NULL; > + } > +} > + > +static int lpi2c_dma_init(struct device *dev, > + struct lpi2c_imx_struct *lpi2c_imx) { > + int ret; > + struct dma_slave_config dma_sconfig; > + > + /* Prepare for TX DMA: */ > + lpi2c_imx->dma_tx = dma_request_chan(dev, "tx"); > + if (IS_ERR(lpi2c_imx->dma_tx)) { > + ret = PTR_ERR(lpi2c_imx->dma_tx); > + dev_err(dev, "can't get the TX DMA channel, error %d!\n", ret); > + lpi2c_imx->dma_tx = NULL; > + goto err; > + } > + > + dma_sconfig.dst_addr = lpi2c_imx->phy_addr + LPI2C_MTDR; > + dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE; > + dma_sconfig.dst_maxburst = 1; > + dma_sconfig.direction = DMA_MEM_TO_DEV; > + ret = dmaengine_slave_config(lpi2c_imx->dma_tx, &dma_sconfig); > + if (ret < 0) { > + dev_err(dev, "can't configure tx channel (%d)\n", ret); > + goto fail_tx; > + } > + > + /* Prepare for RX DMA: */ > + lpi2c_imx->dma_rx = dma_request_chan(dev, "rx"); > + if (IS_ERR(lpi2c_imx->dma_rx)) { > + ret = PTR_ERR(lpi2c_imx->dma_rx); > + dev_err(dev, "can't get the RX DMA channel, error %d\n", ret); > + lpi2c_imx->dma_rx = NULL; > + goto fail_tx; > + } > + > + dma_sconfig.src_addr = lpi2c_imx->phy_addr + LPI2C_MRDR; > + dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE; > + dma_sconfig.src_maxburst = 1; > + dma_sconfig.direction = DMA_DEV_TO_MEM; > + ret = dmaengine_slave_config(lpi2c_imx->dma_rx, &dma_sconfig); > + if (ret < 0) { > + dev_err(dev, "can't configure rx channel (%d)\n", ret); > + goto fail_rx; > + } > + > + lpi2c_imx->can_use_dma = true; > + lpi2c_imx->using_dma = false; > + > + return 0; > +fail_rx: > + dma_release_channel(lpi2c_imx->dma_rx); > +fail_tx: > + dma_release_channel(lpi2c_imx->dma_tx); > +err: > + lpi2c_dma_exit(lpi2c_imx); > + lpi2c_imx->can_use_dma = false; > + return ret; > +} > + > static int lpi2c_imx_probe(struct platform_device *pdev) { > struct lpi2c_imx_struct *lpi2c_imx; > unsigned int temp; > int ret; > + struct resource *res; > > lpi2c_imx = devm_kzalloc(&pdev->dev, sizeof(*lpi2c_imx), GFP_KERNEL); > if (!lpi2c_imx) > return -ENOMEM; > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > lpi2c_imx->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(lpi2c_imx->base)) > return PTR_ERR(lpi2c_imx->base); > > + lpi2c_imx->phy_addr = (dma_addr_t)res->start; > lpi2c_imx->irq = platform_get_irq(pdev, 0); > if (lpi2c_imx->irq < 0) > return lpi2c_imx->irq; > @@ -691,6 +966,18 @@ static int lpi2c_imx_probe(struct platform_device > *pdev) > if (ret == -EPROBE_DEFER) > goto rpm_disable; > > + /* Init DMA */ > + lpi2c_imx->dma_direction = DMA_NONE; > + lpi2c_imx->dma_rx = lpi2c_imx->dma_tx = NULL; Unnecessary line > + ret = lpi2c_dma_init(&pdev->dev, lpi2c_imx); > + if (ret) { > + dev_err_probe(&pdev->dev, ret, "dma setup error %d, use pio\n", > ret); > + if (ret == -EPROBE_DEFER) > + goto rpm_disable; > + } > + > + init_completion(&lpi2c_imx->complete); > + > ret = i2c_add_adapter(&lpi2c_imx->adapter); > if (ret) > goto rpm_disable; > -- > 2.25.1
> -----Original Message----- > From: Aisheng Dong <aisheng.dong@nxp.com> > Sent: Friday, March 19, 2021 12:40 > To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org; > s.hauer@pengutronix.de > Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux- > imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support > > > From: Clark Wang <xiaoning.wang@nxp.com> > > Sent: Wednesday, March 17, 2021 2:54 PM > > Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support > > > > - Add runtime pm support to dynamicly manage the clock. > > - Put the suspend to suspend_noirq. > > - Call .pm_runtime_force_suspend() to force runtime pm suspended > > in .suspend_noirq(). > > > > The patch title needs to be improved as the driver already supports rpm. > And do one thing in one patch. Thanks. I will split this patch into several and resend. Best Regards, Clark Wang > > > Signed-off-by: Fugang Duan <fugang.duan@nxp.com> > > Signed-off-by: Gao Pan <pandy.gao@nxp.com> > > Reviewed-by: Anson Huang <Anson.Huang@nxp.com> > > Please add your sign-off. > > > --- > > drivers/i2c/busses/i2c-imx-lpi2c.c | 50 > > ++++++++++++++++++++---------- > > 1 file changed, 33 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > > b/drivers/i2c/busses/i2c-imx-lpi2c.c > > index bbf44ac95021..1e920e7ac7c1 100644 > > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > > @@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device > > *pdev) > > if (ret) > > lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ; > > > > - ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0, > > + ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, > > + IRQF_NO_SUSPEND, > > This belongs to a separate patch > > > pdev->name, lpi2c_imx); > > if (ret) { > > dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ - > 584,35 > > +585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev) > > i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx); > > platform_set_drvdata(pdev, lpi2c_imx); > > > > - ret = clk_prepare_enable(lpi2c_imx->clk); > > - if (ret) { > > - dev_err(&pdev->dev, "clk enable failed %d\n", ret); > > - return ret; > > - } > > - > > pm_runtime_set_autosuspend_delay(&pdev->dev, > I2C_PM_TIMEOUT); > > pm_runtime_use_autosuspend(&pdev->dev); > > - pm_runtime_get_noresume(&pdev->dev); > > - pm_runtime_set_active(&pdev->dev); > > pm_runtime_enable(&pdev->dev); > > > > + ret = pm_runtime_get_sync(&pdev->dev); > > + if (ret < 0) { > > + pm_runtime_put_noidle(&pdev->dev); > > + dev_err(&pdev->dev, "failed to enable clock\n"); > > + return ret; > > + } > > Can't current clk control via rpm work well? > Please describe why need change. > > > + > > temp = readl(lpi2c_imx->base + LPI2C_PARAM); > > lpi2c_imx->txfifosize = 1 << (temp & 0x0f); > > lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f); > > > > + pm_runtime_put(&pdev->dev); > > + > > ret = i2c_add_adapter(&lpi2c_imx->adapter); > > if (ret) > > goto rpm_disable; > > > > - pm_runtime_mark_last_busy(&pdev->dev); > > - pm_runtime_put_autosuspend(&pdev->dev); > > - > > dev_info(&lpi2c_imx->adapter.dev, "LPI2C adapter registered\n"); > > > > return 0; > > > > rpm_disable: > > - pm_runtime_put(&pdev->dev); > > pm_runtime_disable(&pdev->dev); > > pm_runtime_dont_use_autosuspend(&pdev->dev); > > > > @@ -636,7 +634,7 @@ static int __maybe_unused > > lpi2c_runtime_suspend(struct device *dev) > > struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev); > > > > clk_disable_unprepare(lpi2c_imx->clk); > > - pinctrl_pm_select_sleep_state(dev); > > + pinctrl_pm_select_idle_state(dev); > > This belongs to a separate patch > > > > > return 0; > > } > > @@ -649,16 +647,34 @@ static int __maybe_unused > > lpi2c_runtime_resume(struct device *dev) > > pinctrl_pm_select_default_state(dev); > > ret = clk_prepare_enable(lpi2c_imx->clk); > > if (ret) { > > - dev_err(dev, "failed to enable I2C clock, ret=%d\n", ret); > > + dev_err(dev, "can't enable I2C clock, ret=%d\n", ret); > > Probably unnecessary change > > > return ret; > > } > > > > + return ret; > > +} > > + > > +static int lpi2c_suspend_noirq(struct device *dev) { > > + int ret; > > + > > + ret = pm_runtime_force_suspend(dev); > > + if (ret) > > + return ret; > > + > > + pinctrl_pm_select_sleep_state(dev); > > + > > return 0; > > } > > > > +static int lpi2c_resume_noirq(struct device *dev) { > > + return pm_runtime_force_resume(dev); } > > + > > static const struct dev_pm_ops lpi2c_pm_ops = { > > - SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > > - pm_runtime_force_resume) > > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpi2c_suspend_noirq, > > + lpi2c_resume_noirq) > > Belongs to separate change and explain why need do this > > > SET_RUNTIME_PM_OPS(lpi2c_runtime_suspend, > > lpi2c_runtime_resume, NULL) > > }; > > -- > > 2.25.1
> -----Original Message----- > From: Aisheng Dong <aisheng.dong@nxp.com> > Sent: Friday, March 19, 2021 12:40 > To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org; > s.hauer@pengutronix.de > Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux- > imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support > > > From: Clark Wang <xiaoning.wang@nxp.com> > > Sent: Wednesday, March 17, 2021 2:54 PM > > Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support > > > > - Add runtime pm support to dynamicly manage the clock. > > - Put the suspend to suspend_noirq. > > - Call .pm_runtime_force_suspend() to force runtime pm suspended > > in .suspend_noirq(). > > > > The patch title needs to be improved as the driver already supports rpm. > And do one thing in one patch. > > > Signed-off-by: Fugang Duan <fugang.duan@nxp.com> > > Signed-off-by: Gao Pan <pandy.gao@nxp.com> > > Reviewed-by: Anson Huang <Anson.Huang@nxp.com> > > Please add your sign-off. > > > --- > > drivers/i2c/busses/i2c-imx-lpi2c.c | 50 > > ++++++++++++++++++++---------- > > 1 file changed, 33 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > > b/drivers/i2c/busses/i2c-imx-lpi2c.c > > index bbf44ac95021..1e920e7ac7c1 100644 > > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > > @@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device > > *pdev) > > if (ret) > > lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ; > > > > - ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0, > > + ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, > > + IRQF_NO_SUSPEND, > > This belongs to a separate patch > > > pdev->name, lpi2c_imx); > > if (ret) { > > dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ - > 584,35 > > +585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev) > > i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx); > > platform_set_drvdata(pdev, lpi2c_imx); > > > > - ret = clk_prepare_enable(lpi2c_imx->clk); > > - if (ret) { > > - dev_err(&pdev->dev, "clk enable failed %d\n", ret); > > - return ret; > > - } > > - > > pm_runtime_set_autosuspend_delay(&pdev->dev, > I2C_PM_TIMEOUT); > > pm_runtime_use_autosuspend(&pdev->dev); > > - pm_runtime_get_noresume(&pdev->dev); > > - pm_runtime_set_active(&pdev->dev); > > pm_runtime_enable(&pdev->dev); > > > > + ret = pm_runtime_get_sync(&pdev->dev); > > + if (ret < 0) { > > + pm_runtime_put_noidle(&pdev->dev); > > + dev_err(&pdev->dev, "failed to enable clock\n"); > > + return ret; > > + } > > Can't current clk control via rpm work well? > Please describe why need change. I think the previous patch maker might want to use the return value of pm_runtime_get_sync to check whether the clock has been turned on correctly to avoid the kernel panic. Maybe I can change to the method like this. pm_runtime_get_noresume(&pdev->dev); ret = pm_runtime_set_active(&pdev->dev); if (ret < 0) goto out; pm_runtime_enable(&pdev->dev); Best Regards, Clark Wang
> -----Original Message----- > From: Aisheng Dong <aisheng.dong@nxp.com> > Sent: Friday, March 19, 2021 12:46 > To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org; > s.hauer@pengutronix.de > Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux- > imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Subject: RE: [PATCH 03/11] i2c: imx-lpi2c: add ipg clk for lpi2c driver > > > From: Clark Wang <xiaoning.wang@nxp.com> > > Sent: Wednesday, March 17, 2021 2:54 PM > > > > The lpi2c IP needs two clks: ipg clk and per clk. The old lpi2c driver > > missed ipg clk. This patch adds ipg clk for lpi2c driver. > > > > Pleas also update dt-binding and sent along with this patchset.(before this > one) Okay, thanks. > > > Signed-off-by: Gao Pan <pandy.gao@nxp.com> > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > > Acked-by: Fugang Duan <fugang.duan@nxp.com> > > You can drop the Ack tag if the patch was changed > > > --- > > drivers/i2c/busses/i2c-imx-lpi2c.c | 28 +++++++++++++++++++++------- > > 1 file changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > > b/drivers/i2c/busses/i2c-imx-lpi2c.c > > index 1e920e7ac7c1..664fcc0dba51 100644 > > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > > @@ -94,7 +94,8 @@ enum lpi2c_imx_pincfg { > > > > struct lpi2c_imx_struct { > > struct i2c_adapter adapter; > > - struct clk *clk; > > + struct clk *clk_per; > > + struct clk *clk_ipg; > > void __iomem *base; > > __u8 *rx_buf; > > __u8 *tx_buf; > > @@ -563,10 +564,16 @@ static int lpi2c_imx_probe(struct > > platform_device > > *pdev) > > strlcpy(lpi2c_imx->adapter.name, pdev->name, > > sizeof(lpi2c_imx->adapter.name)); > > > > - lpi2c_imx->clk = devm_clk_get(&pdev->dev, NULL); > > - if (IS_ERR(lpi2c_imx->clk)) { > > + lpi2c_imx->clk_per = devm_clk_get(&pdev->dev, "per"); > > + if (IS_ERR(lpi2c_imx->clk_per)) { > > dev_err(&pdev->dev, "can't get I2C peripheral clock\n"); > > - return PTR_ERR(lpi2c_imx->clk); > > + return PTR_ERR(lpi2c_imx->clk_per); > > + } > > + > > + lpi2c_imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg"); > > + if (IS_ERR(lpi2c_imx->clk_ipg)) { > > + dev_err(&pdev->dev, "can't get I2C ipg clock\n"); > > + return PTR_ERR(lpi2c_imx->clk_ipg); > > } > > Will this break exist dts? It will not break the build. But will break the lpi2c probe for imx7ulp and imx8qxp/qm. I will send two patches to update dts in V2. Best Regards, Clark Wang > > Regards > Aisheng > > > > > ret = of_property_read_u32(pdev->dev.of_node, > > @@ -633,7 +640,8 @@ static int __maybe_unused > > lpi2c_runtime_suspend(struct device *dev) { > > struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev); > > > > - clk_disable_unprepare(lpi2c_imx->clk); > > + clk_disable_unprepare(lpi2c_imx->clk_ipg); > > + clk_disable_unprepare(lpi2c_imx->clk_per); > > pinctrl_pm_select_idle_state(dev); > > > > return 0; > > @@ -645,12 +653,18 @@ static int __maybe_unused > > lpi2c_runtime_resume(struct device *dev) > > int ret; > > > > pinctrl_pm_select_default_state(dev); > > - ret = clk_prepare_enable(lpi2c_imx->clk); > > + ret = clk_prepare_enable(lpi2c_imx->clk_per); > > if (ret) { > > - dev_err(dev, "can't enable I2C clock, ret=%d\n", ret); > > + dev_err(dev, "can't enable I2C per clock, ret=%d\n", ret); > > return ret; > > } > > > > + ret = clk_prepare_enable(lpi2c_imx->clk_ipg); > > + if (ret) { > > + clk_disable_unprepare(lpi2c_imx->clk_per); > > + dev_err(dev, "can't enable I2C ipg clock, ret=%d\n", ret); > > + } > > + > > return ret; > > } > > > > -- > > 2.25.1
> -----Original Message----- > From: Aisheng Dong <aisheng.dong@nxp.com> > Sent: Friday, March 19, 2021 13:15 > To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org; > s.hauer@pengutronix.de > Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux- > imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Subject: RE: [PATCH 09/11] i2c: imx-lpi2c: fix i2c timing issue > > > From: Clark Wang <xiaoning.wang@nxp.com> > > Sent: Wednesday, March 17, 2021 2:54 PM > > > > The clkhi and clklo ratio was not very precise before that can make > > the time of START/STOP/HIGH LEVEL out of specification. > > > > Therefore, the calculation of these times has been modified in this patch. > > At the same time, the mode rate definition of i2c is corrected. > > > > Reviewed-by: Fugang Duan <fugang.duan@nxp.com> > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > > --- > > drivers/i2c/busses/i2c-imx-lpi2c.c | 27 ++++++++++++++------------- > > 1 file changed, 14 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > > b/drivers/i2c/busses/i2c-imx-lpi2c.c > > index 7216a393095d..5dbe85126f24 100644 > > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > > @@ -73,17 +73,17 @@ > > #define MCFGR1_IGNACK BIT(9) > > #define MRDR_RXEMPTY BIT(14) > > > > -#define I2C_CLK_RATIO 2 > > +#define I2C_CLK_RATIO 24 / 59 > > Where is this ratio coming from? > Can you describe why use it in commit message? This ratio is a value obtained after passing the test. I2C's Tlow should longer than the spec. For example, in FAST mode, Tlow should be longer than 1.3us. The previous calculation violated the spec. So I re-write the calculation of clk_cycle by referring the RM. Then test the ratio to let Tlow match the spec by catching the waveform. Best Regards, Clark Wang > > Regards > Aisheng > > > #define CHUNK_DATA 256 > > > > #define I2C_PM_TIMEOUT 1000 /* ms */ > > > > enum lpi2c_imx_mode { > > - STANDARD, /* 100+Kbps */ > > - FAST, /* 400+Kbps */ > > - FAST_PLUS, /* 1.0+Mbps */ > > - HS, /* 3.4+Mbps */ > > - ULTRA_FAST, /* 5.0+Mbps */ > > + STANDARD, /* <=100Kbps */ > > + FAST, /* <=400Kbps */ > > + FAST_PLUS, /* <=1.0Mbps */ > > + HS, /* <=3.4Mbps */ > > + ULTRA_FAST, /* <=5.0Mbps */ > > }; > > > > enum lpi2c_imx_pincfg { > > @@ -156,13 +156,13 @@ static void lpi2c_imx_set_mode(struct > > lpi2c_imx_struct *lpi2c_imx) > > unsigned int bitrate = lpi2c_imx->bitrate; > > enum lpi2c_imx_mode mode; > > > > - if (bitrate < I2C_MAX_FAST_MODE_FREQ) > > + if (bitrate <= I2C_MAX_STANDARD_MODE_FREQ) > > mode = STANDARD; > > - else if (bitrate < I2C_MAX_FAST_MODE_PLUS_FREQ) > > + else if (bitrate <= I2C_MAX_FAST_MODE_FREQ) > > mode = FAST; > > - else if (bitrate < I2C_MAX_HIGH_SPEED_MODE_FREQ) > > + else if (bitrate <= I2C_MAX_FAST_MODE_PLUS_FREQ) > > mode = FAST_PLUS; > > - else if (bitrate < I2C_MAX_ULTRA_FAST_MODE_FREQ) > > + else if (bitrate <= I2C_MAX_HIGH_SPEED_MODE_FREQ) > > mode = HS; > > else > > mode = ULTRA_FAST; > > @@ -209,7 +209,8 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct > > *lpi2c_imx) > > } while (1); > > } > > > > -/* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 > > */ > > +/* CLKLO = (1 - I2C_CLK_RATIO) * clk_cycle, SETHOLD = CLKHI, DATAVD = > > CLKHI/2 > > + CLKHI = I2C_CLK_RATIO * clk_cycle */ > > static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx) { > > u8 prescale, filt, sethold, clkhi, clklo, datavd; @@ -232,8 +233,8 > > @@ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx) > > > > for (prescale = 0; prescale <= 7; prescale++) { > > clk_cycle = clk_rate / ((1 << prescale) * lpi2c_imx->bitrate) > > - - 3 - (filt >> 1); > > - clkhi = (clk_cycle + I2C_CLK_RATIO) / (I2C_CLK_RATIO + 1); > > + - (2 + filt) / (1 << prescale); > > + clkhi = clk_cycle * I2C_CLK_RATIO; > > clklo = clk_cycle - clkhi; > > if (clklo < 64) > > break; > > -- > > 2.25.1
> -----Original Message----- > From: Clark Wang <xiaoning.wang@nxp.com> > Sent: Friday, March 19, 2021 14:16 > To: Aisheng Dong <aisheng.dong@nxp.com>; shawnguo@kernel.org; > s.hauer@pengutronix.de > Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux- > imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support > > > > -----Original Message----- > > From: Aisheng Dong <aisheng.dong@nxp.com> > > Sent: Friday, March 19, 2021 12:40 > > To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org; > > s.hauer@pengutronix.de > > Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux- > > imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com; > > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > > kernel@vger.kernel.org > > Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support > > > > > From: Clark Wang <xiaoning.wang@nxp.com> > > > Sent: Wednesday, March 17, 2021 2:54 PM > > > Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support > > > > > > - Add runtime pm support to dynamicly manage the clock. > > > - Put the suspend to suspend_noirq. > > > - Call .pm_runtime_force_suspend() to force runtime pm suspended > > > in .suspend_noirq(). > > > > > > > The patch title needs to be improved as the driver already supports rpm. > > And do one thing in one patch. > > > > > Signed-off-by: Fugang Duan <fugang.duan@nxp.com> > > > Signed-off-by: Gao Pan <pandy.gao@nxp.com> > > > Reviewed-by: Anson Huang <Anson.Huang@nxp.com> > > > > Please add your sign-off. > > > > > --- > > > drivers/i2c/busses/i2c-imx-lpi2c.c | 50 > > > ++++++++++++++++++++---------- > > > 1 file changed, 33 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > > > b/drivers/i2c/busses/i2c-imx-lpi2c.c > > > index bbf44ac95021..1e920e7ac7c1 100644 > > > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > > > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > > > @@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device > > > *pdev) > > > if (ret) > > > lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ; > > > > > > - ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0, > > > + ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, > > > + IRQF_NO_SUSPEND, > > > > This belongs to a separate patch > > > > > pdev->name, lpi2c_imx); > > > if (ret) { > > > dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ - > > 584,35 > > > +585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev) > > > i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx); > > > platform_set_drvdata(pdev, lpi2c_imx); > > > > > > - ret = clk_prepare_enable(lpi2c_imx->clk); > > > - if (ret) { > > > - dev_err(&pdev->dev, "clk enable failed %d\n", ret); > > > - return ret; > > > - } > > > - > > > pm_runtime_set_autosuspend_delay(&pdev->dev, > > I2C_PM_TIMEOUT); > > > pm_runtime_use_autosuspend(&pdev->dev); > > > - pm_runtime_get_noresume(&pdev->dev); > > > - pm_runtime_set_active(&pdev->dev); > > > pm_runtime_enable(&pdev->dev); > > > > > > + ret = pm_runtime_get_sync(&pdev->dev); > > > + if (ret < 0) { > > > + pm_runtime_put_noidle(&pdev->dev); > > > + dev_err(&pdev->dev, "failed to enable clock\n"); > > > + return ret; > > > + } > > > > Can't current clk control via rpm work well? > > Please describe why need change. > > I think the previous patch maker might want to use the return value of > pm_runtime_get_sync to check whether the clock has been turned on > correctly to > avoid the kernel panic. > Maybe I can change to the method like this. > pm_runtime_get_noresume(&pdev->dev); > ret = pm_runtime_set_active(&pdev->dev); > if (ret < 0) > goto out; > pm_runtime_enable(&pdev->dev); > > Best Regards, > Clark Wang Sorry, I missed the point before. If we use pm_runtime_get_noresume(&pdev->dev); and pm_runtime_set_active(&pdev->dev); here, the clk should be enabled by using clk_prepare_enable() in the probe function. However, the call of clk_prepare_enable() is already in lpi2c_runtime_resume(). Using get_sync() here can help to reduce the repetitive code, especially ipg clk will be added later. Shall we change to use pm_runtime_get_sync() here? Regards, Clark Wang
> > > + > > > + lpi2c_imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg"); > > > + if (IS_ERR(lpi2c_imx->clk_ipg)) { > > > + dev_err(&pdev->dev, "can't get I2C ipg clock\n"); > > > + return PTR_ERR(lpi2c_imx->clk_ipg); > > > } > > > > Will this break exist dts? > > It will not break the build. But will break the lpi2c probe for imx7ulp and > imx8qxp/qm. > I will send two patches to update dts in V2. > Please don't break exist platforms. Regards Aisheng > Best Regards, > Clark Wang > > > > > Regards > > Aisheng > > > > > > > > ret = of_property_read_u32(pdev->dev.of_node, > > > @@ -633,7 +640,8 @@ static int __maybe_unused > > > lpi2c_runtime_suspend(struct device *dev) { > > > struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev); > > > > > > - clk_disable_unprepare(lpi2c_imx->clk); > > > + clk_disable_unprepare(lpi2c_imx->clk_ipg); > > > + clk_disable_unprepare(lpi2c_imx->clk_per); > > > pinctrl_pm_select_idle_state(dev); > > > > > > return 0; > > > @@ -645,12 +653,18 @@ static int __maybe_unused > > > lpi2c_runtime_resume(struct device *dev) > > > int ret; > > > > > > pinctrl_pm_select_default_state(dev); > > > - ret = clk_prepare_enable(lpi2c_imx->clk); > > > + ret = clk_prepare_enable(lpi2c_imx->clk_per); > > > if (ret) { > > > - dev_err(dev, "can't enable I2C clock, ret=%d\n", ret); > > > + dev_err(dev, "can't enable I2C per clock, ret=%d\n", ret); > > > return ret; > > > } > > > > > > + ret = clk_prepare_enable(lpi2c_imx->clk_ipg); > > > + if (ret) { > > > + clk_disable_unprepare(lpi2c_imx->clk_per); > > > + dev_err(dev, "can't enable I2C ipg clock, ret=%d\n", ret); > > > + } > > > + > > > return ret; > > > } > > > > > > -- > > > 2.25.1
[...] > > > > pm_runtime_set_autosuspend_delay(&pdev->dev, > > > I2C_PM_TIMEOUT); > > > > pm_runtime_use_autosuspend(&pdev->dev); > > > > - pm_runtime_get_noresume(&pdev->dev); > > > > - pm_runtime_set_active(&pdev->dev); > > > > pm_runtime_enable(&pdev->dev); > > > > > > > > + ret = pm_runtime_get_sync(&pdev->dev); > > > > + if (ret < 0) { > > > > + pm_runtime_put_noidle(&pdev->dev); > > > > + dev_err(&pdev->dev, "failed to enable clock\n"); > > > > + return ret; > > > > + } > > > > > > Can't current clk control via rpm work well? > > > Please describe why need change. > > > > I think the previous patch maker might want to use the return value of > > pm_runtime_get_sync to check whether the clock has been turned on > > correctly to > > avoid the kernel panic. > > Maybe I can change to the method like this. > > pm_runtime_get_noresume(&pdev->dev); > > ret = pm_runtime_set_active(&pdev->dev); > > if (ret < 0) > > goto out; > > pm_runtime_enable(&pdev->dev); > > > > Best Regards, > > Clark Wang > > Sorry, I missed the point before. > If we use pm_runtime_get_noresume(&pdev->dev); and > pm_runtime_set_active(&pdev->dev); here, the clk should be enabled by using > clk_prepare_enable() in the probe function. However, the call of > clk_prepare_enable() is already in lpi2c_runtime_resume(). > Using get_sync() here can help to reduce the repetitive code, especially ipg > clk will be added later. Let's not do for this negligible improvement unless there're any other good benefits. If really care, move clk operation into an inline function instead. Another benefit if not doing that is the driver still can work even RPM not enabled. Regards Aisheng > Shall we change to use pm_runtime_get_sync() here? > > Regards, > Clark Wang >