| Message ID | 20211021174223.43310-10-kernel@esmil.dk (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Basic StarFive JH7100 RISC-V SoC support | expand |
On Thu, Oct 21, 2021 at 8:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > Add a driver for the StarFive JH7100 reset controller. ... > +config RESET_STARFIVE_JH7100 > + bool "StarFive JH7100 Reset Driver" > + depends on SOC_STARFIVE || COMPILE_TEST > + depends on OF No evidence of this dependency. Why to limit test coverage? > + default SOC_STARFIVE ... > +/* > + * Reset driver for the StarFive JH7100 SoC > + * > + * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk> > + * Redundant empty line. > + */ ... > +#include <linux/of_device.h> No evidence of any usage of this header. Perhaps you meant mod_devicetable.h? ... > +static const u32 jh7100_reset_asserted[4] = { > + BIT(JH7100_RST_U74 % 32) | > + BIT(JH7100_RST_VP6_DRESET % 32) | > + BIT(JH7100_RST_VP6_BRESET % 32), It's hard to notice that this is only one entry. See also below. > + BIT(JH7100_RST_HIFI4_DRESET % 32) | > + BIT(JH7100_RST_HIFI4_BRESET % 32), > + > + BIT(JH7100_RST_E24 % 32) + Comma. > +}; Why all these ugly % 32 against constants? ... > + if (!assert) > + done ^= mask; Can you convert this to simple if (assert) ret = readl_... else ret = readl_... below? > + 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(reg_status, value, (value & mask) == done, 0, 1000); You run delays under spin lock. You need to use _atomic variant. > + spin_unlock_irqrestore(&data->lock, flags); ... > + u32 value = (readl(reg_status) ^ jh7100_reset_asserted[offset]) & mask; > + dev_dbg(rcdev->dev, "status(%lu) = %d\n", id, !value); > + return !value; Dup of ! operator. Can it be value = !(...); above?
On Okt 21 2021, Emil Renner Berthing wrote: > +config RESET_STARFIVE_JH7100 > + bool "StarFive JH7100 Reset Driver" > + depends on SOC_STARFIVE || COMPILE_TEST Why does it need to depend on SOC_STARFIVE? Andreas.
On Fri, 22 Oct 2021 at 14:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Oct 21, 2021 at 8:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > +static const u32 jh7100_reset_asserted[4] = { > > > + BIT(JH7100_RST_U74 % 32) | > > + BIT(JH7100_RST_VP6_DRESET % 32) | > > + BIT(JH7100_RST_VP6_BRESET % 32), > > It's hard to notice that this is only one entry. See also below. Yeah, so what would be a better way to style it? > > + BIT(JH7100_RST_HIFI4_DRESET % 32) | > > + BIT(JH7100_RST_HIFI4_BRESET % 32), > > + > > + BIT(JH7100_RST_E24 % 32) > > + Comma. > > > +}; > > Why all these ugly % 32 against constants? Because the JH7100_RST_ values goes higher than 31. There is a BIT_MASK macro, but that does % BITS_PER_LONG and this is a 64bit machine. > ... > > > + if (!assert) > > + done ^= mask; > > Can you convert this to simple > > if (assert) > ret = readl_... > else > ret = readl_... > > below? I don't see how that would work. We're using the done value in in the readl_poll_timeout. Maybe you can be a bit more explicit. The reason is that for most reset lines a 0 in the status register means it's asserted and a 1 means it's deasserted. For the few reset lines above this is reversed though. /Emil
On Fri, Oct 22, 2021 at 4:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > On Fri, 22 Oct 2021 at 14:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Oct 21, 2021 at 8:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: ... > > Why all these ugly % 32 against constants? > > Because the JH7100_RST_ values goes higher than 31. There is a > BIT_MASK macro, but that does % BITS_PER_LONG and this is a 64bit > machine. And? It's exactly what you have to use! ... > > Can you convert this to simple > > > > if (assert) > > ret = readl_... > > else > > ret = readl_... > > > > below? > > I don't see how that would work. We're using the done value in in the > readl_poll_timeout. Maybe you can be a bit more explicit. Supply done either == mask or == ^mask. Try it.
On Fri, 22 Oct 2021 at 15:06, Andreas Schwab <schwab@linux-m68k.org> wrote: > On Okt 21 2021, Emil Renner Berthing wrote: > > > +config RESET_STARFIVE_JH7100 > > + bool "StarFive JH7100 Reset Driver" > > + depends on SOC_STARFIVE || COMPILE_TEST > > Why does it need to depend on SOC_STARFIVE? It strictly doesn't but most other drivers in the same Kconfig file also depends on ARCH_something or SOC_something. In particular RESET_K210 depends on SOC_CANAAN. I think this is to prevent overwhelming the user with useless choices if they didn't enable the relevant SoC, and for others there are the COMPILE_TEST option. /Emil
On Fri, 22 Oct 2021 at 15:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Oct 22, 2021 at 4:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > On Fri, 22 Oct 2021 at 14:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Thu, Oct 21, 2021 at 8:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > ... > > > > Why all these ugly % 32 against constants? > > > > Because the JH7100_RST_ values goes higher than 31. There is a > > BIT_MASK macro, but that does % BITS_PER_LONG and this is a 64bit > > machine. > > And? It's exactly what you have to use! > > > Can you convert this to simple > > > > > > if (assert) > > > ret = readl_... > > > else > > > ret = readl_... > > > > > > below? > > > > I don't see how that would work. We're using the done value in in the > > readl_poll_timeout. Maybe you can be a bit more explicit. > > Supply done either == mask or == ^mask. Try it. So you want this? if (assert) ret = readl_poll_timeout_atomic(reg_status, value, (value & mask) == done, 0, 1000); else ret = readl_poll_timeout_atomic(reg_status, value, (value & mask) == ^done, 0, 1000); The compiler might be clever enough, but I'd worry the long body of the readl_poll_timeout_atomic macro is inline twice. Rather than just flipping the bit in `done`.
On Fri, Oct 22, 2021 at 4:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > On Fri, 22 Oct 2021 at 15:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Oct 22, 2021 at 4:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > On Fri, 22 Oct 2021 at 14:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Thu, Oct 21, 2021 at 8:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: ... > > > > Can you convert this to simple > > > > > > > > if (assert) > > > > ret = readl_... > > > > else > > > > ret = readl_... > > > > > > > > below? > > > > > > I don't see how that would work. We're using the done value in in the > > > readl_poll_timeout. Maybe you can be a bit more explicit. > > > > Supply done either == mask or == ^mask. Try it. > > So you want this? > if (assert) > ret = readl_poll_timeout_atomic(reg_status, value, (value & mask) == > done, 0, 1000); > else > ret = readl_poll_timeout_atomic(reg_status, value, (value & mask) == > ^done, 0, 1000); > > The compiler might be clever enough, but I'd worry the long body of > the readl_poll_timeout_atomic macro is inline twice. Rather than just > flipping the bit in `done`. You have a point, although it would be nice to have confirmation of either.
On Fri, 22 Oct 2021 at 15:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Oct 22, 2021 at 4:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > On Fri, 22 Oct 2021 at 14:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Thu, Oct 21, 2021 at 8:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > ... > > > > Why all these ugly % 32 against constants? > > > > Because the JH7100_RST_ values goes higher than 31. There is a > > BIT_MASK macro, but that does % BITS_PER_LONG and this is a 64bit > > machine. > > And? It's exactly what you have to use! So you want me to use an unsigned long array or DECLARE_BITMAP and juggle two different index and bit offsets? Also is there a macro for handling that we'd then need 4 commas on 32bit COMPILE_TEST and 2 commas on 64bit? If you have some other way in mind you'll have to be a lot more explicit again. The point of the jh7100_reset_asserted array is that it exactly mirrors the values of the status registers when the lines are asserted. Maybe writing it like this would be more explicit: static const u32 jh7100_reset_asserted[4] = { /* STATUS0 register */ BIT(JH7100_RST_U74 % 32) | BIT(JH7100_RST_VP6_DRESET % 32) | BIT(JH7100_RST_VP6_BRESET % 32), /* STATUS1 register */ BIT(JH7100_RST_HIFI4_DRESET % 32) | BIT(JH7100_RST_HIFI4_BRESET % 32), /* STATUS2 register */ BIT(JH7100_RST_E24 % 32), /* STATUS3 register */ 0, };
On Fri, Oct 22, 2021 at 5:25 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > On Fri, 22 Oct 2021 at 15:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Oct 22, 2021 at 4:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > On Fri, 22 Oct 2021 at 14:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Thu, Oct 21, 2021 at 8:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: ... > > > > Why all these ugly % 32 against constants? > > > > > > Because the JH7100_RST_ values goes higher than 31. There is a > > > BIT_MASK macro, but that does % BITS_PER_LONG and this is a 64bit > > > machine. > > > > And? It's exactly what you have to use! > > So you want me to use an unsigned long array or DECLARE_BITMAP and > juggle two different index and bit offsets? What are the offsets of those status registers? AFAICS they are sequential 4 32-bit registers. So bitmap is exactly what is suitable here, you are right! See gpio-xilinx and gpio-pca953x on how to use bitmaps in the GPIO drivers. > Also is there a macro for handling that we'd then need 4 commas on > 32bit COMPILE_TEST and 2 commas on 64bit? > If you have some other way in mind you'll have to be a lot more explicit again. > > The point of the jh7100_reset_asserted array is that it exactly > mirrors the values of the status registers when the lines are > asserted. Maybe writing it like this would be more explicit: > > static const u32 jh7100_reset_asserted[4] = { > /* STATUS0 register */ > BIT(JH7100_RST_U74 % 32) | > BIT(JH7100_RST_VP6_DRESET % 32) | > BIT(JH7100_RST_VP6_BRESET % 32), > /* STATUS1 register */ > BIT(JH7100_RST_HIFI4_DRESET % 32) | > BIT(JH7100_RST_HIFI4_BRESET % 32), > /* STATUS2 register */ > BIT(JH7100_RST_E24 % 32), > /* STATUS3 register */ > 0, > }; -- With Best Regards, Andy Shevchenko
On Fri, Oct 22, 2021 at 5:49 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Oct 22, 2021 at 5:25 PM Emil Renner Berthing <kernel@esmil.dk> wrote: ... > See gpio-xilinx and gpio-pca953x on how to use bitmaps in the GPIO drivers. It might be confusing, what I meant is take that as an example on how the bitmaps are used in the GPIO drivers.
On Fri, 22 Oct 2021 at 16:50, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Oct 22, 2021 at 5:25 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > On Fri, 22 Oct 2021 at 15:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Fri, Oct 22, 2021 at 4:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > > On Fri, 22 Oct 2021 at 14:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Thu, Oct 21, 2021 at 8:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > ... > > > > > > Why all these ugly % 32 against constants? > > > > > > > > Because the JH7100_RST_ values goes higher than 31. There is a > > > > BIT_MASK macro, but that does % BITS_PER_LONG and this is a 64bit > > > > machine. > > > > > > And? It's exactly what you have to use! > > > > So you want me to use an unsigned long array or DECLARE_BITMAP and > > juggle two different index and bit offsets? > > What are the offsets of those status registers? > AFAICS they are sequential 4 32-bit registers. That's right, but we're on a 64bit machine, so DECLARE_BITMAP will give us an unsigned long array that doesn't match that. > So bitmap is exactly what is suitable here, you are right! > See gpio-xilinx and gpio-pca953x on how to use bitmaps in the GPIO drivers. None of them has a pre-initialized const DECLARE_BITMAP, so they don't have to deal with the 4 vs. 2 commas problem. > > Also is there a macro for handling that we'd then need 4 commas on > > 32bit COMPILE_TEST and 2 commas on 64bit? > > If you have some other way in mind you'll have to be a lot more explicit again. > > > > The point of the jh7100_reset_asserted array is that it exactly > > mirrors the values of the status registers when the lines are > > asserted. Maybe writing it like this would be more explicit: > > > > static const u32 jh7100_reset_asserted[4] = { > > /* STATUS0 register */ > > BIT(JH7100_RST_U74 % 32) | > > BIT(JH7100_RST_VP6_DRESET % 32) | > > BIT(JH7100_RST_VP6_BRESET % 32), > > /* STATUS1 register */ > > BIT(JH7100_RST_HIFI4_DRESET % 32) | > > BIT(JH7100_RST_HIFI4_BRESET % 32), > > /* STATUS2 register */ > > BIT(JH7100_RST_E24 % 32), > > /* STATUS3 register */ > > 0, > > }; > > -- > With Best Regards, > Andy Shevchenko
On Fri, Oct 22, 2021 at 5:56 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > On Fri, 22 Oct 2021 at 16:50, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Oct 22, 2021 at 5:25 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > On Fri, 22 Oct 2021 at 15:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Oct 22, 2021 at 4:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > > > On Fri, 22 Oct 2021 at 14:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > On Thu, Oct 21, 2021 at 8:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: ... > > > > > > Why all these ugly % 32 against constants? > > > > > > > > > > Because the JH7100_RST_ values goes higher than 31. There is a > > > > > BIT_MASK macro, but that does % BITS_PER_LONG and this is a 64bit > > > > > machine. > > > > > > > > And? It's exactly what you have to use! > > > > > > So you want me to use an unsigned long array or DECLARE_BITMAP and > > > juggle two different index and bit offsets? > > > > What are the offsets of those status registers? > > AFAICS they are sequential 4 32-bit registers. > > That's right, but we're on a 64bit machine, so DECLARE_BITMAP will > give us an unsigned long array that doesn't match that. I didn't get it, sorry. You will have a bitmap array which you will split to 32-bit values. What you will probably need is to move xgpio_get_value32() and void xgpio_set_value32() to the one of bitmap related headers (look for bitmap_get_value8() and friends). > > So bitmap is exactly what is suitable here, you are right! > > See gpio-xilinx and gpio-pca953x on how to use bitmaps in the GPIO drivers. > > None of them has a pre-initialized const DECLARE_BITMAP, so they don't > have to deal with the 4 vs. 2 commas problem. I believe it's well possible to refactor this to look much better with bitmaps (as it represents the hardware very well). > > > Also is there a macro for handling that we'd then need 4 commas on > > > 32bit COMPILE_TEST and 2 commas on 64bit? > > > If you have some other way in mind you'll have to be a lot more explicit again. > > > > > > The point of the jh7100_reset_asserted array is that it exactly > > > mirrors the values of the status registers when the lines are > > > asserted. Maybe writing it like this would be more explicit: > > > > > > static const u32 jh7100_reset_asserted[4] = { > > > /* STATUS0 register */ > > > BIT(JH7100_RST_U74 % 32) | > > > BIT(JH7100_RST_VP6_DRESET % 32) | > > > BIT(JH7100_RST_VP6_BRESET % 32), > > > /* STATUS1 register */ > > > BIT(JH7100_RST_HIFI4_DRESET % 32) | > > > BIT(JH7100_RST_HIFI4_BRESET % 32), > > > /* STATUS2 register */ > > > BIT(JH7100_RST_E24 % 32), > > > /* STATUS3 register */ > > > 0, > > > };
On Fri, 22 Oct 2021 at 17:25, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Oct 22, 2021 at 5:56 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > On Fri, 22 Oct 2021 at 16:50, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Fri, Oct 22, 2021 at 5:25 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > > On Fri, 22 Oct 2021 at 15:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Fri, Oct 22, 2021 at 4:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > > > > On Fri, 22 Oct 2021 at 14:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > On Thu, Oct 21, 2021 at 8:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > ... > > > > > > > > Why all these ugly % 32 against constants? > > > > > > > > > > > > Because the JH7100_RST_ values goes higher than 31. There is a > > > > > > BIT_MASK macro, but that does % BITS_PER_LONG and this is a 64bit > > > > > > machine. > > > > > > > > > > And? It's exactly what you have to use! > > > > > > > > So you want me to use an unsigned long array or DECLARE_BITMAP and > > > > juggle two different index and bit offsets? > > > > > > What are the offsets of those status registers? > > > AFAICS they are sequential 4 32-bit registers. > > > > That's right, but we're on a 64bit machine, so DECLARE_BITMAP will > > give us an unsigned long array that doesn't match that. > > I didn't get it, sorry. > You will have a bitmap array which you will split to 32-bit values. > What you will probably need is to move xgpio_get_value32() and void > xgpio_set_value32() to the one of bitmap related headers (look for > bitmap_get_value8() and friends). > > > > So bitmap is exactly what is suitable here, you are right! > > > See gpio-xilinx and gpio-pca953x on how to use bitmaps in the GPIO drivers. > > > > None of them has a pre-initialized const DECLARE_BITMAP, so they don't > > have to deal with the 4 vs. 2 commas problem. > > I believe it's well possible to refactor this to look much better with > bitmaps (as it represents the hardware very well). Right, but how exactly? This works on on 64bit, but not with 32bit COMPILE_TEST: static const DECLARE_BITMAP(jh7100_reset_asserted, JH7100_RSTN_END) = { /* STATUS0 register */ BIT_MASK(JH7100_RST_U74) | BIT_MASK(JH7100_RST_VP6_DRESET) | BIT_MASK(JH7100_RST_VP6_BRESET) | /* STATUS1 register */ BIT_MASK(JH7100_RST_HIFI4_DRESET) | BIT_MASK(JH7100_RST_HIFI4_BRESET), /* STATUS2 register */ BIT_MASK(JH7100_RST_E24) | /* STATUS3 register */ 0, }; > > > > Also is there a macro for handling that we'd then need 4 commas on > > > > 32bit COMPILE_TEST and 2 commas on 64bit? > > > > If you have some other way in mind you'll have to be a lot more explicit again. > > > > > > > > The point of the jh7100_reset_asserted array is that it exactly > > > > mirrors the values of the status registers when the lines are > > > > asserted. Maybe writing it like this would be more explicit: > > > > > > > > static const u32 jh7100_reset_asserted[4] = { > > > > /* STATUS0 register */ > > > > BIT(JH7100_RST_U74 % 32) | > > > > BIT(JH7100_RST_VP6_DRESET % 32) | > > > > BIT(JH7100_RST_VP6_BRESET % 32), > > > > /* STATUS1 register */ > > > > BIT(JH7100_RST_HIFI4_DRESET % 32) | > > > > BIT(JH7100_RST_HIFI4_BRESET % 32), > > > > /* STATUS2 register */ > > > > BIT(JH7100_RST_E24 % 32), > > > > /* STATUS3 register */ > > > > 0, > > > > }; > > -- > With Best Regards, > Andy Shevchenko
On Fri, Oct 22, 2021 at 05:36:21PM +0200, Emil Renner Berthing wrote: > On Fri, 22 Oct 2021 at 17:25, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Oct 22, 2021 at 5:56 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > On Fri, 22 Oct 2021 at 16:50, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Oct 22, 2021 at 5:25 PM Emil Renner Berthing <kernel@esmil.dk> wrote: ... > > > > AFAICS they are sequential 4 32-bit registers. > > > > > > That's right, but we're on a 64bit machine, so DECLARE_BITMAP will > > > give us an unsigned long array that doesn't match that. > > > > I didn't get it, sorry. > > You will have a bitmap array which you will split to 32-bit values. > > What you will probably need is to move xgpio_get_value32() and void > > xgpio_set_value32() to the one of bitmap related headers (look for > > bitmap_get_value8() and friends). > > > > > > So bitmap is exactly what is suitable here, you are right! > > > > See gpio-xilinx and gpio-pca953x on how to use bitmaps in the GPIO drivers. > > > > > > None of them has a pre-initialized const DECLARE_BITMAP, so they don't > > > have to deal with the 4 vs. 2 commas problem. > > > > I believe it's well possible to refactor this to look much better with > > bitmaps (as it represents the hardware very well). > > Right, but how exactly? This works on on 64bit, but not with 32bit COMPILE_TEST: > > static const DECLARE_BITMAP(jh7100_reset_asserted, JH7100_RSTN_END) = { > /* STATUS0 register */ > BIT_MASK(JH7100_RST_U74) | > BIT_MASK(JH7100_RST_VP6_DRESET) | > BIT_MASK(JH7100_RST_VP6_BRESET) | > /* STATUS1 register */ > BIT_MASK(JH7100_RST_HIFI4_DRESET) | > BIT_MASK(JH7100_RST_HIFI4_BRESET), > /* STATUS2 register */ > BIT_MASK(JH7100_RST_E24) | > /* STATUS3 register */ > 0, > }; BITMAP_FROM_U64() ? > > > > > Also is there a macro for handling that we'd then need 4 commas on > > > > > 32bit COMPILE_TEST and 2 commas on 64bit? > > > > > If you have some other way in mind you'll have to be a lot more explicit again. > > > > > > > > > > The point of the jh7100_reset_asserted array is that it exactly > > > > > mirrors the values of the status registers when the lines are > > > > > asserted.
On Fri, 22 Oct 2021 at 17:55, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Oct 22, 2021 at 05:36:21PM +0200, Emil Renner Berthing wrote: > > On Fri, 22 Oct 2021 at 17:25, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Fri, Oct 22, 2021 at 5:56 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > > On Fri, 22 Oct 2021 at 16:50, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Fri, Oct 22, 2021 at 5:25 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > ... > > > > > > AFAICS they are sequential 4 32-bit registers. > > > > > > > > That's right, but we're on a 64bit machine, so DECLARE_BITMAP will > > > > give us an unsigned long array that doesn't match that. > > > > > > I didn't get it, sorry. > > > You will have a bitmap array which you will split to 32-bit values. > > > What you will probably need is to move xgpio_get_value32() and void > > > xgpio_set_value32() to the one of bitmap related headers (look for > > > bitmap_get_value8() and friends). > > > > > > > > So bitmap is exactly what is suitable here, you are right! > > > > > See gpio-xilinx and gpio-pca953x on how to use bitmaps in the GPIO drivers. > > > > > > > > None of them has a pre-initialized const DECLARE_BITMAP, so they don't > > > > have to deal with the 4 vs. 2 commas problem. > > > > > > I believe it's well possible to refactor this to look much better with > > > bitmaps (as it represents the hardware very well). > > > > Right, but how exactly? This works on on 64bit, but not with 32bit COMPILE_TEST: > > > > static const DECLARE_BITMAP(jh7100_reset_asserted, JH7100_RSTN_END) = { > > /* STATUS0 register */ > > BIT_MASK(JH7100_RST_U74) | > > BIT_MASK(JH7100_RST_VP6_DRESET) | > > BIT_MASK(JH7100_RST_VP6_BRESET) | > > /* STATUS1 register */ > > BIT_MASK(JH7100_RST_HIFI4_DRESET) | > > BIT_MASK(JH7100_RST_HIFI4_BRESET), > > /* STATUS2 register */ > > BIT_MASK(JH7100_RST_E24) | > > /* STATUS3 register */ > > 0, > > }; > > BITMAP_FROM_U64() ? So you think this is better? static const DECLARE_BITMAP(jh7100_reset_asserted, JH7100_RSTN_END) = { BITMAP_FROM_U64( /* STATUS0 register */ BIT_MASK(JH7100_RST_U74) | BIT_MASK(JH7100_RST_VP6_DRESET) | BIT_MASK(JH7100_RST_VP6_BRESET) | /* STATUS1 register */ BIT_MASK(JH7100_RST_HIFI4_DRESET) | BIT_MASK(JH7100_RST_HIFI4_BRESET) ), BITMAP_FROM_U64( /* STATUS2 register */ BIT_MASK(JH7100_RST_E24) | /* STATUS3 register */ 0 ), }; > > > > > > Also is there a macro for handling that we'd then need 4 commas on > > > > > > 32bit COMPILE_TEST and 2 commas on 64bit? > > > > > > If you have some other way in mind you'll have to be a lot more explicit again. > > > > > > > > > > > > The point of the jh7100_reset_asserted array is that it exactly > > > > > > mirrors the values of the status registers when the lines are > > > > > > asserted. > > -- > With Best Regards, > Andy Shevchenko > > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
diff --git a/MAINTAINERS b/MAINTAINERS index e5a19b70dfbb..b3f3a29fc91f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17855,6 +17855,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 be799a5abf8a..a41fac304904 100644 --- a/drivers/reset/Kconfig +++ b/drivers/reset/Kconfig @@ -224,6 +224,14 @@ 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 + depends on OF + 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..ae15ed5357f1 --- /dev/null +++ b/drivers/reset/reset-starfive-jh7100.c @@ -0,0 +1,165 @@ +// 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/of_device.h> +#include <linux/platform_device.h> +#include <linux/reset-controller.h> +#include <linux/spinlock.h> + +#include <dt-bindings/reset/starfive-jh7100.h> + +/* 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 + +struct jh7100_reset { + struct reset_controller_dev rcdev; + /* protect registers against overlapping 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 const u32 jh7100_reset_asserted[4] = { + BIT(JH7100_RST_U74 % 32) | + BIT(JH7100_RST_VP6_DRESET % 32) | + BIT(JH7100_RST_VP6_BRESET % 32), + + BIT(JH7100_RST_HIFI4_DRESET % 32) | + BIT(JH7100_RST_HIFI4_BRESET % 32), + + BIT(JH7100_RST_E24 % 32) +}; + +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(id % 32); + 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(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(id % 32); + u32 value = (readl(reg_status) ^ jh7100_reset_asserted[offset]) & mask; + + dev_dbg(rcdev->dev, "status(%lu) = %d\n", id, !value); + return !value; +} + +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 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 = { + .probe = jh7100_reset_probe, + .driver = { + .name = "jh7100-reset", + .of_match_table = jh7100_reset_dt_ids, + }, +}; +builtin_platform_driver(jh7100_reset_driver);
Add a driver for the StarFive JH7100 reset controller. Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> --- MAINTAINERS | 7 ++ drivers/reset/Kconfig | 8 ++ drivers/reset/Makefile | 1 + drivers/reset/reset-starfive-jh7100.c | 165 ++++++++++++++++++++++++++ 4 files changed, 181 insertions(+) create mode 100644 drivers/reset/reset-starfive-jh7100.c