mbox series

[v6,00/33] MT8192 IOMMU support

Message ID 20210111111914.22211-1-yong.wu@mediatek.com
Headers show
Series MT8192 IOMMU support | expand

Message

Yong Wu (吴勇) Jan. 11, 2021, 11:18 a.m. UTC
This patch mainly adds support for mt8192 Multimedia IOMMU and SMI.

mt8192 also is MTK IOMMU gen2 which uses ARM Short-Descriptor translation
table format. The M4U-SMI HW diagram is as below:

                          EMI
                           |
                          M4U
                           |
                      ------------
                       SMI Common
                      ------------
                           |
  +-------+------+------+----------------------+-------+
  |       |      |      |       ......         |       |
  |       |      |      |                      |       |
larb0   larb1  larb2  larb4     ......      larb19   larb20
disp0   disp1   mdp    vdec                   IPE      IPE

All the connections are HW fixed, SW can NOT adjust it.

Comparing with the preview SoC, this patchset mainly adds two new functions:
a) add iova 34 bits support.
b) add multi domains support since several HW has the special iova
region requirement.

change note:
v6:a) base on v5.11-rc1. and tlb v4:
      https://lore.kernel.org/linux-mediatek/20210107122909.16317-1-yong.wu@mediatek.com/T/#t 
   b) Remove the "domain id" definition in the binding header file.
      Get the domain from dev->dma_range_map.
      After this, Change many codes flow.
   c) the patchset adds a new common file(mtk_smi-larb-port.h).
      This version changes that name into mtk-memory-port.h which reflect 
      its file path. This only changes the file name. no other change.
      thus I keep all the Reviewed-by Tags.
      (another reason is that we will add some iommu ports unrelated with
       smi-larb)
   d) Refactor the power-domain flow suggestted by Tomasz.
   e) Some other small fix. use different oas for different soc; Change the
   macro for 34bit iova tlb flush.

v5: https://lore.kernel.org/linux-iommu/20201209080102.26626-1-yong.wu@mediatek.com/
    a) Add a new patch for the header guard for smi-larb-port.h in [5/27].
    b) Add a new patch for error handle for iommu_device_sysfs_add and
 iommu_device_register[15/27].
    c) Add a flag for the iova "ias == 34" case. the previous SoC still keep
 32bits to save 16KB*3 lvl1 pgtable memory[13/27].
    d) Add include <linux/bitfield.h> for FIELD_GET build fail.
    e) In PM power domain patch, add a checking "pm_runtime_enabled" when call
 pm_runtime_get_sync for non power-domain case. and add a pm_runtime_put_noidle
 while pm_runtime_get_sync fail case.

v4: https://lore.kernel.org/linux-iommu/20201111123838.15682-1-yong.wu@mediatek.com/
  a) rebase on v5.10-rc1
  b) Move the smi part to a independent patchset.
  c) Improve v7s code from Robin and Will.
  d) Add a mediatek iommu entry patch in MAINTAIN.

v3: https://lore.kernel.org/linux-iommu/20200930070647.10188-1-yong.wu@mediatek.com/
  a) Fix DT schema issue commented from Rob.
  b) Fix a v7s issue. Use "_lvl" instead of "_l" in the macro(ARM_V7S_PTES_PER_LVL) since 
  it is called in ARM_V7S_LVL_IDX which has already used "_l".
  c) Fix a PM suspend issue: Avoid pm suspend in pm runtime case.

v2: https://lore.kernel.org/linux-iommu/20200905080920.13396-1-yong.wu@mediatek.com/
  a) Convert IOMMU/SMI dt-binding to DT schema.
  b) Fix some comment from Pi-Hsun and Nicolas. like use
  generic_iommu_put_resv_regions.
  c) Reword some comment, like add how to use domain-id.

v1: https://lore.kernel.org/linux-iommu/20200711064846.16007-1-yong.wu@mediatek.com/

