| 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 |
+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,
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
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).
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
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?
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
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...).
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.
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 --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);
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