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@xxxxxxx> 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
--
Mohammed A Billoo
Founder
MAB Labs, LLC
www.mab-labs.com
201-338-2022
