Hi,
please consider applying patches that are chained to this message.
They make getting/enabling the clocks in the etnaviv driver slightly nicer, first two also fix potential problems.
Compared to v1, patch 2/4 was fixed and patch 3/4 was added.
As it was pointed out in response to v1, the clocks documented as mandatory by the binding document are different from what the driver enforces. Moreover, there is no agreement on which clocks must be present in the device tree, so I'm leaving the binding document until it's cleared up.
In any case, the "core" clock is always present so it's safe to make it mandatory and regardless of what ends up happening to the binding documentation, the other clocks can't be enforced without regressions. At most a comment or a warning could be added. I'm leaving it as it is.
Thank you Lubo
Since commit 65f037e8e908 ("drm/etnaviv: add support for slave interface clock") the reg clock is enabled before the bus clock and we need to undo its enablement on error.
Fixes: 65f037e8e908 ("drm/etnaviv: add support for slave interface clock") Signed-off-by: Lubomir Rintel lkundrak@v3.sk --- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index a31eeff2b297a..c6dacfe3d321e 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1496,7 +1496,7 @@ static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu) if (gpu->clk_bus) { ret = clk_prepare_enable(gpu->clk_bus); if (ret) - return ret; + goto disable_clk_reg; }
if (gpu->clk_core) { @@ -1519,6 +1519,9 @@ static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu) disable_clk_bus: if (gpu->clk_bus) clk_disable_unprepare(gpu->clk_bus); +disable_clk_reg: + if (gpu->clk_reg) + clk_disable_unprepare(gpu->clk_reg);
return ret; }
There might be good reasons why the getting a clock failed. To treat the clocks as optional we're specifically only interested in ignoring -ENOENT, and devm_clk_get_optional() does just that.
Note that this preserves the original behavior of all clocks being optional. The binding document mandates the "bus" clock while the dove machine only specifies "core".
Signed-off-by: Lubomir Rintel lkundrak@v3.sk
--- Changes since v1: - Fix the actual return value
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index c6dacfe3d321e..f303172c091db 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1786,26 +1786,26 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev) }
/* Get Clocks: */ - gpu->clk_reg = devm_clk_get(&pdev->dev, "reg"); + gpu->clk_reg = devm_clk_get_optional(&pdev->dev, "reg"); DBG("clk_reg: %p", gpu->clk_reg); if (IS_ERR(gpu->clk_reg)) - gpu->clk_reg = NULL; + return PTR_ERR(gpu->clk_reg);
- gpu->clk_bus = devm_clk_get(&pdev->dev, "bus"); + gpu->clk_bus = devm_clk_get_optional(&pdev->dev, "bus"); DBG("clk_bus: %p", gpu->clk_bus); if (IS_ERR(gpu->clk_bus)) - gpu->clk_bus = NULL; + return PTR_ERR(gpu->clk_bus);
- gpu->clk_core = devm_clk_get(&pdev->dev, "core"); + gpu->clk_core = devm_clk_get_optional(&pdev->dev, "core"); DBG("clk_core: %p", gpu->clk_core); if (IS_ERR(gpu->clk_core)) - gpu->clk_core = NULL; + return PTR_ERR(gpu->clk_core); gpu->base_rate_core = clk_get_rate(gpu->clk_core);
- gpu->clk_shader = devm_clk_get(&pdev->dev, "shader"); + gpu->clk_shader = devm_clk_get_optional(&pdev->dev, "shader"); DBG("clk_shader: %p", gpu->clk_shader); if (IS_ERR(gpu->clk_shader)) - gpu->clk_shader = NULL; + return PTR_ERR(gpu->clk_shader); gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
/* TODO: figure out max mapped size */
It is always present. It was documented as mandatory prior to commit 90aeca875f8a ("dt-bindings: display: Convert etnaviv to json-schema").
Signed-off-by: Lubomir Rintel lkundrak@v3.sk
--- Changes since v1: - Add this patch
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index f303172c091db..798fdbc8ecdb5 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1796,7 +1796,7 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev) if (IS_ERR(gpu->clk_bus)) return PTR_ERR(gpu->clk_bus);
- gpu->clk_core = devm_clk_get_optional(&pdev->dev, "core"); + gpu->clk_core = devm_clk_get(&pdev->dev, "core"); DBG("clk_core: %p", gpu->clk_core); if (IS_ERR(gpu->clk_core)) return PTR_ERR(gpu->clk_core);
All the NULL checks are pointless, clk_*() routines already deal with NULL just fine.
Signed-off-by: Lubomir Rintel lkundrak@v3.sk --- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 53 ++++++++++----------------- 1 file changed, 19 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index 798fdbc8ecdb5..fb37787449bb7 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1487,55 +1487,40 @@ static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu) { int ret;
- if (gpu->clk_reg) { - ret = clk_prepare_enable(gpu->clk_reg); - if (ret) - return ret; - } + ret = clk_prepare_enable(gpu->clk_reg); + if (ret) + return ret;
- if (gpu->clk_bus) { - ret = clk_prepare_enable(gpu->clk_bus); - if (ret) - goto disable_clk_reg; - } + ret = clk_prepare_enable(gpu->clk_bus); + if (ret) + goto disable_clk_reg;
- if (gpu->clk_core) { - ret = clk_prepare_enable(gpu->clk_core); - if (ret) - goto disable_clk_bus; - } + ret = clk_prepare_enable(gpu->clk_core); + if (ret) + goto disable_clk_bus;
- if (gpu->clk_shader) { - ret = clk_prepare_enable(gpu->clk_shader); - if (ret) - goto disable_clk_core; - } + ret = clk_prepare_enable(gpu->clk_shader); + if (ret) + goto disable_clk_core;
return 0;
disable_clk_core: - if (gpu->clk_core) - clk_disable_unprepare(gpu->clk_core); + clk_disable_unprepare(gpu->clk_core); disable_clk_bus: - if (gpu->clk_bus) - clk_disable_unprepare(gpu->clk_bus); + clk_disable_unprepare(gpu->clk_bus); disable_clk_reg: - if (gpu->clk_reg) - clk_disable_unprepare(gpu->clk_reg); + clk_disable_unprepare(gpu->clk_reg);
return ret; }
static int etnaviv_gpu_clk_disable(struct etnaviv_gpu *gpu) { - if (gpu->clk_shader) - clk_disable_unprepare(gpu->clk_shader); - if (gpu->clk_core) - clk_disable_unprepare(gpu->clk_core); - if (gpu->clk_bus) - clk_disable_unprepare(gpu->clk_bus); - if (gpu->clk_reg) - clk_disable_unprepare(gpu->clk_reg); + clk_disable_unprepare(gpu->clk_shader); + clk_disable_unprepare(gpu->clk_core); + clk_disable_unprepare(gpu->clk_bus); + clk_disable_unprepare(gpu->clk_reg);
return 0; }
Am Dienstag, den 16.06.2020, 23:21 +0200 schrieb Lubomir Rintel:
Hi,
please consider applying patches that are chained to this message.
Thanks, I've applied all of them to etnaviv/next.
Regards, Lucas
They make getting/enabling the clocks in the etnaviv driver slightly nicer, first two also fix potential problems.
Compared to v1, patch 2/4 was fixed and patch 3/4 was added.
As it was pointed out in response to v1, the clocks documented as mandatory by the binding document are different from what the driver enforces. Moreover, there is no agreement on which clocks must be present in the device tree, so I'm leaving the binding document until it's cleared up.
In any case, the "core" clock is always present so it's safe to make it mandatory and regardless of what ends up happening to the binding documentation, the other clocks can't be enforced without regressions. At most a comment or a warning could be added. I'm leaving it as it is.
Thank you Lubo
etnaviv mailing list etnaviv@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/etnaviv
dri-devel@lists.freedesktop.org
-
Lubomir Rintel -
Lucas Stach