| Message ID | 1473150503-9550-8-git-send-email-yangbo.lu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu@nxp.com> wrote: > We keep running into cases where device drivers want to know the exact > version of the a SoC they are currently running on. In the past, this has > usually been done through a vendor specific API that can be called by a > driver, or by directly accessing some kind of version register that is > not part of the device itself but that belongs to a global register area > of the chip. > > Common reasons for doing this include: > > - A machine is not using devicetree or similar for passing data about > on-chip devices, but just announces their presence using boot-time > platform devices, and the machine code itself does not care about the > revision. > > - There is existing firmware or boot loaders with existing DT binaries > with generic compatible strings that do not identify the particular > revision of each device, but the driver knows which SoC revisions > include which part. > > - A prerelease version of a chip has some quirks and we are using the same > version of the bootloader and the DT blob on both the prerelease and the > final version. An update of the DT binding seems inappropriate because > that would involve maintaining multiple copies of the dts and/or > bootloader. > > This patch introduces the soc_device_match() interface that is meant to > work like of_match_node() but instead of identifying the version of a > device, it identifies the SoC itself using a vendor-agnostic interface. > > Unlike of_match_node(), we do not do an exact string compare but instead > use glob_match() to allow wildcards in strings. Overall, this change make sense to me, although some minor comment below. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> > --- > Changes for v11: > - Added this patch for soc match > --- > drivers/base/Kconfig | 1 + > drivers/base/soc.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/sys_soc.h | 3 +++ > 3 files changed, 65 insertions(+) > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > index 98504ec..f1591ad2 100644 > --- a/drivers/base/Kconfig > +++ b/drivers/base/Kconfig > @@ -225,6 +225,7 @@ config GENERIC_CPU_AUTOPROBE > > config SOC_BUS > bool > + select GLOB > > source "drivers/base/regmap/Kconfig" > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c > index 75b98aa..5c4e84a 100644 > --- a/drivers/base/soc.c > +++ b/drivers/base/soc.c > @@ -14,6 +14,7 @@ > #include <linux/spinlock.h> > #include <linux/sys_soc.h> > #include <linux/err.h> > +#include <linux/glob.h> > > static DEFINE_IDA(soc_ida); > > @@ -168,3 +169,63 @@ static void __exit soc_bus_unregister(void) > bus_unregister(&soc_bus_type); > } > module_exit(soc_bus_unregister); > + > +static int soc_device_match_one(struct device *dev, void *arg) > +{ > + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev); > + const struct soc_device_attribute *match = arg; > + > + if (match->machine && > + !glob_match(match->machine, soc_dev->attr->machine)) > + return 0; > + > + if (match->family && > + !glob_match(match->family, soc_dev->attr->family)) > + return 0; > + > + if (match->revision && > + !glob_match(match->revision, soc_dev->attr->revision)) > + return 0; > + > + if (match->soc_id && > + !glob_match(match->soc_id, soc_dev->attr->soc_id)) > + return 0; > + > + return 1; > +} > + > +/* > + * soc_device_match - identify the SoC in the machine > + * @matches: zero-terminated array of possible matches Perhaps also express the constraint on the matching entries. As you need at least one of the ->machine(), ->family(), ->revision() or ->soc_id() callbacks implemented, right!? > + * > + * returns the first matching entry of the argument array, or NULL > + * if none of them match. > + * > + * This function is meant as a helper in place of of_match_node() > + * in cases where either no device tree is available or the information > + * in a device node is insufficient to identify a particular variant > + * by its compatible strings or other properties. For new devices, > + * the DT binding should always provide unique compatible strings > + * that allow the use of of_match_node() instead. > + * > + * The calling function can use the .data entry of the > + * soc_device_attribute to pass a structure or function pointer for > + * each entry. I don't get the use case behind this, could you elaborate? Perhaps we should postpone adding the .data entry until we actually see a need for it? > + */ > +const struct soc_device_attribute *soc_device_match( > + const struct soc_device_attribute *matches) > +{ > + struct device *dev; > + int ret; > + > + for (ret = 0; ret == 0; matches++) { This loop looks a bit weird and unsafe. 1) Perhaps using a while loop makes this more readable? 2) As this is an exported API, I guess validation of the ->matches pointer needs to be done before accessing it. > + if (!(matches->machine || matches->family || > + matches->revision || matches->soc_id)) > + return NULL; > + dev = NULL; There's no need to use a struct device just to assign it to NULL. Instead just provide the function below with NULL. > + ret = bus_for_each_dev(&soc_bus_type, dev, (void *)matches, > + soc_device_match_one); > + } > + return matches; > +} > +EXPORT_SYMBOL_GPL(soc_device_match); > diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h > index 2739ccb..9f5eb06 100644 > --- a/include/linux/sys_soc.h > +++ b/include/linux/sys_soc.h > @@ -13,6 +13,7 @@ struct soc_device_attribute { > const char *family; > const char *revision; > const char *soc_id; > + const void *data; > }; > > /** > @@ -34,4 +35,6 @@ void soc_device_unregister(struct soc_device *soc_dev); > */ > struct device *soc_device_to_device(struct soc_device *soc); > > +const struct soc_device_attribute *soc_device_match( > + const struct soc_device_attribute *matches); > #endif /* __SOC_BUS_H */ > -- > 2.1.0.27.g96db324 > Kind regards Uffe
On Tuesday, September 6, 2016 1:44:23 PM CEST Ulf Hansson wrote: > On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu@nxp.com> wrote: > > We keep running into cases where device drivers want to know the exact > > version of the a SoC they are currently running on. In the past, this has > > usually been done through a vendor specific API that can be called by a > > driver, or by directly accessing some kind of version register that is > > not part of the device itself but that belongs to a global register area > > of the chip. Please add "From: Arnd Bergmann <arnd@arndb.de>" as the first line, to preserve authorship. If you use "git send-email" or "git format-patch", that should happen automatically if the author field is set right (if not, use 'git commit --amend --author="Arnd Bergmann <arnd@arndb.de>"' to fix it). > > + > > +/* > > + * soc_device_match - identify the SoC in the machine > > + * @matches: zero-terminated array of possible matches > > Perhaps also express the constraint on the matching entries. As you > need at least one of the ->machine(), ->family(), ->revision() or > ->soc_id() callbacks implemented, right!? They are not callbacks, just strings. Having an empty entry indicates the end of the array, and this is not called. > > + * > > + * returns the first matching entry of the argument array, or NULL > > + * if none of them match. > > + * > > + * This function is meant as a helper in place of of_match_node() > > + * in cases where either no device tree is available or the information > > + * in a device node is insufficient to identify a particular variant > > + * by its compatible strings or other properties. For new devices, > > + * the DT binding should always provide unique compatible strings > > + * that allow the use of of_match_node() instead. > > + * > > + * The calling function can use the .data entry of the > > + * soc_device_attribute to pass a structure or function pointer for > > + * each entry. > > I don't get the use case behind this, could you elaborate? > > Perhaps we should postpone adding the .data entry until we actually > see a need for it? I think the interface is rather useless without a way to figure out which entry you got. Almost all users of of_match_node() actually use the returned ->data field, and I expect this to be the same here. > > + */ > > +const struct soc_device_attribute *soc_device_match( > > + const struct soc_device_attribute *matches) > > +{ > > + struct device *dev; > > + int ret; > > + > > + for (ret = 0; ret == 0; matches++) { > > This loop looks a bit weird and unsafe. Ah, and I thought I was being clever ;-) > 1) Perhaps using a while loop makes this more readable? > 2) As this is an exported API, I guess validation of the ->matches > pointer needs to be done before accessing it. Sounds fine. > > + if (!(matches->machine || matches->family || > > + matches->revision || matches->soc_id)) > > + return NULL; > > + dev = NULL; > > There's no need to use a struct device just to assign it to NULL. > Instead just provide the function below with NULL. > > > + ret = bus_for_each_dev(&soc_bus_type, dev, (void *)matches, > > + soc_device_match_one); I don't remember what led to this, I think you are right, we should just pass NULL as most other callers. Thanks for the review. ARnd
Hi Anrd and Uffe, Thank you for your comment. Please see my comment inline. Best regards, Yangbo Lu > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Tuesday, September 06, 2016 8:46 PM > To: Ulf Hansson > Cc: Y.B. Lu; linux-mmc; Scott Wood; linuxppc-dev@lists.ozlabs.org; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; linux-clk; linux-i2c@vger.kernel.org; > iommu@lists.linux-foundation.org; netdev@vger.kernel.org; Mark Rutland; > Rob Herring; Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil; > Bhupesh Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B. > Xie > Subject: Re: [v11, 7/8] base: soc: introduce soc_device_match() interface > > On Tuesday, September 6, 2016 1:44:23 PM CEST Ulf Hansson wrote: > > On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu@nxp.com> wrote: > > > We keep running into cases where device drivers want to know the > > > exact version of the a SoC they are currently running on. In the > > > past, this has usually been done through a vendor specific API that > > > can be called by a driver, or by directly accessing some kind of > > > version register that is not part of the device itself but that > > > belongs to a global register area of the chip. > > Please add "From: Arnd Bergmann <arnd@arndb.de>" as the first line, to > preserve authorship. If you use "git send-email" or "git format-patch", > that should happen automatically if the author field is set right (if not, > use 'git commit --amend --author="Arnd Bergmann <arnd@arndb.de>"' > to fix it). > [Lu Yangbo-B47093] Oh, I'm sorry for my careless. Will correct it in next version. > > > + > > > +/* > > > + * soc_device_match - identify the SoC in the machine > > > + * @matches: zero-terminated array of possible matches > > > > Perhaps also express the constraint on the matching entries. As you > > need at least one of the ->machine(), ->family(), ->revision() or > > ->soc_id() callbacks implemented, right!? > > They are not callbacks, just strings. Having an empty entry indicates the > end of the array, and this is not called. > > > > + * > > > + * returns the first matching entry of the argument array, or NULL > > > + * if none of them match. > > > + * > > > + * This function is meant as a helper in place of of_match_node() > > > + * in cases where either no device tree is available or the > > > + information > > > + * in a device node is insufficient to identify a particular > > > + variant > > > + * by its compatible strings or other properties. For new devices, > > > + * the DT binding should always provide unique compatible strings > > > + * that allow the use of of_match_node() instead. > > > + * > > > + * The calling function can use the .data entry of the > > > + * soc_device_attribute to pass a structure or function pointer for > > > + * each entry. > > > > I don't get the use case behind this, could you elaborate? > > > > Perhaps we should postpone adding the .data entry until we actually > > see a need for it? > > I think the interface is rather useless without a way to figure out which > entry you got. Almost all users of of_match_node() actually use the > returned ->data field, and I expect this to be the same here. > > > > + */ > > > +const struct soc_device_attribute *soc_device_match( > > > + const struct soc_device_attribute *matches) { > > > + struct device *dev; > > > + int ret; > > > + > > > + for (ret = 0; ret == 0; matches++) { > > > > This loop looks a bit weird and unsafe. > > Ah, and I thought I was being clever ;-) > > > 1) Perhaps using a while loop makes this more readable? > > 2) As this is an exported API, I guess validation of the ->matches > > pointer needs to be done before accessing it. > > Sounds fine. [Lu Yangbo-B47093] Ok, Will change this according to Uffe. And actually there is issue with this for() when I verified it again this morning. We will get matches++ rather than matches which is correct finally :) > > > > + if (!(matches->machine || matches->family || > > > + matches->revision || matches->soc_id)) > > > + return NULL; > > > + dev = NULL; > > > > There's no need to use a struct device just to assign it to NULL. > > Instead just provide the function below with NULL. > > > > > + ret = bus_for_each_dev(&soc_bus_type, dev, (void > *)matches, > > > + soc_device_match_one); > > > I don't remember what led to this, I think you are right, we should just > pass NULL as most other callers. [Lu Yangbo-B47093] Will correct it. Thanks. :) > > Thanks for the review. > > ARnd
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 98504ec..f1591ad2 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -225,6 +225,7 @@ config GENERIC_CPU_AUTOPROBE config SOC_BUS bool + select GLOB source "drivers/base/regmap/Kconfig" diff --git a/drivers/base/soc.c b/drivers/base/soc.c index 75b98aa..5c4e84a 100644 --- a/drivers/base/soc.c +++ b/drivers/base/soc.c @@ -14,6 +14,7 @@ #include <linux/spinlock.h> #include <linux/sys_soc.h> #include <linux/err.h> +#include <linux/glob.h> static DEFINE_IDA(soc_ida); @@ -168,3 +169,63 @@ static void __exit soc_bus_unregister(void) bus_unregister(&soc_bus_type); } module_exit(soc_bus_unregister); + +static int soc_device_match_one(struct device *dev, void *arg) +{ + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev); + const struct soc_device_attribute *match = arg; + + if (match->machine && + !glob_match(match->machine, soc_dev->attr->machine)) + return 0; + + if (match->family && + !glob_match(match->family, soc_dev->attr->family)) + return 0; + + if (match->revision && + !glob_match(match->revision, soc_dev->attr->revision)) + return 0; + + if (match->soc_id && + !glob_match(match->soc_id, soc_dev->attr->soc_id)) + return 0; + + return 1; +} + +/* + * soc_device_match - identify the SoC in the machine + * @matches: zero-terminated array of possible matches + * + * returns the first matching entry of the argument array, or NULL + * if none of them match. + * + * This function is meant as a helper in place of of_match_node() + * in cases where either no device tree is available or the information + * in a device node is insufficient to identify a particular variant + * by its compatible strings or other properties. For new devices, + * the DT binding should always provide unique compatible strings + * that allow the use of of_match_node() instead. + * + * The calling function can use the .data entry of the + * soc_device_attribute to pass a structure or function pointer for + * each entry. + */ +const struct soc_device_attribute *soc_device_match( + const struct soc_device_attribute *matches) +{ + struct device *dev; + int ret; + + for (ret = 0; ret == 0; matches++) { + if (!(matches->machine || matches->family || + matches->revision || matches->soc_id)) + return NULL; + dev = NULL; + ret = bus_for_each_dev(&soc_bus_type, dev, (void *)matches, + soc_device_match_one); + } + return matches; +} +EXPORT_SYMBOL_GPL(soc_device_match); diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h index 2739ccb..9f5eb06 100644 --- a/include/linux/sys_soc.h +++ b/include/linux/sys_soc.h @@ -13,6 +13,7 @@ struct soc_device_attribute { const char *family; const char *revision; const char *soc_id; + const void *data; }; /** @@ -34,4 +35,6 @@ void soc_device_unregister(struct soc_device *soc_dev); */ struct device *soc_device_to_device(struct soc_device *soc); +const struct soc_device_attribute *soc_device_match( + const struct soc_device_attribute *matches); #endif /* __SOC_BUS_H */