lkml.org 
[lkml]   [2021]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 1/2] dt-bindings: iommu: add bindings for sprd iommu
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.

\
 
 \ /
  Last update: 2021-02-26 07:49    [W:0.143 / U:0.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site