| Message ID | 1557147240-29551-4-git-send-email-sagar.kadam@sifive.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add dt bindings to support I2C on sifive devices and a fix for polling mode i2c transfers. | expand |
> /* > * 'process_lock' exists because ocores_process() and ocores_process_timeout() > @@ -239,8 +240,13 @@ static irqreturn_t ocores_isr(int irq, void *dev_id) > struct ocores_i2c *i2c = dev_id; > u8 stat = oc_getreg(i2c, OCI2C_STATUS); > > - if (!(stat & OCI2C_STAT_IF)) > + if (i2c->flags && SIFIVE_FLAG_POLL) { Do you really want && here? > + if (stat & OCI2C_STAT_IF) > + if (!(stat & OCI2C_STAT_BUSY)) > + return IRQ_NONE; > + } else if (!(stat & OCI2C_STAT_IF)) { > return IRQ_NONE; > + } > > ocores_process(i2c, stat); > > @@ -356,6 +362,11 @@ static void ocores_process_polling(struct ocores_i2c *i2c) > ret = ocores_isr(-1, i2c); > if (ret == IRQ_NONE) > break; /* all messages have been transferred */ > + else { > + if (i2c->flags && SIFIVE_FLAG_POLL) And here? > + if (i2c->state == STATE_DONE) > + break; > + } > } > } > > @@ -406,7 +417,7 @@ static int ocores_xfer(struct i2c_adapter *adap, > { > struct ocores_i2c *i2c = i2c_get_adapdata(adap); > > - if (i2c->flags & OCORES_FLAG_POLL) > + if ((i2c->flags & OCORES_FLAG_POLL) || (i2c->flags & SIFIVE_FLAG_POLL)) You can combine this if ((i2c->flags & (OCORES_FLAG_POLL | SIFIVE_FLAG_POLL)) > return ocores_xfer_polling(adap, msgs, num); > return ocores_xfer_core(i2c, msgs, num, false); > } > @@ -597,6 +608,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) > { > struct ocores_i2c *i2c; > struct ocores_i2c_platform_data *pdata; > + const struct of_device_id *match; > struct resource *res; > int irq; > int ret; > @@ -678,13 +690,21 @@ static int ocores_i2c_probe(struct platform_device *pdev) > > irq = platform_get_irq(pdev, 0); > if (irq == -ENXIO) { > - i2c->flags |= OCORES_FLAG_POLL; > + /* > + * Set a SIFIVE_FLAG_POLL to enable workaround for FU540 > + * in polling mode interface of i2c-ocore driver. > + */ > + match = of_match_node(ocores_i2c_match, pdev->dev.of_node); > + if (match && (long)match->data == TYPE_SIFIVE_REV0) > + i2c->flags |= SIFIVE_FLAG_POLL; > + else > + i2c->flags |= OCORES_FLAG_POLL; Please take a look at the whole code, and consider if it is better to set both SIFIVE_FLAG_POLL and OCORES_FLAG_POLL. Maybe rename SIFIVE_FLAG_POLL to OCORES_FLAG_BROKEN_IRQ_BIT? Thanks Andrew
On Mon, May 6, 2019 at 6:59 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > /* > > * 'process_lock' exists because ocores_process() and ocores_process_timeout() > > @@ -239,8 +240,13 @@ static irqreturn_t ocores_isr(int irq, void *dev_id) > > struct ocores_i2c *i2c = dev_id; > > u8 stat = oc_getreg(i2c, OCI2C_STATUS); > > > > - if (!(stat & OCI2C_STAT_IF)) > > + if (i2c->flags && SIFIVE_FLAG_POLL) { > > Do you really want && here? > > > + if (stat & OCI2C_STAT_IF) > > + if (!(stat & OCI2C_STAT_BUSY)) > > + return IRQ_NONE; > > + } else if (!(stat & OCI2C_STAT_IF)) { > > return IRQ_NONE; > > + } > > > > ocores_process(i2c, stat); > > > > @@ -356,6 +362,11 @@ static void ocores_process_polling(struct ocores_i2c *i2c) > > ret = ocores_isr(-1, i2c); > > if (ret == IRQ_NONE) > > break; /* all messages have been transferred */ > > + else { > > + if (i2c->flags && SIFIVE_FLAG_POLL) > > And here? > > > + if (i2c->state == STATE_DONE) > > + break; > > + } > > } > > } > > > > @@ -406,7 +417,7 @@ static int ocores_xfer(struct i2c_adapter *adap, > > { > > struct ocores_i2c *i2c = i2c_get_adapdata(adap); > > > > - if (i2c->flags & OCORES_FLAG_POLL) > > + if ((i2c->flags & OCORES_FLAG_POLL) || (i2c->flags & SIFIVE_FLAG_POLL)) > > You can combine this Thanks for your suggestion's Andrew. Yes, I will optimize this. > > if ((i2c->flags & (OCORES_FLAG_POLL | SIFIVE_FLAG_POLL)) > > > return ocores_xfer_polling(adap, msgs, num); > > return ocores_xfer_core(i2c, msgs, num, false); > > } > > @@ -597,6 +608,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) > > { > > struct ocores_i2c *i2c; > > struct ocores_i2c_platform_data *pdata; > > + const struct of_device_id *match; > > struct resource *res; > > int irq; > > int ret; > > @@ -678,13 +690,21 @@ static int ocores_i2c_probe(struct platform_device *pdev) > > > > irq = platform_get_irq(pdev, 0); > > if (irq == -ENXIO) { > > - i2c->flags |= OCORES_FLAG_POLL; > > + /* > > + * Set a SIFIVE_FLAG_POLL to enable workaround for FU540 > > + * in polling mode interface of i2c-ocore driver. > > + */ > > + match = of_match_node(ocores_i2c_match, pdev->dev.of_node); > > + if (match && (long)match->data == TYPE_SIFIVE_REV0) > > + i2c->flags |= SIFIVE_FLAG_POLL; > > + else > > + i2c->flags |= OCORES_FLAG_POLL; > > Please take a look at the whole code, and consider if it is better to > set both SIFIVE_FLAG_POLL and OCORES_FLAG_POLL. Maybe rename > SIFIVE_FLAG_POLL to OCORES_FLAG_BROKEN_IRQ_BIT? > The intent of this patch is to add a workaround for hardware errratum of FU540 a SiFive Device, hence I had named the flag accordingly. Yes, OCORES_FLAG_BROKEN_IRQ_BIT is a better and generic term, I will rename and resubmit this patch -Thanks Sagar > Thanks > Andrew
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c index 7bf7b0c..08cdfed 100644 --- a/drivers/i2c/busses/i2c-ocores.c +++ b/drivers/i2c/busses/i2c-ocores.c @@ -27,6 +27,7 @@ #include <linux/jiffies.h> #define OCORES_FLAG_POLL BIT(0) +#define SIFIVE_FLAG_POLL BIT(1) /* * 'process_lock' exists because ocores_process() and ocores_process_timeout() @@ -239,8 +240,13 @@ static irqreturn_t ocores_isr(int irq, void *dev_id) struct ocores_i2c *i2c = dev_id; u8 stat = oc_getreg(i2c, OCI2C_STATUS); - if (!(stat & OCI2C_STAT_IF)) + if (i2c->flags && SIFIVE_FLAG_POLL) { + if (stat & OCI2C_STAT_IF) + if (!(stat & OCI2C_STAT_BUSY)) + return IRQ_NONE; + } else if (!(stat & OCI2C_STAT_IF)) { return IRQ_NONE; + } ocores_process(i2c, stat); @@ -356,6 +362,11 @@ static void ocores_process_polling(struct ocores_i2c *i2c) ret = ocores_isr(-1, i2c); if (ret == IRQ_NONE) break; /* all messages have been transferred */ + else { + if (i2c->flags && SIFIVE_FLAG_POLL) + if (i2c->state == STATE_DONE) + break; + } } } @@ -406,7 +417,7 @@ static int ocores_xfer(struct i2c_adapter *adap, { struct ocores_i2c *i2c = i2c_get_adapdata(adap); - if (i2c->flags & OCORES_FLAG_POLL) + if ((i2c->flags & OCORES_FLAG_POLL) || (i2c->flags & SIFIVE_FLAG_POLL)) return ocores_xfer_polling(adap, msgs, num); return ocores_xfer_core(i2c, msgs, num, false); } @@ -597,6 +608,7 @@ static int ocores_i2c_probe(struct platform_device *pdev) { struct ocores_i2c *i2c; struct ocores_i2c_platform_data *pdata; + const struct of_device_id *match; struct resource *res; int irq; int ret; @@ -678,13 +690,21 @@ static int ocores_i2c_probe(struct platform_device *pdev) irq = platform_get_irq(pdev, 0); if (irq == -ENXIO) { - i2c->flags |= OCORES_FLAG_POLL; + /* + * Set a SIFIVE_FLAG_POLL to enable workaround for FU540 + * in polling mode interface of i2c-ocore driver. + */ + match = of_match_node(ocores_i2c_match, pdev->dev.of_node); + if (match && (long)match->data == TYPE_SIFIVE_REV0) + i2c->flags |= SIFIVE_FLAG_POLL; + else + i2c->flags |= OCORES_FLAG_POLL; } else { if (irq < 0) return irq; } - if (!(i2c->flags & OCORES_FLAG_POLL)) { + if (!(i2c->flags & (OCORES_FLAG_POLL | SIFIVE_FLAG_POLL))) { ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0, pdev->name, i2c); if (ret) {
The i2c-ocore driver already has a polling mode interface.But it needs a workaround for FU540 Chipset on HiFive unleashed board (RevA00). There is an erratum in FU540 chip that prevents interrupt driven i2c transfers from working, and also the I2C controller's interrupt bit cannot be cleared if set, due to this the existing i2c polling mode interface added in mainline earlier doesn't work, and CPU stall's infinitely, when-ever i2c transfer is initiated. commit 69c8c0c0efa8 ("i2c: ocores: add polling interface") The workaround / fix is particularly for FU540-COOO SoC. Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> --- drivers/i2c/busses/i2c-ocores.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)