Re: [PATCH v3 2/4] soc: mediatek: Add support of WAY_EN operations

From: Matthias Brugger <matthias.bgg@gmail.com>
To: Markus Schneider-Pargmann <msp@baylibre.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Chun-Jie Chen <chun-jie.chen@mediatek.com>,
	AngeloGioacchino Del Regno 
	<angelogioacchino.delregno@collabora.com>,
	Fabien Parent <parent.f@gmail.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	Alexandre Bailon <abailon@baylibre.com>,
	Fabien Parent <fparent@baylibre.com>
Subject: Re: [PATCH v3 2/4] soc: mediatek: Add support of WAY_EN operations
Date: Tue, 13 Sep 2022 19:40:28 +0200	[thread overview]
Message-ID: <ef8215be-1b68-09b7-ca93-f32a0acd9d5c@gmail.com> (raw)
In-Reply-To: <20220906094947.ghfy2tj4bj4kaule@blmsp>

On 06/09/2022 11:49, Markus Schneider-Pargmann wrote:
> Hi Matthias,
> 
> On Mon, Aug 22, 2022 at 06:17:58PM +0200, Matthias Brugger wrote:
>>
>>
>> On 22/08/2022 16:43, Markus Schneider-Pargmann wrote:
>>> From: Alexandre Bailon <abailon@baylibre.com>
>>>
>>> This updates the power domain to support WAY_EN operations. These
>>> operations enable a path between different units of the chip and are
>>> labeled as 'way_en' in the register descriptions.

If you can come up with a more verbose description of WAY_EN functionality it 
would be appreciated, although I know it's not always easy to get that 
information. 10 years in it will help to understand better what this is about.

