| Message ID | 20200814210154.14402-1-mab@mab-labs.com |
|---|---|
| State | Needs Review / ACK |
| Headers | show |
| Series | i2c: ocores: Allow endian-specific grlib accessors | expand |
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
Andrew,
Thanks for your comments. Does it make sense to replace the big_endian
bool with a small_endian bool? The code to choose the appropriate
non-grlib accessors assumes that big_endian will be specified, either
in a device tree blob or via platform_data:
if (!i2c->setreg || !i2c->getreg) {
bool be = pdata ? pdata->big_endian :
of_device_is_big_endian(pdev->dev.of_node);
.
.
.
case 2:
i2c->setreg = be ? oc_setreg_16be : oc_setreg_16;
And so if endianess isn't specified (assuming the default is big
endian), it will actually default to small endian.
Thanks
On Mon, Aug 17, 2020 at 9:34 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> 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
On Tue, Aug 18, 2020 at 06:14:31PM -0400, Mohammed Billoo wrote: > Andrew, > > Thanks for your comments. Does it make sense to replace the big_endian > bool with a small_endian bool? The code to choose the appropriate > non-grlib accessors assumes that big_endian will be specified, either > in a device tree blob or via platform_data: > > if (!i2c->setreg || !i2c->getreg) { > bool be = pdata ? pdata->big_endian : > of_device_is_big_endian(pdev->dev.of_node); > . > . > . > case 2: > i2c->setreg = be ? oc_setreg_16be : oc_setreg_16; > > And so if endianess isn't specified (assuming the default is big > endian), it will actually default to small endian. You have to assume there is no indication of endianness in device tree by default. For your broken hardware you will indicate little endian in device tree. If you want, you could add support for DT indicating big endian, but it is not required. Andrew
Apologies for belaboring this point, but doesn't your logic mean that
the code for selecting the "standard" (i.e. non-grlib) accessors is
wrong? Putting my broken HW aside, if a device tree doesn't specify
big_endian, assuming that the default is big_endian, then won't these
lines of code, which are in the mainline driver:
bool be = pdata ? pdata->big_endian :
of_device_is_big_endian(pdev->dev.of_node);
.
.
.
case 2:
i2c->setreg = be ? oc_setreg_16be : oc_setreg_16;
i2c->getreg = be ? oc_getreg_16be : oc_getreg_16;
break;
then use the little endian (i.e. oc_setreg_16) accessors? If so,
shouldn't the use of big_endian in this driver be replaced with
little_endian, and the corresponding code updated?
On Tue, Aug 18, 2020 at 10:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Aug 18, 2020 at 06:14:31PM -0400, Mohammed Billoo wrote:
> > Andrew,
> >
> > Thanks for your comments. Does it make sense to replace the big_endian
> > bool with a small_endian bool? The code to choose the appropriate
> > non-grlib accessors assumes that big_endian will be specified, either
> > in a device tree blob or via platform_data:
> >
> > if (!i2c->setreg || !i2c->getreg) {
> > bool be = pdata ? pdata->big_endian :
> > of_device_is_big_endian(pdev->dev.of_node);
> > .
> > .
> > .
> > case 2:
> > i2c->setreg = be ? oc_setreg_16be : oc_setreg_16;
> >
> > And so if endianess isn't specified (assuming the default is big
> > endian), it will actually default to small endian.
>
> You have to assume there is no indication of endianness in device tree
> by default. For your broken hardware you will indicate little endian
> in device tree. If you want, you could add support for DT indicating
> big endian, but it is not required.
>
> Andrew
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c index f5fc75b65a19..2ef735f8c71f 100644 --- a/drivers/i2c/busses/i2c-ocores.c +++ b/drivers/i2c/busses/i2c-ocores.c @@ -488,11 +488,12 @@ MODULE_DEVICE_TABLE(of, ocores_i2c_match); #ifdef CONFIG_OF /* - * Read and write functions for the GRLIB port of the controller. Registers are - * 32-bit big endian and the PRELOW and PREHIGH registers are merged into one + * Read and write functions for the GRLIB port of the controller. Unfortunately, + * do to some broken/inconsistent HW, SW may need to account for different + * endianess of GRLIB. PRELOW and PREHIGH registers are merged into one * register. The subsequent registers have their offsets decreased accordingly. */ -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; } -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; +} + +static void oc_setreg_grlib_be(struct ocores_i2c *i2c, int reg, u8 value) { u32 curr, wr; int rreg = reg; @@ -525,6 +540,25 @@ static void oc_setreg_grlib(struct ocores_i2c *i2c, int reg, u8 value) iowrite32be(wr, i2c->base + (rreg << i2c->reg_shift)); } +static void oc_setreg_grlib_le(struct ocores_i2c *i2c, int reg, u8 value) +{ + u32 curr, wr; + int rreg = reg; + + if (reg != OCI2C_PRELOW) + rreg--; + if (reg == OCI2C_PRELOW || reg == OCI2C_PREHIGH) { + curr = ioread32(i2c->base + (rreg << i2c->reg_shift)); + if (reg == OCI2C_PRELOW) + wr = (curr & 0xff00) | value; + else + wr = (((u32)value) << 8) | (curr & 0xff); + } else { + wr = value; + } + iowrite32(wr, i2c->base + (rreg << i2c->reg_shift)); +} + static int ocores_i2c_of_probe(struct platform_device *pdev, struct ocores_i2c *i2c) { @@ -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; + } } return 0;
Due to inconsistent/broken HW, SW may need to set the appropriate endianess of the grlib accessors (instead of defaulting to big endian). This patch adds such a capability. Signed-off-by: Mohammed Billoo <mab@mab-labs.com> --- drivers/i2c/busses/i2c-ocores.c | 55 +++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 6 deletions(-)