[PATCH 6/9] soc: mediatek: extend bus protection API

Matthias Brugger matthias.bgg at gmail.com
Mon Oct 16 08:04:09 PDT 2017



On 10/16/2017 08:38 AM, Weiyi Lu wrote:
> On Tue, 2017-10-10 at 17:45 +0200, Matthias Brugger wrote:
>>
>> On 08/22/2017 12:28 PM, Weiyi Lu wrote:
>>> 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.
>>> By improving the mtk-infracfg bus protection implementation to
>>> support set/clear bus protection control method by IC configuration.
>>>
>>> Signed-off-by: Weiyi Lu <weiyi.lu at mediatek.com>
>>> ---
>>>    drivers/soc/mediatek/mtk-infracfg.c   | 32 ++++++++++----
>>>    drivers/soc/mediatek/mtk-scpsys.c     | 81 +++++++++++++++++++++++++++--------
>>>    include/linux/soc/mediatek/infracfg.h | 12 ++++--
>>>    3 files changed, 96 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c
>>> index dba3055..f55ceaa 100644
>>> --- a/drivers/soc/mediatek/mtk-infracfg.c
>>> +++ b/drivers/soc/mediatek/mtk-infracfg.c
>>> @@ -17,30 +17,35 @@
>>>    #include <linux/soc/mediatek/infracfg.h>
>>>    #include <asm/processor.h>
>>>    with different design
>>> -#define INFRA_TOPAXI_PROTECTEN		0x0220
>>> -#define INFRA_TOPAXI_PROTECTSTA1	0x0228
>>> -
>>>    /**
>>>     * mtk_infracfg_set_bus_protection - 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_en: The register used to enable protection bits when there doesn't
>>> + *          exist reg_set.
>>> + * @reg_sta: The register used to check the protection bits are enabled.
>>>     *
>>>     * 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(struct regmap *infracfg, u32 mask)
>>> +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask,
>>> +		u32 reg_set, u32 reg_en, u32 reg_sta)
>>>    {
>>>    	unsigned long expired;
>>>    	u32 val;
>>>    	int ret;
>>>    
>>> -	regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, mask);
>>> +	if (reg_set)
>>> +		regmap_write(infracfg, reg_set, mask);
>>> +	else
>>> +		regmap_update_bits(infracfg, reg_en, mask, mask);
>>
>>
>> This looks to me as if it's slightly over-engineered.
>> The reg_set and reg_en and reg_sta are the same on all SoCs, so no need to pass
>> the value to the infra bus protection set and clear functions.
>> I think we could just add a boolean overwrite_mask for every SoC and check for this.
>>
>> Do I miss something?
>>
>> Regards,
>> Matthias
>>
> 
> Yes, currently those SoCs already merged in kernel mainline are the
> same, but I've checked some mediatek SoCs in near future might be
> different.That's the main reason I'd like to make it more flexible by
> passing the register address directly than just adding a boolean to
> check.Do you think it be better to refine like Part A below, or keep it
> simple like Part B only for those SoCs in mainline?
> 

Please use B for now. We can add something like A in the future when we need it.

Thanks!
Matthias


> A.
> struct overwrite_reg {
> 	u32 reg_en;
> 	u32 reg_set;
> 	u32 reg_clr;
> 	u32 reg_sta;
> };
> 
> int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask
> 	bool use_set_reg, bool use_overwrite_reg, struct overwrite_reg
> new_regs)
> {
> 	unsigned long expired;
> 	u32 val;
> 	int ret;
> 	u32 reg_en, reg_set, reg_sta;
> 
> 	if (use_overwrite_reg) {
> 		reg_en = new_regs.reg_en;
> 		reg_set = new_regs.reg_set;
> 		reg_sta = new_regs.reg_sta;
> 	} else {
> 		reg_en = INFRA_TOPAXI_PROTECTEN;
> 		reg_set = INFRA_TOPAXI_PROTECTEN_SET;
> 		reg_sta = INFRA_TOPAXI_PROTECTSTA1;
> 	}
> 		
> 	if (!use_set_reg)
> 		regmap_update_bits(infracfg, reg_en, mask, mask);
> 	else
> 		regmap_write(infracfg, reg_set, mask);
> 
> ...
> }
> 
> B.
> int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask
> 	bool use_set_reg)
> {
> 	unsigned long expired;
> 	u32 val;
> 	int ret;
> 		
> 	if (!use_set_reg)
> 		regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, mask);
> 	else
> 		regmap_write(infracfg, INFRA_TOPAXI_PROTECTEN_SET, mask);
> 
> ...
> }
> 
>>>    
>>>    	expired = jiffies + HZ;
>>>    
>>>    	while (1) {
>>> -		ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
>>> +		ret = regmap_read(infracfg, reg_sta, &val);
>>>    		if (ret)
>>>    			return ret;
>>>    
>>> @@ -59,23 +64,32 @@ int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask)
>>>     * mtk_infracfg_clear_bus_protection - 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_en: The register used to disable protection bits when there doesn't
>>> + *          exist reg_clr.
>>> + * @reg_sta: The register used to check the protection bits are disabled.
>>>     *
>>>     * This function disables the bus protection bits previously enabled with
>>>     * mtk_infracfg_set_bus_protection.
>>>     */
>>> -int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask)
>>> +
>>> +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask,
>>> +		u32 reg_clr, u32 reg_en, u32 reg_sta)
>>>    {
>>>    	unsigned long expired;
>>>    	int ret;
>>>    
>>> -	regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, 0);
>>> +	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, INFRA_TOPAXI_PROTECTSTA1, &val);
>>> +		ret = regmap_read(infracfg, reg_sta, &val);
>>>    		if (ret)
>>>    			return ret;
>>>    
>>> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
>>> index e1ce8b1..2569390 100644
>>> --- a/drivers/soc/mediatek/mtk-scpsys.c
>>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
>>> @@ -127,13 +127,25 @@ struct scp_ctrl_reg {
>>>    	int pwr_sta2nd_offs;
>>>    };
>>>    
>>> +struct bus_prot_reg {
>>> +	u32 set_offs;
>>> +	u32 clr_offs;
>>> +	u32 en_offs;
>>> +	u32 sta_offs;
>>> +};
>>> +
>>> +struct soc_reg {
>>> +	struct scp_ctrl_reg scp_ctrl;
>>> +	struct bus_prot_reg bus_prot;
>>> +};
>>> +
>>>    struct scp {
>>>    	struct scp_domain *domains;
>>>    	struct genpd_onecell_data pd_data;
>>>    	struct device *dev;
>>>    	void __iomem *base;
>>>    	struct regmap *infracfg;
>>> -	struct scp_ctrl_reg ctrl_reg;
>>> +	struct soc_reg soc_reg;
>>>    };
>>>    
>>>    struct scp_subdomain {
>>> @@ -146,16 +158,16 @@ struct scp_soc_data {
>>>    	int num_domains;
>>>    	const struct scp_subdomain *subdomains;
>>>    	int num_subdomains;
>>> -	const struct scp_ctrl_reg regs;
>>> +	const struct soc_reg regs;
>>>    };
>>>    
>>>    static int scpsys_domain_is_on(struct scp_domain *scpd)
>>>    {
>>>    	struct scp *scp = scpd->scp;
>>>    
>>> -	u32 status = readl(scp->base + scp->ctrl_reg.pwr_sta_offs) &
>>> +	u32 status = readl(scp->base + scp->soc_reg.scp_ctrl.pwr_sta_offs) &
>>>    						scpd->data->sta_mask;
>>> -	u32 status2 = readl(scp->base + scp->ctrl_reg.pwr_sta2nd_offs) &
>>> +	u32 status2 = readl(scp->base + scp->soc_reg.scp_ctrl.pwr_sta2nd_offs) &
>>>    						scpd->data->sta_mask;
>>>    
>>>    	/*
>>> @@ -254,7 +266,10 @@ 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);
>>> +				scpd->data->bus_prot_mask,
>>> +				scp->soc_reg.bus_prot.clr_offs,
>>> +				scp->soc_reg.bus_prot.en_offs,
>>> +				scp->soc_reg.bus_prot.sta_offs);
>>>    		if (ret)
>>>    			goto err_pwr_ack;
>>>    	}
>>> @@ -289,7 +304,10 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>>>    
>>>    	if (scpd->data->bus_prot_mask) {
>>>    		ret = mtk_infracfg_set_bus_protection(scp->infracfg,
>>> -				scpd->data->bus_prot_mask);
>>> +				scpd->data->bus_prot_mask,
>>> +				scp->soc_reg.bus_prot.set_offs,
>>> +				scp->soc_reg.bus_prot.en_offs,
>>> +				scp->soc_reg.bus_prot.sta_offs);
>>>    		if (ret)
>>>    			goto out;
>>>    	}
>>> @@ -382,7 +400,7 @@ static void init_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)
>>> +			const struct soc_reg *soc_reg)
>>>    {
>>>    	struct genpd_onecell_data *pd_data;
>>>    	struct resource *res;
>>> @@ -394,8 +412,13 @@ static struct scp *init_scp(struct platform_device *pdev,
>>>    	if (!scp)
>>>    		return ERR_PTR(-ENOMEM);
>>>    
>>> -	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->soc_reg.scp_ctrl.pwr_sta_offs = soc_reg->scp_ctrl.pwr_sta_offs;
>>> +	scp->soc_reg.scp_ctrl.pwr_sta2nd_offs =
>>> +			soc_reg->scp_ctrl.pwr_sta2nd_offs;
>>> +	scp->soc_reg.bus_prot.set_offs = soc_reg->bus_prot.set_offs;
>>> +	scp->soc_reg.bus_prot.clr_offs = soc_reg->bus_prot.clr_offs;
>>> +	scp->soc_reg.bus_prot.en_offs = soc_reg->bus_prot.en_offs;
>>> +	scp->soc_reg.bus_prot.sta_offs = soc_reg->bus_prot.sta_offs;
>>>    
>>>    	scp->dev = &pdev->dev;
>>>    
>>> @@ -814,8 +837,14 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>>    	.domains = scp_domain_data_mt2701,
>>>    	.num_domains = ARRAY_SIZE(scp_domain_data_mt2701),
>>>    	.regs = {
>>> -		.pwr_sta_offs = SPM_PWR_STATUS,
>>> -		.pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
>>> +		.scp_ctrl = {
>>> +			.pwr_sta_offs = SPM_PWR_STATUS,
>>> +			.pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
>>> +		},
>>> +		.bus_prot = {
>>> +			.en_offs = INFRA_TOPAXI_PROTECTEN,
>>> +			.sta_offs = INFRA_TOPAXI_PROTECTSTA1
>>> +		},
>>>    	}
>>>    };
>>>    
>>> @@ -825,8 +854,14 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>>    	.subdomains = scp_subdomain_mt6797,
>>>    	.num_subdomains = ARRAY_SIZE(scp_subdomain_mt6797),
>>>    	.regs = {
>>> -		.pwr_sta_offs = SPM_PWR_STATUS_MT6797,
>>> -		.pwr_sta2nd_offs = SPM_PWR_STATUS_2ND_MT6797
>>> +		.scp_ctrl = {
>>> +			.pwr_sta_offs = SPM_PWR_STATUS_MT6797,
>>> +			.pwr_sta2nd_offs = SPM_PWR_STATUS_2ND_MT6797
>>> +		},
>>> +		.bus_prot = {
>>> +			.en_offs = INFRA_TOPAXI_PROTECTEN,
>>> +			.sta_offs = INFRA_TOPAXI_PROTECTSTA1
>>> +		},
>>>    	}
>>>    };
>>>    
>>> @@ -834,8 +869,14 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>>    	.domains = scp_domain_data_mt7622,
>>>    	.num_domains = ARRAY_SIZE(scp_domain_data_mt7622),
>>>    	.regs = {
>>> -		.pwr_sta_offs = SPM_PWR_STATUS,
>>> -		.pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
>>> +		.scp_ctrl = {
>>> +			.pwr_sta_offs = SPM_PWR_STATUS,
>>> +			.pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
>>> +		},
>>> +		.bus_prot = {
>>> +			.en_offs = INFRA_TOPAXI_PROTECTEN,
>>> +			.sta_offs = INFRA_TOPAXI_PROTECTSTA1
>>> +		},
>>>    	}
>>>    };
>>>    
>>> @@ -845,8 +886,14 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>>    	.subdomains = scp_subdomain_mt8173,
>>>    	.num_subdomains = ARRAY_SIZE(scp_subdomain_mt8173),
>>>    	.regs = {
>>> -		.pwr_sta_offs = SPM_PWR_STATUS,
>>> -		.pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
>>> +		.scp_ctrl = {
>>> +			.pwr_sta_offs = SPM_PWR_STATUS,
>>> +			.pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
>>> +		},
>>> +		.bus_prot = {
>>> +			.en_offs = INFRA_TOPAXI_PROTECTEN,
>>> +			.sta_offs = INFRA_TOPAXI_PROTECTSTA1
>>> +		},
>>>    	}
>>>    };
>>>    
>>> diff --git a/include/linux/soc/mediatek/infracfg.h b/include/linux/soc/mediatek/infracfg.h
>>> index a0182ec..c704af5 100644
>>> --- a/include/linux/soc/mediatek/infracfg.h
>>> +++ b/include/linux/soc/mediatek/infracfg.h
>>> @@ -1,6 +1,11 @@
>>>    #ifndef __SOC_MEDIATEK_INFRACFG_H
>>>    #define __SOC_MEDIATEK_INFRACFG_H
>>>    
>>> +#define INFRA_TOPAXI_PROTECTEN		0x0220
>>> +#define INFRA_TOPAXI_PROTECTSTA1	0x0228
>>> +#define INFRA_TOPAXI_PROTECTEN_SET	0x0260
>>> +#define INFRA_TOPAXI_PROTECTEN_CLR	0x0264
>>
>> The last two a never used.
> 
> Currently, the last two would be used in Patch 8 like below, will be
> related by how bus protect API is implemented.
> +static const struct scp_soc_data mt2712_data = {
> +       .domains = scp_domain_data_mt2712,
> +       .num_domains = ARRAY_SIZE(scp_domain_data_mt2712),
> +       .regs = {
> +               .scp_ctrl = {
> +                       .pwr_sta_offs = SPM_PWR_STATUS,
> +                       .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> +               },
> +               .bus_prot = {
> +                       .set_offs = INFRA_TOPAXI_PROTECTEN_SET,
> +                       .clr_offs = INFRA_TOPAXI_PROTECTEN_CLR,
> +                       .sta_offs = INFRA_TOPAXI_PROTECTSTA1
> +               },
> +       }
> +};
>>
>>> +
>>>    #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)
>>> @@ -27,7 +32,8 @@
>>>    #define MT7622_TOP_AXI_PROT_EN_WB		(BIT(2) | BIT(6) | \
>>>    						 BIT(7) | BIT(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(struct regmap *infracfg, u32 mask,
>>> +		u32 reg_set, u32 reg_en, u32 reg_sta);
>>> +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask,
>>> +		u32 reg_clr, u32 reg_en, u32 reg_sta);
>>>    #endif /* __SOC_MEDIATEK_INFRACFG_H */
>>>
> 
> 



More information about the linux-arm-kernel mailing list