[PATCH v3 1/3] soc: rockchip: Clean up Kconfig whitespace

[PATCH v3 1/3] soc: rockchip: Clean up Kconfig whitespace

[Brian Norris]

There are spaces instead of tabs, and other inconsistent indentation.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

(no changes since v2)

Changes in v2:
 * New patch

 drivers/soc/rockchip/Kconfig | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
index 156ac0e0c8fe..aff2f7e95237 100644
--- a/drivers/soc/rockchip/Kconfig
+++ b/drivers/soc/rockchip/Kconfig
@@ -23,23 +23,23 @@ config ROCKCHIP_IODOMAIN
 	  voltage supplied by the regulators.
 
 config ROCKCHIP_PM_DOMAINS
-        bool "Rockchip generic power domain"
-        depends on PM
-        select PM_GENERIC_DOMAINS
-        help
-          Say y here to enable power domain support.
-          In order to meet high performance and low power requirements, a power
-          management unit is designed or saving power when RK3288 in low power
-          mode. The RK3288 PMU is dedicated for managing the power of the whole chip.
+	bool "Rockchip generic power domain"
+	depends on PM
+	select PM_GENERIC_DOMAINS
+	help
+	  Say y here to enable power domain support.
+	  In order to meet high performance and low power requirements, a power
+	  management unit is designed or saving power when RK3288 in low power
+	  mode. The RK3288 PMU is dedicated for managing the power of the whole chip.
 
-          If unsure, say N.
+	  If unsure, say N.
 
 config ROCKCHIP_DTPM
 	tristate "Rockchip DTPM hierarchy"
 	depends on DTPM && m
 	help
-	 Describe the hierarchy for the Dynamic Thermal Power
-	 Management tree on this platform. That will create all the
-	 power capping capable devices.
+	  Describe the hierarchy for the Dynamic Thermal Power Management tree
+	  on this platform. That will create all the power capping capable
+	  devices.
 
 endif
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v3 2/3] soc: rockchip: power-domain: Replace dsb() with smb()

[Brian Norris]

It's unclear if these are really needed at all, but seemingly their
purpose is only as a write barrier. Use the general macro instead of the
ARM-specific one.

This driver is partially marked for COMPILE_TEST'ing, but it doesn't
build under non-ARM architectures. Fix this up before *really* enabling
it for COMPILE_TEST.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v3:
 * New in v3

 drivers/soc/rockchip/pm_domains.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index 1b029e494274..cf16ff9b73b3 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c
@@ -178,7 +178,7 @@ static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
 		regmap_update_bits(pmu->regmap, pmu->info->req_offset,
 				   pd_info->req_mask, idle ? -1U : 0);
 
-	dsb(sy);
+	wmb();
 
 	/* Wait util idle_ack = 1 */
 	target_ack = idle ? pd_info->ack_mask : 0;
@@ -285,7 +285,7 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
 		regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
 				   pd->info->pwr_mask, on ? 0 : -1U);
 
-	dsb(sy);
+	wmb();
 
 	if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
 				      is_on == on, 0, 10000)) {
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH v3 3/3] soc: rockchip: Fix compile-testing SoC drivers

[Brian Norris]