Yong Wu (33):
  dt-bindings: iommu: mediatek: Convert IOMMU to DT schema
  dt-bindings: memory: mediatek: Add a common memory header file
  dt-bindings: memory: mediatek: Extend LARB_NR_MAX to 32
  dt-bindings: memory: mediatek: Rename header guard for SMI header file
  dt-bindings: mediatek: Add binding for mt8192 IOMMU
  of/device: Move dma_range_map before of_iommu_configure
  iommu: Avoid reallocate default domain for a group
  iommu/mediatek: Use the common mtk-memory-port.h
  iommu/io-pgtable-arm-v7s: Use ias to check the valid iova in unmap
  iommu/io-pgtable-arm-v7s: Extend PA34 for MediaTek
  iommu/io-pgtable-arm-v7s: Clarify LVL_SHIFT/BITS macro
  iommu/io-pgtable-arm-v7s: Add cfg as a param in some macros
  iommu/io-pgtable-arm-v7s: Quad lvl1 pgtable for MediaTek
  iommu/mediatek: Add a flag for iova 34bits case
  iommu/mediatek: Update oas for v7s
  iommu/mediatek: Move hw_init into attach_device
  iommu/mediatek: Add error handle for mtk_iommu_probe
  iommu/mediatek: Add device link for smi-common and m4u
  iommu/mediatek: Add pm runtime callback
  iommu/mediatek: Add power-domain operation
  iommu/mediatek: Support up to 34bit iova in tlb flush
  iommu/mediatek: Support report iova 34bit translation fault in ISR
  iommu/mediatek: Adjust the structure
  iommu/mediatek: Move domain_finalise into attach_device
  iommu/mediatek: Move geometry.aperture updating into domain_finalise
  iommu/mediatek: Add iova_region structure
  iommu/mediatek: Add get_domain_id from dev->dma_range_map
  iommu/mediatek: Support for multi domains
  iommu/mediatek: Add iova reserved function
  iommu/mediatek: Support master use iova over 32bit
  iommu/mediatek: Remove unnecessary check in attach_device
  iommu/mediatek: Add mt8192 support
  MAINTAINERS: Add entry for MediaTek IOMMU

 .../bindings/iommu/mediatek,iommu.txt         | 105 -----
 .../bindings/iommu/mediatek,iommu.yaml        | 183 +++++++++
 MAINTAINERS                                   |   9 +
 drivers/iommu/io-pgtable-arm-v7s.c            |  56 +--
 drivers/iommu/iommu.c                         |   3 +-
 drivers/iommu/mtk_iommu.c                     | 366 ++++++++++++++----
 drivers/iommu/mtk_iommu.h                     |  12 +-
 drivers/memory/mtk-smi.c                      |   8 +
 drivers/of/device.c                           |   3 +-
 include/dt-bindings/memory/mt2701-larb-port.h |   4 +-
 include/dt-bindings/memory/mt2712-larb-port.h |   6 +-
 include/dt-bindings/memory/mt6779-larb-port.h |   6 +-
 include/dt-bindings/memory/mt8167-larb-port.h |   6 +-
 include/dt-bindings/memory/mt8173-larb-port.h |   6 +-
 include/dt-bindings/memory/mt8183-larb-port.h |   6 +-
 include/dt-bindings/memory/mt8192-larb-port.h | 243 ++++++++++++
 include/dt-bindings/memory/mtk-memory-port.h  |  15 +
 include/linux/io-pgtable.h                    |   4 +-
 include/soc/mediatek/smi.h                    |   3 +-
 19 files changed, 810 insertions(+), 234 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
 create mode 100644 Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml
 create mode 100644 include/dt-bindings/memory/mt8192-larb-port.h
 create mode 100644 include/dt-bindings/memory/mtk-memory-port.h

Comments

Rob Herring Jan. 14, 2021, 7:27 p.m. UTC | #1
On Mon, Jan 11, 2021 at 07:18:47PM +0800, Yong Wu wrote:
> "dev->dma_range_map" contains the devices' dma_ranges information,

> This patch moves dma_range_map before of_iommu_configure. The iommu

> driver may need to know the dma_address requirements of its iommu

> consumer devices.

> 

> CC: Rob Herring <robh+dt@kernel.org>

> CC: Frank Rowand <frowand.list@gmail.com>

> Signed-off-by: Yong Wu <yong.wu@mediatek.com>

> ---

>  drivers/of/device.c | 3 ++-

>  1 file changed, 2 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/of/device.c b/drivers/of/device.c

> index aedfaaafd3e7..1d84636149df 100644

> --- a/drivers/of/device.c

> +++ b/drivers/of/device.c

> @@ -170,9 +170,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

>  	dev_dbg(dev, "device is%sdma coherent\n",

>  		coherent ? " " : " not ");

>  

> +	dev->dma_range_map = map;

