Message ID | 1580198925-50411-1-git-send-email-makarand.pawagi@nxp.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | bus: fsl-mc: Add ACPI support for fsl-mc | expand |
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 <makarand.pawagi@nxp.com> > --- > 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.
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 <makarand.pawagi@nxp.com> > --- > 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
> -----Original Message----- > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: Tuesday, January 28, 2020 4:39 PM > To: Makarand Pawagi <makarand.pawagi@nxp.com> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-acpi@vger.kernel.org; linux@armlinux.org.uk; > jon@solid-run.com; Cristi Sovaiala <cristian.sovaiala@nxp.com>; Laurentiu > Tudor <laurentiu.tudor@nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>; > Varun Sethi <V.Sethi@nxp.com>; Calvin Johnson <calvin.johnson@nxp.com>; > Pankaj Bansal <pankaj.bansal@nxp.com>; guohanjun@huawei.com; > sudeep.holla@arm.com; rjw@rjwysocki.net; lenb@kernel.org; > stuyoder@gmail.com; tglx@linutronix.de; jason@lakedaemon.net; > maz@kernel.org; shameerali.kolothum.thodi@huawei.com; will@kernel.org; > robin.murphy@arm.com; nleeder@codeaurora.org > 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 <makarand.pawagi@nxp.com> > > --- > > 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.
On Fri, Jan 31, 2020 at 10:35:48AM +0000, Makarand Pawagi wrote: > > -----Original Message----- > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Sent: Tuesday, January 28, 2020 4:39 PM > > To: Makarand Pawagi <makarand.pawagi@nxp.com> > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org; linux-acpi@vger.kernel.org; linux@armlinux.org.uk; > > jon@solid-run.com; Cristi Sovaiala <cristian.sovaiala@nxp.com>; Laurentiu > > Tudor <laurentiu.tudor@nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>; > > Varun Sethi <V.Sethi@nxp.com>; Calvin Johnson <calvin.johnson@nxp.com>; > > Pankaj Bansal <pankaj.bansal@nxp.com>; guohanjun@huawei.com; > > sudeep.holla@arm.com; rjw@rjwysocki.net; lenb@kernel.org; > > stuyoder@gmail.com; tglx@linutronix.de; jason@lakedaemon.net; > > maz@kernel.org; shameerali.kolothum.thodi@huawei.com; will@kernel.org; > > robin.murphy@arm.com; nleeder@codeaurora.org > > 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 <makarand.pawagi@nxp.com> > > > --- > > > 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
On 2020-01-31 10:35, Makarand Pawagi wrote: >> -----Original Message----- >> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Sent: Tuesday, January 28, 2020 4:39 PM >> To: Makarand Pawagi <makarand.pawagi@nxp.com> >> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- >> kernel@lists.infradead.org; linux-acpi@vger.kernel.org; >> linux@armlinux.org.uk; >> jon@solid-run.com; Cristi Sovaiala <cristian.sovaiala@nxp.com>; >> Laurentiu >> Tudor <laurentiu.tudor@nxp.com>; Ioana Ciornei >> <ioana.ciornei@nxp.com>; >> Varun Sethi <V.Sethi@nxp.com>; Calvin Johnson >> <calvin.johnson@nxp.com>; >> Pankaj Bansal <pankaj.bansal@nxp.com>; guohanjun@huawei.com; >> sudeep.holla@arm.com; rjw@rjwysocki.net; lenb@kernel.org; >> stuyoder@gmail.com; tglx@linutronix.de; jason@lakedaemon.net; >> maz@kernel.org; shameerali.kolothum.thodi@huawei.com; will@kernel.org; >> robin.murphy@arm.com; nleeder@codeaurora.org >> 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 <makarand.pawagi@nxp.com> >> > --- >> > 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.
On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <maz@kernel.org> wrote: > > On 2020-01-31 10:35, Makarand Pawagi wrote: > >> -----Original Message----- > >> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> Sent: Tuesday, January 28, 2020 4:39 PM > >> To: Makarand Pawagi <makarand.pawagi@nxp.com> > >> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > >> kernel@lists.infradead.org; linux-acpi@vger.kernel.org; > >> linux@armlinux.org.uk; > >> jon@solid-run.com; Cristi Sovaiala <cristian.sovaiala@nxp.com>; > >> Laurentiu > >> Tudor <laurentiu.tudor@nxp.com>; Ioana Ciornei > >> <ioana.ciornei@nxp.com>; > >> Varun Sethi <V.Sethi@nxp.com>; Calvin Johnson > >> <calvin.johnson@nxp.com>; > >> Pankaj Bansal <pankaj.bansal@nxp.com>; guohanjun@huawei.com; > >> sudeep.holla@arm.com; rjw@rjwysocki.net; lenb@kernel.org; > >> stuyoder@gmail.com; tglx@linutronix.de; jason@lakedaemon.net; > >> maz@kernel.org; shameerali.kolothum.thodi@huawei.com; will@kernel.org; > >> robin.murphy@arm.com; nleeder@codeaurora.org > >> 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 <makarand.pawagi@nxp.com> > >> > --- > >> > 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.
On Fri, Jan 31, 2020 at 1:02 PM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <maz@kernel.org> wrote: > > > > On 2020-01-31 10:35, Makarand Pawagi wrote: > > >> -----Original Message----- > > >> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > >> Sent: Tuesday, January 28, 2020 4:39 PM > > >> To: Makarand Pawagi <makarand.pawagi@nxp.com> > > >> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > > >> kernel@lists.infradead.org; linux-acpi@vger.kernel.org; > > >> linux@armlinux.org.uk; > > >> jon@solid-run.com; Cristi Sovaiala <cristian.sovaiala@nxp.com>; > > >> Laurentiu > > >> Tudor <laurentiu.tudor@nxp.com>; Ioana Ciornei > > >> <ioana.ciornei@nxp.com>; > > >> Varun Sethi <V.Sethi@nxp.com>; Calvin Johnson > > >> <calvin.johnson@nxp.com>; > > >> Pankaj Bansal <pankaj.bansal@nxp.com>; guohanjun@huawei.com; > > >> sudeep.holla@arm.com; rjw@rjwysocki.net; lenb@kernel.org; > > >> stuyoder@gmail.com; tglx@linutronix.de; jason@lakedaemon.net; > > >> maz@kernel.org; shameerali.kolothum.thodi@huawei.com; will@kernel.org; > > >> robin.murphy@arm.com; nleeder@codeaurora.org > > >> 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 <makarand.pawagi@nxp.com> > > >> > --- > > >> > 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
On 2020-01-31 12:28 pm, Jon Nettleton wrote: > On Fri, Jan 31, 2020 at 1:02 PM Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> >> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <maz@kernel.org> wrote: >>> >>> On 2020-01-31 10:35, Makarand Pawagi wrote: >>>>> -----Original Message----- >>>>> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>>>> Sent: Tuesday, January 28, 2020 4:39 PM >>>>> To: Makarand Pawagi <makarand.pawagi@nxp.com> >>>>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- >>>>> kernel@lists.infradead.org; linux-acpi@vger.kernel.org; >>>>> linux@armlinux.org.uk; >>>>> jon@solid-run.com; Cristi Sovaiala <cristian.sovaiala@nxp.com>; >>>>> Laurentiu >>>>> Tudor <laurentiu.tudor@nxp.com>; Ioana Ciornei >>>>> <ioana.ciornei@nxp.com>; >>>>> Varun Sethi <V.Sethi@nxp.com>; Calvin Johnson >>>>> <calvin.johnson@nxp.com>; >>>>> Pankaj Bansal <pankaj.bansal@nxp.com>; guohanjun@huawei.com; >>>>> sudeep.holla@arm.com; rjw@rjwysocki.net; lenb@kernel.org; >>>>> stuyoder@gmail.com; tglx@linutronix.de; jason@lakedaemon.net; >>>>> maz@kernel.org; shameerali.kolothum.thodi@huawei.com; will@kernel.org; >>>>> robin.murphy@arm.com; nleeder@codeaurora.org >>>>> 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 <makarand.pawagi@nxp.com> >>>>>> --- >>>>>> 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 >
On Fri, Jan 31, 2020 at 1:48 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2020-01-31 12:28 pm, Jon Nettleton wrote: > > On Fri, Jan 31, 2020 at 1:02 PM Ard Biesheuvel > > <ard.biesheuvel@linaro.org> wrote: > >> > >> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <maz@kernel.org> wrote: > >>> > >>> On 2020-01-31 10:35, Makarand Pawagi wrote: > >>>>> -----Original Message----- > >>>>> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >>>>> Sent: Tuesday, January 28, 2020 4:39 PM > >>>>> To: Makarand Pawagi <makarand.pawagi@nxp.com> > >>>>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > >>>>> kernel@lists.infradead.org; linux-acpi@vger.kernel.org; > >>>>> linux@armlinux.org.uk; > >>>>> jon@solid-run.com; Cristi Sovaiala <cristian.sovaiala@nxp.com>; > >>>>> Laurentiu > >>>>> Tudor <laurentiu.tudor@nxp.com>; Ioana Ciornei > >>>>> <ioana.ciornei@nxp.com>; > >>>>> Varun Sethi <V.Sethi@nxp.com>; Calvin Johnson > >>>>> <calvin.johnson@nxp.com>; > >>>>> Pankaj Bansal <pankaj.bansal@nxp.com>; guohanjun@huawei.com; > >>>>> sudeep.holla@arm.com; rjw@rjwysocki.net; lenb@kernel.org; > >>>>> stuyoder@gmail.com; tglx@linutronix.de; jason@lakedaemon.net; > >>>>> maz@kernel.org; shameerali.kolothum.thodi@huawei.com; will@kernel.org; > >>>>> robin.murphy@arm.com; nleeder@codeaurora.org > >>>>> 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 <makarand.pawagi@nxp.com> > >>>>>> --- > >>>>>> 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
On Fri, 31 Jan 2020 at 14:12, Jon Nettleton <jon@solid-run.com> wrote: > > On Fri, Jan 31, 2020 at 1:48 PM Robin Murphy <robin.murphy@arm.com> wrote: > > > > On 2020-01-31 12:28 pm, Jon Nettleton wrote: > > > On Fri, Jan 31, 2020 at 1:02 PM Ard Biesheuvel > > > <ard.biesheuvel@linaro.org> wrote: > > >> > > >> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <maz@kernel.org> wrote: > > >>> > > >>> On 2020-01-31 10:35, Makarand Pawagi wrote: > > >>>>> -----Original Message----- > > >>>>> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > >>>>> Sent: Tuesday, January 28, 2020 4:39 PM > > >>>>> To: Makarand Pawagi <makarand.pawagi@nxp.com> > > >>>>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > > >>>>> kernel@lists.infradead.org; linux-acpi@vger.kernel.org; > > >>>>> linux@armlinux.org.uk; > > >>>>> jon@solid-run.com; Cristi Sovaiala <cristian.sovaiala@nxp.com>; > > >>>>> Laurentiu > > >>>>> Tudor <laurentiu.tudor@nxp.com>; Ioana Ciornei > > >>>>> <ioana.ciornei@nxp.com>; > > >>>>> Varun Sethi <V.Sethi@nxp.com>; Calvin Johnson > > >>>>> <calvin.johnson@nxp.com>; > > >>>>> Pankaj Bansal <pankaj.bansal@nxp.com>; guohanjun@huawei.com; > > >>>>> sudeep.holla@arm.com; rjw@rjwysocki.net; lenb@kernel.org; > > >>>>> stuyoder@gmail.com; tglx@linutronix.de; jason@lakedaemon.net; > > >>>>> maz@kernel.org; shameerali.kolothum.thodi@huawei.com; will@kernel.org; > > >>>>> robin.murphy@arm.com; nleeder@codeaurora.org > > >>>>> 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 <makarand.pawagi@nxp.com> > > >>>>>> --- > > >>>>>> 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.
On 2020-01-31 1:11 pm, Jon Nettleton wrote: > On Fri, Jan 31, 2020 at 1:48 PM Robin Murphy <robin.murphy@arm.com> wrote: >> >> On 2020-01-31 12:28 pm, Jon Nettleton wrote: >>> On Fri, Jan 31, 2020 at 1:02 PM Ard Biesheuvel >>> <ard.biesheuvel@linaro.org> wrote: >>>> >>>> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <maz@kernel.org> wrote: >>>>> >>>>> On 2020-01-31 10:35, Makarand Pawagi wrote: >>>>>>> -----Original Message----- >>>>>>> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>>>>>> Sent: Tuesday, January 28, 2020 4:39 PM >>>>>>> To: Makarand Pawagi <makarand.pawagi@nxp.com> >>>>>>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- >>>>>>> kernel@lists.infradead.org; linux-acpi@vger.kernel.org; >>>>>>> linux@armlinux.org.uk; >>>>>>> jon@solid-run.com; Cristi Sovaiala <cristian.sovaiala@nxp.com>; >>>>>>> Laurentiu >>>>>>> Tudor <laurentiu.tudor@nxp.com>; Ioana Ciornei >>>>>>> <ioana.ciornei@nxp.com>; >>>>>>> Varun Sethi <V.Sethi@nxp.com>; Calvin Johnson >>>>>>> <calvin.johnson@nxp.com>; >>>>>>> Pankaj Bansal <pankaj.bansal@nxp.com>; guohanjun@huawei.com; >>>>>>> sudeep.holla@arm.com; rjw@rjwysocki.net; lenb@kernel.org; >>>>>>> stuyoder@gmail.com; tglx@linutronix.de; jason@lakedaemon.net; >>>>>>> maz@kernel.org; shameerali.kolothum.thodi@huawei.com; will@kernel.org; >>>>>>> robin.murphy@arm.com; nleeder@codeaurora.org >>>>>>> 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 <makarand.pawagi@nxp.com> >>>>>>>> --- >>>>>>>> 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.
> > 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
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
> 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
On Fri, Jan 31, 2020 at 4:09 PM Andrew Lunn <andrew@lunn.ch> 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
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.
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.
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.
> 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
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.
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 :)
> -----Original Message----- > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Sent: Friday, January 31, 2020 5:32 PM > To: Marc Zyngier <maz@kernel.org> > Cc: Makarand Pawagi <makarand.pawagi@nxp.com>; Calvin Johnson > <calvin.johnson@nxp.com>; stuyoder@gmail.com; nleeder@codeaurora.org; > Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi Sovaiala > <cristian.sovaiala@nxp.com>; Hanjun Guo <guohanjun@huawei.com>; Will > Deacon <will@kernel.org>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; > Pankaj Bansal <pankaj.bansal@nxp.com>; jon@solid-run.com; Russell King > <linux@armlinux.org.uk>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; > Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; Andy > Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas > Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > kernel@lists.infradead.org>; Laurentiu Tudor <laurentiu.tudor@nxp.com>; Paul > Yang <Paul.Yang@arm.com>; <netdev@vger.kernel.org> > <netdev@vger.kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Linux Kernel > Mailing List <linux-kernel@vger.kernel.org>; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@huawei.com>; Sudeep Holla > <sudeep.holla@arm.com>; Robin Murphy <robin.murphy@arm.com> > Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for fsl-mc > > On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <maz@kernel.org> wrote: > > > > On 2020-01-31 10:35, Makarand Pawagi wrote: > > >> -----Original Message----- > > >> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > >> Sent: Tuesday, January 28, 2020 4:39 PM > > >> To: Makarand Pawagi <makarand.pawagi@nxp.com> > > >> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > > >> linux-arm- kernel@lists.infradead.org; linux-acpi@vger.kernel.org; > > >> linux@armlinux.org.uk; jon@solid-run.com; Cristi Sovaiala > > >> <cristian.sovaiala@nxp.com>; Laurentiu Tudor > > >> <laurentiu.tudor@nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>; > > >> Varun Sethi <V.Sethi@nxp.com>; Calvin Johnson > > >> <calvin.johnson@nxp.com>; Pankaj Bansal <pankaj.bansal@nxp.com>; > > >> guohanjun@huawei.com; sudeep.holla@arm.com; rjw@rjwysocki.net; > > >> lenb@kernel.org; stuyoder@gmail.com; tglx@linutronix.de; > > >> jason@lakedaemon.net; maz@kernel.org; > > >> shameerali.kolothum.thodi@huawei.com; will@kernel.org; > > >> robin.murphy@arm.com; nleeder@codeaurora.org > > >> 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 <makarand.pawagi@nxp.com> > > >> > --- > > >> > 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 > > >> > &data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cdb56d889d85646277ee > 30 > > >> > 8d7a64562fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6371606 > 892 > > >> > 50769265&sdata=C7nCty8%2BVeuq6VhcEUXCwiAinN01rCfe12NRVnXJCIY% > 3D > > >> &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
On 2020-02-14 15:05, Pankaj Bansal wrote: >> -----Original Message----- >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Sent: Friday, January 31, 2020 5:32 PM >> To: Marc Zyngier <maz@kernel.org> >> Cc: Makarand Pawagi <makarand.pawagi@nxp.com>; Calvin Johnson >> <calvin.johnson@nxp.com>; stuyoder@gmail.com; nleeder@codeaurora.org; >> Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi Sovaiala >> <cristian.sovaiala@nxp.com>; Hanjun Guo <guohanjun@huawei.com>; Will >> Deacon <will@kernel.org>; Lorenzo Pieralisi >> <lorenzo.pieralisi@arm.com>; >> Pankaj Bansal <pankaj.bansal@nxp.com>; jon@solid-run.com; Russell King >> <linux@armlinux.org.uk>; ACPI Devel Maling List >> <linux-acpi@vger.kernel.org>; >> Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; Andy >> Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas >> Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- >> kernel@lists.infradead.org>; Laurentiu Tudor >> <laurentiu.tudor@nxp.com>; Paul >> Yang <Paul.Yang@arm.com>; <netdev@vger.kernel.org> >> <netdev@vger.kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Linux >> Kernel >> Mailing List <linux-kernel@vger.kernel.org>; Shameerali Kolothum Thodi >> <shameerali.kolothum.thodi@huawei.com>; Sudeep Holla >> <sudeep.holla@arm.com>; Robin Murphy <robin.murphy@arm.com> >> Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for >> fsl-mc >> >> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <maz@kernel.org> wrote: >> > >> > On 2020-01-31 10:35, Makarand Pawagi wrote: >> > >> -----Original Message----- >> > >> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> > >> Sent: Tuesday, January 28, 2020 4:39 PM >> > >> To: Makarand Pawagi <makarand.pawagi@nxp.com> >> > >> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >> > >> linux-arm- kernel@lists.infradead.org; linux-acpi@vger.kernel.org; >> > >> linux@armlinux.org.uk; jon@solid-run.com; Cristi Sovaiala >> > >> <cristian.sovaiala@nxp.com>; Laurentiu Tudor >> > >> <laurentiu.tudor@nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>; >> > >> Varun Sethi <V.Sethi@nxp.com>; Calvin Johnson >> > >> <calvin.johnson@nxp.com>; Pankaj Bansal <pankaj.bansal@nxp.com>; >> > >> guohanjun@huawei.com; sudeep.holla@arm.com; rjw@rjwysocki.net; >> > >> lenb@kernel.org; stuyoder@gmail.com; tglx@linutronix.de; >> > >> jason@lakedaemon.net; maz@kernel.org; >> > >> shameerali.kolothum.thodi@huawei.com; will@kernel.org; >> > >> robin.murphy@arm.com; nleeder@codeaurora.org >> > >> 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 <makarand.pawagi@nxp.com> >> > >> > --- >> > >> > 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 >> > >> >> &data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cdb56d889d85646277ee >> 30 >> > >> >> 8d7a64562fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6371606 >> 892 >> > >> >> 50769265&sdata=C7nCty8%2BVeuq6VhcEUXCwiAinN01rCfe12NRVnXJCIY% >> 3D >> > >> &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.
> -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: Friday, February 14, 2020 9:24 PM > To: Pankaj Bansal <pankaj.bansal@nxp.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com>; Makarand Pawagi <makarand.pawagi@nxp.com>; > Calvin Johnson <calvin.johnson@nxp.com>; stuyoder@gmail.com; > nleeder@codeaurora.org; Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi > Sovaiala <cristian.sovaiala@nxp.com>; Hanjun Guo <guohanjun@huawei.com>; > Will Deacon <will@kernel.org>; jon@solid-run.com; Russell King > <linux@armlinux.org.uk>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; > Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; Andy > Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas > Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > kernel@lists.infradead.org>; Laurentiu Tudor <laurentiu.tudor@nxp.com>; Paul > Yang <Paul.Yang@arm.com>; netdev@vger.kernel.org; Rafael J. Wysocki > <rjw@rjwysocki.net>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > Sudeep Holla <sudeep.holla@arm.com>; Robin Murphy > <robin.murphy@arm.com> > 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 <ard.biesheuvel@linaro.org> > >> Sent: Friday, January 31, 2020 5:32 PM > >> To: Marc Zyngier <maz@kernel.org> > >> Cc: Makarand Pawagi <makarand.pawagi@nxp.com>; Calvin Johnson > >> <calvin.johnson@nxp.com>; stuyoder@gmail.com; nleeder@codeaurora.org; > >> Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi Sovaiala > >> <cristian.sovaiala@nxp.com>; Hanjun Guo <guohanjun@huawei.com>; Will > >> Deacon <will@kernel.org>; Lorenzo Pieralisi > >> <lorenzo.pieralisi@arm.com>; Pankaj Bansal <pankaj.bansal@nxp.com>; > >> jon@solid-run.com; Russell King <linux@armlinux.org.uk>; ACPI Devel > >> Maling List <linux-acpi@vger.kernel.org>; Len Brown > >> <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; Andy Wang > >> <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas Gleixner > >> <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > >> kernel@lists.infradead.org>; Laurentiu Tudor > >> <laurentiu.tudor@nxp.com>; Paul Yang <Paul.Yang@arm.com>; > >> <netdev@vger.kernel.org> <netdev@vger.kernel.org>; Rafael J. Wysocki > >> <rjw@rjwysocki.net>; Linux Kernel Mailing List > >> <linux-kernel@vger.kernel.org>; Shameerali Kolothum Thodi > >> <shameerali.kolothum.thodi@huawei.com>; Sudeep Holla > >> <sudeep.holla@arm.com>; Robin Murphy <robin.murphy@arm.com> > >> Subject: Re: [EXT] Re: [PATCH] bus: fsl-mc: Add ACPI support for > >> fsl-mc > >> > >> On Fri, 31 Jan 2020 at 12:06, Marc Zyngier <maz@kernel.org> wrote: > >> > > >> > On 2020-01-31 10:35, Makarand Pawagi wrote: > >> > >> -----Original Message----- > >> > >> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> > >> Sent: Tuesday, January 28, 2020 4:39 PM > >> > >> To: Makarand Pawagi <makarand.pawagi@nxp.com> > >> > >> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > >> > >> linux-arm- kernel@lists.infradead.org; > >> > >> linux-acpi@vger.kernel.org; linux@armlinux.org.uk; > >> > >> jon@solid-run.com; Cristi Sovaiala <cristian.sovaiala@nxp.com>; > >> > >> Laurentiu Tudor <laurentiu.tudor@nxp.com>; Ioana Ciornei > >> > >> <ioana.ciornei@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Calvin > >> > >> Johnson <calvin.johnson@nxp.com>; Pankaj Bansal > >> > >> <pankaj.bansal@nxp.com>; guohanjun@huawei.com; > >> > >> sudeep.holla@arm.com; rjw@rjwysocki.net; lenb@kernel.org; > >> > >> stuyoder@gmail.com; tglx@linutronix.de; jason@lakedaemon.net; > >> > >> maz@kernel.org; shameerali.kolothum.thodi@huawei.com; > >> > >> will@kernel.org; robin.murphy@arm.com; nleeder@codeaurora.org > >> > >> 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 <makarand.pawagi@nxp.com> > >> > >> > --- > >> > >> > 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 > >> > >> > >> > &data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cdb56d889d85646277ee > >> 30 > >> > >> > >> > 8d7a64562fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6371606 > >> 892 > >> > >> > >> > 50769265&sdata=C7nCty8%2BVeuq6VhcEUXCwiAinN01rCfe12NRVnXJCIY% > >> 3D > >> > >> &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&data=02%7C01%7Cpankaj.bansal%40nxp.co > > > m%7Ce18eca3d0494432f73e608d7b1662bdb%7C686ea1d3bc2b4c6fa92cd99c5c > 30163 > > > 5%7C0%7C1%7C637172924698903527&sdata=N44g3HIK3AL3Gx6%2BlbW > %2B0QnWE > > 4LPqzrL9uuhRFIy5Lc%3D&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&data=02%7C01%7Cpankaj.bansal%40nxp.com%7Ce18eca3d0 > 4944 > > > 32f73e608d7b1662bdb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7 > C63717 > > > 2924698903527&sdata=LoWA6lxY4N%2FidPNaDs2DEqETxYGMdFJsDnr%2B > xGjGUB > > 0%3D&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...
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
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.
> -----Original Message----- > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: Friday, February 14, 2020 9:50 PM > To: Pankaj Bansal <pankaj.bansal@nxp.com> > Cc: Marc Zyngier <maz@kernel.org>; Ard Biesheuvel > <ard.biesheuvel@linaro.org>; Makarand Pawagi <makarand.pawagi@nxp.com>; > Calvin Johnson <calvin.johnson@nxp.com>; stuyoder@gmail.com; > nleeder@codeaurora.org; Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi > Sovaiala <cristian.sovaiala@nxp.com>; Hanjun Guo <guohanjun@huawei.com>; > Will Deacon <will@kernel.org>; jon@solid-run.com; Russell King > <linux@armlinux.org.uk>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; > Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; Andy > Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas > Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > kernel@lists.infradead.org>; Laurentiu Tudor <laurentiu.tudor@nxp.com>; Paul > Yang <Paul.Yang@arm.com>; netdev@vger.kernel.org; Rafael J. Wysocki > <rjw@rjwysocki.net>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > Sudeep Holla <sudeep.holla@arm.com>; Robin Murphy > <robin.murphy@arm.com> > 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
On Fri, Feb 14, 2020 at 04:35:10PM +0000, Pankaj Bansal wrote: [...] > > -----Original Message----- > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Sent: Friday, February 14, 2020 9:50 PM > > To: Pankaj Bansal <pankaj.bansal@nxp.com> > > Cc: Marc Zyngier <maz@kernel.org>; Ard Biesheuvel > > <ard.biesheuvel@linaro.org>; Makarand Pawagi <makarand.pawagi@nxp.com>; > > Calvin Johnson <calvin.johnson@nxp.com>; stuyoder@gmail.com; > > nleeder@codeaurora.org; Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi > > Sovaiala <cristian.sovaiala@nxp.com>; Hanjun Guo <guohanjun@huawei.com>; > > Will Deacon <will@kernel.org>; jon@solid-run.com; Russell King > > <linux@armlinux.org.uk>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; > > Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; Andy > > Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas > > Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > > kernel@lists.infradead.org>; Laurentiu Tudor <laurentiu.tudor@nxp.com>; Paul > > Yang <Paul.Yang@arm.com>; netdev@vger.kernel.org; Rafael J. Wysocki > > <rjw@rjwysocki.net>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; > > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > > Sudeep Holla <sudeep.holla@arm.com>; Robin Murphy > > <robin.murphy@arm.com> > > 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
> -----Original Message----- > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: Friday, February 14, 2020 11:20 PM > To: Pankaj Bansal <pankaj.bansal@nxp.com> > Cc: Marc Zyngier <maz@kernel.org>; Ard Biesheuvel > <ard.biesheuvel@linaro.org>; Makarand Pawagi <makarand.pawagi@nxp.com>; > Calvin Johnson <calvin.johnson@nxp.com>; stuyoder@gmail.com; > nleeder@codeaurora.org; Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi > Sovaiala <cristian.sovaiala@nxp.com>; Hanjun Guo <guohanjun@huawei.com>; > Will Deacon <will@kernel.org>; jon@solid-run.com; Russell King > <linux@armlinux.org.uk>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; > Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; Andy > Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas > Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > kernel@lists.infradead.org>; Laurentiu Tudor <laurentiu.tudor@nxp.com>; Paul > Yang <Paul.Yang@arm.com>; netdev@vger.kernel.org; Rafael J. Wysocki > <rjw@rjwysocki.net>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > Sudeep Holla <sudeep.holla@arm.com>; Robin Murphy > <robin.murphy@arm.com> > 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 <lorenzo.pieralisi@arm.com> > > > Sent: Friday, February 14, 2020 9:50 PM > > > To: Pankaj Bansal <pankaj.bansal@nxp.com> > > > Cc: Marc Zyngier <maz@kernel.org>; Ard Biesheuvel > > > <ard.biesheuvel@linaro.org>; Makarand Pawagi > <makarand.pawagi@nxp.com>; > > > Calvin Johnson <calvin.johnson@nxp.com>; stuyoder@gmail.com; > > > nleeder@codeaurora.org; Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi > > > Sovaiala <cristian.sovaiala@nxp.com>; Hanjun Guo > <guohanjun@huawei.com>; > > > Will Deacon <will@kernel.org>; jon@solid-run.com; Russell King > > > <linux@armlinux.org.uk>; ACPI Devel Maling List <linux- > acpi@vger.kernel.org>; > > > Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; > Andy > > > Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas > > > Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > > > kernel@lists.infradead.org>; Laurentiu Tudor <laurentiu.tudor@nxp.com>; > Paul > > > Yang <Paul.Yang@arm.com>; netdev@vger.kernel.org; Rafael J. Wysocki > > > <rjw@rjwysocki.net>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; > > > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > > > Sudeep Holla <sudeep.holla@arm.com>; Robin Murphy > > > <robin.murphy@arm.com> > > > 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&data=02%7C0 > 1%7Cpankaj.bansal%40nxp.com%7Cf1bcfc907a42463b617408d7b1764f0f%7C6 > 86ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637172993997763634&am > p;sdata=fPcC2UfsHoS25oYGmxJmsbPXxb1brKxP1i2qJfPdRpk%3D&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
On Mon, Feb 17, 2020 at 12:35:12PM +0000, Pankaj Bansal wrote: > > > > -----Original Message----- > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Sent: Friday, February 14, 2020 11:20 PM > > To: Pankaj Bansal <pankaj.bansal@nxp.com> > > Cc: Marc Zyngier <maz@kernel.org>; Ard Biesheuvel > > <ard.biesheuvel@linaro.org>; Makarand Pawagi <makarand.pawagi@nxp.com>; > > Calvin Johnson <calvin.johnson@nxp.com>; stuyoder@gmail.com; > > nleeder@codeaurora.org; Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi > > Sovaiala <cristian.sovaiala@nxp.com>; Hanjun Guo <guohanjun@huawei.com>; > > Will Deacon <will@kernel.org>; jon@solid-run.com; Russell King > > <linux@armlinux.org.uk>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; > > Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; Andy > > Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas > > Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > > kernel@lists.infradead.org>; Laurentiu Tudor <laurentiu.tudor@nxp.com>; Paul > > Yang <Paul.Yang@arm.com>; netdev@vger.kernel.org; Rafael J. Wysocki > > <rjw@rjwysocki.net>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; > > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > > Sudeep Holla <sudeep.holla@arm.com>; Robin Murphy > > <robin.murphy@arm.com> > > 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 <lorenzo.pieralisi@arm.com> > > > > Sent: Friday, February 14, 2020 9:50 PM > > > > To: Pankaj Bansal <pankaj.bansal@nxp.com> > > > > Cc: Marc Zyngier <maz@kernel.org>; Ard Biesheuvel > > > > <ard.biesheuvel@linaro.org>; Makarand Pawagi > > <makarand.pawagi@nxp.com>; > > > > Calvin Johnson <calvin.johnson@nxp.com>; stuyoder@gmail.com; > > > > nleeder@codeaurora.org; Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi > > > > Sovaiala <cristian.sovaiala@nxp.com>; Hanjun Guo > > <guohanjun@huawei.com>; > > > > Will Deacon <will@kernel.org>; jon@solid-run.com; Russell King > > > > <linux@armlinux.org.uk>; ACPI Devel Maling List <linux- > > acpi@vger.kernel.org>; > > > > Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; > > Andy > > > > Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas > > > > Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > > > > kernel@lists.infradead.org>; Laurentiu Tudor <laurentiu.tudor@nxp.com>; > > Paul > > > > Yang <Paul.Yang@arm.com>; netdev@vger.kernel.org; Rafael J. Wysocki > > > > <rjw@rjwysocki.net>; Linux Kernel Mailing List <linux- > > kernel@vger.kernel.org>; > > > > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > > > > Sudeep Holla <sudeep.holla@arm.com>; Robin Murphy > > > > <robin.murphy@arm.com> > > > > 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
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.
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
> -----Original Message----- > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: Monday, February 17, 2020 8:55 PM > To: Pankaj Bansal <pankaj.bansal@nxp.com> > Cc: Marc Zyngier <maz@kernel.org>; Ard Biesheuvel > <ard.biesheuvel@linaro.org>; Makarand Pawagi <makarand.pawagi@nxp.com>; > Calvin Johnson <calvin.johnson@nxp.com>; stuyoder@gmail.com; > nleeder@codeaurora.org; Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi > Sovaiala <cristian.sovaiala@nxp.com>; Hanjun Guo <guohanjun@huawei.com>; > Will Deacon <will@kernel.org>; jon@solid-run.com; Russell King > <linux@armlinux.org.uk>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; > Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; Andy > Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas > Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > kernel@lists.infradead.org>; Laurentiu Tudor <laurentiu.tudor@nxp.com>; Paul > Yang <Paul.Yang@arm.com>; netdev@vger.kernel.org; Rafael J. Wysocki > <rjw@rjwysocki.net>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > Sudeep Holla <sudeep.holla@arm.com>; Robin Murphy > <robin.murphy@arm.com> > 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 <lorenzo.pieralisi@arm.com> > > > Sent: Friday, February 14, 2020 11:20 PM > > > To: Pankaj Bansal <pankaj.bansal@nxp.com> > > > Cc: Marc Zyngier <maz@kernel.org>; Ard Biesheuvel > > > <ard.biesheuvel@linaro.org>; Makarand Pawagi > <makarand.pawagi@nxp.com>; > > > Calvin Johnson <calvin.johnson@nxp.com>; stuyoder@gmail.com; > > > nleeder@codeaurora.org; Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi > > > Sovaiala <cristian.sovaiala@nxp.com>; Hanjun Guo > <guohanjun@huawei.com>; > > > Will Deacon <will@kernel.org>; jon@solid-run.com; Russell King > > > <linux@armlinux.org.uk>; ACPI Devel Maling List <linux- > acpi@vger.kernel.org>; > > > Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; > Andy > > > Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; Thomas > > > Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > > > kernel@lists.infradead.org>; Laurentiu Tudor <laurentiu.tudor@nxp.com>; > Paul > > > Yang <Paul.Yang@arm.com>; netdev@vger.kernel.org; Rafael J. Wysocki > > > <rjw@rjwysocki.net>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; > > > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > > > Sudeep Holla <sudeep.holla@arm.com>; Robin Murphy > > > <robin.murphy@arm.com> > > > 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 <lorenzo.pieralisi@arm.com> > > > > > Sent: Friday, February 14, 2020 9:50 PM > > > > > To: Pankaj Bansal <pankaj.bansal@nxp.com> > > > > > Cc: Marc Zyngier <maz@kernel.org>; Ard Biesheuvel > > > > > <ard.biesheuvel@linaro.org>; Makarand Pawagi > > > <makarand.pawagi@nxp.com>; > > > > > Calvin Johnson <calvin.johnson@nxp.com>; stuyoder@gmail.com; > > > > > nleeder@codeaurora.org; Ioana Ciornei <ioana.ciornei@nxp.com>; Cristi > > > > > Sovaiala <cristian.sovaiala@nxp.com>; Hanjun Guo > > > <guohanjun@huawei.com>; > > > > > Will Deacon <will@kernel.org>; jon@solid-run.com; Russell King > > > > > <linux@armlinux.org.uk>; ACPI Devel Maling List <linux- > > > acpi@vger.kernel.org>; > > > > > Len Brown <lenb@kernel.org>; Jason Cooper <jason@lakedaemon.net>; > > > Andy > > > > > Wang <Andy.Wang@arm.com>; Varun Sethi <V.Sethi@nxp.com>; > Thomas > > > > > Gleixner <tglx@linutronix.de>; linux-arm-kernel <linux-arm- > > > > > kernel@lists.infradead.org>; Laurentiu Tudor > <laurentiu.tudor@nxp.com>; > > > Paul > > > > > Yang <Paul.Yang@arm.com>; netdev@vger.kernel.org; Rafael J. Wysocki > > > > > <rjw@rjwysocki.net>; Linux Kernel Mailing List <linux- > > > kernel@vger.kernel.org>; > > > > > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > > > > > Sudeep Holla <sudeep.holla@arm.com>; Robin Murphy > > > > > <robin.murphy@arm.com> > > > > > 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
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 <German.Rivera@freescale.com> + * 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 <German.Rivera@freescale.com> + * 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 <German.Rivera@freescale.com> + * 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 <German.Rivera@freescale.com> + * 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; }
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 <makarand.pawagi@nxp.com> --- 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(-)