* [PATCH v2 0/3] Remove one more platform_device_add_properties() call
@ 2021-01-11 14:10 Heikki Krogerus
2021-01-11 14:10 ` [PATCH v2 1/3] software node: Introduce device_add_software_node() Heikki Krogerus
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Heikki Krogerus @ 2021-01-11 14:10 UTC (permalink / raw)
To: Felipe Balbi, Rafael J. Wysocki
Cc: Andy Shevchenko, linux-kernel, linux-usb, linux-acpi
Hi Felipe, Rafael,
This is the second version of this series. There are no real changes,
but I added the Tiger Lake ID patch to this series in hope that it
will make your life a bit easier, assuming that Rafael will still pick
these.
The original over letter:
I originally introduced these as part of my series where I was
proposing PM ops for software nodes [1], but since that still needs
work, I'm sending these two separately.
So basically I'm only modifying dwc3-pci.c so it registers a software
node directly at this point. That will remove one more user of
platform_device_add_properties().
[1] https://lore.kernel.org/lkml/20201029105941.63410-1-heikki.krogerus@linux.intel.com/
thanks,
Heikki Krogerus (3):
software node: Introduce device_add_software_node()
usb: dwc3: pci: Register a software node for the dwc3 platform device
usb: dwc3: pci: ID for Tiger Lake CPU
drivers/base/swnode.c | 69 ++++++++++++++++++++++++++++++++-----
drivers/usb/dwc3/dwc3-pci.c | 65 +++++++++++++++++++++-------------
include/linux/property.h | 3 ++
3 files changed, 104 insertions(+), 33 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v2 1/3] software node: Introduce device_add_software_node() 2021-01-11 14:10 [PATCH v2 0/3] Remove one more platform_device_add_properties() call Heikki Krogerus @ 2021-01-11 14:10 ` Heikki Krogerus 2021-01-13 0:40 ` Daniel Scally 2021-01-11 14:10 ` [PATCH v2 2/3] usb: dwc3: pci: Register a software node for the dwc3 platform device Heikki Krogerus ` (3 subsequent siblings) 4 siblings, 1 reply; 16+ messages in thread From: Heikki Krogerus @ 2021-01-11 14:10 UTC (permalink / raw) To: Felipe Balbi, Rafael J. Wysocki Cc: Andy Shevchenko, linux-kernel, linux-usb, linux-acpi This helper will register a software node and then assign it to device at the same time. The function will also make sure that the device can't have more than one software node. Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/base/swnode.c | 69 ++++++++++++++++++++++++++++++++++------ include/linux/property.h | 3 ++ 2 files changed, 63 insertions(+), 9 deletions(-) diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 4a4b2008fbc26..60a8501a4976c 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -48,6 +48,19 @@ EXPORT_SYMBOL_GPL(is_software_node); struct swnode, fwnode) : NULL; \ }) +static inline struct swnode *dev_to_swnode(struct device *dev) +{ + struct fwnode_handle *fwnode = dev_fwnode(dev); + + if (!fwnode) + return NULL; + + if (!is_software_node(fwnode)) + fwnode = fwnode->secondary; + + return to_swnode(fwnode); +} + static struct swnode * software_node_to_swnode(const struct software_node *node) { @@ -843,22 +856,60 @@ void fwnode_remove_software_node(struct fwnode_handle *fwnode) } EXPORT_SYMBOL_GPL(fwnode_remove_software_node); +/** + * device_add_software_node - Assign software node to a device + * @dev: The device the software node is meant for. + * @swnode: The software node. + * + * This function will register @swnode and make it the secondary firmware node + * pointer of @dev. If @dev has no primary node, then @swnode will become the primary + * node. + */ +int device_add_software_node(struct device *dev, const struct software_node *swnode) +{ + int ret; + + /* Only one software node per device. */ + if (dev_to_swnode(dev)) + return -EBUSY; + + ret = software_node_register(swnode); + if (ret) + return ret; + + set_secondary_fwnode(dev, software_node_fwnode(swnode)); + + return 0; +} +EXPORT_SYMBOL_GPL(device_add_software_node); + +/** + * device_remove_software_node - Remove device's software node + * @dev: The device with the software node. + * + * This function will unregister the software node of @dev. + */ +void device_remove_software_node(struct device *dev) +{ + struct swnode *swnode; + + swnode = dev_to_swnode(dev); + if (!swnode) + return; + + kobject_put(&swnode->kobj); +} +EXPORT_SYMBOL_GPL(device_remove_software_node); + int software_node_notify(struct device *dev, unsigned long action) { - struct fwnode_handle *fwnode = dev_fwnode(dev); struct swnode *swnode; int ret; - if (!fwnode) - return 0; - - if (!is_software_node(fwnode)) - fwnode = fwnode->secondary; - if (!is_software_node(fwnode)) + swnode = dev_to_swnode(dev); + if (!swnode) return 0; - swnode = to_swnode(fwnode); - switch (action) { case KOBJ_ADD: ret = sysfs_create_link(&dev->kobj, &swnode->kobj, diff --git a/include/linux/property.h b/include/linux/property.h index 0a9001fe7aeab..b0e413dc59271 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -488,4 +488,7 @@ fwnode_create_software_node(const struct property_entry *properties, const struct fwnode_handle *parent); void fwnode_remove_software_node(struct fwnode_handle *fwnode); +int device_add_software_node(struct device *dev, const struct software_node *swnode); +void device_remove_software_node(struct device *dev); + #endif /* _LINUX_PROPERTY_H_ */ -- 2.29.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] software node: Introduce device_add_software_node() 2021-01-11 14:10 ` [PATCH v2 1/3] software node: Introduce device_add_software_node() Heikki Krogerus @ 2021-01-13 0:40 ` Daniel Scally 2021-01-13 11:39 ` Heikki Krogerus 2021-01-13 15:55 ` Andy Shevchenko 0 siblings, 2 replies; 16+ messages in thread From: Daniel Scally @ 2021-01-13 0:40 UTC (permalink / raw) To: Heikki Krogerus, Felipe Balbi, Rafael J. Wysocki Cc: Andy Shevchenko, linux-kernel, linux-usb, linux-acpi Hi Heikki On 11/01/2021 14:10, Heikki Krogerus wrote: > This helper will register a software node and then assign > it to device at the same time. The function will also make > sure that the device can't have more than one software node. > > Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- I like this change. One comment below, but for what it's worth: Reviewed-by: Daniel Scally <djrscally@gmail.com> > +/** > + * device_remove_software_node - Remove device's software node > + * @dev: The device with the software node. > + * > + * This function will unregister the software node of @dev. > + */ > +void device_remove_software_node(struct device *dev) > +{ > + struct swnode *swnode; > + > + swnode = dev_to_swnode(dev); > + if (!swnode) > + return; > + > + kobject_put(&swnode->kobj); > +} > +EXPORT_SYMBOL_GPL(device_remove_software_node); I wonder if this also ought to set dev_fwnode(dev)->secondary back to ERR_PTR(-ENODEV)? > + > int software_node_notify(struct device *dev, unsigned long action) > { > - struct fwnode_handle *fwnode = dev_fwnode(dev); > struct swnode *swnode; > int ret; > > - if (!fwnode) > - return 0; > - > - if (!is_software_node(fwnode)) > - fwnode = fwnode->secondary; > - if (!is_software_node(fwnode)) > + swnode = dev_to_swnode(dev); > + if (!swnode) > return 0; > > - swnode = to_swnode(fwnode); > - > switch (action) { > case KOBJ_ADD: > ret = sysfs_create_link(&dev->kobj, &swnode->kobj, > diff --git a/include/linux/property.h b/include/linux/property.h > index 0a9001fe7aeab..b0e413dc59271 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -488,4 +488,7 @@ fwnode_create_software_node(const struct property_entry *properties, > const struct fwnode_handle *parent); > void fwnode_remove_software_node(struct fwnode_handle *fwnode); > > +int device_add_software_node(struct device *dev, const struct software_node *swnode); > +void device_remove_software_node(struct device *dev); > + > #endif /* _LINUX_PROPERTY_H_ */ > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] software node: Introduce device_add_software_node() 2021-01-13 0:40 ` Daniel Scally @ 2021-01-13 11:39 ` Heikki Krogerus 2021-01-13 15:30 ` Andy Shevchenko 2021-01-13 15:55 ` Andy Shevchenko 1 sibling, 1 reply; 16+ messages in thread From: Heikki Krogerus @ 2021-01-13 11:39 UTC (permalink / raw) To: Daniel Scally Cc: Felipe Balbi, Rafael J. Wysocki, Andy Shevchenko, linux-kernel, linux-usb, linux-acpi Hi Daniel, On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote: > Hi Heikki > > On 11/01/2021 14:10, Heikki Krogerus wrote: > > This helper will register a software node and then assign > > it to device at the same time. The function will also make > > sure that the device can't have more than one software node. > > > > Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > --- > > I like this change. One comment below, but for what it's worth: > > Reviewed-by: Daniel Scally <djrscally@gmail.com> Thanks! > > +/** > > + * device_remove_software_node - Remove device's software node > > + * @dev: The device with the software node. > > + * > > + * This function will unregister the software node of @dev. > > + */ > > +void device_remove_software_node(struct device *dev) > > +{ > > + struct swnode *swnode; > > + > > + swnode = dev_to_swnode(dev); > > + if (!swnode) > > + return; > > + > > + kobject_put(&swnode->kobj); > > +} > > +EXPORT_SYMBOL_GPL(device_remove_software_node); > > I wonder if this also ought to set dev_fwnode(dev)->secondary back to > ERR_PTR(-ENODEV)? We can't do that here unfortunately. Other places still have a reference to the swnode at this point and they may still need to access it using the dev_fwnode(dev)->secondary pointer. -- heikki ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] software node: Introduce device_add_software_node() 2021-01-13 11:39 ` Heikki Krogerus @ 2021-01-13 15:30 ` Andy Shevchenko 2021-01-14 13:19 ` Heikki Krogerus 0 siblings, 1 reply; 16+ messages in thread From: Andy Shevchenko @ 2021-01-13 15:30 UTC (permalink / raw) To: Heikki Krogerus Cc: Daniel Scally, Felipe Balbi, Rafael J. Wysocki, linux-kernel, linux-usb, linux-acpi On Wed, Jan 13, 2021 at 01:39:18PM +0200, Heikki Krogerus wrote: > On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote: > > On 11/01/2021 14:10, Heikki Krogerus wrote: ... > > > +/** > > > + * device_remove_software_node - Remove device's software node > > > + * @dev: The device with the software node. > > > + * > > > + * This function will unregister the software node of @dev. > > > + */ > > > +void device_remove_software_node(struct device *dev) > > > +{ > > > + struct swnode *swnode; > > > + > > > + swnode = dev_to_swnode(dev); > > > + if (!swnode) > > > + return; > > > + > > > + kobject_put(&swnode->kobj); > > > +} > > > +EXPORT_SYMBOL_GPL(device_remove_software_node); > > > > I wonder if this also ought to set dev_fwnode(dev)->secondary back to > > ERR_PTR(-ENODEV)? Actually it's a good question. > We can't do that here unfortunately. Other places still have a > reference to the swnode at this point and they may still need to > access it using the dev_fwnode(dev)->secondary pointer. Yeah, but in this case we potentially leave a dangling pointer when last of the user gone and kobject_put() will call for release. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] software node: Introduce device_add_software_node() 2021-01-13 15:30 ` Andy Shevchenko @ 2021-01-14 13:19 ` Heikki Krogerus 2021-01-14 14:24 ` Heikki Krogerus 0 siblings, 1 reply; 16+ messages in thread From: Heikki Krogerus @ 2021-01-14 13:19 UTC (permalink / raw) To: Andy Shevchenko Cc: Daniel Scally, Felipe Balbi, Rafael J. Wysocki, linux-kernel, linux-usb, linux-acpi On Wed, Jan 13, 2021 at 05:30:03PM +0200, Andy Shevchenko wrote: > On Wed, Jan 13, 2021 at 01:39:18PM +0200, Heikki Krogerus wrote: > > On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote: > > > On 11/01/2021 14:10, Heikki Krogerus wrote: > > ... > > > > > +/** > > > > + * device_remove_software_node - Remove device's software node > > > > + * @dev: The device with the software node. > > > > + * > > > > + * This function will unregister the software node of @dev. > > > > + */ > > > > +void device_remove_software_node(struct device *dev) > > > > +{ > > > > + struct swnode *swnode; > > > > + > > > > + swnode = dev_to_swnode(dev); > > > > + if (!swnode) > > > > + return; > > > > + > > > > + kobject_put(&swnode->kobj); > > > > +} > > > > +EXPORT_SYMBOL_GPL(device_remove_software_node); > > > > > > I wonder if this also ought to set dev_fwnode(dev)->secondary back to > > > ERR_PTR(-ENODEV)? > > Actually it's a good question. > > > We can't do that here unfortunately. Other places still have a > > reference to the swnode at this point and they may still need to > > access it using the dev_fwnode(dev)->secondary pointer. > > Yeah, but in this case we potentially leave a dangling pointer when last of the > user gone and kobject_put() will call for release. The caller has to be responsible of setting the secondary back to ERR_PTR(-ENODEV). We can not do anything here like I explained. We can not even do that in software_node_notify() when the association to the struct device is removed, because the fwnode->secondary is still accessed after that. The caller needs to remove both the node and the device, and only after that it is safe to set the secondary back to ERR_PTR(-ENODEV). This clearly needs to explained in the comment... I'll fix it. thanks, -- heikki ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] software node: Introduce device_add_software_node() 2021-01-14 13:19 ` Heikki Krogerus @ 2021-01-14 14:24 ` Heikki Krogerus 0 siblings, 0 replies; 16+ messages in thread From: Heikki Krogerus @ 2021-01-14 14:24 UTC (permalink / raw) To: Andy Shevchenko Cc: Daniel Scally, Felipe Balbi, Rafael J. Wysocki, linux-kernel, linux-usb, linux-acpi On Thu, Jan 14, 2021 at 03:19:52PM +0200, Heikki Krogerus wrote: > On Wed, Jan 13, 2021 at 05:30:03PM +0200, Andy Shevchenko wrote: > > On Wed, Jan 13, 2021 at 01:39:18PM +0200, Heikki Krogerus wrote: > > > On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote: > > > > On 11/01/2021 14:10, Heikki Krogerus wrote: > > > > ... > > > > > > > +/** > > > > > + * device_remove_software_node - Remove device's software node > > > > > + * @dev: The device with the software node. > > > > > + * > > > > > + * This function will unregister the software node of @dev. > > > > > + */ > > > > > +void device_remove_software_node(struct device *dev) > > > > > +{ > > > > > + struct swnode *swnode; > > > > > + > > > > > + swnode = dev_to_swnode(dev); > > > > > + if (!swnode) > > > > > + return; > > > > > + > > > > > + kobject_put(&swnode->kobj); > > > > > +} > > > > > +EXPORT_SYMBOL_GPL(device_remove_software_node); > > > > > > > > I wonder if this also ought to set dev_fwnode(dev)->secondary back to > > > > ERR_PTR(-ENODEV)? > > > > Actually it's a good question. > > > > > We can't do that here unfortunately. Other places still have a > > > reference to the swnode at this point and they may still need to > > > access it using the dev_fwnode(dev)->secondary pointer. > > > > Yeah, but in this case we potentially leave a dangling pointer when last of the > > user gone and kobject_put() will call for release. > > The caller has to be responsible of setting the secondary back to > ERR_PTR(-ENODEV). We can not do anything here like I explained. We can > not even do that in software_node_notify() when the association to the > struct device is removed, because the fwnode->secondary is still > accessed after that. The caller needs to remove both the node and the > device, and only after that it is safe to set the secondary back to > ERR_PTR(-ENODEV). I studied the code again, and it actually looks like this is only a problem when device_add_properties() is used and there is an expectation that the node/properties are removed automatically in device_del(). When this new API is used, the only place that needs to access the swnode using the secondary pointer is software_node_notify(), so if we simply handle that separately here, we should be able to clear the secondary pointer after all. It would look something like this: void device_remove_software_node(struct device *dev) { struct swnode *swnode; swnode = dev_to_swnode(dev); if (!swnode) return; software_node_notify(dev, KOBJ_REMOVE); set_secondary_fwnode(dev, NULL); kobject_put(&swnode->kobj); } I'll test that, and if it works, and you guys don't see any problems with it, I'll use it in v3. Br, -- heikki ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] software node: Introduce device_add_software_node() 2021-01-13 0:40 ` Daniel Scally 2021-01-13 11:39 ` Heikki Krogerus @ 2021-01-13 15:55 ` Andy Shevchenko 2021-01-13 15:58 ` Andy Shevchenko 1 sibling, 1 reply; 16+ messages in thread From: Andy Shevchenko @ 2021-01-13 15:55 UTC (permalink / raw) To: Daniel Scally Cc: Heikki Krogerus, Felipe Balbi, Rafael J. Wysocki, linux-kernel, linux-usb, linux-acpi On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote: > On 11/01/2021 14:10, Heikki Krogerus wrote: ... > > +/** > > + * device_remove_software_node - Remove device's software node > > + * @dev: The device with the software node. > > + * > > + * This function will unregister the software node of @dev. > > + */ > > +void device_remove_software_node(struct device *dev) > > +{ > > + struct swnode *swnode; > > + > > + swnode = dev_to_swnode(dev); > > + if (!swnode) > > + return; > > + > > + kobject_put(&swnode->kobj); > > +} > > +EXPORT_SYMBOL_GPL(device_remove_software_node); > > I wonder if this also ought to set dev_fwnode(dev)->secondary back to > ERR_PTR(-ENODEV)? Looking more into this code I think we need to call set_secondary_fwnode(dev, NULL); among these lines. The real problem is that set_primary_fwnode() and set_secondary_fwnode() have no reference counting. If we have a chain ->primary->secondary->-ENODEV is being used somewhere we can't tell from here. So, in practice it means that we lack of some kind of primary node to increment reference count of the secondary node when the latter is chained to the given primary. But it makes things too complicated. Any other options for shared primary-secondary chain? Standalone primary along with standalone (exclusive) secondary doesn't need this AFAICS. Perhaps a flag to primary like shared / exclusive that will prevent breaking the chain in set_secondary_fwnode()? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] software node: Introduce device_add_software_node() 2021-01-13 15:55 ` Andy Shevchenko @ 2021-01-13 15:58 ` Andy Shevchenko 2021-01-14 14:00 ` Heikki Krogerus 0 siblings, 1 reply; 16+ messages in thread From: Andy Shevchenko @ 2021-01-13 15:58 UTC (permalink / raw) To: Daniel Scally Cc: Heikki Krogerus, Felipe Balbi, Rafael J. Wysocki, linux-kernel, linux-usb, linux-acpi On Wed, Jan 13, 2021 at 05:55:04PM +0200, Andy Shevchenko wrote: > On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote: > > On 11/01/2021 14:10, Heikki Krogerus wrote: > > ... > > > > +/** > > > + * device_remove_software_node - Remove device's software node > > > + * @dev: The device with the software node. > > > + * > > > + * This function will unregister the software node of @dev. > > > + */ > > > +void device_remove_software_node(struct device *dev) > > > +{ > > > + struct swnode *swnode; > > > + > > > + swnode = dev_to_swnode(dev); > > > + if (!swnode) > > > + return; > > > + > > > + kobject_put(&swnode->kobj); > > > +} > > > +EXPORT_SYMBOL_GPL(device_remove_software_node); > > > > I wonder if this also ought to set dev_fwnode(dev)->secondary back to > > ERR_PTR(-ENODEV)? > > Looking more into this code I think we need to call > > set_secondary_fwnode(dev, NULL); > > among these lines. > > The real problem is that set_primary_fwnode() and set_secondary_fwnode() have > no reference counting. If we have a chain ->primary->secondary->-ENODEV is > being used somewhere we can't tell from here. > > So, in practice it means that we lack of some kind of primary node to increment > reference count of the secondary node when the latter is chained to the given > primary. But it makes things too complicated. Any other options for shared > primary-secondary chain? Standalone primary along with standalone (exclusive) > secondary doesn't need this AFAICS. Perhaps a flag to primary like shared / > exclusive that will prevent breaking the chain in set_secondary_fwnode()? Or maybe I imagined only theoretical cases and we have no such issue? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] software node: Introduce device_add_software_node() 2021-01-13 15:58 ` Andy Shevchenko @ 2021-01-14 14:00 ` Heikki Krogerus 0 siblings, 0 replies; 16+ messages in thread From: Heikki Krogerus @ 2021-01-14 14:00 UTC (permalink / raw) To: Andy Shevchenko Cc: Daniel Scally, Felipe Balbi, Rafael J. Wysocki, linux-kernel, linux-usb, linux-acpi On Wed, Jan 13, 2021 at 05:58:12PM +0200, Andy Shevchenko wrote: > On Wed, Jan 13, 2021 at 05:55:04PM +0200, Andy Shevchenko wrote: > > On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote: > > > On 11/01/2021 14:10, Heikki Krogerus wrote: > > > > ... > > > > > > +/** > > > > + * device_remove_software_node - Remove device's software node > > > > + * @dev: The device with the software node. > > > > + * > > > > + * This function will unregister the software node of @dev. > > > > + */ > > > > +void device_remove_software_node(struct device *dev) > > > > +{ > > > > + struct swnode *swnode; > > > > + > > > > + swnode = dev_to_swnode(dev); > > > > + if (!swnode) > > > > + return; > > > > + > > > > + kobject_put(&swnode->kobj); > > > > +} > > > > +EXPORT_SYMBOL_GPL(device_remove_software_node); > > > > > > I wonder if this also ought to set dev_fwnode(dev)->secondary back to > > > ERR_PTR(-ENODEV)? > > > > Looking more into this code I think we need to call > > > > set_secondary_fwnode(dev, NULL); > > > > among these lines. > > > > The real problem is that set_primary_fwnode() and set_secondary_fwnode() have > > no reference counting. If we have a chain ->primary->secondary->-ENODEV is > > being used somewhere we can't tell from here. > > > > So, in practice it means that we lack of some kind of primary node to increment > > reference count of the secondary node when the latter is chained to the given > > primary. But it makes things too complicated. Any other options for shared > > primary-secondary chain? Standalone primary along with standalone (exclusive) > > secondary doesn't need this AFAICS. Perhaps a flag to primary like shared / > > exclusive that will prevent breaking the chain in set_secondary_fwnode()? > > Or maybe I imagined only theoretical cases and we have no such issue? I think we should really start looking into the possibility of removing the whole secondary coupling, because that is the thing that is crippling us. Br, -- heikki ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/3] usb: dwc3: pci: Register a software node for the dwc3 platform device 2021-01-11 14:10 [PATCH v2 0/3] Remove one more platform_device_add_properties() call Heikki Krogerus 2021-01-11 14:10 ` [PATCH v2 1/3] software node: Introduce device_add_software_node() Heikki Krogerus @ 2021-01-11 14:10 ` Heikki Krogerus 2021-01-11 14:10 ` [PATCH v2 3/3] usb: dwc3: pci: ID for Tiger Lake CPU Heikki Krogerus ` (2 subsequent siblings) 4 siblings, 0 replies; 16+ messages in thread From: Heikki Krogerus @ 2021-01-11 14:10 UTC (permalink / raw) To: Felipe Balbi, Rafael J. Wysocki Cc: Andy Shevchenko, linux-kernel, linux-usb, linux-acpi By registering the software node directly instead of just the properties in it, the driver can take advantage of also the other features the software nodes have. Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/usb/dwc3/dwc3-pci.c | 61 ++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c index bae6a70664c80..037bc21bffa66 100644 --- a/drivers/usb/dwc3/dwc3-pci.c +++ b/drivers/usb/dwc3/dwc3-pci.c @@ -142,6 +142,18 @@ static const struct property_entry dwc3_pci_amd_properties[] = { {} }; +static const struct software_node dwc3_pci_intel_swnode = { + .properties = dwc3_pci_intel_properties, +}; + +static const struct software_node dwc3_pci_intel_mrfld_swnode = { + .properties = dwc3_pci_mrfld_properties, +}; + +static const struct software_node dwc3_pci_amd_swnode = { + .properties = dwc3_pci_amd_properties, +}; + static int dwc3_pci_quirks(struct dwc3_pci *dwc) { struct pci_dev *pdev = dwc->pci; @@ -222,7 +234,6 @@ static void dwc3_pci_resume_work(struct work_struct *work) static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id) { - struct property_entry *p = (struct property_entry *)id->driver_data; struct dwc3_pci *dwc; struct resource res[2]; int ret; @@ -265,7 +276,7 @@ static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id) dwc->dwc3->dev.parent = dev; ACPI_COMPANION_SET(&dwc->dwc3->dev, ACPI_COMPANION(dev)); - ret = platform_device_add_properties(dwc->dwc3, p); + ret = device_add_software_node(&dwc->dwc3->dev, (void *)id->driver_data); if (ret < 0) goto err; @@ -288,6 +299,7 @@ static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id) return 0; err: + device_remove_software_node(&dwc->dwc3->dev); platform_device_put(dwc->dwc3); return ret; } @@ -304,75 +316,76 @@ static void dwc3_pci_remove(struct pci_dev *pci) #endif device_init_wakeup(&pci->dev, false); pm_runtime_get(&pci->dev); + device_remove_software_node(&dwc->dwc3->dev); platform_device_unregister(dwc->dwc3); } static const struct pci_device_id dwc3_pci_id_table[] = { { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BSW), - (kernel_ulong_t) &dwc3_pci_intel_properties }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BYT), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_MRFLD), - (kernel_ulong_t) &dwc3_pci_mrfld_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_mrfld_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CMLLP), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CMLH), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SPTLP), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SPTH), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BXT), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_BXT_M), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_APL), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_KBP), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_GLK), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPLP), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPH), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CNPV), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICLLP), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_EHLLP), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TGPLP), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TGPH), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_JSP), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ADLS), - (kernel_ulong_t) &dwc3_pci_intel_properties, }, + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_NL_USB), - (kernel_ulong_t) &dwc3_pci_amd_properties, }, + (kernel_ulong_t) &dwc3_pci_amd_swnode, }, { } /* Terminating Entry */ }; MODULE_DEVICE_TABLE(pci, dwc3_pci_id_table); -- 2.29.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/3] usb: dwc3: pci: ID for Tiger Lake CPU 2021-01-11 14:10 [PATCH v2 0/3] Remove one more platform_device_add_properties() call Heikki Krogerus 2021-01-11 14:10 ` [PATCH v2 1/3] software node: Introduce device_add_software_node() Heikki Krogerus 2021-01-11 14:10 ` [PATCH v2 2/3] usb: dwc3: pci: Register a software node for the dwc3 platform device Heikki Krogerus @ 2021-01-11 14:10 ` Heikki Krogerus 2021-01-12 8:46 ` [PATCH v2 0/3] Remove one more platform_device_add_properties() call Felipe Balbi 2021-01-12 11:49 ` Greg KH 4 siblings, 0 replies; 16+ messages in thread From: Heikki Krogerus @ 2021-01-11 14:10 UTC (permalink / raw) To: Felipe Balbi, Rafael J. Wysocki Cc: Andy Shevchenko, linux-kernel, linux-usb, linux-acpi Tiger Lake SOC (the versions of it that have integrated USB4 controller) may have two DWC3 controllers. One is part of the PCH (Platform Controller Hub, i.e. the chipset) as usual, and the other is inside the actual CPU block. On all Intel platforms that have the two separate DWC3 controllers, the one inside the CPU handles USB3 and only USB3 traffic, while the PCH version handles USB2 and USB2 alone. The reason for splitting the two busses like this is to allow easy USB3 tunneling over USB4 connections. As USB2 is not tunneled over USB4, it has dedicated USB controllers (both xHCI and DWC3). Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/usb/dwc3/dwc3-pci.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c index 037bc21bffa66..51029cec119ed 100644 --- a/drivers/usb/dwc3/dwc3-pci.c +++ b/drivers/usb/dwc3/dwc3-pci.c @@ -41,6 +41,7 @@ #define PCI_DEVICE_ID_INTEL_TGPH 0x43ee #define PCI_DEVICE_ID_INTEL_JSP 0x4dee #define PCI_DEVICE_ID_INTEL_ADLS 0x7ae1 +#define PCI_DEVICE_ID_INTEL_TGL 0x9a15 #define PCI_INTEL_BXT_DSM_GUID "732b85d5-b7a7-4a1b-9ba0-4bbd00ffd511" #define PCI_INTEL_BXT_FUNC_PMU_PWR 4 @@ -384,6 +385,9 @@ static const struct pci_device_id dwc3_pci_id_table[] = { { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ADLS), (kernel_ulong_t) &dwc3_pci_intel_swnode, }, + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TGL), + (kernel_ulong_t) &dwc3_pci_intel_swnode, }, + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_NL_USB), (kernel_ulong_t) &dwc3_pci_amd_swnode, }, { } /* Terminating Entry */ -- 2.29.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] Remove one more platform_device_add_properties() call 2021-01-11 14:10 [PATCH v2 0/3] Remove one more platform_device_add_properties() call Heikki Krogerus ` (2 preceding siblings ...) 2021-01-11 14:10 ` [PATCH v2 3/3] usb: dwc3: pci: ID for Tiger Lake CPU Heikki Krogerus @ 2021-01-12 8:46 ` Felipe Balbi 2021-01-12 11:49 ` Greg KH 4 siblings, 0 replies; 16+ messages in thread From: Felipe Balbi @ 2021-01-12 8:46 UTC (permalink / raw) To: Heikki Krogerus, Rafael J. Wysocki Cc: Andy Shevchenko, linux-kernel, linux-usb, linux-acpi [-- Attachment #1: Type: text/plain, Size: 1101 bytes --] Heikki Krogerus <heikki.krogerus@linux.intel.com> writes: > Hi Felipe, Rafael, > > This is the second version of this series. There are no real changes, > but I added the Tiger Lake ID patch to this series in hope that it > will make your life a bit easier, assuming that Rafael will still pick > these. > > > The original over letter: > > I originally introduced these as part of my series where I was > proposing PM ops for software nodes [1], but since that still needs > work, I'm sending these two separately. > > So basically I'm only modifying dwc3-pci.c so it registers a software > node directly at this point. That will remove one more user of > platform_device_add_properties(). > > [1] https://lore.kernel.org/lkml/20201029105941.63410-1-heikki.krogerus@linux.intel.com/ > > thanks, > > Heikki Krogerus (3): > software node: Introduce device_add_software_node() > usb: dwc3: pci: Register a software node for the dwc3 platform device > usb: dwc3: pci: ID for Tiger Lake CPU Looks good to me. Acked-by: Felipe Balbi <balbi@kernel.org> -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 857 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] Remove one more platform_device_add_properties() call 2021-01-11 14:10 [PATCH v2 0/3] Remove one more platform_device_add_properties() call Heikki Krogerus ` (3 preceding siblings ...) 2021-01-12 8:46 ` [PATCH v2 0/3] Remove one more platform_device_add_properties() call Felipe Balbi @ 2021-01-12 11:49 ` Greg KH 2021-01-15 15:01 ` Greg KH 4 siblings, 1 reply; 16+ messages in thread From: Greg KH @ 2021-01-12 11:49 UTC (permalink / raw) To: Heikki Krogerus Cc: Felipe Balbi, Rafael J. Wysocki, Andy Shevchenko, linux-kernel, linux-usb, linux-acpi On Mon, Jan 11, 2021 at 05:10:42PM +0300, Heikki Krogerus wrote: > Hi Felipe, Rafael, > > This is the second version of this series. There are no real changes, > but I added the Tiger Lake ID patch to this series in hope that it > will make your life a bit easier, assuming that Rafael will still pick > these. I can take all 3 of these if that makes it easier. Rafael, let me know what you want to do, either is fine with me. thanks, greg k-h ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] Remove one more platform_device_add_properties() call 2021-01-12 11:49 ` Greg KH @ 2021-01-15 15:01 ` Greg KH 2021-01-15 15:05 ` Greg KH 0 siblings, 1 reply; 16+ messages in thread From: Greg KH @ 2021-01-15 15:01 UTC (permalink / raw) To: Heikki Krogerus Cc: Felipe Balbi, Rafael J. Wysocki, Andy Shevchenko, linux-kernel, linux-usb, linux-acpi On Tue, Jan 12, 2021 at 12:49:14PM +0100, Greg KH wrote: > On Mon, Jan 11, 2021 at 05:10:42PM +0300, Heikki Krogerus wrote: > > Hi Felipe, Rafael, > > > > This is the second version of this series. There are no real changes, > > but I added the Tiger Lake ID patch to this series in hope that it > > will make your life a bit easier, assuming that Rafael will still pick > > these. > > I can take all 3 of these if that makes it easier. Rafael, let me know > what you want to do, either is fine with me. I've added it to my usb-next branch now. thanks, greg k-h ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] Remove one more platform_device_add_properties() call 2021-01-15 15:01 ` Greg KH @ 2021-01-15 15:05 ` Greg KH 0 siblings, 0 replies; 16+ messages in thread From: Greg KH @ 2021-01-15 15:05 UTC (permalink / raw) To: Heikki Krogerus Cc: Felipe Balbi, Rafael J. Wysocki, Andy Shevchenko, linux-kernel, linux-usb, linux-acpi On Fri, Jan 15, 2021 at 04:01:37PM +0100, Greg KH wrote: > On Tue, Jan 12, 2021 at 12:49:14PM +0100, Greg KH wrote: > > On Mon, Jan 11, 2021 at 05:10:42PM +0300, Heikki Krogerus wrote: > > > Hi Felipe, Rafael, > > > > > > This is the second version of this series. There are no real changes, > > > but I added the Tiger Lake ID patch to this series in hope that it > > > will make your life a bit easier, assuming that Rafael will still pick > > > these. > > > > I can take all 3 of these if that makes it easier. Rafael, let me know > > what you want to do, either is fine with me. > > I've added it to my usb-next branch now. Argh, grabbed the wrong one, will drop this and grab v3... ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-01-15 15:06 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-11 14:10 [PATCH v2 0/3] Remove one more platform_device_add_properties() call Heikki Krogerus 2021-01-11 14:10 ` [PATCH v2 1/3] software node: Introduce device_add_software_node() Heikki Krogerus 2021-01-13 0:40 ` Daniel Scally 2021-01-13 11:39 ` Heikki Krogerus 2021-01-13 15:30 ` Andy Shevchenko 2021-01-14 13:19 ` Heikki Krogerus 2021-01-14 14:24 ` Heikki Krogerus 2021-01-13 15:55 ` Andy Shevchenko 2021-01-13 15:58 ` Andy Shevchenko 2021-01-14 14:00 ` Heikki Krogerus 2021-01-11 14:10 ` [PATCH v2 2/3] usb: dwc3: pci: Register a software node for the dwc3 platform device Heikki Krogerus 2021-01-11 14:10 ` [PATCH v2 3/3] usb: dwc3: pci: ID for Tiger Lake CPU Heikki Krogerus 2021-01-12 8:46 ` [PATCH v2 0/3] Remove one more platform_device_add_properties() call Felipe Balbi 2021-01-12 11:49 ` Greg KH 2021-01-15 15:01 ` Greg KH 2021-01-15 15:05 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).