diff mbox series

drm/v3d: Add clock handling

Message ID 20250201125046.33030-1-wahrenst@gmx.net (mailing list archive)
State New, archived
Headers show
Series drm/v3d: Add clock handling | expand

Commit Message

Stefan Wahren Feb. 1, 2025, 12:50 p.m. UTC 
Since the initial commit 57692c94dcbe ("drm/v3d: Introduce a new DRM driver
for Broadcom V3D V3.x+") the struct v3d_dev reserved a pointer for
an optional V3D clock. But there wasn't any code, which fetched it.
So add the missing clock handling before accessing any V3D registers.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/gpu/drm/v3d/v3d_drv.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

--
2.34.1

Comments

Maíra Canal Feb. 10, 2025, 12:23 p.m. UTC | #1 
Hi Stefan,

Thanks for your patch!

On 01/02/25 09:50, Stefan Wahren wrote:
> Since the initial commit 57692c94dcbe ("drm/v3d: Introduce a new DRM driver
> for Broadcom V3D V3.x+") the struct v3d_dev reserved a pointer for
> an optional V3D clock. But there wasn't any code, which fetched it.
> So add the missing clock handling before accessing any V3D registers.

Actually, I believe we should remove `v3d->clk`. In the past, we used
`v3d->clk` for PM management, but we removed PM management a while ago
(it was somewhat broken).

I believe the best move would be to remove `v3d->clk`. If we decide to
use the clock at some point, we can reintroduce the variable to the
struct. Right now, it doesn't make sense to add clock handling if we
don't use it.

Let me know if you have any use case for this variable. If you have,
then we can add clock handling.

Best Regards,
- Maíra

> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>   drivers/gpu/drm/v3d/v3d_drv.c | 25 ++++++++++++++++++++-----
>   1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
> index 930737a9347b..852015214e97 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.c
> +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> @@ -295,11 +295,21 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
> 
> +	v3d->clk = devm_clk_get_optional(dev, NULL);
> +	if (IS_ERR(v3d->clk))
> +		return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");
> +
> +	ret = clk_prepare_enable(v3d->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't enable the V3D clock\n");
> +		return ret;
> +	}
> +
>   	mmu_debug = V3D_READ(V3D_MMU_DEBUG_INFO);
>   	mask = DMA_BIT_MASK(30 + V3D_GET_FIELD(mmu_debug, V3D_MMU_PA_WIDTH));
>   	ret = dma_set_mask_and_coherent(dev, mask);
>   	if (ret)
> -		return ret;
> +		goto clk_disable;
> 
>   	v3d->va_width = 30 + V3D_GET_FIELD(mmu_debug, V3D_MMU_VA_WIDTH);
> 
> @@ -319,28 +329,29 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
>   		ret = PTR_ERR(v3d->reset);
> 
>   		if (ret == -EPROBE_DEFER)
> -			return ret;
> +			goto clk_disable;
> 
>   		v3d->reset = NULL;
>   		ret = map_regs(v3d, &v3d->bridge_regs, "bridge");
>   		if (ret) {
>   			dev_err(dev,
>   				"Failed to get reset control or bridge regs\n");
> -			return ret;
> +			goto clk_disable;
>   		}
>   	}
> 
>   	if (v3d->ver < 41) {
>   		ret = map_regs(v3d, &v3d->gca_regs, "gca");
>   		if (ret)
> -			return ret;
> +			goto clk_disable;
>   	}
> 
>   	v3d->mmu_scratch = dma_alloc_wc(dev, 4096, &v3d->mmu_scratch_paddr,
>   					GFP_KERNEL | __GFP_NOWARN | __GFP_ZERO);
>   	if (!v3d->mmu_scratch) {
>   		dev_err(dev, "Failed to allocate MMU scratch page\n");
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto clk_disable;
>   	}
> 
>   	ret = v3d_gem_init(drm);
> @@ -369,6 +380,8 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
>   	v3d_gem_destroy(drm);
>   dma_free:
>   	dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);
> +clk_disable:
> +	clk_disable_unprepare(v3d->clk);
>   	return ret;
>   }
> 
> @@ -386,6 +399,8 @@ static void v3d_platform_drm_remove(struct platform_device *pdev)
> 
>   	dma_free_wc(v3d->drm.dev, 4096, v3d->mmu_scratch,
>   		    v3d->mmu_scratch_paddr);
> +
> +	clk_disable_unprepare(v3d->clk);
>   }
> 
>   static struct platform_driver v3d_platform_driver = {
> --
> 2.34.1
>
Stefan Wahren Feb. 10, 2025, 1:27 p.m. UTC | #2 
Hi Maíra,

Am 10.02.25 um 13:23 schrieb Maíra Canal:
> Hi Stefan,
>
> Thanks for your patch!
>
> On 01/02/25 09:50, Stefan Wahren wrote:
>> Since the initial commit 57692c94dcbe ("drm/v3d: Introduce a new DRM
>> driver
>> for Broadcom V3D V3.x+") the struct v3d_dev reserved a pointer for
>> an optional V3D clock. But there wasn't any code, which fetched it.
>> So add the missing clock handling before accessing any V3D registers.
>
> Actually, I believe we should remove `v3d->clk`. In the past, we used
> `v3d->clk` for PM management, but we removed PM management a while ago
> (it was somewhat broken).
I disagree. Clock handling and power management are not the same things.
Btw the brokeness partly based on the missing clock handling, but this
not my point here. The DT binding of the Broadcom V3D GPU describe an
optional clock, so the Linux kernel should ensure that before accessing
any V3D registers, the relevant clock must be enabled. Please compare
with VC4, which does the same thing.

At the end we had just luck because the GPU firmware enabled the clock
at boot?
>
> I believe the best move would be to remove `v3d->clk`. If we decide to
> use the clock at some point, we can reintroduce the variable to the
> struct. Right now, it doesn't make sense to add clock handling if we
> don't use it.
clk_prepare_enable() / clk_disable_unprepare() is actually using the
clock. There is no need to set some kind of rate, in case you expecting
this.

Best regards
Maíra Canal Feb. 12, 2025, 1:07 p.m. UTC | #3 
Hi Stefan,

On 10/02/25 10:27, Stefan Wahren wrote:
> Hi Maíra,
> 
> Am 10.02.25 um 13:23 schrieb Maíra Canal:
>> Hi Stefan,
>>
>> Thanks for your patch!
>>
>> On 01/02/25 09:50, Stefan Wahren wrote:
>>> Since the initial commit 57692c94dcbe ("drm/v3d: Introduce a new DRM
>>> driver
>>> for Broadcom V3D V3.x+") the struct v3d_dev reserved a pointer for
>>> an optional V3D clock. But there wasn't any code, which fetched it.
>>> So add the missing clock handling before accessing any V3D registers.
>>
>> Actually, I believe we should remove `v3d->clk`. In the past, we used
>> `v3d->clk` for PM management, but we removed PM management a while ago
>> (it was somewhat broken).
> I disagree. Clock handling and power management are not the same things.
> Btw the brokeness partly based on the missing clock handling, but this
> not my point here. The DT binding of the Broadcom V3D GPU describe an
> optional clock, so the Linux kernel should ensure that before accessing
> any V3D registers, the relevant clock must be enabled. Please compare
> with VC4, which does the same thing.
> 
> At the end we had just luck because the GPU firmware enabled the clock
> at boot?

I see your point now, thanks for taking your time to send this patch!

Applied to misc/kernel.git (drm-misc-next) [1]

[1] 
https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/4dd40b5f9c3d89b67af0dbe059cf4a51aac6bf06

Best Regards,
- Maíra

>>
>> I believe the best move would be to remove `v3d->clk`. If we decide to
>> use the clock at some point, we can reintroduce the variable to the
>> struct. Right now, it doesn't make sense to add clock handling if we
>> don't use it.
> clk_prepare_enable() / clk_disable_unprepare() is actually using the
> clock. There is no need to set some kind of rate, in case you expecting
> this.
> 
> Best regards
diff mbox series

Patch

diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 930737a9347b..852015214e97 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -295,11 +295,21 @@  static int v3d_platform_drm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;

+	v3d->clk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(v3d->clk))
+		return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");
+
+	ret = clk_prepare_enable(v3d->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't enable the V3D clock\n");
+		return ret;
+	}
+
 	mmu_debug = V3D_READ(V3D_MMU_DEBUG_INFO);
 	mask = DMA_BIT_MASK(30 + V3D_GET_FIELD(mmu_debug, V3D_MMU_PA_WIDTH));
 	ret = dma_set_mask_and_coherent(dev, mask);
 	if (ret)
-		return ret;
+		goto clk_disable;

 	v3d->va_width = 30 + V3D_GET_FIELD(mmu_debug, V3D_MMU_VA_WIDTH);

@@ -319,28 +329,29 @@  static int v3d_platform_drm_probe(struct platform_device *pdev)
 		ret = PTR_ERR(v3d->reset);

 		if (ret == -EPROBE_DEFER)
-			return ret;
+			goto clk_disable;

 		v3d->reset = NULL;
 		ret = map_regs(v3d, &v3d->bridge_regs, "bridge");
 		if (ret) {
 			dev_err(dev,
 				"Failed to get reset control or bridge regs\n");
-			return ret;
+			goto clk_disable;
 		}
 	}

 	if (v3d->ver < 41) {
 		ret = map_regs(v3d, &v3d->gca_regs, "gca");
 		if (ret)
-			return ret;
+			goto clk_disable;
 	}

 	v3d->mmu_scratch = dma_alloc_wc(dev, 4096, &v3d->mmu_scratch_paddr,
 					GFP_KERNEL | __GFP_NOWARN | __GFP_ZERO);
 	if (!v3d->mmu_scratch) {
 		dev_err(dev, "Failed to allocate MMU scratch page\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto clk_disable;
 	}

 	ret = v3d_gem_init(drm);
@@ -369,6 +380,8 @@  static int v3d_platform_drm_probe(struct platform_device *pdev)
 	v3d_gem_destroy(drm);
 dma_free:
 	dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);
+clk_disable:
+	clk_disable_unprepare(v3d->clk);
 	return ret;
 }

@@ -386,6 +399,8 @@  static void v3d_platform_drm_remove(struct platform_device *pdev)

 	dma_free_wc(v3d->drm.dev, 4096, v3d->mmu_scratch,
 		    v3d->mmu_scratch_paddr);
+
+	clk_disable_unprepare(v3d->clk);
 }

 static struct platform_driver v3d_platform_driver = {