bus: fsl-mc: Add ACPI support for fsl-mc

Commit Message

Makarand Pawagi Jan. 28, 2020, 8:08 a.m. UTC

ACPI support is added in the fsl-mc driver. Driver will parse
MC DSDT table to extract memory and other resorces.

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

IORT table will be parsed to configure DMA.

Signed-off-by: Makarand Pawagi <[e-mail]>
---
 drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
 drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
 drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
 drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
 drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
 drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71 ++++++++++++++++++++++++++++-
 include/linux/acpi_iort.h                   |  5 ++
 7 files changed, 174 insertions(+), 20 deletions(-)

Comments

Marc Zyngier Jan. 28, 2020, 10:58 a.m. UTC | #1

On 2020-01-28 08:08, Makarand Pawagi wrote:
> ACPI support is added in the fsl-mc driver. Driver will parse
> MC DSDT table to extract memory and other resorces.
> 
> Interrupt (GIC ITS) information will be extracted from MADT table
> by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> 
> IORT table will be parsed to configure DMA.
> 
> Signed-off-by: Makarand Pawagi <[e-mail]>
> ---
>  drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
>  drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
>  drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
>  drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
>  drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
>  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71 
> ++++++++++++++++++++++++++++-
>  include/linux/acpi_iort.h                   |  5 ++
>  7 files changed, 174 insertions(+), 20 deletions(-)

A general comment when you do this kind of work:

Do not write a single patch that impacts at least three different
subsystems. As it is, it is unmergeable.

Now the real question is *WHY* we need this kind of monstruosity?
ACPI deals with PCI, not with exotic busses and whatnot. If you want
to be creative, DT is your space. ACPI is designed to be plain and
boring, and that's how we like it.

Thanks,

         M.

Lorenzo Pieralisi Jan. 28, 2020, 11:09 a.m. UTC | #2

On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
> ACPI support is added in the fsl-mc driver. Driver will parse
> MC DSDT table to extract memory and other resorces.
> 
> Interrupt (GIC ITS) information will be extracted from MADT table
> by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> 
> IORT table will be parsed to configure DMA.
> 
> Signed-off-by: Makarand Pawagi <[e-mail]>
> ---
>  drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
>  drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
>  drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
>  drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
>  drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
>  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71 ++++++++++++++++++++++++++++-
>  include/linux/acpi_iort.h                   |  5 ++
>  7 files changed, 174 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 33f7198..beb9cd5 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -15,6 +15,7 @@
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/pci.h>
> +#include <linux/fsl/mc.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
> @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct device *dev, u32 req_id,
>  }
>  
>  /**
> + * iort_get_fsl_mc_device_domain() - Find MSI domain related to a device
> + * @dev: The device.
> + * @mc_icid: ICID for the fsl_mc device.
> + *
> + * Returns: the MSI domain for this device, NULL otherwise
> + */
> +struct irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
> +							u32 mc_icid)
> +{
> +	struct fwnode_handle *handle;
> +	int its_id;
> +
> +	if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
> +		return NULL;
> +
> +	handle = iort_find_domain_token(its_id);
> +	if (!handle)
> +		return NULL;
> +
> +	return irq_find_matching_fwnode(handle, DOMAIN_BUS_FSL_MC_MSI);
> +}

NAK

I am not willing to take platform specific code in the generic IORT
layer.

ACPI on ARM64 works on platforms that comply with SBSA/SBBR guidelines:

https://developer.arm.com/architectures/platform-design/server-systems

Deviating from those requires butchering ACPI specifications (ie IORT)
and related kernel code which goes totally against what ACPI is meant
for on ARM64 systems, so there is no upstream pathway for this code
I am afraid.

Thanks,
Lorenzo

Makarand Pawagi Jan. 31, 2020, 10:35 a.m. UTC | #3

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: Tuesday, January 28, 2020 4:39 PM
> To: Makarand Pawagi <[e-mail]>
> Cc: [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Cristi Sovaiala <[e-mail]>; Laurentiu
> Tudor <[e-mail]>; Ioana Ciornei <[e-mail]>;
> Varun Sethi <[e-mail]>; Calvin Johnson <[e-mail]>;
> Pankaj Bansal <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> 
> Caution: EXT Email
> 
> On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
> > ACPI support is added in the fsl-mc driver. Driver will parse MC DSDT
> > table to extract memory and other resorces.
> >
> > Interrupt (GIC ITS) information will be extracted from MADT table by
> > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> >
> > IORT table will be parsed to configure DMA.
> >
> > Signed-off-by: Makarand Pawagi <[e-mail]>
> > ---
> >  drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
> >  drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
> >  drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
> >  drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
> >  drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
> >  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
> ++++++++++++++++++++++++++++-
> >  include/linux/acpi_iort.h                   |  5 ++
> >  7 files changed, 174 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index 33f7198..beb9cd5 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> >  #include <linux/pci.h>
> > +#include <linux/fsl/mc.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >
> > @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct device
> > *dev, u32 req_id,  }
> >
> >  /**
> > + * iort_get_fsl_mc_device_domain() - Find MSI domain related to a
> > +device
> > + * @dev: The device.
> > + * @mc_icid: ICID for the fsl_mc device.
> > + *
> > + * Returns: the MSI domain for this device, NULL otherwise  */ struct
> > +irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
> > +                                                     u32 mc_icid) {
> > +     struct fwnode_handle *handle;
> > +     int its_id;
> > +
> > +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
> > +             return NULL;
> > +
> > +     handle = iort_find_domain_token(its_id);
> > +     if (!handle)
> > +             return NULL;
> > +
> > +     return irq_find_matching_fwnode(handle, DOMAIN_BUS_FSL_MC_MSI);
> > +}
> 
> NAK
> 
> I am not willing to take platform specific code in the generic IORT layer.
> 
> ACPI on ARM64 works on platforms that comply with SBSA/SBBR guidelines:
> 
> 
> https://developer.arm.com/architectures/platform-design/server-systems
>
> Deviating from those requires butchering ACPI specifications (ie IORT) and
> related kernel code which goes totally against what ACPI is meant for on ARM64
> systems, so there is no upstream pathway for this code I am afraid.
> 
Reason of adding this platform specific function in the generic IORT layer is 
That iort_get_device_domain() only deals with PCI bus (DOMAIN_BUS_PCI_MSI).

fsl-mc objects when probed, need to find irq_domain which is associated with
the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will not be possible to do that
if we do not add this function because there are no other suitable APIs exported
by IORT layer to do the job.

Lorenzo Pieralisi Jan. 31, 2020, 11:06 a.m. UTC | #4

On Fri, Jan 31, 2020 at 10:35:48AM +0000, Makarand Pawagi wrote:
> > -----Original Message-----
> > From: Lorenzo Pieralisi <[e-mail]>
> > Sent: Tuesday, January 28, 2020 4:39 PM
> > To: Makarand Pawagi <[e-mail]>
> > Cc: [e-mail]; [e-mail]; linux-arm-
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Cristi Sovaiala <[e-mail]>; Laurentiu
> > Tudor <[e-mail]>; Ioana Ciornei <[e-mail]>;
> > Varun Sethi <[e-mail]>; Calvin Johnson <[e-mail]>;
> > Pankaj Bansal <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]
> > Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> > 
> > Caution: EXT Email
> > 
> > On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
> > > ACPI support is added in the fsl-mc driver. Driver will parse MC DSDT
> > > table to extract memory and other resorces.
> > >
> > > Interrupt (GIC ITS) information will be extracted from MADT table by
> > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> > >
> > > IORT table will be parsed to configure DMA.
> > >
> > > Signed-off-by: Makarand Pawagi <[e-mail]>
> > > ---
> > >  drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
> > >  drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
> > >  drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
> > >  drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
> > >  drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
> > >  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
> > ++++++++++++++++++++++++++++-
> > >  include/linux/acpi_iort.h                   |  5 ++
> > >  7 files changed, 174 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > index 33f7198..beb9cd5 100644
> > > --- a/drivers/acpi/arm64/iort.c
> > > +++ b/drivers/acpi/arm64/iort.c
> > > @@ -15,6 +15,7 @@
> > >  #include <linux/kernel.h>
> > >  #include <linux/list.h>
> > >  #include <linux/pci.h>
> > > +#include <linux/fsl/mc.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/slab.h>
> > >
> > > @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct device
> > > *dev, u32 req_id,  }
> > >
> > >  /**
> > > + * iort_get_fsl_mc_device_domain() - Find MSI domain related to a
> > > +device
> > > + * @dev: The device.
> > > + * @mc_icid: ICID for the fsl_mc device.
> > > + *
> > > + * Returns: the MSI domain for this device, NULL otherwise  */ struct
> > > +irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
> > > +                                                     u32 mc_icid) {
> > > +     struct fwnode_handle *handle;
> > > +     int its_id;
> > > +
> > > +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
> > > +             return NULL;
> > > +
> > > +     handle = iort_find_domain_token(its_id);
> > > +     if (!handle)
> > > +             return NULL;
> > > +
> > > +     return irq_find_matching_fwnode(handle, DOMAIN_BUS_FSL_MC_MSI);
> > > +}
> > 
> > NAK
> > 
> > I am not willing to take platform specific code in the generic IORT layer.
> > 
> > ACPI on ARM64 works on platforms that comply with SBSA/SBBR guidelines:
> > 
> > 
> > https://developer.arm.com/architectures/platform-design/server-systems
> >
> > Deviating from those requires butchering ACPI specifications (ie IORT) and
> > related kernel code which goes totally against what ACPI is meant for on ARM64
> > systems, so there is no upstream pathway for this code I am afraid.
> > 
> Reason of adding this platform specific function in the generic IORT
> layer is That iort_get_device_domain() only deals with PCI bus
> (DOMAIN_BUS_PCI_MSI).
> 
> fsl-mc objects when probed, need to find irq_domain which is
> associated with the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will not be
> possible to do that if we do not add this function because there are
> no other suitable APIs exported by IORT layer to do the job.

And that's by design.

I don't know what the FSL bus is and I don't want to know, what
I am telling you is that the ACPI code in the mainline is sufficient
to support SBSA compliant HW and that's what we support with ACPI
on ARM64.

We won't hack the kernel (and ACPI tables) up to boot with ACPI on
non-compliant platforms, I don't know how I can be any clearer than
that.

All is needed to configure the (platform dev/PCI->IOMMU->ITS) chain is
in the ACPI/IORT specifications and again, that's by design, adding
DSDT objects and hacking the kernel to make it work "like DT" won't
cut it, you are solving the wrong problem here, boot this platform
with a device tree, it is a problem that has been solved a long time
ago and it is supported in the mainline kernel.

Thanks,
Lorenzo

Marc Zyngier Jan. 31, 2020, 11:06 a.m. UTC | #5

On 2020-01-31 10:35, Makarand Pawagi wrote:
>> -----Original Message-----
>> From: Lorenzo Pieralisi <[e-mail]>
>> Sent: Tuesday, January 28, 2020 4:39 PM
>> To: Makarand Pawagi <[e-mail]>
>> Cc: [e-mail]; [e-mail]; linux-arm-
>> [e-mail]; [e-mail]; 
>> [e-mail];
>> [e-mail]; Cristi Sovaiala <[e-mail]>; 
>> Laurentiu
>> Tudor <[e-mail]>; Ioana Ciornei 
>> <[e-mail]>;
>> Varun Sethi <[e-mail]>; Calvin Johnson 
>> <[e-mail]>;
>> Pankaj Bansal <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
>> 
>> Caution: EXT Email
>> 
>> On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
>> > ACPI support is added in the fsl-mc driver. Driver will parse MC DSDT
>> > table to extract memory and other resorces.
>> >
>> > Interrupt (GIC ITS) information will be extracted from MADT table by
>> > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
>> >
>> > IORT table will be parsed to configure DMA.
>> >
>> > Signed-off-by: Makarand Pawagi <[e-mail]>
>> > ---
>> >  drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
>> >  drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
>> >  drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
>> >  drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
>> >  drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
>> >  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
>> ++++++++++++++++++++++++++++-
>> >  include/linux/acpi_iort.h                   |  5 ++
>> >  7 files changed, 174 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> > index 33f7198..beb9cd5 100644
>> > --- a/drivers/acpi/arm64/iort.c
>> > +++ b/drivers/acpi/arm64/iort.c
>> > @@ -15,6 +15,7 @@
>> >  #include <linux/kernel.h>
>> >  #include <linux/list.h>
>> >  #include <linux/pci.h>
>> > +#include <linux/fsl/mc.h>
>> >  #include <linux/platform_device.h>
>> >  #include <linux/slab.h>
>> >
>> > @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct device
>> > *dev, u32 req_id,  }
>> >
>> >  /**
>> > + * iort_get_fsl_mc_device_domain() - Find MSI domain related to a
>> > +device
>> > + * @dev: The device.
>> > + * @mc_icid: ICID for the fsl_mc device.
>> > + *
>> > + * Returns: the MSI domain for this device, NULL otherwise  */ struct
>> > +irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
>> > +                                                     u32 mc_icid) {
>> > +     struct fwnode_handle *handle;
>> > +     int its_id;
>> > +
>> > +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
>> > +             return NULL;
>> > +
>> > +     handle = iort_find_domain_token(its_id);
>> > +     if (!handle)
>> > +             return NULL;
>> > +
>> > +     return irq_find_matching_fwnode(handle, DOMAIN_BUS_FSL_MC_MSI);
>> > +}
>> 
>> NAK
>> 
>> I am not willing to take platform specific code in the generic IORT 
>> layer.
>> 
>> ACPI on ARM64 works on platforms that comply with SBSA/SBBR 
>> guidelines:
>> 
>> 
>> https://developer.arm.com/architectures/platform-design/server-systems
>> 
>> Deviating from those requires butchering ACPI specifications (ie IORT) 
>> and
>> related kernel code which goes totally against what ACPI is meant for 
>> on ARM64
>> systems, so there is no upstream pathway for this code I am afraid.
>> 
> Reason of adding this platform specific function in the generic IORT 
> layer is
> That iort_get_device_domain() only deals with PCI bus 
> (DOMAIN_BUS_PCI_MSI).
> 
> fsl-mc objects when probed, need to find irq_domain which is associated 
> with
> the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will not be possible to do 
> that
> if we do not add this function because there are no other suitable APIs 
> exported
> by IORT layer to do the job.

I think we all understood the patch. What both Lorenzo and myself are 
saying is
that we do not want non-PCI support in IORT.

You have decided to have exotic hardware, and sidestep all the 
standardization
efforts. This is your right. But you can't have your cake and eat it.

Thanks,

         M.

Ard Biesheuvel Jan. 31, 2020, 12:01 p.m. UTC | #6

On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <[e-mail]> wrote:
>
> On 2020-01-31 10:35, Makarand Pawagi wrote:
> >> -----Original Message-----
> >> From: Lorenzo Pieralisi <[e-mail]>
> >> Sent: Tuesday, January 28, 2020 4:39 PM
> >> To: Makarand Pawagi <[e-mail]>
> >> Cc: [e-mail]; [e-mail]; linux-arm-
> >> [e-mail]; [e-mail];
> >> [e-mail];
> >> [e-mail]; Cristi Sovaiala <[e-mail]>;
> >> Laurentiu
> >> Tudor <[e-mail]>; Ioana Ciornei
> >> <[e-mail]>;
> >> Varun Sethi <[e-mail]>; Calvin Johnson
> >> <[e-mail]>;
> >> Pankaj Bansal <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> >>
> >> Caution: EXT Email
> >>
> >> On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
> >> > ACPI support is added in the fsl-mc driver. Driver will parse MC DSDT
> >> > table to extract memory and other resorces.
> >> >
> >> > Interrupt (GIC ITS) information will be extracted from MADT table by
> >> > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> >> >
> >> > IORT table will be parsed to configure DMA.
> >> >
> >> > Signed-off-by: Makarand Pawagi <[e-mail]>
> >> > ---
> >> >  drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
> >> >  drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
> >> >  drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
> >> >  drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
> >> >  drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
> >> >  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
> >> ++++++++++++++++++++++++++++-
> >> >  include/linux/acpi_iort.h                   |  5 ++
> >> >  7 files changed, 174 insertions(+), 20 deletions(-)
> >> >
> >> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >> > index 33f7198..beb9cd5 100644
> >> > --- a/drivers/acpi/arm64/iort.c
> >> > +++ b/drivers/acpi/arm64/iort.c
> >> > @@ -15,6 +15,7 @@
> >> >  #include <linux/kernel.h>
> >> >  #include <linux/list.h>
> >> >  #include <linux/pci.h>
> >> > +#include <linux/fsl/mc.h>
> >> >  #include <linux/platform_device.h>
> >> >  #include <linux/slab.h>
> >> >
> >> > @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct device
> >> > *dev, u32 req_id,  }
> >> >
> >> >  /**
> >> > + * iort_get_fsl_mc_device_domain() - Find MSI domain related to a
> >> > +device
> >> > + * @dev: The device.
> >> > + * @mc_icid: ICID for the fsl_mc device.
> >> > + *
> >> > + * Returns: the MSI domain for this device, NULL otherwise  */ struct
> >> > +irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
> >> > +                                                     u32 mc_icid) {
> >> > +     struct fwnode_handle *handle;
> >> > +     int its_id;
> >> > +
> >> > +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
> >> > +             return NULL;
> >> > +
> >> > +     handle = iort_find_domain_token(its_id);
> >> > +     if (!handle)
> >> > +             return NULL;
> >> > +
> >> > +     return irq_find_matching_fwnode(handle, DOMAIN_BUS_FSL_MC_MSI);
> >> > +}
> >>
> >> NAK
> >>
> >> I am not willing to take platform specific code in the generic IORT
> >> layer.
> >>
> >> ACPI on ARM64 works on platforms that comply with SBSA/SBBR
> >> guidelines:
> >>
> >>
> >> https://developer.arm.com/architectures/platform-design/server-systems
> >>
> >> Deviating from those requires butchering ACPI specifications (ie IORT)
> >> and
> >> related kernel code which goes totally against what ACPI is meant for
> >> on ARM64
> >> systems, so there is no upstream pathway for this code I am afraid.
> >>
> > Reason of adding this platform specific function in the generic IORT
> > layer is
> > That iort_get_device_domain() only deals with PCI bus
> > (DOMAIN_BUS_PCI_MSI).
> >
> > fsl-mc objects when probed, need to find irq_domain which is associated
> > with
> > the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will not be possible to do
> > that
> > if we do not add this function because there are no other suitable APIs
> > exported
> > by IORT layer to do the job.
>
> I think we all understood the patch. What both Lorenzo and myself are
> saying is
> that we do not want non-PCI support in IORT.
>

