[v2,2/2] soc: mediatek: add a fixed wait for SRAM stable

Commit Message

Sean Wang April 23, 2018, 8:36 a.m. UTC

From: Sean Wang <[e-mail]>

MT7622_POWER_DOMAIN_WB doesn't send an ACK when its managed SRAM becomes
stable, which is not like the behavior the other power domains should
have. Therefore, it's necessary for such a power domain to have a fixed
and well-predefined duration to wait until its managed SRAM can be allowed
to access by all functions running on the top.

v1 -> v2:
 - use MTK_SCPD_FWAIT_SRAM flag as an indication requiring force waiting.

Signed-off-by: Sean Wang <[e-mail]>
Cc: Matthias Brugger <[e-mail]>
Cc: Ulf Hansson <[e-mail]>
Cc: Weiyi Lu <[e-mail]>
---
 drivers/soc/mediatek/mtk-scpsys.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

Comments

Matthias Brugger April 23, 2018, 9:31 a.m. UTC | #1

On 04/23/2018 10:36 AM, [e-mail] wrote:
> From: Sean Wang <[e-mail]>
> 
> MT7622_POWER_DOMAIN_WB doesn't send an ACK when its managed SRAM becomes
> stable, which is not like the behavior the other power domains should
> have. Therefore, it's necessary for such a power domain to have a fixed
> and well-predefined duration to wait until its managed SRAM can be allowed
> to access by all functions running on the top.
> 
> v1 -> v2:
>  - use MTK_SCPD_FWAIT_SRAM flag as an indication requiring force waiting.
> 
> Signed-off-by: Sean Wang <[e-mail]>
> Cc: Matthias Brugger <[e-mail]>
> Cc: Ulf Hansson <[e-mail]>
> Cc: Weiyi Lu <[e-mail]>
> ---
>  drivers/soc/mediatek/mtk-scpsys.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> index b1b45e4..d4f1a63 100644
> --- a/drivers/soc/mediatek/mtk-scpsys.c
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> @@ -32,6 +32,7 @@
>  #define MTK_POLL_TIMEOUT    (jiffies_to_usecs(HZ))
>  
>  #define MTK_SCPD_ACTIVE_WAKEUP		BIT(0)
> +#define MTK_SCPD_FWAIT_SRAM		BIT(1)
>  #define MTK_SCPD_CAPS(_scpd, _x)	((_scpd)->data->caps & (_x))
>  
>  #define SPM_VDE_PWR_CON			0x0210
> @@ -237,11 +238,22 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>  	val &= ~scpd->data->sram_pdn_bits;
>  	writel(val, ctl_addr);
>  
> -	/* wait until SRAM_PDN_ACK all 0 */
> -	ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
> -				 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> -	if (ret < 0)
> -		goto err_pwr_ack;
> +	/* Either wait until SRAM_PDN_ACK all 0 or have a force wait */
> +	if (!MTK_SCPD_CAPS(scpd, MTK_SCPD_FWAIT_SRAM)) {
> +		ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
> +					 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> +		if (ret < 0)
> +			goto err_pwr_ack;
> +	} else {
> +		/*
> +		 * Currently, MTK_SCPD_FWAIT_SRAM is necessary only for
> +		 * MT7622_POWER_DOMAIN_WB and thus just a trivial setup is
> +		 * applied here. If there're more domains which need to force
> +		 * waiting for its own pre-defined value, the duration should
> +		 * be coded in the caps field.
> +		 */

I would say, if necessary in the future we can add a switch statement here.
Other then that the patches look good. If you are OK, I'll just delete the last
sentence when applying the patch.

Regards,
Matthias

> +		usleep_range(12000, 12100);
> +	};
>  
>  	if (scpd->data->bus_prot_mask) {
>  		ret = mtk_infracfg_clear_bus_protection(scp->infracfg,
> @@ -785,7 +797,7 @@ static const struct scp_domain_data scp_domain_data_mt7622[] = {
>  		.sram_pdn_ack_bits = 0,
>  		.clk_id = {CLK_NONE},
>  		.bus_prot_mask = MT7622_TOP_AXI_PROT_EN_WB,
> -		.caps = MTK_SCPD_ACTIVE_WAKEUP,
> +		.caps = MTK_SCPD_ACTIVE_WAKEUP | MTK_SCPD_FWAIT_SRAM,
>  	},
>  };
>  
>

Sean Wang April 23, 2018, 9:39 a.m. UTC | #2

On Mon, 2018-04-23 at 11:31 +0200, Matthias Brugger wrote:
> 
> On 04/23/2018 10:36 AM, [e-mail] wrote:
> > From: Sean Wang <[e-mail]>
> > 
> > MT7622_POWER_DOMAIN_WB doesn't send an ACK when its managed SRAM becomes
> > stable, which is not like the behavior the other power domains should
> > have. Therefore, it's necessary for such a power domain to have a fixed
> > and well-predefined duration to wait until its managed SRAM can be allowed
> > to access by all functions running on the top.
> > 
> > v1 -> v2:
> >  - use MTK_SCPD_FWAIT_SRAM flag as an indication requiring force waiting.
> > 
> > Signed-off-by: Sean Wang <[e-mail]>
> > Cc: Matthias Brugger <[e-mail]>
> > Cc: Ulf Hansson <[e-mail]>
> > Cc: Weiyi Lu <[e-mail]>
> > ---
> >  drivers/soc/mediatek/mtk-scpsys.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> > index b1b45e4..d4f1a63 100644
> > --- a/drivers/soc/mediatek/mtk-scpsys.c
> > +++ b/drivers/soc/mediatek/mtk-scpsys.c
> > @@ -32,6 +32,7 @@
> >  #define MTK_POLL_TIMEOUT    (jiffies_to_usecs(HZ))
> >  
> >  #define MTK_SCPD_ACTIVE_WAKEUP		BIT(0)
> > +#define MTK_SCPD_FWAIT_SRAM		BIT(1)
> >  #define MTK_SCPD_CAPS(_scpd, _x)	((_scpd)->data->caps & (_x))
> >  
> >  #define SPM_VDE_PWR_CON			0x0210
> > @@ -237,11 +238,22 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> >  	val &= ~scpd->data->sram_pdn_bits;
> >  	writel(val, ctl_addr);
> >  
> > -	/* wait until SRAM_PDN_ACK all 0 */
> > -	ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
> > -				 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > -	if (ret < 0)
> > -		goto err_pwr_ack;
> > +	/* Either wait until SRAM_PDN_ACK all 0 or have a force wait */
> > +	if (!MTK_SCPD_CAPS(scpd, MTK_SCPD_FWAIT_SRAM)) {
> > +		ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
> > +					 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > +		if (ret < 0)
> > +			goto err_pwr_ack;
> > +	} else {
> > +		/*
> > +		 * Currently, MTK_SCPD_FWAIT_SRAM is necessary only for
> > +		 * MT7622_POWER_DOMAIN_WB and thus just a trivial setup is
> > +		 * applied here. If there're more domains which need to force
> > +		 * waiting for its own pre-defined value, the duration should
> > +		 * be coded in the caps field.
> > +		 */
> 
> I would say, if necessary in the future we can add a switch statement here.
> Other then that the patches look good. If you are OK, I'll just delete the last
> sentence when applying the patch.
> 

yes, it's okay for me. 

> Regards,
> Matthias
> 
> > +		usleep_range(12000, 12100);
> > +	};
> >  
> >  	if (scpd->data->bus_prot_mask) {
> >  		ret = mtk_infracfg_clear_bus_protection(scp->infracfg,
> > @@ -785,7 +797,7 @@ static const struct scp_domain_data scp_domain_data_mt7622[] = {
> >  		.sram_pdn_ack_bits = 0,
> >  		.clk_id = {CLK_NONE},
> >  		.bus_prot_mask = MT7622_TOP_AXI_PROT_EN_WB,
> > -		.caps = MTK_SCPD_ACTIVE_WAKEUP,
> > +		.caps = MTK_SCPD_ACTIVE_WAKEUP | MTK_SCPD_FWAIT_SRAM,
> >  	},
> >  };
> >  
> >

Matthias Brugger April 27, 2018, 9:46 a.m. UTC | #3

Hi Sean,

On 04/23/2018 11:39 AM, Sean Wang wrote:
> On Mon, 2018-04-23 at 11:31 +0200, Matthias Brugger wrote:
>>
>> On 04/23/2018 10:36 AM, [e-mail] wrote:
>>> From: Sean Wang <[e-mail]>
>>>
>>> MT7622_POWER_DOMAIN_WB doesn't send an ACK when its managed SRAM becomes
>>> stable, which is not like the behavior the other power domains should
>>> have. Therefore, it's necessary for such a power domain to have a fixed
>>> and well-predefined duration to wait until its managed SRAM can be allowed
>>> to access by all functions running on the top.
>>>
>>> v1 -> v2:
>>>  - use MTK_SCPD_FWAIT_SRAM flag as an indication requiring force waiting.
>>>
>>> Signed-off-by: Sean Wang <[e-mail]>
>>> Cc: Matthias Brugger <[e-mail]>
>>> Cc: Ulf Hansson <[e-mail]>
>>> Cc: Weiyi Lu <[e-mail]>
>>> ---
>>>  drivers/soc/mediatek/mtk-scpsys.c | 24 ++++++++++++++++++------
>>>  1 file changed, 18 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
>>> index b1b45e4..d4f1a63 100644
>>> --- a/drivers/soc/mediatek/mtk-scpsys.c
>>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
>>> @@ -32,6 +32,7 @@
>>>  #define MTK_POLL_TIMEOUT    (jiffies_to_usecs(HZ))
>>>  
>>>  #define MTK_SCPD_ACTIVE_WAKEUP		BIT(0)
>>> +#define MTK_SCPD_FWAIT_SRAM		BIT(1)
>>>  #define MTK_SCPD_CAPS(_scpd, _x)	((_scpd)->data->caps & (_x))
>>>  
>>>  #define SPM_VDE_PWR_CON			0x0210
>>> @@ -237,11 +238,22 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>>  	val &= ~scpd->data->sram_pdn_bits;
>>>  	writel(val, ctl_addr);
>>>  
>>> -	/* wait until SRAM_PDN_ACK all 0 */
>>> -	ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
>>> -				 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> -	if (ret < 0)
>>> -		goto err_pwr_ack;
>>> +	/* Either wait until SRAM_PDN_ACK all 0 or have a force wait */
>>> +	if (!MTK_SCPD_CAPS(scpd, MTK_SCPD_FWAIT_SRAM)) {

After having another look on the patch, could you change the order of the if:
So that we check for the existence of the MTK_SCPD_FWAIT_SRAM and sleep and in
the else branch we to the readl_poll_timeout.

I think in the future this will make the code easier to understand as you can
easily oversee the '!' negation in the if.

Regards,
Matthias


>>> +		ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
>>> +					 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> +		if (ret < 0)
>>> +			goto err_pwr_ack;
>>> +	} else {
>>> +		/*
>>> +		 * Currently, MTK_SCPD_FWAIT_SRAM is necessary only for
>>> +		 * MT7622_POWER_DOMAIN_WB and thus just a trivial setup is
>>> +		 * applied here. If there're more domains which need to force
>>> +		 * waiting for its own pre-defined value, the duration should
>>> +		 * be coded in the caps field.
>>> +		 */
>>
>> I would say, if necessary in the future we can add a switch statement here.
>> Other then that the patches look good. If you are OK, I'll just delete the last
>> sentence when applying the patch.
>>
> 
> yes, it's okay for me. 
> 
>> Regards,
>> Matthias
>>
>>> +		usleep_range(12000, 12100);
>>> +	};
>>>  
>>>  	if (scpd->data->bus_prot_mask) {
>>>  		ret = mtk_infracfg_clear_bus_protection(scp->infracfg,
>>> @@ -785,7 +797,7 @@ static const struct scp_domain_data scp_domain_data_mt7622[] = {
>>>  		.sram_pdn_ack_bits = 0,
>>>  		.clk_id = {CLK_NONE},
>>>  		.bus_prot_mask = MT7622_TOP_AXI_PROT_EN_WB,
>>> -		.caps = MTK_SCPD_ACTIVE_WAKEUP,
>>> +		.caps = MTK_SCPD_ACTIVE_WAKEUP | MTK_SCPD_FWAIT_SRAM,
>>>  	},
>>>  };
>>>  
>>>
> 
>

Sean Wang April 30, 2018, 7:08 a.m. UTC | #4

On Fri, 2018-04-27 at 11:46 +0200, Matthias Brugger wrote:
> Hi Sean,
> 
> On 04/23/2018 11:39 AM, Sean Wang wrote:
> > On Mon, 2018-04-23 at 11:31 +0200, Matthias Brugger wrote:
> >>
> >> On 04/23/2018 10:36 AM, [e-mail] wrote:
> >>> From: Sean Wang <[e-mail]>
> >>>
> >>> MT7622_POWER_DOMAIN_WB doesn't send an ACK when its managed SRAM becomes
> >>> stable, which is not like the behavior the other power domains should
> >>> have. Therefore, it's necessary for such a power domain to have a fixed
> >>> and well-predefined duration to wait until its managed SRAM can be allowed
> >>> to access by all functions running on the top.
> >>>
> >>> v1 -> v2:
> >>>  - use MTK_SCPD_FWAIT_SRAM flag as an indication requiring force waiting.
> >>>
> >>> Signed-off-by: Sean Wang <[e-mail]>
> >>> Cc: Matthias Brugger <[e-mail]>
> >>> Cc: Ulf Hansson <[e-mail]>
> >>> Cc: Weiyi Lu <[e-mail]>
> >>> ---
> >>>  drivers/soc/mediatek/mtk-scpsys.c | 24 ++++++++++++++++++------
> >>>  1 file changed, 18 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> >>> index b1b45e4..d4f1a63 100644
> >>> --- a/drivers/soc/mediatek/mtk-scpsys.c
> >>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> >>> @@ -32,6 +32,7 @@
> >>>  #define MTK_POLL_TIMEOUT    (jiffies_to_usecs(HZ))
> >>>  
> >>>  #define MTK_SCPD_ACTIVE_WAKEUP		BIT(0)
> >>> +#define MTK_SCPD_FWAIT_SRAM		BIT(1)
> >>>  #define MTK_SCPD_CAPS(_scpd, _x)	((_scpd)->data->caps & (_x))
> >>>  
> >>>  #define SPM_VDE_PWR_CON			0x0210
> >>> @@ -237,11 +238,22 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> >>>  	val &= ~scpd->data->sram_pdn_bits;
> >>>  	writel(val, ctl_addr);
> >>>  
> >>> -	/* wait until SRAM_PDN_ACK all 0 */
> >>> -	ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
> >>> -				 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> >>> -	if (ret < 0)
> >>> -		goto err_pwr_ack;
> >>> +	/* Either wait until SRAM_PDN_ACK all 0 or have a force wait */
> >>> +	if (!MTK_SCPD_CAPS(scpd, MTK_SCPD_FWAIT_SRAM)) {
> 
> After having another look on the patch, could you change the order of the if:
> So that we check for the existence of the MTK_SCPD_FWAIT_SRAM and sleep and in
> the else branch we to the readl_poll_timeout.
> 
> I think in the future this will make the code easier to understand as you can
> easily oversee the '!' negation in the if.
> 
> Regards,
> Matthias
> 

Initial thought on the patch is that I would like to save a branch
instruction for a most possibly executed block. Or would it be better to
add a compiler to branch prediction information? something like that  

        /* Either wait until SRAM_PDN_ACK all 0 or have a force wait */
        if (unlikely(MTK_SCPD_CAPS(scpd, MTK_SCPD_FWAIT_SRAM))) {
                /*
                 * Currently, MTK_SCPD_FWAIT_SRAM is necessary only for
                 * MT7622_POWER_DOMAIN_WB and thus just a trivial setup
is
                 * applied here.
                 */
                usleep_range(12000, 12100);
...
 


> 
> >>> +		ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
> >>> +					 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> >>> +		if (ret < 0)
> >>> +			goto err_pwr_ack;
> >>> +	} else {
> >>> +		/*
> >>> +		 * Currently, MTK_SCPD_FWAIT_SRAM is necessary only for
> >>> +		 * MT7622_POWER_DOMAIN_WB and thus just a trivial setup is
> >>> +		 * applied here. If there're more domains which need to force
> >>> +		 * waiting for its own pre-defined value, the duration should
> >>> +		 * be coded in the caps field.
> >>> +		 */
> >>
> >> I would say, if necessary in the future we can add a switch statement here.
> >> Other then that the patches look good. If you are OK, I'll just delete the last
> >> sentence when applying the patch.
> >>
> > 
> > yes, it's okay for me. 
> > 
> >> Regards,
> >> Matthias
> >>
> >>> +		usleep_range(12000, 12100);
> >>> +	};
> >>>  
> >>>  	if (scpd->data->bus_prot_mask) {
> >>>  		ret = mtk_infracfg_clear_bus_protection(scp->infracfg,
> >>> @@ -785,7 +797,7 @@ static const struct scp_domain_data scp_domain_data_mt7622[] = {
> >>>  		.sram_pdn_ack_bits = 0,
> >>>  		.clk_id = {CLK_NONE},
> >>>  		.bus_prot_mask = MT7622_TOP_AXI_PROT_EN_WB,
> >>> -		.caps = MTK_SCPD_ACTIVE_WAKEUP,
> >>> +		.caps = MTK_SCPD_ACTIVE_WAKEUP | MTK_SCPD_FWAIT_SRAM,
> >>>  	},
> >>>  };
> >>>  
> >>>
> > 
> >

Matthias Brugger April 30, 2018, 9:10 a.m. UTC | #5

On 04/30/2018 09:08 AM, Sean Wang wrote:
> On Fri, 2018-04-27 at 11:46 +0200, Matthias Brugger wrote:
>> Hi Sean,
>>
>> On 04/23/2018 11:39 AM, Sean Wang wrote:
>>> On Mon, 2018-04-23 at 11:31 +0200, Matthias Brugger wrote:
>>>>
>>>> On 04/23/2018 10:36 AM, [e-mail] wrote:
>>>>> From: Sean Wang <[e-mail]>
>>>>>
>>>>> MT7622_POWER_DOMAIN_WB doesn't send an ACK when its managed SRAM becomes
>>>>> stable, which is not like the behavior the other power domains should
>>>>> have. Therefore, it's necessary for such a power domain to have a fixed
>>>>> and well-predefined duration to wait until its managed SRAM can be allowed
>>>>> to access by all functions running on the top.
>>>>>
>>>>> v1 -> v2:
>>>>>  - use MTK_SCPD_FWAIT_SRAM flag as an indication requiring force waiting.
>>>>>
>>>>> Signed-off-by: Sean Wang <[e-mail]>
>>>>> Cc: Matthias Brugger <[e-mail]>
>>>>> Cc: Ulf Hansson <[e-mail]>
>>>>> Cc: Weiyi Lu <[e-mail]>
>>>>> ---
>>>>>  drivers/soc/mediatek/mtk-scpsys.c | 24 ++++++++++++++++++------
>>>>>  1 file changed, 18 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
>>>>> index b1b45e4..d4f1a63 100644
>>>>> --- a/drivers/soc/mediatek/mtk-scpsys.c
>>>>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
>>>>> @@ -32,6 +32,7 @@
>>>>>  #define MTK_POLL_TIMEOUT    (jiffies_to_usecs(HZ))
>>>>>  
>>>>>  #define MTK_SCPD_ACTIVE_WAKEUP		BIT(0)
>>>>> +#define MTK_SCPD_FWAIT_SRAM		BIT(1)
>>>>>  #define MTK_SCPD_CAPS(_scpd, _x)	((_scpd)->data->caps & (_x))
>>>>>  
>>>>>  #define SPM_VDE_PWR_CON			0x0210
>>>>> @@ -237,11 +238,22 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>>>>  	val &= ~scpd->data->sram_pdn_bits;
>>>>>  	writel(val, ctl_addr);
>>>>>  
>>>>> -	/* wait until SRAM_PDN_ACK all 0 */
>>>>> -	ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
>>>>> -				 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>>>> -	if (ret < 0)
>>>>> -		goto err_pwr_ack;
>>>>> +	/* Either wait until SRAM_PDN_ACK all 0 or have a force wait */
>>>>> +	if (!MTK_SCPD_CAPS(scpd, MTK_SCPD_FWAIT_SRAM)) {
>>
>> After having another look on the patch, could you change the order of the if:
>> So that we check for the existence of the MTK_SCPD_FWAIT_SRAM and sleep and in
>> the else branch we to the readl_poll_timeout.
>>
>> I think in the future this will make the code easier to understand as you can
>> easily oversee the '!' negation in the if.
>>
>> Regards,
>> Matthias
>>
> 
> Initial thought on the patch is that I would like to save a branch
> instruction for a most possibly executed block. Or would it be better to
> add a compiler to branch prediction information? something like that  
> 
>         /* Either wait until SRAM_PDN_ACK all 0 or have a force wait */
>         if (unlikely(MTK_SCPD_CAPS(scpd, MTK_SCPD_FWAIT_SRAM))) {
>                 /*
>                  * Currently, MTK_SCPD_FWAIT_SRAM is necessary only for
>                  * MT7622_POWER_DOMAIN_WB and thus just a trivial setup
> is
>                  * applied here.
>                  */
>                 usleep_range(12000, 12100);
> ...
>  

Is this a performance critical path? I thought if you turn on the power domain
for some peripherals, it does not matter if you need a few CPU cycles more or less.

Regards,
Matthias

Sean Wang April 30, 2018, 9:59 a.m. UTC | #6

On Mon, 2018-04-30 at 11:10 +0200, Matthias Brugger wrote:
> 
> On 04/30/2018 09:08 AM, Sean Wang wrote:
> > On Fri, 2018-04-27 at 11:46 +0200, Matthias Brugger wrote:
> >> Hi Sean,
> >>
> >> On 04/23/2018 11:39 AM, Sean Wang wrote:
> >>> On Mon, 2018-04-23 at 11:31 +0200, Matthias Brugger wrote:
> >>>>
> >>>> On 04/23/2018 10:36 AM, [e-mail] wrote:
> >>>>> From: Sean Wang <[e-mail]>
> >>>>>
> >>>>> MT7622_POWER_DOMAIN_WB doesn't send an ACK when its managed SRAM becomes
> >>>>> stable, which is not like the behavior the other power domains should
> >>>>> have. Therefore, it's necessary for such a power domain to have a fixed
> >>>>> and well-predefined duration to wait until its managed SRAM can be allowed
> >>>>> to access by all functions running on the top.
> >>>>>
> >>>>> v1 -> v2:
> >>>>>  - use MTK_SCPD_FWAIT_SRAM flag as an indication requiring force waiting.
> >>>>>
> >>>>> Signed-off-by: Sean Wang <[e-mail]>
> >>>>> Cc: Matthias Brugger <[e-mail]>
> >>>>> Cc: Ulf Hansson <[e-mail]>
> >>>>> Cc: Weiyi Lu <[e-mail]>
> >>>>> ---
> >>>>>  drivers/soc/mediatek/mtk-scpsys.c | 24 ++++++++++++++++++------
> >>>>>  1 file changed, 18 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> >>>>> index b1b45e4..d4f1a63 100644
> >>>>> --- a/drivers/soc/mediatek/mtk-scpsys.c
> >>>>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> >>>>> @@ -32,6 +32,7 @@
> >>>>>  #define MTK_POLL_TIMEOUT    (jiffies_to_usecs(HZ))
> >>>>>  
> >>>>>  #define MTK_SCPD_ACTIVE_WAKEUP		BIT(0)
> >>>>> +#define MTK_SCPD_FWAIT_SRAM		BIT(1)
> >>>>>  #define MTK_SCPD_CAPS(_scpd, _x)	((_scpd)->data->caps & (_x))
> >>>>>  
> >>>>>  #define SPM_VDE_PWR_CON			0x0210
> >>>>> @@ -237,11 +238,22 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> >>>>>  	val &= ~scpd->data->sram_pdn_bits;
> >>>>>  	writel(val, ctl_addr);
> >>>>>  
> >>>>> -	/* wait until SRAM_PDN_ACK all 0 */
> >>>>> -	ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
> >>>>> -				 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> >>>>> -	if (ret < 0)
> >>>>> -		goto err_pwr_ack;
> >>>>> +	/* Either wait until SRAM_PDN_ACK all 0 or have a force wait */
> >>>>> +	if (!MTK_SCPD_CAPS(scpd, MTK_SCPD_FWAIT_SRAM)) {
> >>
> >> After having another look on the patch, could you change the order of the if:
> >> So that we check for the existence of the MTK_SCPD_FWAIT_SRAM and sleep and in
> >> the else branch we to the readl_poll_timeout.
> >>
> >> I think in the future this will make the code easier to understand as you can
> >> easily oversee the '!' negation in the if.
> >>
> >> Regards,
> >> Matthias
> >>
> > 
> > Initial thought on the patch is that I would like to save a branch
> > instruction for a most possibly executed block. Or would it be better to
> > add a compiler to branch prediction information? something like that  
> > 
> >         /* Either wait until SRAM_PDN_ACK all 0 or have a force wait */
> >         if (unlikely(MTK_SCPD_CAPS(scpd, MTK_SCPD_FWAIT_SRAM))) {
> >                 /*
> >                  * Currently, MTK_SCPD_FWAIT_SRAM is necessary only for
> >                  * MT7622_POWER_DOMAIN_WB and thus just a trivial setup
> > is
> >                  * applied here.
> >                  */
> >                 usleep_range(12000, 12100);
> > ...
> >  
> 
> Is this a performance critical path? I thought if you turn on the power domain
> for some peripherals, it does not matter if you need a few CPU cycles more or less.

thanks for your advice

it's not a critical path.

i'll send a new patch according to the result.

> Regards,
> Matthias

Patch

diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index b1b45e4..d4f1a63 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -32,6 +32,7 @@ 
 #define MTK_POLL_TIMEOUT    (jiffies_to_usecs(HZ))
 
 #define MTK_SCPD_ACTIVE_WAKEUP		BIT(0)
+#define MTK_SCPD_FWAIT_SRAM		BIT(1)
 #define MTK_SCPD_CAPS(_scpd, _x)	((_scpd)->data->caps & (_x))
 
 #define SPM_VDE_PWR_CON			0x0210
@@ -237,11 +238,22 @@  static int scpsys_power_on(struct generic_pm_domain *genpd)
 	val &= ~scpd->data->sram_pdn_bits;
 	writel(val, ctl_addr);
 
-	/* wait until SRAM_PDN_ACK all 0 */
-	ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
-				 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
-	if (ret < 0)
-		goto err_pwr_ack;
+	/* Either wait until SRAM_PDN_ACK all 0 or have a force wait */
+	if (!MTK_SCPD_CAPS(scpd, MTK_SCPD_FWAIT_SRAM)) {
+		ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
+					 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
+		if (ret < 0)
+			goto err_pwr_ack;
+	} else {
+		/*
+		 * Currently, MTK_SCPD_FWAIT_SRAM is necessary only for
+		 * MT7622_POWER_DOMAIN_WB and thus just a trivial setup is
+		 * applied here. If there're more domains which need to force
+		 * waiting for its own pre-defined value, the duration should
+		 * be coded in the caps field.
+		 */
+		usleep_range(12000, 12100);
+	};
 
 	if (scpd->data->bus_prot_mask) {
 		ret = mtk_infracfg_clear_bus_protection(scp->infracfg,
@@ -785,7 +797,7 @@  static const struct scp_domain_data scp_domain_data_mt7622[] = {
 		.sram_pdn_ack_bits = 0,
 		.clk_id = {CLK_NONE},
 		.bus_prot_mask = MT7622_TOP_AXI_PROT_EN_WB,
-		.caps = MTK_SCPD_ACTIVE_WAKEUP,
+		.caps = MTK_SCPD_ACTIVE_WAKEUP | MTK_SCPD_FWAIT_SRAM,
 	},
 };