Similar to commit 7f94b69ece51 ("ARM: ixp4xx: fix compile-testing soc
drivers").

drivers/soc/rockchip/Kconfig makes plenty of provision for configuring
drivers either for ARCH_ROCKCHIP or for COMPILE_TEST. But
drivers/soc/Makefile pulls the rug out from under us, by refusing to
build anything if we specified COMPILE_TEST but not ARCH_ROCKCHIP.

Currently, I'm not aware of anything that breaks without this patch, but
it certainly makes for confusing builds (CONFIG_ROCKCHIP_PM_DOMAINS=y,
but we didn't actually compile the driver?) and leaves room for future
error (I have pending patches that break confusingly with COMPILE_TEST=y
even though their Kconfig dependencies seem correct [1]).

Defer to drivers/soc/rockchip/{Makefile,Kconfig} to do the right thing.

[1] e.g.,
https://lore.kernel.org/linux-rockchip/20220405184816.RFC.2.I2d73b403944f0b8b5871a77585b73f31ccc62999@changeid/
[RFC PATCH 2/2] PM / devfreq: rk3399_dmc: Block PMU during transitions

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v3:
 * Drop ARM||ARM64 dependency, now that bare dsb()s have been removed

Changes in v2:
 * Depend on ARM||ARM64

 drivers/soc/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index fd7717d597fc..4c91705a8bb3 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -23,7 +23,7 @@ obj-y				+= pxa/
 obj-y				+= amlogic/
 obj-y				+= qcom/
 obj-y				+= renesas/
-obj-$(CONFIG_ARCH_ROCKCHIP)	+= rockchip/
+obj-y				+= rockchip/
 obj-$(CONFIG_SOC_SAMSUNG)	+= samsung/
 obj-$(CONFIG_SOC_SIFIVE)	+= sifive/
 obj-y				+= sunxi/
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

Re: [PATCH v3 1/3] soc: rockchip: Clean up Kconfig whitespace

[Guenter Roeck]

On Mon, Apr 25, 2022 at 06:45:42PM -0700, Brian Norris wrote:
> There are spaces instead of tabs, and other inconsistent indentation.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> 
> (no changes since v2)
> 
> Changes in v2:
>  * New patch
> 
>  drivers/soc/rockchip/Kconfig | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> index 156ac0e0c8fe..aff2f7e95237 100644
> --- a/drivers/soc/rockchip/Kconfig
> +++ b/drivers/soc/rockchip/Kconfig
> @@ -23,23 +23,23 @@ config ROCKCHIP_IODOMAIN
>  	  voltage supplied by the regulators.
>  
>  config ROCKCHIP_PM_DOMAINS
> -        bool "Rockchip generic power domain"
> -        depends on PM
> -        select PM_GENERIC_DOMAINS
> -        help
> -          Say y here to enable power domain support.
> -          In order to meet high performance and low power requirements, a power
> -          management unit is designed or saving power when RK3288 in low power
> -          mode. The RK3288 PMU is dedicated for managing the power of the whole chip.
> +	bool "Rockchip generic power domain"
> +	depends on PM
> +	select PM_GENERIC_DOMAINS
> +	help
> +	  Say y here to enable power domain support.
> +	  In order to meet high performance and low power requirements, a power
> +	  management unit is designed or saving power when RK3288 in low power
> +	  mode. The RK3288 PMU is dedicated for managing the power of the whole chip.
>  
> -          If unsure, say N.
> +	  If unsure, say N.
>  
>  config ROCKCHIP_DTPM
>  	tristate "Rockchip DTPM hierarchy"
>  	depends on DTPM && m
>  	help
> -	 Describe the hierarchy for the Dynamic Thermal Power
> -	 Management tree on this platform. That will create all the
> -	 power capping capable devices.
> +	  Describe the hierarchy for the Dynamic Thermal Power Management tree
> +	  on this platform. That will create all the power capping capable
> +	  devices.
>  
>  endif
> -- 
> 2.36.0.rc2.479.g8af0fa9b8e-goog
> 

Re: [PATCH v3 2/3] soc: rockchip: power-domain: Replace dsb() with smb()

[Guenter Roeck]

On Mon, Apr 25, 2022 at 06:45:43PM -0700, Brian Norris wrote:
> It's unclear if these are really needed at all, but seemingly their
> purpose is only as a write barrier. Use the general macro instead of the
> ARM-specific one.
> 
> This driver is partially marked for COMPILE_TEST'ing, but it doesn't
> build under non-ARM architectures. Fix this up before *really* enabling
> it for COMPILE_TEST.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> 
> Changes in v3:
>  * New in v3
> 
>  drivers/soc/rockchip/pm_domains.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
> index 1b029e494274..cf16ff9b73b3 100644
> --- a/drivers/soc/rockchip/pm_domains.c
> +++ b/drivers/soc/rockchip/pm_domains.c
> @@ -178,7 +178,7 @@ static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
>  		regmap_update_bits(pmu->regmap, pmu->info->req_offset,
>  				   pd_info->req_mask, idle ? -1U : 0);
>  
> -	dsb(sy);
> +	wmb();
>  
>  	/* Wait util idle_ack = 1 */
>  	target_ack = idle ? pd_info->ack_mask : 0;
> @@ -285,7 +285,7 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>  		regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
>  				   pd->info->pwr_mask, on ? 0 : -1U);
>  
> -	dsb(sy);
> +	wmb();
>  
>  	if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
>  				      is_on == on, 0, 10000)) {
> -- 
> 2.36.0.rc2.479.g8af0fa9b8e-goog
> 

Re: [PATCH v3 3/3] soc: rockchip: Fix compile-testing SoC drivers

[Guenter Roeck]

On Mon, Apr 25, 2022 at 06:45:44PM -0700, Brian Norris wrote:
> Similar to commit 7f94b69ece51 ("ARM: ixp4xx: fix compile-testing soc
> drivers").
> 
> drivers/soc/rockchip/Kconfig makes plenty of provision for configuring
> drivers either for ARCH_ROCKCHIP or for COMPILE_TEST. But
> drivers/soc/Makefile pulls the rug out from under us, by refusing to
> build anything if we specified COMPILE_TEST but not ARCH_ROCKCHIP.
> 
> Currently, I'm not aware of anything that breaks without this patch, but
> it certainly makes for confusing builds (CONFIG_ROCKCHIP_PM_DOMAINS=y,
> but we didn't actually compile the driver?) and leaves room for future
> error (I have pending patches that break confusingly with COMPILE_TEST=y
> even though their Kconfig dependencies seem correct [1]).
> 
> Defer to drivers/soc/rockchip/{Makefile,Kconfig} to do the right thing.
> 
> [1] e.g.,
> https://lore.kernel.org/linux-rockchip/20220405184816.RFC.2.I2d73b403944f0b8b5871a77585b73f31ccc62999@changeid/
> [RFC PATCH 2/2] PM / devfreq: rk3399_dmc: Block PMU during transitions
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> 
> Changes in v3:
>  * Drop ARM||ARM64 dependency, now that bare dsb()s have been removed
> 
> Changes in v2:
>  * Depend on ARM||ARM64
> 
>  drivers/soc/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index fd7717d597fc..4c91705a8bb3 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -23,7 +23,7 @@ obj-y				+= pxa/
>  obj-y				+= amlogic/
>  obj-y				+= qcom/
>  obj-y				+= renesas/
> -obj-$(CONFIG_ARCH_ROCKCHIP)	+= rockchip/
> +obj-y				+= rockchip/
>  obj-$(CONFIG_SOC_SAMSUNG)	+= samsung/
>  obj-$(CONFIG_SOC_SIFIVE)	+= sifive/
>  obj-y				+= sunxi/
> -- 
> 2.36.0.rc2.479.g8af0fa9b8e-goog
> 

Re: [PATCH v3 2/3] soc: rockchip: power-domain: Replace dsb() with smb()

[Peter Geis]

On Mon, Apr 25, 2022 at 9:46 PM Brian Norris <briannorris@chromium.org> wrote:
>
> It's unclear if these are really needed at all, but seemingly their
> purpose is only as a write barrier. Use the general macro instead of the
> ARM-specific one.
>
> This driver is partially marked for COMPILE_TEST'ing, but it doesn't
> build under non-ARM architectures. Fix this up before *really* enabling
> it for COMPILE_TEST.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>
> Changes in v3:
>  * New in v3
>
>  drivers/soc/rockchip/pm_domains.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
> index 1b029e494274..cf16ff9b73b3 100644
> --- a/drivers/soc/rockchip/pm_domains.c
> +++ b/drivers/soc/rockchip/pm_domains.c
> @@ -178,7 +178,7 @@ static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
>                 regmap_update_bits(pmu->regmap, pmu->info->req_offset,
>                                    pd_info->req_mask, idle ? -1U : 0);
>
> -       dsb(sy);
> +       wmb();

Just curious, shouldn't this be mb() instead of wmb()?
From the arm64 barrier.h:

#define mb() dsb(sy)
#define wmb() dsb(st)


>
>         /* Wait util idle_ack = 1 */
>         target_ack = idle ? pd_info->ack_mask : 0;
> @@ -285,7 +285,7 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>                 regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
>                                    pd->info->pwr_mask, on ? 0 : -1U);
>
> -       dsb(sy);
> +       wmb();
>
>         if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
>                                       is_on == on, 0, 10000)) {
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 2/3] soc: rockchip: power-domain: Replace dsb() with smb()