IORT supports platform devices (aka named components) as well, and
there is some support for platform MSIs in the GIC layer.

So it may be possible to hide your exotic bus from the OS entirely,
and make the firmware instantiate a DSDT with device objects and
associated IORT nodes that describe whatever lives on that bus as
named components.

That way, you will not have to change the OS at all, so your hardware
will not only be supported in linux v5.7+, it will also be supported
by OSes that commercial distro vendors are shipping today. *That* is
the whole point of using ACPI.

If you are going to bother and modify the OS, you lose this advantage,
and ACPI gives you no benefit over DT at all.

Jon Nettleton Jan. 31, 2020, 12:28 p.m. UTC | #7

On Fri, Jan 31, 2020 at 1:02 PM Ard Biesheuvel
<[e-mail]> wrote:
>
> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <[e-mail]> wrote:
> >
> > On 2020-01-31 10:35, Makarand Pawagi wrote:
> > >> -----Original Message-----
> > >> From: Lorenzo Pieralisi <[e-mail]>
> > >> Sent: Tuesday, January 28, 2020 4:39 PM
> > >> To: Makarand Pawagi <[e-mail]>
> > >> Cc: [e-mail]; [e-mail]; linux-arm-
> > >> [e-mail]; [e-mail];
> > >> [e-mail];
> > >> [e-mail]; Cristi Sovaiala <[e-mail]>;
> > >> Laurentiu
> > >> Tudor <[e-mail]>; Ioana Ciornei
> > >> <[e-mail]>;
> > >> Varun Sethi <[e-mail]>; Calvin Johnson
> > >> <[e-mail]>;
> > >> Pankaj Bansal <[e-mail]>; [e-mail];
> > >> [e-mail]; [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail]
> > >> Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> > >>
> > >> Caution: EXT Email
> > >>
> > >> On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
> > >> > ACPI support is added in the fsl-mc driver. Driver will parse MC DSDT
> > >> > table to extract memory and other resorces.
> > >> >
> > >> > Interrupt (GIC ITS) information will be extracted from MADT table by
> > >> > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> > >> >
> > >> > IORT table will be parsed to configure DMA.
> > >> >
> > >> > Signed-off-by: Makarand Pawagi <[e-mail]>
> > >> > ---
> > >> >  drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
> > >> >  drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
> > >> >  drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
> > >> >  drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
> > >> >  drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
> > >> >  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
> > >> ++++++++++++++++++++++++++++-
> > >> >  include/linux/acpi_iort.h                   |  5 ++
> > >> >  7 files changed, 174 insertions(+), 20 deletions(-)
> > >> >
> > >> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > >> > index 33f7198..beb9cd5 100644
> > >> > --- a/drivers/acpi/arm64/iort.c
> > >> > +++ b/drivers/acpi/arm64/iort.c
> > >> > @@ -15,6 +15,7 @@
> > >> >  #include <linux/kernel.h>
> > >> >  #include <linux/list.h>
> > >> >  #include <linux/pci.h>
> > >> > +#include <linux/fsl/mc.h>
> > >> >  #include <linux/platform_device.h>
> > >> >  #include <linux/slab.h>
> > >> >
> > >> > @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct device
> > >> > *dev, u32 req_id,  }
> > >> >
> > >> >  /**
> > >> > + * iort_get_fsl_mc_device_domain() - Find MSI domain related to a
> > >> > +device
> > >> > + * @dev: The device.
> > >> > + * @mc_icid: ICID for the fsl_mc device.
> > >> > + *
> > >> > + * Returns: the MSI domain for this device, NULL otherwise  */ struct
> > >> > +irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
> > >> > +                                                     u32 mc_icid) {
> > >> > +     struct fwnode_handle *handle;
> > >> > +     int its_id;
> > >> > +
> > >> > +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
> > >> > +             return NULL;
> > >> > +
> > >> > +     handle = iort_find_domain_token(its_id);
> > >> > +     if (!handle)
> > >> > +             return NULL;
> > >> > +
> > >> > +     return irq_find_matching_fwnode(handle, DOMAIN_BUS_FSL_MC_MSI);
> > >> > +}
> > >>
> > >> NAK
> > >>
> > >> I am not willing to take platform specific code in the generic IORT
> > >> layer.
> > >>
> > >> ACPI on ARM64 works on platforms that comply with SBSA/SBBR
> > >> guidelines:
> > >>
> > >>
> > >> https://developer.arm.com/architectures/platform-design/server-systems
> > >>
> > >> Deviating from those requires butchering ACPI specifications (ie IORT)
> > >> and
> > >> related kernel code which goes totally against what ACPI is meant for
> > >> on ARM64
> > >> systems, so there is no upstream pathway for this code I am afraid.
> > >>
> > > Reason of adding this platform specific function in the generic IORT
> > > layer is
> > > That iort_get_device_domain() only deals with PCI bus
> > > (DOMAIN_BUS_PCI_MSI).
> > >
> > > fsl-mc objects when probed, need to find irq_domain which is associated
> > > with
> > > the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will not be possible to do
> > > that
> > > if we do not add this function because there are no other suitable APIs
> > > exported
> > > by IORT layer to do the job.
> >
> > I think we all understood the patch. What both Lorenzo and myself are
> > saying is
> > that we do not want non-PCI support in IORT.
> >
>
> IORT supports platform devices (aka named components) as well, and
> there is some support for platform MSIs in the GIC layer.
>
> So it may be possible to hide your exotic bus from the OS entirely,
> and make the firmware instantiate a DSDT with device objects and
> associated IORT nodes that describe whatever lives on that bus as
> named components.
>
> That way, you will not have to change the OS at all, so your hardware
> will not only be supported in linux v5.7+, it will also be supported
> by OSes that commercial distro vendors are shipping today. *That* is
> the whole point of using ACPI.
>
> If you are going to bother and modify the OS, you lose this advantage,
> and ACPI gives you no benefit over DT at all.

You beat me to it, but thanks for the clarification Ard.  No where in
the SBSA spec that I have read does it state that only PCIe devices
are supported by the SMMU.  It uses PCIe devices as an example, but
the SMMU section is very generic in term and only says "devices".

I feel the SBSA omission of SerDes best practices is an oversight in
the standard and something that probably needs to be revisited.
Forcing high speed networking interfaces to be hung off a bus just for
the sake of having a "standard" PCIe interface seems like a step
backward in this regard.  I would much rather have the Spec include a
common standard that could be exposed in a consistent manner.  But
this is a conversation for a different place.

I will work with NXP and find a better way to implement this.

-Jon

Robin Murphy Jan. 31, 2020, 12:48 p.m. UTC | #8

On 2020-01-31 12:28 pm, Jon Nettleton wrote:
> On Fri, Jan 31, 2020 at 1:02 PM Ard Biesheuvel
> <[e-mail]> wrote:
>>
>> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <[e-mail]> wrote:
>>>
>>> On 2020-01-31 10:35, Makarand Pawagi wrote:
>>>>> -----Original Message-----
>>>>> From: Lorenzo Pieralisi <[e-mail]>
>>>>> Sent: Tuesday, January 28, 2020 4:39 PM
>>>>> To: Makarand Pawagi <[e-mail]>
>>>>> Cc: [e-mail]; [e-mail]; linux-arm-
>>>>> [e-mail]; [e-mail];
>>>>> [e-mail];
>>>>> [e-mail]; Cristi Sovaiala <[e-mail]>;
>>>>> Laurentiu
>>>>> Tudor <[e-mail]>; Ioana Ciornei
>>>>> <[e-mail]>;
>>>>> Varun Sethi <[e-mail]>; Calvin Johnson
>>>>> <[e-mail]>;
>>>>> Pankaj Bansal <[e-mail]>; [e-mail];
>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail]
>>>>> Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
>>>>>
>>>>> Caution: EXT Email
>>>>>
>>>>> On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
>>>>>> ACPI support is added in the fsl-mc driver. Driver will parse MC DSDT
>>>>>> table to extract memory and other resorces.
>>>>>>
>>>>>> Interrupt (GIC ITS) information will be extracted from MADT table by
>>>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
>>>>>>
>>>>>> IORT table will be parsed to configure DMA.
>>>>>>
>>>>>> Signed-off-by: Makarand Pawagi <[e-mail]>
>>>>>> ---
>>>>>>   drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
>>>>>>   drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
>>>>>>   drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
>>>>>>   drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
>>>>>>   drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
>>>>>>   drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
>>>>> ++++++++++++++++++++++++++++-
>>>>>>   include/linux/acpi_iort.h                   |  5 ++
>>>>>>   7 files changed, 174 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>>>> index 33f7198..beb9cd5 100644
>>>>>> --- a/drivers/acpi/arm64/iort.c
>>>>>> +++ b/drivers/acpi/arm64/iort.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>>   #include <linux/kernel.h>
>>>>>>   #include <linux/list.h>
>>>>>>   #include <linux/pci.h>
>>>>>> +#include <linux/fsl/mc.h>
>>>>>>   #include <linux/platform_device.h>
>>>>>>   #include <linux/slab.h>
>>>>>>
>>>>>> @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct device
>>>>>> *dev, u32 req_id,  }
>>>>>>
>>>>>>   /**
>>>>>> + * iort_get_fsl_mc_device_domain() - Find MSI domain related to a
>>>>>> +device
>>>>>> + * @dev: The device.
>>>>>> + * @mc_icid: ICID for the fsl_mc device.
>>>>>> + *
>>>>>> + * Returns: the MSI domain for this device, NULL otherwise  */ struct
>>>>>> +irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
>>>>>> +                                                     u32 mc_icid) {
>>>>>> +     struct fwnode_handle *handle;
>>>>>> +     int its_id;
>>>>>> +
>>>>>> +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
>>>>>> +             return NULL;
>>>>>> +
>>>>>> +     handle = iort_find_domain_token(its_id);
>>>>>> +     if (!handle)
>>>>>> +             return NULL;
>>>>>> +
>>>>>> +     return irq_find_matching_fwnode(handle, DOMAIN_BUS_FSL_MC_MSI);
>>>>>> +}
>>>>>
>>>>> NAK
>>>>>
>>>>> I am not willing to take platform specific code in the generic IORT
>>>>> layer.
>>>>>
>>>>> ACPI on ARM64 works on platforms that comply with SBSA/SBBR
>>>>> guidelines:
>>>>>
>>>>>
>>>>> https://developer.arm.com/architectures/platform-design/server-systems
>>>>>
>>>>> Deviating from those requires butchering ACPI specifications (ie IORT)
>>>>> and
>>>>> related kernel code which goes totally against what ACPI is meant for
>>>>> on ARM64
>>>>> systems, so there is no upstream pathway for this code I am afraid.
>>>>>
>>>> Reason of adding this platform specific function in the generic IORT
>>>> layer is
>>>> That iort_get_device_domain() only deals with PCI bus
>>>> (DOMAIN_BUS_PCI_MSI).
>>>>
>>>> fsl-mc objects when probed, need to find irq_domain which is associated
>>>> with
>>>> the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will not be possible to do
>>>> that
>>>> if we do not add this function because there are no other suitable APIs
>>>> exported
>>>> by IORT layer to do the job.
>>>
>>> I think we all understood the patch. What both Lorenzo and myself are
>>> saying is
>>> that we do not want non-PCI support in IORT.
>>>
>>
>> IORT supports platform devices (aka named components) as well, and
>> there is some support for platform MSIs in the GIC layer.
>>
>> So it may be possible to hide your exotic bus from the OS entirely,
>> and make the firmware instantiate a DSDT with device objects and
>> associated IORT nodes that describe whatever lives on that bus as
>> named components.
>>
>> That way, you will not have to change the OS at all, so your hardware
>> will not only be supported in linux v5.7+, it will also be supported
>> by OSes that commercial distro vendors are shipping today. *That* is
>> the whole point of using ACPI.
>>
>> If you are going to bother and modify the OS, you lose this advantage,
>> and ACPI gives you no benefit over DT at all.
> 
> You beat me to it, but thanks for the clarification Ard.  No where in
> the SBSA spec that I have read does it state that only PCIe devices
> are supported by the SMMU.  It uses PCIe devices as an example, but
> the SMMU section is very generic in term and only says "devices".
> 
> I feel the SBSA omission of SerDes best practices is an oversight in
> the standard and something that probably needs to be revisited.
> Forcing high speed networking interfaces to be hung off a bus just for
> the sake of having a "standard" PCIe interface seems like a step
> backward in this regard.  I would much rather have the Spec include a
> common standard that could be exposed in a consistent manner.  But
> this is a conversation for a different place.

Just to clarify further, it's not about serdes or high-speed networking 
per se - describing a fixed-function network adapter as a named 
component is entirely within scope. The issue is when the hardware is 
merely a pool of accelerator components that can be dynamically 
configured at runtime into something that looks like one or more 
'virtual' network adapters - there is no standard interface for *that* 
for SBSA to consider.

Robin.

> 
> I will work with NXP and find a better way to implement this.
> 
> -Jon
>

Jon Nettleton Jan. 31, 2020, 1:11 p.m. UTC | #9

On Fri, Jan 31, 2020 at 1:48 PM Robin Murphy <[e-mail]> wrote:
>
> On 2020-01-31 12:28 pm, Jon Nettleton wrote:
> > On Fri, Jan 31, 2020 at 1:02 PM Ard Biesheuvel
> > <[e-mail]> wrote:
> >>
> >> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <[e-mail]> wrote:
> >>>
> >>> On 2020-01-31 10:35, Makarand Pawagi wrote:
> >>>>> -----Original Message-----
> >>>>> From: Lorenzo Pieralisi <[e-mail]>
> >>>>> Sent: Tuesday, January 28, 2020 4:39 PM
> >>>>> To: Makarand Pawagi <[e-mail]>
> >>>>> Cc: [e-mail]; [e-mail]; linux-arm-
> >>>>> [e-mail]; [e-mail];
> >>>>> [e-mail];
> >>>>> [e-mail]; Cristi Sovaiala <[e-mail]>;
> >>>>> Laurentiu
> >>>>> Tudor <[e-mail]>; Ioana Ciornei
> >>>>> <[e-mail]>;
> >>>>> Varun Sethi <[e-mail]>; Calvin Johnson
> >>>>> <[e-mail]>;
> >>>>> Pankaj Bansal <[e-mail]>; [e-mail];
> >>>>> [e-mail]; [e-mail]; [e-mail];
> >>>>> [e-mail]; [e-mail]; [e-mail];
> >>>>> [e-mail]; [e-mail]; [e-mail];
> >>>>> [e-mail]; [e-mail]
> >>>>> Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> >>>>>
> >>>>> Caution: EXT Email
> >>>>>
> >>>>> On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
> >>>>>> ACPI support is added in the fsl-mc driver. Driver will parse MC DSDT
> >>>>>> table to extract memory and other resorces.
> >>>>>>
> >>>>>> Interrupt (GIC ITS) information will be extracted from MADT table by
> >>>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> >>>>>>
> >>>>>> IORT table will be parsed to configure DMA.
> >>>>>>
> >>>>>> Signed-off-by: Makarand Pawagi <[e-mail]>
> >>>>>> ---
> >>>>>>   drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
> >>>>>>   drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
> >>>>>>   drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
> >>>>>>   drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
> >>>>>>   drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
> >>>>>>   drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
> >>>>> ++++++++++++++++++++++++++++-
> >>>>>>   include/linux/acpi_iort.h                   |  5 ++
> >>>>>>   7 files changed, 174 insertions(+), 20 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >>>>>> index 33f7198..beb9cd5 100644
> >>>>>> --- a/drivers/acpi/arm64/iort.c
> >>>>>> +++ b/drivers/acpi/arm64/iort.c
> >>>>>> @@ -15,6 +15,7 @@
> >>>>>>   #include <linux/kernel.h>
> >>>>>>   #include <linux/list.h>
> >>>>>>   #include <linux/pci.h>
> >>>>>> +#include <linux/fsl/mc.h>
> >>>>>>   #include <linux/platform_device.h>
> >>>>>>   #include <linux/slab.h>
> >>>>>>
> >>>>>> @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct device
> >>>>>> *dev, u32 req_id,  }
> >>>>>>
> >>>>>>   /**
> >>>>>> + * iort_get_fsl_mc_device_domain() - Find MSI domain related to a
> >>>>>> +device
> >>>>>> + * @dev: The device.
> >>>>>> + * @mc_icid: ICID for the fsl_mc device.
> >>>>>> + *
> >>>>>> + * Returns: the MSI domain for this device, NULL otherwise  */ struct
> >>>>>> +irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
> >>>>>> +                                                     u32 mc_icid) {
> >>>>>> +     struct fwnode_handle *handle;
> >>>>>> +     int its_id;
> >>>>>> +
> >>>>>> +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
> >>>>>> +             return NULL;
> >>>>>> +
> >>>>>> +     handle = iort_find_domain_token(its_id);
> >>>>>> +     if (!handle)
> >>>>>> +             return NULL;
> >>>>>> +
> >>>>>> +     return irq_find_matching_fwnode(handle, DOMAIN_BUS_FSL_MC_MSI);
> >>>>>> +}
> >>>>>
> >>>>> NAK
> >>>>>
> >>>>> I am not willing to take platform specific code in the generic IORT
> >>>>> layer.
> >>>>>
> >>>>> ACPI on ARM64 works on platforms that comply with SBSA/SBBR
> >>>>> guidelines:
> >>>>>
> >>>>>
> >>>>> https://developer.arm.com/architectures/platform-design/server-systems
> >>>>>
> >>>>> Deviating from those requires butchering ACPI specifications (ie IORT)
> >>>>> and
> >>>>> related kernel code which goes totally against what ACPI is meant for
> >>>>> on ARM64
> >>>>> systems, so there is no upstream pathway for this code I am afraid.
> >>>>>
> >>>> Reason of adding this platform specific function in the generic IORT
> >>>> layer is
> >>>> That iort_get_device_domain() only deals with PCI bus
> >>>> (DOMAIN_BUS_PCI_MSI).
> >>>>
> >>>> fsl-mc objects when probed, need to find irq_domain which is associated
> >>>> with
> >>>> the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will not be possible to do
> >>>> that
> >>>> if we do not add this function because there are no other suitable APIs
> >>>> exported
> >>>> by IORT layer to do the job.
> >>>
> >>> I think we all understood the patch. What both Lorenzo and myself are
> >>> saying is
> >>> that we do not want non-PCI support in IORT.
> >>>
> >>
> >> IORT supports platform devices (aka named components) as well, and
> >> there is some support for platform MSIs in the GIC layer.
> >>
> >> So it may be possible to hide your exotic bus from the OS entirely,
> >> and make the firmware instantiate a DSDT with device objects and
> >> associated IORT nodes that describe whatever lives on that bus as
> >> named components.
> >>
> >> That way, you will not have to change the OS at all, so your hardware
> >> will not only be supported in linux v5.7+, it will also be supported
> >> by OSes that commercial distro vendors are shipping today. *That* is
> >> the whole point of using ACPI.
> >>
> >> If you are going to bother and modify the OS, you lose this advantage,
> >> and ACPI gives you no benefit over DT at all.
> >
> > You beat me to it, but thanks for the clarification Ard.  No where in
> > the SBSA spec that I have read does it state that only PCIe devices
> > are supported by the SMMU.  It uses PCIe devices as an example, but
> > the SMMU section is very generic in term and only says "devices".
> >
> > I feel the SBSA omission of SerDes best practices is an oversight in
> > the standard and something that probably needs to be revisited.
> > Forcing high speed networking interfaces to be hung off a bus just for
> > the sake of having a "standard" PCIe interface seems like a step
> > backward in this regard.  I would much rather have the Spec include a
> > common standard that could be exposed in a consistent manner.  But
> > this is a conversation for a different place.
>
> Just to clarify further, it's not about serdes or high-speed networking
> per se - describing a fixed-function network adapter as a named
> component is entirely within scope. The issue is when the hardware is
> merely a pool of accelerator components that can be dynamically
> configured at runtime into something that looks like one or more
> 'virtual' network adapters - there is no standard interface for *that*
> for SBSA to consider.
>
> Robin.
>
> >
> > I will work with NXP and find a better way to implement this.
> >
> > -Jon
> >

