Message ID | 20241111220304.1228821-3-samuel.holland@sifive.com |
---|---|
State | Accepted |
Headers | show |
Series | Deduplicate driver initialization code | expand |
On Tue, Nov 12, 2024 at 3:33 AM Samuel Holland <samuel.holland@sifive.com> wrote: > > Currently, each driver subsystem contains its own code for matching > drivers against the platform's devicetree blob. This bloats firmware > size because the several FDT scanning loops are almost exact copies of > each other, and is confusing because the loops do have some subtle > differences. Furthermore, the existing match algorithm is inefficient: > it scans the FDT structure separately for each driver in the list. A > faster algorithm scans the FDT blob only once, matching all drivers in > the list for each `compatible` property seen. > > Add new helpers implementing this faster algorithm. Since they must > iterate through the list of drivers, the driver structure cannot be > opaque. However, since the driver list is an array of pointers, the > `struct fdt_driver` can be embedded in a subsystem-specific driver > structure if needed. These three helpers cover all existing use cases > for driver initialization within OpenSBI. > > An additional benefit of centralized driver initialization is the > consistent use of fdt_node_is_enabled(). > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com> LGTM. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup > --- > > (no changes since v1) > > include/sbi_utils/fdt/fdt_driver.h | 59 ++++++++++++++++++++++ > lib/utils/fdt/fdt_driver.c | 80 ++++++++++++++++++++++++++++++ > lib/utils/fdt/objects.mk | 1 + > 3 files changed, 140 insertions(+) > create mode 100644 include/sbi_utils/fdt/fdt_driver.h > create mode 100644 lib/utils/fdt/fdt_driver.c > > diff --git a/include/sbi_utils/fdt/fdt_driver.h b/include/sbi_utils/fdt/fdt_driver.h > new file mode 100644 > index 00000000..f6fd26f9 > --- /dev/null > +++ b/include/sbi_utils/fdt/fdt_driver.h > @@ -0,0 +1,59 @@ > +/* > + * SPDX-License-Identifier: BSD-2-Clause > + * > + * fdt_driver.h - Generic support for initializing drivers from DT nodes. > + * > + * Copyright (c) 2024 SiFive > + */ > + > +#ifndef __FDT_DRIVER_H__ > +#define __FDT_DRIVER_H__ > + > +#include <sbi_utils/fdt/fdt_helper.h> > + > +struct fdt_driver { > + const struct fdt_match *match_table; > + int (*init)(const void *fdt, int nodeoff, > + const struct fdt_match *match); > +}; > + > +/** > + * Initialize a driver instance for a specific DT node > + * > + * @param fdt devicetree blob > + * @param nodeoff offset of a node in the devicetree blob > + * @param drivers NULL-terminated array of drivers to match against this node > + * > + * @return 0 if a driver was matched and successfully initialized or a negative > + * error code on failure > + */ > +int fdt_driver_init_by_offset(const void *fdt, int nodeoff, > + const struct fdt_driver *const *drivers); > + > +/** > + * Initialize a driver instance for each DT node that matches any of the > + * provided drivers > + * > + * @param fdt devicetree blob > + * @param drivers NULL-terminated array of drivers to match against each node > + * > + * @return 0 if drivers for all matches (if any) were successfully initialized > + * or a negative error code on failure > + */ > +int fdt_driver_init_all(const void *fdt, > + const struct fdt_driver *const *drivers); > + > +/** > + * Initialize a driver instance for the first DT node that matches any of the > + * provided drivers > + * > + * @param fdt devicetree blob > + * @param drivers NULL-terminated array of drivers to match against each node > + * > + * @return 0 if a driver was matched and successfully initialized or a negative > + * error code on failure > + */ > +int fdt_driver_init_one(const void *fdt, > + const struct fdt_driver *const *drivers); > + > +#endif /* __FDT_DRIVER_H__ */ > diff --git a/lib/utils/fdt/fdt_driver.c b/lib/utils/fdt/fdt_driver.c > new file mode 100644 > index 00000000..586ea8aa > --- /dev/null > +++ b/lib/utils/fdt/fdt_driver.c > @@ -0,0 +1,80 @@ > +/* > + * SPDX-License-Identifier: BSD-2-Clause > + * > + * Copyright (c) 2024 SiFive > + */ > + > +#include <libfdt.h> > +#include <sbi/sbi_console.h> > +#include <sbi/sbi_error.h> > +#include <sbi_utils/fdt/fdt_driver.h> > +#include <sbi_utils/fdt/fdt_helper.h> > + > +int fdt_driver_init_by_offset(const void *fdt, int nodeoff, > + const struct fdt_driver *const *drivers) > +{ > + const struct fdt_driver *driver; > + const struct fdt_match *match; > + const void *prop; > + int len, rc; > + > + if (!fdt_node_is_enabled(fdt, nodeoff)) > + return SBI_ENODEV; > + > + prop = fdt_getprop(fdt, nodeoff, "compatible", &len); > + if (!prop) > + return SBI_ENODEV; > + > + while ((driver = *drivers++)) { > + for (match = driver->match_table; match->compatible; match++) { > + if (!fdt_stringlist_contains(prop, len, match->compatible)) > + continue; > + > + rc = driver->init(fdt, nodeoff, match); > + if (rc < 0) { > + const char *name; > + > + name = fdt_get_name(fdt, nodeoff, NULL); > + sbi_printf("%s: %s (%s) init failed: %d\n", > + __func__, name, match->compatible, rc); > + } > + > + return rc; > + } > + } > + > + return SBI_ENODEV; > +} > + > +static int fdt_driver_init_scan(const void *fdt, > + const struct fdt_driver *const *drivers, > + bool one) > +{ > + int nodeoff, rc; > + > + for (nodeoff = fdt_next_node(fdt, -1, NULL); > + nodeoff >= 0; > + nodeoff = fdt_next_node(fdt, nodeoff, NULL)) { > + rc = fdt_driver_init_by_offset(fdt, nodeoff, drivers); > + if (rc == SBI_ENODEV) > + continue; > + if (rc < 0) > + return rc; > + if (one) > + return 0; > + } > + > + return one ? SBI_ENODEV : 0; > +} > + > +int fdt_driver_init_all(const void *fdt, > + const struct fdt_driver *const *drivers) > +{ > + return fdt_driver_init_scan(fdt, drivers, false); > +} > + > +int fdt_driver_init_one(const void *fdt, > + const struct fdt_driver *const *drivers) > +{ > + return fdt_driver_init_scan(fdt, drivers, true); > +} > diff --git a/lib/utils/fdt/objects.mk b/lib/utils/fdt/objects.mk > index 5cede811..1a2298be 100644 > --- a/lib/utils/fdt/objects.mk > +++ b/lib/utils/fdt/objects.mk > @@ -7,4 +7,5 @@ > libsbiutils-objs-$(CONFIG_FDT_DOMAIN) += fdt/fdt_domain.o > libsbiutils-objs-$(CONFIG_FDT_PMU) += fdt/fdt_pmu.o > libsbiutils-objs-$(CONFIG_FDT) += fdt/fdt_helper.o > +libsbiutils-objs-$(CONFIG_FDT) += fdt/fdt_driver.o > libsbiutils-objs-$(CONFIG_FDT) += fdt/fdt_fixup.o > -- > 2.45.1 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
diff --git a/include/sbi_utils/fdt/fdt_driver.h b/include/sbi_utils/fdt/fdt_driver.h new file mode 100644 index 00000000..f6fd26f9 --- /dev/null +++ b/include/sbi_utils/fdt/fdt_driver.h @@ -0,0 +1,59 @@ +/* + * SPDX-License-Identifier: BSD-2-Clause + * + * fdt_driver.h - Generic support for initializing drivers from DT nodes. + * + * Copyright (c) 2024 SiFive + */ + +#ifndef __FDT_DRIVER_H__ +#define __FDT_DRIVER_H__ + +#include <sbi_utils/fdt/fdt_helper.h> + +struct fdt_driver { + const struct fdt_match *match_table; + int (*init)(const void *fdt, int nodeoff, + const struct fdt_match *match); +}; + +/** + * Initialize a driver instance for a specific DT node + * + * @param fdt devicetree blob + * @param nodeoff offset of a node in the devicetree blob + * @param drivers NULL-terminated array of drivers to match against this node + * + * @return 0 if a driver was matched and successfully initialized or a negative + * error code on failure + */ +int fdt_driver_init_by_offset(const void *fdt, int nodeoff, + const struct fdt_driver *const *drivers); + +/** + * Initialize a driver instance for each DT node that matches any of the + * provided drivers + * + * @param fdt devicetree blob + * @param drivers NULL-terminated array of drivers to match against each node + * + * @return 0 if drivers for all matches (if any) were successfully initialized + * or a negative error code on failure + */ +int fdt_driver_init_all(const void *fdt, + const struct fdt_driver *const *drivers); + +/** + * Initialize a driver instance for the first DT node that matches any of the + * provided drivers + * + * @param fdt devicetree blob + * @param drivers NULL-terminated array of drivers to match against each node + * + * @return 0 if a driver was matched and successfully initialized or a negative + * error code on failure + */ +int fdt_driver_init_one(const void *fdt, + const struct fdt_driver *const *drivers); + +#endif /* __FDT_DRIVER_H__ */ diff --git a/lib/utils/fdt/fdt_driver.c b/lib/utils/fdt/fdt_driver.c new file mode 100644 index 00000000..586ea8aa --- /dev/null +++ b/lib/utils/fdt/fdt_driver.c @@ -0,0 +1,80 @@ +/* + * SPDX-License-Identifier: BSD-2-Clause + * + * Copyright (c) 2024 SiFive + */ + +#include <libfdt.h> +#include <sbi/sbi_console.h> +#include <sbi/sbi_error.h> +#include <sbi_utils/fdt/fdt_driver.h> +#include <sbi_utils/fdt/fdt_helper.h> + +int fdt_driver_init_by_offset(const void *fdt, int nodeoff, + const struct fdt_driver *const *drivers) +{ + const struct fdt_driver *driver; + const struct fdt_match *match; + const void *prop; + int len, rc; + + if (!fdt_node_is_enabled(fdt, nodeoff)) + return SBI_ENODEV; + + prop = fdt_getprop(fdt, nodeoff, "compatible", &len); + if (!prop) + return SBI_ENODEV; + + while ((driver = *drivers++)) { + for (match = driver->match_table; match->compatible; match++) { + if (!fdt_stringlist_contains(prop, len, match->compatible)) + continue; + + rc = driver->init(fdt, nodeoff, match); + if (rc < 0) { + const char *name; + + name = fdt_get_name(fdt, nodeoff, NULL); + sbi_printf("%s: %s (%s) init failed: %d\n", + __func__, name, match->compatible, rc); + } + + return rc; + } + } + + return SBI_ENODEV; +} + +static int fdt_driver_init_scan(const void *fdt, + const struct fdt_driver *const *drivers, + bool one) +{ + int nodeoff, rc; + + for (nodeoff = fdt_next_node(fdt, -1, NULL); + nodeoff >= 0; + nodeoff = fdt_next_node(fdt, nodeoff, NULL)) { + rc = fdt_driver_init_by_offset(fdt, nodeoff, drivers); + if (rc == SBI_ENODEV) + continue; + if (rc < 0) + return rc; + if (one) + return 0; + } + + return one ? SBI_ENODEV : 0; +} + +int fdt_driver_init_all(const void *fdt, + const struct fdt_driver *const *drivers) +{ + return fdt_driver_init_scan(fdt, drivers, false); +} + +int fdt_driver_init_one(const void *fdt, + const struct fdt_driver *const *drivers) +{ + return fdt_driver_init_scan(fdt, drivers, true); +} diff --git a/lib/utils/fdt/objects.mk b/lib/utils/fdt/objects.mk index 5cede811..1a2298be 100644 --- a/lib/utils/fdt/objects.mk +++ b/lib/utils/fdt/objects.mk @@ -7,4 +7,5 @@ libsbiutils-objs-$(CONFIG_FDT_DOMAIN) += fdt/fdt_domain.o libsbiutils-objs-$(CONFIG_FDT_PMU) += fdt/fdt_pmu.o libsbiutils-objs-$(CONFIG_FDT) += fdt/fdt_helper.o +libsbiutils-objs-$(CONFIG_FDT) += fdt/fdt_driver.o libsbiutils-objs-$(CONFIG_FDT) += fdt/fdt_fixup.o
Currently, each driver subsystem contains its own code for matching drivers against the platform's devicetree blob. This bloats firmware size because the several FDT scanning loops are almost exact copies of each other, and is confusing because the loops do have some subtle differences. Furthermore, the existing match algorithm is inefficient: it scans the FDT structure separately for each driver in the list. A faster algorithm scans the FDT blob only once, matching all drivers in the list for each `compatible` property seen. Add new helpers implementing this faster algorithm. Since they must iterate through the list of drivers, the driver structure cannot be opaque. However, since the driver list is an array of pointers, the `struct fdt_driver` can be embedded in a subsystem-specific driver structure if needed. These three helpers cover all existing use cases for driver initialization within OpenSBI. An additional benefit of centralized driver initialization is the consistent use of fdt_node_is_enabled(). Signed-off-by: Samuel Holland <samuel.holland@sifive.com> --- (no changes since v1) include/sbi_utils/fdt/fdt_driver.h | 59 ++++++++++++++++++++++ lib/utils/fdt/fdt_driver.c | 80 ++++++++++++++++++++++++++++++ lib/utils/fdt/objects.mk | 1 + 3 files changed, 140 insertions(+) create mode 100644 include/sbi_utils/fdt/fdt_driver.h create mode 100644 lib/utils/fdt/fdt_driver.c