From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [PATCH v5 01/14] drivers: iommu: add FWNODE_IOMMU fwnode type Date: Fri, 30 Sep 2016 17:48:01 +0200 Message-ID: References: <20160909142343.13314-1-lorenzo.pieralisi@arm.com> <20160909142343.13314-2-lorenzo.pieralisi@arm.com> <20160929141520.GA29244@red-moon> <3178073.UTpgCTN6if@vostro.rjw.lan> <20160930090702.GA7725@red-moon> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <20160930090702.GA7725@red-moon> Sender: linux-kernel-owner@vger.kernel.org To: Lorenzo Pieralisi Cc: "Rafael J. Wysocki" , "open list:AMD IOMMU (AMD-VI)" , Joerg Roedel , Will Deacon , Marc Zyngier , Robin Murphy , Tomasz Nowicki , Hanjun Guo , Jon Masters , Eric Auger , Sinan Kaya , Nate Watterson , Prem Mallappa , Dennis Chen , ACPI Devel Maling List , Linux PCI , Linux Kernel Mailing List , "linux-arm-kernel@lists.infradead.org" List-Id: linux-acpi@vger.kernel.org On Fri, Sep 30, 2016 at 11:07 AM, Lorenzo Pieralisi wrote: > On Thu, Sep 29, 2016 at 10:59:40PM +0200, Rafael J. Wysocki wrote: >> On Thursday, September 29, 2016 03:15:20 PM Lorenzo Pieralisi wrote: >> > Hi Rafael, >> > >> > On Fri, Sep 09, 2016 at 03:23:30PM +0100, Lorenzo Pieralisi wrote: >> > > On systems booting with a device tree, every struct device is >> > > associated with a struct device_node, that represents its DT >> > > representation. The device node can be used in generic kernel >> > > contexts (eg IRQ translation, IOMMU streamid mapping), to >> > > retrieve the properties associated with the device and carry >> > > out kernel operation accordingly. Owing to the 1:1 relationship >> > > between the device and its device_node, the device_node can also >> > > be used as a look-up token for the device (eg looking up a device >> > > through its device_node), to retrieve the device in kernel paths >> > > where the device_node is available. >> > > >> > > On systems booting with ACPI, the same abstraction provided by >> > > the device_node is required to provide look-up functionality. >> > > >> > > Therefore, mirroring the approach implemented in the IRQ domain >> > > kernel layer, this patch adds an additional fwnode type FWNODE_IOMMU. >> > > >> > > This patch also implements a glue kernel layer that allows to >> > > allocate/free FWNODE_IOMMU fwnode_handle structures and associate >> > > them with IOMMU devices. >> > > >> > > Signed-off-by: Lorenzo Pieralisi >> > > Reviewed-by: Hanjun Guo >> > > Cc: Joerg Roedel >> > > Cc: "Rafael J. Wysocki" >> > > --- >> > > include/linux/fwnode.h | 1 + >> > > include/linux/iommu.h | 25 +++++++++++++++++++++++++ >> > > 2 files changed, 26 insertions(+) >> > > >> > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h >> > > index 8516717..6e10050 100644 >> > > --- a/include/linux/fwnode.h >> > > +++ b/include/linux/fwnode.h >> > > @@ -19,6 +19,7 @@ enum fwnode_type { >> > > FWNODE_ACPI_DATA, >> > > FWNODE_PDATA, >> > > FWNODE_IRQCHIP, >> > > + FWNODE_IOMMU, >> > >> > This patch provides groundwork for this series and it is key for >> > the rest of it, basically the point here is that we need a fwnode >> > to differentiate platform devices created out of static ACPI tables >> > entries (ie IORT), that represent IOMMU components. >> > >> > The corresponding device is not an ACPI device (I could fabricate one as >> > it is done for other static tables entries eg FADT power button, but I >> > do not necessarily see the reason for doing that given that all we need >> > the fwnode for is a token identifier), so FWNODE_ACPI does not apply >> > here. >> > >> > Please let me know if it is reasonable how I sorted this out (it >> > is basically identical to IRQCHIP, just another enum entry), the >> > remainder of the code depends on this. >> >> I'm not familiar with the use case, so I don't see anything unreasonable >> in it. > > The use case is pretty simple: on ARM SMMU devices are platform devices. > When booting with DT they are identified through an of_node and related > FWNODE_OF type. When booting with ACPI, the ARM SMMU platform devices, > to be equivalent to DT booting path, should be created out of static > IORT table entries (that's how we describe SMMUs); we need to create > a fwnode "token" to associate with those platform devices and that's > not a FWNODE_ACPI (that is for an ACPI device firmware object, here we > really do not need one), so this patch. > >> If you're asking about whether or not I mind adding more fwnode types in >> principle, then no, I don't. :-) > > Yes, that's what I was asking, the only point that bugs me is that for > both FWNODE_IRQCHIP and FWNODE_IOMMU the fwnode is just a "token" (ie a > valid pointer) used for look-up and the type in the fwnode_handle is > mostly there for error checking, I was wondering if we could create a > specific fwnode_type for this specific usage (eg FWNODE_TAG and then add > a type to it as part of its container struct) instead of adding an enum > value per subsystem - it seems there are other fwnode types in the > pipeline :), so I am asking: > > lkml.kernel.org/r/3D1468514043-21081-3-git-send-email-minyard@acm.org OK, I see your concern now, so thanks for presenting it so clearly. While I don't see anything wrong with having per-subsystem fwnode types in principle, I agree that if the only purpose of them is to mean "this comes from ACPI, but from a static table, not from the namespace", it would be better to have a single fwnode type for that, like FWNODE_ACPI_STATIC or similar. Thanks, Rafael From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933799AbcI3PsM (ORCPT ); Fri, 30 Sep 2016 11:48:12 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:35566 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932988AbcI3PsE (ORCPT ); Fri, 30 Sep 2016 11:48:04 -0400 MIME-Version: 1.0 In-Reply-To: <20160930090702.GA7725@red-moon> References: <20160909142343.13314-1-lorenzo.pieralisi@arm.com> <20160909142343.13314-2-lorenzo.pieralisi@arm.com> <20160929141520.GA29244@red-moon> <3178073.UTpgCTN6if@vostro.rjw.lan> <20160930090702.GA7725@red-moon> From: "Rafael J. Wysocki" Date: Fri, 30 Sep 2016 17:48:01 +0200 X-Google-Sender-Auth: sVMQ__rNnNQ0njndfj8FYolFXUo Message-ID: Subject: Re: [PATCH v5 01/14] drivers: iommu: add FWNODE_IOMMU fwnode type To: Lorenzo Pieralisi Cc: "Rafael J. Wysocki" , "open list:AMD IOMMU (AMD-VI)" , Joerg Roedel , Will Deacon , Marc Zyngier , Robin Murphy , Tomasz Nowicki , Hanjun Guo , Jon Masters , Eric Auger , Sinan Kaya , Nate Watterson , Prem Mallappa , Dennis Chen , ACPI Devel Maling List , Linux PCI , Linux Kernel Mailing List , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Sep 30, 2016 at 11:07 AM, Lorenzo Pieralisi wrote: > On Thu, Sep 29, 2016 at 10:59:40PM +0200, Rafael J. Wysocki wrote: >> On Thursday, September 29, 2016 03:15:20 PM Lorenzo Pieralisi wrote: >> > Hi Rafael, >> > >> > On Fri, Sep 09, 2016 at 03:23:30PM +0100, Lorenzo Pieralisi wrote: >> > > On systems booting with a device tree, every struct device is >> > > associated with a struct device_node, that represents its DT >> > > representation. The device node can be used in generic kernel >> > > contexts (eg IRQ translation, IOMMU streamid mapping), to >> > > retrieve the properties associated with the device and carry >> > > out kernel operation accordingly. Owing to the 1:1 relationship >> > > between the device and its device_node, the device_node can also >> > > be used as a look-up token for the device (eg looking up a device >> > > through its device_node), to retrieve the device in kernel paths >> > > where the device_node is available. >> > > >> > > On systems booting with ACPI, the same abstraction provided by >> > > the device_node is required to provide look-up functionality. >> > > >> > > Therefore, mirroring the approach implemented in the IRQ domain >> > > kernel layer, this patch adds an additional fwnode type FWNODE_IOMMU. >> > > >> > > This patch also implements a glue kernel layer that allows to >> > > allocate/free FWNODE_IOMMU fwnode_handle structures and associate >> > > them with IOMMU devices. >> > > >> > > Signed-off-by: Lorenzo Pieralisi >> > > Reviewed-by: Hanjun Guo >> > > Cc: Joerg Roedel >> > > Cc: "Rafael J. Wysocki" >> > > --- >> > > include/linux/fwnode.h | 1 + >> > > include/linux/iommu.h | 25 +++++++++++++++++++++++++ >> > > 2 files changed, 26 insertions(+) >> > > >> > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h >> > > index 8516717..6e10050 100644 >> > > --- a/include/linux/fwnode.h >> > > +++ b/include/linux/fwnode.h >> > > @@ -19,6 +19,7 @@ enum fwnode_type { >> > > FWNODE_ACPI_DATA, >> > > FWNODE_PDATA, >> > > FWNODE_IRQCHIP, >> > > + FWNODE_IOMMU, >> > >> > This patch provides groundwork for this series and it is key for >> > the rest of it, basically the point here is that we need a fwnode >> > to differentiate platform devices created out of static ACPI tables >> > entries (ie IORT), that represent IOMMU components. >> > >> > The corresponding device is not an ACPI device (I could fabricate one as >> > it is done for other static tables entries eg FADT power button, but I >> > do not necessarily see the reason for doing that given that all we need >> > the fwnode for is a token identifier), so FWNODE_ACPI does not apply >> > here. >> > >> > Please let me know if it is reasonable how I sorted this out (it >> > is basically identical to IRQCHIP, just another enum entry), the >> > remainder of the code depends on this. >> >> I'm not familiar with the use case, so I don't see anything unreasonable >> in it. > > The use case is pretty simple: on ARM SMMU devices are platform devices. > When booting with DT they are identified through an of_node and related > FWNODE_OF type. When booting with ACPI, the ARM SMMU platform devices, > to be equivalent to DT booting path, should be created out of static > IORT table entries (that's how we describe SMMUs); we need to create > a fwnode "token" to associate with those platform devices and that's > not a FWNODE_ACPI (that is for an ACPI device firmware object, here we > really do not need one), so this patch. > >> If you're asking about whether or not I mind adding more fwnode types in >> principle, then no, I don't. :-) > > Yes, that's what I was asking, the only point that bugs me is that for > both FWNODE_IRQCHIP and FWNODE_IOMMU the fwnode is just a "token" (ie a > valid pointer) used for look-up and the type in the fwnode_handle is > mostly there for error checking, I was wondering if we could create a > specific fwnode_type for this specific usage (eg FWNODE_TAG and then add > a type to it as part of its container struct) instead of adding an enum > value per subsystem - it seems there are other fwnode types in the > pipeline :), so I am asking: > > lkml.kernel.org/r/3D1468514043-21081-3-git-send-email-minyard@acm.org OK, I see your concern now, so thanks for presenting it so clearly. While I don't see anything wrong with having per-subsystem fwnode types in principle, I agree that if the only purpose of them is to mean "this comes from ACPI, but from a static table, not from the namespace", it would be better to have a single fwnode type for that, like FWNODE_ACPI_STATIC or similar. Thanks, Rafael From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <20160930090702.GA7725@red-moon> References: <20160909142343.13314-1-lorenzo.pieralisi@arm.com> <20160909142343.13314-2-lorenzo.pieralisi@arm.com> <20160929141520.GA29244@red-moon> <3178073.UTpgCTN6if@vostro.rjw.lan> <20160930090702.GA7725@red-moon> From: "Rafael J. Wysocki" Date: Fri, 30 Sep 2016 17:48:01 +0200 Message-ID: Subject: Re: [PATCH v5 01/14] drivers: iommu: add FWNODE_IOMMU fwnode type To: Lorenzo Pieralisi Cc: "Rafael J. Wysocki" , "open list:AMD IOMMU (AMD-VI)" , Joerg Roedel , Will Deacon , Marc Zyngier , Robin Murphy , Tomasz Nowicki , Hanjun Guo , Jon Masters , Eric Auger , Sinan Kaya , Nate Watterson , Prem Mallappa , Dennis Chen , ACPI Devel Maling List , Linux PCI , Linux Kernel Mailing List , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-acpi-owner@vger.kernel.org List-ID: On Fri, Sep 30, 2016 at 11:07 AM, Lorenzo Pieralisi wrote: > On Thu, Sep 29, 2016 at 10:59:40PM +0200, Rafael J. Wysocki wrote: >> On Thursday, September 29, 2016 03:15:20 PM Lorenzo Pieralisi wrote: >> > Hi Rafael, >> > >> > On Fri, Sep 09, 2016 at 03:23:30PM +0100, Lorenzo Pieralisi wrote: >> > > On systems booting with a device tree, every struct device is >> > > associated with a struct device_node, that represents its DT >> > > representation. The device node can be used in generic kernel >> > > contexts (eg IRQ translation, IOMMU streamid mapping), to >> > > retrieve the properties associated with the device and carry >> > > out kernel operation accordingly. Owing to the 1:1 relationship >> > > between the device and its device_node, the device_node can also >> > > be used as a look-up token for the device (eg looking up a device >> > > through its device_node), to retrieve the device in kernel paths >> > > where the device_node is available. >> > > >> > > On systems booting with ACPI, the same abstraction provided by >> > > the device_node is required to provide look-up functionality. >> > > >> > > Therefore, mirroring the approach implemented in the IRQ domain >> > > kernel layer, this patch adds an additional fwnode type FWNODE_IOMMU. >> > > >> > > This patch also implements a glue kernel layer that allows to >> > > allocate/free FWNODE_IOMMU fwnode_handle structures and associate >> > > them with IOMMU devices. >> > > >> > > Signed-off-by: Lorenzo Pieralisi >> > > Reviewed-by: Hanjun Guo >> > > Cc: Joerg Roedel >> > > Cc: "Rafael J. Wysocki" >> > > --- >> > > include/linux/fwnode.h | 1 + >> > > include/linux/iommu.h | 25 +++++++++++++++++++++++++ >> > > 2 files changed, 26 insertions(+) >> > > >> > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h >> > > index 8516717..6e10050 100644 >> > > --- a/include/linux/fwnode.h >> > > +++ b/include/linux/fwnode.h >> > > @@ -19,6 +19,7 @@ enum fwnode_type { >> > > FWNODE_ACPI_DATA, >> > > FWNODE_PDATA, >> > > FWNODE_IRQCHIP, >> > > + FWNODE_IOMMU, >> > >> > This patch provides groundwork for this series and it is key for >> > the rest of it, basically the point here is that we need a fwnode >> > to differentiate platform devices created out of static ACPI tables >> > entries (ie IORT), that represent IOMMU components. >> > >> > The corresponding device is not an ACPI device (I could fabricate one as >> > it is done for other static tables entries eg FADT power button, but I >> > do not necessarily see the reason for doing that given that all we need >> > the fwnode for is a token identifier), so FWNODE_ACPI does not apply >> > here. >> > >> > Please let me know if it is reasonable how I sorted this out (it >> > is basically identical to IRQCHIP, just another enum entry), the >> > remainder of the code depends on this. >> >> I'm not familiar with the use case, so I don't see anything unreasonable >> in it. > > The use case is pretty simple: on ARM SMMU devices are platform devices. > When booting with DT they are identified through an of_node and related > FWNODE_OF type. When booting with ACPI, the ARM SMMU platform devices, > to be equivalent to DT booting path, should be created out of static > IORT table entries (that's how we describe SMMUs); we need to create > a fwnode "token" to associate with those platform devices and that's > not a FWNODE_ACPI (that is for an ACPI device firmware object, here we > really do not need one), so this patch. > >> If you're asking about whether or not I mind adding more fwnode types in >> principle, then no, I don't. :-) > > Yes, that's what I was asking, the only point that bugs me is that for > both FWNODE_IRQCHIP and FWNODE_IOMMU the fwnode is just a "token" (ie a > valid pointer) used for look-up and the type in the fwnode_handle is > mostly there for error checking, I was wondering if we could create a > specific fwnode_type for this specific usage (eg FWNODE_TAG and then add > a type to it as part of its container struct) instead of adding an enum > value per subsystem - it seems there are other fwnode types in the > pipeline :), so I am asking: > > lkml.kernel.org/r/3D1468514043-21081-3-git-send-email-minyard@acm.org OK, I see your concern now, so thanks for presenting it so clearly. While I don't see anything wrong with having per-subsystem fwnode types in principle, I agree that if the only purpose of them is to mean "this comes from ACPI, but from a static table, not from the namespace", it would be better to have a single fwnode type for that, like FWNODE_ACPI_STATIC or similar. Thanks, Rafael From mboxrd@z Thu Jan 1 00:00:00 1970 From: rafael@kernel.org (Rafael J. Wysocki) Date: Fri, 30 Sep 2016 17:48:01 +0200 Subject: [PATCH v5 01/14] drivers: iommu: add FWNODE_IOMMU fwnode type In-Reply-To: <20160930090702.GA7725@red-moon> References: <20160909142343.13314-1-lorenzo.pieralisi@arm.com> <20160909142343.13314-2-lorenzo.pieralisi@arm.com> <20160929141520.GA29244@red-moon> <3178073.UTpgCTN6if@vostro.rjw.lan> <20160930090702.GA7725@red-moon> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Sep 30, 2016 at 11:07 AM, Lorenzo Pieralisi wrote: > On Thu, Sep 29, 2016 at 10:59:40PM +0200, Rafael J. Wysocki wrote: >> On Thursday, September 29, 2016 03:15:20 PM Lorenzo Pieralisi wrote: >> > Hi Rafael, >> > >> > On Fri, Sep 09, 2016 at 03:23:30PM +0100, Lorenzo Pieralisi wrote: >> > > On systems booting with a device tree, every struct device is >> > > associated with a struct device_node, that represents its DT >> > > representation. The device node can be used in generic kernel >> > > contexts (eg IRQ translation, IOMMU streamid mapping), to >> > > retrieve the properties associated with the device and carry >> > > out kernel operation accordingly. Owing to the 1:1 relationship >> > > between the device and its device_node, the device_node can also >> > > be used as a look-up token for the device (eg looking up a device >> > > through its device_node), to retrieve the device in kernel paths >> > > where the device_node is available. >> > > >> > > On systems booting with ACPI, the same abstraction provided by >> > > the device_node is required to provide look-up functionality. >> > > >> > > Therefore, mirroring the approach implemented in the IRQ domain >> > > kernel layer, this patch adds an additional fwnode type FWNODE_IOMMU. >> > > >> > > This patch also implements a glue kernel layer that allows to >> > > allocate/free FWNODE_IOMMU fwnode_handle structures and associate >> > > them with IOMMU devices. >> > > >> > > Signed-off-by: Lorenzo Pieralisi >> > > Reviewed-by: Hanjun Guo >> > > Cc: Joerg Roedel >> > > Cc: "Rafael J. Wysocki" >> > > --- >> > > include/linux/fwnode.h | 1 + >> > > include/linux/iommu.h | 25 +++++++++++++++++++++++++ >> > > 2 files changed, 26 insertions(+) >> > > >> > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h >> > > index 8516717..6e10050 100644 >> > > --- a/include/linux/fwnode.h >> > > +++ b/include/linux/fwnode.h >> > > @@ -19,6 +19,7 @@ enum fwnode_type { >> > > FWNODE_ACPI_DATA, >> > > FWNODE_PDATA, >> > > FWNODE_IRQCHIP, >> > > + FWNODE_IOMMU, >> > >> > This patch provides groundwork for this series and it is key for >> > the rest of it, basically the point here is that we need a fwnode >> > to differentiate platform devices created out of static ACPI tables >> > entries (ie IORT), that represent IOMMU components. >> > >> > The corresponding device is not an ACPI device (I could fabricate one as >> > it is done for other static tables entries eg FADT power button, but I >> > do not necessarily see the reason for doing that given that all we need >> > the fwnode for is a token identifier), so FWNODE_ACPI does not apply >> > here. >> > >> > Please let me know if it is reasonable how I sorted this out (it >> > is basically identical to IRQCHIP, just another enum entry), the >> > remainder of the code depends on this. >> >> I'm not familiar with the use case, so I don't see anything unreasonable >> in it. > > The use case is pretty simple: on ARM SMMU devices are platform devices. > When booting with DT they are identified through an of_node and related > FWNODE_OF type. When booting with ACPI, the ARM SMMU platform devices, > to be equivalent to DT booting path, should be created out of static > IORT table entries (that's how we describe SMMUs); we need to create > a fwnode "token" to associate with those platform devices and that's > not a FWNODE_ACPI (that is for an ACPI device firmware object, here we > really do not need one), so this patch. > >> If you're asking about whether or not I mind adding more fwnode types in >> principle, then no, I don't. :-) > > Yes, that's what I was asking, the only point that bugs me is that for > both FWNODE_IRQCHIP and FWNODE_IOMMU the fwnode is just a "token" (ie a > valid pointer) used for look-up and the type in the fwnode_handle is > mostly there for error checking, I was wondering if we could create a > specific fwnode_type for this specific usage (eg FWNODE_TAG and then add > a type to it as part of its container struct) instead of adding an enum > value per subsystem - it seems there are other fwnode types in the > pipeline :), so I am asking: > > lkml.kernel.org/r/3D1468514043-21081-3-git-send-email-minyard at acm.org OK, I see your concern now, so thanks for presenting it so clearly. While I don't see anything wrong with having per-subsystem fwnode types in principle, I agree that if the only purpose of them is to mean "this comes from ACPI, but from a static table, not from the namespace", it would be better to have a single fwnode type for that, like FWNODE_ACPI_STATIC or similar. Thanks, Rafael