diff mbox series

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

Message ID 20220426014545.628100-2-briannorris@chromium.org (mailing list archive)
State New
Headers show
Series [v3,1/3] soc: rockchip: Clean up Kconfig whitespace | expand

Commit Message

Brian Norris April 26, 2022, 1:45 a.m. UTC
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(-)

Comments

Guenter Roeck April 26, 2022, 8:14 p.m. UTC | #1
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
>
Peter Geis April 26, 2022, 11:55 p.m. UTC | #2
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
Robin Murphy April 27, 2022, 12:25 a.m. UTC | #3
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
Brian Norris April 27, 2022, 9:47 p.m. UTC | #4
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
diff mbox series

Patch

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)) {