But by design SFP, SFP+, and QSFP cages are not fixed function network
adapters.  They are physical and logical devices that can adapt to
what is plugged into them.  How the devices are exposed should be
irrelevant to this conversation it is about the underlying
connectivity.  For instance if this were an accelerator block on a
PCIe card then we wouldn't be having this discussion, even if it did
run a firmware and have a third party driver that exposed virtual
network interfaces.

-Jon

Ard Biesheuvel Jan. 31, 2020, 1:29 p.m. UTC | #10

On Fri, 31 Jan 2020 at 14:12, Jon Nettleton <[e-mail]> wrote:
>
> On Fri, Jan 31, 2020 at 1:48 PM Robin Murphy <[e-mail]> wrote:
> >
> > On 2020-01-31 12:28 pm, Jon Nettleton wrote:
> > > On Fri, Jan 31, 2020 at 1:02 PM Ard Biesheuvel
> > > <[e-mail]> wrote:
> > >>
> > >> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <[e-mail]> wrote:
> > >>>
> > >>> On 2020-01-31 10:35, Makarand Pawagi wrote:
> > >>>>> -----Original Message-----
> > >>>>> From: Lorenzo Pieralisi <[e-mail]>
> > >>>>> Sent: Tuesday, January 28, 2020 4:39 PM
> > >>>>> To: Makarand Pawagi <[e-mail]>
> > >>>>> Cc: [e-mail]; [e-mail]; linux-arm-
> > >>>>> [e-mail]; [e-mail];
> > >>>>> [e-mail];
> > >>>>> [e-mail]; Cristi Sovaiala <[e-mail]>;
> > >>>>> Laurentiu
> > >>>>> Tudor <[e-mail]>; Ioana Ciornei
> > >>>>> <[e-mail]>;
> > >>>>> Varun Sethi <[e-mail]>; Calvin Johnson
> > >>>>> <[e-mail]>;
> > >>>>> Pankaj Bansal <[e-mail]>; [e-mail];
> > >>>>> [e-mail]; [e-mail]; [e-mail];
> > >>>>> [e-mail]; [e-mail]; [e-mail];
> > >>>>> [e-mail]; [e-mail]; [e-mail];
> > >>>>> [e-mail]; [e-mail]
> > >>>>> Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> > >>>>>
> > >>>>> Caution: EXT Email
> > >>>>>
> > >>>>> On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
> > >>>>>> ACPI support is added in the fsl-mc driver. Driver will parse MC DSDT
> > >>>>>> table to extract memory and other resorces.
> > >>>>>>
> > >>>>>> Interrupt (GIC ITS) information will be extracted from MADT table by
> > >>>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> > >>>>>>
> > >>>>>> IORT table will be parsed to configure DMA.
> > >>>>>>
> > >>>>>> Signed-off-by: Makarand Pawagi <[e-mail]>
> > >>>>>> ---
> > >>>>>>   drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
> > >>>>>>   drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
> > >>>>>>   drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
> > >>>>>>   drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
> > >>>>>>   drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
> > >>>>>>   drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
> > >>>>> ++++++++++++++++++++++++++++-
> > >>>>>>   include/linux/acpi_iort.h                   |  5 ++
> > >>>>>>   7 files changed, 174 insertions(+), 20 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > >>>>>> index 33f7198..beb9cd5 100644
> > >>>>>> --- a/drivers/acpi/arm64/iort.c
> > >>>>>> +++ b/drivers/acpi/arm64/iort.c
> > >>>>>> @@ -15,6 +15,7 @@
> > >>>>>>   #include <linux/kernel.h>
> > >>>>>>   #include <linux/list.h>
> > >>>>>>   #include <linux/pci.h>
> > >>>>>> +#include <linux/fsl/mc.h>
> > >>>>>>   #include <linux/platform_device.h>
> > >>>>>>   #include <linux/slab.h>
> > >>>>>>
> > >>>>>> @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct device
> > >>>>>> *dev, u32 req_id,  }
> > >>>>>>
> > >>>>>>   /**
> > >>>>>> + * iort_get_fsl_mc_device_domain() - Find MSI domain related to a
> > >>>>>> +device
> > >>>>>> + * @dev: The device.
> > >>>>>> + * @mc_icid: ICID for the fsl_mc device.
> > >>>>>> + *
> > >>>>>> + * Returns: the MSI domain for this device, NULL otherwise  */ struct
> > >>>>>> +irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
> > >>>>>> +                                                     u32 mc_icid) {
> > >>>>>> +     struct fwnode_handle *handle;
> > >>>>>> +     int its_id;
> > >>>>>> +
> > >>>>>> +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
> > >>>>>> +             return NULL;
> > >>>>>> +
> > >>>>>> +     handle = iort_find_domain_token(its_id);
> > >>>>>> +     if (!handle)
> > >>>>>> +             return NULL;
> > >>>>>> +
> > >>>>>> +     return irq_find_matching_fwnode(handle, DOMAIN_BUS_FSL_MC_MSI);
> > >>>>>> +}
> > >>>>>
> > >>>>> NAK
> > >>>>>
> > >>>>> I am not willing to take platform specific code in the generic IORT
> > >>>>> layer.
> > >>>>>
> > >>>>> ACPI on ARM64 works on platforms that comply with SBSA/SBBR
> > >>>>> guidelines:
> > >>>>>
> > >>>>>
> > >>>>> https://developer.arm.com/architectures/platform-design/server-systems
> > >>>>>
> > >>>>> Deviating from those requires butchering ACPI specifications (ie IORT)
> > >>>>> and
> > >>>>> related kernel code which goes totally against what ACPI is meant for
> > >>>>> on ARM64
> > >>>>> systems, so there is no upstream pathway for this code I am afraid.
> > >>>>>
> > >>>> Reason of adding this platform specific function in the generic IORT
> > >>>> layer is
> > >>>> That iort_get_device_domain() only deals with PCI bus
> > >>>> (DOMAIN_BUS_PCI_MSI).
> > >>>>
> > >>>> fsl-mc objects when probed, need to find irq_domain which is associated
> > >>>> with
> > >>>> the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will not be possible to do
> > >>>> that
> > >>>> if we do not add this function because there are no other suitable APIs
> > >>>> exported
> > >>>> by IORT layer to do the job.
> > >>>
> > >>> I think we all understood the patch. What both Lorenzo and myself are
> > >>> saying is
> > >>> that we do not want non-PCI support in IORT.
> > >>>
> > >>
> > >> IORT supports platform devices (aka named components) as well, and
> > >> there is some support for platform MSIs in the GIC layer.
> > >>
> > >> So it may be possible to hide your exotic bus from the OS entirely,
> > >> and make the firmware instantiate a DSDT with device objects and
> > >> associated IORT nodes that describe whatever lives on that bus as
> > >> named components.
> > >>
> > >> That way, you will not have to change the OS at all, so your hardware
> > >> will not only be supported in linux v5.7+, it will also be supported
> > >> by OSes that commercial distro vendors are shipping today. *That* is
> > >> the whole point of using ACPI.
> > >>
> > >> If you are going to bother and modify the OS, you lose this advantage,
> > >> and ACPI gives you no benefit over DT at all.
> > >
> > > You beat me to it, but thanks for the clarification Ard.  No where in
> > > the SBSA spec that I have read does it state that only PCIe devices
> > > are supported by the SMMU.  It uses PCIe devices as an example, but
> > > the SMMU section is very generic in term and only says "devices".
> > >
> > > I feel the SBSA omission of SerDes best practices is an oversight in
> > > the standard and something that probably needs to be revisited.
> > > Forcing high speed networking interfaces to be hung off a bus just for
> > > the sake of having a "standard" PCIe interface seems like a step
> > > backward in this regard.  I would much rather have the Spec include a
> > > common standard that could be exposed in a consistent manner.  But
> > > this is a conversation for a different place.
> >
> > Just to clarify further, it's not about serdes or high-speed networking
> > per se - describing a fixed-function network adapter as a named
> > component is entirely within scope. The issue is when the hardware is
> > merely a pool of accelerator components that can be dynamically
> > configured at runtime into something that looks like one or more
> > 'virtual' network adapters - there is no standard interface for *that*
> > for SBSA to consider.
> >
> > Robin.
> >
> > >
> > > I will work with NXP and find a better way to implement this.
> > >
> > > -Jon
> > >
>
> But by design SFP, SFP+, and QSFP cages are not fixed function network
> adapters.  They are physical and logical devices that can adapt to
> what is plugged into them.  How the devices are exposed should be
> irrelevant to this conversation it is about the underlying
> connectivity.  For instance if this were an accelerator block on a
> PCIe card then we wouldn't be having this discussion, even if it did
> run a firmware and have a third party driver that exposed virtual
> network interfaces.
>

Again, on an ACPI system, it is the firmware's job to abstract away
from these platform details. Firmware can perform logical hotplug and
hot unplug of virtual devices, and hide the discovery and
initalization of those pluggable gadgets from the OS. If the
configuration changes, one virtual device appears and another one
disappears, and the firmware can signal such a change to the OS.

What is needed here is for people to really buy into the ACPI
paradigm, rather than treat it as MS flavored DT, where every little
soc detail is decribed to, and therefore managed by the OS. As we
know, there are substantial scalability issues around DT, which is why
ACPI aims to provide a higher level of abstraction, with more runtime
firmware to iron out the wrinkles.

In general, simply inventing ACPI _HIDs for every DT node that
describes a given platform, and adding ACPI probe support to the
existing drivers literally gives you the worst of both worlds, and in
this case [where we are getting into interrupt and DMA routing], it is
not even feasible to begin with.

So I'd suggest approaching this from the opposite direction: don't
look at the existing DT description and existing drivers, but look at
what ACPI supports today, and how the hardware can be made to look
like that through the use of firmware magic.

Robin Murphy Jan. 31, 2020, 1:39 p.m. UTC | #11

On 2020-01-31 1:11 pm, Jon Nettleton wrote:
> On Fri, Jan 31, 2020 at 1:48 PM Robin Murphy <[e-mail]> wrote:
>>
>> On 2020-01-31 12:28 pm, Jon Nettleton wrote:
>>> On Fri, Jan 31, 2020 at 1:02 PM Ard Biesheuvel
>>> <[e-mail]> wrote:
>>>>
>>>> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <[e-mail]> wrote:
>>>>>
>>>>> On 2020-01-31 10:35, Makarand Pawagi wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Lorenzo Pieralisi <[e-mail]>
>>>>>>> Sent: Tuesday, January 28, 2020 4:39 PM
>>>>>>> To: Makarand Pawagi <[e-mail]>
>>>>>>> Cc: [e-mail]; [e-mail]; linux-arm-
>>>>>>> [e-mail]; [e-mail];
>>>>>>> [e-mail];
>>>>>>> [e-mail]; Cristi Sovaiala <[e-mail]>;
>>>>>>> Laurentiu
>>>>>>> Tudor <[e-mail]>; Ioana Ciornei
>>>>>>> <[e-mail]>;
>>>>>>> Varun Sethi <[e-mail]>; Calvin Johnson
>>>>>>> <[e-mail]>;
>>>>>>> Pankaj Bansal <[e-mail]>; [e-mail];
>>>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>>>> [e-mail]; [e-mail]
>>>>>>> Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
>>>>>>>
>>>>>>> Caution: EXT Email
>>>>>>>
>>>>>>> On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
>>>>>>>> ACPI support is added in the fsl-mc driver. Driver will parse MC DSDT
>>>>>>>> table to extract memory and other resorces.
>>>>>>>>
>>>>>>>> Interrupt (GIC ITS) information will be extracted from MADT table by
>>>>>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
>>>>>>>>
>>>>>>>> IORT table will be parsed to configure DMA.
>>>>>>>>
>>>>>>>> Signed-off-by: Makarand Pawagi <[e-mail]>
>>>>>>>> ---
>>>>>>>>    drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
>>>>>>>>    drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
>>>>>>>>    drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
>>>>>>>>    drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
>>>>>>>>    drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
>>>>>>>>    drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
>>>>>>> ++++++++++++++++++++++++++++-
>>>>>>>>    include/linux/acpi_iort.h                   |  5 ++
>>>>>>>>    7 files changed, 174 insertions(+), 20 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>>>>>> index 33f7198..beb9cd5 100644
>>>>>>>> --- a/drivers/acpi/arm64/iort.c
>>>>>>>> +++ b/drivers/acpi/arm64/iort.c
>>>>>>>> @@ -15,6 +15,7 @@
>>>>>>>>    #include <linux/kernel.h>
>>>>>>>>    #include <linux/list.h>
>>>>>>>>    #include <linux/pci.h>
>>>>>>>> +#include <linux/fsl/mc.h>
>>>>>>>>    #include <linux/platform_device.h>
>>>>>>>>    #include <linux/slab.h>
>>>>>>>>
>>>>>>>> @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct device
>>>>>>>> *dev, u32 req_id,  }
>>>>>>>>
>>>>>>>>    /**
>>>>>>>> + * iort_get_fsl_mc_device_domain() - Find MSI domain related to a
>>>>>>>> +device
>>>>>>>> + * @dev: The device.
>>>>>>>> + * @mc_icid: ICID for the fsl_mc device.
>>>>>>>> + *
>>>>>>>> + * Returns: the MSI domain for this device, NULL otherwise  */ struct
>>>>>>>> +irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
>>>>>>>> +                                                     u32 mc_icid) {
>>>>>>>> +     struct fwnode_handle *handle;
>>>>>>>> +     int its_id;
>>>>>>>> +
>>>>>>>> +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
>>>>>>>> +             return NULL;
>>>>>>>> +
>>>>>>>> +     handle = iort_find_domain_token(its_id);
>>>>>>>> +     if (!handle)
>>>>>>>> +             return NULL;
>>>>>>>> +
>>>>>>>> +     return irq_find_matching_fwnode(handle, DOMAIN_BUS_FSL_MC_MSI);
>>>>>>>> +}
>>>>>>>
>>>>>>> NAK
>>>>>>>
>>>>>>> I am not willing to take platform specific code in the generic IORT
>>>>>>> layer.
>>>>>>>
>>>>>>> ACPI on ARM64 works on platforms that comply with SBSA/SBBR
>>>>>>> guidelines:
>>>>>>>
>>>>>>>
>>>>>>> https://developer.arm.com/architectures/platform-design/server-systems
>>>>>>>
>>>>>>> Deviating from those requires butchering ACPI specifications (ie IORT)
>>>>>>> and
>>>>>>> related kernel code which goes totally against what ACPI is meant for
>>>>>>> on ARM64
>>>>>>> systems, so there is no upstream pathway for this code I am afraid.
>>>>>>>
>>>>>> Reason of adding this platform specific function in the generic IORT
>>>>>> layer is
>>>>>> That iort_get_device_domain() only deals with PCI bus
>>>>>> (DOMAIN_BUS_PCI_MSI).
>>>>>>
>>>>>> fsl-mc objects when probed, need to find irq_domain which is associated
>>>>>> with
>>>>>> the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will not be possible to do
>>>>>> that
>>>>>> if we do not add this function because there are no other suitable APIs
>>>>>> exported
>>>>>> by IORT layer to do the job.
>>>>>
>>>>> I think we all understood the patch. What both Lorenzo and myself are
>>>>> saying is
>>>>> that we do not want non-PCI support in IORT.
>>>>>
>>>>
>>>> IORT supports platform devices (aka named components) as well, and
>>>> there is some support for platform MSIs in the GIC layer.
>>>>
>>>> So it may be possible to hide your exotic bus from the OS entirely,
>>>> and make the firmware instantiate a DSDT with device objects and
>>>> associated IORT nodes that describe whatever lives on that bus as
>>>> named components.
>>>>
>>>> That way, you will not have to change the OS at all, so your hardware
>>>> will not only be supported in linux v5.7+, it will also be supported
>>>> by OSes that commercial distro vendors are shipping today. *That* is
>>>> the whole point of using ACPI.
>>>>
>>>> If you are going to bother and modify the OS, you lose this advantage,
>>>> and ACPI gives you no benefit over DT at all.
>>>
>>> You beat me to it, but thanks for the clarification Ard.  No where in
>>> the SBSA spec that I have read does it state that only PCIe devices
>>> are supported by the SMMU.  It uses PCIe devices as an example, but
>>> the SMMU section is very generic in term and only says "devices".
>>>
>>> I feel the SBSA omission of SerDes best practices is an oversight in
>>> the standard and something that probably needs to be revisited.
>>> Forcing high speed networking interfaces to be hung off a bus just for
>>> the sake of having a "standard" PCIe interface seems like a step
>>> backward in this regard.  I would much rather have the Spec include a
>>> common standard that could be exposed in a consistent manner.  But
>>> this is a conversation for a different place.
>>
>> Just to clarify further, it's not about serdes or high-speed networking
>> per se - describing a fixed-function network adapter as a named
>> component is entirely within scope. The issue is when the hardware is
>> merely a pool of accelerator components that can be dynamically
>> configured at runtime into something that looks like one or more
>> 'virtual' network adapters - there is no standard interface for *that*
>> for SBSA to consider.
>>
>> Robin.
>>
>>>
>>> I will work with NXP and find a better way to implement this.
>>>
>>> -Jon
>>>
> 
> But by design SFP, SFP+, and QSFP cages are not fixed function network
> adapters.  They are physical and logical devices that can adapt to
> what is plugged into them.  How the devices are exposed should be
> irrelevant to this conversation it is about the underlying
> connectivity.

