[v2,12/12] bus: fsl-mc: Add ACPI support for fsl-mc

Commit Message

Lorenzo Pieralisi June 19, 2020, 8:20 a.m. UTC

From: Makarand Pawagi <[e-mail]>

Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table to
extract memory and other resources.

Interrupt (GIC ITS) information is extracted from the MADT table
by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.

IORT table is parsed to configure DMA.

Signed-off-by: Makarand Pawagi <[e-mail]>
Signed-off-by: Diana Craciun <[e-mail]>
Signed-off-by: Laurentiu Tudor <[e-mail]>
---
 drivers/bus/fsl-mc/fsl-mc-bus.c             | 73 ++++++++++++----
 drivers/bus/fsl-mc/fsl-mc-msi.c             | 37 +++++----
 drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92 ++++++++++++++++-----
 3 files changed, 150 insertions(+), 52 deletions(-)

Comments

Laurentiu Tudor July 1, 2020, 4:55 p.m. UTC | #1

On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote:
> From: Makarand Pawagi <[e-mail]>
> 
> Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table to
> extract memory and other resources.
> 
> Interrupt (GIC ITS) information is extracted from the MADT table
> by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> 
> IORT table is parsed to configure DMA.
> 
> Signed-off-by: Makarand Pawagi <[e-mail]>
> Signed-off-by: Diana Craciun <[e-mail]>
> Signed-off-by: Laurentiu Tudor <[e-mail]>
> ---
>  drivers/bus/fsl-mc/fsl-mc-bus.c             | 73 ++++++++++++----
>  drivers/bus/fsl-mc/fsl-mc-msi.c             | 37 +++++----
>  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92 ++++++++++++++++-----
>  3 files changed, 150 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> index 824ff77bbe86..324d49d6df89 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> @@ -18,6 +18,8 @@
>  #include <linux/bitops.h>
>  #include <linux/msi.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/acpi.h>
> +#include <linux/iommu.h>
>  
>  #include "fsl-mc-private.h"
>  
> @@ -38,6 +40,7 @@ struct fsl_mc {
>  	struct fsl_mc_device *root_mc_bus_dev;
>  	u8 num_translation_ranges;
>  	struct fsl_mc_addr_translation_range *translation_ranges;
> +	void *fsl_mc_regs;
>  };
>  
>  /**
> @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range {
>  	phys_addr_t start_phys_addr;
>  };
>  
> +#define FSL_MC_FAPR	0x28
> +#define MC_FAPR_PL	BIT(18)
> +#define MC_FAPR_BMT	BIT(17)
> +
>  /**
>   * fsl_mc_bus_match - device to driver matching callback
>   * @dev: the fsl-mc device to match against
> @@ -124,7 +131,10 @@ static int fsl_mc_dma_configure(struct device *dev)
>  	while (dev_is_fsl_mc(dma_dev))
>  		dma_dev = dma_dev->parent;
>  
> -	return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
> +	if (dev_of_node(dma_dev))
> +		return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
> +
> +	return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
>  }
>  
>  static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
> @@ -865,8 +875,11 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
>  	struct fsl_mc_io *mc_io = NULL;
>  	int container_id;
>  	phys_addr_t mc_portal_phys_addr;
> -	u32 mc_portal_size;
> -	struct resource res;
> +	u32 mc_portal_size, mc_stream_id;
> +	struct resource *plat_res;
> +
> +	if (!iommu_present(&fsl_mc_bus_type))
> +		return -EPROBE_DEFER;
>  
>  	mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
>  	if (!mc)
> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, mc);
>  
> +	plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res);
> +	if (IS_ERR(mc->fsl_mc_regs))
> +		return PTR_ERR(mc->fsl_mc_regs);
> +
> +	if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) {
> +		mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR);
> +		/*
> +		 * HW ORs the PL and BMT bit, places the result in bit 15 of
> +		 * the StreamID and ORs in the ICID. Calculate it accordingly.
> +		 */
> +		mc_stream_id = (mc_stream_id & 0xffff) |
> +				((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ?
> +					0x4000 : 0);
> +		error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT,
> +					      &mc_stream_id);
> +		if (error)
> +			dev_warn(&pdev->dev, "failed to configure dma: %d.\n",
> +				 error);
> +	}
> +
>  	/*
>  	 * Get physical address of MC portal for the root DPRC:
>  	 */
> -	error = of_address_to_resource(pdev->dev.of_node, 0, &res);
> -	if (error < 0) {
> -		dev_err(&pdev->dev,
> -			"of_address_to_resource() failed for %pOF\n",
> -			pdev->dev.of_node);
> -		return error;
> -	}
> -
> -	mc_portal_phys_addr = res.start;
> -	mc_portal_size = resource_size(&res);
> +	plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mc_portal_phys_addr = plat_res->start;
> +	mc_portal_size = resource_size(plat_res);
>  	error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr,
>  				 mc_portal_size, NULL,
>  				 FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, &mc_io);
> @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
>  	dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n",
>  		 mc_version.major, mc_version.minor, mc_version.revision);
>  
> -	error = get_mc_addr_translation_ranges(&pdev->dev,
> -					       &mc->translation_ranges,
> -					       &mc->num_translation_ranges);
> -	if (error < 0)
> -		goto error_cleanup_mc_io;
> +	if (dev_of_node(&pdev->dev)) {
> +		error = get_mc_addr_translation_ranges(&pdev->dev,
> +						&mc->translation_ranges,
> +						&mc->num_translation_ranges);
> +		if (error < 0)
> +			goto error_cleanup_mc_io;
> +	}
>  
>  	error = dprc_get_container_id(mc_io, 0, &container_id);
>  	if (error < 0) {
> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
>  		goto error_cleanup_mc_io;
>  
>  	mc->root_mc_bus_dev = mc_bus_dev;
> +	mc_bus_dev->dev.fwnode = pdev->dev.fwnode;

Makarand, this looks a bit weird. Is there really a reason for it?

---
Best Regards, Laurentiu

Lorenzo Pieralisi July 9, 2020, 9:19 a.m. UTC | #2

On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote:
> 
> 
> On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote:
> > From: Makarand Pawagi <[e-mail]>
> > 
> > Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table to
> > extract memory and other resources.
> > 
> > Interrupt (GIC ITS) information is extracted from the MADT table
> > by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> > 
> > IORT table is parsed to configure DMA.
> > 
> > Signed-off-by: Makarand Pawagi <[e-mail]>
> > Signed-off-by: Diana Craciun <[e-mail]>
> > Signed-off-by: Laurentiu Tudor <[e-mail]>
> > ---
> >  drivers/bus/fsl-mc/fsl-mc-bus.c             | 73 ++++++++++++----
> >  drivers/bus/fsl-mc/fsl-mc-msi.c             | 37 +++++----
> >  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92 ++++++++++++++++-----
> >  3 files changed, 150 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > index 824ff77bbe86..324d49d6df89 100644
> > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > @@ -18,6 +18,8 @@
> >  #include <linux/bitops.h>
> >  #include <linux/msi.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/acpi.h>
> > +#include <linux/iommu.h>
> >  
> >  #include "fsl-mc-private.h"
> >  
> > @@ -38,6 +40,7 @@ struct fsl_mc {
> >  	struct fsl_mc_device *root_mc_bus_dev;
> >  	u8 num_translation_ranges;
> >  	struct fsl_mc_addr_translation_range *translation_ranges;
> > +	void *fsl_mc_regs;
> >  };
> >  
> >  /**
> > @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range {
> >  	phys_addr_t start_phys_addr;
> >  };
> >  
> > +#define FSL_MC_FAPR	0x28
> > +#define MC_FAPR_PL	BIT(18)
> > +#define MC_FAPR_BMT	BIT(17)
> > +
> >  /**
> >   * fsl_mc_bus_match - device to driver matching callback
> >   * @dev: the fsl-mc device to match against
> > @@ -124,7 +131,10 @@ static int fsl_mc_dma_configure(struct device *dev)
> >  	while (dev_is_fsl_mc(dma_dev))
> >  		dma_dev = dma_dev->parent;
> >  
> > -	return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
> > +	if (dev_of_node(dma_dev))
> > +		return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
> > +
> > +	return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
> >  }
> >  
> >  static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
> > @@ -865,8 +875,11 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
> >  	struct fsl_mc_io *mc_io = NULL;
> >  	int container_id;
> >  	phys_addr_t mc_portal_phys_addr;
> > -	u32 mc_portal_size;
> > -	struct resource res;
> > +	u32 mc_portal_size, mc_stream_id;
> > +	struct resource *plat_res;
> > +
> > +	if (!iommu_present(&fsl_mc_bus_type))
> > +		return -EPROBE_DEFER;
> >  
> >  	mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
> >  	if (!mc)
> > @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, mc);
> >  
> > +	plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res);
> > +	if (IS_ERR(mc->fsl_mc_regs))
> > +		return PTR_ERR(mc->fsl_mc_regs);
> > +
> > +	if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) {
> > +		mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR);
> > +		/*
> > +		 * HW ORs the PL and BMT bit, places the result in bit 15 of
> > +		 * the StreamID and ORs in the ICID. Calculate it accordingly.
> > +		 */
> > +		mc_stream_id = (mc_stream_id & 0xffff) |
> > +				((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ?
> > +					0x4000 : 0);
> > +		error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT,
> > +					      &mc_stream_id);
> > +		if (error)
> > +			dev_warn(&pdev->dev, "failed to configure dma: %d.\n",
> > +				 error);
> > +	}
> > +
> >  	/*
> >  	 * Get physical address of MC portal for the root DPRC:
> >  	 */
> > -	error = of_address_to_resource(pdev->dev.of_node, 0, &res);
> > -	if (error < 0) {
> > -		dev_err(&pdev->dev,
> > -			"of_address_to_resource() failed for %pOF\n",
> > -			pdev->dev.of_node);
> > -		return error;
> > -	}
> > -
> > -	mc_portal_phys_addr = res.start;
> > -	mc_portal_size = resource_size(&res);
> > +	plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	mc_portal_phys_addr = plat_res->start;
> > +	mc_portal_size = resource_size(plat_res);
> >  	error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr,
> >  				 mc_portal_size, NULL,
> >  				 FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, &mc_io);
> > @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
> >  	dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n",
> >  		 mc_version.major, mc_version.minor, mc_version.revision);
> >  
> > -	error = get_mc_addr_translation_ranges(&pdev->dev,
> > -					       &mc->translation_ranges,
> > -					       &mc->num_translation_ranges);
> > -	if (error < 0)
> > -		goto error_cleanup_mc_io;
> > +	if (dev_of_node(&pdev->dev)) {
> > +		error = get_mc_addr_translation_ranges(&pdev->dev,
> > +						&mc->translation_ranges,
> > +						&mc->num_translation_ranges);
> > +		if (error < 0)
> > +			goto error_cleanup_mc_io;
> > +	}
> >  
> >  	error = dprc_get_container_id(mc_io, 0, &container_id);
> >  	if (error < 0) {
> > @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
> >  		goto error_cleanup_mc_io;
> >  
> >  	mc->root_mc_bus_dev = mc_bus_dev;
> > +	mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
> 
> Makarand, this looks a bit weird. Is there really a reason for it?

Can you clarify please so that we can reach a conclusion on this matter ?

Thanks,
Lorenzo

Makarand Pawagi July 9, 2020, 9:26 a.m. UTC | #3

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: Thursday, July 9, 2020 2:50 PM
> To: Laurentiu Tudor <[e-mail]>
> Cc: [e-mail]; Makarand Pawagi
> <[e-mail]>; Diana Madalina Craciun (OSS)
> <[e-mail]>; [e-mail]; linux-
> [e-mail]; [e-mail]; [e-mail];
> Rob Herring <[e-mail]>; Rafael J. Wysocki <[e-mail]>;
> Joerg Roedel <[e-mail]>; Hanjun Guo <[e-mail]>; Bjorn
> Helgaas <[e-mail]>; Sudeep Holla <[e-mail]>;
> Robin Murphy <[e-mail]>; Catalin Marinas
> <[e-mail]>; Will Deacon <[e-mail]>; Marc Zyngier
> <[e-mail]>
> Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc
> 
> Caution: EXT Email
> 
> On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote:
> >
> >
> > On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote:
> > > From: Makarand Pawagi <[e-mail]>
> > >
> > > Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table
> > > to extract memory and other resources.
> > >
> > > Interrupt (GIC ITS) information is extracted from the MADT table by
> > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> > >
> > > IORT table is parsed to configure DMA.
> > >
> > > Signed-off-by: Makarand Pawagi <[e-mail]>
> > > Signed-off-by: Diana Craciun <[e-mail]>
> > > Signed-off-by: Laurentiu Tudor <[e-mail]>
> > > ---
> > >  drivers/bus/fsl-mc/fsl-mc-bus.c             | 73 ++++++++++++----
> > >  drivers/bus/fsl-mc/fsl-mc-msi.c             | 37 +++++----
> > >  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92
> > > ++++++++++++++++-----
> > >  3 files changed, 150 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > b/drivers/bus/fsl-mc/fsl-mc-bus.c index 824ff77bbe86..324d49d6df89
> > > 100644
> > > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > @@ -18,6 +18,8 @@
> > >  #include <linux/bitops.h>
> > >  #include <linux/msi.h>
> > >  #include <linux/dma-mapping.h>
> > > +#include <linux/acpi.h>
> > > +#include <linux/iommu.h>
> > >
> > >  #include "fsl-mc-private.h"
> > >
> > > @@ -38,6 +40,7 @@ struct fsl_mc {
> > >     struct fsl_mc_device *root_mc_bus_dev;
> > >     u8 num_translation_ranges;
> > >     struct fsl_mc_addr_translation_range *translation_ranges;
> > > +   void *fsl_mc_regs;
> > >  };
> > >
> > >  /**
> > > @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range {
> > >     phys_addr_t start_phys_addr;
> > >  };
> > >
> > > +#define FSL_MC_FAPR        0x28
> > > +#define MC_FAPR_PL BIT(18)
> > > +#define MC_FAPR_BMT        BIT(17)
> > > +
> > >  /**
> > >   * fsl_mc_bus_match - device to driver matching callback
> > >   * @dev: the fsl-mc device to match against @@ -124,7 +131,10 @@
> > > static int fsl_mc_dma_configure(struct device *dev)
> > >     while (dev_is_fsl_mc(dma_dev))
> > >             dma_dev = dma_dev->parent;
> > >
> > > -   return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
> > > +   if (dev_of_node(dma_dev))
> > > +           return of_dma_configure_id(dev, dma_dev->of_node, 0,
> > > + &input_id);
> > > +
> > > +   return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
> > >  }
> > >
> > >  static ssize_t modalias_show(struct device *dev, struct
> > > device_attribute *attr, @@ -865,8 +875,11 @@ static int
> fsl_mc_bus_probe(struct platform_device *pdev)
> > >     struct fsl_mc_io *mc_io = NULL;
> > >     int container_id;
> > >     phys_addr_t mc_portal_phys_addr;
> > > -   u32 mc_portal_size;
> > > -   struct resource res;
> > > +   u32 mc_portal_size, mc_stream_id;
> > > +   struct resource *plat_res;
> > > +
> > > +   if (!iommu_present(&fsl_mc_bus_type))
> > > +           return -EPROBE_DEFER;
> > >
> > >     mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
> > >     if (!mc)
> > > @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct
> > > platform_device *pdev)
> > >
> > >     platform_set_drvdata(pdev, mc);
> > >
> > > +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > +   mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res);
> > > +   if (IS_ERR(mc->fsl_mc_regs))
> > > +           return PTR_ERR(mc->fsl_mc_regs);
> > > +
> > > +   if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) {
> > > +           mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR);
> > > +           /*
> > > +            * HW ORs the PL and BMT bit, places the result in bit 15 of
> > > +            * the StreamID and ORs in the ICID. Calculate it accordingly.
> > > +            */
> > > +           mc_stream_id = (mc_stream_id & 0xffff) |
> > > +                           ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ?
> > > +                                   0x4000 : 0);
> > > +           error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT,
> > > +                                         &mc_stream_id);
> > > +           if (error)
> > > +                   dev_warn(&pdev->dev, "failed to configure dma: %d.\n",
> > > +                            error);
> > > +   }
> > > +
> > >     /*
> > >      * Get physical address of MC portal for the root DPRC:
> > >      */
> > > -   error = of_address_to_resource(pdev->dev.of_node, 0, &res);
> > > -   if (error < 0) {
> > > -           dev_err(&pdev->dev,
> > > -                   "of_address_to_resource() failed for %pOF\n",
> > > -                   pdev->dev.of_node);
> > > -           return error;
> > > -   }
> > > -
> > > -   mc_portal_phys_addr = res.start;
> > > -   mc_portal_size = resource_size(&res);
> > > +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +   mc_portal_phys_addr = plat_res->start;
> > > +   mc_portal_size = resource_size(plat_res);
> > >     error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr,
> > >                              mc_portal_size, NULL,
> > >                              FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
> > > &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct
> platform_device *pdev)
> > >     dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n",
> > >              mc_version.major, mc_version.minor,
> > > mc_version.revision);
> > >
> > > -   error = get_mc_addr_translation_ranges(&pdev->dev,
> > > -                                          &mc->translation_ranges,
> > > -                                          &mc->num_translation_ranges);
> > > -   if (error < 0)
> > > -           goto error_cleanup_mc_io;
> > > +   if (dev_of_node(&pdev->dev)) {
> > > +           error = get_mc_addr_translation_ranges(&pdev->dev,
> > > +                                           &mc->translation_ranges,
> > > +                                           &mc->num_translation_ranges);
> > > +           if (error < 0)
> > > +                   goto error_cleanup_mc_io;
> > > +   }
> > >
> > >     error = dprc_get_container_id(mc_io, 0, &container_id);
> > >     if (error < 0) {
> > > @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct platform_device
> *pdev)
> > >             goto error_cleanup_mc_io;
> > >
> > >     mc->root_mc_bus_dev = mc_bus_dev;
> > > +   mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
> >
> > Makarand, this looks a bit weird. Is there really a reason for it?
> 
> Can you clarify please so that we can reach a conclusion on this matter ?
> 
Laurentiu, can you clarify what exactly is the doubt here? Are you asking about fwnode assignment from pdev to mc_bus_dev?

