Messages in this thread | | | Date | Tue, 7 Feb 2023 15:12:23 +0100 | Subject | Re: [PATCH v3 4/6] bus: stm32_sys_bus: add support for STM32MP15 and STM32MP13 system bus | From | Gatien CHEVALLIER <> |
| |
Hi Jonathan,
On 1/28/23 17:12, Jonathan Cameron wrote: > On Fri, 27 Jan 2023 17:40:38 +0100 > Gatien Chevallier <gatien.chevallier@foss.st.com> 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: Gatien Chevallier <gatien.chevallier@foss.st.com> >> Signed-off-by: Loic PALLARDY <loic.pallardy@st.com> > > Hi Gatien, > > A few comments inline, > > Thanks, > > Jonathan > >> diff --git a/drivers/bus/stm32_sys_bus.c b/drivers/bus/stm32_sys_bus.c >> new file mode 100644 >> index 000000000000..c12926466bae >> --- /dev/null >> +++ b/drivers/bus/stm32_sys_bus.c >> @@ -0,0 +1,168 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2023, 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 > > This define makes the code harder to read. What we care about is > the number of bits in the register divided by number of entries. > (which is 2) hence the shift by 1. See below for more on this. > > >> + >> +#define IDS_PER_DECPROT_REGS 16 > >> +#define STM32MP15_ETZPC_ENTRIES 96 >> +#define STM32MP13_ETZPC_ENTRIES 64 > > These defines just make the code harder to check. > They aren't magic numbers, but rather just telling us how many > entries there are, so I would just put them in the structures directly. > Their use make it clear what they are without needing to give them a name. >
Honestly, I'd rather read the hardware configuration registers to get this information instead of differentiating MP13/15. Would you agree on that?
> >> +struct stm32_sys_bus_match_data { > > Comment on naming of this below. > >> + unsigned int max_entries; >> +}; >> + > > +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; > > Use of defines in here is actively unhelpful when it comes to review. I would suggest letting > the maths be self explanatory (even if it's more code). > > offset = (id % IDS_PER_DECPROT_REGS) * (sizeof(u32) * BITS_PER_BYTE / IDS_PER_DECPROT_REGS); > > Or if you prefer have a define of > > #define DECPROT_BITS_PER_ID (sizeof(u32) * BITS_PER_BYTE / IDS_PER_DECPROT_REGS) > > and > offset = (id % IDS_PER_DECPROT_REGS) * DECPROT_BITS_PER_ID; >
Ok I'll rework this for better understanding. Your suggestion seems fine
> + > + /* 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 int stm32_sys_bus_probe(struct platform_device *pdev) >> +{ >> + struct sys_bus_data *pdata; >> + void __iomem *mmio; >> + struct device_node *np = pdev->dev.of_node; > > I'd be consistent. You use dev_of_node() accessor elsewhere, so should > use it here as well >> + >> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + return -ENOMEM; >> + >> + mmio = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(mmio)) >> + return PTR_ERR(mmio); >> + >> + pdata->sys_bus_base = mmio; >> + pdata->pconf = of_device_get_match_data(&pdev->dev); >> + pdata->dev = &pdev->dev; >> + >> + platform_set_drvdata(pdev, pdata); > > Does this get used? I can't immediately spot where but maybe I just > missed it. >
Not for now :)
>> + >> + stm32_sys_bus_populate(pdata); >> + >> + /* Populate all available nodes */ >> + return of_platform_populate(np, NULL, NULL, &pdev->dev); > > As np only used here, I'd not bother with the local variable in this function. >
Agreed
>> +} >> + >> +static const struct stm32_sys_bus_match_data stm32mp15_sys_bus_data = { > > Naming a structure after where it comes from is a little unusual and > confusion when a given call gets it from somewhere else. > > I'd expect it to be named after what sort of thing it contains. > stm32_sys_bus_info or something like that. >
Then, this shall be removed thanks to the read to hardware registers.
>> + .max_entries = STM32MP15_ETZPC_ENTRIES, >> +}; >> + >> +static const struct stm32_sys_bus_match_data stm32mp13_sys_bus_data = { >> + .max_entries = STM32MP13_ETZPC_ENTRIES, >> +}; >> + >> +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 }, > > Alphabetical order usually preferred when there isn't a strong reason for > another choice. >
I second that
>> + {} >> +}; >> +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); >> + > > Unwanted trailing blank line. >
Good spot, thanks
>
Best regards, Gatien
| |