diff mbox series

[v3,09/16] reset: starfive-jh7100: Add StarFive JH7100 reset driver

Message ID 20211102161125.1144023-10-kernel@esmil.dk (mailing list archive)
State Not Applicable, archived
Headers show
Series Basic StarFive JH7100 RISC-V SoC support | expand

Commit Message

Emil Renner Berthing Nov. 2, 2021, 4:11 p.m. UTC
Add a driver for the StarFive JH7100 reset controller.

Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
---
 MAINTAINERS                           |   7 +
 drivers/reset/Kconfig                 |   7 +
 drivers/reset/Makefile                |   1 +
 drivers/reset/reset-starfive-jh7100.c | 178 ++++++++++++++++++++++++++
 4 files changed, 193 insertions(+)
 create mode 100644 drivers/reset/reset-starfive-jh7100.c

Comments

Andy Shevchenko Nov. 2, 2021, 7:42 p.m. UTC | #1
+Cc: Yury (bitmap expert)

On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
>
> Add a driver for the StarFive JH7100 reset controller.

...

> +#define BIT_MASK32(x) BIT((x) % 32)

Possible namespace collision.

...

> +/*
> + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> + * same line.
> + * most reset lines have their status inverted so a 0 in the STATUS register
> + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> + * though, so store the expected value of the status registers when all lines
> + * are asserted.
> + */

Besides missing capitalization, if it sounds like bitmap, use bitmap.
I have checked DT definitions and it seems you don't even need the
BIT_MASK() macro,

> +static const u32 jh7100_reset_asserted[4] = {
> +       /* STATUS0 register */
> +       BIT_MASK32(JH7100_RST_U74) |
> +       BIT_MASK32(JH7100_RST_VP6_DRESET) |
> +       BIT_MASK32(JH7100_RST_VP6_BRESET),
> +       /* STATUS1 register */
> +       BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> +       BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> +       /* STATUS2 register */
> +       BIT_MASK32(JH7100_RST_E24),
> +       /* STATUS3 register */
> +       0,
> +};

Yury, do we have any clever (clean) way to initialize a bitmap with
particular bits so that it will be a constant from the beginning? If
no, any suggestion what we can provide to such users?

...

> +       dev_dbg(rcdev->dev, "reset(%lu)\n", id);

These debug messages are useless since one should use ftrace facility instead,
Emil Renner Berthing Nov. 2, 2021, 7:58 p.m. UTC | #2
On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> +Cc: Yury (bitmap expert)
>
> On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> >
> > Add a driver for the StarFive JH7100 reset controller.
>
> ...
>
> > +#define BIT_MASK32(x) BIT((x) % 32)
>
> Possible namespace collision.
>
> ...
>
> > +/*
> > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > + * same line.
> > + * most reset lines have their status inverted so a 0 in the STATUS register
> > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > + * though, so store the expected value of the status registers when all lines
> > + * are asserted.
> > + */
>
> Besides missing capitalization,

I'm confused. it was you who wanted all comments to capitalized the same..
64bi
if it sounds like bitmap, use bitmap.
> I have checked DT definitions and it seems you don't even need the
> BIT_MASK() macro,
>
> > +static const u32 jh7100_reset_asserted[4] = {
> > +       /* STATUS0 register */
> > +       BIT_MASK32(JH7100_RST_U74) |
> > +       BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > +       BIT_MASK32(JH7100_RST_VP6_BRESET),
> > +       /* STATUS1 register */
> > +       BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > +       BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > +       /* STATUS2 register */
> > +       BIT_MASK32(JH7100_RST_E24),
> > +       /* STATUS3 register */
> > +       0,
> > +};
>
> Yury, do we have any clever (clean) way to initialize a bitmap with
> particular bits so that it will be a constant from the beginning? If
> no, any suggestion what we can provide to such users?

The problem is, that even if we could initialize this without the
monstrosity in our last conversation a 64bit bitmap would still
produce worse code. As it is now it's simply a 32bit load and mask
with index and mask already calculated for the registers. In the
status callback the mask can even be folded into the register read
mask. With a 64bit bitmap you'd need to calculate new 64bit index and
masks, and then conditionally shift the bits into position.