> Thanks,
> Lorenzo

Laurentiu Tudor July 9, 2020, 10:14 a.m. UTC | #4

On 7/9/2020 12:26 PM, Makarand Pawagi wrote:
> 
> 
>> -----Original Message-----
>> From: Lorenzo Pieralisi <[e-mail]>
>> Sent: Thursday, July 9, 2020 2:50 PM
>> To: Laurentiu Tudor <[e-mail]>
>> Cc: [e-mail]; Makarand Pawagi
>> <[e-mail]>; Diana Madalina Craciun (OSS)
>> <[e-mail]>; [e-mail]; linux-
>> [e-mail]; [e-mail]; [e-mail];
>> Rob Herring <[e-mail]>; Rafael J. Wysocki <[e-mail]>;
>> Joerg Roedel <[e-mail]>; Hanjun Guo <[e-mail]>; Bjorn
>> Helgaas <[e-mail]>; Sudeep Holla <[e-mail]>;
>> Robin Murphy <[e-mail]>; Catalin Marinas
>> <[e-mail]>; Will Deacon <[e-mail]>; Marc Zyngier
>> <[e-mail]>
>> Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc
>>
>> Caution: EXT Email
>>
>> On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote:
>>>
>>>
>>> On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote:
>>>> From: Makarand Pawagi <[e-mail]>
>>>>
>>>> Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table
>>>> to extract memory and other resources.
>>>>
>>>> Interrupt (GIC ITS) information is extracted from the MADT table by
>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
>>>>
>>>> IORT table is parsed to configure DMA.
>>>>
>>>> Signed-off-by: Makarand Pawagi <[e-mail]>
>>>> Signed-off-by: Diana Craciun <[e-mail]>
>>>> Signed-off-by: Laurentiu Tudor <[e-mail]>
>>>> ---
>>>>  drivers/bus/fsl-mc/fsl-mc-bus.c             | 73 ++++++++++++----
>>>>  drivers/bus/fsl-mc/fsl-mc-msi.c             | 37 +++++----
>>>>  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92
>>>> ++++++++++++++++-----
>>>>  3 files changed, 150 insertions(+), 52 deletions(-)
>>>>
>>>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c
>>>> b/drivers/bus/fsl-mc/fsl-mc-bus.c index 824ff77bbe86..324d49d6df89
>>>> 100644
>>>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
>>>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
>>>> @@ -18,6 +18,8 @@
>>>>  #include <linux/bitops.h>
>>>>  #include <linux/msi.h>
>>>>  #include <linux/dma-mapping.h>
>>>> +#include <linux/acpi.h>
>>>> +#include <linux/iommu.h>
>>>>
>>>>  #include "fsl-mc-private.h"
>>>>
>>>> @@ -38,6 +40,7 @@ struct fsl_mc {
>>>>     struct fsl_mc_device *root_mc_bus_dev;
>>>>     u8 num_translation_ranges;
>>>>     struct fsl_mc_addr_translation_range *translation_ranges;
>>>> +   void *fsl_mc_regs;
>>>>  };
>>>>
>>>>  /**
>>>> @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range {
>>>>     phys_addr_t start_phys_addr;
>>>>  };
>>>>
>>>> +#define FSL_MC_FAPR        0x28
>>>> +#define MC_FAPR_PL BIT(18)
>>>> +#define MC_FAPR_BMT        BIT(17)
>>>> +
>>>>  /**
>>>>   * fsl_mc_bus_match - device to driver matching callback
>>>>   * @dev: the fsl-mc device to match against @@ -124,7 +131,10 @@
>>>> static int fsl_mc_dma_configure(struct device *dev)
>>>>     while (dev_is_fsl_mc(dma_dev))
>>>>             dma_dev = dma_dev->parent;
>>>>
>>>> -   return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
>>>> +   if (dev_of_node(dma_dev))
>>>> +           return of_dma_configure_id(dev, dma_dev->of_node, 0,
>>>> + &input_id);
>>>> +
>>>> +   return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
>>>>  }
>>>>
>>>>  static ssize_t modalias_show(struct device *dev, struct
>>>> device_attribute *attr, @@ -865,8 +875,11 @@ static int
>> fsl_mc_bus_probe(struct platform_device *pdev)
>>>>     struct fsl_mc_io *mc_io = NULL;
>>>>     int container_id;
>>>>     phys_addr_t mc_portal_phys_addr;
>>>> -   u32 mc_portal_size;
>>>> -   struct resource res;
>>>> +   u32 mc_portal_size, mc_stream_id;
>>>> +   struct resource *plat_res;
>>>> +
>>>> +   if (!iommu_present(&fsl_mc_bus_type))
>>>> +           return -EPROBE_DEFER;
>>>>
>>>>     mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
>>>>     if (!mc)
>>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct
>>>> platform_device *pdev)
>>>>
>>>>     platform_set_drvdata(pdev, mc);
>>>>
>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>> +   mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res);
>>>> +   if (IS_ERR(mc->fsl_mc_regs))
>>>> +           return PTR_ERR(mc->fsl_mc_regs);
>>>> +
>>>> +   if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) {
>>>> +           mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR);
>>>> +           /*
>>>> +            * HW ORs the PL and BMT bit, places the result in bit 15 of
>>>> +            * the StreamID and ORs in the ICID. Calculate it accordingly.
>>>> +            */
>>>> +           mc_stream_id = (mc_stream_id & 0xffff) |
>>>> +                           ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ?
>>>> +                                   0x4000 : 0);
>>>> +           error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT,
>>>> +                                         &mc_stream_id);
>>>> +           if (error)
>>>> +                   dev_warn(&pdev->dev, "failed to configure dma: %d.\n",
>>>> +                            error);
>>>> +   }
>>>> +
>>>>     /*
>>>>      * Get physical address of MC portal for the root DPRC:
>>>>      */
>>>> -   error = of_address_to_resource(pdev->dev.of_node, 0, &res);
>>>> -   if (error < 0) {
>>>> -           dev_err(&pdev->dev,
>>>> -                   "of_address_to_resource() failed for %pOF\n",
>>>> -                   pdev->dev.of_node);
>>>> -           return error;
>>>> -   }
>>>> -
>>>> -   mc_portal_phys_addr = res.start;
>>>> -   mc_portal_size = resource_size(&res);
>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +   mc_portal_phys_addr = plat_res->start;
>>>> +   mc_portal_size = resource_size(plat_res);
>>>>     error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr,
>>>>                              mc_portal_size, NULL,
>>>>                              FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
>>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct
>> platform_device *pdev)
>>>>     dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n",
>>>>              mc_version.major, mc_version.minor,
>>>> mc_version.revision);
>>>>
>>>> -   error = get_mc_addr_translation_ranges(&pdev->dev,
>>>> -                                          &mc->translation_ranges,
>>>> -                                          &mc->num_translation_ranges);
>>>> -   if (error < 0)
>>>> -           goto error_cleanup_mc_io;
>>>> +   if (dev_of_node(&pdev->dev)) {
>>>> +           error = get_mc_addr_translation_ranges(&pdev->dev,
>>>> +                                           &mc->translation_ranges,
>>>> +                                           &mc->num_translation_ranges);
>>>> +           if (error < 0)
>>>> +                   goto error_cleanup_mc_io;
>>>> +   }
>>>>
>>>>     error = dprc_get_container_id(mc_io, 0, &container_id);
>>>>     if (error < 0) {
>>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct platform_device
>> *pdev)
>>>>             goto error_cleanup_mc_io;
>>>>
>>>>     mc->root_mc_bus_dev = mc_bus_dev;
>>>> +   mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
>>>
>>> Makarand, this looks a bit weird. Is there really a reason for it?
>>
>> Can you clarify please so that we can reach a conclusion on this matter ?
>>
> Laurentiu, can you clarify what exactly is the doubt here? Are you asking about fwnode assignment from pdev to mc_bus_dev?
> 

