| Message ID | 0ef8e87ba7156e626d1a1a48388f222ce917099b.1524472331.git.sean.wang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/23/2018 10:36 AM, sean.wang@mediatek.com wrote: > From: Sean Wang <sean.wang@mediatek.com> > > 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 <sean.wang@mediatek.com> > Cc: Matthias Brugger <matthias.bgg@gmail.com> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Weiyi Lu <weiyi.lu@mediatek.com> > --- > 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, > }, > }; > >
On Mon, 2018-04-23 at 11:31 +0200, Matthias Brugger wrote: > > On 04/23/2018 10:36 AM, sean.wang@mediatek.com wrote: > > From: Sean Wang <sean.wang@mediatek.com> > > > > 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 <sean.wang@mediatek.com> > > Cc: Matthias Brugger <matthias.bgg@gmail.com> > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > > Cc: Weiyi Lu <weiyi.lu@mediatek.com> > > --- > > 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, > > }, > > }; > > > >
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, sean.wang@mediatek.com wrote: >>> From: Sean Wang <sean.wang@mediatek.com> >>> >>> 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 <sean.wang@mediatek.com> >>> Cc: Matthias Brugger <matthias.bgg@gmail.com> >>> Cc: Ulf Hansson <ulf.hansson@linaro.org> >>> Cc: Weiyi Lu <weiyi.lu@mediatek.com> >>> --- >>> 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, >>> }, >>> }; >>> >>> > >
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, sean.wang@mediatek.com wrote: > >>> From: Sean Wang <sean.wang@mediatek.com> > >>> > >>> 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 <sean.wang@mediatek.com> > >>> Cc: Matthias Brugger <matthias.bgg@gmail.com> > >>> Cc: Ulf Hansson <ulf.hansson@linaro.org> > >>> Cc: Weiyi Lu <weiyi.lu@mediatek.com> > >>> --- > >>> 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, > >>> }, > >>> }; > >>> > >>> > > > >
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, sean.wang@mediatek.com wrote: >>>>> From: Sean Wang <sean.wang@mediatek.com> >>>>> >>>>> 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 <sean.wang@mediatek.com> >>>>> Cc: Matthias Brugger <matthias.bgg@gmail.com> >>>>> Cc: Ulf Hansson <ulf.hansson@linaro.org> >>>>> Cc: Weiyi Lu <weiyi.lu@mediatek.com> >>>>> --- >>>>> 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
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, sean.wang@mediatek.com wrote: > >>>>> From: Sean Wang <sean.wang@mediatek.com> > >>>>> > >>>>> 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 <sean.wang@mediatek.com> > >>>>> Cc: Matthias Brugger <matthias.bgg@gmail.com> > >>>>> Cc: Ulf Hansson <ulf.hansson@linaro.org> > >>>>> Cc: Weiyi Lu <weiyi.lu@mediatek.com> > >>>>> --- > >>>>> 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
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, }, };