Re: [PATCH 3/4] drm/panfrost: Add bus_ace optional clock support for RZ/G2L

[Steven Price]

On 20/03/2026 12:30, Biju Das wrote:
> Hi Steven Price,
> 
> Thanks for the feedback.
> 
>> -----Original Message-----
>> From: Steven Price <steven.price@xxxxxxx>
>> Sent: 20 March 2026 12:16
>> Subject: Re: [PATCH 3/4] drm/panfrost: Add bus_ace optional clock support for RZ/G2L
>>
>> On 04/03/2026 13:48, Biju wrote:
>>> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>>>
>>> On RZ/G2L SoCs, the GPU MMU requires a bus_ace clock to operate correctly.
>>> Without it, unbind/bind cycles leave the GPU non-operational,
>>> manifesting as an AS_ACTIVE bit stuck and a soft reset timeout falling
>>> back to hard reset. Add bus_ace_clock as an optional clock, wiring it
>>> into init/fini, and the runtime suspend/resume paths alongside the
>>> existing optional bus_clock.
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/panfrost/panfrost_device.c | 24
>>> ++++++++++++++++++++++  drivers/gpu/drm/panfrost/panfrost_device.h |
>>> 1 +
>>>  2 files changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c
>>> b/drivers/gpu/drm/panfrost/panfrost_device.c
>>> index 01e702a0b2f0..87dae0ed748a 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
>>> @@ -70,8 +70,23 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
>>>  			goto disable_clock;
>>>  	}
>>>
>>> +	pfdev->bus_ace_clock = devm_clk_get_optional(pfdev->base.dev, "bus_ace");
>>> +	if (IS_ERR(pfdev->bus_ace_clock)) {
>>> +		err = PTR_ERR(pfdev->bus_ace_clock);
>>> +		dev_err(pfdev->base.dev, "get bus_ace_clock failed %ld\n",
>>> +			PTR_ERR(pfdev->bus_ace_clock));
>>> +		err = PTR_ERR(pfdev->bus_ace_clock);
>>
>> You've assigned err twice (with the same value), and you can simplify the dev_err() line by using err
> 
> Oops, forgot to take out the bottom assignment.
> 
>> rather than the same PTR_ERR() expression again.
> 
> I get a warning, if I use "err" in dev_err()
> 
> panfrost_device.c:76:42: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘int’ [-Wformat=]
>    76 |                 dev_err(pfdev->base.dev, "get bus_ace_clock failed %ld\n",

You can simply change the format string to "%d".

Explanation:

PTR_ERR returns a long (which matches the kernel's idea that a long is
the same size as a pointer). But the standard return code size is int.
So technically the assignment to err is truncating the type. However,
the IS_ERR() check uses MAX_ERRNO which is 4095 so all error values will
fit in an int. So we know the assignment into 'int' isn't going to
truncate. [ Also it's just an error message... ;) ]

Thanks,
Steve

> 
> Cheers,
> Biju
> 
>>
>> With that fixed:
>>
>> Reviewed-by: Steven Price <steven.price@xxxxxxx>
>>
>> Thanks,
>> Steve
>>
>>> +		goto disable_bus_clock;
>>> +	}
>>> +
>>> +	err = clk_prepare_enable(pfdev->bus_ace_clock);
>>> +	if (err)
>>> +		goto disable_bus_clock;
>>> +
>>>  	return 0;
>>>
>>> +disable_bus_clock:
>>> +	clk_disable_unprepare(pfdev->bus_clock);
>>>  disable_clock:
>>>  	clk_disable_unprepare(pfdev->clock);
>>>
>>> @@ -80,6 +95,7 @@ static int panfrost_clk_init(struct panfrost_device
>>> *pfdev)
>>>
>>>  static void panfrost_clk_fini(struct panfrost_device *pfdev)  {
>>> +	clk_disable_unprepare(pfdev->bus_ace_clock);
>>>  	clk_disable_unprepare(pfdev->bus_clock);
>>>  	clk_disable_unprepare(pfdev->clock);
>>>  }
>>> @@ -432,6 +448,10 @@ static int panfrost_device_runtime_resume(struct device *dev)
>>>  		ret = clk_enable(pfdev->bus_clock);
>>>  		if (ret)
>>>  			goto err_bus_clk;
>>> +
>>> +		ret = clk_enable(pfdev->bus_ace_clock);
>>> +		if (ret)
>>> +			goto err_bus_ace_clk;
>>>  	}
>>>
>>>  	panfrost_device_reset(pfdev, true);
>>> @@ -439,6 +459,9 @@ static int panfrost_device_runtime_resume(struct
>>> device *dev)
>>>
>>>  	return 0;
>>>
>>> +err_bus_ace_clk:
>>> +	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
>>> +		clk_disable(pfdev->bus_clock);
>>>  err_bus_clk:
>>>  	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
>>>  		clk_disable(pfdev->clock);
>>> @@ -462,6 +485,7 @@ static int panfrost_device_runtime_suspend(struct device *dev)
>>>  	panfrost_gpu_power_off(pfdev);
>>>
>>>  	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
>>> +		clk_disable(pfdev->bus_ace_clock);
>>>  		clk_disable(pfdev->bus_clock);
>>>  		clk_disable(pfdev->clock);
>>>  		reset_control_assert(pfdev->rstc);
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h
>>> b/drivers/gpu/drm/panfrost/panfrost_device.h
>>> index 0f3992412205..ec55c136b1b6 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
>>> @@ -136,6 +136,7 @@ struct panfrost_device {
>>>  	void __iomem *iomem;
>>>  	struct clk *clock;
>>>  	struct clk *bus_clock;
>>> +	struct clk *bus_ace_clock;
>>>  	struct regulator_bulk_data *regulators;
>>>  	struct reset_control *rstc;
>>>  	/* pm_domains for devices with more than one. */
>