>  	iommu = of_iommu_configure(dev, np, id);

>  	if (PTR_ERR(iommu) == -EPROBE_DEFER) {

>  		kfree(map);

> +		dev->dma_range_map = NULL;


Not really going to matter, but you should probably clear dma_range_map 
before what it points to is freed.

With that,

Reviewed-by: Rob Herring <robh@kernel.org>


>  		return -EPROBE_DEFER;

>  	}

>  

> @@ -181,7 +183,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

>  

>  	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);

>  

> -	dev->dma_range_map = map;

>  	return 0;

>  }

>  EXPORT_SYMBOL_GPL(of_dma_configure_id);

> -- 

> 2.18.0

>
Yong Wu (吴勇) Jan. 15, 2021, 5:30 a.m. UTC | #2
On Thu, 2021-01-14 at 13:27 -0600, Rob Herring wrote:
> On Mon, Jan 11, 2021 at 07:18:47PM +0800, Yong Wu wrote:

> > "dev->dma_range_map" contains the devices' dma_ranges information,

> > This patch moves dma_range_map before of_iommu_configure. The iommu

> > driver may need to know the dma_address requirements of its iommu

> > consumer devices.

> > 

> > CC: Rob Herring <robh+dt@kernel.org>

> > CC: Frank Rowand <frowand.list@gmail.com>

> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>

> > ---

> >  drivers/of/device.c | 3 ++-

> >  1 file changed, 2 insertions(+), 1 deletion(-)

> > 

> > diff --git a/drivers/of/device.c b/drivers/of/device.c

> > index aedfaaafd3e7..1d84636149df 100644

> > --- a/drivers/of/device.c

> > +++ b/drivers/of/device.c

> > @@ -170,9 +170,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

> >  	dev_dbg(dev, "device is%sdma coherent\n",

> >  		coherent ? " " : " not ");

> >  

> > +	dev->dma_range_map = map;

> >  	iommu = of_iommu_configure(dev, np, id);

> >  	if (PTR_ERR(iommu) == -EPROBE_DEFER) {

> >  		kfree(map);

> > +		dev->dma_range_map = NULL;

> 

> Not really going to matter, but you should probably clear dma_range_map 

> before what it points to is freed.

> 

> With that,

> 

> Reviewed-by: Rob Herring <robh@kernel.org>


Thanks for the review. I will move it before "kfree(map)" in next
version.

> 

> >  		return -EPROBE_DEFER;

> >  	}

> >  

> > @@ -181,7 +183,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

> >  

> >  	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);

> >  

> > -	dev->dma_range_map = map;

> >  	return 0;

> >  }

> >  EXPORT_SYMBOL_GPL(of_dma_configure_id);

> > -- 

> > 2.18.0

> >
Robin Murphy Jan. 18, 2021, 3:49 p.m. UTC | #3
On 2021-01-15 05:30, Yong Wu wrote:
> On Thu, 2021-01-14 at 13:27 -0600, Rob Herring wrote:

>> On Mon, Jan 11, 2021 at 07:18:47PM +0800, Yong Wu wrote:

>>> "dev->dma_range_map" contains the devices' dma_ranges information,

>>> This patch moves dma_range_map before of_iommu_configure. The iommu

>>> driver may need to know the dma_address requirements of its iommu

>>> consumer devices.

>>>

>>> CC: Rob Herring <robh+dt@kernel.org>

>>> CC: Frank Rowand <frowand.list@gmail.com>

>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>

>>> ---

>>>   drivers/of/device.c | 3 ++-

>>>   1 file changed, 2 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/drivers/of/device.c b/drivers/of/device.c

>>> index aedfaaafd3e7..1d84636149df 100644

>>> --- a/drivers/of/device.c

>>> +++ b/drivers/of/device.c

>>> @@ -170,9 +170,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

>>>   	dev_dbg(dev, "device is%sdma coherent\n",

>>>   		coherent ? " " : " not ");

>>>   

>>> +	dev->dma_range_map = map;

>>>   	iommu = of_iommu_configure(dev, np, id);

>>>   	if (PTR_ERR(iommu) == -EPROBE_DEFER) {

>>>   		kfree(map);

>>> +		dev->dma_range_map = NULL;

>>

>> Not really going to matter, but you should probably clear dma_range_map

>> before what it points to is freed.

>>

>> With that,

>>

>> Reviewed-by: Rob Herring <robh@kernel.org>

> 

> Thanks for the review. I will move it before "kfree(map)" in next

> version.


Paul noticed that we already have a bug in assigning to this 
unconditionally[1] - I'd totally forgotten about this series when I 
theorised about IOMMU drivers wanting the information earlier, but 
sweeping my inbox now only goes to show I was right to think of it :)

We should really get something in as a fix independent of this series, 
taking both angles into account.

Robin.

[1] 
https://lore.kernel.org/linux-arm-kernel/5c7946f3-b56e-da00-a750-be097c7ceb32@arm.com/

>>

>>>   		return -EPROBE_DEFER;

>>>   	}

>>>   

>>> @@ -181,7 +183,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

>>>   

>>>   	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);

>>>   

>>> -	dev->dma_range_map = map;

>>>   	return 0;

>>>   }

>>>   EXPORT_SYMBOL_GPL(of_dma_configure_id);

>>> -- 

>>> 2.18.0

>>>

> 

> _______________________________________________

> iommu mailing list

> iommu@lists.linux-foundation.org

> https://lists.linuxfoundation.org/mailman/listinfo/iommu

>
Paul Kocialkowski Jan. 19, 2021, 9:13 a.m. UTC | #4
Hi,

On Mon 18 Jan 21, 15:49, Robin Murphy wrote:
> On 2021-01-15 05:30, Yong Wu wrote:

> > On Thu, 2021-01-14 at 13:27 -0600, Rob Herring wrote:

> > > On Mon, Jan 11, 2021 at 07:18:47PM +0800, Yong Wu wrote:

> > > > "dev->dma_range_map" contains the devices' dma_ranges information,

> > > > This patch moves dma_range_map before of_iommu_configure. The iommu

> > > > driver may need to know the dma_address requirements of its iommu

> > > > consumer devices.

> > > > 

> > > > CC: Rob Herring <robh+dt@kernel.org>

> > > > CC: Frank Rowand <frowand.list@gmail.com>

> > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com>

> > > > ---

> > > >   drivers/of/device.c | 3 ++-

> > > >   1 file changed, 2 insertions(+), 1 deletion(-)

> > > > 

> > > > diff --git a/drivers/of/device.c b/drivers/of/device.c

> > > > index aedfaaafd3e7..1d84636149df 100644

> > > > --- a/drivers/of/device.c

> > > > +++ b/drivers/of/device.c

> > > > @@ -170,9 +170,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

> > > >   	dev_dbg(dev, "device is%sdma coherent\n",

> > > >   		coherent ? " " : " not ");

> > > > +	dev->dma_range_map = map;

> > > >   	iommu = of_iommu_configure(dev, np, id);

> > > >   	if (PTR_ERR(iommu) == -EPROBE_DEFER) {

> > > >   		kfree(map);

> > > > +		dev->dma_range_map = NULL;

> > > 

> > > Not really going to matter, but you should probably clear dma_range_map

> > > before what it points to is freed.

> > > 

> > > With that,

> > > 

> > > Reviewed-by: Rob Herring <robh@kernel.org>

> > 

> > Thanks for the review. I will move it before "kfree(map)" in next

> > version.

> 

> Paul noticed that we already have a bug in assigning to this

> unconditionally[1] - I'd totally forgotten about this series when I

> theorised about IOMMU drivers wanting the information earlier, but sweeping

> my inbox now only goes to show I was right to think of it :)

> 

> We should really get something in as a fix independent of this series,

> taking both angles into account.


Okay, I can also fix this while fixing my case. So we'd go for setting
dev->dma_range_map = map; under the if (!ret).

Then I think the error case for of_iommu_configure should be to set
dev->dma_range_map = NULL; only if map != NULL (otherwise we'd be overwriting
and leaking the previously-set map).

I think a comment to remind that dev->dma_range_map can be set prior to this
function would be useful too.

What do you think?

Cheers,

Paul

> Robin.

> 

> [1] https://lore.kernel.org/linux-arm-kernel/5c7946f3-b56e-da00-a750-be097c7ceb32@arm.com/

> 

> > > 

> > > >   		return -EPROBE_DEFER;

> > > >   	}

> > > > @@ -181,7 +183,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

> > > >   	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);

> > > > -	dev->dma_range_map = map;

> > > >   	return 0;

> > > >   }

> > > >   EXPORT_SYMBOL_GPL(of_dma_configure_id);