Yes. I remember that a while ago I tested without this fwnode assignment
and didn't encounter any issues. Maybe we can just drop it?

---
Best Regards, Laurentiu

Makarand Pawagi July 9, 2020, 10:37 a.m. UTC | #5

> -----Original Message-----
> From: Laurentiu Tudor <[e-mail]>
> Sent: Thursday, July 9, 2020 3:45 PM
> To: Makarand Pawagi <[e-mail]>; Lorenzo Pieralisi
> <[e-mail]>
> Cc: [e-mail]; Diana Madalina Craciun (OSS)
> <[e-mail]>; [e-mail]; linux-
> [e-mail]; [e-mail]; [e-mail];
> Rob Herring <[e-mail]>; Rafael J. Wysocki <[e-mail]>;
> Joerg Roedel <[e-mail]>; Hanjun Guo <[e-mail]>; Bjorn
> Helgaas <[e-mail]>; Sudeep Holla <[e-mail]>;
> Robin Murphy <[e-mail]>; Catalin Marinas
> <[e-mail]>; Will Deacon <[e-mail]>; Marc Zyngier
> <[e-mail]>
> Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc
> 
> 
> 
> On 7/9/2020 12:26 PM, Makarand Pawagi wrote:
> >
> >
> >> -----Original Message-----
> >> From: Lorenzo Pieralisi <[e-mail]>
> >> Sent: Thursday, July 9, 2020 2:50 PM
> >> To: Laurentiu Tudor <[e-mail]>
> >> Cc: [e-mail]; Makarand Pawagi
> >> <[e-mail]>; Diana Madalina Craciun (OSS)
> >> <[e-mail]>; [e-mail]; linux-
> >> [e-mail]; [e-mail];
> >> [e-mail]; Rob Herring <[e-mail]>; Rafael
> >> J. Wysocki <[e-mail]>; Joerg Roedel <[e-mail]>;
> >> Hanjun Guo <[e-mail]>; Bjorn Helgaas
> >> <[e-mail]>; Sudeep Holla <[e-mail]>; Robin
> >> Murphy <[e-mail]>; Catalin Marinas
> >> <[e-mail]>; Will Deacon <[e-mail]>; Marc
> >> Zyngier <[e-mail]>
> >> Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for
> >> fsl-mc
> >>
> >> Caution: EXT Email
> >>
> >> On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote:
> >>>
> >>>
> >>> On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote:
> >>>> From: Makarand Pawagi <[e-mail]>
> >>>>
> >>>> Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table
> >>>> to extract memory and other resources.
> >>>>
> >>>> Interrupt (GIC ITS) information is extracted from the MADT table by
> >>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> >>>>
> >>>> IORT table is parsed to configure DMA.
> >>>>
> >>>> Signed-off-by: Makarand Pawagi <[e-mail]>
> >>>> Signed-off-by: Diana Craciun <[e-mail]>
> >>>> Signed-off-by: Laurentiu Tudor <[e-mail]>
> >>>> ---
> >>>>  drivers/bus/fsl-mc/fsl-mc-bus.c             | 73 ++++++++++++----
> >>>>  drivers/bus/fsl-mc/fsl-mc-msi.c             | 37 +++++----
> >>>>  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92
> >>>> ++++++++++++++++-----
> >>>>  3 files changed, 150 insertions(+), 52 deletions(-)
> >>>>
> >>>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c
> >>>> b/drivers/bus/fsl-mc/fsl-mc-bus.c index 824ff77bbe86..324d49d6df89
> >>>> 100644
> >>>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> >>>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> >>>> @@ -18,6 +18,8 @@
> >>>>  #include <linux/bitops.h>
> >>>>  #include <linux/msi.h>
> >>>>  #include <linux/dma-mapping.h>
> >>>> +#include <linux/acpi.h>
> >>>> +#include <linux/iommu.h>
> >>>>
> >>>>  #include "fsl-mc-private.h"
> >>>>
> >>>> @@ -38,6 +40,7 @@ struct fsl_mc {
> >>>>     struct fsl_mc_device *root_mc_bus_dev;
> >>>>     u8 num_translation_ranges;
> >>>>     struct fsl_mc_addr_translation_range *translation_ranges;
> >>>> +   void *fsl_mc_regs;
> >>>>  };
> >>>>
> >>>>  /**
> >>>> @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range {
> >>>>     phys_addr_t start_phys_addr;
> >>>>  };
> >>>>
> >>>> +#define FSL_MC_FAPR        0x28
> >>>> +#define MC_FAPR_PL BIT(18)
> >>>> +#define MC_FAPR_BMT        BIT(17)
> >>>> +
> >>>>  /**
> >>>>   * fsl_mc_bus_match - device to driver matching callback
> >>>>   * @dev: the fsl-mc device to match against @@ -124,7 +131,10 @@
> >>>> static int fsl_mc_dma_configure(struct device *dev)
> >>>>     while (dev_is_fsl_mc(dma_dev))
> >>>>             dma_dev = dma_dev->parent;
> >>>>
> >>>> -   return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
> >>>> +   if (dev_of_node(dma_dev))
> >>>> +           return of_dma_configure_id(dev, dma_dev->of_node, 0,
> >>>> + &input_id);
> >>>> +
> >>>> +   return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
> >>>>  }
> >>>>
> >>>>  static ssize_t modalias_show(struct device *dev, struct
> >>>> device_attribute *attr, @@ -865,8 +875,11 @@ static int
> >> fsl_mc_bus_probe(struct platform_device *pdev)
> >>>>     struct fsl_mc_io *mc_io = NULL;
> >>>>     int container_id;
> >>>>     phys_addr_t mc_portal_phys_addr;
> >>>> -   u32 mc_portal_size;
> >>>> -   struct resource res;
> >>>> +   u32 mc_portal_size, mc_stream_id;
> >>>> +   struct resource *plat_res;
> >>>> +
> >>>> +   if (!iommu_present(&fsl_mc_bus_type))
> >>>> +           return -EPROBE_DEFER;
> >>>>
> >>>>     mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
> >>>>     if (!mc)
> >>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct
> >>>> platform_device *pdev)
> >>>>
> >>>>     platform_set_drvdata(pdev, mc);
> >>>>
> >>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >>>> +   mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res);
> >>>> +   if (IS_ERR(mc->fsl_mc_regs))
> >>>> +           return PTR_ERR(mc->fsl_mc_regs);
> >>>> +
> >>>> +   if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) {
> >>>> +           mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR);
> >>>> +           /*
> >>>> +            * HW ORs the PL and BMT bit, places the result in bit 15 of
> >>>> +            * the StreamID and ORs in the ICID. Calculate it accordingly.
> >>>> +            */
> >>>> +           mc_stream_id = (mc_stream_id & 0xffff) |
> >>>> +                           ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ?
> >>>> +                                   0x4000 : 0);
> >>>> +           error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT,
> >>>> +                                         &mc_stream_id);
> >>>> +           if (error)
> >>>> +                   dev_warn(&pdev->dev, "failed to configure dma: %d.\n",
> >>>> +                            error);
> >>>> +   }
> >>>> +
> >>>>     /*
> >>>>      * Get physical address of MC portal for the root DPRC:
> >>>>      */
> >>>> -   error = of_address_to_resource(pdev->dev.of_node, 0, &res);
> >>>> -   if (error < 0) {
> >>>> -           dev_err(&pdev->dev,
> >>>> -                   "of_address_to_resource() failed for %pOF\n",
> >>>> -                   pdev->dev.of_node);
> >>>> -           return error;
> >>>> -   }
> >>>> -
> >>>> -   mc_portal_phys_addr = res.start;
> >>>> -   mc_portal_size = resource_size(&res);
> >>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>>> +   mc_portal_phys_addr = plat_res->start;
> >>>> +   mc_portal_size = resource_size(plat_res);
> >>>>     error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr,
> >>>>                              mc_portal_size, NULL,
> >>>>                              FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
> >>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct
> >> platform_device *pdev)
> >>>>     dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n",
> >>>>              mc_version.major, mc_version.minor,
> >>>> mc_version.revision);
> >>>>
> >>>> -   error = get_mc_addr_translation_ranges(&pdev->dev,
> >>>> -                                          &mc->translation_ranges,
> >>>> -                                          &mc->num_translation_ranges);
> >>>> -   if (error < 0)
> >>>> -           goto error_cleanup_mc_io;
> >>>> +   if (dev_of_node(&pdev->dev)) {
> >>>> +           error = get_mc_addr_translation_ranges(&pdev->dev,
> >>>> +                                           &mc->translation_ranges,
> >>>> +                                           &mc->num_translation_ranges);
> >>>> +           if (error < 0)
> >>>> +                   goto error_cleanup_mc_io;
> >>>> +   }
> >>>>
> >>>>     error = dprc_get_container_id(mc_io, 0, &container_id);
> >>>>     if (error < 0) {
> >>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct
> >>>> platform_device
> >> *pdev)
> >>>>             goto error_cleanup_mc_io;
> >>>>
> >>>>     mc->root_mc_bus_dev = mc_bus_dev;
> >>>> +   mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
> >>>
> >>> Makarand, this looks a bit weird. Is there really a reason for it?
> >>
> >> Can you clarify please so that we can reach a conclusion on this matter ?
> >>
> > Laurentiu, can you clarify what exactly is the doubt here? Are you asking about
> fwnode assignment from pdev to mc_bus_dev?
> >
> 
> Yes. I remember that a while ago I tested without this fwnode assignment and
> didn't encounter any issues. Maybe we can just drop it?