Apologies - I was under the impression that SFP and friends were a 
physical-layer thing and that a MAC in the SoC would still be fixed such 
that its DMA and interrupt configuration could be statically described 
regardless of what transceiver was plugged in (even if some 
configurations might not use every interrupt/stream ID/etc.) If that 
isn't the case I shall go and educate myself further.

>  For instance if this were an accelerator block on a
> PCIe card then we wouldn't be having this discussion, even if it did
> run a firmware and have a third party driver that exposed virtual
> network interfaces.

Right, because in that case the interrupts and DMA have to travel 
through the PCIe layer, and thus generic code only needs to worry about 
things from the point of the PCI host bridge. That's rather the point of 
having an industry-standard interface.

Robin.

Andrew Lunn Jan. 31, 2020, 2:29 p.m. UTC | #12

> > But by design SFP, SFP+, and QSFP cages are not fixed function network
> > adapters.  They are physical and logical devices that can adapt to
> > what is plugged into them.  How the devices are exposed should be
> > irrelevant to this conversation it is about the underlying
> > connectivity.
> 
> Apologies - I was under the impression that SFP and friends were a
> physical-layer thing and that a MAC in the SoC would still be fixed such
> that its DMA and interrupt configuration could be statically described
> regardless of what transceiver was plugged in (even if some configurations
> might not use every interrupt/stream ID/etc.) If that isn't the case I shall
> go and educate myself further.

Hi Robin

It gets interesting with QSFP cages. The Q is quad, there are 4 SERDES
lanes. You can use them for 1x 40G link, or you can split them into 4x
10G links. So you either need one MAC or 4 MACs connecting to the
cage, and this can change on the fly when a modules is ejected and
replaced with another module. There are only one set of control pins
for i2c, loss of signal, TX disable, module inserted. So where the
interrupt/stream ID/etc are mapped needs some flexibility.

There is also to some degree a conflict with hiding all this inside
firmware. This is complex stuff. It is much better to have one core
implementing in Linux plus some per hardware driver support, than
having X firmware blobs, generally closed source, each with there own
bugs which nobody can fix.

     Andrew

Will Deacon Jan. 31, 2020, 2:47 p.m. UTC | #13

On Fri, Jan 31, 2020 at 03:29:06PM +0100, Andrew Lunn wrote:
> > > But by design SFP, SFP+, and QSFP cages are not fixed function network
> > > adapters.  They are physical and logical devices that can adapt to
> > > what is plugged into them.  How the devices are exposed should be
> > > irrelevant to this conversation it is about the underlying
> > > connectivity.
> > 
> > Apologies - I was under the impression that SFP and friends were a
> > physical-layer thing and that a MAC in the SoC would still be fixed such
> > that its DMA and interrupt configuration could be statically described
> > regardless of what transceiver was plugged in (even if some configurations
> > might not use every interrupt/stream ID/etc.) If that isn't the case I shall
> > go and educate myself further.
> 
> It gets interesting with QSFP cages. The Q is quad, there are 4 SERDES
> lanes. You can use them for 1x 40G link, or you can split them into 4x
> 10G links. So you either need one MAC or 4 MACs connecting to the
> cage, and this can change on the fly when a modules is ejected and
> replaced with another module. There are only one set of control pins
> for i2c, loss of signal, TX disable, module inserted. So where the
> interrupt/stream ID/etc are mapped needs some flexibility.
> 
> There is also to some degree a conflict with hiding all this inside
> firmware. This is complex stuff. It is much better to have one core
> implementing in Linux plus some per hardware driver support, than
> having X firmware blobs, generally closed source, each with there own
> bugs which nobody can fix.

Devicetree to the rescue!

Entertaining the use of ACPI without any firmware abstraction for this
hardware really feels like a square peg / round hole situation, so I'm
assuming somebody's telling you that you need it "FOAR ENTAPRYZE". Who
is it and can you tell them to bog off?

Will

Andrew Lunn Jan. 31, 2020, 3:09 p.m. UTC | #14

> Devicetree to the rescue!

Yes, exactly. We have good, standardised descriptions for most of this
in device tree. And phylink can handle SFP and SFP+. Nobody has worked
on QSFP yet, since phylink has mostly been pushed by the embedded
world and 40G is not yet popular in the embedded world.

> Entertaining the use of ACPI without any firmware abstraction for this
> hardware really feels like a square peg / round hole situation, so I'm
> assuming somebody's telling you that you need it "FOAR ENTAPRYZE". Who
> is it and can you tell them to bog off?

The issues here is that SFPs are appearing in more and more server
systems, replacing plain old copper Ethernet. If the boxes use off the
shelf Mellanox or Intel PCIe cards, it is not an issue. But silicon
vendors are integrating this into the SoC in the ARM way of doing
things, memory mapped, spread over a number of controllers, not a
single PCIe device.

Maybe we need hybrid systems. Plain, old, simple, boring things like
CPUs, serial ports, SATA, PCIe busses are described in ACPI. Complex
interesting things are in DT. The hard thing is the interface between
the two. DT having a phandle to an ACPI object, e.g a GPIO, interrupt
or an i2c bus.

   Andrew

Jon Nettleton Jan. 31, 2020, 3:14 p.m. UTC | #15

On Fri, Jan 31, 2020 at 4:09 PM Andrew Lunn <[e-mail]> wrote:
>
> > Devicetree to the rescue!
>
> Yes, exactly. We have good, standardised descriptions for most of this
> in device tree. And phylink can handle SFP and SFP+. Nobody has worked
> on QSFP yet, since phylink has mostly been pushed by the embedded
> world and 40G is not yet popular in the embedded world.
>
> > Entertaining the use of ACPI without any firmware abstraction for this
> > hardware really feels like a square peg / round hole situation, so I'm
> > assuming somebody's telling you that you need it "FOAR ENTAPRYZE". Who
> > is it and can you tell them to bog off?
>
> The issues here is that SFPs are appearing in more and more server
> systems, replacing plain old copper Ethernet. If the boxes use off the
> shelf Mellanox or Intel PCIe cards, it is not an issue. But silicon
> vendors are integrating this into the SoC in the ARM way of doing
> things, memory mapped, spread over a number of controllers, not a
> single PCIe device.
>
> Maybe we need hybrid systems. Plain, old, simple, boring things like
> CPUs, serial ports, SATA, PCIe busses are described in ACPI. Complex
> interesting things are in DT. The hard thing is the interface between
> the two. DT having a phandle to an ACPI object, e.g a GPIO, interrupt
> or an i2c bus.
>
>    Andrew

Just as a review reference, I have found an old attempted
implementation documented here.
https://github.com/CumulusNetworks/apd-tools/wiki

-Jon

Russell King (Oracle) Jan. 31, 2020, 3:15 p.m. UTC | #16

On Fri, Jan 31, 2020 at 03:29:06PM +0100, Andrew Lunn wrote:
> > > But by design SFP, SFP+, and QSFP cages are not fixed function network
> > > adapters.  They are physical and logical devices that can adapt to
> > > what is plugged into them.  How the devices are exposed should be
> > > irrelevant to this conversation it is about the underlying
> > > connectivity.
> > 
> > Apologies - I was under the impression that SFP and friends were a
> > physical-layer thing and that a MAC in the SoC would still be fixed such
> > that its DMA and interrupt configuration could be statically described
> > regardless of what transceiver was plugged in (even if some configurations
> > might not use every interrupt/stream ID/etc.) If that isn't the case I shall
> > go and educate myself further.
> 
> Hi Robin
> 
> It gets interesting with QSFP cages. The Q is quad, there are 4 SERDES
> lanes. You can use them for 1x 40G link, or you can split them into 4x
> 10G links. So you either need one MAC or 4 MACs connecting to the
> cage, and this can change on the fly when a modules is ejected and
> replaced with another module.

I think it's even more complicated than that.  If you have a QSFP+
fiber module, that can be connected to four fibers which can either
go to another QSFP+ module, or four separate SFP+ modules.

That means it's a manual configuration decision whether to operate
the QSFP+ module as a single 40G link, or as four separate 10G links.

> There are only one set of control pins
> for i2c, loss of signal, TX disable, module inserted. So where the
> interrupt/stream ID/etc are mapped needs some flexibility.

QSFP changes the way the modules are controlled; gone are many of the
hardware signals, replaced by registers in the I2C space.  The
remaining hardware signals are:

ModSelL	module select (to enable the I2C bus)
ResetL	module reset
SCL/SDA	I2C bus
ModPrsL	module present
IntL	interrupt (but not too useful from what I can see!)
LPMode	low power mode (can be overriden via the I2C bus)

> There is also to some degree a conflict with hiding all this inside
> firmware. This is complex stuff. It is much better to have one core
> implementing in Linux plus some per hardware driver support, than
> having X firmware blobs, generally closed source, each with there own
> bugs which nobody can fix.

QSFP and SFP support is not really part of the DPAA2 firmware.

I have some prototype implementation for driving the QSFP+ cage, but
I haven't yet worked out how to sensible deal with the "is it 4x 10G
or 1x 40G" issue you mention above, and how to interface the QSFP+
driver sensibly with one or four network drivers.

I've been concentrating more on the SFP/SFP+ problem on the Honeycomb
board which is what most people will have, working out how to sensibly
drive the hardware so that our existing SFP support in the kernel can
work sensibly.  In the last couple of days, I've managed to get
something together which works, switching between 1000base-X and SGMII
on this hardware, using some of the patches I've already pointed to
over the last few weeks.  This hardware falls into the "split PCS and
MAC" problem space, so it's relevent to many people - and it's
important that we don't rush into a solution that works for one
implementation and not everyone.  This is why I haven't responded to
Jose's proposal - I'm still working out what is required for others,
but what I can say is that it isn't what Jose has proposed.  I had
asked Jose to hold off, but he's understandably eager to solve the
problem in front of him at the expense of everyone else.

What I've found is that any attempt to split the current
"phylink_mac_ops" interface between the PCS and MAC blocks results, as
I suspected, in mvneta and mvpp2 suffering very badly; the hardware
does not split along those functional blocks at all well.

My current state of play for this is in my "cex7" branch, pushed out
earlier today.  It's a bit hacky right now, and there's various issues
that need to be solved, but it is functional with the right board boot
configuration (basically the DPC file, which is one of the configs for
the MC firmware.)

I'm planning to look at what's required for the faster speeds; there's
other PCS PHYs on this platform that support the other speeds (10G, 25G,
40G, 100G) accessed via Clause 45 cycles.

As for the DSA issue you've raised with DSA links, I don't see any
obvious solution for that - the whole "if no fixed-link is specified,
default to the highest speed" is a real problem; the conversion of DSA
to phylink for the CPU and DSA ports did not take account of that.
phylink has _zero_ information in that case to know how the link should
be configured - there is no PHY, there is no fixed-link specification,
there is absolutely nothing.  So it's no surprise when phylink tries to
configure speed=0 duplex=half pause=off on these interfaces when they're
brought up.  I notice that this work was contributed by NXP - and in my
mind illustrates that they did not think about what they were doing
there either.  They certainly never ran phylink with debugging on and
considered whether the phylink_mac_config() calls contained sensible
information.  Did they even have all the information necessary to work
out what was required - I doubt it very much.  Did they realise that the
fixed-link specification was optional, did they realise that there
could be a PHY on these links, and did they consider what the behaviour
would be in those cases?  And now we have something of a headache
trying to work out how to solve this - one thing is certain, whatever
the fix is, it isn't going to be nice to be backported to stable trees.

Russell King (Oracle) Jan. 31, 2020, 3:39 p.m. UTC | #17

On Fri, Jan 31, 2020 at 04:09:29PM +0100, Andrew Lunn wrote:
> > Devicetree to the rescue!
> 
> Yes, exactly. We have good, standardised descriptions for most of this
> in device tree. And phylink can handle SFP and SFP+. Nobody has worked
> on QSFP yet, since phylink has mostly been pushed by the embedded
> world and 40G is not yet popular in the embedded world.

That's incorrect (if you read my previous reply.)  It shouldn't come
as any surprise that I have some experimental QSFP code.

Jakub Kicinski Jan. 31, 2020, 3:40 p.m. UTC | #18

On Fri, 31 Jan 2020 15:15:00 +0000, Russell King - ARM Linux admin
wrote:
> I have some prototype implementation for driving the QSFP+ cage, but
> I haven't yet worked out how to sensible deal with the "is it 4x 10G
> or 1x 40G" issue you mention above, and how to interface the QSFP+
> driver sensibly with one or four network drivers.

I'm pretty sure you know this but just FWIW - vendors who do it in FW
write the current config down in NVM so it doesn't get affected by
reboots and use devlink port splitting to change it.

Andrew Lunn Jan. 31, 2020, 3:41 p.m. UTC | #19

> Just as a review reference, I have found an old attempted
> implementation documented here.
> https://github.com/CumulusNetworks/apd-tools/wiki

That is interesting, but you need to be careful with it.

Top of rack switches are very different beasts to the majority of
switches supported by Linux, which are SOHO. TOR switches have a lot
of firmware running on them doing most of the work. A typical SOHO
switch with a Linux kernel driver has very little firmware, Linux
really is driving it, not just asking it to do things.

Also, TOR switches typically have a binary blob running in userspace,
meaning different information is probably required compared to kernel
driver.

There may be useful information here, but please take it with a big
pinch of salt. And make sure you have the appropriate networking
people involved in any specification work you do.

      Andrew

Russell King (Oracle) Feb. 1, 2020, 11:49 a.m. UTC | #20

On Fri, Jan 31, 2020 at 07:40:50AM -0800, Jakub Kicinski wrote:
> On Fri, 31 Jan 2020 15:15:00 +0000, Russell King - ARM Linux admin
> wrote:
> > I have some prototype implementation for driving the QSFP+ cage, but
> > I haven't yet worked out how to sensible deal with the "is it 4x 10G
> > or 1x 40G" issue you mention above, and how to interface the QSFP+
> > driver sensibly with one or four network drivers.
> 
> I'm pretty sure you know this but just FWIW - vendors who do it in FW
> write the current config down in NVM so it doesn't get affected by
> reboots and use devlink port splitting to change it.

+Jiri

I wasn't aware of devlink port splitting, so thanks for that. However,
it could do with some better documentation - there's nothing on it
afaics in the Documentation subdirectory, and "man devlink-port"
doesn't give much away either:

   devlink port split - split devlink port into more
       DEV/PORT_INDEX - specifies the devlink port to operate on.

       count COUNT
              number of ports to split to.

It's the "into more" that's not clear - presumably more ports, and
presumably each port is a network device, but this isn't explained.

I think what this is trying to say is that, if we have a QSFP+ cage
with 4 serdes lines running at 10G, and they are initially treated as
a single 40G ethernet:

	devlink port split device/1 count 4

will then give us four 10G network devices, and:

	devlink port unsplit device/1

will recombine them back to a single 40G network device.

What if someone decides to do:

	devlink port split device/1 count 2

what do we end up with?  Presumably two network devices running with
two serdes lanes each (if supported by the hardware).  At that point
can they then do:

	devlink port split device/2 count 2

