Re: [PATCH v10 06/12] soc: mediatek: Use bp_table for all compatibles

[Nicolas Boichat <drinkcat@chromium.org>]

On Wed, Dec 18, 2019 at 4:31 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
>
> Only use bp_table for bus protection of all compatibles,
> instead of mixing bus_prot_mask and bus_prot_reg_update.

ditto, I'd just squash in the previous patch.

> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-scpsys.c | 94 ++++++++++++++++++++-------------------
>  1 file changed, 48 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> index 5699d9f..c438c53 100644
> --- a/drivers/soc/mediatek/mtk-scpsys.c
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> @@ -11,7 +11,6 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
>  #include <linux/regulator/consumer.h>
> -#include <linux/soc/mediatek/infracfg.h>
>  #include <linux/soc/mediatek/scpsys-ext.h>
>
>  #include <dt-bindings/power/mt2701-power.h>
> @@ -88,7 +87,6 @@
>   * @ctl_offs: The offset for main power control register.
>   * @sram_pdn_bits: The mask for sram power control bits.
>   * @sram_pdn_ack_bits: The mask for sram power control acked bits.
> - * @bus_prot_mask: The mask for single step bus protection.
>   * @basic_clk_name: The basic clocks required by this power domain.
>   * @caps: The flag for active wake-up action.
>   * @bp_table: The mask table for multiple step bus protection.
> @@ -99,7 +97,6 @@ struct scp_domain_data {
>         int ctl_offs;
>         u32 sram_pdn_bits;
>         u32 sram_pdn_ack_bits;
> -       u32 bus_prot_mask;
>         const char *basic_clk_name[MAX_CLKS];
>         u8 caps;
>         struct bus_prot bp_table[MAX_STEPS];
> @@ -128,7 +125,6 @@ struct scp {
>         struct regmap *infracfg;
>         struct regmap *smi_common;
>         struct scp_ctrl_reg ctrl_reg;
> -       bool bus_prot_reg_update;
>  };
>
>  struct scp_subdomain {
> @@ -142,7 +138,6 @@ struct scp_soc_data {
>         const struct scp_subdomain *subdomains;
>         int num_subdomains;
>         const struct scp_ctrl_reg regs;
> -       bool bus_prot_reg_update;
>  };
>
>  static int scpsys_domain_is_on(struct scp_domain *scpd)
> @@ -256,12 +251,6 @@ static int scpsys_bus_protect_enable(struct scp_domain *scpd)
>  {
>         struct scp *scp = scpd->scp;
>
> -       if (scpd->data->bus_prot_mask) {
> -               return mtk_infracfg_set_bus_protection(scp->infracfg,
> -                               scpd->data->bus_prot_mask,
> -                               scp->bus_prot_reg_update);
> -       }
> -
>         return mtk_scpsys_ext_set_bus_protection(scpd->data->bp_table,
>                         scp->infracfg, scp->smi_common);
>  }
> @@ -270,12 +259,6 @@ static int scpsys_bus_protect_disable(struct scp_domain *scpd)
>  {
>         struct scp *scp = scpd->scp;
>
> -       if (scpd->data->bus_prot_mask) {
> -               return mtk_infracfg_clear_bus_protection(scp->infracfg,
> -                               scpd->data->bus_prot_mask,
> -                               scp->bus_prot_reg_update);
> -       }
> -
>         return mtk_scpsys_ext_clear_bus_protection(scpd->data->bp_table,
>                         scp->infracfg, scp->smi_common);
>  }
> @@ -412,8 +395,7 @@ static int init_basic_clks(struct platform_device *pdev, struct clk **clk,
>
>  static struct scp *init_scp(struct platform_device *pdev,
>                         const struct scp_domain_data *scp_domain_data, int num,
> -                       const struct scp_ctrl_reg *scp_ctrl_reg,
> -                       bool bus_prot_reg_update)
> +                       const struct scp_ctrl_reg *scp_ctrl_reg)
>  {
>         struct genpd_onecell_data *pd_data;
>         struct resource *res;
> @@ -427,8 +409,6 @@ static struct scp *init_scp(struct platform_device *pdev,
>         scp->ctrl_reg.pwr_sta_offs = scp_ctrl_reg->pwr_sta_offs;
>         scp->ctrl_reg.pwr_sta2nd_offs = scp_ctrl_reg->pwr_sta2nd_offs;
>
> -       scp->bus_prot_reg_update = bus_prot_reg_update;
> -
>         scp->dev = &pdev->dev;
>
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -549,8 +529,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>                 .name = "conn",
>                 .sta_mask = PWR_STATUS_CONN,
>                 .ctl_offs = SPM_CONN_PWR_CON,
> -               .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN_M |
> -                                MT2701_TOP_AXI_PROT_EN_CONN_S,
> +               .bp_table = {
> +                       BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> +                               BIT(2) | BIT(8), BIT(2) | BIT(8)),
> +               },

I'm a bit sad we lose the information about the BIT meaning.

Of course this looks ugly and verbose:
                      BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
                               MT2701_TOP_AXI_PROT_EN_CONN_M |
MT2701_TOP_AXI_PROT_EN_CONN_S,
                               MT2701_TOP_AXI_PROT_EN_CONN_M |
MT2701_TOP_AXI_PROT_EN_CONN_S),

But if you make "check_clr_mask" a boolean, you wouldn't have to
repeat the mask twice and you could keep the nice register bit
definitions.

[snip, many similar occurences below]