Did you tested with PHY changes? Because this is needed for MAC driver, where it needs the mc bus node.

> 
> ---
> Best Regards, Laurentiu

Laurentiu Tudor July 9, 2020, 10:39 a.m. UTC | #6

On 7/9/2020 1:37 PM, Makarand Pawagi wrote:
> 
> 
>> -----Original Message-----
>> From: Laurentiu Tudor <[e-mail]>
>> Sent: Thursday, July 9, 2020 3:45 PM
>> To: Makarand Pawagi <[e-mail]>; Lorenzo Pieralisi
>> <[e-mail]>
>> Cc: [e-mail]; Diana Madalina Craciun (OSS)
>> <[e-mail]>; [e-mail]; linux-
>> [e-mail]; [e-mail]; [e-mail];
>> Rob Herring <[e-mail]>; Rafael J. Wysocki <[e-mail]>;
>> Joerg Roedel <[e-mail]>; Hanjun Guo <[e-mail]>; Bjorn
>> Helgaas <[e-mail]>; Sudeep Holla <[e-mail]>;
>> Robin Murphy <[e-mail]>; Catalin Marinas
>> <[e-mail]>; Will Deacon <[e-mail]>; Marc Zyngier
>> <[e-mail]>
>> Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc
>>
>>
>>
>> On 7/9/2020 12:26 PM, Makarand Pawagi wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Lorenzo Pieralisi <[e-mail]>
>>>> Sent: Thursday, July 9, 2020 2:50 PM
>>>> To: Laurentiu Tudor <[e-mail]>
>>>> Cc: [e-mail]; Makarand Pawagi
>>>> <[e-mail]>; Diana Madalina Craciun (OSS)
>>>> <[e-mail]>; [e-mail]; linux-
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; Rob Herring <[e-mail]>; Rafael
>>>> J. Wysocki <[e-mail]>; Joerg Roedel <[e-mail]>;
>>>> Hanjun Guo <[e-mail]>; Bjorn Helgaas
>>>> <[e-mail]>; Sudeep Holla <[e-mail]>; Robin
>>>> Murphy <[e-mail]>; Catalin Marinas
>>>> <[e-mail]>; Will Deacon <[e-mail]>; Marc
>>>> Zyngier <[e-mail]>
>>>> Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for
>>>> fsl-mc
>>>>
>>>> Caution: EXT Email
>>>>
>>>> On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote:
>>>>>
>>>>>
>>>>> On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote:
>>>>>> From: Makarand Pawagi <[e-mail]>
>>>>>>
>>>>>> Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table
>>>>>> to extract memory and other resources.
>>>>>>
>>>>>> Interrupt (GIC ITS) information is extracted from the MADT table by
>>>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
>>>>>>
>>>>>> IORT table is parsed to configure DMA.
>>>>>>
>>>>>> Signed-off-by: Makarand Pawagi <[e-mail]>
>>>>>> Signed-off-by: Diana Craciun <[e-mail]>
>>>>>> Signed-off-by: Laurentiu Tudor <[e-mail]>
>>>>>> ---
>>>>>>  drivers/bus/fsl-mc/fsl-mc-bus.c             | 73 ++++++++++++----
>>>>>>  drivers/bus/fsl-mc/fsl-mc-msi.c             | 37 +++++----
>>>>>>  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92
>>>>>> ++++++++++++++++-----
>>>>>>  3 files changed, 150 insertions(+), 52 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c
>>>>>> b/drivers/bus/fsl-mc/fsl-mc-bus.c index 824ff77bbe86..324d49d6df89
>>>>>> 100644
>>>>>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
>>>>>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
>>>>>> @@ -18,6 +18,8 @@
>>>>>>  #include <linux/bitops.h>
>>>>>>  #include <linux/msi.h>
>>>>>>  #include <linux/dma-mapping.h>
>>>>>> +#include <linux/acpi.h>
>>>>>> +#include <linux/iommu.h>
>>>>>>
>>>>>>  #include "fsl-mc-private.h"
>>>>>>
>>>>>> @@ -38,6 +40,7 @@ struct fsl_mc {
>>>>>>     struct fsl_mc_device *root_mc_bus_dev;
>>>>>>     u8 num_translation_ranges;
>>>>>>     struct fsl_mc_addr_translation_range *translation_ranges;
>>>>>> +   void *fsl_mc_regs;
>>>>>>  };
>>>>>>
>>>>>>  /**
>>>>>> @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range {
>>>>>>     phys_addr_t start_phys_addr;
>>>>>>  };
>>>>>>
>>>>>> +#define FSL_MC_FAPR        0x28
>>>>>> +#define MC_FAPR_PL BIT(18)
>>>>>> +#define MC_FAPR_BMT        BIT(17)
>>>>>> +
>>>>>>  /**
>>>>>>   * fsl_mc_bus_match - device to driver matching callback
>>>>>>   * @dev: the fsl-mc device to match against @@ -124,7 +131,10 @@
>>>>>> static int fsl_mc_dma_configure(struct device *dev)
>>>>>>     while (dev_is_fsl_mc(dma_dev))
>>>>>>             dma_dev = dma_dev->parent;
>>>>>>
>>>>>> -   return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
>>>>>> +   if (dev_of_node(dma_dev))
>>>>>> +           return of_dma_configure_id(dev, dma_dev->of_node, 0,
>>>>>> + &input_id);
>>>>>> +
>>>>>> +   return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
>>>>>>  }
>>>>>>
>>>>>>  static ssize_t modalias_show(struct device *dev, struct
>>>>>> device_attribute *attr, @@ -865,8 +875,11 @@ static int
>>>> fsl_mc_bus_probe(struct platform_device *pdev)
>>>>>>     struct fsl_mc_io *mc_io = NULL;
>>>>>>     int container_id;
>>>>>>     phys_addr_t mc_portal_phys_addr;
>>>>>> -   u32 mc_portal_size;
>>>>>> -   struct resource res;
>>>>>> +   u32 mc_portal_size, mc_stream_id;
>>>>>> +   struct resource *plat_res;
>>>>>> +
>>>>>> +   if (!iommu_present(&fsl_mc_bus_type))
>>>>>> +           return -EPROBE_DEFER;
>>>>>>
>>>>>>     mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
>>>>>>     if (!mc)
>>>>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct
>>>>>> platform_device *pdev)
>>>>>>
>>>>>>     platform_set_drvdata(pdev, mc);
>>>>>>
>>>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>>>> +   mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res);
>>>>>> +   if (IS_ERR(mc->fsl_mc_regs))
>>>>>> +           return PTR_ERR(mc->fsl_mc_regs);
>>>>>> +
>>>>>> +   if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) {
>>>>>> +           mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR);
>>>>>> +           /*
>>>>>> +            * HW ORs the PL and BMT bit, places the result in bit 15 of
>>>>>> +            * the StreamID and ORs in the ICID. Calculate it accordingly.
>>>>>> +            */
>>>>>> +           mc_stream_id = (mc_stream_id & 0xffff) |
>>>>>> +                           ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ?
>>>>>> +                                   0x4000 : 0);
>>>>>> +           error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT,
>>>>>> +                                         &mc_stream_id);
>>>>>> +           if (error)
>>>>>> +                   dev_warn(&pdev->dev, "failed to configure dma: %d.\n",
>>>>>> +                            error);
>>>>>> +   }
>>>>>> +
>>>>>>     /*
>>>>>>      * Get physical address of MC portal for the root DPRC:
>>>>>>      */
>>>>>> -   error = of_address_to_resource(pdev->dev.of_node, 0, &res);
>>>>>> -   if (error < 0) {
>>>>>> -           dev_err(&pdev->dev,
>>>>>> -                   "of_address_to_resource() failed for %pOF\n",
>>>>>> -                   pdev->dev.of_node);
>>>>>> -           return error;
>>>>>> -   }
>>>>>> -
>>>>>> -   mc_portal_phys_addr = res.start;
>>>>>> -   mc_portal_size = resource_size(&res);
>>>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>> +   mc_portal_phys_addr = plat_res->start;
>>>>>> +   mc_portal_size = resource_size(plat_res);
>>>>>>     error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr,
>>>>>>                              mc_portal_size, NULL,
>>>>>>                              FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
>>>>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct
>>>> platform_device *pdev)
>>>>>>     dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n",
>>>>>>              mc_version.major, mc_version.minor,
>>>>>> mc_version.revision);
>>>>>>
>>>>>> -   error = get_mc_addr_translation_ranges(&pdev->dev,
>>>>>> -                                          &mc->translation_ranges,
>>>>>> -                                          &mc->num_translation_ranges);
>>>>>> -   if (error < 0)
>>>>>> -           goto error_cleanup_mc_io;
>>>>>> +   if (dev_of_node(&pdev->dev)) {
>>>>>> +           error = get_mc_addr_translation_ranges(&pdev->dev,
>>>>>> +                                           &mc->translation_ranges,
>>>>>> +                                           &mc->num_translation_ranges);
>>>>>> +           if (error < 0)
>>>>>> +                   goto error_cleanup_mc_io;
>>>>>> +   }
>>>>>>
>>>>>>     error = dprc_get_container_id(mc_io, 0, &container_id);
>>>>>>     if (error < 0) {
>>>>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct
>>>>>> platform_device
>>>> *pdev)
>>>>>>             goto error_cleanup_mc_io;
>>>>>>
>>>>>>     mc->root_mc_bus_dev = mc_bus_dev;
>>>>>> +   mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
>>>>>
>>>>> Makarand, this looks a bit weird. Is there really a reason for it?
>>>>
>>>> Can you clarify please so that we can reach a conclusion on this matter ?
>>>>
>>> Laurentiu, can you clarify what exactly is the doubt here? Are you asking about
>> fwnode assignment from pdev to mc_bus_dev?
>>>
>>
>> Yes. I remember that a while ago I tested without this fwnode assignment and
>> didn't encounter any issues. Maybe we can just drop it?
> 
> Did you tested with PHY changes? Because this is needed for MAC driver, where it needs the mc bus node.
> 

Good point, no i haven't. Then I'm ok with leaving it for the time being.

---
Best Regards, Laurentiu

Diana Madalina Craciun July 9, 2020, 10:47 a.m. UTC | #7