and end up with one network device with two 10G serdes lanes, and two
network devices each with one 10G serdes lane, or can port splitting
only be used on the "master" device/port ?

Unfortunately, I don't think I have any network devices that support
this so I can't experiment to find out how this should work; yes, I
have a Mellanox card, but it supports a single 10G SFP+, and therefore
does not support port splitting.

Jakub Kicinski Feb. 1, 2020, 5:36 p.m. UTC | #21

On Sat, 1 Feb 2020 11:49:19 +0000, Russell King - ARM Linux admin wrote:
> What if someone decides to do:
> 
> 	devlink port split device/1 count 2
> 
> what do we end up with?  Presumably two network devices running with
> two serdes lanes each (if supported by the hardware).  At that point
> can they then do:
> 
> 	devlink port split device/2 count 2
> 
> and end up with one network device with two 10G serdes lanes, and two
> network devices each with one 10G serdes lane, 

I think all your guesses are correct, it's a pretty straight forward
API, but it's also pretty thin, and some of the logic is in FW, so
there isn't much in a way of a standard on how things should behave :S

> or can port splitting only be used on the "master" device/port ?

I think both mlxsw and the NFP rejects re-split/further splitting.
Ports have to be unsplit first. So there is only one device for
splitting, and unsplitting can be done on any of the sub-devices.

> Unfortunately, I don't think I have any network devices that support
> this so I can't experiment to find out how this should work; yes, I
> have a Mellanox card, but it supports a single 10G SFP+, and therefore
> does not support port splitting.

I think you'd need a mlxsw or an nfp to play with this.

Maybe Jiri can clarify further :)

Pankaj Bansal Feb. 14, 2020, 3:05 p.m. UTC | #22

> -----Original Message-----
> From: Ard Biesheuvel <[e-mail]>
> Sent: Friday, January 31, 2020 5:32 PM
> To: Marc Zyngier <[e-mail]>
> Cc: Makarand Pawagi <[e-mail]>; Calvin Johnson
> <[e-mail]>; [e-mail]; [e-mail];
> Ioana Ciornei <[e-mail]>; Cristi Sovaiala
> <[e-mail]>; Hanjun Guo <[e-mail]>; Will
> Deacon <[e-mail]>; Lorenzo Pieralisi <[e-mail]>;
> Pankaj Bansal <[e-mail]>; [e-mail]; Russell King
> <[e-mail]>; ACPI Devel Maling List <[e-mail]>;
> Len Brown <[e-mail]>; Jason Cooper <[e-mail]>; Andy
> Wang <[e-mail]>; Varun Sethi <[e-mail]>; Thomas
> Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
> [e-mail]>; Laurentiu Tudor <[e-mail]>; Paul
> Yang <[e-mail]>; <[e-mail]>
> <[e-mail]>; Rafael J. Wysocki <[e-mail]>; Linux Kernel
> Mailing List <[e-mail]>; Shameerali Kolothum Thodi
> <[e-mail]>; Sudeep Holla
> <[e-mail]>; Robin Murphy <[e-mail]>
> Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> 
> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <[e-mail]> wrote:
> >
> > On 2020-01-31 10:35, Makarand Pawagi wrote:
> > >> -----Original Message-----
> > >> From: Lorenzo Pieralisi <[e-mail]>
> > >> Sent: Tuesday, January 28, 2020 4:39 PM
> > >> To: Makarand Pawagi <[e-mail]>
> > >> Cc: [e-mail]; [e-mail];
> > >> linux-arm- [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail]; Cristi Sovaiala
> > >> <[e-mail]>; Laurentiu Tudor
> > >> <[e-mail]>; Ioana Ciornei <[e-mail]>;
> > >> Varun Sethi <[e-mail]>; Calvin Johnson
> > >> <[e-mail]>; Pankaj Bansal <[e-mail]>;
> > >> [e-mail]; [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail]
> > >> Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> > >>
> > >> Caution: EXT Email
> > >>
> > >> On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
> > >> > ACPI support is added in the fsl-mc driver. Driver will parse MC
> > >> > DSDT table to extract memory and other resorces.
> > >> >
> > >> > Interrupt (GIC ITS) information will be extracted from MADT table
> > >> > by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> > >> >
> > >> > IORT table will be parsed to configure DMA.
> > >> >
> > >> > Signed-off-by: Makarand Pawagi <[e-mail]>
> > >> > ---
> > >> >  drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
> > >> >  drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
> > >> >  drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
> > >> >  drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
> > >> >  drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
> > >> >  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
> > >> ++++++++++++++++++++++++++++-
> > >> >  include/linux/acpi_iort.h                   |  5 ++
> > >> >  7 files changed, 174 insertions(+), 20 deletions(-)
> > >> >
> > >> > diff --git a/drivers/acpi/arm64/iort.c
> > >> > b/drivers/acpi/arm64/iort.c index 33f7198..beb9cd5 100644
> > >> > --- a/drivers/acpi/arm64/iort.c
> > >> > +++ b/drivers/acpi/arm64/iort.c
> > >> > @@ -15,6 +15,7 @@
> > >> >  #include <linux/kernel.h>
> > >> >  #include <linux/list.h>
> > >> >  #include <linux/pci.h>
> > >> > +#include <linux/fsl/mc.h>
> > >> >  #include <linux/platform_device.h>  #include <linux/slab.h>
> > >> >
> > >> > @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct
> > >> > device *dev, u32 req_id,  }
> > >> >
> > >> >  /**
> > >> > + * iort_get_fsl_mc_device_domain() - Find MSI domain related to
> > >> > +a device
> > >> > + * @dev: The device.
> > >> > + * @mc_icid: ICID for the fsl_mc device.
> > >> > + *
> > >> > + * Returns: the MSI domain for this device, NULL otherwise  */
> > >> > +struct irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
> > >> > +                                                     u32 mc_icid) {
> > >> > +     struct fwnode_handle *handle;
> > >> > +     int its_id;
> > >> > +
> > >> > +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
> > >> > +             return NULL;
> > >> > +
> > >> > +     handle = iort_find_domain_token(its_id);
> > >> > +     if (!handle)
> > >> > +             return NULL;
> > >> > +
> > >> > +     return irq_find_matching_fwnode(handle,
> > >> > +DOMAIN_BUS_FSL_MC_MSI); }
> > >>
> > >> NAK
> > >>
> > >> I am not willing to take platform specific code in the generic IORT
> > >> layer.
> > >>
> > >> ACPI on ARM64 works on platforms that comply with SBSA/SBBR
> > >> guidelines:
> > >>
> > >>
> > >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fd
> > >> eveloper.arm.com%2Farchitectures%2Fplatform-design%2Fserver-systems
> > >>
> &amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cdb56d889d85646277ee
> 30
> > >>
> 8d7a64562fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6371606
> 892
> > >>
> 50769265&amp;sdata=C7nCty8%2BVeuq6VhcEUXCwiAinN01rCfe12NRVnXJCIY%
> 3D
> > >> &amp;reserved=0
> > >>
> > >> Deviating from those requires butchering ACPI specifications (ie
> > >> IORT) and related kernel code which goes totally against what ACPI
> > >> is meant for on ARM64 systems, so there is no upstream pathway for
> > >> this code I am afraid.
> > >>
> > > Reason of adding this platform specific function in the generic IORT
> > > layer is That iort_get_device_domain() only deals with PCI bus
> > > (DOMAIN_BUS_PCI_MSI).
> > >
> > > fsl-mc objects when probed, need to find irq_domain which is
> > > associated with the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will not
> > > be possible to do that if we do not add this function because there
> > > are no other suitable APIs exported by IORT layer to do the job.
> >
> > I think we all understood the patch. What both Lorenzo and myself are
> > saying is that we do not want non-PCI support in IORT.
> >
> 
> IORT supports platform devices (aka named components) as well, and
> there is some support for platform MSIs in the GIC layer.
> 
> So it may be possible to hide your exotic bus from the OS entirely,
> and make the firmware instantiate a DSDT with device objects and
> associated IORT nodes that describe whatever lives on that bus as
> named components.
> 
> That way, you will not have to change the OS at all, so your hardware
> will not only be supported in linux v5.7+, it will also be supported
> by OSes that commercial distro vendors are shipping today. *That* is
> the whole point of using ACPI.
> 
> If you are going to bother and modify the OS, you lose this advantage,
> and ACPI gives you no benefit over DT at all.

I am replying to old message in this conversation, because the discussion got sidetracked from IORT
table to SFP/QSFP/devlink stuff from this point onwards, which is not related to IORT.
I will only focus on representing the MC device in IORT and using the same in linux.
As Ard said:
"IORT supports platform devices (aka named components) as well, and
there is some support for platform MSIs in the GIC layer."

We can represent MC bus as named component in IORT table and use platform MSIs.
The only caveat is that with current implementation of platform MSIs, the Input id of a device is not considered.
https://elixir.bootlin.com/linux/latest/source/drivers/irqchip/irq-gic-v3-its-platform-msi.c#L50
https://elixir.bootlin.com/linux/latest/source/drivers/acpi/arm64/iort.c#L464

While, IORT spec doesn't specify any such limitation.

we can easily update iort.c to remove this limitation.
But, I am not sure how the input id would be passed from platform MSI GIC layer to IORT.
Most obviously, the input id should be supplied by dev itself.

Any thoughts?

If we go by this path, I think we can remove special handling for fsl-mc for of* cases altogether.
https://elixir.bootlin.com/linux/latest/source/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c

Regards,
Pankaj Bansal

Marc Zyngier Feb. 14, 2020, 3:54 p.m. UTC | #23

On 2020-02-14 15:05, Pankaj Bansal wrote:
>> -----Original Message-----
>> From: Ard Biesheuvel <[e-mail]>
>> Sent: Friday, January 31, 2020 5:32 PM
>> To: Marc Zyngier <[e-mail]>
>> Cc: Makarand Pawagi <[e-mail]>; Calvin Johnson
>> <[e-mail]>; [e-mail]; [e-mail];
>> Ioana Ciornei <[e-mail]>; Cristi Sovaiala
>> <[e-mail]>; Hanjun Guo <[e-mail]>; Will
>> Deacon <[e-mail]>; Lorenzo Pieralisi 
>> <[e-mail]>;
>> Pankaj Bansal <[e-mail]>; [e-mail]; Russell King
>> <[e-mail]>; ACPI Devel Maling List 
>> <[e-mail]>;
>> Len Brown <[e-mail]>; Jason Cooper <[e-mail]>; Andy
>> Wang <[e-mail]>; Varun Sethi <[e-mail]>; Thomas
>> Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
>> [e-mail]>; Laurentiu Tudor 
>> <[e-mail]>; Paul
>> Yang <[e-mail]>; <[e-mail]>
>> <[e-mail]>; Rafael J. Wysocki <[e-mail]>; Linux 
>> Kernel
>> Mailing List <[e-mail]>; Shameerali Kolothum Thodi
>> <[e-mail]>; Sudeep Holla
>> <[e-mail]>; Robin Murphy <[e-mail]>
>> Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for 
>> fsl-mc
>> 
>> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <[e-mail]> wrote:
>> >
>> > On 2020-01-31 10:35, Makarand Pawagi wrote:
>> > >> -----Original Message-----
>> > >> From: Lorenzo Pieralisi <[e-mail]>
>> > >> Sent: Tuesday, January 28, 2020 4:39 PM
>> > >> To: Makarand Pawagi <[e-mail]>
>> > >> Cc: [e-mail]; [e-mail];
>> > >> linux-arm- [e-mail]; [e-mail];
>> > >> [e-mail]; [e-mail]; Cristi Sovaiala
>> > >> <[e-mail]>; Laurentiu Tudor
>> > >> <[e-mail]>; Ioana Ciornei <[e-mail]>;
>> > >> Varun Sethi <[e-mail]>; Calvin Johnson
>> > >> <[e-mail]>; Pankaj Bansal <[e-mail]>;
>> > >> [e-mail]; [e-mail]; [e-mail];
>> > >> [e-mail]; [e-mail]; [e-mail];
>> > >> [e-mail]; [e-mail];
>> > >> [e-mail]; [e-mail];
>> > >> [e-mail]; [e-mail]
>> > >> Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
>> > >>
>> > >> Caution: EXT Email
>> > >>
>> > >> On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
>> > >> > ACPI support is added in the fsl-mc driver. Driver will parse MC
>> > >> > DSDT table to extract memory and other resorces.
>> > >> >
>> > >> > Interrupt (GIC ITS) information will be extracted from MADT table
>> > >> > by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
>> > >> >
>> > >> > IORT table will be parsed to configure DMA.
>> > >> >
>> > >> > Signed-off-by: Makarand Pawagi <[e-mail]>
>> > >> > ---
>> > >> >  drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
>> > >> >  drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
>> > >> >  drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
>> > >> >  drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
>> > >> >  drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
>> > >> >  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
>> > >> ++++++++++++++++++++++++++++-
>> > >> >  include/linux/acpi_iort.h                   |  5 ++
>> > >> >  7 files changed, 174 insertions(+), 20 deletions(-)
>> > >> >
>> > >> > diff --git a/drivers/acpi/arm64/iort.c
>> > >> > b/drivers/acpi/arm64/iort.c index 33f7198..beb9cd5 100644
>> > >> > --- a/drivers/acpi/arm64/iort.c
>> > >> > +++ b/drivers/acpi/arm64/iort.c
>> > >> > @@ -15,6 +15,7 @@
>> > >> >  #include <linux/kernel.h>
>> > >> >  #include <linux/list.h>
>> > >> >  #include <linux/pci.h>
>> > >> > +#include <linux/fsl/mc.h>
>> > >> >  #include <linux/platform_device.h>  #include <linux/slab.h>
>> > >> >
>> > >> > @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct
>> > >> > device *dev, u32 req_id,  }
>> > >> >
>> > >> >  /**
>> > >> > + * iort_get_fsl_mc_device_domain() - Find MSI domain related to
>> > >> > +a device
>> > >> > + * @dev: The device.
>> > >> > + * @mc_icid: ICID for the fsl_mc device.
>> > >> > + *
>> > >> > + * Returns: the MSI domain for this device, NULL otherwise  */
>> > >> > +struct irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
>> > >> > +                                                     u32 mc_icid) {
>> > >> > +     struct fwnode_handle *handle;
>> > >> > +     int its_id;
>> > >> > +
>> > >> > +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
>> > >> > +             return NULL;
>> > >> > +
>> > >> > +     handle = iort_find_domain_token(its_id);
>> > >> > +     if (!handle)
>> > >> > +             return NULL;
>> > >> > +
>> > >> > +     return irq_find_matching_fwnode(handle,
>> > >> > +DOMAIN_BUS_FSL_MC_MSI); }
>> > >>
>> > >> NAK
>> > >>
>> > >> I am not willing to take platform specific code in the generic IORT
>> > >> layer.
>> > >>
>> > >> ACPI on ARM64 works on platforms that comply with SBSA/SBBR
>> > >> guidelines:
>> > >>
>> > >>
>> > >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fd
>> > >> eveloper.arm.com%2Farchitectures%2Fplatform-design%2Fserver-systems
>> > >>
>> &amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cdb56d889d85646277ee
>> 30
>> > >>
>> 8d7a64562fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6371606
>> 892
>> > >>
>> 50769265&amp;sdata=C7nCty8%2BVeuq6VhcEUXCwiAinN01rCfe12NRVnXJCIY%
>> 3D
>> > >> &amp;reserved=0
>> > >>
>> > >> Deviating from those requires butchering ACPI specifications (ie
>> > >> IORT) and related kernel code which goes totally against what ACPI
>> > >> is meant for on ARM64 systems, so there is no upstream pathway for
>> > >> this code I am afraid.
>> > >>
>> > > Reason of adding this platform specific function in the generic IORT
>> > > layer is That iort_get_device_domain() only deals with PCI bus
>> > > (DOMAIN_BUS_PCI_MSI).
>> > >
>> > > fsl-mc objects when probed, need to find irq_domain which is
>> > > associated with the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will not
>> > > be possible to do that if we do not add this function because there
>> > > are no other suitable APIs exported by IORT layer to do the job.
>> >
>> > I think we all understood the patch. What both Lorenzo and myself are
>> > saying is that we do not want non-PCI support in IORT.
>> >
>> 
>> IORT supports platform devices (aka named components) as well, and
>> there is some support for platform MSIs in the GIC layer.
>> 
>> So it may be possible to hide your exotic bus from the OS entirely,
>> and make the firmware instantiate a DSDT with device objects and
>> associated IORT nodes that describe whatever lives on that bus as
>> named components.
>> 
>> That way, you will not have to change the OS at all, so your hardware
>> will not only be supported in linux v5.7+, it will also be supported
>> by OSes that commercial distro vendors are shipping today. *That* is
>> the whole point of using ACPI.
>> 
>> If you are going to bother and modify the OS, you lose this advantage,
>> and ACPI gives you no benefit over DT at all.
> 
> I am replying to old message in this conversation, because the
> discussion got sidetracked from IORT
> table to SFP/QSFP/devlink stuff from this point onwards, which is not
> related to IORT.
> I will only focus on representing the MC device in IORT and using the
> same in linux.
> As Ard said:
> "IORT supports platform devices (aka named components) as well, and
> there is some support for platform MSIs in the GIC layer."
> 
> We can represent MC bus as named component in IORT table and use 
> platform MSIs.
> The only caveat is that with current implementation of platform MSIs,
> the Input id of a device is not considered.
> https://elixir.bootlin.com/linux/latest/source/drivers/irqchip/irq-gic-v3-its-platform-msi.c#L50
> https://elixir.bootlin.com/linux/latest/source/drivers/acpi/arm64/iort.c#L464

I don't understand what you mean. Platform MSI using IORT uses the DevID
of end-points. How could the ITS could work without specifying a DevID?
See for example how the SMMUv3 driver uses platform MSI.