> > > > -- 

> > > > 2.18.0

> > > > 

> > 

> > _______________________________________________

> > iommu mailing list

> > iommu@lists.linux-foundation.org

> > https://lists.linuxfoundation.org/mailman/listinfo/iommu

> > 


-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Yong Wu (吴勇) Jan. 19, 2021, 9:20 a.m. UTC | #5
On Mon, 2021-01-18 at 15:49 +0000, Robin Murphy wrote:
> On 2021-01-15 05:30, Yong Wu wrote:

> > On Thu, 2021-01-14 at 13:27 -0600, Rob Herring wrote:

> >> On Mon, Jan 11, 2021 at 07:18:47PM +0800, Yong Wu wrote:

> >>> "dev->dma_range_map" contains the devices' dma_ranges information,

> >>> This patch moves dma_range_map before of_iommu_configure. The iommu

> >>> driver may need to know the dma_address requirements of its iommu

> >>> consumer devices.

> >>>

> >>> CC: Rob Herring <robh+dt@kernel.org>

> >>> CC: Frank Rowand <frowand.list@gmail.com>

> >>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>

> >>> ---

> >>>   drivers/of/device.c | 3 ++-

> >>>   1 file changed, 2 insertions(+), 1 deletion(-)

> >>>

> >>> diff --git a/drivers/of/device.c b/drivers/of/device.c

> >>> index aedfaaafd3e7..1d84636149df 100644

> >>> --- a/drivers/of/device.c

> >>> +++ b/drivers/of/device.c

> >>> @@ -170,9 +170,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

> >>>   	dev_dbg(dev, "device is%sdma coherent\n",

> >>>   		coherent ? " " : " not ");

> >>>   

> >>> +	dev->dma_range_map = map;

> >>>   	iommu = of_iommu_configure(dev, np, id);

> >>>   	if (PTR_ERR(iommu) == -EPROBE_DEFER) {

> >>>   		kfree(map);

> >>> +		dev->dma_range_map = NULL;

> >>

> >> Not really going to matter, but you should probably clear dma_range_map

> >> before what it points to is freed.

> >>

> >> With that,

> >>

> >> Reviewed-by: Rob Herring <robh@kernel.org>

> > 

> > Thanks for the review. I will move it before "kfree(map)" in next

> > version.

> 

> Paul noticed that we already have a bug in assigning to this 

> unconditionally[1] - I'd totally forgotten about this series when I 

> theorised about IOMMU drivers wanting the information earlier, but 

> sweeping my inbox now only goes to show I was right to think of it :)

> 

> We should really get something in as a fix independent of this series, 

> taking both angles into account.


Thanks this info. Following your suggestion, Move this into the "if (!
ret)". Then it is like this:


--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -163,8 +163,10 @@ int of_dma_configure_id(struct device *dev, struct
device_node *np,
 	dev->coherent_dma_mask &= mask;
 	*dev->dma_mask &= mask;
 	/* ...but only set bus limit if we found valid dma-ranges earlier */
-	if (!ret)
+	if (!ret) {
 		dev->bus_dma_limit = end;
+		dev->dma_range_map = map;
+	}
 
 	coherent = of_dma_is_coherent(np);
 	dev_dbg(dev, "device is%sdma coherent\n",
@@ -172,6 +174,8 @@ int of_dma_configure_id(struct device *dev, struct
device_node *np,
 
 	iommu = of_iommu_configure(dev, np, id);
 	if (PTR_ERR(iommu) == -EPROBE_DEFER) {
+		if (!ret)
+			dev->dma_range_map = NULL;
 		kfree(map);
 		return -EPROBE_DEFER;
 	}
@@ -181,7 +185,6 @@ int of_dma_configure_id(struct device *dev, struct
device_node *np,
 
 	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
 
-	dev->dma_range_map = map;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_dma_configure_id);


If this is ok, I will send this as a independent patch.

> 

> Robin.

> 

> [1] 

> https://lore.kernel.org/linux-arm-kernel/5c7946f3-b56e-da00-a750-be097c7ceb32@arm.com/

> 

> >>

> >>>   		return -EPROBE_DEFER;

> >>>   	}

> >>>   

> >>> @@ -181,7 +183,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

> >>>   

> >>>   	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);

> >>>   

> >>> -	dev->dma_range_map = map;

> >>>   	return 0;

> >>>   }

> >>>   EXPORT_SYMBOL_GPL(of_dma_configure_id);

> >>> -- 

> >>> 2.18.0

> >>>

> > 

> > _______________________________________________

> > iommu mailing list

> > iommu@lists.linux-foundation.org

> > https://lists.linuxfoundation.org/mailman/listinfo/iommu

> >
Paul Kocialkowski Jan. 19, 2021, 9:37 a.m. UTC | #6
Hi,

On Tue 19 Jan 21, 17:20, Yong Wu wrote:
> On Mon, 2021-01-18 at 15:49 +0000, Robin Murphy wrote:

> > On 2021-01-15 05:30, Yong Wu wrote:

> > > On Thu, 2021-01-14 at 13:27 -0600, Rob Herring wrote:

> > >> On Mon, Jan 11, 2021 at 07:18:47PM +0800, Yong Wu wrote:

> > >>> "dev->dma_range_map" contains the devices' dma_ranges information,

> > >>> This patch moves dma_range_map before of_iommu_configure. The iommu

> > >>> driver may need to know the dma_address requirements of its iommu

> > >>> consumer devices.

> > >>>

> > >>> CC: Rob Herring <robh+dt@kernel.org>

> > >>> CC: Frank Rowand <frowand.list@gmail.com>

> > >>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>

> > >>> ---

> > >>>   drivers/of/device.c | 3 ++-

> > >>>   1 file changed, 2 insertions(+), 1 deletion(-)

> > >>>

> > >>> diff --git a/drivers/of/device.c b/drivers/of/device.c

> > >>> index aedfaaafd3e7..1d84636149df 100644

> > >>> --- a/drivers/of/device.c

> > >>> +++ b/drivers/of/device.c

> > >>> @@ -170,9 +170,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

> > >>>   	dev_dbg(dev, "device is%sdma coherent\n",

> > >>>   		coherent ? " " : " not ");

> > >>>   

> > >>> +	dev->dma_range_map = map;

> > >>>   	iommu = of_iommu_configure(dev, np, id);

> > >>>   	if (PTR_ERR(iommu) == -EPROBE_DEFER) {

> > >>>   		kfree(map);

> > >>> +		dev->dma_range_map = NULL;

> > >>

> > >> Not really going to matter, but you should probably clear dma_range_map

> > >> before what it points to is freed.

> > >>

> > >> With that,

> > >>

> > >> Reviewed-by: Rob Herring <robh@kernel.org>

> > > 

> > > Thanks for the review. I will move it before "kfree(map)" in next

> > > version.

> > 

> > Paul noticed that we already have a bug in assigning to this 

> > unconditionally[1] - I'd totally forgotten about this series when I 

> > theorised about IOMMU drivers wanting the information earlier, but 

> > sweeping my inbox now only goes to show I was right to think of it :)

> > 

> > We should really get something in as a fix independent of this series, 

> > taking both angles into account.

> 

> Thanks this info. Following your suggestion, Move this into the "if (!

> ret)". Then it is like this:


Thanks for preparing the change :)

> 

> --- a/drivers/of/device.c

> +++ b/drivers/of/device.c

> @@ -163,8 +163,10 @@ int of_dma_configure_id(struct device *dev, struct

> device_node *np,

>  	dev->coherent_dma_mask &= mask;

>  	*dev->dma_mask &= mask;

>  	/* ...but only set bus limit if we found valid dma-ranges earlier */


Maybe the comment would need some update too, like:
/* ...but only set bus limit and map if we found valid dma-ranges earlier */

> -	if (!ret)

> +	if (!ret) {

>  		dev->bus_dma_limit = end;

> +		dev->dma_range_map = map;

> +	}

>  

>  	coherent = of_dma_is_coherent(np);

>  	dev_dbg(dev, "device is%sdma coherent\n",

> @@ -172,6 +174,8 @@ int of_dma_configure_id(struct device *dev, struct

> device_node *np,

>  

>  	iommu = of_iommu_configure(dev, np, id);

>  	if (PTR_ERR(iommu) == -EPROBE_DEFER) {


And maybe one here, something like:
/* don't touch range map if it wasn't set from a valid dma-ranges */

> +		if (!ret)

> +			dev->dma_range_map = NULL;

>  		kfree(map);

>  		return -EPROBE_DEFER;

>  	}

> @@ -181,7 +185,6 @@ int of_dma_configure_id(struct device *dev, struct

> device_node *np,

>  

>  	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);

>  

> -	dev->dma_range_map = map;

>  	return 0;

>  }

>  EXPORT_SYMBOL_GPL(of_dma_configure_id);

> 

> 

> If this is ok, I will send this as a independent patch.


With the suggested changes, this looks good to me!

Thanks,

Paul

> > 

> > Robin.

> > 

> > [1] 

> > https://lore.kernel.org/linux-arm-kernel/5c7946f3-b56e-da00-a750-be097c7ceb32@arm.com/

> > 

> > >>

> > >>>   		return -EPROBE_DEFER;

> > >>>   	}

> > >>>   

> > >>> @@ -181,7 +183,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,

> > >>>   

> > >>>   	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);