On 7/9/2020 1:37 PM, Makarand Pawagi wrote:
>
>> -----Original Message-----
>> From: Laurentiu Tudor <[e-mail]>
>> Sent: Thursday, July 9, 2020 3:45 PM
>> To: Makarand Pawagi <[e-mail]>; Lorenzo Pieralisi
>> <[e-mail]>
>> Cc: [e-mail]; Diana Madalina Craciun (OSS)
>> <[e-mail]>; [e-mail]; linux-
>> [e-mail]; [e-mail]; [e-mail];
>> Rob Herring <[e-mail]>; Rafael J. Wysocki <[e-mail]>;
>> Joerg Roedel <[e-mail]>; Hanjun Guo <[e-mail]>; Bjorn
>> Helgaas <[e-mail]>; Sudeep Holla <[e-mail]>;
>> Robin Murphy <[e-mail]>; Catalin Marinas
>> <[e-mail]>; Will Deacon <[e-mail]>; Marc Zyngier
>> <[e-mail]>
>> Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc
>>
>>
>>
>> On 7/9/2020 12:26 PM, Makarand Pawagi wrote:
>>>
>>>> -----Original Message-----
>>>> From: Lorenzo Pieralisi <[e-mail]>
>>>> Sent: Thursday, July 9, 2020 2:50 PM
>>>> To: Laurentiu Tudor <[e-mail]>
>>>> Cc: [e-mail]; Makarand Pawagi
>>>> <[e-mail]>; Diana Madalina Craciun (OSS)
>>>> <[e-mail]>; [e-mail]; linux-
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; Rob Herring <[e-mail]>; Rafael
>>>> J. Wysocki <[e-mail]>; Joerg Roedel <[e-mail]>;
>>>> Hanjun Guo <[e-mail]>; Bjorn Helgaas
>>>> <[e-mail]>; Sudeep Holla <[e-mail]>; Robin
>>>> Murphy <[e-mail]>; Catalin Marinas
>>>> <[e-mail]>; Will Deacon <[e-mail]>; Marc
>>>> Zyngier <[e-mail]>
>>>> Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for
>>>> fsl-mc
>>>>
>>>> Caution: EXT Email
>>>>
>>>> On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote:
>>>>>
>>>>> On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote:
>>>>>> From: Makarand Pawagi <[e-mail]>
>>>>>>
>>>>>> Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table
>>>>>> to extract memory and other resources.
>>>>>>
>>>>>> Interrupt (GIC ITS) information is extracted from the MADT table by
>>>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
>>>>>>
>>>>>> IORT table is parsed to configure DMA.
>>>>>>
>>>>>> Signed-off-by: Makarand Pawagi <[e-mail]>
>>>>>> Signed-off-by: Diana Craciun <[e-mail]>
>>>>>> Signed-off-by: Laurentiu Tudor <[e-mail]>
>>>>>> ---
>>>>>>   drivers/bus/fsl-mc/fsl-mc-bus.c             | 73 ++++++++++++----
>>>>>>   drivers/bus/fsl-mc/fsl-mc-msi.c             | 37 +++++----
>>>>>>   drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92
>>>>>> ++++++++++++++++-----
>>>>>>   3 files changed, 150 insertions(+), 52 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c
>>>>>> b/drivers/bus/fsl-mc/fsl-mc-bus.c index 824ff77bbe86..324d49d6df89
>>>>>> 100644
>>>>>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
>>>>>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
>>>>>> @@ -18,6 +18,8 @@
>>>>>>   #include <linux/bitops.h>
>>>>>>   #include <linux/msi.h>
>>>>>>   #include <linux/dma-mapping.h>
>>>>>> +#include <linux/acpi.h>
>>>>>> +#include <linux/iommu.h>
>>>>>>
>>>>>>   #include "fsl-mc-private.h"
>>>>>>
>>>>>> @@ -38,6 +40,7 @@ struct fsl_mc {
>>>>>>      struct fsl_mc_device *root_mc_bus_dev;
>>>>>>      u8 num_translation_ranges;
>>>>>>      struct fsl_mc_addr_translation_range *translation_ranges;
>>>>>> +   void *fsl_mc_regs;
>>>>>>   };
>>>>>>
>>>>>>   /**
>>>>>> @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range {
>>>>>>      phys_addr_t start_phys_addr;
>>>>>>   };
>>>>>>
>>>>>> +#define FSL_MC_FAPR        0x28
>>>>>> +#define MC_FAPR_PL BIT(18)
>>>>>> +#define MC_FAPR_BMT        BIT(17)
>>>>>> +
>>>>>>   /**
>>>>>>    * fsl_mc_bus_match - device to driver matching callback
>>>>>>    * @dev: the fsl-mc device to match against @@ -124,7 +131,10 @@
>>>>>> static int fsl_mc_dma_configure(struct device *dev)
>>>>>>      while (dev_is_fsl_mc(dma_dev))
>>>>>>              dma_dev = dma_dev->parent;
>>>>>>
>>>>>> -   return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
>>>>>> +   if (dev_of_node(dma_dev))
>>>>>> +           return of_dma_configure_id(dev, dma_dev->of_node, 0,
>>>>>> + &input_id);
>>>>>> +
>>>>>> +   return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
>>>>>>   }
>>>>>>
>>>>>>   static ssize_t modalias_show(struct device *dev, struct
>>>>>> device_attribute *attr, @@ -865,8 +875,11 @@ static int
>>>> fsl_mc_bus_probe(struct platform_device *pdev)
>>>>>>      struct fsl_mc_io *mc_io = NULL;
>>>>>>      int container_id;
>>>>>>      phys_addr_t mc_portal_phys_addr;
>>>>>> -   u32 mc_portal_size;
>>>>>> -   struct resource res;
>>>>>> +   u32 mc_portal_size, mc_stream_id;
>>>>>> +   struct resource *plat_res;
>>>>>> +
>>>>>> +   if (!iommu_present(&fsl_mc_bus_type))
>>>>>> +           return -EPROBE_DEFER;
>>>>>>
>>>>>>      mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
>>>>>>      if (!mc)
>>>>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct
>>>>>> platform_device *pdev)
>>>>>>
>>>>>>      platform_set_drvdata(pdev, mc);
>>>>>>
>>>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>>>> +   mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res);
>>>>>> +   if (IS_ERR(mc->fsl_mc_regs))
>>>>>> +           return PTR_ERR(mc->fsl_mc_regs);
>>>>>> +
>>>>>> +   if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) {
>>>>>> +           mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR);
>>>>>> +           /*
>>>>>> +            * HW ORs the PL and BMT bit, places the result in bit 15 of
>>>>>> +            * the StreamID and ORs in the ICID. Calculate it accordingly.
>>>>>> +            */
>>>>>> +           mc_stream_id = (mc_stream_id & 0xffff) |
>>>>>> +                           ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ?
>>>>>> +                                   0x4000 : 0);
>>>>>> +           error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT,
>>>>>> +                                         &mc_stream_id);
>>>>>> +           if (error)
>>>>>> +                   dev_warn(&pdev->dev, "failed to configure dma: %d.\n",
>>>>>> +                            error);
>>>>>> +   }
>>>>>> +
>>>>>>      /*
>>>>>>       * Get physical address of MC portal for the root DPRC:
>>>>>>       */
>>>>>> -   error = of_address_to_resource(pdev->dev.of_node, 0, &res);
>>>>>> -   if (error < 0) {
>>>>>> -           dev_err(&pdev->dev,
>>>>>> -                   "of_address_to_resource() failed for %pOF\n",
>>>>>> -                   pdev->dev.of_node);
>>>>>> -           return error;
>>>>>> -   }
>>>>>> -
>>>>>> -   mc_portal_phys_addr = res.start;
>>>>>> -   mc_portal_size = resource_size(&res);
>>>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>> +   mc_portal_phys_addr = plat_res->start;
>>>>>> +   mc_portal_size = resource_size(plat_res);
>>>>>>      error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr,
>>>>>>                               mc_portal_size, NULL,
>>>>>>                               FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
>>>>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct
>>>> platform_device *pdev)
>>>>>>      dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n",
>>>>>>               mc_version.major, mc_version.minor,
>>>>>> mc_version.revision);
>>>>>>
>>>>>> -   error = get_mc_addr_translation_ranges(&pdev->dev,
>>>>>> -                                          &mc->translation_ranges,
>>>>>> -                                          &mc->num_translation_ranges);
>>>>>> -   if (error < 0)
>>>>>> -           goto error_cleanup_mc_io;
>>>>>> +   if (dev_of_node(&pdev->dev)) {
>>>>>> +           error = get_mc_addr_translation_ranges(&pdev->dev,
>>>>>> +                                           &mc->translation_ranges,
>>>>>> +                                           &mc->num_translation_ranges);
>>>>>> +           if (error < 0)
>>>>>> +                   goto error_cleanup_mc_io;
>>>>>> +   }
>>>>>>
>>>>>>      error = dprc_get_container_id(mc_io, 0, &container_id);
>>>>>>      if (error < 0) {
>>>>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct
>>>>>> platform_device
>>>> *pdev)
>>>>>>              goto error_cleanup_mc_io;
>>>>>>
>>>>>>      mc->root_mc_bus_dev = mc_bus_dev;
>>>>>> +   mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
>>>>> Makarand, this looks a bit weird. Is there really a reason for it?
>>>> Can you clarify please so that we can reach a conclusion on this matter ?
>>>>
>>> Laurentiu, can you clarify what exactly is the doubt here? Are you asking about
>> fwnode assignment from pdev to mc_bus_dev?
>> Yes. I remember that a while ago I tested without this fwnode assignment and
>> didn't encounter any issues. Maybe we can just drop it?
> Did you tested with PHY changes? Because this is needed for MAC driver, where it needs the mc bus node.

Maybe it worth a comment or maybe have it in a different patch?

Thanks,
Diana

>
>> ---
>> Best Regards, Laurentiu

Makarand Pawagi July 9, 2020, 10:52 a.m. UTC | #8