> While, IORT spec doesn't specify any such limitation.
> 
> we can easily update iort.c to remove this limitation.
> But, I am not sure how the input id would be passed from platform MSI
> GIC layer to IORT.
> Most obviously, the input id should be supplied by dev itself.

Why should the device know about its own ID? That's a bus/interconnect
thing. And nothing should be passed *to* IORT. IORT is the source.

> Any thoughts?

I think that in this thread, we have been fairly explicit about what our
train of though was.

         M.

Pankaj Bansal Feb. 14, 2020, 3:58 p.m. UTC | #24

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Friday, February 14, 2020 9:24 PM
> To: Pankaj Bansal <[e-mail]>
> Cc: Ard Biesheuvel <[e-mail]>; Lorenzo Pieralisi
> <[e-mail]>; Makarand Pawagi <[e-mail]>;
> Calvin Johnson <[e-mail]>; [e-mail];
> [e-mail]; Ioana Ciornei <[e-mail]>; Cristi
> Sovaiala <[e-mail]>; Hanjun Guo <[e-mail]>;
> Will Deacon <[e-mail]>; [e-mail]; Russell King
> <[e-mail]>; ACPI Devel Maling List <[e-mail]>;
> Len Brown <[e-mail]>; Jason Cooper <[e-mail]>; Andy
> Wang <[e-mail]>; Varun Sethi <[e-mail]>; Thomas
> Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
> [e-mail]>; Laurentiu Tudor <[e-mail]>; Paul
> Yang <[e-mail]>; [e-mail]; Rafael J. Wysocki
> <[e-mail]>; Linux Kernel Mailing List <[e-mail]>;
> Shameerali Kolothum Thodi <[e-mail]>;
> Sudeep Holla <[e-mail]>; Robin Murphy
> <[e-mail]>
> Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> 
> On 2020-02-14 15:05, Pankaj Bansal wrote:
> >> -----Original Message-----
> >> From: Ard Biesheuvel <[e-mail]>
> >> Sent: Friday, January 31, 2020 5:32 PM
> >> To: Marc Zyngier <[e-mail]>
> >> Cc: Makarand Pawagi <[e-mail]>; Calvin Johnson
> >> <[e-mail]>; [e-mail]; [e-mail];
> >> Ioana Ciornei <[e-mail]>; Cristi Sovaiala
> >> <[e-mail]>; Hanjun Guo <[e-mail]>; Will
> >> Deacon <[e-mail]>; Lorenzo Pieralisi
> >> <[e-mail]>; Pankaj Bansal <[e-mail]>;
> >> [e-mail]; Russell King <[e-mail]>; ACPI Devel
> >> Maling List <[e-mail]>; Len Brown
> >> <[e-mail]>; Jason Cooper <[e-mail]>; Andy Wang
> >> <[e-mail]>; Varun Sethi <[e-mail]>; Thomas Gleixner
> >> <[e-mail]>; linux-arm-kernel <linux-arm-
> >> [e-mail]>; Laurentiu Tudor
> >> <[e-mail]>; Paul Yang <[e-mail]>;
> >> <[e-mail]> <[e-mail]>; Rafael J. Wysocki
> >> <[e-mail]>; Linux Kernel Mailing List
> >> <[e-mail]>; Shameerali Kolothum Thodi
> >> <[e-mail]>; Sudeep Holla
> >> <[e-mail]>; Robin Murphy <[e-mail]>
> >> Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for
> >> fsl-mc
> >>
> >> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <[e-mail]> wrote:
> >> >
> >> > On 2020-01-31 10:35, Makarand Pawagi wrote:
> >> > >> -----Original Message-----
> >> > >> From: Lorenzo Pieralisi <[e-mail]>
> >> > >> Sent: Tuesday, January 28, 2020 4:39 PM
> >> > >> To: Makarand Pawagi <[e-mail]>
> >> > >> Cc: [e-mail]; [e-mail];
> >> > >> linux-arm- [e-mail];
> >> > >> [e-mail]; [e-mail];
> >> > >> [e-mail]; Cristi Sovaiala <[e-mail]>;
> >> > >> Laurentiu Tudor <[e-mail]>; Ioana Ciornei
> >> > >> <[e-mail]>; Varun Sethi <[e-mail]>; Calvin
> >> > >> Johnson <[e-mail]>; Pankaj Bansal
> >> > >> <[e-mail]>; [e-mail];
> >> > >> [e-mail]; [e-mail]; [e-mail];
> >> > >> [e-mail]; [e-mail]; [e-mail];
> >> > >> [e-mail]; [e-mail];
> >> > >> [e-mail]; [e-mail]; [e-mail]
> >> > >> Subject: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for
> >> > >> fsl-mc
> >> > >>
> >> > >> Caution: EXT Email
> >> > >>
> >> > >> On Tue, Jan 28, 2020 at 01:38:45PM +0530, Makarand Pawagi wrote:
> >> > >> > ACPI support is added in the fsl-mc driver. Driver will parse
> >> > >> > MC DSDT table to extract memory and other resorces.
> >> > >> >
> >> > >> > Interrupt (GIC ITS) information will be extracted from MADT
> >> > >> > table by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> >> > >> >
> >> > >> > IORT table will be parsed to configure DMA.
> >> > >> >
> >> > >> > Signed-off-by: Makarand Pawagi <[e-mail]>
> >> > >> > ---
> >> > >> >  drivers/acpi/arm64/iort.c                   | 53 +++++++++++++++++++++
> >> > >> >  drivers/bus/fsl-mc/dprc-driver.c            |  3 +-
> >> > >> >  drivers/bus/fsl-mc/fsl-mc-bus.c             | 48 +++++++++++++------
> >> > >> >  drivers/bus/fsl-mc/fsl-mc-msi.c             | 10 +++-
> >> > >> >  drivers/bus/fsl-mc/fsl-mc-private.h         |  4 +-
> >> > >> >  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 71
> >> > >> ++++++++++++++++++++++++++++-
> >> > >> >  include/linux/acpi_iort.h                   |  5 ++
> >> > >> >  7 files changed, 174 insertions(+), 20 deletions(-)
> >> > >> >
> >> > >> > diff --git a/drivers/acpi/arm64/iort.c
> >> > >> > b/drivers/acpi/arm64/iort.c index 33f7198..beb9cd5 100644
> >> > >> > --- a/drivers/acpi/arm64/iort.c
> >> > >> > +++ b/drivers/acpi/arm64/iort.c
> >> > >> > @@ -15,6 +15,7 @@
> >> > >> >  #include <linux/kernel.h>
> >> > >> >  #include <linux/list.h>
> >> > >> >  #include <linux/pci.h>
> >> > >> > +#include <linux/fsl/mc.h>
> >> > >> >  #include <linux/platform_device.h>  #include <linux/slab.h>
> >> > >> >
> >> > >> > @@ -622,6 +623,29 @@ static int iort_dev_find_its_id(struct
> >> > >> > device *dev, u32 req_id,  }
> >> > >> >
> >> > >> >  /**
> >> > >> > + * iort_get_fsl_mc_device_domain() - Find MSI domain related
> >> > >> > +to a device
> >> > >> > + * @dev: The device.
> >> > >> > + * @mc_icid: ICID for the fsl_mc device.
> >> > >> > + *
> >> > >> > + * Returns: the MSI domain for this device, NULL otherwise
> >> > >> > +*/ struct irq_domain *iort_get_fsl_mc_device_domain(struct device
> *dev,
> >> > >> > +                                                     u32 mc_icid) {
> >> > >> > +     struct fwnode_handle *handle;
> >> > >> > +     int its_id;
> >> > >> > +
> >> > >> > +     if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
> >> > >> > +             return NULL;
> >> > >> > +
> >> > >> > +     handle = iort_find_domain_token(its_id);
> >> > >> > +     if (!handle)
> >> > >> > +             return NULL;
> >> > >> > +
> >> > >> > +     return irq_find_matching_fwnode(handle,
> >> > >> > +DOMAIN_BUS_FSL_MC_MSI); }
> >> > >>
> >> > >> NAK
> >> > >>
> >> > >> I am not willing to take platform specific code in the generic
> >> > >> IORT layer.
> >> > >>
> >> > >> ACPI on ARM64 works on platforms that comply with SBSA/SBBR
> >> > >> guidelines:
> >> > >>
> >> > >>
> >> > >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%
> >> > >> 2Fd
> >> > >> eveloper.arm.com%2Farchitectures%2Fplatform-design%2Fserver-syst
> >> > >> ems
> >> > >>
> >>
> &amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cdb56d889d85646277ee
> >> 30
> >> > >>
> >>
> 8d7a64562fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6371606
> >> 892
> >> > >>
> >>
> 50769265&amp;sdata=C7nCty8%2BVeuq6VhcEUXCwiAinN01rCfe12NRVnXJCIY%
> >> 3D
> >> > >> &amp;reserved=0
> >> > >>
> >> > >> Deviating from those requires butchering ACPI specifications (ie
> >> > >> IORT) and related kernel code which goes totally against what
> >> > >> ACPI is meant for on ARM64 systems, so there is no upstream
> >> > >> pathway for this code I am afraid.
> >> > >>
> >> > > Reason of adding this platform specific function in the generic
> >> > > IORT layer is That iort_get_device_domain() only deals with PCI
> >> > > bus (DOMAIN_BUS_PCI_MSI).
> >> > >
> >> > > fsl-mc objects when probed, need to find irq_domain which is
> >> > > associated with the fsl-mc bus (DOMAIN_BUS_FSL_MC_MSI). It will
> >> > > not be possible to do that if we do not add this function because
> >> > > there are no other suitable APIs exported by IORT layer to do the job.
> >> >
> >> > I think we all understood the patch. What both Lorenzo and myself
> >> > are saying is that we do not want non-PCI support in IORT.
> >> >
> >>
> >> IORT supports platform devices (aka named components) as well, and
> >> there is some support for platform MSIs in the GIC layer.
> >>
> >> So it may be possible to hide your exotic bus from the OS entirely,
> >> and make the firmware instantiate a DSDT with device objects and
> >> associated IORT nodes that describe whatever lives on that bus as
> >> named components.
> >>
> >> That way, you will not have to change the OS at all, so your hardware
> >> will not only be supported in linux v5.7+, it will also be supported
> >> by OSes that commercial distro vendors are shipping today. *That* is
> >> the whole point of using ACPI.
> >>
> >> If you are going to bother and modify the OS, you lose this
> >> advantage, and ACPI gives you no benefit over DT at all.
> >
> > I am replying to old message in this conversation, because the
> > discussion got sidetracked from IORT table to SFP/QSFP/devlink stuff
> > from this point onwards, which is not related to IORT.
> > I will only focus on representing the MC device in IORT and using the
> > same in linux.
> > As Ard said:
> > "IORT supports platform devices (aka named components) as well, and
> > there is some support for platform MSIs in the GIC layer."
> >
> > We can represent MC bus as named component in IORT table and use
> > platform MSIs.
> > The only caveat is that with current implementation of platform MSIs,
> > the Input id of a device is not considered.
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felix
> > ir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Firqchip%2Firq-gic
> > -v3-its-platform-
> msi.c%23L50&amp;data=02%7C01%7Cpankaj.bansal%40nxp.co
> >
> m%7Ce18eca3d0494432f73e608d7b1662bdb%7C686ea1d3bc2b4c6fa92cd99c5c
> 30163
> >
> 5%7C0%7C1%7C637172924698903527&amp;sdata=N44g3HIK3AL3Gx6%2BlbW
> %2B0QnWE
> > 4LPqzrL9uuhRFIy5Lc%3D&amp;reserved=0
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felix
> >
> ir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Facpi%2Farm64%2Fio
> >
> rt.c%23L464&amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7Ce18eca3d0
> 4944
> >
> 32f73e608d7b1662bdb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7
> C63717
> >
> 2924698903527&amp;sdata=LoWA6lxY4N%2FidPNaDs2DEqETxYGMdFJsDnr%2B
> xGjGUB
> > 0%3D&amp;reserved=0
> 
> I don't understand what you mean. Platform MSI using IORT uses the DevID of
> end-points. How could the ITS could work without specifying a DevID?
> See for example how the SMMUv3 driver uses platform MSI.

DevID is input ID for PCIe devices. BUT what would be the input ID for platform device? Are we saying that Platform devices can't specify an Input ID ?

> 
> > While, IORT spec doesn't specify any such limitation.
> >
> > we can easily update iort.c to remove this limitation.
> > But, I am not sure how the input id would be passed from platform MSI
> > GIC layer to IORT.
> > Most obviously, the input id should be supplied by dev itself.
> 
> Why should the device know about its own ID? That's a bus/interconnect thing.
> And nothing should be passed *to* IORT. IORT is the source.

IORT is translation between Input IDs <-> Output IDs. The Input ID is still expected to be passed to parse IORT table.

> 
> > Any thoughts?
> 
> I think that in this thread, we have been fairly explicit about what our train of
> though was.
> 
>          M.
> --
> Jazz is not dead. It just smells funny...

Lorenzo Pieralisi Feb. 14, 2020, 4:19 p.m. UTC | #25

On Fri, Feb 14, 2020 at 03:58:14PM +0000, Pankaj Bansal wrote:

[...]

> > Why should the device know about its own ID? That's a bus/interconnect thing.
> > And nothing should be passed *to* IORT. IORT is the source.
> 
> IORT is translation between Input IDs <-> Output IDs. The Input ID is still expected to be passed to parse IORT table.

Named components use an array of single mappings (as in entries with
single mapping flag set) - Input ID is irrelevant.

Not sure what your named component is though and what you want to do
with it, the fact that IORT allows mapping for named components do
not necessarily mean that it can describe what your system really is,
on that you need to elaborate for us to be able to help.

Lorenzo

Robin Murphy Feb. 14, 2020, 4:29 p.m. UTC | #26

On 14/02/2020 3:58 pm, Pankaj Bansal wrote:
[...]
>> I don't understand what you mean. Platform MSI using IORT uses the DevID of
>> end-points. How could the ITS could work without specifying a DevID?
>> See for example how the SMMUv3 driver uses platform MSI.
> 
> DevID is input ID for PCIe devices. BUT what would be the input ID for platform device? Are we saying that Platform devices can't specify an Input ID ?

No, from the IORT perspective, the input for the ID mappings belonging 
to a root complex is the PCI requester ID. The (ITS) DevID is the 
ultimate *output* of a root complex mapping.

Whilst you can indeed represent the MC as a black-box Named Component 
with an ID mapping range not using the "single mapping" flag, that means 
your input IDs come from some device-specific domain beyond the scope of 
IORT. That's why you can't easily get away from your special bus 
integration code.

>>> While, IORT spec doesn't specify any such limitation.
>>>
>>> we can easily update iort.c to remove this limitation.
>>> But, I am not sure how the input id would be passed from platform MSI
>>> GIC layer to IORT.
>>> Most obviously, the input id should be supplied by dev itself.
>>
>> Why should the device know about its own ID? That's a bus/interconnect thing.
>> And nothing should be passed *to* IORT. IORT is the source.
> 
> IORT is translation between Input IDs <-> Output IDs. The Input ID is still expected to be passed to parse IORT table.

...except for single mappings, where the input ID is ignored and the 
output ID is the "source", which is exactly what iort_node_get_id() 
deals with for devices represented in IORT. With what you're talking 
about, "the device" is *not* represented in IORT, but is something 
beyond the MC 'bridge'. Now it probably is technically possible to 
handle that somehow, but it's definitely not something that the existing 
code was ever designed to anticipate.

Robin.

Pankaj Bansal Feb. 14, 2020, 4:35 p.m. UTC | #27

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: Friday, February 14, 2020 9:50 PM
> To: Pankaj Bansal <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>; Ard Biesheuvel
> <[e-mail]>; Makarand Pawagi <[e-mail]>;
> Calvin Johnson <[e-mail]>; [e-mail];
> [e-mail]; Ioana Ciornei <[e-mail]>; Cristi
> Sovaiala <[e-mail]>; Hanjun Guo <[e-mail]>;
> Will Deacon <[e-mail]>; [e-mail]; Russell King
> <[e-mail]>; ACPI Devel Maling List <[e-mail]>;
> Len Brown <[e-mail]>; Jason Cooper <[e-mail]>; Andy
> Wang <[e-mail]>; Varun Sethi <[e-mail]>; Thomas
> Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
> [e-mail]>; Laurentiu Tudor <[e-mail]>; Paul
> Yang <[e-mail]>; [e-mail]; Rafael J. Wysocki
> <[e-mail]>; Linux Kernel Mailing List <[e-mail]>;
> Shameerali Kolothum Thodi <[e-mail]>;
> Sudeep Holla <[e-mail]>; Robin Murphy
> <[e-mail]>
> Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> 
> On Fri, Feb 14, 2020 at 03:58:14PM +0000, Pankaj Bansal wrote:
> 
> [...]
> 
> > > Why should the device know about its own ID? That's a bus/interconnect
> thing.
> > > And nothing should be passed *to* IORT. IORT is the source.
> >
> > IORT is translation between Input IDs <-> Output IDs. The Input ID is still
> expected to be passed to parse IORT table.
> 
> Named components use an array of single mappings (as in entries with single
> mapping flag set) - Input ID is irrelevant.
> 
> Not sure what your named component is though and what you want to do with
> it, the fact that IORT allows mapping for named components do not necessarily
> mean that it can describe what your system really is, on that you need to
> elaborate for us to be able to help.

Details about MC bus can be read from here:
https://elixir.bootlin.com/linux/latest/source/Documentation/networking/device_drivers/freescale/dpaa2/overview.rst#L324

As stated above, in Linux MC is a bus (just like PCI bus, AMBA bus etc)
There can be multiple devices attached to this bus. Moreover, we can dynamically create/destroy these devices.
Now, we want to represent this BUS (not individual devices connected to bus) in IORT table.
The only possible way right now we see is that we describe it as Named components having a pool of ID mappings.
As and when devices are created and attached to bus, we sift through this pool to correctly determine the output ID for the device.
Now the input ID that we provide, can come from device itself.
Then we can use the Platform MSI framework for MC bus devices.

