[v1,v1,0/3] Add dt bindings to support I2C on sifive devices and a fix for polling mode i2c transfers.

The patch is based on mainline v5.1 and is intended to add DT-bindings for Opencore based I2C device 
support in FU540 SoC, available on HiFive unleashed board (Rev A00), and also provide a workaround to 
make I2C polling mode interface work with FU540 chipsets.

The polling mode workaround patch fixes the CPU stall issue, when-ever i2c transfer are initiated 

This workaround checks if it's a FU540 chipset based on device tree information, and check's for open
core's IF(interrupt flag) and TIP flags to break from the polling loop upon completion of transfer.

To test the patch, a PMOD-AD2 sensor is connected to HiFive Unleashed board over J1 connector, and 
appropriate device node is added into board specific device tree as per the information provided in 
dt-bindings in Documentation/devicetree/bindings/i2c/i2c-sifive.txt.
Without this workaround, the CPU stall's infinitely.

Busybox i2c utilities used to verify workaround : i2cdetect, i2cdump, i2cset, i2cget


Sagar Shrikant Kadam (3):
  dt-bindings: i2c: add documentation for adding SiFive I2C driver
  i2c-ocore: sifive: add support for i2c device on FU540-c000 SoC.
  i2c-ocores: sifive: add polling mode workaround for FU540-C000 SoC.

 .../devicetree/bindings/i2c/i2c-sifive.txt         | 29 +++++++++++++++++++
 drivers/i2c/busses/i2c-ocores.c                    | 33 +++++++++++++++++++---
 2 files changed, 58 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sifive.txt

>  /*
>   * '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