Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
Cc += robh On Wed, May 20, 2020 at 04:04:39PM +0200, Lucas Stach wrote: > Am Mittwoch, den 20.05.2020, 15:38 +0200 schrieb Lubomir Rintel: > > On Thu, May 14, 2020 at 09:53:08AM +0100, Russell King - ARM Linux admin > > wrote: > > > On Thu, May 14, 2020 at 10:40:58AM +0200, Lucas Stach wrote: > > > > Am Donnerstag, den 14.05.2020, 09:27 +0100 schrieb Russell King - ARM > > > > Linux admin: > > > > > On Thu, May 14, 2020 at 10:18:02AM +0200, Lucas Stach wrote: > > > > > > Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > > > > > > > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam > > > > > > > wrote: > > > > > > > > > > > > > > > The binding doc > > > > > > > > Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > > > > > > > says that only the 'reg' clock could be optional, the others are > > > > > > > > required. > > > > > > > > > > > > > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > > > > > > > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > > > > > > > > > > > > > Maybe the binding needs to be updated and it seems that using > > > > > > > devm_clk_get_optional() like you propose is safe. > > > > > > > > > > > > The binding is correct as-is. We want to require those clocks to be > > > > > > present, but the dove DT was added before the binding was > > > > > > finalized, so > > > > > > the driver still treats the clocks as optional to not break > > > > > > compatibility with old DTs. Maybe this warrants a comment in the > > > > > > code... > > > > > > > > > > The binding doc in mainline says: > > > > > > > > > > clocks: > > > > > items: > > > > > - description: AXI/master interface clock > > > > > - description: GPU core clock > > > > > - description: Shader clock (only required if GPU has feature > > > > > PIPE_3D) > > > > > - description: AHB/slave interface clock (only required if GPU > > > > > can gate slave interface independently) > > > > > minItems: 1 > > > > > maxItems: 4 > > > > > > > > > > clock-names: > > > > > items: > > > > > enum: [ bus, core, shader, reg ] > > > > > minItems: 1 > > > > > maxItems: 4 > > > > > > > > > > which looks correct to me - and means that Dove is compliant with > > > > > that. > > > > > > > > The YAML binding actually did loose something in translation here, > > > > which I didn't notice. Previously all those clocks were listed under > > > > "Required properties", with the exceptions listed in parenthesis. So > > > > the Dove GPU, which is a combined 2D/3D core should have axi, core and > > > > shader clocks specified. > > > > > > That may be your desire, but that is impossible without knowing that > > > (a) it has the clocks > > > (b) what those clocks are connected to > > > > > > I guess we could "make something up" but as DT is supposed to describe > > > hardware, I don't see how we can satisfy that and your requirement. > > > > > > The only thing that is known from the documentation is that there is > > > one clock for the GPU on Dove. > > > > Yes. This means that in fact "core" is the only required clock for all > > implementations of vivante,gc and the common binding needs to be updated > > to reflect that. I'll follow with a patch that does that, unless there > > are strong objections. > > > > If there are implementations that require different clock inputs, then they > > need to use additional compatible string for the particular flavor and the > > binding should have conditionals for them. Something like this: > > > > if: > > properties: > > compatible: > > contains: > > const: fsl,imx6sx-gpu > > then: > > properties: > > clocks: > > minItems: 4 > > The DT binding of a device should describe the hardware of the device, > not the specific integration into a SoC. I'm not too convinced about this. While I'm not able to produce a reference from a quick view either into ieee1275 and DTSpec, I believe the DT describes the hardware from software's perspective. That is, there's no point in describing hardware implementation details that have no bearing on software interface (such as a single software-controlled clock being routed to different parts of a chip). Adding Rob to Cc, he will likely be able to clarify. > Now it's a bit hard to make > any definite statements about the Vivante GC GPU module itself, as most > of the information we have is from reverse engineering. It's pretty > clear though that the GPU module has at least 2 clock inputs: axi and > core, as there is a feature bit that tells us if it's okay to gate the > axi clock independently from core. > > I'm not 100% sure about the older cores as found in Dove, but all the > more recent cores allow to clock the shader partition independently of > the core partition, so that's another clock input. > > Now when it comes to a SoC integration, it's totally fine to have all > those GPU module clock inputs fed from the same clock
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
On Thu, May 14, 2020 at 09:53:08AM +0100, Russell King - ARM Linux admin wrote: > On Thu, May 14, 2020 at 10:40:58AM +0200, Lucas Stach wrote: > > Am Donnerstag, den 14.05.2020, 09:27 +0100 schrieb Russell King - ARM Linux > > admin: > > > On Thu, May 14, 2020 at 10:18:02AM +0200, Lucas Stach wrote: > > > > Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > > > > > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam > > > > > wrote: > > > > > > > > > > > The binding doc > > > > > > Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > > > > > says that only the 'reg' clock could be optional, the others are > > > > > > required. > > > > > > > > > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > > > > > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > > > > > > > > > Maybe the binding needs to be updated and it seems that using > > > > > devm_clk_get_optional() like you propose is safe. > > > > > > > > The binding is correct as-is. We want to require those clocks to be > > > > present, but the dove DT was added before the binding was finalized, so > > > > the driver still treats the clocks as optional to not break > > > > compatibility with old DTs. Maybe this warrants a comment in the > > > > code... > > > > > > The binding doc in mainline says: > > > > > > clocks: > > > items: > > > - description: AXI/master interface clock > > > - description: GPU core clock > > > - description: Shader clock (only required if GPU has feature > > > PIPE_3D) > > > - description: AHB/slave interface clock (only required if GPU can > > > gate slave interface independently) > > > minItems: 1 > > > maxItems: 4 > > > > > > clock-names: > > > items: > > > enum: [ bus, core, shader, reg ] > > > minItems: 1 > > > maxItems: 4 > > > > > > which looks correct to me - and means that Dove is compliant with that. > > > > The YAML binding actually did loose something in translation here, > > which I didn't notice. Previously all those clocks were listed under > > "Required properties", with the exceptions listed in parenthesis. So > > the Dove GPU, which is a combined 2D/3D core should have axi, core and > > shader clocks specified. > > That may be your desire, but that is impossible without knowing that > (a) it has the clocks > (b) what those clocks are connected to > > I guess we could "make something up" but as DT is supposed to describe > hardware, I don't see how we can satisfy that and your requirement. > > The only thing that is known from the documentation is that there is > one clock for the GPU on Dove. Yes. This means that in fact "core" is the only required clock for all implementations of vivante,gc and the common binding needs to be updated to reflect that. I'll follow with a patch that does that, unless there are strong objections. If there are implementations that require different clock inputs, then they need to use additional compatible string for the particular flavor and the binding should have conditionals for them. Something like this: if: properties: compatible: contains: const: fsl,imx6sx-gpu then: properties: clocks: minItems: 4 Lubo ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
On Wed, May 20, 2020 at 04:04:39PM +0200, Lucas Stach wrote: > Am Mittwoch, den 20.05.2020, 15:38 +0200 schrieb Lubomir Rintel: > > Yes. This means that in fact "core" is the only required clock for all > > implementations of vivante,gc and the common binding needs to be updated > > to reflect that. I'll follow with a patch that does that, unless there > > are strong objections. > > > > If there are implementations that require different clock inputs, then they > > need to use additional compatible string for the particular flavor and the > > binding should have conditionals for them. Something like this: > > > > if: > > properties: > > compatible: > > contains: > > const: fsl,imx6sx-gpu > > then: > > properties: > > clocks: > > minItems: 4 > > The DT binding of a device should describe the hardware of the device, > not the specific integration into a SoC. Now it's a bit hard to make > any definite statements about the Vivante GC GPU module itself, as most > of the information we have is from reverse engineering. It's pretty > clear though that the GPU module has at least 2 clock inputs: axi and > core, as there is a feature bit that tells us if it's okay to gate the > axi clock independently from core. > > I'm not 100% sure about the older cores as found in Dove, but all the > more recent cores allow to clock the shader partition independently of > the core partition, so that's another clock input. > > Now when it comes to a SoC integration, it's totally fine to have all > those GPU module clock inputs fed from the same clock source and behind > a shared gate maybe. But that doesn't change the clock inputs from the > device perspective, it's still 3 independent clock inputs, which then > just point to the same clock source in the DT. > > imx6sx.dtsi is even a precedent of such a setup: all module clock > inputs are fed by a common clock and share a single gate. It's very good to stand on a pedistal, and try to dictate what you want, but what you want is not always possible, and that is the case here. You have made your idea of how you see the hardware quite plain. Getting back to reality, again, what we know is that there is one clock for the GC600 on Dove, since are register settings to control that clock. What we also know is that there is an AHB clock, but how that relates to the rest of the system, we have no documentation. It is that is used for the AHB bus, which incidentally includes the GC600 for access to its register set. I don't remember whether AHB peripherals require the clock or not, that is something that would need to be checked. If they do, then it seems that clock is missing from the binding. Then there's a description of the AXI interface to the GC600 which mentions no clock (see 10.7.2 below). It has a clock, but what that clock is, and how it relates to the rest of the system is not clearly specified. So, again, what you're basically asking for is for someone to guess and throw some ficticious model into DT. No, if it's not known, then we should not be guessing and throwing random garbage into a SoC DT description - once it's in the DT description, it has to be supported going forward, and if it's later found to be incorrect, then we have a problem. So, I don't see how Dove can meet your requirements, and I think you have to compromise on this, and just accept that Dove only has one known clock. If you want to continue being pedantic about this, then in order to support that pedanty, we need to add an AHB clock to all Vivante GC descriptions as well since the AHB interface requires it - there it is called HCLK in the AHB specs. Here is the extract from the Dove documentation for the AHB interface: 10.7.1 AHB Interface The main features of the AHB interface are: n256k addressable register space n32-bit accesses only (no bursts) n32-bit data bus nError response for illegal accesses nAsynchronous interface to the Graphics Core nInterrupt support The interface uses a separate clock that is slower than the AXI Bus clock, thus allowing a more relaxed logic design. Because the Core clock is different from the interface clock, design within the GPU Core ensures that incoming and outgoing data are handled properly when they cross clock boundaries. The GPU block occupies 256 KB (64k 32-bit words) of the system address space. An AHB ERROR response is returned if an illegal access is detected. Only 32-bit reads and writes are permitted. 10.7.2 AXI Interface The main features of the AXI interface are:
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
Am Mittwoch, den 20.05.2020, 15:38 +0200 schrieb Lubomir Rintel: > On Thu, May 14, 2020 at 09:53:08AM +0100, Russell King - ARM Linux admin > wrote: > > On Thu, May 14, 2020 at 10:40:58AM +0200, Lucas Stach wrote: > > > Am Donnerstag, den 14.05.2020, 09:27 +0100 schrieb Russell King - ARM > > > Linux admin: > > > > On Thu, May 14, 2020 at 10:18:02AM +0200, Lucas Stach wrote: > > > > > Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > > > > > > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam > > > > > > wrote: > > > > > > > > > > > > > The binding doc > > > > > > > Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > > > > > > says that only the 'reg' clock could be optional, the others are > > > > > > > required. > > > > > > > > > > > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > > > > > > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > > > > > > > > > > > Maybe the binding needs to be updated and it seems that using > > > > > > devm_clk_get_optional() like you propose is safe. > > > > > > > > > > The binding is correct as-is. We want to require those clocks to be > > > > > present, but the dove DT was added before the binding was finalized, > > > > > so > > > > > the driver still treats the clocks as optional to not break > > > > > compatibility with old DTs. Maybe this warrants a comment in the > > > > > code... > > > > > > > > The binding doc in mainline says: > > > > > > > > clocks: > > > > items: > > > > - description: AXI/master interface clock > > > > - description: GPU core clock > > > > - description: Shader clock (only required if GPU has feature > > > > PIPE_3D) > > > > - description: AHB/slave interface clock (only required if GPU > > > > can gate slave interface independently) > > > > minItems: 1 > > > > maxItems: 4 > > > > > > > > clock-names: > > > > items: > > > > enum: [ bus, core, shader, reg ] > > > > minItems: 1 > > > > maxItems: 4 > > > > > > > > which looks correct to me - and means that Dove is compliant with that. > > > > > > The YAML binding actually did loose something in translation here, > > > which I didn't notice. Previously all those clocks were listed under > > > "Required properties", with the exceptions listed in parenthesis. So > > > the Dove GPU, which is a combined 2D/3D core should have axi, core and > > > shader clocks specified. > > > > That may be your desire, but that is impossible without knowing that > > (a) it has the clocks > > (b) what those clocks are connected to > > > > I guess we could "make something up" but as DT is supposed to describe > > hardware, I don't see how we can satisfy that and your requirement. > > > > The only thing that is known from the documentation is that there is > > one clock for the GPU on Dove. > > Yes. This means that in fact "core" is the only required clock for all > implementations of vivante,gc and the common binding needs to be updated > to reflect that. I'll follow with a patch that does that, unless there > are strong objections. > > If there are implementations that require different clock inputs, then they > need to use additional compatible string for the particular flavor and the > binding should have conditionals for them. Something like this: > > if: > properties: > compatible: > contains: > const: fsl,imx6sx-gpu > then: > properties: > clocks: > minItems: 4 The DT binding of a device should describe the hardware of the device, not the specific integration into a SoC. Now it's a bit hard to make any definite statements about the Vivante GC GPU module itself, as most of the information we have is from reverse engineering. It's pretty clear though that the GPU module has at least 2 clock inputs: axi and core, as there is a feature bit that tells us if it's okay to gate the axi clock independently from core. I'm not 100% sure about the older cores as found in Dove, but all the more recent cores allow to clock the shader partition independently of the core partition, so that's another clock input. Now when it comes to a SoC integration, it's totally fine to have all those GPU module clock inputs fed from the same clock source and behind a shared gate maybe. But that doesn't change the clock inputs from the device perspective, it's still 3 independent clock inputs, which then just point to the same clock source in the DT. imx6sx.dtsi is even a precedent of such a setup: all module clock inputs are fed by a common clock and share a single gate. Regards, Lucas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
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.
Signed-off-by: Lubomir Rintel
---
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 c6dacfe3d321..e7dbb924f576 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(>dev, "reg");
+ gpu->clk_reg = devm_clk_get_optional(>dev, "reg");
DBG("clk_reg: %p", gpu->clk_reg);
if (IS_ERR(gpu->clk_reg))
- gpu->clk_reg = NULL;
+ return err;
- gpu->clk_bus = devm_clk_get(>dev, "bus");
+ gpu->clk_bus = devm_clk_get_optional(>dev, "bus");
DBG("clk_bus: %p", gpu->clk_bus);
if (IS_ERR(gpu->clk_bus))
- gpu->clk_bus = NULL;
+ return err;
- gpu->clk_core = devm_clk_get(>dev, "core");
+ gpu->clk_core = devm_clk_get_optional(>dev, "core");
DBG("clk_core: %p", gpu->clk_core);
if (IS_ERR(gpu->clk_core))
- gpu->clk_core = NULL;
+ return err;
gpu->base_rate_core = clk_get_rate(gpu->clk_core);
- gpu->clk_shader = devm_clk_get(>dev, "shader");
+ gpu->clk_shader = devm_clk_get_optional(>dev, "shader");
DBG("clk_shader: %p", gpu->clk_shader);
if (IS_ERR(gpu->clk_shader))
- gpu->clk_shader = NULL;
+ return err;
gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
/* TODO: figure out max mapped size */
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
On Thu, May 14, 2020 at 10:40:58AM +0200, Lucas Stach wrote: > Am Donnerstag, den 14.05.2020, 09:27 +0100 schrieb Russell King - ARM Linux > admin: > > On Thu, May 14, 2020 at 10:18:02AM +0200, Lucas Stach wrote: > > > Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > > > > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam > > > > wrote: > > > > > > > > > The binding doc Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > > > > says that only the 'reg' clock could be optional, the others are > > > > > required. > > > > > > > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > > > > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > > > > > > > Maybe the binding needs to be updated and it seems that using > > > > devm_clk_get_optional() like you propose is safe. > > > > > > The binding is correct as-is. We want to require those clocks to be > > > present, but the dove DT was added before the binding was finalized, so > > > the driver still treats the clocks as optional to not break > > > compatibility with old DTs. Maybe this warrants a comment in the > > > code... > > > > The binding doc in mainline says: > > > > clocks: > > items: > > - description: AXI/master interface clock > > - description: GPU core clock > > - description: Shader clock (only required if GPU has feature PIPE_3D) > > - description: AHB/slave interface clock (only required if GPU can > > gate slave interface independently) > > minItems: 1 > > maxItems: 4 > > > > clock-names: > > items: > > enum: [ bus, core, shader, reg ] > > minItems: 1 > > maxItems: 4 > > > > which looks correct to me - and means that Dove is compliant with that. > > The YAML binding actually did loose something in translation here, > which I didn't notice. Previously all those clocks were listed under > "Required properties", with the exceptions listed in parenthesis. So > the Dove GPU, which is a combined 2D/3D core should have axi, core and > shader clocks specified. That may be your desire, but that is impossible without knowing that (a) it has the clocks (b) what those clocks are connected to I guess we could "make something up" but as DT is supposed to describe hardware, I don't see how we can satisfy that and your requirement. The only thing that is known from the documentation is that there is one clock for the GPU on Dove. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
On Thu, May 14, 2020 at 10:18:02AM +0200, Lucas Stach wrote: > Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam wrote: > > > > > The binding doc Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > > says that only the 'reg' clock could be optional, the others are > > > required. > > > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > > > Maybe the binding needs to be updated and it seems that using > > devm_clk_get_optional() like you propose is safe. > > The binding is correct as-is. We want to require those clocks to be > present, but the dove DT was added before the binding was finalized, so > the driver still treats the clocks as optional to not break > compatibility with old DTs. Maybe this warrants a comment in the > code... The binding doc in mainline says: clocks: items: - description: AXI/master interface clock - description: GPU core clock - description: Shader clock (only required if GPU has feature PIPE_3D) - description: AHB/slave interface clock (only required if GPU can gate slave interface independently) minItems: 1 maxItems: 4 clock-names: items: enum: [ bus, core, shader, reg ] minItems: 1 maxItems: 4 which looks correct to me - and means that Dove is compliant with that. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
Am Donnerstag, den 14.05.2020, 09:27 +0100 schrieb Russell King - ARM Linux admin: > On Thu, May 14, 2020 at 10:18:02AM +0200, Lucas Stach wrote: > > Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > > > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam wrote: > > > > > > > The binding doc Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > > > says that only the 'reg' clock could be optional, the others are > > > > required. > > > > > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > > > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > > > > > Maybe the binding needs to be updated and it seems that using > > > devm_clk_get_optional() like you propose is safe. > > > > The binding is correct as-is. We want to require those clocks to be > > present, but the dove DT was added before the binding was finalized, so > > the driver still treats the clocks as optional to not break > > compatibility with old DTs. Maybe this warrants a comment in the > > code... > > The binding doc in mainline says: > > clocks: > items: > - description: AXI/master interface clock > - description: GPU core clock > - description: Shader clock (only required if GPU has feature PIPE_3D) > - description: AHB/slave interface clock (only required if GPU can gate > slave interface independently) > minItems: 1 > maxItems: 4 > > clock-names: > items: > enum: [ bus, core, shader, reg ] > minItems: 1 > maxItems: 4 > > which looks correct to me - and means that Dove is compliant with that. The YAML binding actually did loose something in translation here, which I didn't notice. Previously all those clocks were listed under "Required properties", with the exceptions listed in parenthesis. So the Dove GPU, which is a combined 2D/3D core should have axi, core and shader clocks specified. Regards, Lucas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam wrote: > > > The binding doc Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > says that only the 'reg' clock could be optional, the others are > > required. > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > Maybe the binding needs to be updated and it seems that using > devm_clk_get_optional() like you propose is safe. The binding is correct as-is. We want to require those clocks to be present, but the dove DT was added before the binding was finalized, so the driver still treats the clocks as optional to not break compatibility with old DTs. Maybe this warrants a comment in the code... Regards, Lucas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
On Wed, May 13, 2020 at 2:09 PM Fabio Estevam wrote: > The binding doc Documentation/devicetree/bindings/gpu/vivante,gc.yaml > says that only the 'reg' clock could be optional, the others are > required. arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' Maybe the binding needs to be updated and it seems that using devm_clk_get_optional() like you propose is safe. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
On Wed, May 13, 2020 at 12:08 PM Lubomir Rintel wrote:
>
> 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.
>
> Signed-off-by: Lubomir Rintel
> ---
> 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 c6dacfe3d321..e7dbb924f576 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(>dev, "reg");
> + gpu->clk_reg = devm_clk_get_optional(>dev, "reg");
> DBG("clk_reg: %p", gpu->clk_reg);
> if (IS_ERR(gpu->clk_reg))
> - gpu->clk_reg = NULL;
> + return err;
>
> - gpu->clk_bus = devm_clk_get(>dev, "bus");
> + gpu->clk_bus = devm_clk_get_optional(>dev, "bus");
The binding doc Documentation/devicetree/bindings/gpu/vivante,gc.yaml
says that only the 'reg' clock could be optional, the others are
required.
> DBG("clk_bus: %p", gpu->clk_bus);
> if (IS_ERR(gpu->clk_bus))
> - gpu->clk_bus = NULL;
> + return err;
>
> - gpu->clk_core = devm_clk_get(>dev, "core");
> + gpu->clk_core = devm_clk_get_optional(>dev, "core");
> DBG("clk_core: %p", gpu->clk_core);
> if (IS_ERR(gpu->clk_core))
> - gpu->clk_core = NULL;
> + return err;
> gpu->base_rate_core = clk_get_rate(gpu->clk_core);
>
> - gpu->clk_shader = devm_clk_get(>dev, "shader");
> + gpu->clk_shader = devm_clk_get_optional(>dev, "shader");
> DBG("clk_shader: %p", gpu->clk_shader);
> if (IS_ERR(gpu->clk_shader))
> - gpu->clk_shader = NULL;
> + return err;
> gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
>
> /* TODO: figure out max mapped size */
> --
> 2.26.2
>
> ___
> etnaviv mailing list
> etna...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/etnaviv
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
Hi Lubomir,
On Wed, May 13, 2020 at 12:08 PM Lubomir Rintel wrote:
> /* Get Clocks: */
> - gpu->clk_reg = devm_clk_get(>dev, "reg");
> + gpu->clk_reg = devm_clk_get_optional(>dev, "reg");
> DBG("clk_reg: %p", gpu->clk_reg);
> if (IS_ERR(gpu->clk_reg))
> - gpu->clk_reg = NULL;
> + return err;
You should return PTR_ERR(gpu->clk_reg) instead.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