> 
> Lorenzo

Lorenzo Pieralisi Feb. 14, 2020, 5:49 p.m. UTC | #28

On Fri, Feb 14, 2020 at 04:35:10PM +0000, Pankaj Bansal wrote:

[...]

> > -----Original Message-----
> > From: Lorenzo Pieralisi <[e-mail]>
> > Sent: Friday, February 14, 2020 9:50 PM
> > To: Pankaj Bansal <[e-mail]>
> > Cc: Marc Zyngier <[e-mail]>; Ard Biesheuvel
> > <[e-mail]>; Makarand Pawagi <[e-mail]>;
> > Calvin Johnson <[e-mail]>; [e-mail];
> > [e-mail]; Ioana Ciornei <[e-mail]>; Cristi
> > Sovaiala <[e-mail]>; Hanjun Guo <[e-mail]>;
> > Will Deacon <[e-mail]>; [e-mail]; Russell King
> > <[e-mail]>; ACPI Devel Maling List <[e-mail]>;
> > Len Brown <[e-mail]>; Jason Cooper <[e-mail]>; Andy
> > Wang <[e-mail]>; Varun Sethi <[e-mail]>; Thomas
> > Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
> > [e-mail]>; Laurentiu Tudor <[e-mail]>; Paul
> > Yang <[e-mail]>; [e-mail]; Rafael J. Wysocki
> > <[e-mail]>; Linux Kernel Mailing List <[e-mail]>;
> > Shameerali Kolothum Thodi <[e-mail]>;
> > Sudeep Holla <[e-mail]>; Robin Murphy
> > <[e-mail]>
> > Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
Side note: would you mind removing the email headers (as above) in your
replies please ?

> > On Fri, Feb 14, 2020 at 03:58:14PM +0000, Pankaj Bansal wrote:
> > 
> > [...]
> > 
> > > > Why should the device know about its own ID? That's a bus/interconnect
> > thing.
> > > > And nothing should be passed *to* IORT. IORT is the source.
> > >
> > > IORT is translation between Input IDs <-> Output IDs. The Input ID is still
> > expected to be passed to parse IORT table.
> > 
> > Named components use an array of single mappings (as in entries with single
> > mapping flag set) - Input ID is irrelevant.
> > 
> > Not sure what your named component is though and what you want to do with
> > it, the fact that IORT allows mapping for named components do not necessarily
> > mean that it can describe what your system really is, on that you need to
> > elaborate for us to be able to help.
> 
> Details about MC bus can be read from here:
> https://elixir.bootlin.com/linux/latest/source/Documentation/networking/device_drivers/freescale/dpaa2/overview.rst#L324
> 
> As stated above, in Linux MC is a bus (just like PCI bus, AMBA bus etc)
> There can be multiple devices attached to this bus. Moreover, we can dynamically create/destroy these devices.
> Now, we want to represent this BUS (not individual devices connected to bus) in IORT table.
> The only possible way right now we see is that we describe it as Named components having a pool of ID mappings.
> As and when devices are created and attached to bus, we sift through this pool to correctly determine the output ID for the device.
> Now the input ID that we provide, can come from device itself.
> Then we can use the Platform MSI framework for MC bus devices.

So are you asking me if that's OK ? Or there is something you can't
describe with IORT ?

Side note: can you explain to me please how the MSI allocation flow
and kernel data structures/drivers are modeled in DT ? I had a quick
look at:

drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c

and to start with, does that code imply that we create a
DOMAIN_BUS_FSL_MC_MSI on ALL DT systems with an ITS device node ?

I *think* you have a specific API to allocate MSIs for MC devices:

fsl_mc_msi_domain_alloc_irqs()

which hook into the IRQ domain created in the file above that handles
the cascading to an ITS domain, correct ?

Thanks,
Lorenzo

Pankaj Bansal Feb. 17, 2020, 12:35 p.m. UTC | #29

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: Friday, February 14, 2020 11:20 PM
> To: Pankaj Bansal <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>; Ard Biesheuvel
> <[e-mail]>; Makarand Pawagi <[e-mail]>;
> Calvin Johnson <[e-mail]>; [e-mail];
> [e-mail]; Ioana Ciornei <[e-mail]>; Cristi
> Sovaiala <[e-mail]>; Hanjun Guo <[e-mail]>;
> Will Deacon <[e-mail]>; [e-mail]; Russell King
> <[e-mail]>; ACPI Devel Maling List <[e-mail]>;
> Len Brown <[e-mail]>; Jason Cooper <[e-mail]>; Andy
> Wang <[e-mail]>; Varun Sethi <[e-mail]>; Thomas
> Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
> [e-mail]>; Laurentiu Tudor <[e-mail]>; Paul
> Yang <[e-mail]>; [e-mail]; Rafael J. Wysocki
> <[e-mail]>; Linux Kernel Mailing List <[e-mail]>;
> Shameerali Kolothum Thodi <[e-mail]>;
> Sudeep Holla <[e-mail]>; Robin Murphy
> <[e-mail]>
> Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> 
> On Fri, Feb 14, 2020 at 04:35:10PM +0000, Pankaj Bansal wrote:
> 
> [...]
> 
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi <[e-mail]>
> > > Sent: Friday, February 14, 2020 9:50 PM
> > > To: Pankaj Bansal <[e-mail]>
> > > Cc: Marc Zyngier <[e-mail]>; Ard Biesheuvel
> > > <[e-mail]>; Makarand Pawagi
> <[e-mail]>;
> > > Calvin Johnson <[e-mail]>; [e-mail];
> > > [e-mail]; Ioana Ciornei <[e-mail]>; Cristi
> > > Sovaiala <[e-mail]>; Hanjun Guo
> <[e-mail]>;
> > > Will Deacon <[e-mail]>; [e-mail]; Russell King
> > > <[e-mail]>; ACPI Devel Maling List <linux-
> [e-mail]>;
> > > Len Brown <[e-mail]>; Jason Cooper <[e-mail]>;
> Andy
> > > Wang <[e-mail]>; Varun Sethi <[e-mail]>; Thomas
> > > Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
> > > [e-mail]>; Laurentiu Tudor <[e-mail]>;
> Paul
> > > Yang <[e-mail]>; [e-mail]; Rafael J. Wysocki
> > > <[e-mail]>; Linux Kernel Mailing List <linux-
> [e-mail]>;
> > > Shameerali Kolothum Thodi <[e-mail]>;
> > > Sudeep Holla <[e-mail]>; Robin Murphy
> > > <[e-mail]>
> > > Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> Side note: would you mind removing the email headers (as above) in your
> replies please ?
> 
> > > On Fri, Feb 14, 2020 at 03:58:14PM +0000, Pankaj Bansal wrote:
> > >
> > > [...]
> > >
> > > > > Why should the device know about its own ID? That's a bus/interconnect
> > > thing.
> > > > > And nothing should be passed *to* IORT. IORT is the source.
> > > >
> > > > IORT is translation between Input IDs <-> Output IDs. The Input ID is still
> > > expected to be passed to parse IORT table.
> > >
> > > Named components use an array of single mappings (as in entries with single
> > > mapping flag set) - Input ID is irrelevant.
> > >
> > > Not sure what your named component is though and what you want to do
> with
> > > it, the fact that IORT allows mapping for named components do not
> necessarily
> > > mean that it can describe what your system really is, on that you need to
> > > elaborate for us to be able to help.
> >
> > Details about MC bus can be read from here:
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.boo
> tlin.com%2Flinux%2Flatest%2Fsource%2FDocumentation%2Fnetworking%2Fdev
> ice_drivers%2Ffreescale%2Fdpaa2%2Foverview.rst%23L324&amp;data=02%7C0
> 1%7Cpankaj.bansal%40nxp.com%7Cf1bcfc907a42463b617408d7b1764f0f%7C6
> 86ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637172993997763634&am
> p;sdata=fPcC2UfsHoS25oYGmxJmsbPXxb1brKxP1i2qJfPdRpk%3D&amp;reserved
> =0
> >
> > As stated above, in Linux MC is a bus (just like PCI bus, AMBA bus etc)
> > There can be multiple devices attached to this bus. Moreover, we can
> dynamically create/destroy these devices.
> > Now, we want to represent this BUS (not individual devices connected to bus)
> in IORT table.
> > The only possible way right now we see is that we describe it as Named
> components having a pool of ID mappings.
> > As and when devices are created and attached to bus, we sift through this pool
> to correctly determine the output ID for the device.
> > Now the input ID that we provide, can come from device itself.
> > Then we can use the Platform MSI framework for MC bus devices.
> 
> So are you asking me if that's OK ? Or there is something you can't
> describe with IORT ?

I am asking if that would be acceptable?
i.e. we represent MC bus as Named component is IORT table with a pool of IDs (without single ID mapping flag)
and then we use the Platform MSI framework for all children devices of MC bus.
Note that it would require the Platform MSI layer to correctly pass an input id for a platform device to IORT layer.
And IORT layer ought to retrieve the output id based on single ID mapping flag as well as input id.

> 
> Side note: can you explain to me please how the MSI allocation flow
> and kernel data structures/drivers are modeled in DT ? I had a quick
> look at:
> 
> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
> 
> and to start with, does that code imply that we create a
> DOMAIN_BUS_FSL_MC_MSI on ALL DT systems with an ITS device node ?

Yes. It's being done for all DT systems having ITS node.
The domain creation is handled in drivers/bus/fsl-mc/fsl-mc-msi.c

> 
> I *think* you have a specific API to allocate MSIs for MC devices:
> 
> fsl_mc_msi_domain_alloc_irqs()
> 
> which hook into the IRQ domain created in the file above that handles
> the cascading to an ITS domain, correct ?

We associate the above created domain with dpaa2 device here:
https://elixir.bootlin.com/linux/latest/source/drivers/bus/fsl-mc/dprc-driver.c#L640

Then fsl_mc_msi_domain_alloc_irqs, retrieves the domain associated with device and calls the msi_prepare
API from its_fsl_mc_msi_prepare.

> 
> Thanks,
> Lorenzo

Lorenzo Pieralisi Feb. 17, 2020, 3:25 p.m. UTC | #30

On Mon, Feb 17, 2020 at 12:35:12PM +0000, Pankaj Bansal wrote:
> 
> 
> > -----Original Message-----
> > From: Lorenzo Pieralisi <[e-mail]>
> > Sent: Friday, February 14, 2020 11:20 PM
> > To: Pankaj Bansal <[e-mail]>
> > Cc: Marc Zyngier <[e-mail]>; Ard Biesheuvel
> > <[e-mail]>; Makarand Pawagi <[e-mail]>;
> > Calvin Johnson <[e-mail]>; [e-mail];
> > [e-mail]; Ioana Ciornei <[e-mail]>; Cristi
> > Sovaiala <[e-mail]>; Hanjun Guo <[e-mail]>;
> > Will Deacon <[e-mail]>; [e-mail]; Russell King
> > <[e-mail]>; ACPI Devel Maling List <[e-mail]>;
> > Len Brown <[e-mail]>; Jason Cooper <[e-mail]>; Andy
> > Wang <[e-mail]>; Varun Sethi <[e-mail]>; Thomas
> > Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
> > [e-mail]>; Laurentiu Tudor <[e-mail]>; Paul
> > Yang <[e-mail]>; [e-mail]; Rafael J. Wysocki
> > <[e-mail]>; Linux Kernel Mailing List <[e-mail]>;
> > Shameerali Kolothum Thodi <[e-mail]>;
> > Sudeep Holla <[e-mail]>; Robin Murphy
> > <[e-mail]>
> > Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> > 
> > On Fri, Feb 14, 2020 at 04:35:10PM +0000, Pankaj Bansal wrote:
> > 
> > [...]
> > 
> > > > -----Original Message-----
> > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > Sent: Friday, February 14, 2020 9:50 PM
> > > > To: Pankaj Bansal <[e-mail]>
> > > > Cc: Marc Zyngier <[e-mail]>; Ard Biesheuvel
> > > > <[e-mail]>; Makarand Pawagi
> > <[e-mail]>;
> > > > Calvin Johnson <[e-mail]>; [e-mail];
> > > > [e-mail]; Ioana Ciornei <[e-mail]>; Cristi
> > > > Sovaiala <[e-mail]>; Hanjun Guo
> > <[e-mail]>;
> > > > Will Deacon <[e-mail]>; [e-mail]; Russell King
> > > > <[e-mail]>; ACPI Devel Maling List <linux-
> > [e-mail]>;
> > > > Len Brown <[e-mail]>; Jason Cooper <[e-mail]>;
> > Andy
> > > > Wang <[e-mail]>; Varun Sethi <[e-mail]>; Thomas
> > > > Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
> > > > [e-mail]>; Laurentiu Tudor <[e-mail]>;
> > Paul
> > > > Yang <[e-mail]>; [e-mail]; Rafael J. Wysocki
> > > > <[e-mail]>; Linux Kernel Mailing List <linux-
> > [e-mail]>;
> > > > Shameerali Kolothum Thodi <[e-mail]>;
> > > > Sudeep Holla <[e-mail]>; Robin Murphy
> > > > <[e-mail]>
> > > > Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> > Side note: would you mind removing the email headers (as above) in your
> > replies please ?

Read the question above please.

[...]

> > > As stated above, in Linux MC is a bus (just like PCI bus, AMBA bus etc)
> > > There can be multiple devices attached to this bus. Moreover, we can
> > dynamically create/destroy these devices.
> > > Now, we want to represent this BUS (not individual devices connected to bus)
> > in IORT table.
> > > The only possible way right now we see is that we describe it as Named
> > components having a pool of ID mappings.
> > > As and when devices are created and attached to bus, we sift through this pool
> > to correctly determine the output ID for the device.
> > > Now the input ID that we provide, can come from device itself.
> > > Then we can use the Platform MSI framework for MC bus devices.
> > 
> > So are you asking me if that's OK ? Or there is something you can't
> > describe with IORT ?
> 
> I am asking if that would be acceptable?
> i.e. we represent MC bus as Named component is IORT table with a pool of IDs (without single ID mapping flag)
> and then we use the Platform MSI framework for all children devices of MC bus.
> Note that it would require the Platform MSI layer to correctly pass an input id for a platform device to IORT layer.

How is this solved in DT ? You don't seem to need any DT binding on top
of the msi-parent property, which is equivalent to IORT single mappings
AFAICS so I would like to understand the whole DT flow (so that I
understand how this FSL bus works) before commenting any further.

> And IORT layer ought to retrieve the output id based on single ID mapping flag as well as input id.
> 
> > 
> > Side note: can you explain to me please how the MSI allocation flow
> > and kernel data structures/drivers are modeled in DT ? I had a quick
> > look at:
> > 
> > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
> > 
> > and to start with, does that code imply that we create a
> > DOMAIN_BUS_FSL_MC_MSI on ALL DT systems with an ITS device node ?
> 
> Yes. It's being done for all DT systems having ITS node.

This does not seem correct to me, I will let Marc comment on
the matter.

> The domain creation is handled in drivers/bus/fsl-mc/fsl-mc-msi.c

Thanks,
Lorenzo

Marc Zyngier Feb. 17, 2020, 3:35 p.m. UTC | #31

On 2020-02-17 15:25, Lorenzo Pieralisi wrote:
> On Mon, Feb 17, 2020 at 12:35:12PM +0000, Pankaj Bansal wrote:

Hi Lorenzo,

[...]

>> > Side note: can you explain to me please how the MSI allocation flow
>> > and kernel data structures/drivers are modeled in DT ? I had a quick
>> > look at:
>> >
>> > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
>> >
>> > and to start with, does that code imply that we create a
>> > DOMAIN_BUS_FSL_MC_MSI on ALL DT systems with an ITS device node ?
>> 
>> Yes. It's being done for all DT systems having ITS node.
> 
> This does not seem correct to me, I will let Marc comment on
> the matter.

Unfortunately, there isn't a very good way to avoid that ATM,
other than defering the registration of the irqdomain until
we know that a particular bus (for example a PCIe RC) is registered.

I started working on that at some point, and ended up nowhere because
no bus (PCI, FSL, or anything else) really give us the right information
when it is actually required (when a device starts claiming interrupts).

I *think* we could try a defer it until a bus root is found, and that
this bus has a topological link to an ITS. probably invasive though,
as you would need a set of "MSI providers" for each available irqchip
node.

In short, messy. But I'd be happy to revive this and have a look again.

         M.

Lorenzo Pieralisi Feb. 17, 2020, 4:26 p.m. UTC | #32

On Mon, Feb 17, 2020 at 03:35:01PM +0000, Marc Zyngier wrote:
> On 2020-02-17 15:25, Lorenzo Pieralisi wrote:
> > On Mon, Feb 17, 2020 at 12:35:12PM +0000, Pankaj Bansal wrote:
> 
> Hi Lorenzo,
> 
> [...]
> 
> > > > Side note: can you explain to me please how the MSI allocation flow
> > > > and kernel data structures/drivers are modeled in DT ? I had a quick
> > > > look at:
> > > >
> > > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
> > > >
> > > > and to start with, does that code imply that we create a
> > > > DOMAIN_BUS_FSL_MC_MSI on ALL DT systems with an ITS device node ?
> > > 
> > > Yes. It's being done for all DT systems having ITS node.
> > 
> > This does not seem correct to me, I will let Marc comment on
> > the matter.
> 
> Unfortunately, there isn't a very good way to avoid that ATM,
> other than defering the registration of the irqdomain until
> we know that a particular bus (for example a PCIe RC) is registered.
> 
> I started working on that at some point, and ended up nowhere because
> no bus (PCI, FSL, or anything else) really give us the right information
> when it is actually required (when a device starts claiming interrupts).
> 
> I *think* we could try a defer it until a bus root is found, and that
> this bus has a topological link to an ITS. probably invasive though,
> as you would need a set of "MSI providers" for each available irqchip
> node.

