Re: [PATCH v3 2/5] bus: stm32: Introduce firewall controller helpers

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, May 05, 2020 at 09:33:05AM +0200, Benjamin Gaignard wrote:
> The goal of these helpers are to offer an interface for the
> hardware blocks controlling bus accesses rights.
> 
> Bus firewall controllers are typically used to control if a
> hardware block can perform read or write operations on bus.
> 
> Smarter firewall controllers could be able to define accesses
> rights per hardware blocks to control where they can read
> or write.
> 
> Firewall controller configurations are provided in device node,
> parsed by the helpers and send to the driver to apply them.
> Each controller may need different number and type of inputs
> to configure the firewall so device-tree properties size have to
> be define by using "#firewall-cells".
> Firewall configurations properties have to be named "firewall-X"
> on device node.
> "firewall-names" keyword can also be used to give a name to
> a specific configuration.
> 
> Example of device-tree:
> ctrl0: firewall@0 {
> 	#firewall-cells = <2>;
>       };
> 
> foo: foo@0 {
> 	firewall-names = "default", "setting1";
> 	firewall-0 = <&ctrl0 1 2>;
> 	firewall-1 = <&ctrl0 3 4>;
> };
> 
> Configurations could be applied with functions like
> firewall_set_config_by_index() or firewall_set_config_by_name().
> 
> firewall_set_default_config() function will apply the
> configuration named "default" (if existing) or the configuration
> with index 0 (i.e. firewall-0).
> 
> Drivers could register/unregister themselves be calling
> firewall_register/firewall_unregister functions.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  drivers/bus/Kconfig          |   2 +
>  drivers/bus/Makefile         |   2 +
>  drivers/bus/stm32/Kconfig    |   3 +
>  drivers/bus/stm32/Makefile   |   1 +
>  drivers/bus/stm32/firewall.c | 266 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/bus/stm32/firewall.h |  75 ++++++++++++
>  6 files changed, 349 insertions(+)
>  create mode 100644 drivers/bus/stm32/Kconfig
>  create mode 100644 drivers/bus/stm32/Makefile
>  create mode 100644 drivers/bus/stm32/firewall.c
>  create mode 100644 drivers/bus/stm32/firewall.h
> 
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index 6d4e4497b59b..843b356322d9 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -203,4 +203,6 @@ config DA8XX_MSTPRI
>  source "drivers/bus/fsl-mc/Kconfig"
>  source "drivers/bus/mhi/Kconfig"
>  
> +source "drivers/bus/stm32/Kconfig"
> +
>  endmenu
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index 05f32cd694a4..5e0e34b10235 100644
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -37,3 +37,5 @@ obj-$(CONFIG_DA8XX_MSTPRI)	+= da8xx-mstpri.o
>  
>  # MHI
>  obj-$(CONFIG_MHI_BUS)		+= mhi/
> +
> +obj-$(CONFIG_MACH_STM32MP157) 	+= stm32/
> \ No newline at end of file
> diff --git a/drivers/bus/stm32/Kconfig b/drivers/bus/stm32/Kconfig
> new file mode 100644
> index 000000000000..57221e833e2d
> --- /dev/null
> +++ b/drivers/bus/stm32/Kconfig
> @@ -0,0 +1,3 @@
> +config FIREWALL_CONTROLLERS
> +	bool "Support of bus firewall controllers"
> +	depends on OF
> diff --git a/drivers/bus/stm32/Makefile b/drivers/bus/stm32/Makefile
> new file mode 100644
> index 000000000000..eb6b978d6450
> --- /dev/null
> +++ b/drivers/bus/stm32/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_FIREWALL_CONTROLLERS) += firewall.o
> diff --git a/drivers/bus/stm32/firewall.c b/drivers/bus/stm32/firewall.c
> new file mode 100644
> index 000000000000..95f716cf926f
> --- /dev/null
> +++ b/drivers/bus/stm32/firewall.c
> @@ -0,0 +1,266 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +
> +#include "firewall.h"
> +
> +/* Mutex taken to protect firewall_list */
> +static DEFINE_MUTEX(firewall_list_mutex);
> +
> +/* Global list of firewall control devices */
> +static LIST_HEAD(firewall_list);

Why is that needed?  Why can't you just walk the list of devices on this
"bus/class" if you really wanted to?

Along those lines, why is this going around the driver model and
ignoring it?  Shouldn't this be a bus and you have devices attached to
it of the specific type?


greg k-h