> > >>>   

> > >>> -	dev->dma_range_map = map;

> > >>>   	return 0;

> > >>>   }

> > >>>   EXPORT_SYMBOL_GPL(of_dma_configure_id);

> > >>> -- 

> > >>> 2.18.0

> > >>>

> > > 

> > > _______________________________________________

> > > iommu mailing list

> > > iommu@lists.linux-foundation.org

> > > https://lists.linuxfoundation.org/mailman/listinfo/iommu

> > > 

> 


-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Will Deacon Jan. 26, 2021, 10:25 p.m. UTC | #7
On Mon, Jan 11, 2021 at 07:18:41PM +0800, Yong Wu wrote:
> This patch mainly adds support for mt8192 Multimedia IOMMU and SMI.

> 

> mt8192 also is MTK IOMMU gen2 which uses ARM Short-Descriptor translation

> table format. The M4U-SMI HW diagram is as below:

> 

>                           EMI

>                            |

>                           M4U

>                            |

>                       ------------

>                        SMI Common

>                       ------------

>                            |

>   +-------+------+------+----------------------+-------+

>   |       |      |      |       ......         |       |

>   |       |      |      |                      |       |

> larb0   larb1  larb2  larb4     ......      larb19   larb20

> disp0   disp1   mdp    vdec                   IPE      IPE

> 

> All the connections are HW fixed, SW can NOT adjust it.

> 

> Comparing with the preview SoC, this patchset mainly adds two new functions:

> a) add iova 34 bits support.

> b) add multi domains support since several HW has the special iova

> region requirement.


This is looking good and I'd really like to see it merged, especially as it
has changes to the io-pgtable code. Please could you post a new version ASAP
to address the comments on patches 6 and 7?

Will
Will Deacon Feb. 1, 2021, 2:54 p.m. UTC | #8
On Mon, Jan 11, 2021 at 07:18:41PM +0800, Yong Wu wrote:
> This patch mainly adds support for mt8192 Multimedia IOMMU and SMI.

> 

> mt8192 also is MTK IOMMU gen2 which uses ARM Short-Descriptor translation

> table format. The M4U-SMI HW diagram is as below:

> 

>                           EMI

>                            |

>                           M4U

>                            |

>                       ------------

>                        SMI Common

>                       ------------

>                            |

>   +-------+------+------+----------------------+-------+

>   |       |      |      |       ......         |       |

>   |       |      |      |                      |       |

> larb0   larb1  larb2  larb4     ......      larb19   larb20

> disp0   disp1   mdp    vdec                   IPE      IPE

> 

> All the connections are HW fixed, SW can NOT adjust it.

> 

> Comparing with the preview SoC, this patchset mainly adds two new functions:

> a) add iova 34 bits support.

> b) add multi domains support since several HW has the special iova

> region requirement.

> 

> change note:

> v6:a) base on v5.11-rc1. and tlb v4:

>       https://lore.kernel.org/linux-mediatek/20210107122909.16317-1-yong.wu@mediatek.com/T/#t 


I've queued this up apart from patches 6 and 7.

Thanks,

Will
Yong Wu (吴勇) Feb. 2, 2021, 2:03 a.m. UTC | #9
On Mon, 2021-02-01 at 14:54 +0000, Will Deacon wrote:
> On Mon, Jan 11, 2021 at 07:18:41PM +0800, Yong Wu wrote:

> > This patch mainly adds support for mt8192 Multimedia IOMMU and SMI.

> > 

> > mt8192 also is MTK IOMMU gen2 which uses ARM Short-Descriptor translation

> > table format. The M4U-SMI HW diagram is as below:

> > 

> >                           EMI

> >                            |

> >                           M4U

> >                            |

> >                       ------------

> >                        SMI Common

> >                       ------------

> >                            |

> >   +-------+------+------+----------------------+-------+

> >   |       |      |      |       ......         |       |

> >   |       |      |      |                      |       |

> > larb0   larb1  larb2  larb4     ......      larb19   larb20

> > disp0   disp1   mdp    vdec                   IPE      IPE

> > 

> > All the connections are HW fixed, SW can NOT adjust it.

> > 

> > Comparing with the preview SoC, this patchset mainly adds two new functions:

> > a) add iova 34 bits support.

> > b) add multi domains support since several HW has the special iova

> > region requirement.

> > 

> > change note:

> > v6:a) base on v5.11-rc1. and tlb v4:

> >       https://lore.kernel.org/linux-mediatek/20210107122909.16317-1-yong.wu@mediatek.com/T/#t 

> 

> I've queued this up apart from patches 6 and 7.


Thanks very much for the applying. I'd like to show there is a little
conflict with a smi change[1] in /include/soc/mediatek/smi.h.

This is the detailed conflict:

--- a/include/soc/mediatek/smi.h
+++ b/include/soc/mediatek/smi.h
@@ -9,7 +9,7 @@
 #include <linux/bitops.h>
 #include <linux/device.h>
 
-#ifdef CONFIG_MTK_SMI
+#if IS_ENABLED(CONFIG_MTK_SMI)   <---The smi patch change here.
 
 #define MTK_LARB_NR_MAX   16  <---This iommu patchset delete this line.


This code is simple. Please feel free to tell me how to do this if this
is not convenient to merge.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-mem-ctrl.git/commit/?h=for-next&id=50fc8d9232cdc64b9e9d1b9488452f153de52b69

> 

> Thanks,

> 

> Will
Will Deacon Feb. 2, 2021, 1:33 p.m. UTC | #10
On Tue, Feb 02, 2021 at 10:03:45AM +0800, Yong Wu wrote:
> On Mon, 2021-02-01 at 14:54 +0000, Will Deacon wrote:

> > On Mon, Jan 11, 2021 at 07:18:41PM +0800, Yong Wu wrote:

> > > This patch mainly adds support for mt8192 Multimedia IOMMU and SMI.

> > > 

> > > mt8192 also is MTK IOMMU gen2 which uses ARM Short-Descriptor translation

> > > table format. The M4U-SMI HW diagram is as below:

> > > 

> > >                           EMI

> > >                            |

> > >                           M4U

> > >                            |

> > >                       ------------

> > >                        SMI Common

> > >                       ------------

> > >                            |

> > >   +-------+------+------+----------------------+-------+

> > >   |       |      |      |       ......         |       |

> > >   |       |      |      |                      |       |

> > > larb0   larb1  larb2  larb4     ......      larb19   larb20

> > > disp0   disp1   mdp    vdec                   IPE      IPE

> > > 

> > > All the connections are HW fixed, SW can NOT adjust it.

> > > 

> > > Comparing with the preview SoC, this patchset mainly adds two new functions:

> > > a) add iova 34 bits support.

> > > b) add multi domains support since several HW has the special iova

> > > region requirement.

> > > 

> > > change note:

> > > v6:a) base on v5.11-rc1. and tlb v4:

> > >       https://lore.kernel.org/linux-mediatek/20210107122909.16317-1-yong.wu@mediatek.com/T/#t 

> > 

> > I've queued this up apart from patches 6 and 7.

> 

> Thanks very much for the applying. I'd like to show there is a little

> conflict with a smi change[1] in /include/soc/mediatek/smi.h.

> 

> This is the detailed conflict:

> 

> --- a/include/soc/mediatek/smi.h

> +++ b/include/soc/mediatek/smi.h

> @@ -9,7 +9,7 @@

>  #include <linux/bitops.h>

>  #include <linux/device.h>

>  

> -#ifdef CONFIG_MTK_SMI

> +#if IS_ENABLED(CONFIG_MTK_SMI)   <---The smi patch change here.

>  

>  #define MTK_LARB_NR_MAX   16  <---This iommu patchset delete this line.

> 

> 

> This code is simple. Please feel free to tell me how to do this if this

> is not convenient to merge.


Thanks, but this should be trivial to resolve, so I don't think we need to
worry about it.

Will