Yes I see, it is not trivial - I just raised the point while reading the
code to understand how the IRQ domain structures are connected in the DT
bootstrap case, I don't think that's an urgent issue to solve, just
noticed and reported it to make sure you are aware.

Thanks !
Lorenzo

Pankaj Bansal (OSS) Feb. 18, 2020, 8:02 a.m. UTC | #33

> -----Original Message-----
> From: Lorenzo Pieralisi <[e-mail]>
> Sent: Monday, February 17, 2020 8:55 PM
> To: Pankaj Bansal <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>; Ard Biesheuvel
> <[e-mail]>; Makarand Pawagi <[e-mail]>;
> Calvin Johnson <[e-mail]>; [e-mail];
> [e-mail]; Ioana Ciornei <[e-mail]>; Cristi
> Sovaiala <[e-mail]>; Hanjun Guo <[e-mail]>;
> Will Deacon <[e-mail]>; [e-mail]; Russell King
> <[e-mail]>; ACPI Devel Maling List <[e-mail]>;
> Len Brown <[e-mail]>; Jason Cooper <[e-mail]>; Andy
> Wang <[e-mail]>; Varun Sethi <[e-mail]>; Thomas
> Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
> [e-mail]>; Laurentiu Tudor <[e-mail]>; Paul
> Yang <[e-mail]>; [e-mail]; Rafael J. Wysocki
> <[e-mail]>; Linux Kernel Mailing List <[e-mail]>;
> Shameerali Kolothum Thodi <[e-mail]>;
> Sudeep Holla <[e-mail]>; Robin Murphy
> <[e-mail]>
> Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> 
> On Mon, Feb 17, 2020 at 12:35:12PM +0000, Pankaj Bansal wrote:
> >
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi <[e-mail]>
> > > Sent: Friday, February 14, 2020 11:20 PM
> > > To: Pankaj Bansal <[e-mail]>
> > > Cc: Marc Zyngier <[e-mail]>; Ard Biesheuvel
> > > <[e-mail]>; Makarand Pawagi
> <[e-mail]>;
> > > Calvin Johnson <[e-mail]>; [e-mail];
> > > [e-mail]; Ioana Ciornei <[e-mail]>; Cristi
> > > Sovaiala <[e-mail]>; Hanjun Guo
> <[e-mail]>;
> > > Will Deacon <[e-mail]>; [e-mail]; Russell King
> > > <[e-mail]>; ACPI Devel Maling List <linux-
> [e-mail]>;
> > > Len Brown <[e-mail]>; Jason Cooper <[e-mail]>;
> Andy
> > > Wang <[e-mail]>; Varun Sethi <[e-mail]>; Thomas
> > > Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
> > > [e-mail]>; Laurentiu Tudor <[e-mail]>;
> Paul
> > > Yang <[e-mail]>; [e-mail]; Rafael J. Wysocki
> > > <[e-mail]>; Linux Kernel Mailing List <linux-
> [e-mail]>;
> > > Shameerali Kolothum Thodi <[e-mail]>;
> > > Sudeep Holla <[e-mail]>; Robin Murphy
> > > <[e-mail]>
> > > Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> > >
> > > On Fri, Feb 14, 2020 at 04:35:10PM +0000, Pankaj Bansal wrote:
> > >
> > > [...]
> > >
> > > > > -----Original Message-----
> > > > > From: Lorenzo Pieralisi <[e-mail]>
> > > > > Sent: Friday, February 14, 2020 9:50 PM
> > > > > To: Pankaj Bansal <[e-mail]>
> > > > > Cc: Marc Zyngier <[e-mail]>; Ard Biesheuvel
> > > > > <[e-mail]>; Makarand Pawagi
> > > <[e-mail]>;
> > > > > Calvin Johnson <[e-mail]>; [e-mail];
> > > > > [e-mail]; Ioana Ciornei <[e-mail]>; Cristi
> > > > > Sovaiala <[e-mail]>; Hanjun Guo
> > > <[e-mail]>;
> > > > > Will Deacon <[e-mail]>; [e-mail]; Russell King
> > > > > <[e-mail]>; ACPI Devel Maling List <linux-
> > > [e-mail]>;
> > > > > Len Brown <[e-mail]>; Jason Cooper <[e-mail]>;
> > > Andy
> > > > > Wang <[e-mail]>; Varun Sethi <[e-mail]>;
> Thomas
> > > > > Gleixner <[e-mail]>; linux-arm-kernel <linux-arm-
> > > > > [e-mail]>; Laurentiu Tudor
> <[e-mail]>;
> > > Paul
> > > > > Yang <[e-mail]>; [e-mail]; Rafael J. Wysocki
> > > > > <[e-mail]>; Linux Kernel Mailing List <linux-
> > > [e-mail]>;
> > > > > Shameerali Kolothum Thodi <[e-mail]>;
> > > > > Sudeep Holla <[e-mail]>; Robin Murphy
> > > > > <[e-mail]>
> > > > > Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc
> > > Side note: would you mind removing the email headers (as above) in your
> > > replies please ?
> 
> Read the question above please.
> 
> [...]
> 
> > > > As stated above, in Linux MC is a bus (just like PCI bus, AMBA bus etc)
> > > > There can be multiple devices attached to this bus. Moreover, we can
> > > dynamically create/destroy these devices.
> > > > Now, we want to represent this BUS (not individual devices connected to
> bus)
> > > in IORT table.
> > > > The only possible way right now we see is that we describe it as Named
> > > components having a pool of ID mappings.
> > > > As and when devices are created and attached to bus, we sift through this
> pool
> > > to correctly determine the output ID for the device.
> > > > Now the input ID that we provide, can come from device itself.
> > > > Then we can use the Platform MSI framework for MC bus devices.
> > >
> > > So are you asking me if that's OK ? Or there is something you can't
> > > describe with IORT ?
> >
> > I am asking if that would be acceptable?
> > i.e. we represent MC bus as Named component is IORT table with a pool of IDs
> (without single ID mapping flag)
> > and then we use the Platform MSI framework for all children devices of MC
> bus.
> > Note that it would require the Platform MSI layer to correctly pass an input id
> for a platform device to IORT layer.
> 
> How is this solved in DT ? You don't seem to need any DT binding on top
> of the msi-parent property, which is equivalent to IORT single mappings
> AFAICS so I would like to understand the whole DT flow (so that I
> understand how this FSL bus works) before commenting any further.

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)

> 
> > And IORT layer ought to retrieve the output id based on single ID mapping flag
> as well as input id.
> >
> > >
> > > Side note: can you explain to me please how the MSI allocation flow
> > > and kernel data structures/drivers are modeled in DT ? I had a quick
> > > look at:
> > >
> > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
> > >
> > > and to start with, does that code imply that we create a
> > > DOMAIN_BUS_FSL_MC_MSI on ALL DT systems with an ITS device node ?
> >
> > Yes. It's being done for all DT systems having ITS node.
> 
> This does not seem correct to me, I will let Marc comment on
> the matter.
> 
> > The domain creation is handled in drivers/bus/fsl-mc/fsl-mc-msi.c
> 
> Thanks,
> Lorenzo

Patch

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 33f7198..beb9cd5 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -15,6 +15,7 @@ 
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/pci.h>
+#include <linux/fsl/mc.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
@@ -622,6 +623,29 @@  static int iort_dev_find_its_id(struct device *dev, u32 req_id,
 }
 
 /**
+ * iort_get_fsl_mc_device_domain() - Find MSI domain related to a device
+ * @dev: The device.
+ * @mc_icid: ICID for the fsl_mc device.
+ *
+ * Returns: the MSI domain for this device, NULL otherwise
+ */
+struct irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
+							u32 mc_icid)
+{
+	struct fwnode_handle *handle;
+	int its_id;
+
+	if (iort_dev_find_its_id(dev, mc_icid, 0, &its_id))
+		return NULL;
+
+	handle = iort_find_domain_token(its_id);
+	if (!handle)
+		return NULL;
+
+	return irq_find_matching_fwnode(handle, DOMAIN_BUS_FSL_MC_MSI);
+}
+
+/**
  * iort_get_device_domain() - Find MSI domain related to a device
  * @dev: The device.
  * @req_id: Requester ID for the device.
@@ -924,6 +948,21 @@  static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
 	return iort_iommu_xlate(info->dev, parent, streamid);
 }
 
+static int iort_fsl_mc_iommu_init(struct device *dev,
+				struct acpi_iort_node *node, u32 *streamid)
+{
+	struct acpi_iort_node *parent;
+	struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
+
+	parent = iort_node_map_id(node, mc_dev->icid, streamid,
+						IORT_IOMMU_TYPE);
+
+	if (parent)
+		return iort_iommu_xlate(dev, parent, *streamid);
+
+	return 0;
+}
+
 /**
  * iort_iommu_configure - Set-up IOMMU configuration for a device.
  *
@@ -962,6 +1001,20 @@  const struct iommu_ops *iort_iommu_configure(struct device *dev)
 
 		if (!err && iort_pci_rc_supports_ats(node))
 			dev->iommu_fwspec->flags |= IOMMU_FWSPEC_PCI_RC_ATS;
+	} else if (dev_is_fsl_mc(dev)) {
+		struct device *dma_dev = dev;
+
+		if (!(to_acpi_device_node(dma_dev->fwnode))) {
+			while (dev_is_fsl_mc(dma_dev))
+				dma_dev = dma_dev->parent;
+		}
+
+		node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
+					iort_match_node_callback, dma_dev);
+		if (!node)
+			return NULL;
+
+		err = iort_fsl_mc_iommu_init(dev, node, &streamid);
 	} else {
 		int i = 0;
 
diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
index c8b1c38..31dd790 100644
--- a/drivers/bus/fsl-mc/dprc-driver.c
+++ b/drivers/bus/fsl-mc/dprc-driver.c
@@ -4,6 +4,7 @@ 
  *
  * Copyright (C) 2014-2016 Freescale Semiconductor, Inc.
  * Author: German Rivera <[e-mail]>
+ * Copyright 2018-2020 NXP
  *
  */
 
@@ -638,7 +639,7 @@  static int dprc_probe(struct fsl_mc_device *mc_dev)
 			return -EINVAL;
 
 		error = fsl_mc_find_msi_domain(parent_dev,
-					       &mc_msi_domain);
+					&mc_msi_domain, mc_dev);
 		if (error < 0) {
 			dev_warn(&mc_dev->dev,
 				 "WARNING: MC bus without interrupt support\n");
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index a07cc19..5d388e4 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -4,11 +4,13 @@ 
  *
  * Copyright (C) 2014-2016 Freescale Semiconductor, Inc.
  * Author: German Rivera <[e-mail]>
+ * Copyright 2018-2020 NXP
  *
  */
 
 #define pr_fmt(fmt) "fsl-mc: " fmt
 
+#include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/of_address.h>
@@ -129,12 +131,25 @@  static int fsl_mc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 
 static int fsl_mc_dma_configure(struct device *dev)
 {
+	enum dev_dma_attr attr;
 	struct device *dma_dev = dev;
+	int err = -ENODEV;
 
 	while (dev_is_fsl_mc(dma_dev))
 		dma_dev = dma_dev->parent;
 
-	return of_dma_configure(dev, dma_dev->of_node, 0);
+	if (dma_dev->of_node)
+		err = of_dma_configure(dev, dma_dev->of_node, 1);
+	else {
+		if (has_acpi_companion(dma_dev)) {
+			attr = acpi_get_dma_attr(to_acpi_device_node
+							(dma_dev->fwnode));
+			if (attr != DEV_DMA_NOT_SUPPORTED)
+				err = acpi_dma_configure(dev, attr);
+		}
+	}
+
+	return err;
 }
 
 static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
@@ -863,7 +878,7 @@  static int fsl_mc_bus_probe(struct platform_device *pdev)
 	phys_addr_t mc_portal_phys_addr;
 	u32 mc_portal_size;
 	struct mc_version mc_version;
-	struct resource res;
+	struct resource *plat_res;
 
 	mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
 	if (!mc)
@@ -874,16 +889,9 @@  static int fsl_mc_bus_probe(struct platform_device *pdev)
 	/*
 	 * 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);
@@ -900,11 +908,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,
+	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;
+		if (error < 0)
+			goto error_cleanup_mc_io;
+	}
 
 	error = dprc_get_container_id(mc_io, 0, &container_id);
 	if (error < 0) {
@@ -931,6 +941,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:
@@ -964,11 +975,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 8b9c66d..bd10952 100644
--- a/drivers/bus/fsl-mc/fsl-mc-msi.c
+++ b/drivers/bus/fsl-mc/fsl-mc-msi.c
@@ -4,6 +4,7 @@ 
  *
  * Copyright (C) 2015-2016 Freescale Semiconductor, Inc.
  * Author: German Rivera <[e-mail]>
+ * Copyright 2020 NXP
  *
  */
 
@@ -13,6 +14,7 @@ 
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/msi.h>
+#include <linux/acpi_iort.h>
 
 #include "fsl-mc-private.h"
 
@@ -178,13 +180,19 @@  struct irq_domain *fsl_mc_msi_create_irq_domain(struct fwnode_handle *fwnode,
 }
 
 int fsl_mc_find_msi_domain(struct device *mc_platform_dev,
-			   struct irq_domain **mc_msi_domain)
+			   struct irq_domain **mc_msi_domain,
+				struct fsl_mc_device *mc_dev)
 {
 	struct irq_domain *msi_domain;
 	struct device_node *mc_of_node = mc_platform_dev->of_node;
 
 	msi_domain = of_msi_get_domain(mc_platform_dev, mc_of_node,
 				       DOMAIN_BUS_FSL_MC_MSI);
+
+	if (!msi_domain)
+		msi_domain = iort_get_fsl_mc_device_domain(mc_platform_dev,
+								mc_dev->icid);
+
 	if (!msi_domain) {
 		pr_err("Unable to find fsl-mc MSI domain for %pOF\n",
 		       mc_of_node);
diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
index 21ca8c756..e8f4c0f 100644
--- a/drivers/bus/fsl-mc/fsl-mc-private.h
+++ b/drivers/bus/fsl-mc/fsl-mc-private.h
@@ -3,6 +3,7 @@ 
  * Freescale Management Complex (MC) bus private declarations
  *
  * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2020 NXP
  *
  */
 #ifndef _FSL_MC_PRIVATE_H_
@@ -596,7 +597,8 @@  int fsl_mc_msi_domain_alloc_irqs(struct device *dev,
 void fsl_mc_msi_domain_free_irqs(struct device *dev);
 
 int fsl_mc_find_msi_domain(struct device *mc_platform_dev,
-			   struct irq_domain **mc_msi_domain);
+			   struct irq_domain **mc_msi_domain,
+				struct fsl_mc_device *mc_dev);
 
 int fsl_mc_populate_irq_pool(struct fsl_mc_bus *mc_bus,
 			     unsigned int irq_count);
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 606efa6..df99170 100644
--- a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
@@ -4,9 +4,11 @@ 
  *
  * Copyright (C) 2015-2016 Freescale Semiconductor, Inc.
  * Author: German Rivera <[e-mail]>
+ * Copyright 2020 NXP
  *
  */
 
+#include <linux/acpi_iort.h>
 #include <linux/of_device.h>
 #include <linux/of_address.h>
 #include <linux/irq.h>
@@ -66,7 +68,66 @@  static const struct of_device_id its_device_id[] = {
 	{},
 };
 
-static int __init its_fsl_mc_msi_init(void)
+static int __init its_fsl_mc_msi_init_one(struct fwnode_handle *handle,
+					const char *name)
+{
+	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 -ENXIO;
+	}
+
+	mc_msi_domain = fsl_mc_msi_create_irq_domain(
+					 handle,
+					 &its_fsl_mc_msi_domain_info,
+					 parent);
+	if (!mc_msi_domain)
+		pr_err("ACPIF: unable to create fsl-mc domain\n");
+
+	pr_info("fsl-mc MSI: domain created\n");
+
+	return 0;
+}
+
+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 = -ENXIO;
+
+	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);
+		goto out;
+	}
+
+	err = its_fsl_mc_msi_init_one(dom_handle, node_name);
+	if (!err)
+		pr_info("fsl-mc MSI: %s domain created\n", node_name);
+
+out:
+	kfree(node_name);
+	return err;
+}
+
+static int __init its_fsl_mc_acpi_msi_init(void)
+{
+	acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
+				its_fsl_mc_msi_parse_madt, 0);
+	return 0;
+}
+
+static int __init its_fsl_mc_of_msi_init(void)
 {
 	struct device_node *np;
 	struct irq_domain *parent;
@@ -96,8 +157,14 @@  static int __init its_fsl_mc_msi_init(void)
 
 		pr_info("fsl-mc MSI: %pOF domain created\n", np);
 	}
-
 	return 0;
 }
 
+static int __init its_fsl_mc_msi_init(void)
+{
+	its_fsl_mc_of_msi_init();
+	its_fsl_mc_acpi_msi_init();
+
+	return 0;
+}
 early_initcall(its_fsl_mc_msi_init);
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 8e7e2ec..0afc608 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -30,6 +30,8 @@  struct fwnode_handle *iort_find_domain_token(int trans_id);
 void acpi_iort_init(void);
 u32 iort_msi_map_rid(struct device *dev, u32 req_id);
 struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
+struct irq_domain *iort_get_fsl_mc_device_domain(struct device *dev,
+								u32 req_id);
 void acpi_configure_pmsi_domain(struct device *dev);
 int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
 /* IOMMU interface */
@@ -40,6 +42,9 @@  int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head);
 static inline void acpi_iort_init(void) { }
 static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
 { return req_id; }
+static inline struct irq_domain *iort_get_fsl_mc_device_domain
+					(struct device *dev, u32 req_id)
+{ return NULL; }
 static inline struct irq_domain *iort_get_device_domain(struct device *dev,
 							u32 req_id)
 { return NULL; }