If this reflection of the 32bit registers bothers you that much we
could alternatively do

static bool jh7100_reset_inverted(unsigned int idx)
{
  switch (idx) {
  case JH7100_RST_U74:
  case JH7100_RST_VP6_DRESET:
  ..
    return false;
  }
  return true;
}

It'd still produce worse code, but at least it would be readable.

/Emil

> ...
>
> > +       dev_dbg(rcdev->dev, "reset(%lu)\n", id);
>
> These debug messages are useless since one should use ftrace facility instead,
>
> --
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko Nov. 2, 2021, 8:13 p.m. UTC | #3
On Tue, Nov 2, 2021 at 9:59 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:

...

> > > +/*
> > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > > + * same line.
> > > + * most reset lines have their status inverted so a 0 in the STATUS register
> > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > > + * though, so store the expected value of the status registers when all lines
> > > + * are asserted.
> > > + */
> >
> > Besides missing capitalization,
>
> I'm confused. it was you who wanted all comments to capitalized the same..

Yes and there are two types of the comments, one-liners and
multi-line. In multi-line you usually use proper English grammar,
where capitalization means what it means. For the one-liners just
choose either small letters or capital letters to start them with.

> 64bi

Something is missing here.

> if it sounds like bitmap, use bitmap.
> > I have checked DT definitions and it seems you don't even need the
> > BIT_MASK() macro,
> >
> > > +static const u32 jh7100_reset_asserted[4] = {
> > > +       /* STATUS0 register */
> > > +       BIT_MASK32(JH7100_RST_U74) |
> > > +       BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > > +       BIT_MASK32(JH7100_RST_VP6_BRESET),
> > > +       /* STATUS1 register */
> > > +       BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > > +       BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > > +       /* STATUS2 register */
> > > +       BIT_MASK32(JH7100_RST_E24),
> > > +       /* STATUS3 register */
> > > +       0,
> > > +};
> >
> > Yury, do we have any clever (clean) way to initialize a bitmap with
> > particular bits so that it will be a constant from the beginning? If
> > no, any suggestion what we can provide to such users?
>
> The problem is, that even if we could initialize this without the
> monstrosity in our last conversation a 64bit bitmap would still
> produce worse code. As it is now it's simply a 32bit load and mask
> with index and mask already calculated for the registers. In the
> status callback the mask can even be folded into the register read
> mask. With a 64bit bitmap you'd need to calculate new 64bit index and
> masks, and then conditionally shift the bits into position.

Why? You may use 8 byte IO (writeq() / readq() or their relaxed versions), no?

...

> If this reflection of the 32bit registers bothers you that much

What bothers me is hidden endianess issues (yeah, here it might be
theoretical, but consider that somebody will look at your code and use
it as the best example ever).
Yury Norov Nov. 2, 2021, 8:55 p.m. UTC | #4
On Tue, Nov 2, 2021 at 12:59 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
>
> On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > +Cc: Yury (bitmap expert)
> >
> > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> > >
> > > Add a driver for the StarFive JH7100 reset controller.
> >
> > ...
> >
> > > +#define BIT_MASK32(x) BIT((x) % 32)
> >
> > Possible namespace collision.
> >
> > ...
> >
> > > +/*
> > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > > + * same line.

We don't have 32-bit bitmaps. Bitmaps are always arrays of unsigned longs. On a
64-bit system this '32-bit bitmap' may be broken due to endianness issues.

> > > + * most reset lines have their status inverted so a 0 in the STATUS register
> > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > > + * though, so store the expected value of the status registers when all lines
> > > + * are asserted.
> > > + */
> >
> > Besides missing capitalization,
>
> I'm confused. it was you who wanted all comments to capitalized the same..
> 64bi
> if it sounds like bitmap, use bitmap.
> > I have checked DT definitions and it seems you don't even need the
> > BIT_MASK() macro,
> >
> > > +static const u32 jh7100_reset_asserted[4] = {
> > > +       /* STATUS0 register */
> > > +       BIT_MASK32(JH7100_RST_U74) |

I think we have no BIT_MASK32() for a good reason. Natural alignment is
always preferable.

> > > +       BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > > +       BIT_MASK32(JH7100_RST_VP6_BRESET),
> > > +       /* STATUS1 register */
> > > +       BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > > +       BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > > +       /* STATUS2 register */
> > > +       BIT_MASK32(JH7100_RST_E24),
> > > +       /* STATUS3 register */
> > > +       0,
> > > +};
> >
> > Yury, do we have any clever (clean) way to initialize a bitmap with
> > particular bits so that it will be a constant from the beginning? If
> > no, any suggestion what we can provide to such users?

