Re: [PATCH v9 4/9] soc: mediatek: Add multiple step bus protection control
From: Weiyi Lu <hidden>
Date: 2019-12-18 02:25:43
Also in:
linux-mediatek, lkml
On Tue, 2019-12-17 at 13:33 +0800, Nicolas Boichat wrote:
On Tue, Dec 17, 2019 at 10:51 AM Weiyi Lu [off-list ref] wrote:quoted
On Mon, 2019-12-16 at 15:21 +0800, Nicolas Boichat wrote:quoted
On Tue, Dec 10, 2019 at 2:47 PM Weiyi Lu [off-list ref] wrote:quoted
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 <redacted> --- 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.hdiff --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.odiff --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.quoted
quoted
quoted
+ + 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?
quoted
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);quoted
quoted
+} + [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.quoted
quoted
+ 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.
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel