Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752282AbdHQK5w (ORCPT ); Thu, 17 Aug 2017 06:57:52 -0400 Received: from mail-wr0-f193.google.com ([209.85.128.193]:32809 "EHLO mail-wr0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751622AbdHQK5u (ORCPT ); Thu, 17 Aug 2017 06:57:50 -0400 Subject: Re: [PATCH v1 6/9] soc: mediatek: add bus protection extend API To: weiyi.lu@mediatek.com, Stephen Boyd Cc: James Liao , Fan Chen , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-clk@vger.kernel.org, srv_heupstream@mediatek.com References: <1502779370-30150-1-git-send-email-weiyi.lu@mediatek.com> <1502779370-30150-7-git-send-email-weiyi.lu@mediatek.com> From: Matthias Brugger Message-ID: Date: Thu, 17 Aug 2017 12:57:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1502779370-30150-7-git-send-email-weiyi.lu@mediatek.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8040 Lines: 242 On 08/15/2017 08:42 AM, weiyi.lu@mediatek.com wrote: > From: Weiyi Lu > > MT2712 add "set/clear" bus control register to each control register set > instead of providing only one "enable" control register, we could avoid > the read-modify-write racing by using extend API with such new design. > Also there exists the second bus control register set, we could not access > this setting via original API rather than the extend API. > By improving the mtk-infracfg bus protection implementation to > support set/clear bus protection control method and second > register set while power on/off scpsys. > > Signed-off-by: Weiyi Lu > --- > drivers/soc/mediatek/mtk-infracfg.c | 90 +++++++++++++++++++++++++++++++++-- > drivers/soc/mediatek/mtk-scpsys.c | 36 ++++++++++++-- > include/linux/soc/mediatek/infracfg.h | 14 +++++- > 3 files changed, 132 insertions(+), 8 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c > index dba3055..63898e4 100644 > --- a/drivers/soc/mediatek/mtk-infracfg.c > +++ b/drivers/soc/mediatek/mtk-infracfg.c > @@ -17,9 +17,6 @@ > #include > #include > > -#define INFRA_TOPAXI_PROTECTEN 0x0220 > -#define INFRA_TOPAXI_PROTECTSTA1 0x0228 > - > /** > * mtk_infracfg_set_bus_protection - enable bus protection > * @regmap: The infracfg regmap > @@ -89,3 +86,90 @@ int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask) > > return 0; > } > + > +/** > + * mtk_infracfg_set_bus_protection_ext - enable bus protection > + * @regmap: The infracfg regmap > + * @mask: The mask containing the protection bits to be enabled. > + * @reg_set: The register used to enable protection bits. > + * @reg_sta: The register used to check the protection bits are enabled. > + * @reg_en: The register used to enable protection bits when there doesn't > + * exist reg_set. > + * > + * This function enables the bus protection bits for disabled power > + * domains so that the system does not hang when some unit accesses the > + * bus while in power down. > + */ > +int mtk_infracfg_set_bus_protection_ext(struct regmap *infracfg, u32 mask, > + u32 reg_set, u32 reg_sta, u32 reg_en) There is no need to add a new function. The only differences are: 1. it passes the register offsets instead of assuming default ones 2. it checks if reg_set/reg_clr and enables/disable. We should change the original function instead of adding a new one. From what I see, we are all done if we provide a boolean which tells infracfg if it should use set/clear or update register. For now we don't have any user of INFRA_TOPAXI_PROTECTSTA3 (same for *PROTECTEN1*). Do you expect one in the near future? If so, we could pass a flags value instead of bool which points that out. It seems there is no need for now to pass the register value. Regards, Matthias > +{ > + unsigned long expired; > + u32 val; > + int ret; > + > + if (reg_set) > + regmap_write(infracfg, reg_set, mask); > + else > + regmap_update_bits(infracfg, reg_en, mask, mask); > + > + expired = jiffies + HZ; > + > + while (1) { > + ret = regmap_read(infracfg, reg_sta, &val); > + if (ret) > + return ret; > + > + if ((val & mask) == mask) > + break; > + > + cpu_relax(); > + if (time_after(jiffies, expired)) > + return -EIO; > + } > + > + return 0; > +} > + > +/** > + * mtk_infracfg_clear_bus_protection_ext - disable bus protection > + * @regmap: The infracfg regmap > + * @mask: The mask containing the protection bits to be disabled. > + * @reg_clr: The register used to disable protection bits. > + * @reg_sta: The register used to check the protection bits are disabled. > + * @reg_en: The register used to disable protection bits when there doesn't > + * exist reg_clr. > + * > + * This function disables the bus protection bits previously enabled with > + * mtk_infracfg_set_bus_protection. > + */ > + > +int mtk_infracfg_clear_bus_protection_ext(struct regmap *infracfg, u32 mask, > + u32 reg_clr, u32 reg_sta, u32 reg_en) > +{ > + unsigned long expired; > + int ret; > + > + if (reg_clr) > + regmap_write(infracfg, reg_clr, mask); > + else > + regmap_update_bits(infracfg, reg_en, mask, 0); > + > + expired = jiffies + HZ; > + > + while (1) { > + u32 val; > + > + ret = regmap_read(infracfg, reg_sta, &val); > + if (ret) > + return ret; > + > + if (!(val & mask)) > + break; > + > + cpu_relax(); > + if (time_after(jiffies, expired)) > + return -EIO; > + } > + > + return 0; > +} > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c > index ceb2cc4..b181ef2 100644 > --- a/drivers/soc/mediatek/mtk-scpsys.c > +++ b/drivers/soc/mediatek/mtk-scpsys.c > @@ -89,6 +89,13 @@ enum clk_id { > > #define MAX_CLKS 2 > > +struct bus_prot_ext { > + u32 set_ofs; > + u32 clr_ofs; > + u32 en_ofs; > + u32 sta_ofs; > +}; > + > struct scp_domain_data { > const char *name; > u32 sta_mask; > @@ -96,6 +103,7 @@ struct scp_domain_data { > u32 sram_pdn_bits; > u32 sram_pdn_ack_bits; > u32 bus_prot_mask; > + struct bus_prot_ext bp_ext; > enum clk_id clk_id[MAX_CLKS]; > bool active_wakeup; > }; > @@ -228,8 +236,18 @@ static int scpsys_power_on(struct generic_pm_domain *genpd) > } > > if (scpd->data->bus_prot_mask) { > - ret = mtk_infracfg_clear_bus_protection(scp->infracfg, > - scpd->data->bus_prot_mask); > + if (((scpd->data->bp_ext.set_ofs && scpd->data->bp_ext.clr_ofs) > + || scpd->data->bp_ext.en_ofs) > + && scpd->data->bp_ext.sta_ofs) > + ret = mtk_infracfg_clear_bus_protection_ext( > + scp->infracfg, > + scpd->data->bus_prot_mask, > + scpd->data->bp_ext.clr_ofs, > + scpd->data->bp_ext.sta_ofs, > + scpd->data->bp_ext.en_ofs); > + else > + ret = mtk_infracfg_clear_bus_protection(scp->infracfg, > + scpd->data->bus_prot_mask); > if (ret) > goto err_pwr_ack; > } > @@ -263,8 +281,18 @@ static int scpsys_power_off(struct generic_pm_domain *genpd) > int i; > > if (scpd->data->bus_prot_mask) { > - ret = mtk_infracfg_set_bus_protection(scp->infracfg, > - scpd->data->bus_prot_mask); > + if (((scpd->data->bp_ext.set_ofs && scpd->data->bp_ext.clr_ofs) > + || scpd->data->bp_ext.en_ofs) > + && scpd->data->bp_ext.sta_ofs) > + ret = mtk_infracfg_set_bus_protection_ext( > + scp->infracfg, > + scpd->data->bus_prot_mask, > + scpd->data->bp_ext.set_ofs, > + scpd->data->bp_ext.sta_ofs, > + scpd->data->bp_ext.en_ofs); > + else > + ret = mtk_infracfg_set_bus_protection(scp->infracfg, > + scpd->data->bus_prot_mask); > if (ret) > goto out; > } > diff --git a/include/linux/soc/mediatek/infracfg.h b/include/linux/soc/mediatek/infracfg.h > index a5714e9..d3fbb45 100644 > --- a/include/linux/soc/mediatek/infracfg.h > +++ b/include/linux/soc/mediatek/infracfg.h > @@ -1,6 +1,15 @@ > #ifndef __SOC_MEDIATEK_INFRACFG_H > #define __SOC_MEDIATEK_INFRACFG_H > > +#define INFRA_TOPAXI_PROTECTEN 0x0220 > +#define INFRA_TOPAXI_PROTECTSTA1 0x0228 > +#define INFRA_TOPAXI_PROTECTEN1 0x0250 > +#define INFRA_TOPAXI_PROTECTSTA3 0x0258 > +#define INFRA_TOPAXI_PROTECTEN_SET 0x0260 > +#define INFRA_TOPAXI_PROTECTEN_CLR 0x0264 > +#define INFRA_TOPAXI_PROTECTEN1_SET 0x0270 > +#define INFRA_TOPAXI_PROTECTEN1_CLR 0x0274 > + > #define MT8173_TOP_AXI_PROT_EN_MCI_M2 BIT(0) > #define MT8173_TOP_AXI_PROT_EN_MM_M0 BIT(1) > #define MT8173_TOP_AXI_PROT_EN_MM_M1 BIT(2) > @@ -22,5 +31,8 @@ > > int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask); > int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask); > - > +int mtk_infracfg_set_bus_protection_ext(struct regmap *infracfg, u32 mask, > + u32 reg_set, u32 reg_sta, u32 reg_en); > +int mtk_infracfg_clear_bus_protection_ext(struct regmap *infracfg, u32 mask, > + u32 reg_clr, u32 reg_sta, u32 reg_en); > #endif /* __SOC_MEDIATEK_INFRACFG_H */ >