If you want your array to be a true bitmap, ie, all bitmap functions should
work with it correctly, you'd initialize it like this:

static const unsigned long jh7100_reset_asserted[] = {
        BITMAP_FROM_U64(BIT_MASK(JH7100_RST_VP6_DRESET) |
                          BIT_MASK(JH7100_RST_VP6_BRESET) |
                          BIT_MASK(JH7100_RST_HIFI4_DRESET) |
                          BIT_MASK(JH7100_RST_HIFI4_BRESET)),
        BITMAP_FROM_U64(BIT_MASK(JH7100_RST_E24)),
}

Look at lib/test_bitmap.c for example, and comment to BITMAP_FROM_U64()
for internal details.

On the other hand, if you hardware has tricky requirements for bit
positions, and they should depend on endianness and/or size of
long in a way not compatible with bitmaps, you probably know better
how to handle this. Just don't refer to your structure as a bitmap.

Thanks,
Yury

> The problem is, that even if we could initialize this without the
> monstrosity in our last conversation a 64bit bitmap would still
> produce worse code. As it is now it's simply a 32bit load and mask
> with index and mask already calculated for the registers. In the
> status callback the mask can even be folded into the register read
> mask. With a 64bit bitmap you'd need to calculate new 64bit index and
> masks, and then conditionally shift the bits into position.
>
> If this reflection of the 32bit registers bothers you that much we
> could alternatively do
>
> static bool jh7100_reset_inverted(unsigned int idx)
> {
>   switch (idx) {
>   case JH7100_RST_U74:
>   case JH7100_RST_VP6_DRESET:
>   ..
>     return false;
>   }
>   return true;
> }
>
> It'd still produce worse code, but at least it would be readable.
>
> /Emil
>
> > ...
> >
> > > +       dev_dbg(rcdev->dev, "reset(%lu)\n", id);
> >
> > These debug messages are useless since one should use ftrace facility instead,
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
Emil Renner Berthing Nov. 2, 2021, 9:17 p.m. UTC | #5
On Tue, 2 Nov 2021 at 21:14, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Nov 2, 2021 at 9:59 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> > On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
>
> ...
>
> > > > +/*
> > > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > > > + * same line.
> > > > + * most reset lines have their status inverted so a 0 in the STATUS register
> > > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > > > + * though, so store the expected value of the status registers when all lines
> > > > + * are asserted.
> > > > + */
> > >
> > > Besides missing capitalization,
> >
> > I'm confused. it was you who wanted all comments to capitalized the same..
>
> Yes and there are two types of the comments, one-liners and
> multi-line. In multi-line you usually use proper English grammar,
> where capitalization means what it means. For the one-liners just
> choose either small letters or capital letters to start them with.

That sounds reasonable, it was just that you complained about
inconsistent comments in the pinctrl driver that follows the above.

> > if it sounds like bitmap, use bitmap.
> > > I have checked DT definitions and it seems you don't even need the
> > > BIT_MASK() macro,
> > >
> > > > +static const u32 jh7100_reset_asserted[4] = {
> > > > +       /* STATUS0 register */
> > > > +       BIT_MASK32(JH7100_RST_U74) |
> > > > +       BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > > > +       BIT_MASK32(JH7100_RST_VP6_BRESET),
> > > > +       /* STATUS1 register */
> > > > +       BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > > > +       BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > > > +       /* STATUS2 register */
> > > > +       BIT_MASK32(JH7100_RST_E24),
> > > > +       /* STATUS3 register */
> > > > +       0,
> > > > +};
> > >
> > > Yury, do we have any clever (clean) way to initialize a bitmap with
> > > particular bits so that it will be a constant from the beginning? If
> > > no, any suggestion what we can provide to such users?
> >
> > The problem is, that even if we could initialize this without the
> > monstrosity in our last conversation a 64bit bitmap would still
> > produce worse code. As it is now it's simply a 32bit load and mask
> > with index and mask already calculated for the registers. In the
> > status callback the mask can even be folded into the register read
> > mask. With a 64bit bitmap you'd need to calculate new 64bit index and
> > masks, and then conditionally shift the bits into position.
>
> Why? You may use 8 byte IO (writeq() / readq() or their relaxed versions), no?
>
> ...
>
> > If this reflection of the 32bit registers bothers you that much
>
> What bothers me is hidden endianess issues (yeah, here it might be
> theoretical, but consider that somebody will look at your code and use
> it as the best example ever).

