Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

Posted by Heiko Stübner 10 months ago

Am Dienstag, 18. Februar 2025, 15:13:07 MEZ schrieb Sebastian Reichel:
> Hi,
> 
> On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote:
> > On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote:
> > >So I guess step1, check what error is actually returned.
> > 
> > I have checked that the return value is -517:
> > 
> > rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517
> > 
> > >Step2 check if clk_get_optional need to be adapted or alternatively
> > >catch the error in the vop2 and set the clock to NULL ourself in that case.
> > 
> > I tried the following patch to set the clock to NULL when clk_get_optional
> > failed with value -517, and hdmi0 is working now. There are also some
> > boards like rock 5 itx which only use hdmi1, I think we should also add
> > this logic to vop2->pll_hdmiphy0.
> > 
> > @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
> >  		return PTR_ERR(vop2->pll_hdmiphy0);
> >  	}
> >  
> > +	vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");
> > +	if (IS_ERR(vop2->pll_hdmiphy1)) {
> > +		drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1);
> > +		if (vop2->pll_hdmiphy1 == -EPROBE_DEFER)
> > +			vop2->pll_hdmiphy1 = NULL;
> > +		else
> > +			return PTR_ERR(vop2->pll_hdmiphy1);
> > +	}
> > +
> 
> This first of all shows, that we should replace drm_err in this
> place with dev_err_probe(), which hides -EPROBE_DEFER errors by
> default and instead captures the reason for /sys/kernel/debug/devices_deferred.
> 
> Second what you are doing in the above suggestion will break kernel
> configurations where VOP is built-in and the HDMI PHY is build as a
> module.
> 
> But I also think it would be better to have the clocks defined in the
> SoC level DT. I suppose that means vop2_bind would have to check if
> the HDMI controller <ID> is enabled and only requests pll_hdmiphy<ID>
> based on that. Considering there is the OF graph pointing from VOP
> to the enabled HDMI controllers, it should be able to do that.


I was more thinking about fixing the correct thing, with something like:

----------- 8< ----------
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cf7720b9172f..50faafbf5dda 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -5258,6 +5258,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
        if (!clkspec)
                return ERR_PTR(-EINVAL);