>>>
>>> This operation is required by the mt8365 for the MM power domain.
>>>
>>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>>> Signed-off-by: Fabien Parent <fparent@baylibre.com>
>>> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
>>> ---
>>>
>>> Notes:
>>>       Changes in v3:
>>>       - Separated the way_en functions for clarity
>>>       - Added some checks for infracfg_nao
>>>       Changes in v2:
>>>       - some minor style fixes.
>>>       - Renamed 'wayen' to 'way_en' to clarify the meaning
>>>       - Updated commit message
>>>
>>>    drivers/soc/mediatek/mtk-pm-domains.c | 162 +++++++++++++++++++++-----
>>>    drivers/soc/mediatek/mtk-pm-domains.h |  28 +++--
>>>    2 files changed, 149 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
>>> index 9734f1091c69..c2cbe0de6aa1 100644
>>> --- a/drivers/soc/mediatek/mtk-pm-domains.c
>>> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
>>> @@ -43,6 +43,7 @@ struct scpsys_domain {
>>>    	struct clk_bulk_data *clks;
>>>    	int num_subsys_clks;
>>>    	struct clk_bulk_data *subsys_clks;
>>> +	struct regmap *infracfg_nao;
>>>    	struct regmap *infracfg;
>>>    	struct regmap *smi;
>>>    	struct regulator *supply;
>>> @@ -117,26 +118,61 @@ static int scpsys_sram_disable(struct scpsys_domain *pd)
>>>    					MTK_POLL_TIMEOUT);
>>>    }
>>> -static int _scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd, struct regmap *regmap)
>>> +static int __scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd,
>>> +				       struct regmap *regmap)
>>> +{
>>> +	u32 val;
>>> +	u32 mask = bpd->bus_prot_mask;
>>> +	u32 sta_mask = bpd->bus_prot_sta_mask;
>>> +
>>> +	if (bpd->bus_prot_reg_update)
>>> +		regmap_set_bits(regmap, bpd->bus_prot_set, mask);
>>> +	else
>>> +		regmap_write(regmap, bpd->bus_prot_set, mask);
>>> +
>>> +	return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, val,
>>> +					(val & sta_mask) == sta_mask,
>>> +					MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> +}
>>> +
>>> +static int scpsys_bus_way_disable(const struct scpsys_bus_prot_data *bpd,
>>> +				  struct regmap *regmap,
>>> +				  struct regmap *ack_regmap)
>>> +{
>>> +	u32 val;
>>> +	u32 mask = bpd->bus_prot_mask;
>>> +	u32 sta_mask = bpd->bus_prot_sta_mask;
>>> +
>>> +	if (bpd->bus_prot_reg_update)
>>> +		regmap_clear_bits(regmap, bpd->bus_prot_set, mask);
>>> +	else
>>> +		regmap_write(regmap, bpd->bus_prot_set, mask);

BUS_PROT_WAY_EN sets bus_prot_reg_update to true, what do we need the else 
branch for?

>>> +
>>> +	if (bpd->ignore_clr_ack)
>>> +		return 0;

Same here, ignore_clr_ack is always false.

Actually it seems to me that __scpsys_bus_protect_enable and 
scpsys_bus_way_disable is nearly the same (other regmap, regmap_clear_bits 
instead of set_bits). Can't we put the check of way_en in e.g. the old 
_scpsys_bus_protect_enable to get rid of all the code duplication?

>>> +
>>> +	return regmap_read_poll_timeout(ack_regmap, bpd->bus_prot_sta, val,
>>> +					(val & sta_mask) == sta_mask,
>>> +					MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> +}
>>> +
>>> +static int _scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd,
>>> +				      struct regmap *regmap, struct regmap *infracfg_nao)
>>>    {
>>>    	int i, ret;
>>>    	for (i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
>>> -		u32 val, mask = bpd[i].bus_prot_mask;
>>> -
>>> -		if (!mask)
>>> +		if (!bpd[i].bus_prot_mask)
>>>    			break;
>>> -		if (bpd[i].bus_prot_reg_update)
>>> -			regmap_set_bits(regmap, bpd[i].bus_prot_set, mask);
>>> +		if (bpd[i].way_en)
>>> +			ret = scpsys_bus_way_disable(&bpd[i], regmap, infracfg_nao);
>>>    		else
>>> -			regmap_write(regmap, bpd[i].bus_prot_set, mask);
>>> -
>>> -		ret = regmap_read_poll_timeout(regmap, bpd[i].bus_prot_sta,
>>> -					       val, (val & mask) == mask,
>>> -					       MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> -		if (ret)
>>> +			ret = __scpsys_bus_protect_enable(&bpd[i], regmap);
>>> +		if (ret) {
>>> +			pr_err("%s %d %d\n", __PRETTY_FUNCTION__, __LINE__, ret);
>>>    			return ret;
>>> +		}
>>>    	}
>>>    	return 0;
>>> @@ -146,37 +182,71 @@ static int scpsys_bus_protect_enable(struct scpsys_domain *pd)
>>>    {
>>>    	int ret;
>>> -	ret = _scpsys_bus_protect_enable(pd->data->bp_infracfg, pd->infracfg);
>>> +	ret = _scpsys_bus_protect_enable(pd->data->bp_infracfg,
>>> +					 pd->infracfg, pd->infracfg_nao);
>>>    	if (ret)
>>>    		return ret;
>>> -	return _scpsys_bus_protect_enable(pd->data->bp_smi, pd->smi);
>>> +	return _scpsys_bus_protect_enable(pd->data->bp_smi, pd->smi, NULL);
>>> +}
>>> +
>>> +static int __scpsys_bus_protect_disable(const struct scpsys_bus_prot_data *bpd,
>>> +					struct regmap *regmap)
>>> +{
>>> +	u32 val;
>>> +	u32 mask = bpd->bus_prot_mask;
>>> +	u32 sta_mask = bpd->bus_prot_sta_mask;
>>> +
>>> +	if (bpd->bus_prot_reg_update)
>>> +		regmap_clear_bits(regmap, bpd->bus_prot_clr, mask);
>>> +	else
>>> +		regmap_write(regmap, bpd->bus_prot_clr, mask);
>>> +
>>> +	if (bpd->ignore_clr_ack)
>>> +		return 0;
>>> +
>>> +	return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, val,
>>> +					!(val & sta_mask), MTK_POLL_DELAY_US,
>>> +					MTK_POLL_TIMEOUT);
>>> +}
>>> +
>>> +static int scpsys_bus_way_enable(const struct scpsys_bus_prot_data *bpd,
>>> +				 struct regmap *regmap,
>>> +				 struct regmap *ack_regmap)
>>> +{
>>> +	u32 val;
>>> +	u32 mask = bpd->bus_prot_mask;
>>> +	u32 sta_mask = bpd->bus_prot_sta_mask;
>>> +
>>> +	if (bpd->bus_prot_reg_update)
>>> +		regmap_set_bits(regmap, bpd->bus_prot_clr, mask);
>>> +	else
>>> +		regmap_write(regmap, bpd->bus_prot_clr, mask);

same as disable case.

>>> +
>>> +	return regmap_read_poll_timeout(ack_regmap, bpd->bus_prot_sta, val,
>>> +					(val & sta_mask) == sta_mask,
>>> +					MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>>    }
>>>    static int _scpsys_bus_protect_disable(const struct scpsys_bus_prot_data *bpd,
>>> -				       struct regmap *regmap)
>>> +				       struct regmap *regmap,
>>> +				       struct regmap *infracfg_nao)
>>>    {
>>>    	int i, ret;
>>>    	for (i = SPM_MAX_BUS_PROT_DATA - 1; i >= 0; i--) {
>>> -		u32 val, mask = bpd[i].bus_prot_mask;
>>> -
>>> -		if (!mask)
>>> +		if (!bpd[i].bus_prot_mask)
>>>    			continue;
>>> -		if (bpd[i].bus_prot_reg_update)
>>> -			regmap_clear_bits(regmap, bpd[i].bus_prot_clr, mask);
>>> +		if (bpd[i].way_en)
>>> +			ret = scpsys_bus_way_enable(&bpd[i], regmap,
>>> +						    infracfg_nao);
>>>    		else
>>> -			regmap_write(regmap, bpd[i].bus_prot_clr, mask);
>>> -
>>> -		if (bpd[i].ignore_clr_ack)
>>> -			continue;
>>> -
>>> -		ret = regmap_read_poll_timeout(regmap, bpd[i].bus_prot_sta,
>>> -					       val, !(val & mask),
>>> -					       MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> -		if (ret)
>>> +			ret = __scpsys_bus_protect_disable(&bpd[i], regmap);
>>> +		if (ret) {
>>> +			pr_err("%s %d %d\n", __PRETTY_FUNCTION__, __LINE__, ret);
>>>    			return ret;
>>> +		}
>>>    	}
>>>    	return 0;
>>> @@ -186,11 +256,12 @@ static int scpsys_bus_protect_disable(struct scpsys_domain *pd)
>>>    {
>>>    	int ret;
>>> -	ret = _scpsys_bus_protect_disable(pd->data->bp_smi, pd->smi);
>>> +	ret = _scpsys_bus_protect_disable(pd->data->bp_smi, pd->smi, NULL);
>>>    	if (ret)
>>>    		return ret;
>>> -	return _scpsys_bus_protect_disable(pd->data->bp_infracfg, pd->infracfg);
>>> +	return _scpsys_bus_protect_disable(pd->data->bp_infracfg,
>>> +			pd->infracfg, pd->infracfg_nao);
>>>    }
>>>    static int scpsys_regulator_enable(struct regulator *supply)
>>> @@ -294,6 +365,21 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>>>    	return 0;
>>>    }
>>> +static bool scpsys_bp_infracfg_has_way_en(const struct scpsys_bus_prot_data *bpd)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
>>> +		if (!bpd[i].bus_prot_mask)
>>> +			break;
>>
>> So MT8365_POWER_DOMAIN_MM will return false as the first member of
>> bp_infracfg is BUS_PROT_WR(...)
> 
> I am not sure I understand what you mean. Why should it break out of the
> loop if the first member is a BUS_PROT_WR()? BUS_PROT_WR() sets a mask
> as well which is checked here exactly the same way as is done in
> _scpsys_bus_protect_enable() even before this patch.

Right, my error, please see comment further down about the new macros.

> 
> This is only a loop condition. Actually I can move it into the loop
> header as well. Either you define SPM_MAX_BUS_PROT_DATA fields or you
> have to exit if you find a field that is empty, basically the mask not
> being set.
> 
>>
>> Apart from that, why don't you use a CAPS to acheive the same?
>>

Just in case you missed that. I'd pretty much prefer to add a caps to 
scpsys_domain_data signaling that the power domain uses WAY_EN.

>>> +
>>> +		if (bpd[i].way_en)
>>> +			return true;
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +
>>>    static struct
>>>    generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node)
>>>    {
>>> @@ -364,6 +450,20 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
>>>    			return ERR_CAST(pd->smi);
>>>    	}
>>> +	if (scpsys_bp_infracfg_has_way_en(pd->data->bp_smi)) {
>>> +		dev_err(scpsys->dev, "bp_smi does not support WAY_EN\n");
>>
>> Do we really need to check the correctness of the driver data at runtime?
> 
> bp_smi is called without a infracfg_nao regmap. If we don't check it
> here, we need to make a check during bus protection operations. Last
> time I got a review to not do it during in the bus protection path.
> 

Maybe I'm missing something here but bpi_smi is defined in the 
scpsys_domain_data. Why do we need to check at runtime if the SoC specific data 
hardcoded in the dirver is correct? What am I missing here?

Apart do I understand correctly that the second call to 
_scpsys_bus_protect_enable() in scpsys_bus_protect_enable() will never use 
way_en? My understanding that this also holds for the disable path. Can't we 
remodel the code that in this case we don't loop over scpsys_bus_prot_data and 
check for way_en?

>>
>>> +		return ERR_PTR(-EINVAL);
>>> +	}
>>> +
>>> +	pd->infracfg_nao = syscon_regmap_lookup_by_phandle_optional(
>>> +		node, "mediatek,infracfg_nao");
>>
>> Not in the binding description.
> 
> Thanks, I will fix that for the next version.
> 
>>
>>> +	if (IS_ERR(pd->infracfg_nao)) {
>>> +		if (scpsys_bp_infracfg_has_way_en(pd->data->bp_infracfg))
>>> +			return ERR_CAST(pd->infracfg_nao);
>>> +
>>> +		pd->infracfg_nao = NULL;
>>> +	}
>>> +
>>>    	num_clks = of_clk_get_parent_count(node);
>>>    	if (num_clks > 0) {
>>>    		/* Calculate number of subsys_clks */
>>> diff --git a/drivers/soc/mediatek/mtk-pm-domains.h b/drivers/soc/mediatek/mtk-pm-domains.h
>>> index 7d3c0c36316c..974c68a1d89c 100644
>>> --- a/drivers/soc/mediatek/mtk-pm-domains.h
>>> +++ b/drivers/soc/mediatek/mtk-pm-domains.h
>>> @@ -41,23 +41,29 @@
>>>    #define SPM_MAX_BUS_PROT_DATA		6
>>> -#define _BUS_PROT(_mask, _set, _clr, _sta, _update, _ignore) {	\
>>> -		.bus_prot_mask = (_mask),			\
>>> -		.bus_prot_set = _set,				\
>>> -		.bus_prot_clr = _clr,				\
>>> -		.bus_prot_sta = _sta,				\
>>> -		.bus_prot_reg_update = _update,			\
>>> -		.ignore_clr_ack = _ignore,			\
>>> +#define _BUS_PROT(_mask, _sta_mask, _set, _clr, _sta, _update, _ignore, _way_en) {	\

_sta_mask is second parameter but not second member of the struct. That makes 
reading the driver unnecessary complicated.

>>> +		.bus_prot_mask = (_mask),				\
>>> +		.bus_prot_set = _set,					\
>>> +		.bus_prot_clr = _clr,					\
>>> +		.bus_prot_sta = _sta,					\
>>> +		.bus_prot_sta_mask = _sta_mask,				\
>>> +		.bus_prot_reg_update = _update,				\
>>> +		.ignore_clr_ack = _ignore,				\
>>> +		.way_en = _way_en,					\
>>>    	}
>>>    #define BUS_PROT_WR(_mask, _set, _clr, _sta)			\
>>> -		_BUS_PROT(_mask, _set, _clr, _sta, false, false)
>>> +		_BUS_PROT(_mask, _mask, _set, _clr, _sta, false, false, false)
>>>    #define BUS_PROT_WR_IGN(_mask, _set, _clr, _sta)		\
>>> -		_BUS_PROT(_mask, _set, _clr, _sta, false, true)
>>> +		_BUS_PROT(_mask, _mask, _set, _clr, _sta, false, true, false)
>>>    #define BUS_PROT_UPDATE(_mask, _set, _clr, _sta)		\
>>> -		_BUS_PROT(_mask, _set, _clr, _sta, true, false)
>>> +		_BUS_PROT(_mask, _mask, _set, _clr, _sta, true, false, false)
>>> +
>>> +#define BUS_PROT_WAY_EN(_en_mask, _sta_mask, _set, _sta)	\
>>> +		_BUS_PROT(_en_mask, _sta_mask, _set, _set, _sta, true, false, \
>>> +			  true)
>>>    #define BUS_PROT_UPDATE_TOPAXI(_mask)				\
>>>    		BUS_PROT_UPDATE(_mask,				\
>>> @@ -70,8 +76,10 @@ struct scpsys_bus_prot_data {
>>>    	u32 bus_prot_set;
>>>    	u32 bus_prot_clr;
>>>    	u32 bus_prot_sta;
>>> +	u32 bus_prot_sta_mask;
>>
>> I'm not very happy with the naming. In the end we need an extra mask for bus
>> protection using WAY_EN. But right now I can't come up with a good name.
> 
> I think the naming is good as it is a specific mask for the status
> register. bus_prot_mask is now basically only responsible for set and
> clr. Maybe renaming bus_prot_mask to bus_prot_set_clr_mask is better?

Yes makes sense. Please add this as an extra patch for easier review.

Regards,
Matthias

> 
> Thanks,
> Markus
> 
>>
>> Regards,
>> Matthias
>>
>>>    	bool bus_prot_reg_update;
>>>    	bool ignore_clr_ack;
>>> +	bool way_en;
>>>    };
>>>    /**