[PATCH 0/2] iommu/rockchip: fix clock handling to not break old dts

[PATCH 0/2] iommu/rockchip: fix clock handling to not break old dts

[Heiko Stuebner]

The newly added clock handling makes iommu clocks required, thus breaking
devicetrees from the last 4 years.

This series fixes the binding and driver to work without clocks being
present, like it did before and should therefore probably go in as fix
on top of the clock-handling patch.


Heiko Stuebner (2):
  dt-bindings: iommu/rockchip: Make clock properties optional
  iommu/rockchip: make clock handling optional

 Documentation/devicetree/bindings/iommu/rockchip,iommu.txt | 4 ++--
 drivers/iommu/rockchip-iommu.c                             | 8 ++++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.16.2

[PATCH 1/2] dt-bindings: iommu/rockchip: Make clock properties optional

[Heiko Stuebner]

Rockchip IOMMUs are used without explicit clock handling for 4 years
now, so we should keep compatibility with old devicetrees if possible.
Therefore make iommu clocks optional.

Fixes: 8fa9eb39c614 ("dt-bindings: iommu/rockchip: Add clock property")
Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 Documentation/devicetree/bindings/iommu/rockchip,iommu.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
index 6ecefea1c6f9..25bfad987513 100644
--- a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
@@ -14,13 +14,13 @@ Required properties:
                     "single-master" device, and needs no additional information
                     to associate with its master device.  See:
                     Documentation/devicetree/bindings/iommu/iommu.txt
+
+Optional properties:
 - clocks          : A list of clocks required for the IOMMU to be accessible by
                     the host CPU.
 - clock-names     : Should contain the following:
 	"iface" - Main peripheral bus clock (PCLK/HCL) (required)
 	"aclk"  - AXI bus clock (required)
-
-Optional properties:
 - rockchip,disable-mmu-reset : Don't use the mmu reset operation.
 			       Some mmu instances may produce unexpected results
 			       when the reset operation is used.
-- 
2.16.2

Re: [PATCH 1/2] dt-bindings: iommu/rockchip: Make clock properties optional

[Robin Murphy]

On 10/04/18 10:26, Heiko Stuebner wrote:
> Rockchip IOMMUs are used without explicit clock handling for 4 years
> now, so we should keep compatibility with old devicetrees if possible.
> Therefore make iommu clocks optional.

Do we need to touch the binding itself? Obviously the driver has to 
treat clocks as optional in existing DTs (and I feel a bit dumb now for 
managing to overlook that in review), but the binding effectively only 
covers future DTs, and I'd assume we want to encourage the clocks to be 
correctly specified there.

Robin.

> Fixes: 8fa9eb39c614 ("dt-bindings: iommu/rockchip: Add clock property")
> Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
>   Documentation/devicetree/bindings/iommu/rockchip,iommu.txt | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
> index 6ecefea1c6f9..25bfad987513 100644
> --- a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
> +++ b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
> @@ -14,13 +14,13 @@ Required properties:
>                       "single-master" device, and needs no additional information
>                       to associate with its master device.  See:
>                       Documentation/devicetree/bindings/iommu/iommu.txt
> +
> +Optional properties:
>   - clocks          : A list of clocks required for the IOMMU to be accessible by
>                       the host CPU.
>   - clock-names     : Should contain the following:
>   	"iface" - Main peripheral bus clock (PCLK/HCL) (required)
>   	"aclk"  - AXI bus clock (required)
> -
> -Optional properties:
>   - rockchip,disable-mmu-reset : Don't use the mmu reset operation.
>   			       Some mmu instances may produce unexpected results
>   			       when the reset operation is used.
> 

Re: [PATCH 1/2] dt-bindings: iommu/rockchip: Make clock properties optional

[Heiko Stuebner]

Hi Robin,

Am Dienstag, 10. April 2018, 13:18:48 CEST schrieb Robin Murphy:
> On 10/04/18 10:26, Heiko Stuebner wrote:
> > Rockchip IOMMUs are used without explicit clock handling for 4 years
> > now, so we should keep compatibility with old devicetrees if possible.
> > Therefore make iommu clocks optional.
> 
> Do we need to touch the binding itself? Obviously the driver has to 
> treat clocks as optional in existing DTs (and I feel a bit dumb now for 
> managing to overlook that in review), but the binding effectively only 
> covers future DTs, and I'd assume we want to encourage the clocks to be 
> correctly specified there.

I guess that depends on your definition of the timespan for backwards
compatibility. I'm always starting out at indefinite till convinced
otherwise ;-). Hence the clocks would need to stay optional for (nearly)
forever.

Also, having the binding claim them as required but the code handling
them as optional just calls for someone to remove the optional handling :-D

Not sure if there is a established way of saying
"we want this for all future devices, but allow it to be missing for old dts".


Heiko

Re: [PATCH 1/2] dt-bindings: iommu/rockchip: Make clock properties optional

[Rob Herring]

