Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc

[Diana Craciun OSS <diana.craciun@oss.nxp.com>]

Hi Lorenzo,

On 2/18/2020 4:46 PM, Lorenzo Pieralisi wrote:
> On Tue, Feb 18, 2020 at 12:48:39PM +0000, Pankaj Bansal (OSS) wrote:
>
> [...]
>
>>>> In DT case, we create the domain DOMAIN_BUS_FSL_MC_MSI for MC bus and
>>> it's children.
>>>> And then when MC child device is created, we search the "msi-parent"
>>> property from the MC
>>>> DT node and get the ITS associated with MC bus. Then we search
>>> DOMAIN_BUS_FSL_MC_MSI
>>>> on that ITS. Once we find the domain, we can call msi_domain_alloc_irqs for
>>> that domain.
>>>> This is exactly what we tried to do initially with ACPI. But the searching
>>> DOMAIN_BUS_FSL_MC_MSI
>>>> associated to an ITS, is something that is part of drivers/acpi/arm64/iort.c.
>>>> (similar to DOMAIN_BUS_PLATFORM_MSI and DOMAIN_BUS_PCI_MSI)
>>> Can you have a look at mbigen driver (drivers/irqchip/irq-mbigen.c) to see if
>>> it helps you?
>>>
>>> mbigen is an irq converter to convert device's wired interrupts into MSI
>>> (connecting to ITS), which will alloc a bunch of MSIs from ITS platform MSI
>>> domain at the setup.
>> Unfortunately this is not the same case as ours. As I see Hisilicon IORT table
>> Is using single id mapping with named components.
>>
>> https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hisilicon/Hi1616/D05AcpiTables/D05Iort.asl#L300
>>
>> while we are not:
>>
>> https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platforms/tree/Platform/NXP/LX2160aRdbPkg/AcpiTables/Iort.aslc?h=LX2160_UEFI_ACPI_EAR1#n290
>>
>> This is because as I said, we are trying to represent a bus in IORT
>> via named components and not individual devices connected to that bus.
> I had a thorough look into this and strictly speaking there is no
> *mapping* requirement at all, all you need to know is what ITS the FSL
> MC bus is mapping MSIs to. Which brings me to the next question (which
> is orthogonal to how to model FSL MC in IORT, that has to be discussed
> but I want to have a full picture in mind first).
>
> When you probe the FSL MC as a platform device, the ACPI core,
> through IORT (if you add the 1:1 mapping as an array of single
> mappings) already link the platform device to ITS platform
> device MSI domain (acpi_configure_pmsi_domain()).
>
> The associated fwnode is the *same* (IIUC) as for the
> DOMAIN_BUS_FSL_MC_MSI and ITS DOMAIN_BUS_NEXUS, so in practice
> you don't need IORT code to retrieve the DOMAIN_BUS_FSL_MC_MSI
> domain, the fwnode is the same as the one in the FSL MC platform
> device IRQ domain->fwnode pointer and you can use it to
> retrieve the DOMAIN_BUS_FSL_MC_MSI domain through it.
>
> Is my reading correct ?

Thank you very much for your effort! Really appreciated! Yes, the 
understanding is correct. I have prototyped this idea for DT, see below [1].
So, I get the fwnode from the platform device domain (because they are 
the same with the devices underneath the MC-BUS bridge) and use the 
fwnode to retrieve the MC-BUS domain.

>
> Overall, DOMAIN_BUS_FSL_MC_MSI is just an MSI layer to override the
> provide the MSI domain ->prepare hook (ie to stash the MC device id), no
> more (ie its_fsl_mc_msi_prepare()).
>
> That's it for the MSI layer - I need to figure out whether we *want* to
> extend IORT (and/or ACPI) to defined bindings for "additional busses",
> what I write above is a summary of my understanding, I have not made my
> mind up yet.
>
> As for the IOMMU code, it seems like the only thing needed is
> extending named components configuration to child devices,
> hierarchically.

Laurentiu used a similar approach for DMA configuration (again 
prototyped for DT). [2]
It involves wiring up a custom .dma_configure for our devices as anyway, 
it made little sense to pretend that these devices are platform devices 
and trick the DT or ACPI layers into that. As a nice side effect, this 
will allow to get rid of our existing hooks in the DT generic code.

