| Message ID | 20221221173055.11719-6-gatien.chevallier@foss.st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce STM32 system bus | expand |
On 21/12/2022 18:30, Gatien Chevallier wrote: > This driver is checking the access rights of the different > peripherals connected to the system bus. If access is denied, > the associated device tree node is skipped so the platform bus > does not probe it. > > Signed-off-by: Loic PALLARDY <loic.pallardy@st.com> > Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com> > --- > MAINTAINERS | 6 ++ > drivers/bus/Kconfig | 9 ++ > drivers/bus/Makefile | 1 + > drivers/bus/stm32_sys_bus.c | 180 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 196 insertions(+) > create mode 100644 drivers/bus/stm32_sys_bus.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 886d3f69ee64..768a8272233e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19522,6 +19522,12 @@ L: linux-spi@vger.kernel.org > S: Maintained > F: drivers/spi/spi-stm32.c > > +ST STM32 SYS BUS DRIVER > +M: Gatien Chevallier <gatien.chevallier@foss.st.com> > +S: Maintained > +F: Documentation/devicetree/bindings/bus/st,sys-bus.yaml > +F: drivers/bus/stm32_sys_bus.c > + > ST STPDDC60 DRIVER > M: Daniel Nilsson <daniel.nilsson@flex.com> > L: linux-hwmon@vger.kernel.org > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > index 7bfe998f3514..638bf5839cb0 100644 > --- a/drivers/bus/Kconfig > +++ b/drivers/bus/Kconfig > @@ -163,6 +163,15 @@ config QCOM_SSC_BLOCK_BUS > i2c/spi/uart controllers, a hexagon core, and a clock controller > which provides clocks for the above. > > +config STM32_SYS_BUS > + bool "STM32 System bus controller" > + depends on ARCH_STM32 || COMPILE_TEST > + default MACH_STM32MP157 || MACH_STM32MP13 > + help > + Say y to enable device access right verification before device probing. > + If access not granted, device won't be probed and an error message will > + provide the reason. (...) > + > +static int stm32_sys_bus_probe(struct platform_device *pdev) > +{ > + struct sys_bus_data *pdata; > + struct resource *res; > + void __iomem *mmio; > + struct stm32_sys_bus_match_data *mdata; > + struct device_node *np = pdev->dev.of_node; > + > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + mmio = devm_ioremap_resource(&pdev->dev, res); Use helper for these two. > + if (IS_ERR(mmio)) > + return PTR_ERR(mmio); > + > + pdata->sys_bus_base = mmio; > + > + mdata = (struct stm32_sys_bus_match_data *)of_device_get_match_data(&pdev->dev); Why do you need the cast? > + if (!mdata) Can you explain the case when this can actually happen? If it can, you have bug in ID table. > + return -EINVAL; > + > + pdata->pconf = mdata; > + pdata->dev = &pdev->dev; > + > + platform_set_drvdata(pdev, pdata); > + > + stm32_sys_bus_populate(pdata); > + > + /* Populate all available nodes */ > + return of_platform_populate(np, NULL, NULL, &pdev->dev); > +} > + > +static const struct stm32_sys_bus_match_data stm32mp15_sys_bus_data = { > + .max_entries = STM32MP15_ETZPC_ENTRIES, > + .sys_bus_get_access = stm32_etzpc_get_access, > +}; > + > +static const struct stm32_sys_bus_match_data stm32mp13_sys_bus_data = { > + .max_entries = STM32MP13_ETZPC_ENTRIES, > + .sys_bus_get_access = stm32_etzpc_get_access, It's the same as previous, drop. > +}; > + > +static const struct of_device_id stm32_sys_bus_of_match[] = { > + { .compatible = "st,stm32mp15-sys-bus", .data = &stm32mp15_sys_bus_data }, > + { .compatible = "st,stm32mp13-sys-bus", .data = &stm32mp13_sys_bus_data }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, stm32_sys_bus_of_match); > + > +static struct platform_driver stm32_sys_bus_driver = { > + .probe = stm32_sys_bus_probe, > + .driver = { > + .name = "stm32-sys-bus", > + .of_match_table = stm32_sys_bus_of_match, > + }, > +}; > + > +static int __init stm32_sys_bus_init(void) > +{ > + return platform_driver_register(&stm32_sys_bus_driver); > +} > +arch_initcall(stm32_sys_bus_init); > + Best regards, Krzysztof
On 12/22/22 11:28, Krzysztof Kozlowski wrote: > On 21/12/2022 18:30, Gatien Chevallier wrote: >> This driver is checking the access rights of the different >> peripherals connected to the system bus. If access is denied, >> the associated device tree node is skipped so the platform bus >> does not probe it. >> >> Signed-off-by: Loic PALLARDY <loic.pallardy@st.com> >> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com> >> --- >> MAINTAINERS | 6 ++ >> drivers/bus/Kconfig | 9 ++ >> drivers/bus/Makefile | 1 + >> drivers/bus/stm32_sys_bus.c | 180 ++++++++++++++++++++++++++++++++++++ >> 4 files changed, 196 insertions(+) >> create mode 100644 drivers/bus/stm32_sys_bus.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 886d3f69ee64..768a8272233e 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -19522,6 +19522,12 @@ L: linux-spi@vger.kernel.org >> S: Maintained >> F: drivers/spi/spi-stm32.c >> >> +ST STM32 SYS BUS DRIVER >> +M: Gatien Chevallier <gatien.chevallier@foss.st.com> >> +S: Maintained >> +F: Documentation/devicetree/bindings/bus/st,sys-bus.yaml >> +F: drivers/bus/stm32_sys_bus.c >> + >> ST STPDDC60 DRIVER >> M: Daniel Nilsson <daniel.nilsson@flex.com> >> L: linux-hwmon@vger.kernel.org >> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig >> index 7bfe998f3514..638bf5839cb0 100644 >> --- a/drivers/bus/Kconfig >> +++ b/drivers/bus/Kconfig >> @@ -163,6 +163,15 @@ config QCOM_SSC_BLOCK_BUS >> i2c/spi/uart controllers, a hexagon core, and a clock controller >> which provides clocks for the above. >> >> +config STM32_SYS_BUS >> + bool "STM32 System bus controller" >> + depends on ARCH_STM32 > > || COMPILE_TEST Sure, I will add this in V3 > >> + default MACH_STM32MP157 || MACH_STM32MP13 >> + help >> + Say y to enable device access right verification before device probing. >> + If access not granted, device won't be probed and an error message will >> + provide the reason. > > (...) > >> + >> +static int stm32_sys_bus_probe(struct platform_device *pdev) >> +{ >> + struct sys_bus_data *pdata; >> + struct resource *res; >> + void __iomem *mmio; >> + struct stm32_sys_bus_match_data *mdata; >> + struct device_node *np = pdev->dev.of_node; >> + >> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + mmio = devm_ioremap_resource(&pdev->dev, res); > > Use helper for these two. Ok, I will use devm_platform_ioremap_resource() > >> + if (IS_ERR(mmio)) >> + return PTR_ERR(mmio); >> + >> + pdata->sys_bus_base = mmio; >> + >> + mdata = (struct stm32_sys_bus_match_data *)of_device_get_match_data(&pdev->dev); > > Why do you need the cast? I do not :) TBH, mdata is not useful at all. Changing to directly assign to pdata->pconf, that is now const there is no reason to modify it. > >> + if (!mdata) > > Can you explain the case when this can actually happen? If it can, you > have bug in ID table. No I cannot as the driver is probed. It is only a sanity check, I can remove it for V3. However the function can return NULL... Would you prefer an explicit check on NULL or a simple removal? > >> + return -EINVAL; >> + >> + pdata->pconf = mdata; >> + pdata->dev = &pdev->dev; >> + >> + platform_set_drvdata(pdev, pdata); >> + >> + stm32_sys_bus_populate(pdata); >> + >> + /* Populate all available nodes */ >> + return of_platform_populate(np, NULL, NULL, &pdev->dev); >> +} >> + >> +static const struct stm32_sys_bus_match_data stm32mp15_sys_bus_data = { >> + .max_entries = STM32MP15_ETZPC_ENTRIES, >> + .sys_bus_get_access = stm32_etzpc_get_access, >> +}; >> + >> +static const struct stm32_sys_bus_match_data stm32mp13_sys_bus_data = { >> + .max_entries = STM32MP13_ETZPC_ENTRIES, >> + .sys_bus_get_access = stm32_etzpc_get_access, > > It's the same as previous, drop. Yes, ops is useless, I will directly call stm32_etzpc_get_access() in V3. > >> +}; >> + >> +static const struct of_device_id stm32_sys_bus_of_match[] = { >> + { .compatible = "st,stm32mp15-sys-bus", .data = &stm32mp15_sys_bus_data }, >> + { .compatible = "st,stm32mp13-sys-bus", .data = &stm32mp13_sys_bus_data }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, stm32_sys_bus_of_match); >> + >> +static struct platform_driver stm32_sys_bus_driver = { >> + .probe = stm32_sys_bus_probe, >> + .driver = { >> + .name = "stm32-sys-bus", >> + .of_match_table = stm32_sys_bus_of_match, >> + }, >> +}; >> + >> +static int __init stm32_sys_bus_init(void) >> +{ >> + return platform_driver_register(&stm32_sys_bus_driver); >> +} >> +arch_initcall(stm32_sys_bus_init); >> + > > Best regards, > Krzysztof > Best regards, Gatien
On 22/12/2022 15:30, Gatien CHEVALLIER wrote: >> >>> + if (IS_ERR(mmio)) >>> + return PTR_ERR(mmio); >>> + >>> + pdata->sys_bus_base = mmio; >>> + >>> + mdata = (struct stm32_sys_bus_match_data *)of_device_get_match_data(&pdev->dev); >> >> Why do you need the cast? > > I do not :) TBH, mdata is not useful at all. Changing to directly assign > to pdata->pconf, that is now const there is no reason to modify it. mdata should be const and then no need for cast. > >> >>> + if (!mdata) >> >> Can you explain the case when this can actually happen? If it can, you >> have bug in ID table. > > No I cannot as the driver is probed. It is only a sanity check, I can > remove it for V3. However the function can return NULL... Would you > prefer an explicit check on NULL or a simple removal? I propose to drop it. This simply cannot happen. > >> >>> + return -EINVAL; >>> + Best regards, Krzysztof
diff --git a/MAINTAINERS b/MAINTAINERS index 886d3f69ee64..768a8272233e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19522,6 +19522,12 @@ L: linux-spi@vger.kernel.org S: Maintained F: drivers/spi/spi-stm32.c +ST STM32 SYS BUS DRIVER +M: Gatien Chevallier <gatien.chevallier@foss.st.com> +S: Maintained +F: Documentation/devicetree/bindings/bus/st,sys-bus.yaml +F: drivers/bus/stm32_sys_bus.c + ST STPDDC60 DRIVER M: Daniel Nilsson <daniel.nilsson@flex.com> L: linux-hwmon@vger.kernel.org diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig index 7bfe998f3514..638bf5839cb0 100644 --- a/drivers/bus/Kconfig +++ b/drivers/bus/Kconfig @@ -163,6 +163,15 @@ config QCOM_SSC_BLOCK_BUS i2c/spi/uart controllers, a hexagon core, and a clock controller which provides clocks for the above. +config STM32_SYS_BUS + bool "STM32 System bus controller" + depends on ARCH_STM32 + default MACH_STM32MP157 || MACH_STM32MP13 + help + Say y to enable device access right verification before device probing. + If access not granted, device won't be probed and an error message will + provide the reason. + config SUN50I_DE2_BUS bool "Allwinner A64 DE2 Bus Driver" default ARM64 diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile index d90eed189a65..b15fdc42d0be 100644 --- a/drivers/bus/Makefile +++ b/drivers/bus/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_OMAP_INTERCONNECT) += omap_l3_smx.o omap_l3_noc.o obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o obj-$(CONFIG_QCOM_EBI2) += qcom-ebi2.o obj-$(CONFIG_QCOM_SSC_BLOCK_BUS) += qcom-ssc-block-bus.o +obj-$(CONFIG_STM32_SYS_BUS) += stm32_sys_bus.o obj-$(CONFIG_SUN50I_DE2_BUS) += sun50i-de2.o obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o obj-$(CONFIG_OF) += simple-pm-bus.o diff --git a/drivers/bus/stm32_sys_bus.c b/drivers/bus/stm32_sys_bus.c new file mode 100644 index 000000000000..e4c21e24b65e --- /dev/null +++ b/drivers/bus/stm32_sys_bus.c @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2022, STMicroelectronics - All Rights Reserved + */ + +#include <linux/bitfield.h> +#include <linux/bits.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> + +/* ETZPC peripheral as firewall bus */ +/* ETZPC registers */ +#define ETZPC_DECPROT 0x10 + +/* ETZPC miscellaneous */ +#define ETZPC_PROT_MASK GENMASK(1, 0) +#define ETZPC_PROT_A7NS 0x3 +#define ETZPC_DECPROT_SHIFT 1 + +#define IDS_PER_DECPROT_REGS 16 +#define STM32MP15_ETZPC_ENTRIES 96 +#define STM32MP13_ETZPC_ENTRIES 64 + +struct sys_bus_data; + +struct stm32_sys_bus_match_data { + const u32 *map_table; + unsigned int max_entries; + int (*sys_bus_get_access)(struct sys_bus_data *pdata, struct device_node *np); +}; + +struct sys_bus_data { + struct stm32_sys_bus_match_data *pconf; + void __iomem *sys_bus_base; + struct device *dev; +}; + +static int stm32_sys_bus_get_periph_id(struct sys_bus_data *pdata, struct device_node *np, u32 *id) +{ + int err; + u32 feature_domain_cell[2]; + u32 id_bus; + + /* Get reg from device node */ + err = of_property_read_u32_array(np, "feature-domains", feature_domain_cell, 2); + if (err) { + dev_err(pdata->dev, "Unable to find feature-domains property\n"); + return -ENODEV; + } + + id_bus = feature_domain_cell[1]; + + if (id_bus >= pdata->pconf->max_entries) { + dev_err(pdata->dev, "Invalid sys bus ID for %s\n", np->full_name); + return -EINVAL; + } + + *id = id_bus; + + return 0; +} + +static int stm32_etzpc_get_access(struct sys_bus_data *pdata, struct device_node *np) +{ + int err; + u32 offset, reg_offset, sec_val, id; + + err = stm32_sys_bus_get_periph_id(pdata, np, &id); + if (err) + return err; + + /* Check access configuration, 16 peripherals per register */ + reg_offset = ETZPC_DECPROT + 0x4 * (id / IDS_PER_DECPROT_REGS); + offset = (id % IDS_PER_DECPROT_REGS) << ETZPC_DECPROT_SHIFT; + + /* Verify peripheral is non-secure and attributed to cortex A7 */ + sec_val = (readl(pdata->sys_bus_base + reg_offset) >> offset) & ETZPC_PROT_MASK; + if (sec_val != ETZPC_PROT_A7NS) { + dev_dbg(pdata->dev, "Invalid bus configuration: reg_offset %#x, value %d\n", + reg_offset, sec_val); + return -EACCES; + } + + return 0; +} + +static void stm32_sys_bus_populate(struct sys_bus_data *pdata) +{ + struct device *parent; + struct device_node *child; + + parent = pdata->dev; + + dev_dbg(parent, "Populating %s system bus\n", pdata->dev->driver->name); + + for_each_available_child_of_node(dev_of_node(parent), child) { + if (pdata->pconf->sys_bus_get_access(pdata, child)) { + /* + * Peripheral access not allowed. + * Mark the node as populated so platform bus won't probe it + */ + of_node_set_flag(child, OF_POPULATED); + dev_dbg(parent, "%s: Peripheral will not be probed\n", + child->full_name); + } + } +} + +static int stm32_sys_bus_probe(struct platform_device *pdev) +{ + struct sys_bus_data *pdata; + struct resource *res; + void __iomem *mmio; + struct stm32_sys_bus_match_data *mdata; + struct device_node *np = pdev->dev.of_node; + + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + mmio = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(mmio)) + return PTR_ERR(mmio); + + pdata->sys_bus_base = mmio; + + mdata = (struct stm32_sys_bus_match_data *)of_device_get_match_data(&pdev->dev); + if (!mdata) + return -EINVAL; + + pdata->pconf = mdata; + pdata->dev = &pdev->dev; + + platform_set_drvdata(pdev, pdata); + + stm32_sys_bus_populate(pdata); + + /* Populate all available nodes */ + return of_platform_populate(np, NULL, NULL, &pdev->dev); +} + +static const struct stm32_sys_bus_match_data stm32mp15_sys_bus_data = { + .max_entries = STM32MP15_ETZPC_ENTRIES, + .sys_bus_get_access = stm32_etzpc_get_access, +}; + +static const struct stm32_sys_bus_match_data stm32mp13_sys_bus_data = { + .max_entries = STM32MP13_ETZPC_ENTRIES, + .sys_bus_get_access = stm32_etzpc_get_access, +}; + +static const struct of_device_id stm32_sys_bus_of_match[] = { + { .compatible = "st,stm32mp15-sys-bus", .data = &stm32mp15_sys_bus_data }, + { .compatible = "st,stm32mp13-sys-bus", .data = &stm32mp13_sys_bus_data }, + {} +}; +MODULE_DEVICE_TABLE(of, stm32_sys_bus_of_match); + +static struct platform_driver stm32_sys_bus_driver = { + .probe = stm32_sys_bus_probe, + .driver = { + .name = "stm32-sys-bus", + .of_match_table = stm32_sys_bus_of_match, + }, +}; + +static int __init stm32_sys_bus_init(void) +{ + return platform_driver_register(&stm32_sys_bus_driver); +} +arch_initcall(stm32_sys_bus_init); +