Re: [RFC PATCH 5/7] bus: stm32_sys_bus: add support for STM32MP15 and STM32MP13 system bus

[Gatien CHEVALLIER]

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@xxxxxx>
> > Signed-off-by: Gatien Chevallier <gatien.chevallier@xxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxx
> >   S:	Maintained
> >   F:	drivers/spi/spi-stm32.c
> >   
> > +ST STM32 SYS BUS DRIVER
> > +M:	Gatien Chevallier <gatien.chevallier@xxxxxxxxxxx>
> > +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@xxxxxxxx>
> >   L:	linux-hwmon@xxxxxxxxxxxxxxx
> > 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