Re: [PATCHv2 03/16] Documentation: dt: add OMAP iommu bindings

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

Hi Suman,

On Wednesday 26 February 2014 11:02:24 Suman Anna wrote:
> On 02/25/2014 08:13 PM, Laurent Pinchart wrote:
> > On Tuesday 25 February 2014 17:02:35 Suman Anna wrote:
> >> On 02/25/2014 03:26 PM, Laurent Pinchart wrote:
> >>> On Thursday 13 February 2014 12:15:34 Suman Anna wrote:
> >>>> From: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
> >>>> 
> >>>> This patch adds the iommu bindings for all OMAP2+ SoCs. Apart from
> >>>> the standard bindings used by OMAP peripherals, this patch uses a
> >>>> 'dma-window' (already used by Tegra SMMU) and adds two OMAP custom
> >>>> bindings - 'ti,#tlb-entries' and 'ti,iommu-bus-err-back'.
> >>>> 
> >>>> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
> >>>> [s-anna-l0cyMroinI0@public.gmane.org: split bindings document, add dra7 and bus error back]
> >>>> Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
> >>>> ---
> >>>> 
> >>>>  .../devicetree/bindings/iommu/ti,omap-iommu.txt    | 28 ++++++++++++++
> >>>>  1 file changed, 28 insertions(+)
> >>>>  create mode 100644
> >>>>  Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
> >>>> 
> >>>> diff --git a/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
> >>>> b/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt new file
> >>>> mode 100644
> >>>> index 0000000..116492d
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
> >>>> @@ -0,0 +1,28 @@
> >>>> +OMAP2+ IOMMU
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible : Should be one of,
> >>>> +		"ti,omap2-iommu" for OMAP2/OMAP3 IOMMU instances
> >>>> +		"ti,omap4-iommu" for OMAP4/OMAP5 IOMMU instances
> >>>> +		"ti,dra7-iommu" for DRA7xx IOMMU instances
> >>>> +- ti,hwmods  : Name of the hwmod associated with the IOMMU instance
> >>>> +- reg        : Address space for the configuration registers
> >>>> +- interrupts : Interrupt specifier for the IOMMU instance
> >>>> +- dma-window : IOVA start address and length
> >>> 
> >>> Isn't the dma window more of a system configuration property than a
> >>> hardware property ? How do you expect it to be set?
> >> 
> >> We are setting it based on the addressable range for the MMU.
> > 
> > A quick look at the ISP and IVA IOMMUs in the OMAP3 shows that both
> > support the full 4GB VA space. Why do you need to restrict it ?
> 
> I should have rephrased it better when I said addressable range. While
> the MMUs are capable of programming the full 4GB space, there are some
> address ranges that are private from the processor view. This window is
> currently used to set the range for the omap-iovmm driver (which only
> OMAP3 ISP is using atm), and there is no point in allowing the
> omap-iovmm driver the full range when the processor could never
> reach/access those addresses.

But the IOMMU VA space is from a device point of view, not from a CPU point of 
view. Could you point me to where those private ranges are documented, in 
order to understand the problem correctly ?

> >> We are reusing the existing defined property and it allows us to get rid
> >> of the IOVA start and end addresses defined in the pre-DT OMAP iommu
> >> platform data.
> >> 
> >>>> +Optional properties:
> >>>> +- ti,#tlb-entries : Number of entries in the translation look-aside
> >>>> buffer. +                    Should be either 8 or 32 (default: 32)
> >>>> +- ti,iommu-bus-err-back : Indicates the IOMMU instance supports
> >>>> throwing
> >>>> +		          back a bus error response on MMU faults.
> >>> 
> >>> Do these features vary per IOMMU instance or per IOMMU model ? In the
> >>> latter case they could be inferred from the compatible string by the
> >>> driver without requiring them to be explicit in DT (whether you want to
> >>> do so is left to you though).
> >> 
> >> Well, these are fixed features given an IOMMU instance, like the OMAP3
> >> ISP is the only one that has 8 TLB entries, all the remaining ones have
> >> 32, and the IPU iommu instances are the only ones that support the bus
> >> error response back. I have no preference to any particular way, and
> >> sure the driver can infer these easily based on unique compatible
> >> strings per subsystem per SoC. I just happened to go with defining
> >> compatible strings per SoC, with the optional properties differentiating
> >> the fixed behavior between different IOMMU instances on that SoC. This
> >> is where I was looking for some inputs/guidance from the DT bindings
> >> maintainers on what is the preferred method.
> > 
> > I think you've made the right choice. I wasn't sure whether those
> > parameters varied across IOMMU instances of compatible devices (from a
> > compatible string point of view) or were constant. As they vary they
> > should be expressed in DT.
>
> Yeah, I wasn't sure if these qualify as features (as per
> Documentation/devicetree/bindings/ABI.txt section II.2).
> 
> regards
> Suman
> 
> >>>> +Example:
> >>>> +	/* OMAP3 ISP MMU */
> >>>> +	mmu_isp: mmu@480bd400 {
> >>>> +		compatible = "ti,omap2-iommu";
> >>>> +		reg = <0x480bd400 0x80>;
> >>>> +		interrupts = <24>;
> >>>> +		ti,hwmods = "mmu_isp";
> >>>> +		ti,#tlb-entries = <8>;
> >>>> +		dma-window = <0 0xfffff000>;
> >>>> +	};

-- 
Regards,

Laurent Pinchart