[v5,01/14] drivers: iommu: add FWNODE_IOMMU fwnode type

Commit Message

Lorenzo Pieralisi Sept. 9, 2016, 2:23 p.m. UTC

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 <[e-mail]>
Reviewed-by: Hanjun Guo <[e-mail]>
Cc: Joerg Roedel <[e-mail]>
Cc: "Rafael J. Wysocki" <[e-mail]>
---
 include/linux/fwnode.h |  1 +
 include/linux/iommu.h  | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

Comments

Lorenzo Pieralisi Sept. 29, 2016, 2:15 p.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Hanjun Guo <[e-mail]>
> Cc: Joerg Roedel <[e-mail]>
> Cc: "Rafael J. Wysocki" <[e-mail]>
> ---
>  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.

Thanks !
Lorenzo

>  };
>  
>  struct fwnode_handle {
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index a35fb8b..6456528 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -38,6 +38,7 @@ struct bus_type;
>  struct device;
>  struct iommu_domain;
>  struct notifier_block;
> +struct fwnode_handle;
>  
>  /* iommu fault flags */
>  #define IOMMU_FAULT_READ	0x0
> @@ -543,4 +544,28 @@ static inline void iommu_device_unlink(struct device *dev, struct device *link)
>  
>  #endif /* CONFIG_IOMMU_API */
>  
> +/* IOMMU fwnode handling */
> +static inline bool is_fwnode_iommu(struct fwnode_handle *fwnode)
> +{
> +	return fwnode && fwnode->type == FWNODE_IOMMU;
> +}
> +
> +static inline struct fwnode_handle *iommu_alloc_fwnode(void)
> +{
> +	struct fwnode_handle *fwnode;
> +
> +	fwnode = kzalloc(sizeof(struct fwnode_handle), GFP_KERNEL);
> +	fwnode->type = FWNODE_IOMMU;
> +
> +	return fwnode;
> +}
> +
> +static inline void iommu_free_fwnode(struct fwnode_handle *fwnode)
> +{
> +	if (WARN_ON(!is_fwnode_iommu(fwnode)))
> +		return;
> +
> +	kfree(fwnode);
> +}
> +
>  #endif /* __LINUX_IOMMU_H */
> -- 
> 2.10.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki Sept. 29, 2016, 8:59 p.m. UTC | #2

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 <[e-mail]>
> > Reviewed-by: Hanjun Guo <[e-mail]>
> > Cc: Joerg Roedel <[e-mail]>
> > Cc: "Rafael J. Wysocki" <[e-mail]>
> > ---
> >  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.

If you're asking about whether or not I mind adding more fwnode types in
principle, then no, I don't. :-) 

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi Sept. 30, 2016, 9:07 a.m. UTC | #3

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 <[e-mail]>
> > > Reviewed-by: Hanjun Guo <[e-mail]>
> > > Cc: Joerg Roedel <[e-mail]>
> > > Cc: "Rafael J. Wysocki" <[e-mail]>
> > > ---
> > >  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/[e-mail]

If it is ok for you and Joerg I will go ahead with current patch
keeping in mind that the above should not be that complicated to
implement if we deem it reasonable.

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki Sept. 30, 2016, 3:48 p.m. UTC | #4

On Fri, Sep 30, 2016 at 11:07 AM, Lorenzo Pieralisi
<[e-mail]> 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 <[e-mail]>
>> > > Reviewed-by: Hanjun Guo <[e-mail]>
>> > > Cc: Joerg Roedel <[e-mail]>
>> > > Cc: "Rafael J. Wysocki" <[e-mail]>
>> > > ---
>> > >  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/[e-mail]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi Oct. 13, 2016, 4:32 p.m. UTC | #5

Hi Rafael,

On Fri, Sep 30, 2016 at 05:48:01PM +0200, Rafael J. Wysocki wrote:
> On Fri, Sep 30, 2016 at 11:07 AM, Lorenzo Pieralisi
> <[e-mail]> 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 <[e-mail]>
> >> > > Reviewed-by: Hanjun Guo <[e-mail]>
> >> > > Cc: Joerg Roedel <[e-mail]>
> >> > > Cc: "Rafael J. Wysocki" <[e-mail]>
> >> > > ---
> >> > >  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/[e-mail]
> 
> 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.

Coming back to this, I updated the series with new fwnode type
FWNODE_ACPI_STATIC, which IMHO makes more sense (because that
represents the FW interface it was obtained from rather than
its content and plays better with upcoming extension above - DMI is a
different firmware interface so it will be represented with a different
fwnode type). Thanks.

However, I still have a question. The approach I took (create platform
devices out of static IORT table entries for SMMUs) is common in ACPI
(eg GHES, ACPI watchdog) even though those subsystems ignore the fwnode,
but I think that's a detail.

Still, fixed HW like power button and sleep button took a different
approach, which consists in creating struct acpi_device objects out
of FADT fixed HW features (with a NULL ACPI handle because there is
no real ACPI object in the namespace for them).

I would like to understand the reasoning behind the difference (I
think it is related to notification events and the need for an
ACPI object for them - and sysfs userspace HID IF exposure ?).

In theory (but looks crazy to me that's why I did not do it), I could
fabricate a Device object in the ACPI namespace (?) (with _HID, _CRS and
related properties - is that doable ?) out of the static table entry in
the IORT table that provides the ARM SMMU component data (ie its MMIO
space, IRQs and SMMU properties like cache coherency), this would
allow the kernel to create a struct acpi_device (and related fwnode)
+ its physical node platform device but that looks insanely complicated
(if feasible and more importantly if correct from an ACPI standpoint).

This approach would allow me to match the SMMU driver with an _HID,
the kernel would create a physical_node (ie platform_device) for
me out of the namespace ACPI device object and I would get the
FWNODE_ACPI for free (out of the struct acpi_device) instead of having
to fiddle about with a new fwnode_handle type.

I think this alternative approach (if doable at all) creates more issues
than it solves but I wanted to make sure what I am doing is kosher
from an ACPI perspective so I am asking.

I definitely think the current approach I took is much better, with its
own downsides (eg matching the ARM SMMU driver by name instead of
acpi_device_id/_HID), but I wanted to ask.

The point is: ARM SMMU drivers are platform drivers. In DT the SMMUs
are represented through DT nodes, in ACPI through _static_ IORT table
entries, somehow a platform device must be created for them, so
this whole series (and related fwnode issues).

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki Oct. 13, 2016, 8:53 p.m. UTC | #6

Hi,

On Thu, Oct 13, 2016 at 6:32 PM, Lorenzo Pieralisi
<[e-mail]> wrote:
> Hi Rafael,
>
> On Fri, Sep 30, 2016 at 05:48:01PM +0200, Rafael J. Wysocki wrote:
>> On Fri, Sep 30, 2016 at 11:07 AM, Lorenzo Pieralisi
>> <[e-mail]> 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 <[e-mail]>
>> >> > > Reviewed-by: Hanjun Guo <[e-mail]>
>> >> > > Cc: Joerg Roedel <[e-mail]>
>> >> > > Cc: "Rafael J. Wysocki" <[e-mail]>
>> >> > > ---
>> >> > >  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/[e-mail]
>>
>> 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.
>
> Coming back to this, I updated the series with new fwnode type
> FWNODE_ACPI_STATIC, which IMHO makes more sense (because that
> represents the FW interface it was obtained from rather than
> its content and plays better with upcoming extension above - DMI is a
> different firmware interface so it will be represented with a different
> fwnode type). Thanks.

OK

> However, I still have a question. The approach I took (create platform
> devices out of static IORT table entries for SMMUs) is common in ACPI
> (eg GHES, ACPI watchdog) even though those subsystems ignore the fwnode,
> but I think that's a detail.
>
> Still, fixed HW like power button and sleep button took a different
> approach, which consists in creating struct acpi_device objects out
> of FADT fixed HW features (with a NULL ACPI handle because there is
> no real ACPI object in the namespace for them).

That's how it was done in the past and the code is not functionally
problematic, so I saw no reason to re-implement it (which might
introduce regressions).

> I would like to understand the reasoning behind the difference (I
> think it is related to notification events and the need for an
> ACPI object for them - and sysfs userspace HID IF exposure ?).

I don't think there is a real need for that model and that's why it is
not used any more.  It's legacy mostly.

> In theory (but looks crazy to me that's why I did not do it), I could
> fabricate a Device object in the ACPI namespace (?) (with _HID, _CRS and
> related properties - is that doable ?) out of the static table entry in
> the IORT table that provides the ARM SMMU component data (ie its MMIO
> space, IRQs and SMMU properties like cache coherency), this would
> allow the kernel to create a struct acpi_device (and related fwnode)
> + its physical node platform device but that looks insanely complicated
> (if feasible and more importantly if correct from an ACPI standpoint).
>
> This approach would allow me to match the SMMU driver with an _HID,
> the kernel would create a physical_node (ie platform_device) for
> me out of the namespace ACPI device object and I would get the
> FWNODE_ACPI for free (out of the struct acpi_device) instead of having
> to fiddle about with a new fwnode_handle type.
>
> I think this alternative approach (if doable at all) creates more issues
> than it solves but I wanted to make sure what I am doing is kosher
> from an ACPI perspective so I am asking.

That's most likely how I would do it, so fine by me. :-)

> I definitely think the current approach I took is much better, with its
> own downsides (eg matching the ARM SMMU driver by name instead of
> acpi_device_id/_HID), but I wanted to ask.
>
> The point is: ARM SMMU drivers are platform drivers. In DT the SMMUs
> are represented through DT nodes, in ACPI through _static_ IORT table
> entries, somehow a platform device must be created for them, so
> this whole series (and related fwnode issues).

Right.  No disagreement.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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,
 };
 
 struct fwnode_handle {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a35fb8b..6456528 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -38,6 +38,7 @@  struct bus_type;
 struct device;
 struct iommu_domain;
 struct notifier_block;
+struct fwnode_handle;
 
 /* iommu fault flags */
 #define IOMMU_FAULT_READ	0x0
@@ -543,4 +544,28 @@  static inline void iommu_device_unlink(struct device *dev, struct device *link)
 
 #endif /* CONFIG_IOMMU_API */
 
+/* IOMMU fwnode handling */
+static inline bool is_fwnode_iommu(struct fwnode_handle *fwnode)
+{
+	return fwnode && fwnode->type == FWNODE_IOMMU;
+}
+
+static inline struct fwnode_handle *iommu_alloc_fwnode(void)
+{
+	struct fwnode_handle *fwnode;
+
+	fwnode = kzalloc(sizeof(struct fwnode_handle), GFP_KERNEL);
+	fwnode->type = FWNODE_IOMMU;
+
+	return fwnode;
+}
+
+static inline void iommu_free_fwnode(struct fwnode_handle *fwnode)
+{
+	if (WARN_ON(!is_fwnode_iommu(fwnode)))
+		return;
+
+	kfree(fwnode);
+}
+
 #endif /* __LINUX_IOMMU_H */