Re: [PATCH 3/9] memory: mtk-smi: Use clk_bulk instead of the clk ops

[Krzysztof Kozlowski]

On 11/07/2021 10:29, Yong Wu wrote:
> On Thu, 2021-07-08 at 11:32 +0200, Krzysztof Kozlowski wrote:
>> On 16/06/2021 13:43, Yong Wu wrote:
>>> smi have many clocks: apb/smi/gals.
>>> This patch use clk_bulk interface instead of the orginal one to simply
>>> the code.
>>>
>>> gals is optional clk(some larbs may don't have gals). use clk_bulk_optional
>>> instead. and then remove the has_gals flag.
>>>
>>> Also remove clk fail logs since bulk interface already output fail log.
>>>
>>> Signed-off-by: Yong Wu <yong.wu@xxxxxxxxxxxx>
>>> ---
>>>  drivers/memory/mtk-smi.c | 124 +++++++++++----------------------------
>>>  1 file changed, 34 insertions(+), 90 deletions(-)
>>>
>>> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
>>> index c5fb51f73b34..bcd2bf130655 100644
>>> --- a/drivers/memory/mtk-smi.c
>>> +++ b/drivers/memory/mtk-smi.c
>>> @@ -60,9 +60,18 @@ enum mtk_smi_gen {
>>>  	MTK_SMI_GEN2
>>>  };
>>>  
>>> +#define MTK_SMI_CLK_NR_MAX			4
>>> +
>>> +static const char * const mtk_smi_common_clocks[] = {
>>> +	"apb", "smi", "gals0", "gals1", /* glas is optional */
>>
>> Typo here - glas.
> 
> Will Fix. Thanks.
> 
>>
>>> +};
>>> +
> 
> [snip]
> 
>>> @@ -493,7 +449,7 @@ static int mtk_smi_common_probe(struct platform_device *pdev)
>>>  	struct device *dev = &pdev->dev;
>>>  	struct mtk_smi *common;
>>>  	struct resource *res;
>>> -	int ret;
>>> +	int i, ret;
>>>  
>>>  	common = devm_kzalloc(dev, sizeof(*common), GFP_KERNEL);
>>>  	if (!common)
>>> @@ -501,23 +457,13 @@ static int mtk_smi_common_probe(struct platform_device *pdev)
>>>  	common->dev = dev;
>>>  	common->plat = of_device_get_match_data(dev);
>>>  
>>> -	common->clk_apb = devm_clk_get(dev, "apb");
>>> -	if (IS_ERR(common->clk_apb))
>>> -		return PTR_ERR(common->clk_apb);
>>> -
>>> -	common->clk_smi = devm_clk_get(dev, "smi");
>>> -	if (IS_ERR(common->clk_smi))
>>> -		return PTR_ERR(common->clk_smi);
>>> +	common->clk_num = ARRAY_SIZE(mtk_smi_common_clocks);
>>> +	for (i = 0; i < common->clk_num; i++)
>>> +		common->clks[i].id = mtk_smi_common_clocks[i];
>>>  
>>> -	if (common->plat->has_gals) {
>>> -		common->clk_gals0 = devm_clk_get(dev, "gals0");
>>> -		if (IS_ERR(common->clk_gals0))
>>> -			return PTR_ERR(common->clk_gals0);
>>> -
>>> -		common->clk_gals1 = devm_clk_get(dev, "gals1");
>>> -		if (IS_ERR(common->clk_gals1))
>>> -			return PTR_ERR(common->clk_gals1);
>>> -	}
>>> +	ret = devm_clk_bulk_get_optional(dev, common->clk_num, common->clks);
>>> +	if (ret)
>>> +		return ret;
>>
>> How do you handle now missing required clocks?
> 
> It looks this is a common issue for this function which supports all the
> clocks could be optional. Is there common suggestion for this?
> 
> For our case, the apb/smi clocks are required while "gals" are optional.
> 
> thus, we should use devm_clk_bulk_get for the necessary clocks and
> devm_clk_bulk_get_optional for the optional ones. right?

Yes, I think that's the solution. Otherwise you might not have proper
clocks leading to accesses to disabled/gated hardware.

Best regards,
Krzysztof