[Robin Murphy]

On 2022-04-27 00:55, Peter Geis wrote:
> On Mon, Apr 25, 2022 at 9:46 PM Brian Norris <briannorris@chromium.org> wrote:
>>
>> It's unclear if these are really needed at all, but seemingly their
>> purpose is only as a write barrier. Use the general macro instead of the
>> ARM-specific one.
>>
>> This driver is partially marked for COMPILE_TEST'ing, but it doesn't
>> build under non-ARM architectures. Fix this up before *really* enabling
>> it for COMPILE_TEST.
>>
>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>> ---
>>
>> Changes in v3:
>>   * New in v3
>>
>>   drivers/soc/rockchip/pm_domains.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
>> index 1b029e494274..cf16ff9b73b3 100644
>> --- a/drivers/soc/rockchip/pm_domains.c
>> +++ b/drivers/soc/rockchip/pm_domains.c
>> @@ -178,7 +178,7 @@ static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
>>                  regmap_update_bits(pmu->regmap, pmu->info->req_offset,
>>                                     pd_info->req_mask, idle ? -1U : 0);
>>
>> -       dsb(sy);
>> +       wmb();
> 
> Just curious, shouldn't this be mb() instead of wmb()?
>  From the arm64 barrier.h:
> 
> #define mb() dsb(sy)
> #define wmb() dsb(st)