>
> As Marc already mentioned, IOMMU and IRQ code must be separate for
> future postings but first we need to find a suitable answer to
> the problem at hand.
>
> Lorenzo
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[1] MSI configuration

  drivers/bus/fsl-mc/fsl-mc-msi.c | 11 +++++++++--
  1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/fsl-mc/fsl-mc-msi.c 
b/drivers/bus/fsl-mc/fsl-mc-msi.c
index 8b9c66d7c4ff..674f5a60109b 100644
--- a/drivers/bus/fsl-mc/fsl-mc-msi.c
+++ b/drivers/bus/fsl-mc/fsl-mc-msi.c
@@ -182,16 +182,23 @@ int fsl_mc_find_msi_domain(struct device 
*mc_platform_dev,
  {
      struct irq_domain *msi_domain;
      struct device_node *mc_of_node = mc_platform_dev->of_node;
+    struct fwnode_handle *fwnode;

-    msi_domain = of_msi_get_domain(mc_platform_dev, mc_of_node,
-                       DOMAIN_BUS_FSL_MC_MSI);
+    msi_domain = dev_get_msi_domain(mc_platform_dev);
      if (!msi_domain) {
          pr_err("Unable to find fsl-mc MSI domain for %pOF\n",
                 mc_of_node);

          return -ENOENT;
      }
+    fwnode = msi_domain->fwnode;
+    msi_domain = irq_find_matching_fwnode(fwnode, DOMAIN_BUS_FSL_MC_MSI);
+    if (!msi_domain) {
+        pr_err("Unable to find fsl-mc MSI domain for %pOF\n",
+              mc_of_node);

+        return -ENOENT;
+    }
      *mc_msi_domain = msi_domain;
      return 0;
  }
-- 
2.17.1



[2] DMA configuration

  drivers/bus/fsl-mc/fsl-mc-bus.c | 42 ++++++++++++++++++++++++++++++++-
  1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c 
b/drivers/bus/fsl-mc/fsl-mc-bus.c
index f9bc9c384ab5..5c6021a13612 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -132,11 +132,51 @@ static int fsl_mc_bus_uevent(struct device *dev, 
struct kobj_uevent_env *env)
  static int fsl_mc_dma_configure(struct device *dev)
  {
      struct device *dma_dev = dev;
+    struct iommu_fwspec *fwspec;
+    const struct iommu_ops *iommu_ops;
+    struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
+    int ret;
+    u32 icid;

      while (dev_is_fsl_mc(dma_dev))
          dma_dev = dma_dev->parent;

-    return of_dma_configure(dev, dma_dev->of_node, 0);
+    fwspec = dev_iommu_fwspec_get(dma_dev);
+    if (!fwspec) {
+        dev_err(dev, "%s: null fwspec\n", __func__);
+        return -ENODEV;
+    }
+    iommu_ops = iommu_ops_from_fwnode(fwspec->iommu_fwnode);
+    if (!iommu_ops) {
+        dev_err(dev, "%s: null iommu ops\n", __func__);
+        return -ENODEV;
+    }
+
+    ret = iommu_fwspec_init(dev, fwspec->iommu_fwnode, iommu_ops);
+    if (ret) {
+        dev_err(dev, "%s: iommu_fwspec_init failed with %d\n", 
__func__, ret);
+        return ret;
+    }
+
+    icid = mc_dev->icid;
+    ret = iommu_fwspec_add_ids(dev, &icid, 1);
+    if (ret) {
+        dev_err(dev, "%s: iommu_fwspec_add_ids failed with %d\n", 
__func__, ret);
+        return ret;
+    }
+
+    if (!device_iommu_mapped(dev)) {
+        ret = iommu_probe_device(dev);
+        if (ret) {
+            dev_err(dev, "%s: iommu_fwspec_add_ids failed with %d\n", 
__func__, ret);
+            return ret;
+        }
+    }
+
+    arch_setup_dma_ops(dev, 0, *dma_dev->dma_mask + 1,
+                iommu_ops, true);
+
+    return 0;
  }

  static ssize_t modalias_show(struct device *dev, struct 
device_attribute *attr,
-- 
2.17.1

Regards,
Diana