Wouldn't endian issues be a reason to make sure we read 32bit
registers with 32bit reads? Or do you expect a hypothetical big-endian
StarFive SoC to also change the order of the registers?
Emil Renner Berthing Nov. 4, 2021, 12:15 p.m. UTC | #6
On Tue, 2 Nov 2021 at 22:17, Emil Renner Berthing <kernel@esmil.dk> wrote:
> On Tue, 2 Nov 2021 at 21:14, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Nov 2, 2021 at 9:59 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> > > On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> >
> > ...
> >
> > > > > +/*
> > > > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > > > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > > > > + * same line.
> > > > > + * most reset lines have their status inverted so a 0 in the STATUS register
> > > > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > > > > + * though, so store the expected value of the status registers when all lines
> > > > > + * are asserted.
> > > > > + */
> > > >
> > > > Besides missing capitalization,
> > >
> > > I'm confused. it was you who wanted all comments to capitalized the same..
> >
> > Yes and there are two types of the comments, one-liners and
> > multi-line. In multi-line you usually use proper English grammar,
> > where capitalization means what it means. For the one-liners just
> > choose either small letters or capital letters to start them with.
>
> That sounds reasonable, it was just that you complained about
> inconsistent comments in the pinctrl driver that follows the above.
>
> > > if it sounds like bitmap, use bitmap.
> > > > I have checked DT definitions and it seems you don't even need the
> > > > BIT_MASK() macro,
> > > >
> > > > > +static const u32 jh7100_reset_asserted[4] = {
> > > > > +       /* STATUS0 register */
> > > > > +       BIT_MASK32(JH7100_RST_U74) |
> > > > > +       BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > > > > +       BIT_MASK32(JH7100_RST_VP6_BRESET),
> > > > > +       /* STATUS1 register */
> > > > > +       BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > > > > +       BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > > > > +       /* STATUS2 register */
> > > > > +       BIT_MASK32(JH7100_RST_E24),
> > > > > +       /* STATUS3 register */
> > > > > +       0,
> > > > > +};
> > > >
> > > > Yury, do we have any clever (clean) way to initialize a bitmap with
> > > > particular bits so that it will be a constant from the beginning? If
> > > > no, any suggestion what we can provide to such users?
> > >
> > > The problem is, that even if we could initialize this without the
> > > monstrosity in our last conversation a 64bit bitmap would still
> > > produce worse code. As it is now it's simply a 32bit load and mask
> > > with index and mask already calculated for the registers. In the
> > > status callback the mask can even be folded into the register read
> > > mask. With a 64bit bitmap you'd need to calculate new 64bit index and
> > > masks, and then conditionally shift the bits into position.
> >
> > Why? You may use 8 byte IO (writeq() / readq() or their relaxed versions), no?
> >
> > > If this reflection of the 32bit registers bothers you that much
> >
> > What bothers me is hidden endianess issues (yeah, here it might be
> > theoretical, but consider that somebody will look at your code and use
> > it as the best example ever).
>
> Wouldn't endian issues be a reason to make sure we read 32bit
> registers with 32bit reads? Or do you expect a hypothetical big-endian
> StarFive SoC to also change the order of the registers?

Hi Andy.

I'd really like to understand your reasoning here. As far as I can
tell reading 2 adjacent 32bit registers with a 64bit read as you're
proposing is exactly what would cause endian issues. Eg. on little
endian you'd get reg0 | reg1 << 32 whereas on big-endian you'd get
reg0 << 32 | reg1.