> -----Original Message-----
> From: Diana Madalina Craciun (OSS) <[e-mail]>
> Sent: Thursday, July 9, 2020 4:17 PM
> To: Makarand Pawagi <[e-mail]>; Laurentiu Tudor
> <[e-mail]>; Lorenzo Pieralisi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; Rob Herring <[e-mail]>; Rafael J. Wysocki
> <[e-mail]>; Joerg Roedel <[e-mail]>; Hanjun Guo
> <[e-mail]>; Bjorn Helgaas <[e-mail]>; Sudeep
> Holla <[e-mail]>; Robin Murphy <[e-mail]>;
> Catalin Marinas <[e-mail]>; Will Deacon <[e-mail]>;
> Marc Zyngier <[e-mail]>
> Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc
> 
> On 7/9/2020 1:37 PM, Makarand Pawagi wrote:
> >
> >> -----Original Message-----
> >> From: Laurentiu Tudor <[e-mail]>
> >> Sent: Thursday, July 9, 2020 3:45 PM
> >> To: Makarand Pawagi <[e-mail]>; Lorenzo Pieralisi
> >> <[e-mail]>
> >> Cc: [e-mail]; Diana Madalina Craciun
> >> (OSS) <[e-mail]>; [e-mail];
> >> linux- [e-mail]; [e-mail];
> >> [e-mail]; Rob Herring <[e-mail]>; Rafael
> >> J. Wysocki <[e-mail]>; Joerg Roedel <[e-mail]>;
> >> Hanjun Guo <[e-mail]>; Bjorn Helgaas
> >> <[e-mail]>; Sudeep Holla <[e-mail]>; Robin
> >> Murphy <[e-mail]>; Catalin Marinas
> >> <[e-mail]>; Will Deacon <[e-mail]>; Marc
> >> Zyngier <[e-mail]>
> >> Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support
> >> for fsl-mc
> >>
> >>
> >>
> >> On 7/9/2020 12:26 PM, Makarand Pawagi wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: Lorenzo Pieralisi <[e-mail]>
> >>>> Sent: Thursday, July 9, 2020 2:50 PM
> >>>> To: Laurentiu Tudor <[e-mail]>
> >>>> Cc: [e-mail]; Makarand Pawagi
> >>>> <[e-mail]>; Diana Madalina Craciun (OSS)
> >>>> <[e-mail]>; [e-mail];
> >>>> linux- [e-mail]; [e-mail];
> >>>> [e-mail]; Rob Herring <[e-mail]>; Rafael
> >>>> J. Wysocki <[e-mail]>; Joerg Roedel <[e-mail]>;
> >>>> Hanjun Guo <[e-mail]>; Bjorn Helgaas
> >>>> <[e-mail]>; Sudeep Holla <[e-mail]>; Robin
> >>>> Murphy <[e-mail]>; Catalin Marinas
> >>>> <[e-mail]>; Will Deacon <[e-mail]>; Marc
> >>>> Zyngier <[e-mail]>
> >>>> Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support
> >>>> for fsl-mc
> >>>>
> >>>> Caution: EXT Email
> >>>>
> >>>> On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote:
> >>>>>
> >>>>> On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote:
> >>>>>> From: Makarand Pawagi <[e-mail]>
> >>>>>>
> >>>>>> Add ACPI support in the fsl-mc driver. Driver parses MC DSDT
> >>>>>> table to extract memory and other resources.
> >>>>>>
> >>>>>> Interrupt (GIC ITS) information is extracted from the MADT table
> >>>>>> by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> >>>>>>
> >>>>>> IORT table is parsed to configure DMA.
> >>>>>>
> >>>>>> Signed-off-by: Makarand Pawagi <[e-mail]>
> >>>>>> Signed-off-by: Diana Craciun <[e-mail]>
> >>>>>> Signed-off-by: Laurentiu Tudor <[e-mail]>
> >>>>>> ---
> >>>>>>   drivers/bus/fsl-mc/fsl-mc-bus.c             | 73 ++++++++++++----
> >>>>>>   drivers/bus/fsl-mc/fsl-mc-msi.c             | 37 +++++----
> >>>>>>   drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92
> >>>>>> ++++++++++++++++-----
> >>>>>>   3 files changed, 150 insertions(+), 52 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c
> >>>>>> b/drivers/bus/fsl-mc/fsl-mc-bus.c index
> >>>>>> 824ff77bbe86..324d49d6df89
> >>>>>> 100644
> >>>>>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> >>>>>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> >>>>>> @@ -18,6 +18,8 @@
> >>>>>>   #include <linux/bitops.h>
> >>>>>>   #include <linux/msi.h>
> >>>>>>   #include <linux/dma-mapping.h>
> >>>>>> +#include <linux/acpi.h>
> >>>>>> +#include <linux/iommu.h>
> >>>>>>
> >>>>>>   #include "fsl-mc-private.h"
> >>>>>>
> >>>>>> @@ -38,6 +40,7 @@ struct fsl_mc {
> >>>>>>      struct fsl_mc_device *root_mc_bus_dev;
> >>>>>>      u8 num_translation_ranges;
> >>>>>>      struct fsl_mc_addr_translation_range *translation_ranges;
> >>>>>> +   void *fsl_mc_regs;
> >>>>>>   };
> >>>>>>
> >>>>>>   /**
> >>>>>> @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range {
> >>>>>>      phys_addr_t start_phys_addr;
> >>>>>>   };
> >>>>>>
> >>>>>> +#define FSL_MC_FAPR        0x28
> >>>>>> +#define MC_FAPR_PL BIT(18)
> >>>>>> +#define MC_FAPR_BMT        BIT(17)
> >>>>>> +
> >>>>>>   /**
> >>>>>>    * fsl_mc_bus_match - device to driver matching callback
> >>>>>>    * @dev: the fsl-mc device to match against @@ -124,7 +131,10
> >>>>>> @@ static int fsl_mc_dma_configure(struct device *dev)
> >>>>>>      while (dev_is_fsl_mc(dma_dev))
> >>>>>>              dma_dev = dma_dev->parent;
> >>>>>>
> >>>>>> -   return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
> >>>>>> +   if (dev_of_node(dma_dev))
> >>>>>> +           return of_dma_configure_id(dev, dma_dev->of_node, 0,
> >>>>>> + &input_id);
> >>>>>> +
> >>>>>> +   return acpi_dma_configure_id(dev, DEV_DMA_COHERENT,
> >>>>>> + &input_id);
> >>>>>>   }
> >>>>>>
> >>>>>>   static ssize_t modalias_show(struct device *dev, struct
> >>>>>> device_attribute *attr, @@ -865,8 +875,11 @@ static int
> >>>> fsl_mc_bus_probe(struct platform_device *pdev)
> >>>>>>      struct fsl_mc_io *mc_io = NULL;
> >>>>>>      int container_id;
> >>>>>>      phys_addr_t mc_portal_phys_addr;
> >>>>>> -   u32 mc_portal_size;
> >>>>>> -   struct resource res;
> >>>>>> +   u32 mc_portal_size, mc_stream_id;
> >>>>>> +   struct resource *plat_res;
> >>>>>> +
> >>>>>> +   if (!iommu_present(&fsl_mc_bus_type))
> >>>>>> +           return -EPROBE_DEFER;
> >>>>>>
> >>>>>>      mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
> >>>>>>      if (!mc)
> >>>>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct
> >>>>>> platform_device *pdev)
> >>>>>>
> >>>>>>      platform_set_drvdata(pdev, mc);
> >>>>>>
> >>>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >>>>>> +   mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res);
> >>>>>> +   if (IS_ERR(mc->fsl_mc_regs))
> >>>>>> +           return PTR_ERR(mc->fsl_mc_regs);
> >>>>>> +
> >>>>>> +   if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) {
> >>>>>> +           mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR);
> >>>>>> +           /*
> >>>>>> +            * HW ORs the PL and BMT bit, places the result in bit 15 of
> >>>>>> +            * the StreamID and ORs in the ICID. Calculate it accordingly.
> >>>>>> +            */
> >>>>>> +           mc_stream_id = (mc_stream_id & 0xffff) |
> >>>>>> +                           ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ?
> >>>>>> +                                   0x4000 : 0);
> >>>>>> +           error = acpi_dma_configure_id(&pdev->dev,
> DEV_DMA_COHERENT,
> >>>>>> +                                         &mc_stream_id);
> >>>>>> +           if (error)
> >>>>>> +                   dev_warn(&pdev->dev, "failed to configure dma: %d.\n",
> >>>>>> +                            error);
> >>>>>> +   }
> >>>>>> +
> >>>>>>      /*
> >>>>>>       * Get physical address of MC portal for the root DPRC:
> >>>>>>       */
> >>>>>> -   error = of_address_to_resource(pdev->dev.of_node, 0, &res);
> >>>>>> -   if (error < 0) {
> >>>>>> -           dev_err(&pdev->dev,
> >>>>>> -                   "of_address_to_resource() failed for %pOF\n",
> >>>>>> -                   pdev->dev.of_node);
> >>>>>> -           return error;
> >>>>>> -   }
> >>>>>> -
> >>>>>> -   mc_portal_phys_addr = res.start;
> >>>>>> -   mc_portal_size = resource_size(&res);
> >>>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>>>>> +   mc_portal_phys_addr = plat_res->start;
> >>>>>> +   mc_portal_size = resource_size(plat_res);
> >>>>>>      error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr,
> >>>>>>                               mc_portal_size, NULL,
> >>>>>>                               FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
> >>>>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct
> >>>> platform_device *pdev)
> >>>>>>      dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n",
> >>>>>>               mc_version.major, mc_version.minor,
> >>>>>> mc_version.revision);
> >>>>>>
> >>>>>> -   error = get_mc_addr_translation_ranges(&pdev->dev,
> >>>>>> -                                          &mc->translation_ranges,
> >>>>>> -                                          &mc->num_translation_ranges);
> >>>>>> -   if (error < 0)
> >>>>>> -           goto error_cleanup_mc_io;
> >>>>>> +   if (dev_of_node(&pdev->dev)) {
> >>>>>> +           error = get_mc_addr_translation_ranges(&pdev->dev,
> >>>>>> +                                           &mc->translation_ranges,
> >>>>>> +                                           &mc->num_translation_ranges);
> >>>>>> +           if (error < 0)
> >>>>>> +                   goto error_cleanup_mc_io;
> >>>>>> +   }
> >>>>>>
> >>>>>>      error = dprc_get_container_id(mc_io, 0, &container_id);
> >>>>>>      if (error < 0) {
> >>>>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct
> >>>>>> platform_device
> >>>> *pdev)
> >>>>>>              goto error_cleanup_mc_io;
> >>>>>>
> >>>>>>      mc->root_mc_bus_dev = mc_bus_dev;
> >>>>>> +   mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
> >>>>> Makarand, this looks a bit weird. Is there really a reason for it?
> >>>> Can you clarify please so that we can reach a conclusion on this matter ?
> >>>>
> >>> Laurentiu, can you clarify what exactly is the doubt here? Are you
> >>> asking about
> >> fwnode assignment from pdev to mc_bus_dev?
> >> Yes. I remember that a while ago I tested without this fwnode
> >> assignment and didn't encounter any issues. Maybe we can just drop it?
> > Did you tested with PHY changes? Because this is needed for MAC driver,
> where it needs the mc bus node.
> 
> Maybe it worth a comment or maybe have it in a different patch?
> 
Since this change is needed for ACPI case and this is ACPI support case,
I feel we should have this change in this patch only instead of separate patch. 

> Thanks,
> Diana
> 
> >
> >> ---
> >> Best Regards, Laurentiu

Lorenzo Pieralisi July 15, 2020, 10:06 a.m. UTC | #9

On Thu, Jul 09, 2020 at 10:52:52AM +0000, Makarand Pawagi wrote:

[...]

