| Message ID | 1473150503-9550-8-git-send-email-yangbo.lu@nxp.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Stephen Boyd |
| 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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */