Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <81276850-d371-e61d-e13d-3a90aa950981@loongson.cn>
Date:   Thu, 1 Jun 2023 21:21:37 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v6 2/6] drm/etnaviv: add a dedicated function to get
 various clocks
Content-Language: en-US
To:     Lucas Stach <l.stach@pengutronix.de>,
        Russell King <linux+etnaviv@armlinux.org.uk>,
        Christian Gmeiner <christian.gmeiner@gmail.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>
Cc:     linux-kernel@vger.kernel.org, etnaviv@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn
References: <20230530160643.2344551-1-suijingfeng@loongson.cn>
 <20230530160643.2344551-3-suijingfeng@loongson.cn>
 <85565974d45b5553035aeabe8a98a667718482d5.camel@pengutronix.de>
From:   Sui Jingfeng <suijingfeng@loongson.cn>
Organization: Loongson
In-Reply-To: <85565974d45b5553035aeabe8a98a667718482d5.camel@pengutronix.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 2023/6/1 02:07, Lucas Stach wrote:
>> +static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu)
>> +{
>> +	struct device *dev = gpu->dev;
>> +
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
>> +	gpu->clk_reg = devm_clk_get_optional(dev, "reg");
>> +	DBG("clk_reg: %p", gpu->clk_reg);
>> +	if (IS_ERR(gpu->clk_reg))
>> +		return PTR_ERR(gpu->clk_reg);
>> +
>> +	gpu->clk_bus = devm_clk_get_optional(dev, "bus");
>> +	DBG("clk_bus: %p", gpu->clk_bus);
>> +	if (IS_ERR(gpu->clk_bus))
>> +		return PTR_ERR(gpu->clk_bus);
>> +
>> +	gpu->clk_core = devm_clk_get(dev, "core");
>> +	DBG("clk_core: %p", gpu->clk_core);
>> +	if (IS_ERR(gpu->clk_core))
>> +		return PTR_ERR(gpu->clk_core);
>> +	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
>> +
>> +	gpu->clk_shader = devm_clk_get_optional(dev, "shader");
>> +	DBG("clk_shader: %p", gpu->clk_shader);
>> +	if (IS_ERR(gpu->clk_shader))
>> +		return PTR_ERR(gpu->clk_shader);
>> +	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
>> +
>> +	return 0;
>> +}
>> +
>>   static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
>>   {
>>   	int ret;
>>   
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
> I don't see why this would be needed?
I have just tested, this do not needed.
> If your platform doesn't provide
> CONFIG_HAVE_CLK all those functions should be successful no-ops, so
> there is no need to special case this in the driver.

My platform do enable CONFIG_HAVE_CLK,

for ls3a5000 + ls7a1000, my system do not provide device tree support,

that's is to say, there is no DT support.


For ls3a4000 + ls7a1000 platform, the system has DT support, but don't 
has CLK drivers implement toward the clock tree.

typically, our platform's firmware will do such thing(setting a default 
working frequency).


When I first saw etnaviv, I'm also astonishing.

I don't know why there so much clock controllable.

As far as I can understand, my system/hardware have only one clock,

It shall corresponding to the core clk.

> Or does your platform in fact provide a clk subsystem, just the GPU
> clocks are managed by it?
>
> Also all those functions are fine with being called on a NULL clk, so
> shouldn't it be enough to simply avoid calling etnaviv_gpu_clk_get() in
> the PCI device case?
>
> Regards,
> Lucas
>
-- 
Jingfeng