On Tue, Apr 10, 2018 at 01:26:41PM +0200, Heiko Stuebner wrote:
> Hi Robin,
> 
> Am Dienstag, 10. April 2018, 13:18:48 CEST schrieb Robin Murphy:
> > On 10/04/18 10:26, Heiko Stuebner wrote:
> > > Rockchip IOMMUs are used without explicit clock handling for 4 years
> > > now, so we should keep compatibility with old devicetrees if possible.
> > > Therefore make iommu clocks optional.
> > 
> > Do we need to touch the binding itself? Obviously the driver has to 
> > treat clocks as optional in existing DTs (and I feel a bit dumb now for 
> > managing to overlook that in review), but the binding effectively only 
> > covers future DTs, and I'd assume we want to encourage the clocks to be 
> > correctly specified there.

I'd prefer the DT docs reflect what is correct for new/current dts 
files. That's the only way the docs can validate the dts files.

> I guess that depends on your definition of the timespan for backwards
> compatibility. I'm always starting out at indefinite till convinced
> otherwise ;-). Hence the clocks would need to stay optional for (nearly)
> forever.
> 
> Also, having the binding claim them as required but the code handling
> them as optional just calls for someone to remove the optional handling :-D

A comment in the code saying why missing clocks are allowed should 
suffice.

> Not sure if there is a established way of saying
> "we want this for all future devices, but allow it to be missing for old dts".

We don't really...

Rob

Re: [PATCH 1/2] dt-bindings: iommu/rockchip: Make clock properties optional

[Heiko Stuebner]

Am Freitag, 13. April 2018, 23:02:26 CEST schrieb Rob Herring:
> On Tue, Apr 10, 2018 at 01:26:41PM +0200, Heiko Stuebner wrote:
> > Hi Robin,
> > 
> > Am Dienstag, 10. April 2018, 13:18:48 CEST schrieb Robin Murphy:
> > > On 10/04/18 10:26, Heiko Stuebner wrote:
> > > > Rockchip IOMMUs are used without explicit clock handling for 4 years
> > > > now, so we should keep compatibility with old devicetrees if possible.
> > > > Therefore make iommu clocks optional.
> > > 
> > > Do we need to touch the binding itself? Obviously the driver has to 
> > > treat clocks as optional in existing DTs (and I feel a bit dumb now for 
> > > managing to overlook that in review), but the binding effectively only 
> > > covers future DTs, and I'd assume we want to encourage the clocks to be 
> > > correctly specified there.
> 
> I'd prefer the DT docs reflect what is correct for new/current dts 
> files. That's the only way the docs can validate the dts files.
> 
> > I guess that depends on your definition of the timespan for backwards
> > compatibility. I'm always starting out at indefinite till convinced
> > otherwise ;-). Hence the clocks would need to stay optional for (nearly)
> > forever.
> > 
> > Also, having the binding claim them as required but the code handling
> > them as optional just calls for someone to remove the optional handling :-D
> 
> A comment in the code saying why missing clocks are allowed should 
> suffice.
> 
> > Not sure if there is a established way of saying
> > "we want this for all future devices, but allow it to be missing for old dts".
> 
> We don't really...

Ok, I'll drop the binding change and add a code comment then.


Heiko

[PATCH 2/2] iommu/rockchip: make clock handling optional

[Heiko Stuebner]

iommu clocks are optional, so the driver should not fail if they are not
present. Instead just set the number of clocks to 0, which the clk-blk APIs
can handle just fine.

Fixes: f2e3a5f557ad ("iommu/rockchip: Control clocks needed to access the IOMMU")
Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 drivers/iommu/rockchip-iommu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 5fc8656c60f9..7215c683cb8f 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1176,8 +1176,12 @@ static int rk_iommu_probe(struct platform_device *pdev)
 		iommu->clocks[i].id = rk_iommu_clocks[i];
 
 	err = devm_clk_bulk_get(iommu->dev, iommu->num_clocks, iommu->clocks);
-	if (err)
-		return err;
+	if (err) {
+		if (err == -ENOENT)
+			iommu->num_clocks = 0;
+		else
+			return err;
+	}
 
 	err = clk_bulk_prepare(iommu->num_clocks, iommu->clocks);
 	if (err)
-- 
2.16.2

Re: [PATCH 2/2] iommu/rockchip: make clock handling optional

[Robin Murphy]

On 10/04/18 10:26, Heiko Stuebner wrote:
> iommu clocks are optional, so the driver should not fail if they are not
> present. Instead just set the number of clocks to 0, which the clk-blk APIs
> can handle just fine.
> 
> Fixes: f2e3a5f557ad ("iommu/rockchip: Control clocks needed to access the IOMMU")
> Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
>   drivers/iommu/rockchip-iommu.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 5fc8656c60f9..7215c683cb8f 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -1176,8 +1176,12 @@ static int rk_iommu_probe(struct platform_device *pdev)
>   		iommu->clocks[i].id = rk_iommu_clocks[i];
>   
>   	err = devm_clk_bulk_get(iommu->dev, iommu->num_clocks, iommu->clocks);
> -	if (err)
> -		return err;
> +	if (err) {
> +		if (err == -ENOENT)
> +			iommu->num_clocks = 0;
> +		else
> +			return err;
> +	}

Nit: this might be a bit nicer as:

	if (err == -ENOENT)
		iommu->num_clocks = 0;
	else if (err)
		return err;

Either way,

Reviewed-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>

Thanks,
Robin.