| Message ID | 20210203090727.789939-2-zhang.lyra@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Unisoc iommu basic driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote: > From: Chunyan Zhang <chunyan.zhang@unisoc.com> > > This iommu module can be used by Unisoc's multimedia devices, such as > display, Image codec(jpeg) and a few signal processors, including > VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc. > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > --- > .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++ > 1 file changed, 72 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > > diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > new file mode 100644 > index 000000000000..4fc99e81fa66 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > @@ -0,0 +1,72 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright 2020 Unisoc Inc. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iommu/sprd,iommu.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Unisoc IOMMU and Multi-media MMU > + > +maintainers: > + - Chunyan Zhang <zhang.lyra@gmail.com> > + > +properties: > + compatible: > + enum: > + - sprd,iommu-v1 > + > + "#iommu-cells": > + const: 0 > + description: > + Unisoc IOMMUs are all single-master IOMMU devices, therefore no > + additional information needs to associate with its master device. > + Please refer to the generic bindings document for more details, > + Documentation/devicetree/bindings/iommu/iommu.txt > + > + reg: > + maxItems: 1 > + description: > + Not required if 'sprd,iommu-regs' is defined. > + > + clocks: > + description: > + Reference to a gate clock phandle, since access to some of IOMMUs are > + controlled by gate clock, but this is not required. > + > + sprd,iommu-regs: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: > + Reference to a syscon phandle plus 1 cell, the syscon defines the > + register range used by the iommu and the media device, the cell > + defines the offset for iommu registers. Since iommu module shares > + the same register range with the media device which uses it. > + > +required: > + - compatible > + - "#iommu-cells" > + > +oneOf: > + - required: > + - reg > + - required: > + - sprd,iommu-regs > + > +additionalProperties: false > + > +examples: > + - | > + iommu_disp: iommu-disp { > + compatible = "sprd,iommu-v1"; > + sprd,iommu-regs = <&dpu_regs 0x800>; If the IOMMU is contained within another device, then it should just be a child node of that device. Or just make 'dpu_regs' an IOMMU provider (i.e. just add #iommu-cells to it). > + #iommu-cells = <0>; > + }; > + > + - | > + iommu_jpg: iommu-jpg { > + compatible = "sprd,iommu-v1"; > + sprd,iommu-regs = <&jpg_regs 0x300>; > + #iommu-cells = <0>; > + clocks = <&mm_gate 1>; > + }; > + > +... > -- > 2.25.1 >
Hi Rob, On Fri, 5 Feb 2021 at 07:25, Rob Herring <robh@kernel.org> wrote: > > On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote: > > From: Chunyan Zhang <chunyan.zhang@unisoc.com> > > > > This iommu module can be used by Unisoc's multimedia devices, such as > > display, Image codec(jpeg) and a few signal processors, including > > VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc. > > > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > > --- > > .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++ > > 1 file changed, 72 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > > > > diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > > new file mode 100644 > > index 000000000000..4fc99e81fa66 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > > @@ -0,0 +1,72 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +# Copyright 2020 Unisoc Inc. > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iommu/sprd,iommu.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Unisoc IOMMU and Multi-media MMU > > + > > +maintainers: > > + - Chunyan Zhang <zhang.lyra@gmail.com> > > + > > +properties: > > + compatible: > > + enum: > > + - sprd,iommu-v1 > > + > > + "#iommu-cells": > > + const: 0 > > + description: > > + Unisoc IOMMUs are all single-master IOMMU devices, therefore no > > + additional information needs to associate with its master device. > > + Please refer to the generic bindings document for more details, > > + Documentation/devicetree/bindings/iommu/iommu.txt > > + > > + reg: > > + maxItems: 1 > > + description: > > + Not required if 'sprd,iommu-regs' is defined. > > + > > + clocks: > > + description: > > + Reference to a gate clock phandle, since access to some of IOMMUs are > > + controlled by gate clock, but this is not required. > > + > > + sprd,iommu-regs: > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > + description: > > + Reference to a syscon phandle plus 1 cell, the syscon defines the > > + register range used by the iommu and the media device, the cell > > + defines the offset for iommu registers. Since iommu module shares > > + the same register range with the media device which uses it. > > + > > +required: > > + - compatible > > + - "#iommu-cells" > > + > > +oneOf: > > + - required: > > + - reg > > + - required: > > + - sprd,iommu-regs > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + iommu_disp: iommu-disp { > > + compatible = "sprd,iommu-v1"; > > + sprd,iommu-regs = <&dpu_regs 0x800>; > > If the IOMMU is contained within another device, then it should just be > a child node of that device. Yes, actually IOMMU can be seen as a child of multimedia devices, I considered moving IOMMU under into multimedia device node, but multimedia devices need IOMMU when probe[1], so I dropped that idea. And they share the same register base, e.g. + mm { + compatible = "simple-bus"; + #address-cells = <2>; + #size-cells = <2>; + ranges; + + dpu_regs: syscon@63000000 { + compatible = "sprd,sc9863a-dpuregs", "syscon"; + reg = <0 0x63000000 0 0x1000>; + }; + + dpu: dpu@63000000 { + compatible = "sprd,sharkl3-dpu"; + sprd,disp-regs = <&dpu_regs>; + iommus = <&iommu_dispc>; + }; + + iommu_dispc: iommu@63000000 { + compatible = "sprd,iommu-v1"; + sprd,iommu-regs = <&dpu_regs 0x800>; + #iommu-cells = <0>; + }; DPU use the registers from 0, IOMMU from 0x800, the purpose of using syscon node was to avoid remapping register physical address. > Or just make 'dpu_regs' an IOMMU provider > (i.e. just add #iommu-cells to it). xxx_regs(syscon node) defines the register range for IOMMU and a multimedia device (such as dpu, image codec, etc.) Hope I've explained the relationship of xxx_regs, multimedia device, and iommu clearly :) Any suggestion for this kind of cases? Thanks, Chunyan [1] https://elixir.bootlin.com/linux/v5.11-rc6/source/drivers/iommu/of_iommu.c#L145 > > > + #iommu-cells = <0>; > > + }; > > + > > + - | > > + iommu_jpg: iommu-jpg { > > + compatible = "sprd,iommu-v1"; > > + sprd,iommu-regs = <&jpg_regs 0x300>; > > + #iommu-cells = <0>; > > + clocks = <&mm_gate 1>; > > + }; > > + > > +... > > -- > > 2.25.1 > >
On Fri, Feb 5, 2021 at 1:21 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote: > > Hi Rob, > > On Fri, 5 Feb 2021 at 07:25, Rob Herring <robh@kernel.org> wrote: > > > > On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote: > > > From: Chunyan Zhang <chunyan.zhang@unisoc.com> > > > > > > This iommu module can be used by Unisoc's multimedia devices, such as > > > display, Image codec(jpeg) and a few signal processors, including > > > VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc. > > > > > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > > > --- > > > .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++ > > > 1 file changed, 72 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > > > new file mode 100644 > > > index 000000000000..4fc99e81fa66 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > > > @@ -0,0 +1,72 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +# Copyright 2020 Unisoc Inc. > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/iommu/sprd,iommu.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Unisoc IOMMU and Multi-media MMU > > > + > > > +maintainers: > > > + - Chunyan Zhang <zhang.lyra@gmail.com> > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - sprd,iommu-v1 > > > + > > > + "#iommu-cells": > > > + const: 0 > > > + description: > > > + Unisoc IOMMUs are all single-master IOMMU devices, therefore no > > > + additional information needs to associate with its master device. > > > + Please refer to the generic bindings document for more details, > > > + Documentation/devicetree/bindings/iommu/iommu.txt > > > + > > > + reg: > > > + maxItems: 1 > > > + description: > > > + Not required if 'sprd,iommu-regs' is defined. > > > + > > > + clocks: > > > + description: > > > + Reference to a gate clock phandle, since access to some of IOMMUs are > > > + controlled by gate clock, but this is not required. > > > + > > > + sprd,iommu-regs: > > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > > + description: > > > + Reference to a syscon phandle plus 1 cell, the syscon defines the > > > + register range used by the iommu and the media device, the cell > > > + defines the offset for iommu registers. Since iommu module shares > > > + the same register range with the media device which uses it. > > > + > > > +required: > > > + - compatible > > > + - "#iommu-cells" > > > + > > > +oneOf: > > > + - required: > > > + - reg > > > + - required: > > > + - sprd,iommu-regs > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + iommu_disp: iommu-disp { > > > + compatible = "sprd,iommu-v1"; > > > + sprd,iommu-regs = <&dpu_regs 0x800>; > > > > If the IOMMU is contained within another device, then it should just be > > a child node of that device. > > Yes, actually IOMMU can be seen as a child of multimedia devices, I > considered moving IOMMU under into multimedia device node, but > multimedia devices need IOMMU when probe[1], so I dropped that idea. Don't design your binding around working-around linux issues. > And they share the same register base, e.g. > > + mm { > + compatible = "simple-bus"; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + dpu_regs: syscon@63000000 { Drop this node. > + compatible = "sprd,sc9863a-dpuregs", "syscon"; > + reg = <0 0x63000000 0 0x1000>; > + }; > + > + dpu: dpu@63000000 { > + compatible = "sprd,sharkl3-dpu"; > + sprd,disp-regs = <&dpu_regs>; reg = <0 0x63000000 0 0x800>; > + iommus = <&iommu_dispc>; > + }; > + > + iommu_dispc: iommu@63000000 { > + compatible = "sprd,iommu-v1"; > + sprd,iommu-regs = <&dpu_regs 0x800>; reg = <0 0x63000800 0 0x800>; > + #iommu-cells = <0>; Though given it seems there is only 1 client and this might really be just 1 h/w block, you don't really need to use the iommu binding at all. The DPU should be able to instantiate it's own IOMMU device. There's other examples of this such as mali GPU though that is all one driver, but that's a Linux implementation detail. Rob
On 2021-02-10 19:21, Rob Herring wrote: > On Fri, Feb 5, 2021 at 1:21 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote: >> >> Hi Rob, >> >> On Fri, 5 Feb 2021 at 07:25, Rob Herring <robh@kernel.org> wrote: >>> >>> On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote: >>>> From: Chunyan Zhang <chunyan.zhang@unisoc.com> >>>> >>>> This iommu module can be used by Unisoc's multimedia devices, such as >>>> display, Image codec(jpeg) and a few signal processors, including >>>> VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc. >>>> >>>> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> >>>> --- >>>> .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++ >>>> 1 file changed, 72 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml >>>> new file mode 100644 >>>> index 000000000000..4fc99e81fa66 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml >>>> @@ -0,0 +1,72 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +# Copyright 2020 Unisoc Inc. >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/iommu/sprd,iommu.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: Unisoc IOMMU and Multi-media MMU >>>> + >>>> +maintainers: >>>> + - Chunyan Zhang <zhang.lyra@gmail.com> >>>> + >>>> +properties: >>>> + compatible: >>>> + enum: >>>> + - sprd,iommu-v1 >>>> + >>>> + "#iommu-cells": >>>> + const: 0 >>>> + description: >>>> + Unisoc IOMMUs are all single-master IOMMU devices, therefore no >>>> + additional information needs to associate with its master device. >>>> + Please refer to the generic bindings document for more details, >>>> + Documentation/devicetree/bindings/iommu/iommu.txt >>>> + >>>> + reg: >>>> + maxItems: 1 >>>> + description: >>>> + Not required if 'sprd,iommu-regs' is defined. >>>> + >>>> + clocks: >>>> + description: >>>> + Reference to a gate clock phandle, since access to some of IOMMUs are >>>> + controlled by gate clock, but this is not required. >>>> + >>>> + sprd,iommu-regs: >>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>> + description: >>>> + Reference to a syscon phandle plus 1 cell, the syscon defines the >>>> + register range used by the iommu and the media device, the cell >>>> + defines the offset for iommu registers. Since iommu module shares >>>> + the same register range with the media device which uses it. >>>> + >>>> +required: >>>> + - compatible >>>> + - "#iommu-cells" OK, so apparently the hardware is not quite as trivial as my initial impression, and you should have interrupts as well. >>>> + >>>> +oneOf: >>>> + - required: >>>> + - reg >>>> + - required: >>>> + - sprd,iommu-regs >>>> + >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + iommu_disp: iommu-disp { >>>> + compatible = "sprd,iommu-v1"; >>>> + sprd,iommu-regs = <&dpu_regs 0x800>; >>> >>> If the IOMMU is contained within another device, then it should just be >>> a child node of that device. >> >> Yes, actually IOMMU can be seen as a child of multimedia devices, I >> considered moving IOMMU under into multimedia device node, but >> multimedia devices need IOMMU when probe[1], so I dropped that idea. > > Don't design your binding around working-around linux issues. Having stumbled across the DRM driver patches the other day, I now see where this is coming from, and it's even worse than that - this whole binding seems to be largely working around bad driver design. >> And they share the same register base, e.g. >> >> + mm { >> + compatible = "simple-bus"; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; >> + >> + dpu_regs: syscon@63000000 { > > Drop this node. > >> + compatible = "sprd,sc9863a-dpuregs", "syscon"; >> + reg = <0 0x63000000 0 0x1000>; >> + }; >> + >> + dpu: dpu@63000000 { >> + compatible = "sprd,sharkl3-dpu"; >> + sprd,disp-regs = <&dpu_regs>; > > reg = <0 0x63000000 0 0x800>; In fact judging by the other driver it looks like the length only needs to be 0x200 here (but maybe there's more to come in future). >> + iommus = <&iommu_dispc>; >> + }; >> + >> + iommu_dispc: iommu@63000000 { >> + compatible = "sprd,iommu-v1"; >> + sprd,iommu-regs = <&dpu_regs 0x800>; > > reg = <0 0x63000800 0 0x800>; ...and this one looks to need less than 0x80, even :) > >> + #iommu-cells = <0>; > > Though given it seems there is only 1 client and this might really be > just 1 h/w block, you don't really need to use the iommu binding at > all. The DPU should be able to instantiate it's own IOMMU device. > There's other examples of this such as mali GPU though that is all one > driver, but that's a Linux implementation detail. FWIW that's really a very different situation - the MMUs in a Mali GPU are fundamental parts of its internal pipelines and would never make sense to handle as separate devices (if it were even feasible to try). An IOMMU like this one is typically a logically-distinct block stuck to the external bus interface of any old device, rewriting transactions that said device has already issued - it's telling that it needs to allocate the prot_page scratchpad for "faulting" transactions to still flow somewhere, implying that it's not even involved enough to be able to terminate them. As such I think it *does* make complete sense to describe even "dedicated" IOMMUs like this one, Rockchip, Exynos, etc. in DT. Otherwise you'd be effectively forcing OSes to turn half their display/media drivers into mini board files with secret knowledge of which blocks are integrated with IOMMUs on which SoCs. Robin.
On Thu, 11 Feb 2021 at 03:21, Rob Herring <robh@kernel.org> wrote: > > On Fri, Feb 5, 2021 at 1:21 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote: > > > > Hi Rob, > > > > On Fri, 5 Feb 2021 at 07:25, Rob Herring <robh@kernel.org> wrote: > > > > > > On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote: > > > > From: Chunyan Zhang <chunyan.zhang@unisoc.com> > > > > > > > > This iommu module can be used by Unisoc's multimedia devices, such as > > > > display, Image codec(jpeg) and a few signal processors, including > > > > VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc. > > > > > > > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > > > > --- > > > > .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++ > > > > 1 file changed, 72 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > > > > new file mode 100644 > > > > index 000000000000..4fc99e81fa66 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > > > > @@ -0,0 +1,72 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +# Copyright 2020 Unisoc Inc. > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/iommu/sprd,iommu.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Unisoc IOMMU and Multi-media MMU > > > > + > > > > +maintainers: > > > > + - Chunyan Zhang <zhang.lyra@gmail.com> > > > > + > > > > +properties: > > > > + compatible: > > > > + enum: > > > > + - sprd,iommu-v1 > > > > + > > > > + "#iommu-cells": > > > > + const: 0 > > > > + description: > > > > + Unisoc IOMMUs are all single-master IOMMU devices, therefore no > > > > + additional information needs to associate with its master device. > > > > + Please refer to the generic bindings document for more details, > > > > + Documentation/devicetree/bindings/iommu/iommu.txt > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + description: > > > > + Not required if 'sprd,iommu-regs' is defined. > > > > + > > > > + clocks: > > > > + description: > > > > + Reference to a gate clock phandle, since access to some of IOMMUs are > > > > + controlled by gate clock, but this is not required. > > > > + > > > > + sprd,iommu-regs: > > > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > > > + description: > > > > + Reference to a syscon phandle plus 1 cell, the syscon defines the > > > > + register range used by the iommu and the media device, the cell > > > > + defines the offset for iommu registers. Since iommu module shares > > > > + the same register range with the media device which uses it. > > > > + > > > > +required: > > > > + - compatible > > > > + - "#iommu-cells" > > > > + > > > > +oneOf: > > > > + - required: > > > > + - reg > > > > + - required: > > > > + - sprd,iommu-regs > > > > + > > > > +additionalProperties: false > > > > + > > > > +examples: > > > > + - | > > > > + iommu_disp: iommu-disp { > > > > + compatible = "sprd,iommu-v1"; > > > > + sprd,iommu-regs = <&dpu_regs 0x800>; > > > > > > If the IOMMU is contained within another device, then it should just be > > > a child node of that device. > > > > Yes, actually IOMMU can be seen as a child of multimedia devices, I > > considered moving IOMMU under into multimedia device node, but > > multimedia devices need IOMMU when probe[1], so I dropped that idea. > > Don't design your binding around working-around linux issues. > > > And they share the same register base, e.g. > > > > + mm { > > + compatible = "simple-bus"; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + ranges; > > + > > + dpu_regs: syscon@63000000 { > > Drop this node. > > > + compatible = "sprd,sc9863a-dpuregs", "syscon"; > > + reg = <0 0x63000000 0 0x1000>; > > + }; > > + > > + dpu: dpu@63000000 { > > + compatible = "sprd,sharkl3-dpu"; > > + sprd,disp-regs = <&dpu_regs>; > > reg = <0 0x63000000 0 0x800>; > > > + iommus = <&iommu_dispc>; > > + }; > > + > > + iommu_dispc: iommu@63000000 { > > + compatible = "sprd,iommu-v1"; > > + sprd,iommu-regs = <&dpu_regs 0x800>; > > reg = <0 0x63000800 0 0x800>; Alright, considering you deprecate using syscon to map registers here, I will drop that. But that would cause the same physical address to be mapped two times at least. And this is not a single case, since there are a few media devices and their IOMMUs which all have this issue. Thanks, Chunyan > > > + #iommu-cells = <0>; > > Though given it seems there is only 1 client and this might really be > just 1 h/w block, you don't really need to use the iommu binding at > all. The DPU should be able to instantiate it's own IOMMU device. > There's other examples of this such as mali GPU though that is all one > driver, but that's a Linux implementation detail. > > Rob
On Tue, 16 Feb 2021 at 23:10, Robin Murphy <robin.murphy@arm.com> wrote: > > On 2021-02-10 19:21, Rob Herring wrote: > > On Fri, Feb 5, 2021 at 1:21 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote: > >> > >> Hi Rob, > >> > >> On Fri, 5 Feb 2021 at 07:25, Rob Herring <robh@kernel.org> wrote: > >>> > >>> On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote: > >>>> From: Chunyan Zhang <chunyan.zhang@unisoc.com> > >>>> > >>>> This iommu module can be used by Unisoc's multimedia devices, such as > >>>> display, Image codec(jpeg) and a few signal processors, including > >>>> VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc. > >>>> > >>>> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > >>>> --- > >>>> .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++ > >>>> 1 file changed, 72 insertions(+) > >>>> create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > >>>> new file mode 100644 > >>>> index 000000000000..4fc99e81fa66 > >>>> --- /dev/null > >>>> +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > >>>> @@ -0,0 +1,72 @@ > >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>>> +# Copyright 2020 Unisoc Inc. > >>>> +%YAML 1.2 > >>>> +--- > >>>> +$id: http://devicetree.org/schemas/iommu/sprd,iommu.yaml# > >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>>> + > >>>> +title: Unisoc IOMMU and Multi-media MMU > >>>> + > >>>> +maintainers: > >>>> + - Chunyan Zhang <zhang.lyra@gmail.com> > >>>> + > >>>> +properties: > >>>> + compatible: > >>>> + enum: > >>>> + - sprd,iommu-v1 > >>>> + > >>>> + "#iommu-cells": > >>>> + const: 0 > >>>> + description: > >>>> + Unisoc IOMMUs are all single-master IOMMU devices, therefore no > >>>> + additional information needs to associate with its master device. > >>>> + Please refer to the generic bindings document for more details, > >>>> + Documentation/devicetree/bindings/iommu/iommu.txt > >>>> + > >>>> + reg: > >>>> + maxItems: 1 > >>>> + description: > >>>> + Not required if 'sprd,iommu-regs' is defined. > >>>> + > >>>> + clocks: > >>>> + description: > >>>> + Reference to a gate clock phandle, since access to some of IOMMUs are > >>>> + controlled by gate clock, but this is not required. > >>>> + > >>>> + sprd,iommu-regs: > >>>> + $ref: /schemas/types.yaml#/definitions/phandle-array > >>>> + description: > >>>> + Reference to a syscon phandle plus 1 cell, the syscon defines the > >>>> + register range used by the iommu and the media device, the cell > >>>> + defines the offset for iommu registers. Since iommu module shares > >>>> + the same register range with the media device which uses it. > >>>> + > >>>> +required: > >>>> + - compatible > >>>> + - "#iommu-cells" > > OK, so apparently the hardware is not quite as trivial as my initial > impression, and you should have interrupts as well. Ok, I will have a look. > > >>>> + > >>>> +oneOf: > >>>> + - required: > >>>> + - reg > >>>> + - required: > >>>> + - sprd,iommu-regs > >>>> + > >>>> +additionalProperties: false > >>>> + > >>>> +examples: > >>>> + - | > >>>> + iommu_disp: iommu-disp { > >>>> + compatible = "sprd,iommu-v1"; > >>>> + sprd,iommu-regs = <&dpu_regs 0x800>; > >>> > >>> If the IOMMU is contained within another device, then it should just be > >>> a child node of that device. > >> > >> Yes, actually IOMMU can be seen as a child of multimedia devices, I > >> considered moving IOMMU under into multimedia device node, but > >> multimedia devices need IOMMU when probe[1], so I dropped that idea. > > > > Don't design your binding around working-around linux issues. > > Having stumbled across the DRM driver patches the other day, I now see > where this is coming from, and it's even worse than that - this whole > binding seems to be largely working around bad driver design. > I guess you mean bad h/w design (not bad driver design)? Having this syscon node just because I don't want a same register range to be mapped to multiple virtual address ranges, and that's the case for many media devices and their IOMMUs. If this issue exsists for one device only, I can even endure, but that's not unfortunately. But anyway, as you all think this is not a good way, I will change to use reg property. > >> And they share the same register base, e.g. > >> > >> + mm { > >> + compatible = "simple-bus"; > >> + #address-cells = <2>; > >> + #size-cells = <2>; > >> + ranges; > >> + > >> + dpu_regs: syscon@63000000 { > > > > Drop this node. > > > >> + compatible = "sprd,sc9863a-dpuregs", "syscon"; > >> + reg = <0 0x63000000 0 0x1000>; > >> + }; > >> + > >> + dpu: dpu@63000000 { > >> + compatible = "sprd,sharkl3-dpu"; > >> + sprd,disp-regs = <&dpu_regs>; > > > > reg = <0 0x63000000 0 0x800>; > > In fact judging by the other driver it looks like the length only needs > to be 0x200 here (but maybe there's more to come in future). > > >> + iommus = <&iommu_dispc>; > >> + }; > >> + > >> + iommu_dispc: iommu@63000000 { > >> + compatible = "sprd,iommu-v1"; > >> + sprd,iommu-regs = <&dpu_regs 0x800>; > > > > reg = <0 0x63000800 0 0x800>; > > ...and this one looks to need less than 0x80, even :) There're some registers not be added in the current driver indeed. The specification defines registers up to 0x7c. > > > > >> + #iommu-cells = <0>; > > > > Though given it seems there is only 1 client and this might really be > > just 1 h/w block, you don't really need to use the iommu binding at > > all. The DPU should be able to instantiate it's own IOMMU device. > > There's other examples of this such as mali GPU though that is all one > > driver, but that's a Linux implementation detail. > > FWIW that's really a very different situation - the MMUs in a Mali GPU > are fundamental parts of its internal pipelines and would never make > sense to handle as separate devices (if it were even feasible to try). > An IOMMU like this one is typically a logically-distinct block stuck to > the external bus interface of any old device, rewriting transactions > that said device has already issued - it's telling that it needs to > allocate the prot_page scratchpad for "faulting" transactions to still > flow somewhere, implying that it's not even involved enough to be able > to terminate them. > > As such I think it *does* make complete sense to describe even > "dedicated" IOMMUs like this one, Rockchip, Exynos, etc. in DT. > Otherwise you'd be effectively forcing OSes to turn half their > display/media drivers into mini board files with secret knowledge of > which blocks are integrated with IOMMUs on which SoCs. > Thanks for helping me explain the situation. Regards, Chunyan > Robin.
Hi Robin, On Tue, 16 Feb 2021 at 23:10, Robin Murphy <robin.murphy@arm.com> wrote: > > >>> > >>> On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote: > >>>> From: Chunyan Zhang <chunyan.zhang@unisoc.com> > >>>> > >>>> This iommu module can be used by Unisoc's multimedia devices, such as > >>>> display, Image codec(jpeg) and a few signal processors, including > >>>> VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc. > >>>> > >>>> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > >>>> --- > >>>> .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++ > >>>> 1 file changed, 72 insertions(+) > >>>> create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > >>>> new file mode 100644 > >>>> index 000000000000..4fc99e81fa66 > >>>> --- /dev/null > >>>> +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml > >>>> @@ -0,0 +1,72 @@ > >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>>> +# Copyright 2020 Unisoc Inc. > >>>> +%YAML 1.2 > >>>> +--- > >>>> +$id: http://devicetree.org/schemas/iommu/sprd,iommu.yaml# > >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>>> + > >>>> +title: Unisoc IOMMU and Multi-media MMU > >>>> + > >>>> +maintainers: > >>>> + - Chunyan Zhang <zhang.lyra@gmail.com> > >>>> + > >>>> +properties: > >>>> + compatible: > >>>> + enum: > >>>> + - sprd,iommu-v1 > >>>> + > >>>> + "#iommu-cells": > >>>> + const: 0 > >>>> + description: > >>>> + Unisoc IOMMUs are all single-master IOMMU devices, therefore no > >>>> + additional information needs to associate with its master device. > >>>> + Please refer to the generic bindings document for more details, > >>>> + Documentation/devicetree/bindings/iommu/iommu.txt > >>>> + > >>>> + reg: > >>>> + maxItems: 1 > >>>> + description: > >>>> + Not required if 'sprd,iommu-regs' is defined. > >>>> + > >>>> + clocks: > >>>> + description: > >>>> + Reference to a gate clock phandle, since access to some of IOMMUs are > >>>> + controlled by gate clock, but this is not required. > >>>> + > >>>> + sprd,iommu-regs: > >>>> + $ref: /schemas/types.yaml#/definitions/phandle-array > >>>> + description: > >>>> + Reference to a syscon phandle plus 1 cell, the syscon defines the > >>>> + register range used by the iommu and the media device, the cell > >>>> + defines the offset for iommu registers. Since iommu module shares > >>>> + the same register range with the media device which uses it. > >>>> + > >>>> +required: > >>>> + - compatible > >>>> + - "#iommu-cells" > > OK, so apparently the hardware is not quite as trivial as my initial > impression, and you should have interrupts as well. I've checked with my colleagues for this issue. And like I explained before, one sprd IOMMU serves one master device only, so interrupts are handled by master devices rather than IOMMUs. Chunyan
diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml new file mode 100644 index 000000000000..4fc99e81fa66 --- /dev/null +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml @@ -0,0 +1,72 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright 2020 Unisoc Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iommu/sprd,iommu.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Unisoc IOMMU and Multi-media MMU + +maintainers: + - Chunyan Zhang <zhang.lyra@gmail.com> + +properties: + compatible: + enum: + - sprd,iommu-v1 + + "#iommu-cells": + const: 0 + description: + Unisoc IOMMUs are all single-master IOMMU devices, therefore no + additional information needs to associate with its master device. + Please refer to the generic bindings document for more details, + Documentation/devicetree/bindings/iommu/iommu.txt + + reg: + maxItems: 1 + description: + Not required if 'sprd,iommu-regs' is defined. + + clocks: + description: + Reference to a gate clock phandle, since access to some of IOMMUs are + controlled by gate clock, but this is not required. + + sprd,iommu-regs: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: + Reference to a syscon phandle plus 1 cell, the syscon defines the + register range used by the iommu and the media device, the cell + defines the offset for iommu registers. Since iommu module shares + the same register range with the media device which uses it. + +required: + - compatible + - "#iommu-cells" + +oneOf: + - required: + - reg + - required: + - sprd,iommu-regs + +additionalProperties: false + +examples: + - | + iommu_disp: iommu-disp { + compatible = "sprd,iommu-v1"; + sprd,iommu-regs = <&dpu_regs 0x800>; + #iommu-cells = <0>; + }; + + - | + iommu_jpg: iommu-jpg { + compatible = "sprd,iommu-v1"; + sprd,iommu-regs = <&jpg_regs 0x300>; + #iommu-cells = <0>; + clocks = <&mm_gate 1>; + }; + +...