> > >>>> fsl_mc_bus_probe(struct platform_device *pdev)
> > >>>>>>      struct fsl_mc_io *mc_io = NULL;
> > >>>>>>      int container_id;
> > >>>>>>      phys_addr_t mc_portal_phys_addr;
> > >>>>>> -   u32 mc_portal_size;
> > >>>>>> -   struct resource res;
> > >>>>>> +   u32 mc_portal_size, mc_stream_id;
> > >>>>>> +   struct resource *plat_res;
> > >>>>>> +
> > >>>>>> +   if (!iommu_present(&fsl_mc_bus_type))
> > >>>>>> +           return -EPROBE_DEFER;
> > >>>>>>
> > >>>>>>      mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
> > >>>>>>      if (!mc)
> > >>>>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct
> > >>>>>> platform_device *pdev)
> > >>>>>>
> > >>>>>>      platform_set_drvdata(pdev, mc);
> > >>>>>>
> > >>>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > >>>>>> +   mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res);
> > >>>>>> +   if (IS_ERR(mc->fsl_mc_regs))
> > >>>>>> +           return PTR_ERR(mc->fsl_mc_regs);
> > >>>>>> +
> > >>>>>> +   if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) {
> > >>>>>> +           mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR);
> > >>>>>> +           /*
> > >>>>>> +            * HW ORs the PL and BMT bit, places the result in bit 15 of
> > >>>>>> +            * the StreamID and ORs in the ICID. Calculate it accordingly.
> > >>>>>> +            */
> > >>>>>> +           mc_stream_id = (mc_stream_id & 0xffff) |
> > >>>>>> +                           ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ?
> > >>>>>> +                                   0x4000 : 0);
> > >>>>>> +           error = acpi_dma_configure_id(&pdev->dev,
> > DEV_DMA_COHERENT,
> > >>>>>> +                                         &mc_stream_id);
> > >>>>>> +           if (error)
> > >>>>>> +                   dev_warn(&pdev->dev, "failed to configure dma: %d.\n",
> > >>>>>> +                            error);
> > >>>>>> +   }
> > >>>>>> +
> > >>>>>>      /*
> > >>>>>>       * Get physical address of MC portal for the root DPRC:
> > >>>>>>       */
> > >>>>>> -   error = of_address_to_resource(pdev->dev.of_node, 0, &res);
> > >>>>>> -   if (error < 0) {
> > >>>>>> -           dev_err(&pdev->dev,
> > >>>>>> -                   "of_address_to_resource() failed for %pOF\n",
> > >>>>>> -                   pdev->dev.of_node);
> > >>>>>> -           return error;
> > >>>>>> -   }
> > >>>>>> -
> > >>>>>> -   mc_portal_phys_addr = res.start;
> > >>>>>> -   mc_portal_size = resource_size(&res);
> > >>>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >>>>>> +   mc_portal_phys_addr = plat_res->start;
> > >>>>>> +   mc_portal_size = resource_size(plat_res);
> > >>>>>>      error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr,
> > >>>>>>                               mc_portal_size, NULL,
> > >>>>>>                               FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
> > >>>>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct
> > >>>> platform_device *pdev)
> > >>>>>>      dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n",
> > >>>>>>               mc_version.major, mc_version.minor,
> > >>>>>> mc_version.revision);
> > >>>>>>
> > >>>>>> -   error = get_mc_addr_translation_ranges(&pdev->dev,
> > >>>>>> -                                          &mc->translation_ranges,
> > >>>>>> -                                          &mc->num_translation_ranges);
> > >>>>>> -   if (error < 0)
> > >>>>>> -           goto error_cleanup_mc_io;
> > >>>>>> +   if (dev_of_node(&pdev->dev)) {
> > >>>>>> +           error = get_mc_addr_translation_ranges(&pdev->dev,
> > >>>>>> +                                           &mc->translation_ranges,
> > >>>>>> +                                           &mc->num_translation_ranges);
> > >>>>>> +           if (error < 0)
> > >>>>>> +                   goto error_cleanup_mc_io;
> > >>>>>> +   }
> > >>>>>>
> > >>>>>>      error = dprc_get_container_id(mc_io, 0, &container_id);
> > >>>>>>      if (error < 0) {
> > >>>>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct
> > >>>>>> platform_device
> > >>>> *pdev)
> > >>>>>>              goto error_cleanup_mc_io;
> > >>>>>>
> > >>>>>>      mc->root_mc_bus_dev = mc_bus_dev;
> > >>>>>> +   mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
> > >>>>> Makarand, this looks a bit weird. Is there really a reason for it?
> > >>>> Can you clarify please so that we can reach a conclusion on this matter ?
> > >>>>
> > >>> Laurentiu, can you clarify what exactly is the doubt here? Are you
> > >>> asking about
> > >> fwnode assignment from pdev to mc_bus_dev?
> > >> Yes. I remember that a while ago I tested without this fwnode
> > >> assignment and didn't encounter any issues. Maybe we can just drop it?
> > > Did you tested with PHY changes? Because this is needed for MAC driver,
> > where it needs the mc bus node.
> > 
> > Maybe it worth a comment or maybe have it in a different patch?
> > 
> Since this change is needed for ACPI case and this is ACPI support
> case, I feel we should have this change in this patch only instead of
> separate patch. 

Anyway - you need to seek feedback from Marc on whether patches
11 and 12 are OK from an irqchip perspective, it is possible we
can take the rest of the series independently if everyone agrees
but I don't necessarily see a reason for that.

Long story short: you need Marc's ACK on [11-12], it is your code.

Thanks,
Lorenzo

Makarand Pawagi July 16, 2020, 3:23 a.m. UTC | #10

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: Wednesday, July 15, 2020 3:37 PM
> To: Makarand Pawagi <[e-mail]>
> Cc: Diana Madalina Craciun (OSS) <[e-mail]>; Laurentiu
> Tudor <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Rob Herring
> <[e-mail]>; Rafael J. Wysocki <[e-mail]>; Joerg Roedel
> <[e-mail]>; Hanjun Guo <[e-mail]>; Bjorn Helgaas
> <[e-mail]>; Sudeep Holla <[e-mail]>; Robin
> Murphy <[e-mail]>; Catalin Marinas
> <[e-mail]>; Will Deacon <[e-mail]>; Marc Zyngier
> <[e-mail]>
> Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc
> 
> Caution: EXT Email
> 
> On Thu, Jul 09, 2020 at 10:52:52AM +0000, Makarand Pawagi wrote:
> 
> [...]
> 
> > > >>>> fsl_mc_bus_probe(struct platform_device *pdev)
> > > >>>>>>      struct fsl_mc_io *mc_io = NULL;
> > > >>>>>>      int container_id;
> > > >>>>>>      phys_addr_t mc_portal_phys_addr;
> > > >>>>>> -   u32 mc_portal_size;
> > > >>>>>> -   struct resource res;
> > > >>>>>> +   u32 mc_portal_size, mc_stream_id;
> > > >>>>>> +   struct resource *plat_res;
> > > >>>>>> +
> > > >>>>>> +   if (!iommu_present(&fsl_mc_bus_type))
> > > >>>>>> +           return -EPROBE_DEFER;
> > > >>>>>>
> > > >>>>>>      mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
> > > >>>>>>      if (!mc)
> > > >>>>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct
> > > >>>>>> platform_device *pdev)
> > > >>>>>>
> > > >>>>>>      platform_set_drvdata(pdev, mc);
> > > >>>>>>
> > > >>>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > >>>>>> +   mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev,
> plat_res);
> > > >>>>>> +   if (IS_ERR(mc->fsl_mc_regs))
> > > >>>>>> +           return PTR_ERR(mc->fsl_mc_regs);
> > > >>>>>> +
> > > >>>>>> +   if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) {
> > > >>>>>> +           mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR);
> > > >>>>>> +           /*
> > > >>>>>> +            * HW ORs the PL and BMT bit, places the result in bit 15 of
> > > >>>>>> +            * the StreamID and ORs in the ICID. Calculate it accordingly.
> > > >>>>>> +            */
> > > >>>>>> +           mc_stream_id = (mc_stream_id & 0xffff) |
> > > >>>>>> +                           ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ?
> > > >>>>>> +                                   0x4000 : 0);
> > > >>>>>> +           error = acpi_dma_configure_id(&pdev->dev,
> > > DEV_DMA_COHERENT,
> > > >>>>>> +                                         &mc_stream_id);
> > > >>>>>> +           if (error)
> > > >>>>>> +                   dev_warn(&pdev->dev, "failed to configure dma: %d.\n",
> > > >>>>>> +                            error);
> > > >>>>>> +   }
> > > >>>>>> +
> > > >>>>>>      /*
> > > >>>>>>       * Get physical address of MC portal for the root DPRC:
> > > >>>>>>       */
> > > >>>>>> -   error = of_address_to_resource(pdev->dev.of_node, 0, &res);
> > > >>>>>> -   if (error < 0) {
> > > >>>>>> -           dev_err(&pdev->dev,
> > > >>>>>> -                   "of_address_to_resource() failed for %pOF\n",
> > > >>>>>> -                   pdev->dev.of_node);
> > > >>>>>> -           return error;
> > > >>>>>> -   }
> > > >>>>>> -
> > > >>>>>> -   mc_portal_phys_addr = res.start;
> > > >>>>>> -   mc_portal_size = resource_size(&res);
> > > >>>>>> +   plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > >>>>>> +   mc_portal_phys_addr = plat_res->start;
> > > >>>>>> +   mc_portal_size = resource_size(plat_res);
> > > >>>>>>      error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr,
> > > >>>>>>                               mc_portal_size, NULL,
> > > >>>>>>
> > > >>>>>> FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, &mc_io); @@ -903,11
> +930,13
> > > >>>>>> @@ static int fsl_mc_bus_probe(struct
> > > >>>> platform_device *pdev)
> > > >>>>>>      dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n",
> > > >>>>>>               mc_version.major, mc_version.minor,
> > > >>>>>> mc_version.revision);
> > > >>>>>>
> > > >>>>>> -   error = get_mc_addr_translation_ranges(&pdev->dev,
> > > >>>>>> -                                          &mc->translation_ranges,
> > > >>>>>> -                                          &mc->num_translation_ranges);
> > > >>>>>> -   if (error < 0)
> > > >>>>>> -           goto error_cleanup_mc_io;
> > > >>>>>> +   if (dev_of_node(&pdev->dev)) {
> > > >>>>>> +           error = get_mc_addr_translation_ranges(&pdev->dev,
> > > >>>>>> +                                           &mc->translation_ranges,
> > > >>>>>> +                                           &mc->num_translation_ranges);
> > > >>>>>> +           if (error < 0)
> > > >>>>>> +                   goto error_cleanup_mc_io;
> > > >>>>>> +   }
> > > >>>>>>
> > > >>>>>>      error = dprc_get_container_id(mc_io, 0, &container_id);
> > > >>>>>>      if (error < 0) {
> > > >>>>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct
> > > >>>>>> platform_device
> > > >>>> *pdev)
> > > >>>>>>              goto error_cleanup_mc_io;
> > > >>>>>>
> > > >>>>>>      mc->root_mc_bus_dev = mc_bus_dev;
> > > >>>>>> +   mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
> > > >>>>> Makarand, this looks a bit weird. Is there really a reason for it?
> > > >>>> Can you clarify please so that we can reach a conclusion on this matter ?
> > > >>>>
> > > >>> Laurentiu, can you clarify what exactly is the doubt here? Are
> > > >>> you asking about
> > > >> fwnode assignment from pdev to mc_bus_dev?
> > > >> Yes. I remember that a while ago I tested without this fwnode
> > > >> assignment and didn't encounter any issues. Maybe we can just drop it?
> > > > Did you tested with PHY changes? Because this is needed for MAC
> > > > driver,
> > > where it needs the mc bus node.
> > >
> > > Maybe it worth a comment or maybe have it in a different patch?
> > >
> > Since this change is needed for ACPI case and this is ACPI support
> > case, I feel we should have this change in this patch only instead of
> > separate patch.
> 
> Anyway - you need to seek feedback from Marc on whether patches
> 11 and 12 are OK from an irqchip perspective, it is possible we can take the rest
> of the series independently if everyone agrees but I don't necessarily see a
> reason for that.
> 
> Long story short: you need Marc's ACK on [11-12], it is your code.
> 
Hi Marc, can you please review/ack this patch?

> Thanks,
> Lorenzo

Marc Zyngier July 16, 2020, 7:57 a.m. UTC | #11

On 2020-07-16 04:23, Makarand Pawagi wrote:
>> -----Original Message-----
>> From: Lorenzo Pieralisi <[e-mail]>

[...]

>> Anyway - you need to seek feedback from Marc on whether patches
>> 11 and 12 are OK from an irqchip perspective, it is possible we can 
>> take the rest
>> of the series independently if everyone agrees but I don't necessarily 
>> see a
>> reason for that.
>> 
>> Long story short: you need Marc's ACK on [11-12], it is your code.
>> 
> Hi Marc, can you please review/ack this patch?

https://lore.kernel.org/linux-acpi/[e-mail]/

         M.

Patch

diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 824ff77bbe86..324d49d6df89 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -18,6 +18,8 @@ 
 #include <linux/bitops.h>
 #include <linux/msi.h>
 #include <linux/dma-mapping.h>