As I mentioned on v2, that would be the literal translation, however 
there's no concurrency since this is happening under a mutex, so there's 
no other agent against whose accesses loads would need to be 
synchronised, therefore the only logical reason those DSBs were ever 
there at all must be to ensure that the prior store(s) have been issued 
to their destination before proceeding. The history implies that this 
dates all the way back to RK3288, where Armv7's argument-less DSB lacked 
that distinction anyway.

Robin.

>>
>>          /* Wait util idle_ack = 1 */
>>          target_ack = idle ? pd_info->ack_mask : 0;
>> @@ -285,7 +285,7 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>>                  regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
>>                                     pd->info->pwr_mask, on ? 0 : -1U);
>>
>> -       dsb(sy);
>> +       wmb();
>>
>>          if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
>>                                        is_on == on, 0, 10000)) {
>> --
>> 2.36.0.rc2.479.g8af0fa9b8e-goog
>>
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 2/3] soc: rockchip: power-domain: Replace dsb() with smb()

[Brian Norris]

On Tue, Apr 26, 2022 at 5:25 PM Robin Murphy <robin.murphy@arm.com> wrote:
> On 2022-04-27 00:55, Peter Geis wrote:
> > On Mon, Apr 25, 2022 at 9:46 PM Brian Norris <briannorris@chromium.org> wrote:
> >>
> >> It's unclear if these are really needed at all, but seemingly their
> >> purpose is only as a write barrier. Use the general macro instead of the
> >> ARM-specific one.
...
> >> -       dsb(sy);
> >> +       wmb();
> >
> > Just curious, shouldn't this be mb() instead of wmb()?
> >  From the arm64 barrier.h:
> >
> > #define mb() dsb(sy)
> > #define wmb() dsb(st)
>
> As I mentioned on v2, that would be the literal translation, however
> there's no concurrency since this is happening under a mutex, so there's
> no other agent against whose accesses loads would need to be
> synchronised, therefore the only logical reason those DSBs were ever
> there at all must be to ensure that the prior store(s) have been issued
> to their destination before proceeding. The history implies that this
> dates all the way back to RK3288, where Armv7's argument-less DSB lacked
> that distinction anyway.

Thanks Robin. I already tried to capture part of this in the commit message:

"It's unclear if these are really needed at all, but seemingly their
purpose is only as a write barrier."

i.e., it's intentional that I'm making a change, not a literal translation.

I ran through a few tests on Rockchip RK3399, FWIW, although I suppose
some nasty memory ordering bugs are not exactly the kind of thing that
would fall out in smoke tests.

Brian

Re: [PATCH v3 1/3] soc: rockchip: Clean up Kconfig whitespace

[Heiko Stuebner]

On Mon, 25 Apr 2022 18:45:42 -0700, Brian Norris wrote:
> There are spaces instead of tabs, and other inconsistent indentation.

Applied, thanks!

[1/3] soc: rockchip: Clean up Kconfig whitespace
      commit: 9be1a9996ebd8da27cafc947e4578b2d419dc07e
[2/3] soc: rockchip: power-domain: Replace dsb() with smb()
      commit: 2ca9e472c70f15de768ab200571e2f6634f66394
[3/3] soc: rockchip: Fix compile-testing SoC drivers
      commit: add9f6f30e54b5c07e7a0260cda459ef1d9646b7

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>