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

From: Weiyi Lu
Date: Tue Dec 17 2019 - 21:25:43 EST

Next message: Chen-Yu Tsai: "Re: [PATCH 01/13] mmc: sunxi-mmc: convert to devm_platform_ioremap_resource"
Previous message: Jin Yao: "[PATCH v4 2/3] perf report: Support a new key to reload the browser"
In reply to: Nicolas Boichat: "Re: [PATCH v9 4/9] soc: mediatek: Add multiple step bus protection control"
Next in thread: Nicolas Boichat: "Re: [PATCH v9 4/9] soc: mediatek: Add multiple step bus protection control"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxx>
> > > > + */
> > > > +#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.
> >
> >

Next message: Chen-Yu Tsai: "Re: [PATCH 01/13] mmc: sunxi-mmc: convert to devm_platform_ioremap_resource"
Previous message: Jin Yao: "[PATCH v4 2/3] perf report: Support a new key to reload the browser"
In reply to: Nicolas Boichat: "Re: [PATCH v9 4/9] soc: mediatek: Add multiple step bus protection control"
Next in thread: Nicolas Boichat: "Re: [PATCH v9 4/9] soc: mediatek: Add multiple step bus protection control"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]