/Emil
Andy Shevchenko Nov. 8, 2021, 9:17 a.m. UTC | #7
On Thu, Nov 04, 2021 at 01:15:46PM +0100, Emil Renner Berthing wrote:
> On Tue, 2 Nov 2021 at 22:17, Emil Renner Berthing <kernel@esmil.dk> wrote:

...

> I'd really like to understand your reasoning here. As far as I can
> tell reading 2 adjacent 32bit registers with a 64bit read as you're
> proposing is exactly what would cause endian issues. Eg. on little
> endian you'd get reg0 | reg1 << 32 whereas on big-endian you'd get
> reg0 << 32 | reg1.

Nope, it won't. The endianess is a property of both CPU and device.

The I/O accessors, such as readl()/writel() and iowrtieXX()/ioreadXX()
are _always_ LE.

So, writeq() will properly put bits to their places in case device is LE.
And most devices are LE (or should be). Of course there are cases, but then
you have to specify them explicitly.

My motive here is simple as that the device is definitely a set of a few
128-bit bitmaps (in registers) and using bitmap _is_ representing hardware
in the kernel. Using something else will deviate from that (maybe not too
far, but still...).
Emil Renner Berthing Nov. 9, 2021, 9:28 a.m. UTC | #8
On Mon, 8 Nov 2021 at 10:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Nov 04, 2021 at 01:15:46PM +0100, Emil Renner Berthing wrote:
> > On Tue, 2 Nov 2021 at 22:17, Emil Renner Berthing <kernel@esmil.dk> wrote:
>
> ...
>
> > I'd really like to understand your reasoning here. As far as I can
> > tell reading 2 adjacent 32bit registers with a 64bit read as you're
> > proposing is exactly what would cause endian issues. Eg. on little
> > endian you'd get reg0 | reg1 << 32 whereas on big-endian you'd get
> > reg0 << 32 | reg1.
>
> Nope, it won't. The endianess is a property of both CPU and device.
>
> The I/O accessors, such as readl()/writel() and iowrtieXX()/ioreadXX()
> are _always_ LE.

Aha! Thanks, that's the bit I was missing.
Yury Norov Nov. 10, 2021, 4:34 p.m. UTC | #9
On Tue, Nov 2, 2021 at 1:55 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> On Tue, Nov 2, 2021 at 12:59 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> >
> > On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > +Cc: Yury (bitmap expert)
> > >
> > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> > > >
> > > > Add a driver for the StarFive JH7100 reset controller.
> > >
> > > ...
> > >
> > > > +#define BIT_MASK32(x) BIT((x) % 32)
> > >
> > > Possible namespace collision.
> > >
> > > ...
> > >
> > > > +/*
> > > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > > > + * same line.
>
> We don't have 32-bit bitmaps. Bitmaps are always arrays of unsigned longs. On a
> 64-bit system this '32-bit bitmap' may be broken due to endianness issues.
>
> > > > + * most reset lines have their status inverted so a 0 in the STATUS register
> > > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > > > + * though, so store the expected value of the status registers when all lines
> > > > + * are asserted.
> > > > + */
> > >
> > > Besides missing capitalization,
> >
> > I'm confused. it was you who wanted all comments to capitalized the same..
> > 64bi
> > if it sounds like bitmap, use bitmap.
> > > I have checked DT definitions and it seems you don't even need the
> > > BIT_MASK() macro,
> > >
> > > > +static const u32 jh7100_reset_asserted[4] = {
> > > > +       /* STATUS0 register */
> > > > +       BIT_MASK32(JH7100_RST_U74) |
>
> I think we have no BIT_MASK32() for a good reason. Natural alignment is
> always preferable.
>
> > > > +       BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > > > +       BIT_MASK32(JH7100_RST_VP6_BRESET),
> > > > +       /* STATUS1 register */
> > > > +       BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > > > +       BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > > > +       /* STATUS2 register */
> > > > +       BIT_MASK32(JH7100_RST_E24),
> > > > +       /* STATUS3 register */
> > > > +       0,
> > > > +};
> > >
> > > Yury, do we have any clever (clean) way to initialize a bitmap with
> > > particular bits so that it will be a constant from the beginning? If
> > > no, any suggestion what we can provide to such users?
>
> If you want your array to be a true bitmap, ie, all bitmap functions should
> work with it correctly, you'd initialize it like this:
>
> static const unsigned long jh7100_reset_asserted[] = {
>         BITMAP_FROM_U64(BIT_MASK(JH7100_RST_VP6_DRESET) |
>                           BIT_MASK(JH7100_RST_VP6_BRESET) |
>                           BIT_MASK(JH7100_RST_HIFI4_DRESET) |
>                           BIT_MASK(JH7100_RST_HIFI4_BRESET)),
>         BITMAP_FROM_U64(BIT_MASK(JH7100_RST_E24)),
> }

