Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 3/9] memory: mtk-smi: Use clk_bulk instead of the clk ops
To:     Yong Wu <yong.wu@mediatek.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Tomasz Figa <tfiga@chromium.org>,
        linux-mediatek@lists.infradead.org, srv_heupstream@mediatek.com,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        iommu@lists.linux-foundation.org, youlin.pei@mediatek.com,
        anan.sun@mediatek.com, ming-fan.chen@mediatek.com,
        yi.kuo@mediatek.com, anthony.huang@mediatek.com
References: <20210616114346.18812-1-yong.wu@mediatek.com>
 <20210616114346.18812-4-yong.wu@mediatek.com>
 <4047dfaf-f2f8-b6b2-52fd-41821475b162@canonical.com>
 <1625992199.22309.15.camel@mhfsdcap03>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Message-ID: <fe426c10-4cfe-103c-9961-1b8fdb18a16a@canonical.com>
Date:   Mon, 12 Jul 2021 08:44:47 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <1625992199.22309.15.camel@mhfsdcap03>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@mediatek.com>
>>> ---
>>>  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