lkml.org 
[lkml]   [2022]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2 v5] regmap: mmio: Support accelerared noinc operations
    On Tue, Aug 16, 2022 at 10:48:32PM +0200, Linus Walleij wrote:
    > Use the newly added callback for accelerated noinc MMIO
    > to provide writesb, writesw, writesl, writesq, readsb, readsw,
    > readsl and readsq.
    >
    > A special quirk is needed to deal with big endian regmaps: there
    > are no accelerated operations defined for big endian, so fall
    > back to calling the big endian operations itereatively for this
    > case.
    >
    > The Hexagon architecture turns out to have an incomplete
    > <asm/io.h>: writesb() is not implemented. Fix this by doing
    > what other architectures do: include <asm-generic/io.h> into
    > the <asm/io.h> file.

    Wonderful!

    So, I have seen a recent blow up by kernel bot due to Alpha issues on these
    accessors.

    Also see below for the further followups.

    ...

    > + if (!IS_ERR(ctx->clk)) {
    > + ret = clk_enable(ctx->clk);
    > + if (ret < 0)
    > + return ret;
    > + }

    It's a new place of the duplicating check, can we have a helper for that?

    ...

    > + /*
    > + * There are no native, assembly-optimized write single register
    > + * operations for big endian, so fall back to emulation if this
    > + * is needed. (Single bytes are fine, they are not affected by
    > + * endianness.)
    > + */

    Wouldn't be faster to memdup() / swap / use corresponding IO accessor?

    ...

    > + /*
    > + * There are no native, assembly-optimized write single register
    > + * operations for big endian, so fall back to emulation if this
    > + * is needed. (Single bytes are fine, they are not affected by
    > + * endianness.)
    > + */
    > + if (ctx->big_endian && (ctx->val_bytes > 1)) {
    > + switch (ctx->val_bytes) {
    > + case 2:
    > + {
    > + u16 *valp = (u16 *)val;
    > + for (i = 0; i < val_count; i++)
    > + valp[i] = swab16(valp[i]);
    > + break;
    > + }
    > + case 4:
    > + {
    > + u32 *valp = (u32 *)val;
    > + for (i = 0; i < val_count; i++)
    > + valp[i] = swab32(valp[i]);
    > + break;
    > + }
    > +#ifdef CONFIG_64BIT
    > + case 8:
    > + {
    > + u64 *valp = (u64 *)val;
    > + for (i = 0; i < val_count; i++)
    > + valp[i] = swab64(valp[i]);
    > + break;
    > + }
    > +#endif
    > + default:
    > + ret = -EINVAL;
    > + break;
    > + }
    > + }

    So, two questions here:

    1) can we use helpers from include/linux/byteorder/generic.h, such as
    cpu_to_be32_array()/be32_to_cpu_array()?

    2) have you considered using memcpy_toio() / memcpy_fromio() and why
    it's not okay to use them?

    ...

    > +
    > +

    Single blank line is enough.

    > +out_clk:
    > + if (!IS_ERR(ctx->clk))
    > + clk_disable(ctx->clk);
    > +
    > + return ret;
    > +
    > + return 0;

    Seems like misrebase? I believe this has to be fixed.

    --
    With Best Regards,
    Andy Shevchenko


    \
     
     \ /
      Last update: 2022-08-30 18:40    [W:2.416 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site