My bad, it should be BIT_ULL_MASK.

Thanks,
Yury
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index ed49827dfb29..8274fa4b8430 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17866,6 +17866,13 @@  F:	Documentation/devicetree/bindings/clock/starfive,jh7100-clkgen.yaml
 F:	drivers/clk/starfive/clk-starfive-jh7100.c
 F:	include/dt-bindings/clock/starfive-jh7100.h
 
+STARFIVE JH7100 RESET CONTROLLER DRIVER
+M:	Emil Renner Berthing <kernel@esmil.dk>
+S:	Maintained
+F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
+F:	drivers/reset/reset-starfive-jh7100.c
+F:	include/dt-bindings/reset/starfive-jh7100.h
+
 STATIC BRANCH/CALL
 M:	Peter Zijlstra <peterz@infradead.org>
 M:	Josh Poimboeuf <jpoimboe@redhat.com>
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index b0056ae5d463..346e66ae690b 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -224,6 +224,13 @@  config RESET_SOCFPGA
 	  This enables the reset driver for the SoCFPGA ARMv7 platforms. This
 	  driver gets initialized early during platform init calls.
 
+config RESET_STARFIVE_JH7100
+	bool "StarFive JH7100 Reset Driver"
+	depends on SOC_STARFIVE || COMPILE_TEST
+	default SOC_STARFIVE
+	help
+	  This enables the reset controller driver for the StarFive JH7100 SoC.
+
 config RESET_SUNXI
 	bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
 	default ARCH_SUNXI
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 21d46d8869ff..bd0a97be18b5 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -29,6 +29,7 @@  obj-$(CONFIG_RESET_RZG2L_USBPHY_CTRL) += reset-rzg2l-usbphy-ctrl.o
 obj-$(CONFIG_RESET_SCMI) += reset-scmi.o
 obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
 obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
+obj-$(CONFIG_RESET_STARFIVE_JH7100) += reset-starfive-jh7100.o
 obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
 obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