+#include <linux/acpi.h>
+#include <linux/iommu.h>
 
 #include "fsl-mc-private.h"
 
@@ -38,6 +40,7 @@  struct fsl_mc {
 	struct fsl_mc_device *root_mc_bus_dev;
 	u8 num_translation_ranges;
 	struct fsl_mc_addr_translation_range *translation_ranges;
+	void *fsl_mc_regs;
 };
 
 /**
@@ -56,6 +59,10 @@  struct fsl_mc_addr_translation_range {
 	phys_addr_t start_phys_addr;
 };
 
+#define FSL_MC_FAPR	0x28
+#define MC_FAPR_PL	BIT(18)
+#define MC_FAPR_BMT	BIT(17)
+
 /**
  * fsl_mc_bus_match - device to driver matching callback
  * @dev: the fsl-mc device to match against
@@ -124,7 +131,10 @@  static int fsl_mc_dma_configure(struct device *dev)
 	while (dev_is_fsl_mc(dma_dev))
 		dma_dev = dma_dev->parent;
 
-	return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
+	if (dev_of_node(dma_dev))
+		return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
+
+	return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
 }
 
 static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
@@ -865,8 +875,11 @@  static int fsl_mc_bus_probe(struct platform_device *pdev)
 	struct fsl_mc_io *mc_io = NULL;
 	int container_id;
 	phys_addr_t mc_portal_phys_addr;
-	u32 mc_portal_size;
-	struct resource res;
+	u32 mc_portal_size, mc_stream_id;
+	struct resource *plat_res;
+
+	if (!iommu_present(&fsl_mc_bus_type))
+		return -EPROBE_DEFER;
 
 	mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
 	if (!mc)
@@ -874,19 +887,33 @@  static int fsl_mc_bus_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mc);
 
+	plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res);
+	if (IS_ERR(mc->fsl_mc_regs))
+		return PTR_ERR(mc->fsl_mc_regs);
+
+	if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) {
+		mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR);
+		/*
+		 * HW ORs the PL and BMT bit, places the result in bit 15 of
+		 * the StreamID and ORs in the ICID. Calculate it accordingly.
+		 */
+		mc_stream_id = (mc_stream_id & 0xffff) |
+				((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ?
+					0x4000 : 0);
+		error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT,
+					      &mc_stream_id);
+		if (error)
+			dev_warn(&pdev->dev, "failed to configure dma: %d.\n",
+				 error);
+	}
+
 	/*
 	 * Get physical address of MC portal for the root DPRC:
 	 */
-	error = of_address_to_resource(pdev->dev.of_node, 0, &res);
-	if (error < 0) {
-		dev_err(&pdev->dev,
-			"of_address_to_resource() failed for %pOF\n",
-			pdev->dev.of_node);
-		return error;
-	}
-
-	mc_portal_phys_addr = res.start;
-	mc_portal_size = resource_size(&res);
+	plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mc_portal_phys_addr = plat_res->start;
+	mc_portal_size = resource_size(plat_res);
 	error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr,
 				 mc_portal_size, NULL,
 				 FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, &mc_io);
@@ -903,11 +930,13 @@  static int fsl_mc_bus_probe(struct platform_device *pdev)
 	dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n",
 		 mc_version.major, mc_version.minor, mc_version.revision);
 
-	error = get_mc_addr_translation_ranges(&pdev->dev,
-					       &mc->translation_ranges,
-					       &mc->num_translation_ranges);
-	if (error < 0)
-		goto error_cleanup_mc_io;
+	if (dev_of_node(&pdev->dev)) {
+		error = get_mc_addr_translation_ranges(&pdev->dev,
+						&mc->translation_ranges,
+						&mc->num_translation_ranges);
+		if (error < 0)
+			goto error_cleanup_mc_io;
+	}
 
 	error = dprc_get_container_id(mc_io, 0, &container_id);
 	if (error < 0) {
@@ -934,6 +963,7 @@  static int fsl_mc_bus_probe(struct platform_device *pdev)
 		goto error_cleanup_mc_io;
 
 	mc->root_mc_bus_dev = mc_bus_dev;
+	mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
 	return 0;
 
 error_cleanup_mc_io:
@@ -967,11 +997,18 @@  static const struct of_device_id fsl_mc_bus_match_table[] = {
 
 MODULE_DEVICE_TABLE(of, fsl_mc_bus_match_table);
 
+static const struct acpi_device_id fsl_mc_bus_acpi_match_table[] = {
+	{"NXP0008", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, fsl_mc_bus_acpi_match_table);
+
 static struct platform_driver fsl_mc_bus_driver = {
 	.driver = {
 		   .name = "fsl_mc_bus",
 		   .pm = NULL,
 		   .of_match_table = fsl_mc_bus_match_table,
+		   .acpi_match_table = fsl_mc_bus_acpi_match_table,
 		   },
 	.probe = fsl_mc_bus_probe,
 	.remove = fsl_mc_bus_remove,
diff --git a/drivers/bus/fsl-mc/fsl-mc-msi.c b/drivers/bus/fsl-mc/fsl-mc-msi.c
index e7bbff445a83..8edadf05cbb7 100644
--- a/drivers/bus/fsl-mc/fsl-mc-msi.c
+++ b/drivers/bus/fsl-mc/fsl-mc-msi.c
@@ -13,6 +13,7 @@ 
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/msi.h>
+#include <linux/acpi_iort.h>
 
 #include "fsl-mc-private.h"
 
@@ -179,25 +180,31 @@  struct irq_domain *fsl_mc_msi_create_irq_domain(struct fwnode_handle *fwnode,
 
 struct irq_domain *fsl_mc_find_msi_domain(struct device *dev)
 {
-	struct irq_domain *msi_domain = NULL;
+	struct device *root_dprc_dev;
+	struct device *bus_dev;
+	struct irq_domain *msi_domain;
 	struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
 
-	msi_domain = of_msi_map_get_device_domain(dev, mc_dev->icid,
+	fsl_mc_get_root_dprc(dev, &root_dprc_dev);
+	bus_dev = root_dprc_dev->parent;
+
+	if (bus_dev->of_node) {
+		msi_domain = of_msi_map_get_device_domain(dev,
+						  mc_dev->icid,
 						  DOMAIN_BUS_FSL_MC_MSI);
 
-	/*
-	 * if the msi-map property is missing assume that all the
-	 * child containers inherit the domain from the parent
-	 */
-	if (!msi_domain) {
-		struct device *root_dprc_dev;
-		struct device *bus_dev;
-
-		fsl_mc_get_root_dprc(dev, &root_dprc_dev);
-		bus_dev = root_dprc_dev->parent;
-		msi_domain = of_msi_get_domain(bus_dev,
-					       bus_dev->of_node,
-					       DOMAIN_BUS_FSL_MC_MSI);
+		/*
+		 * if the msi-map property is missing assume that all the
+		 * child containers inherit the domain from the parent
+		 */
+		if (!msi_domain)
+
+			msi_domain = of_msi_get_domain(bus_dev,
+						bus_dev->of_node,
+						DOMAIN_BUS_FSL_MC_MSI);
+	} else {
+		msi_domain = iort_get_device_domain(dev, mc_dev->icid,
+						    DOMAIN_BUS_FSL_MC_MSI);
 	}
 
 	return msi_domain;
diff --git a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
index a5c8d577e424..634263dfd7b5 100644
--- a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
@@ -7,6 +7,8 @@ 
  *
  */
 
+#include <linux/acpi.h>
+#include <linux/acpi_iort.h>
 #include <linux/of_device.h>
 #include <linux/of_address.h>
 #include <linux/irq.h>
@@ -30,7 +32,8 @@  static u32 fsl_mc_msi_domain_get_msi_id(struct irq_domain *domain,
 	u32 out_id;
 
 	of_node = irq_domain_get_of_node(domain);
-	out_id = of_msi_map_id(&mc_dev->dev, of_node, mc_dev->icid);
+	out_id = of_node ? of_msi_map_id(&mc_dev->dev, of_node, mc_dev->icid) :
+			iort_msi_map_id(&mc_dev->dev, mc_dev->icid);
 
 	return out_id;
 }
@@ -79,12 +82,71 @@  static const struct of_device_id its_device_id[] = {
 	{},
 };
 
-static int __init its_fsl_mc_msi_init(void)
+static void __init its_fsl_mc_msi_init_one(struct fwnode_handle *handle,
+					  const char *name)
 {
-	struct device_node *np;
 	struct irq_domain *parent;
 	struct irq_domain *mc_msi_domain;
 
+	parent = irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS);
+	if (!parent || !msi_get_domain_info(parent)) {
+		pr_err("%s: unable to locate ITS domain\n", name);
+		return;
+	}
+
+	mc_msi_domain = fsl_mc_msi_create_irq_domain(handle,
+						&its_fsl_mc_msi_domain_info,
+						parent);
+	if (!mc_msi_domain) {
+		pr_err("%s: unable to create fsl-mc domain\n", name);
+		return;
+	}
+
+	pr_info("fsl-mc MSI: %s domain created\n", name);
+}
+
+#ifdef CONFIG_ACPI
+static int __init
+its_fsl_mc_msi_parse_madt(union acpi_subtable_headers *header,
+			  const unsigned long end)
+{
+	struct acpi_madt_generic_translator *its_entry;
+	struct fwnode_handle *dom_handle;
+	const char *node_name;
+	int err = 0;
+
+	its_entry = (struct acpi_madt_generic_translator *)header;
+	node_name = kasprintf(GFP_KERNEL, "ITS@0x%lx",
+			      (long)its_entry->base_address);
+
+	dom_handle = iort_find_domain_token(its_entry->translation_id);
+	if (!dom_handle) {
+		pr_err("%s: Unable to locate ITS domain handle\n", node_name);
+		err = -ENXIO;
+		goto out;
+	}
+
+	its_fsl_mc_msi_init_one(dom_handle, node_name);
+
+out:
+	kfree(node_name);
+	return err;
+}
+
+
+static void __init its_fsl_mc_acpi_msi_init(void)
+{
+	acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
+			      its_fsl_mc_msi_parse_madt, 0);
+}
+#else
+static inline void its_fsl_mc_acpi_msi_init(void) { }
+#endif
+
+static void __init its_fsl_mc_of_msi_init(void)
+{
+	struct device_node *np;
+
 	for (np = of_find_matching_node(NULL, its_device_id); np;
 	     np = of_find_matching_node(np, its_device_id)) {
 		if (!of_device_is_available(np))
@@ -92,23 +154,15 @@  static int __init its_fsl_mc_msi_init(void)
 		if (!of_property_read_bool(np, "msi-controller"))
 			continue;
 
-		parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS);
-		if (!parent || !msi_get_domain_info(parent)) {
-			pr_err("%pOF: unable to locate ITS domain\n", np);
-			continue;
-		}
-
-		mc_msi_domain = fsl_mc_msi_create_irq_domain(
-						 of_node_to_fwnode(np),
-						 &its_fsl_mc_msi_domain_info,
-						 parent);
-		if (!mc_msi_domain) {
-			pr_err("%pOF: unable to create fsl-mc domain\n", np);
-			continue;
-		}
-
-		pr_info("fsl-mc MSI: %pOF domain created\n", np);
+		its_fsl_mc_msi_init_one(of_node_to_fwnode(np),
+					np->full_name);
 	}
+}
+
+static int __init its_fsl_mc_msi_init(void)
+{
+	its_fsl_mc_of_msi_init();
+	its_fsl_mc_acpi_msi_init();
 
 	return 0;
 }