diff mbox series

[v9,4/9] soc: mediatek: Add multiple step bus protection control

Message ID 1575960413-6900-5-git-send-email-weiyi.lu@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Mediatek MT8183 scpsys support | expand

Commit Message

Weiyi Lu Dec. 10, 2019, 6:46 a.m. UTC 
Both MT8183 & MT6765 have more control steps of bus protection
than previous project. And there add more bus protection registers
reside at infracfg & smi-common. Also add new APIs for multiple
step bus protection control with more customized arguments.

Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
---
 drivers/soc/mediatek/Makefile           |  2 +-
 drivers/soc/mediatek/mtk-scpsys-ext.c   | 99 +++++++++++++++++++++++++++++++++
 drivers/soc/mediatek/mtk-scpsys.c       | 39 +++++++++----
 include/linux/soc/mediatek/scpsys-ext.h | 39 +++++++++++++
 4 files changed, 168 insertions(+), 11 deletions(-)
 create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
 create mode 100644 include/linux/soc/mediatek/scpsys-ext.h

Comments

Nicolas Boichat Dec. 16, 2019, 7:21 a.m. UTC | #1 
On Tue, Dec 10, 2019 at 2:47 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
>
> Both MT8183 & MT6765 have more control steps of bus protection
> than previous project. And there add more bus protection registers
> reside at infracfg & smi-common. Also add new APIs for multiple
> step bus protection control with more customized arguments.
>
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> ---
>  drivers/soc/mediatek/Makefile           |  2 +-
>  drivers/soc/mediatek/mtk-scpsys-ext.c   | 99 +++++++++++++++++++++++++++++++++
>  drivers/soc/mediatek/mtk-scpsys.c       | 39 +++++++++----
>  include/linux/soc/mediatek/scpsys-ext.h | 39 +++++++++++++
>  4 files changed, 168 insertions(+), 11 deletions(-)
>  create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
>  create mode 100644 include/linux/soc/mediatek/scpsys-ext.h
>
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index b017330..b442be9 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
>  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
>  obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
> new file mode 100644
> index 0000000..4f1adda
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Owen Chen <Owen.Chen@mediatek.com>
> + */
> +#include <linux/ktime.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/soc/mediatek/scpsys-ext.h>
> +
> +#define MTK_POLL_DELAY_US   10
> +#define MTK_POLL_TIMEOUT    USEC_PER_SEC
> +
> +static int set_bus_protection(struct regmap *map, u32 mask, u32 ack_mask,
> +               u32 reg_set, u32 reg_sta, u32 reg_en)
> +{
> +       u32 val;
> +
> +       if (reg_set)
> +               regmap_write(map, reg_set, mask);
> +       else
> +               regmap_update_bits(map, reg_en, mask, mask);

At least for 8183, we never seen to use the reg_set case, can we
simplify this function?

> +
> +       return regmap_read_poll_timeout(map, reg_sta,
> +                       val, (val & ack_mask) == ack_mask,
> +                       MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);

From 8183, I see that you have either:
 1. mask == ack_mask
 2. ack_mask == 0 (essentially this skips this test)

Would it be simpler to just skip this test if reg_sta == 0, and always
assume mask == ack_mask otherwise?

e.g.
if (reg_sta == 0)
   return 0;

return regmap_read_poll_timeout(map, reg_sta,
                       val, (val & mask) == mask,
                       MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);

> +}
> +
> [snip]
> +
> +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
> +       struct regmap *infracfg, struct regmap *smi_common)
> +{
> +       int i;
> +
> +       for (i = 0; i < MAX_STEPS; i++) {
> +               struct regmap *map = NULL;
> +               int ret;
> +
> +               if (bp_table[i].type == INVALID_TYPE)
> +                       continue;

break? (but yes the one below in mtk_scpsys_ext_clear_bus_protection
has to be continue).

> +               else if (bp_table[i].type == IFR_TYPE)
> +                       map = infracfg;
> +               else if (bp_table[i].type == SMI_TYPE)
> +                       map = smi_common;
> +
> +               ret = set_bus_protection(map,
> +                               bp_table[i].mask, bp_table[i].mask,
> +                               bp_table[i].set_ofs, bp_table[i].sta_ofs,
> +                               bp_table[i].en_ofs);
> +
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
> +       struct regmap *infracfg, struct regmap *smi_common)
> +{
> +       int i;
> +
> +       for (i = MAX_STEPS - 1; i >= 0; i--) {
> +               struct regmap *map = NULL;
> +               int ret;
> +
> +               if (bp_table[i].type == INVALID_TYPE)
> +                       continue;
> +               else if (bp_table[i].type == IFR_TYPE)
> +                       map = infracfg;
> +               else if (bp_table[i].type == SMI_TYPE)
> +                       map = smi_common;
> +
> +               ret = clear_bus_protection(map,
> +                               bp_table[i].mask, bp_table[i].clr_ack_mask,
> +                               bp_table[i].clr_ofs, bp_table[i].sta_ofs,
> +                               bp_table[i].en_ofs);
> +
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> index 915d635..466bb749 100644
> --- a/drivers/soc/mediatek/mtk-scpsys.c
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> @@ -12,6 +12,7 @@
>  #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>
>  #include <dt-bindings/power/mt2712-power.h>
> @@ -120,6 +121,7 @@ enum clk_id {
>   * @basic_clk_id: provide the same purpose with field "clk_id"
>   *                by declaring basic clock prefix name rather than clk_id.
>   * @caps: The flag for active wake-up action.
> + * @bp_table: The mask table for multiple step bus protection.
>   */
>  struct scp_domain_data {
>         const char *name;
> @@ -131,6 +133,7 @@ struct scp_domain_data {
>         enum clk_id clk_id[MAX_CLKS];
>         const char *basic_clk_id[MAX_CLKS];
>         u8 caps;
> +       struct bus_prot bp_table[MAX_STEPS];

As with the previous patch, I'm not a big fan of having 2 approaches
for something similar (bus_prot_mask vs bp_table), can we define a
simple macro for this?
e.g.:
.bp_table = BUS_PROT_SINGLE(mask)
Weiyi Lu Dec. 17, 2019, 2:50 a.m. UTC | #2 
On Mon, 2019-12-16 at 15:21 +0800, Nicolas Boichat wrote:
> On Tue, Dec 10, 2019 at 2:47 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> >
> > Both MT8183 & MT6765 have more control steps of bus protection
> > than previous project. And there add more bus protection registers
> > reside at infracfg & smi-common. Also add new APIs for multiple
> > step bus protection control with more customized arguments.
> >
> > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > ---
> >  drivers/soc/mediatek/Makefile           |  2 +-
> >  drivers/soc/mediatek/mtk-scpsys-ext.c   | 99 +++++++++++++++++++++++++++++++++
> >  drivers/soc/mediatek/mtk-scpsys.c       | 39 +++++++++----
> >  include/linux/soc/mediatek/scpsys-ext.h | 39 +++++++++++++
> >  4 files changed, 168 insertions(+), 11 deletions(-)
> >  create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
> >  create mode 100644 include/linux/soc/mediatek/scpsys-ext.h
> >
> > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> > index b017330..b442be9 100644
> > --- a/drivers/soc/mediatek/Makefile
> > +++ b/drivers/soc/mediatek/Makefile
> > @@ -1,5 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> > -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
> >  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> >  obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> > diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
> > new file mode 100644
> > index 0000000..4f1adda
> > --- /dev/null
> > +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Owen Chen <Owen.Chen@mediatek.com>
> > + */
> > +#include <linux/ktime.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/soc/mediatek/scpsys-ext.h>
> > +
> > +#define MTK_POLL_DELAY_US   10
> > +#define MTK_POLL_TIMEOUT    USEC_PER_SEC
> > +
> > +static int set_bus_protection(struct regmap *map, u32 mask, u32 ack_mask,
> > +               u32 reg_set, u32 reg_sta, u32 reg_en)
> > +{
> > +       u32 val;
> > +
> > +       if (reg_set)
> > +               regmap_write(map, reg_set, mask);
> > +       else
> > +               regmap_update_bits(map, reg_en, mask, mask);
> 
> At least for 8183, we never seen to use the reg_set case, can we
> simplify this function?
> 

Actually 6765 will use it and all the other MediaTek chips at least in
near future.
https://patchwork.kernel.org/patch/11042003/

> > +
> > +       return regmap_read_poll_timeout(map, reg_sta,
> > +                       val, (val & ack_mask) == ack_mask,
> > +                       MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> 
> From 8183, I see that you have either:
>  1. mask == ack_mask
>  2. ack_mask == 0 (essentially this skips this test)
> 
> Would it be simpler to just skip this test if reg_sta == 0, and always
> assume mask == ack_mask otherwise?
> 
> e.g.
> if (reg_sta == 0)
>    return 0;
> 
> return regmap_read_poll_timeout(map, reg_sta,
>                        val, (val & mask) == mask,
>                        MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> 

I'm not sure if you mean ack_mask == 0?
reg_sta would be possible to be 0 because it's a register address
offset.

I guess what you'd actually suggest is like below?

if (ack_mask == 0)
    return 0;
 
return regmap_read_poll_timeout(map, reg_sta,
                       val, (val & mask) == mask,
                       MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);


> > +}
> > +
> > [snip]
> > +
> > +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
> > +       struct regmap *infracfg, struct regmap *smi_common)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < MAX_STEPS; i++) {
> > +               struct regmap *map = NULL;
> > +               int ret;
> > +
> > +               if (bp_table[i].type == INVALID_TYPE)
> > +                       continue;
> 
> break? (but yes the one below in mtk_scpsys_ext_clear_bus_protection
> has to be continue).
> 

Thanks. I'll fix in next version.

> > +               else if (bp_table[i].type == IFR_TYPE)
> > +                       map = infracfg;
> > +               else if (bp_table[i].type == SMI_TYPE)
> > +                       map = smi_common;
> > +
> > +               ret = set_bus_protection(map,
> > +                               bp_table[i].mask, bp_table[i].mask,
> > +                               bp_table[i].set_ofs, bp_table[i].sta_ofs,
> > +                               bp_table[i].en_ofs);
> > +
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
> > +       struct regmap *infracfg, struct regmap *smi_common)
> > +{
> > +       int i;
> > +
> > +       for (i = MAX_STEPS - 1; i >= 0; i--) {
> > +               struct regmap *map = NULL;
> > +               int ret;
> > +
> > +               if (bp_table[i].type == INVALID_TYPE)
> > +                       continue;
> > +               else if (bp_table[i].type == IFR_TYPE)
> > +                       map = infracfg;
> > +               else if (bp_table[i].type == SMI_TYPE)
> > +                       map = smi_common;
> > +
> > +               ret = clear_bus_protection(map,
> > +                               bp_table[i].mask, bp_table[i].clr_ack_mask,
> > +                               bp_table[i].clr_ofs, bp_table[i].sta_ofs,
> > +                               bp_table[i].en_ofs);
> > +
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> > index 915d635..466bb749 100644
> > --- a/drivers/soc/mediatek/mtk-scpsys.c
> > +++ b/drivers/soc/mediatek/mtk-scpsys.c
> > @@ -12,6 +12,7 @@
> >  #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>
> >  #include <dt-bindings/power/mt2712-power.h>
> > @@ -120,6 +121,7 @@ enum clk_id {
> >   * @basic_clk_id: provide the same purpose with field "clk_id"
> >   *                by declaring basic clock prefix name rather than clk_id.
> >   * @caps: The flag for active wake-up action.
> > + * @bp_table: The mask table for multiple step bus protection.
> >   */
> >  struct scp_domain_data {
> >         const char *name;
> > @@ -131,6 +133,7 @@ struct scp_domain_data {
> >         enum clk_id clk_id[MAX_CLKS];
> >         const char *basic_clk_id[MAX_CLKS];
> >         u8 caps;
> > +       struct bus_prot bp_table[MAX_STEPS];
> 
> As with the previous patch, I'm not a big fan of having 2 approaches
> for something similar (bus_prot_mask vs bp_table), can we define a
> simple macro for this?
> e.g.:
> .bp_table = BUS_PROT_SINGLE(mask)

Agree! I'll fix it.
Nicolas Boichat Dec. 17, 2019, 5:33 a.m. UTC | #3 
On Tue, Dec 17, 2019 at 10:51 AM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
>
> On Mon, 2019-12-16 at 15:21 +0800, Nicolas Boichat wrote:
> > On Tue, Dec 10, 2019 at 2:47 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> > >
> > > Both MT8183 & MT6765 have more control steps of bus protection
> > > than previous project. And there add more bus protection registers
> > > reside at infracfg & smi-common. Also add new APIs for multiple
> > > step bus protection control with more customized arguments.
> > >
> > > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > > ---
> > >  drivers/soc/mediatek/Makefile           |  2 +-
> > >  drivers/soc/mediatek/mtk-scpsys-ext.c   | 99 +++++++++++++++++++++++++++++++++
> > >  drivers/soc/mediatek/mtk-scpsys.c       | 39 +++++++++----
> > >  include/linux/soc/mediatek/scpsys-ext.h | 39 +++++++++++++
> > >  4 files changed, 168 insertions(+), 11 deletions(-)
> > >  create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
> > >  create mode 100644 include/linux/soc/mediatek/scpsys-ext.h
> > >
> > > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> > > index b017330..b442be9 100644
> > > --- a/drivers/soc/mediatek/Makefile
> > > +++ b/drivers/soc/mediatek/Makefile
> > > @@ -1,5 +1,5 @@
> > >  # SPDX-License-Identifier: GPL-2.0-only
> > >  obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> > > -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> > > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
> > >  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> > >  obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> > > diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
> > > new file mode 100644
> > > index 0000000..4f1adda
> > > --- /dev/null
> > > +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
> > > @@ -0,0 +1,99 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2018 MediaTek Inc.
> > > + * Author: Owen Chen <Owen.Chen@mediatek.com>
> > > + */
> > > +#include <linux/ktime.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/soc/mediatek/scpsys-ext.h>
> > > +
> > > +#define MTK_POLL_DELAY_US   10
> > > +#define MTK_POLL_TIMEOUT    USEC_PER_SEC
> > > +
> > > +static int set_bus_protection(struct regmap *map, u32 mask, u32 ack_mask,
> > > +               u32 reg_set, u32 reg_sta, u32 reg_en)
> > > +{
> > > +       u32 val;
> > > +
> > > +       if (reg_set)
> > > +               regmap_write(map, reg_set, mask);
> > > +       else
> > > +               regmap_update_bits(map, reg_en, mask, mask);
> >
> > At least for 8183, we never seen to use the reg_set case, can we
> > simplify this function?
> >
>
> Actually 6765 will use it and all the other MediaTek chips at least in
> near future.
> https://patchwork.kernel.org/patch/11042003/

Ok, that's fine then.

> > > +
> > > +       return regmap_read_poll_timeout(map, reg_sta,
> > > +                       val, (val & ack_mask) == ack_mask,
> > > +                       MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> >
> > From 8183, I see that you have either:
> >  1. mask == ack_mask
> >  2. ack_mask == 0 (essentially this skips this test)
> >
> > Would it be simpler to just skip this test if reg_sta == 0, and always
> > assume mask == ack_mask otherwise?
> >
> > e.g.
> > if (reg_sta == 0)
> >    return 0;
> >
> > return regmap_read_poll_timeout(map, reg_sta,
> >                        val, (val & mask) == mask,
> >                        MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> >
>
> I'm not sure if you mean ack_mask == 0?
> reg_sta would be possible to be 0 because it's a register address
> offset.

Right, so maybe "0" is not a good invalid value, or maybe you can have a
#define REG_STA_INVALID U32_MAX

And then test for:
if (reg_sta == REG_STA_INVALID)
   return 0;

My point here is that mask and ack_mask are always the same (unless
you don't care about reading back the status), so maybe you only need
to specify mask?

(but if you need different mask and ack_mask for future chips, feel
free to ignore)

> I guess what you'd actually suggest is like below?
>
> if (ack_mask == 0)
>     return 0;
>
> return regmap_read_poll_timeout(map, reg_sta,
>                        val, (val & mask) == mask,
>                        MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>
>
> > > +}
> > > +
> > > [snip]
> > > +
> > > +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
> > > +       struct regmap *infracfg, struct regmap *smi_common)
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = 0; i < MAX_STEPS; i++) {
> > > +               struct regmap *map = NULL;
> > > +               int ret;
> > > +
> > > +               if (bp_table[i].type == INVALID_TYPE)
> > > +                       continue;
> >
> > break? (but yes the one below in mtk_scpsys_ext_clear_bus_protection
> > has to be continue).
> >
>
> Thanks. I'll fix in next version.
>
> > > +               else if (bp_table[i].type == IFR_TYPE)
> > > +                       map = infracfg;
> > > +               else if (bp_table[i].type == SMI_TYPE)
> > > +                       map = smi_common;
> > > +
> > > +               ret = set_bus_protection(map,
> > > +                               bp_table[i].mask, bp_table[i].mask,
> > > +                               bp_table[i].set_ofs, bp_table[i].sta_ofs,
> > > +                               bp_table[i].en_ofs);
> > > +
> > > +               if (ret)
> > > +                       return ret;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
> > > +       struct regmap *infracfg, struct regmap *smi_common)
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = MAX_STEPS - 1; i >= 0; i--) {
> > > +               struct regmap *map = NULL;
> > > +               int ret;
> > > +
> > > +               if (bp_table[i].type == INVALID_TYPE)
> > > +                       continue;
> > > +               else if (bp_table[i].type == IFR_TYPE)
> > > +                       map = infracfg;
> > > +               else if (bp_table[i].type == SMI_TYPE)
> > > +                       map = smi_common;
> > > +
> > > +               ret = clear_bus_protection(map,
> > > +                               bp_table[i].mask, bp_table[i].clr_ack_mask,
> > > +                               bp_table[i].clr_ofs, bp_table[i].sta_ofs,
> > > +                               bp_table[i].en_ofs);
> > > +
> > > +               if (ret)
> > > +                       return ret;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> > > index 915d635..466bb749 100644
> > > --- a/drivers/soc/mediatek/mtk-scpsys.c
> > > +++ b/drivers/soc/mediatek/mtk-scpsys.c
> > > @@ -12,6 +12,7 @@
> > >  #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>
> > >  #include <dt-bindings/power/mt2712-power.h>
> > > @@ -120,6 +121,7 @@ enum clk_id {
> > >   * @basic_clk_id: provide the same purpose with field "clk_id"
> > >   *                by declaring basic clock prefix name rather than clk_id.
> > >   * @caps: The flag for active wake-up action.
> > > + * @bp_table: The mask table for multiple step bus protection.
> > >   */
> > >  struct scp_domain_data {
> > >         const char *name;
> > > @@ -131,6 +133,7 @@ struct scp_domain_data {
> > >         enum clk_id clk_id[MAX_CLKS];
> > >         const char *basic_clk_id[MAX_CLKS];
> > >         u8 caps;
> > > +       struct bus_prot bp_table[MAX_STEPS];
> >
> > As with the previous patch, I'm not a big fan of having 2 approaches
> > for something similar (bus_prot_mask vs bp_table), can we define a
> > simple macro for this?
> > e.g.:
> > .bp_table = BUS_PROT_SINGLE(mask)
>
> Agree! I'll fix it.
>
>
Weiyi Lu Dec. 18, 2019, 2:25 a.m. UTC | #4 
On Tue, 2019-12-17 at 13:33 +0800, Nicolas Boichat wrote:
> On Tue, Dec 17, 2019 at 10:51 AM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> >
> > On Mon, 2019-12-16 at 15:21 +0800, Nicolas Boichat wrote:
> > > On Tue, Dec 10, 2019 at 2:47 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> > > >
> > > > Both MT8183 & MT6765 have more control steps of bus protection
> > > > than previous project. And there add more bus protection registers
> > > > reside at infracfg & smi-common. Also add new APIs for multiple
> > > > step bus protection control with more customized arguments.
> > > >
> > > > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > > > ---
> > > >  drivers/soc/mediatek/Makefile           |  2 +-
> > > >  drivers/soc/mediatek/mtk-scpsys-ext.c   | 99 +++++++++++++++++++++++++++++++++
> > > >  drivers/soc/mediatek/mtk-scpsys.c       | 39 +++++++++----
> > > >  include/linux/soc/mediatek/scpsys-ext.h | 39 +++++++++++++
> > > >  4 files changed, 168 insertions(+), 11 deletions(-)
> > > >  create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
> > > >  create mode 100644 include/linux/soc/mediatek/scpsys-ext.h
> > > >
> > > > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> > > > index b017330..b442be9 100644
> > > > --- a/drivers/soc/mediatek/Makefile
> > > > +++ b/drivers/soc/mediatek/Makefile
> > > > @@ -1,5 +1,5 @@
> > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > >  obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> > > > -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> > > > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
> > > >  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> > > >  obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> > > > diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
> > > > new file mode 100644
> > > > index 0000000..4f1adda
> > > > --- /dev/null
> > > > +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
> > > > @@ -0,0 +1,99 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (c) 2018 MediaTek Inc.
> > > > + * Author: Owen Chen <Owen.Chen@mediatek.com>
> > > > + */
> > > > +#include <linux/ktime.h>
> > > > +#include <linux/mfd/syscon.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/soc/mediatek/scpsys-ext.h>
> > > > +
> > > > +#define MTK_POLL_DELAY_US   10
> > > > +#define MTK_POLL_TIMEOUT    USEC_PER_SEC
> > > > +
> > > > +static int set_bus_protection(struct regmap *map, u32 mask, u32 ack_mask,
> > > > +               u32 reg_set, u32 reg_sta, u32 reg_en)
> > > > +{
> > > > +       u32 val;
> > > > +
> > > > +       if (reg_set)
> > > > +               regmap_write(map, reg_set, mask);
> > > > +       else
> > > > +               regmap_update_bits(map, reg_en, mask, mask);
> > >
> > > At least for 8183, we never seen to use the reg_set case, can we
> > > simplify this function?
> > >
> >
> > Actually 6765 will use it and all the other MediaTek chips at least in
> > near future.
> > https://patchwork.kernel.org/patch/11042003/
> 
> Ok, that's fine then.
> 
> > > > +
> > > > +       return regmap_read_poll_timeout(map, reg_sta,
> > > > +                       val, (val & ack_mask) == ack_mask,
> > > > +                       MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > >
> > > From 8183, I see that you have either:
> > >  1. mask == ack_mask
> > >  2. ack_mask == 0 (essentially this skips this test)
> > >
> > > Would it be simpler to just skip this test if reg_sta == 0, and always
> > > assume mask == ack_mask otherwise?
> > >
> > > e.g.
> > > if (reg_sta == 0)
> > >    return 0;
> > >
> > > return regmap_read_poll_timeout(map, reg_sta,
> > >                        val, (val & mask) == mask,
> > >                        MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > >
> >
> > I'm not sure if you mean ack_mask == 0?
> > reg_sta would be possible to be 0 because it's a register address
> > offset.
> 
> Right, so maybe "0" is not a good invalid value, or maybe you can have a
> #define REG_STA_INVALID U32_MAX
> 
> And then test for:
> if (reg_sta == REG_STA_INVALID)
>    return 0;
> 
> My point here is that mask and ack_mask are always the same (unless
> you don't care about reading back the status), so maybe you only need
> to specify mask?
> 
> (but if you need different mask and ack_mask for future chips, feel
> free to ignore)
> 

I do need different mask and ack_mask.
For the special case of 8183 here, we still have to check the ack status
when setting bus protection but only ignore the ack status when clearing
bus protection.

case A.
SET: reg_set, reg_sta, mask == ack mask
CLEAR: reg_clr, reg_sta, mask == clear ack mask

case B. (the special case we discussed)
SET: reg_set, reg_sta, mask == ack mask
CLEAR: reg_clr, reg_sta, mask != clear ack mask(which is 0 now)

If I use the REG_STA_INVALID to replace the clear ack mask way.
I might need two reg_sta to satisfy the check of set and clear bus
protection.
One is valid for setting case and another is invalid for clearing case,
e.g. case 2 below

case 1.
SET: reg_set, reg_sta(valid), mask == ack mask
CLEAR: reg_clr, reg_sta(valid), mask == ack mask

case 2.
SET: reg_set, reg_sta(valid), mask == ack mask
CLEAR: reg_clr, reg_sta(invalid then return), mask == ack mask

so I'd like to keep the original proposal. What do you think?

> > I guess what you'd actually suggest is like below?
> >
> > if (ack_mask == 0)
> >     return 0;
> >
> > return regmap_read_poll_timeout(map, reg_sta,
> >                        val, (val & mask) == mask,
> >                        MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> >
> >
> > > > +}
> > > > +
> > > > [snip]
> > > > +
> > > > +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
> > > > +       struct regmap *infracfg, struct regmap *smi_common)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < MAX_STEPS; i++) {
> > > > +               struct regmap *map = NULL;
> > > > +               int ret;
> > > > +
> > > > +               if (bp_table[i].type == INVALID_TYPE)
> > > > +                       continue;
> > >
> > > break? (but yes the one below in mtk_scpsys_ext_clear_bus_protection
> > > has to be continue).
> > >
> >
> > Thanks. I'll fix in next version.
> >
> > > > +               else if (bp_table[i].type == IFR_TYPE)
> > > > +                       map = infracfg;
> > > > +               else if (bp_table[i].type == SMI_TYPE)
> > > > +                       map = smi_common;
> > > > +
> > > > +               ret = set_bus_protection(map,
> > > > +                               bp_table[i].mask, bp_table[i].mask,
> > > > +                               bp_table[i].set_ofs, bp_table[i].sta_ofs,
> > > > +                               bp_table[i].en_ofs);
> > > > +
> > > > +               if (ret)
> > > > +                       return ret;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
> > > > +       struct regmap *infracfg, struct regmap *smi_common)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       for (i = MAX_STEPS - 1; i >= 0; i--) {
> > > > +               struct regmap *map = NULL;
> > > > +               int ret;
> > > > +
> > > > +               if (bp_table[i].type == INVALID_TYPE)
> > > > +                       continue;
> > > > +               else if (bp_table[i].type == IFR_TYPE)
> > > > +                       map = infracfg;
> > > > +               else if (bp_table[i].type == SMI_TYPE)
> > > > +                       map = smi_common;
> > > > +
> > > > +               ret = clear_bus_protection(map,
> > > > +                               bp_table[i].mask, bp_table[i].clr_ack_mask,
> > > > +                               bp_table[i].clr_ofs, bp_table[i].sta_ofs,
> > > > +                               bp_table[i].en_ofs);
> > > > +
> > > > +               if (ret)
> > > > +                       return ret;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> > > > index 915d635..466bb749 100644
> > > > --- a/drivers/soc/mediatek/mtk-scpsys.c
> > > > +++ b/drivers/soc/mediatek/mtk-scpsys.c
> > > > @@ -12,6 +12,7 @@
> > > >  #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>
> > > >  #include <dt-bindings/power/mt2712-power.h>
> > > > @@ -120,6 +121,7 @@ enum clk_id {
> > > >   * @basic_clk_id: provide the same purpose with field "clk_id"
> > > >   *                by declaring basic clock prefix name rather than clk_id.
> > > >   * @caps: The flag for active wake-up action.
> > > > + * @bp_table: The mask table for multiple step bus protection.
> > > >   */
> > > >  struct scp_domain_data {
> > > >         const char *name;
> > > > @@ -131,6 +133,7 @@ struct scp_domain_data {
> > > >         enum clk_id clk_id[MAX_CLKS];
> > > >         const char *basic_clk_id[MAX_CLKS];
> > > >         u8 caps;
> > > > +       struct bus_prot bp_table[MAX_STEPS];
> > >
> > > As with the previous patch, I'm not a big fan of having 2 approaches
> > > for something similar (bus_prot_mask vs bp_table), can we define a
> > > simple macro for this?
> > > e.g.:
> > > .bp_table = BUS_PROT_SINGLE(mask)
> >
> > Agree! I'll fix it.
> >
> >
Nicolas Boichat Dec. 19, 2019, 3:42 a.m. UTC | #5 
On Wed, Dec 18, 2019 at 10:25 AM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
>
> On Tue, 2019-12-17 at 13:33 +0800, Nicolas Boichat wrote:
> > On Tue, Dec 17, 2019 at 10:51 AM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> > >
> > > On Mon, 2019-12-16 at 15:21 +0800, Nicolas Boichat wrote:
> > > > On Tue, Dec 10, 2019 at 2:47 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> > > > >
> > > > > Both MT8183 & MT6765 have more control steps of bus protection
> > > > > than previous project. And there add more bus protection registers
> > > > > reside at infracfg & smi-common. Also add new APIs for multiple
> > > > > step bus protection control with more customized arguments.
> > > > >
> > > > > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > > > > ---
> > > > >  drivers/soc/mediatek/Makefile           |  2 +-
> > > > >  drivers/soc/mediatek/mtk-scpsys-ext.c   | 99 +++++++++++++++++++++++++++++++++
> > > > >  drivers/soc/mediatek/mtk-scpsys.c       | 39 +++++++++----
> > > > >  include/linux/soc/mediatek/scpsys-ext.h | 39 +++++++++++++
> > > > >  4 files changed, 168 insertions(+), 11 deletions(-)
> > > > >  create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
> > > > >  create mode 100644 include/linux/soc/mediatek/scpsys-ext.h
> > > > >
> > > > > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> > > > > index b017330..b442be9 100644
> > > > > --- a/drivers/soc/mediatek/Makefile
> > > > > +++ b/drivers/soc/mediatek/Makefile
> > > > > @@ -1,5 +1,5 @@
> > > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > > >  obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> > > > > -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> > > > > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
> > > > >  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> > > > >  obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> > > > > diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
> > > > > new file mode 100644
> > > > > index 0000000..4f1adda
> > > > > --- /dev/null
> > > > > +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
> > > > > @@ -0,0 +1,99 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (c) 2018 MediaTek Inc.
> > > > > + * Author: Owen Chen <Owen.Chen@mediatek.com>
> > > > > + */
> > > > > +#include <linux/ktime.h>
> > > > > +#include <linux/mfd/syscon.h>
> > > > > +#include <linux/of_device.h>
> > > > > +#include <linux/regmap.h>
> > > > > +#include <linux/soc/mediatek/scpsys-ext.h>
> > > > > +
> > > > > +#define MTK_POLL_DELAY_US   10
> > > > > +#define MTK_POLL_TIMEOUT    USEC_PER_SEC
> > > > > +
> > > > > +static int set_bus_protection(struct regmap *map, u32 mask, u32 ack_mask,
> > > > > +               u32 reg_set, u32 reg_sta, u32 reg_en)
> > > > > +{
> > > > > +       u32 val;
> > > > > +
> > > > > +       if (reg_set)
> > > > > +               regmap_write(map, reg_set, mask);
> > > > > +       else
> > > > > +               regmap_update_bits(map, reg_en, mask, mask);
> > > >
> > > > At least for 8183, we never seen to use the reg_set case, can we
> > > > simplify this function?
> > > >
> > >
> > > Actually 6765 will use it and all the other MediaTek chips at least in
> > > near future.
> > > https://patchwork.kernel.org/patch/11042003/
> >
> > Ok, that's fine then.
> >
> > > > > +
> > > > > +       return regmap_read_poll_timeout(map, reg_sta,
> > > > > +                       val, (val & ack_mask) == ack_mask,
> > > > > +                       MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > > >
> > > > From 8183, I see that you have either:
> > > >  1. mask == ack_mask
> > > >  2. ack_mask == 0 (essentially this skips this test)
> > > >
> > > > Would it be simpler to just skip this test if reg_sta == 0, and always
> > > > assume mask == ack_mask otherwise?
> > > >
> > > > e.g.
> > > > if (reg_sta == 0)
> > > >    return 0;
> > > >
> > > > return regmap_read_poll_timeout(map, reg_sta,
> > > >                        val, (val & mask) == mask,
> > > >                        MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > > >
> > >
> > > I'm not sure if you mean ack_mask == 0?
> > > reg_sta would be possible to be 0 because it's a register address
> > > offset.
> >
> > Right, so maybe "0" is not a good invalid value, or maybe you can have a
> > #define REG_STA_INVALID U32_MAX
> >
> > And then test for:
> > if (reg_sta == REG_STA_INVALID)
> >    return 0;
> >
> > My point here is that mask and ack_mask are always the same (unless
> > you don't care about reading back the status), so maybe you only need
> > to specify mask?
> >
> > (but if you need different mask and ack_mask for future chips, feel
> > free to ignore)
> >
>
> I do need different mask and ack_mask.
> For the special case of 8183 here, we still have to check the ack status
> when setting bus protection but only ignore the ack status when clearing
> bus protection.
>
> case A.
> SET: reg_set, reg_sta, mask == ack mask
> CLEAR: reg_clr, reg_sta, mask == clear ack mask
>
> case B. (the special case we discussed)
> SET: reg_set, reg_sta, mask == ack mask
> CLEAR: reg_clr, reg_sta, mask != clear ack mask(which is 0 now)
>
> If I use the REG_STA_INVALID to replace the clear ack mask way.
> I might need two reg_sta to satisfy the check of set and clear bus
> protection.
> One is valid for setting case and another is invalid for clearing case,
> e.g. case 2 below
>
> case 1.
> SET: reg_set, reg_sta(valid), mask == ack mask
> CLEAR: reg_clr, reg_sta(valid), mask == ack mask
>
> case 2.
> SET: reg_set, reg_sta(valid), mask == ack mask
> CLEAR: reg_clr, reg_sta(invalid then return), mask == ack mask
>
> so I'd like to keep the original proposal. What do you think?

Ooh, I see. This is quite confusing to be honest... I wonder if it
wouldn't be clearer to have a boolean instead in bus_prot (something
like "clk_no_check", can't find a better name right now).
diff mbox series

Patch

diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
index b017330..b442be9 100644
--- a/drivers/soc/mediatek/Makefile
+++ b/drivers/soc/mediatek/Makefile
@@ -1,5 +1,5 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
-obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
+obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
 obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
 obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
new file mode 100644
index 0000000..4f1adda
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
@@ -0,0 +1,99 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ * Author: Owen Chen <Owen.Chen@mediatek.com>
+ */
+#include <linux/ktime.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/soc/mediatek/scpsys-ext.h>
+
+#define MTK_POLL_DELAY_US   10
+#define MTK_POLL_TIMEOUT    USEC_PER_SEC
+
+static int set_bus_protection(struct regmap *map, u32 mask, u32 ack_mask,
+		u32 reg_set, u32 reg_sta, u32 reg_en)
+{
+	u32 val;
+
+	if (reg_set)
+		regmap_write(map, reg_set, mask);
+	else
+		regmap_update_bits(map, reg_en, mask, mask);
+
+	return regmap_read_poll_timeout(map, reg_sta,
+			val, (val & ack_mask) == ack_mask,
+			MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
+}
+
+static int clear_bus_protection(struct regmap *map, u32 mask, u32 ack_mask,
+		u32 reg_clr, u32 reg_sta, u32 reg_en)
+{
+	u32 val;
+
+	if (reg_clr)
+		regmap_write(map, reg_clr, mask);
+	else
+		regmap_update_bits(map, reg_en, mask, 0);
+
+	return regmap_read_poll_timeout(map, reg_sta,
+			val, !(val & ack_mask),
+			MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
+}
+
+int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
+	struct regmap *infracfg, struct regmap *smi_common)
+{
+	int i;
+
+	for (i = 0; i < MAX_STEPS; i++) {
+		struct regmap *map = NULL;
+		int ret;
+
+		if (bp_table[i].type == INVALID_TYPE)
+			continue;
+		else if (bp_table[i].type == IFR_TYPE)
+			map = infracfg;
+		else if (bp_table[i].type == SMI_TYPE)
+			map = smi_common;
+
+		ret = set_bus_protection(map,
+				bp_table[i].mask, bp_table[i].mask,
+				bp_table[i].set_ofs, bp_table[i].sta_ofs,
+				bp_table[i].en_ofs);
+
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
+	struct regmap *infracfg, struct regmap *smi_common)
+{
+	int i;
+
+	for (i = MAX_STEPS - 1; i >= 0; i--) {
+		struct regmap *map = NULL;
+		int ret;
+
+		if (bp_table[i].type == INVALID_TYPE)
+			continue;
+		else if (bp_table[i].type == IFR_TYPE)
+			map = infracfg;
+		else if (bp_table[i].type == SMI_TYPE)
+			map = smi_common;
+
+		ret = clear_bus_protection(map,
+				bp_table[i].mask, bp_table[i].clr_ack_mask,
+				bp_table[i].clr_ofs, bp_table[i].sta_ofs,
+				bp_table[i].en_ofs);
+
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 915d635..466bb749 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -12,6 +12,7 @@ 
 #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>
 #include <dt-bindings/power/mt2712-power.h>
@@ -120,6 +121,7 @@  enum clk_id {
  * @basic_clk_id: provide the same purpose with field "clk_id"
  *                by declaring basic clock prefix name rather than clk_id.
  * @caps: The flag for active wake-up action.
+ * @bp_table: The mask table for multiple step bus protection.
  */
 struct scp_domain_data {
 	const char *name;
@@ -131,6 +133,7 @@  struct scp_domain_data {
 	enum clk_id clk_id[MAX_CLKS];
 	const char *basic_clk_id[MAX_CLKS];
 	u8 caps;
+	struct bus_prot bp_table[MAX_STEPS];
 };
 
 struct scp;
@@ -154,6 +157,7 @@  struct scp {
 	struct device *dev;
 	void __iomem *base;
 	struct regmap *infracfg;
+	struct regmap *smi_common;
 	struct scp_ctrl_reg ctrl_reg;
 	bool bus_prot_reg_update;
 };
@@ -283,24 +287,28 @@  static int scpsys_bus_protect_enable(struct scp_domain *scpd)
 {
 	struct scp *scp = scpd->scp;
 
-	if (!scpd->data->bus_prot_mask)
-		return 0;
+	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_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);
 }
 
 static int scpsys_bus_protect_disable(struct scp_domain *scpd)
 {
 	struct scp *scp = scpd->scp;
 
-	if (!scpd->data->bus_prot_mask)
-		return 0;
+	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_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);
 }
 
 static int scpsys_power_on(struct generic_pm_domain *genpd)
@@ -469,6 +477,17 @@  static struct scp *init_scp(struct platform_device *pdev,
 		return ERR_CAST(scp->infracfg);
 	}
 
+	scp->smi_common = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+			"smi_comm");
+
+	if (scp->smi_common == ERR_PTR(-ENODEV)) {
+		scp->smi_common = NULL;
+	} else if (IS_ERR(scp->smi_common)) {
+		dev_err(&pdev->dev, "Cannot find smi_common controller: %ld\n",
+				PTR_ERR(scp->smi_common));
+		return ERR_CAST(scp->smi_common);
+	}
+
 	for (i = 0; i < num; i++) {
 		struct scp_domain *scpd = &scp->domains[i];
 		const struct scp_domain_data *data = &scp_domain_data[i];
diff --git a/include/linux/soc/mediatek/scpsys-ext.h b/include/linux/soc/mediatek/scpsys-ext.h
new file mode 100644
index 0000000..3e5b84d
--- /dev/null
+++ b/include/linux/soc/mediatek/scpsys-ext.h
@@ -0,0 +1,39 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SOC_MEDIATEK_SCPSYS_EXT_H
+#define __SOC_MEDIATEK_SCPSYS_EXT_H
+
+#define MAX_STEPS	4
+
+#define BUS_PROT(_type, _set_ofs, _clr_ofs,			\
+		_en_ofs, _sta_ofs, _mask, _clr_ack_mask) {	\
+		.type = _type,					\
+		.set_ofs = _set_ofs,				\
+		.clr_ofs = _clr_ofs,				\
+		.en_ofs = _en_ofs,				\
+		.sta_ofs = _sta_ofs,				\
+		.mask = _mask,					\
+		.clr_ack_mask = _clr_ack_mask,			\
+	}
+
+enum regmap_type {
+	INVALID_TYPE = 0,
+	IFR_TYPE,
+	SMI_TYPE,
+};
+
+struct bus_prot {
+	enum regmap_type type;
+	u32 set_ofs;
+	u32 clr_ofs;
+	u32 en_ofs;
+	u32 sta_ofs;
+	u32 mask;
+	u32 clr_ack_mask;
+};
+
+int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
+	struct regmap *infracfg, struct regmap *smi_common);
+int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
+	struct regmap *infracfg, struct regmap *smi_common);
+
+#endif /* __SOC_MEDIATEK_SCPSYS_EXT_H */