diff --git a/drivers/reset/reset-starfive-jh7100.c b/drivers/reset/reset-starfive-jh7100.c
new file mode 100644
index 000000000000..a3cbae933ec0
--- /dev/null
+++ b/drivers/reset/reset-starfive-jh7100.c
@@ -0,0 +1,178 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Reset driver for the StarFive JH7100 SoC
+ *
+ * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk>
+ */
+
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/spinlock.h>
+
+#include <dt-bindings/reset/starfive-jh7100.h>
+
+#define BIT_MASK32(x) BIT((x) % 32)
+
+/* register offsets */
+#define JH7100_RESET_ASSERT0	0x00
+#define JH7100_RESET_ASSERT1	0x04
+#define JH7100_RESET_ASSERT2	0x08
+#define JH7100_RESET_ASSERT3	0x0c
+#define JH7100_RESET_STATUS0	0x10
+#define JH7100_RESET_STATUS1	0x14
+#define JH7100_RESET_STATUS2	0x18
+#define JH7100_RESET_STATUS3	0x1c
+
+/*
+ * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
+ * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
+ * same line.
+ * most reset lines have their status inverted so a 0 in the STATUS register
+ * means the line is asserted and a 1 means it's deasserted. a few lines don't
+ * though, so store the expected value of the status registers when all lines
+ * are asserted.
+ */
+static const u32 jh7100_reset_asserted[4] = {
+	/* STATUS0 register */
+	BIT_MASK32(JH7100_RST_U74) |
+	BIT_MASK32(JH7100_RST_VP6_DRESET) |
+	BIT_MASK32(JH7100_RST_VP6_BRESET),
+	/* STATUS1 register */
+	BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
+	BIT_MASK32(JH7100_RST_HIFI4_BRESET),
+	/* STATUS2 register */
+	BIT_MASK32(JH7100_RST_E24),
+	/* STATUS3 register */
+	0,
+};
+
+struct jh7100_reset {
+	struct reset_controller_dev rcdev;
+	/* protect registers against concurrent read-modify-write */
+	spinlock_t lock;
+	void __iomem *base;
+};
+
+static inline struct jh7100_reset *
+jh7100_reset_from(struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct jh7100_reset, rcdev);
+}
+
+static int jh7100_reset_update(struct reset_controller_dev *rcdev,
+			       unsigned long id, bool assert)
+{
+	struct jh7100_reset *data = jh7100_reset_from(rcdev);
+	unsigned long offset = id / 32;
+	void __iomem *reg_assert = data->base + JH7100_RESET_ASSERT0 + 4 * offset;
+	void __iomem *reg_status = data->base + JH7100_RESET_STATUS0 + 4 * offset;
+	u32 mask = BIT_MASK32(id);
+	u32 done = jh7100_reset_asserted[offset] & mask;
+	unsigned long flags;
+	u32 value;
+	int ret;
+
+	if (!assert)
+		done ^= mask;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	value = readl(reg_assert);
+	if (assert)
+		value |= mask;
+	else
+		value &= ~mask;
+	writel(value, reg_assert);
+
+	/* if the associated clock is gated, deasserting might otherwise hang forever */
+	ret = readl_poll_timeout_atomic(reg_status, value, (value & mask) == done, 0, 1000);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+	return ret;
+}
+
+static int jh7100_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	dev_dbg(rcdev->dev, "assert(%lu)\n", id);
+	return jh7100_reset_update(rcdev, id, true);
+}
+
+static int jh7100_reset_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long id)
+{
+	dev_dbg(rcdev->dev, "deassert(%lu)\n", id);
+	return jh7100_reset_update(rcdev, id, false);
+}
+
+static int jh7100_reset_reset(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	int ret;
+
+	dev_dbg(rcdev->dev, "reset(%lu)\n", id);
+	ret = jh7100_reset_assert(rcdev, id);
+	if (ret)
+		return ret;
+
+	return jh7100_reset_deassert(rcdev, id);
+}
+
+static int jh7100_reset_status(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct jh7100_reset *data = jh7100_reset_from(rcdev);
+	unsigned long offset = id / 32;
+	void __iomem *reg_status = data->base + JH7100_RESET_STATUS0 + 4 * offset;
+	u32 mask = BIT_MASK32(id);
+	int ret = !((readl(reg_status) ^ jh7100_reset_asserted[offset]) & mask);
+
+	dev_dbg(rcdev->dev, "status(%lu) = %d\n", id, ret);
+	return ret;
+}
+
+static const struct reset_control_ops jh7100_reset_ops = {
+	.assert		= jh7100_reset_assert,
+	.deassert	= jh7100_reset_deassert,
+	.reset		= jh7100_reset_reset,
+	.status		= jh7100_reset_status,
+};
+
+static int __init jh7100_reset_probe(struct platform_device *pdev)
+{
+	struct jh7100_reset *data;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(data->base))
+		return PTR_ERR(data->base);
+
+	data->rcdev.ops = &jh7100_reset_ops;
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.nr_resets = JH7100_RSTN_END;
+	data->rcdev.dev = &pdev->dev;
+	data->rcdev.of_node = pdev->dev.of_node;
+	spin_lock_init(&data->lock);
+
+	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
+}
+
+static const struct of_device_id jh7100_reset_dt_ids[] = {
+	{ .compatible = "starfive,jh7100-reset" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver jh7100_reset_driver = {
+	.driver = {
+		.name = "jh7100-reset",
+		.of_match_table = jh7100_reset_dt_ids,
+		.suppress_bind_attrs = true,
+	},
+};
+builtin_platform_driver_probe(jh7100_reset_driver, jh7100_reset_probe);