Re: [PATCH] i2c: ocores: Allow endian-specific grlib accessors

[Andrew Lunn <andrew@lunn.ch>]

On Fri, Aug 14, 2020 at 05:01:54PM -0400, Mohammed Billoo wrote:
> Due to inconsistent/broken HW, SW may need to set the appropriate
> endianess of the grlib accessors (instead of defaulting to big endian).

I think you have this wrong.

> -static u8 oc_getreg_grlib(struct ocores_i2c *i2c, int reg)
> +static u8 oc_getreg_grlib_be(struct ocores_i2c *i2c, int reg)
>  {
>  	u32 rd;
>  	int rreg = reg;
> @@ -506,7 +507,21 @@ static u8 oc_getreg_grlib(struct ocores_i2c *i2c, int reg)
>  		return (u8)rd;
>  }

So the existing code is big endian.


> -static void oc_setreg_grlib(struct ocores_i2c *i2c, int reg, u8 value)
> +static u8 oc_getreg_grlib_le(struct ocores_i2c *i2c, int reg)
> +{
> +	u32 rd;
> +	int rreg = reg;
> +
> +	if (reg != OCI2C_PRELOW)
> +		rreg--;
> +	rd = ioread32(i2c->base + (rreg << i2c->reg_shift));
> +	if (reg == OCI2C_PREHIGH)
> +		return (u8)(rd >> 8);
> +	else
> +		return (u8)rd;
> +}

You are adding little endian accesses.

> @@ -592,8 +626,17 @@ static int ocores_i2c_of_probe(struct platform_device *pdev,
>  	match = of_match_node(ocores_i2c_match, pdev->dev.of_node);
>  	if (match && (long)match->data == TYPE_GRLIB) {
>  		dev_dbg(&pdev->dev, "GRLIB variant of i2c-ocores\n");
> -		i2c->setreg = oc_setreg_grlib;
> -		i2c->getreg = oc_getreg_grlib;
> +		/*
> +		 * This is a workaround for inconsistent/broken HW,
> +		 * where SW has to set the appropriate endianess
> +		 */
> +		if (of_device_is_big_endian(pdev->dev.of_node)) {
> +			i2c->setreg = oc_setreg_grlib_be;
> +			i2c->getreg = oc_getreg_grlib_be;
> +		} else {
> +			i2c->setreg = oc_setreg_grlib_le;
> +			i2c->getreg = oc_getreg_grlib_le;
> +		}

Existing device tree blobs won't indicate an endianess. They assume
big endian is the default. But you are changing that, they now need to
indicate they are big endian. And they won't, so you will break them.

For you specific platform, you need to indicate in device tree it
needs little endian, by adding a property.

Please also document the property you add in i2c-ocores.txt.

      Andrew