+       /* Check if node in clkspec is in disabled/fail state */
+       if (!of_device_is_available(clkspec->np))
+               return ERR_PTR(-ENOENT);
+
        mutex_lock(&of_clk_mutex);
        list_for_each_entry(provider, &of_clk_providers, link) {
                if (provider->node == clkspec->np) {
----------- 8< ----------

Because right now the clk framework does not handle nodes in
failed/disabled state and would defer indefinitly.

Posted by Johannes Erdfelt 10 months ago

On Tue, Feb 18, 2025, Heiko Stübner <heiko@sntech.de> wrote:
> I was more thinking about fixing the correct thing, with something like:
> 
> ----------- 8< ----------
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index cf7720b9172f..50faafbf5dda 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5258,6 +5258,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
>         if (!clkspec)
>                 return ERR_PTR(-EINVAL);
> 
> +       /* Check if node in clkspec is in disabled/fail state */
> +       if (!of_device_is_available(clkspec->np))
> +               return ERR_PTR(-ENOENT);
> +
>         mutex_lock(&of_clk_mutex);
>         list_for_each_entry(provider, &of_clk_providers, link) {
>                 if (provider->node == clkspec->np) {
> ----------- 8< ----------
> 
> Because right now the clk framework does not handle nodes in
> failed/disabled state and would defer indefinitly.

I've been testing the recent patches Jimmy Hon has posted to add the
Orange Pi 5 Ultra DT and I ran into this bug. The Ultra uses HDMI1 for
the HDMI TX.

This patch successfully fixes the issue I was seeing.

JE

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

Posted by Sebastian Reichel 10 months ago

Hi,

On Tue, Feb 18, 2025 at 03:53:06PM +0100, Heiko Stübner wrote:
> Am Dienstag, 18. Februar 2025, 15:13:07 MEZ schrieb Sebastian Reichel:
> > On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote:
> > > On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote:
> > > >So I guess step1, check what error is actually returned.
> > > 
> > > I have checked that the return value is -517:
> > > 
> > > rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517
> > > 
> > > >Step2 check if clk_get_optional need to be adapted or alternatively
> > > >catch the error in the vop2 and set the clock to NULL ourself in that case.
> > > 
> > > I tried the following patch to set the clock to NULL when clk_get_optional
> > > failed with value -517, and hdmi0 is working now. There are also some
> > > boards like rock 5 itx which only use hdmi1, I think we should also add
> > > this logic to vop2->pll_hdmiphy0.
> > > 
> > > @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
> > >  		return PTR_ERR(vop2->pll_hdmiphy0);
> > >  	}
> > >  
> > > +	vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");
> > > +	if (IS_ERR(vop2->pll_hdmiphy1)) {
> > > +		drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1);
> > > +		if (vop2->pll_hdmiphy1 == -EPROBE_DEFER)
> > > +			vop2->pll_hdmiphy1 = NULL;
> > > +		else
> > > +			return PTR_ERR(vop2->pll_hdmiphy1);
> > > +	}
> > > +
> > 
> > This first of all shows, that we should replace drm_err in this
> > place with dev_err_probe(), which hides -EPROBE_DEFER errors by
> > default and instead captures the reason for /sys/kernel/debug/devices_deferred.
> > 
> > Second what you are doing in the above suggestion will break kernel
> > configurations where VOP is built-in and the HDMI PHY is build as a
> > module.
> > 
> > But I also think it would be better to have the clocks defined in the
> > SoC level DT. I suppose that means vop2_bind would have to check if
> > the HDMI controller <ID> is enabled and only requests pll_hdmiphy<ID>
> > based on that. Considering there is the OF graph pointing from VOP
> > to the enabled HDMI controllers, it should be able to do that.
> 
> 
> I was more thinking about fixing the correct thing, with something like:
> 
> ----------- 8< ----------
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index cf7720b9172f..50faafbf5dda 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5258,6 +5258,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
>         if (!clkspec)
>                 return ERR_PTR(-EINVAL);
> 
> +       /* Check if node in clkspec is in disabled/fail state */
> +       if (!of_device_is_available(clkspec->np))
> +               return ERR_PTR(-ENOENT);
> +
>         mutex_lock(&of_clk_mutex);
>         list_for_each_entry(provider, &of_clk_providers, link) {
>                 if (provider->node == clkspec->np) {
> ----------- 8< ----------
> 
> Because right now the clk framework does not handle nodes in
> failed/disabled state and would defer indefinitly.

Also LGTM.

Greetings,

-- Sebastian

Re: [PATCH 3/4] arm64: dts: rockchip: Add HDMI1 PHY PLL clock source to VOP2 on RK3588

Posted by Cristian Ciocaltea 10 months ago

Hi,

On 2/18/25 6:05 PM, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Feb 18, 2025 at 03:53:06PM +0100, Heiko Stübner wrote:
>> Am Dienstag, 18. Februar 2025, 15:13:07 MEZ schrieb Sebastian Reichel:
>>> On Tue, Feb 18, 2025 at 08:17:46PM +0800, Jianfeng Liu wrote:
>>>> On Tue, 18 Feb 2025 11:00:57 +0100, Heiko Stübnerwrote:
>>>>> So I guess step1, check what error is actually returned.
>>>>
>>>> I have checked that the return value is -517:
>>>>
>>>> rockchip-drm display-subsystem: [drm] *ERROR* failed to get pll_hdmiphy1 with -517
>>>>
>>>>> Step2 check if clk_get_optional need to be adapted or alternatively
>>>>> catch the error in the vop2 and set the clock to NULL ourself in that case.
>>>>
>>>> I tried the following patch to set the clock to NULL when clk_get_optional
>>>> failed with value -517, and hdmi0 is working now. There are also some
>>>> boards like rock 5 itx which only use hdmi1, I think we should also add
>>>> this logic to vop2->pll_hdmiphy0.
>>>>
>>>> @@ -3733,6 +3751,15 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
>>>>  		return PTR_ERR(vop2->pll_hdmiphy0);
>>>>  	}
>>>>  
>>>> +	vop2->pll_hdmiphy1 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy1");
>>>> +	if (IS_ERR(vop2->pll_hdmiphy1)) {
>>>> +		drm_err(vop2->drm, "failed to get pll_hdmiphy1 with %d\n", vop2->pll_hdmiphy1);
>>>> +		if (vop2->pll_hdmiphy1 == -EPROBE_DEFER)
>>>> +			vop2->pll_hdmiphy1 = NULL;
>>>> +		else
>>>> +			return PTR_ERR(vop2->pll_hdmiphy1);
>>>> +	}
>>>> +
>>>
>>> This first of all shows, that we should replace drm_err in this
>>> place with dev_err_probe(), which hides -EPROBE_DEFER errors by
>>> default and instead captures the reason for /sys/kernel/debug/devices_deferred.
>>>
>>> Second what you are doing in the above suggestion will break kernel
>>> configurations where VOP is built-in and the HDMI PHY is build as a
>>> module.
>>>
>>> But I also think it would be better to have the clocks defined in the
>>> SoC level DT. I suppose that means vop2_bind would have to check if
>>> the HDMI controller <ID> is enabled and only requests pll_hdmiphy<ID>
>>> based on that. Considering there is the OF graph pointing from VOP
>>> to the enabled HDMI controllers, it should be able to do that.
>>
>>
>> I was more thinking about fixing the correct thing, with something like:
>>
>> ----------- 8< ----------
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index cf7720b9172f..50faafbf5dda 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -5258,6 +5258,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
>>         if (!clkspec)
>>                 return ERR_PTR(-EINVAL);
>>
>> +       /* Check if node in clkspec is in disabled/fail state */
>> +       if (!of_device_is_available(clkspec->np))
>> +               return ERR_PTR(-ENOENT);
>> +
>>         mutex_lock(&of_clk_mutex);
>>         list_for_each_entry(provider, &of_clk_providers, link) {
>>                 if (provider->node == clkspec->np) {
>> ----------- 8< ----------
>>
>> Because right now the clk framework does not handle nodes in
>> failed/disabled state and would defer indefinitly.
> 
> Also LGTM.

Thank you all for the feedback and proposed solutions!

I'm currently on leave and without access to any testing hw, but I'll be
back in a couple of days.

Regards,
Cristian