[PATCH 0/3] firmware: arm_scmi: Enable building SCMI as module

[PATCH 0/3] firmware: arm_scmi: Enable building SCMI as module

[Sudeep Holla]

Hi,

Though it was initially developed as module, so some reason(I can't
recollect why apart from some structuring arounf the way bus and
protocols were initialised), it was merged as a built-in only driver.

Now, there is a need to build this as modules. This is mainly needed
by virtio transport. This also aligns well with GKI modularisation
efforts.

--
Regards,
Sudeep

Sudeep Holla (3):
  firmware: arm_scmi: Move scmi bus init and exit calls into the driver
  firmware: arm_scmi: Move scmi protocols initialisation into the driver
  firmware: arm_scmi: Enable building as a single module

 drivers/firmware/Kconfig            |  2 +-
 drivers/firmware/Makefile           |  2 +-
 drivers/firmware/arm_scmi/Makefile  |  4 +++-
 drivers/firmware/arm_scmi/bus.c     |  6 ++----
 drivers/firmware/arm_scmi/clock.c   |  7 +------
 drivers/firmware/arm_scmi/common.h  | 24 ++++++++++++++++++++++++
 drivers/firmware/arm_scmi/driver.c  | 26 +++++++++++++++++++++++++-
 drivers/firmware/arm_scmi/perf.c    |  7 +------
 drivers/firmware/arm_scmi/power.c   |  7 +------
 drivers/firmware/arm_scmi/reset.c   |  7 +------
 drivers/firmware/arm_scmi/sensors.c |  7 +------
 include/linux/scmi_protocol.h       |  2 +-
 12 files changed, 62 insertions(+), 39 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] firmware: arm_scmi: Move scmi bus init and exit calls into the driver

[Sudeep Holla]

In preparation to enable building scmi as a single module, let us move
the scmi bus {de-,}initialisation call into the driver.

The main reason for this is to keep it simple instead of maintaining
it as separate modules and dealing with all possible initcall races
and deferred probe handling. We can move it as separate modules if
needed in future.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/bus.c    |  6 ++----
 drivers/firmware/arm_scmi/common.h |  3 +++
 drivers/firmware/arm_scmi/driver.c | 16 +++++++++++++++-
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index db55c43a2cbd..1377ec76a45d 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -230,7 +230,7 @@ static void scmi_devices_unregister(void)
 	bus_for_each_dev(&scmi_bus_type, NULL, NULL, __scmi_devices_unregister);
 }
 
-static int __init scmi_bus_init(void)
+int __init scmi_bus_init(void)
 {
 	int retval;
 
@@ -240,12 +240,10 @@ static int __init scmi_bus_init(void)
 
 	return retval;
 }
-subsys_initcall(scmi_bus_init);
 
-static void __exit scmi_bus_exit(void)
+void __exit scmi_bus_exit(void)
 {
 	scmi_devices_unregister();
 	bus_unregister(&scmi_bus_type);
 	ida_destroy(&scmi_bus_id);
 }
-module_exit(scmi_bus_exit);
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index c113e578cc6c..5fa42eba6de7 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -156,6 +156,9 @@ void scmi_setup_protocol_implemented(const struct scmi_handle *handle,
 
 int scmi_base_protocol_init(struct scmi_handle *h);
 
+int __init scmi_bus_init(void);
+void __exit scmi_bus_exit(void);
+
 /* SCMI Transport */
 /**
  * struct scmi_chan_info - Structure representing a SCMI channel information
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 03ec74242c14..f4d9601c053f 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -928,7 +928,21 @@ static struct platform_driver scmi_driver = {
 	.remove = scmi_remove,
 };
 
-module_platform_driver(scmi_driver);
+static int __init scmi_driver_init(void)
+{
+	scmi_bus_init();
+
+	return platform_driver_register(&scmi_driver);
+}
+module_init(scmi_driver_init);
+
+static void __exit scmi_driver_exit(void)
+{
+	scmi_bus_exit();
+
+	return platform_driver_unregister(&scmi_driver);
+}
+module_exit(scmi_driver_exit);
 
 MODULE_ALIAS("platform: arm-scmi");
 MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] firmware: arm_scmi: Move scmi protocols initialisation into the driver

[Sudeep Holla]

In preparation to enable building SCMI as a single module, let us move
the SCMI protocol initialisation call into the driver. This enables us
to also add de-initialisation of the SCMI protocols.

The main reason for this is to keep it simple instead of maintaining
it as separate modules and dealing with all possible initcall races
and deferred probe handling. We can move it as separate modules if
needed in future.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/clock.c   |  7 +------
 drivers/firmware/arm_scmi/common.h  | 21 +++++++++++++++++++++
 drivers/firmware/arm_scmi/driver.c  | 10 ++++++++++
 drivers/firmware/arm_scmi/perf.c    |  7 +------
 drivers/firmware/arm_scmi/power.c   |  7 +------
 drivers/firmware/arm_scmi/reset.c   |  7 +------
 drivers/firmware/arm_scmi/sensors.c |  7 +------
 7 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 75e39882746e..606396f748f0 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -364,9 +364,4 @@ static int scmi_clock_protocol_init(struct scmi_handle *handle)
 	return 0;
 }
 
-static int __init scmi_clock_init(void)
-{
-	return scmi_protocol_register(SCMI_PROTOCOL_CLOCK,
-				      &scmi_clock_protocol_init);
-}
-subsys_initcall(scmi_clock_init);
+DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_CLOCK, clock)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 5fa42eba6de7..6d98a6c47005 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -159,6 +159,27 @@ int scmi_base_protocol_init(struct scmi_handle *h);
 int __init scmi_bus_init(void);
 void __exit scmi_bus_exit(void);
 
+#define DECLARE_SCMI_INIT_EXIT(func)		\
+	int __init scmi_##func##_init(void);	\
+	void __exit scmi_##func##_exit(void)
+DECLARE_SCMI_INIT_EXIT(clock);
+DECLARE_SCMI_INIT_EXIT(perf);
+DECLARE_SCMI_INIT_EXIT(power);
+DECLARE_SCMI_INIT_EXIT(reset);
+DECLARE_SCMI_INIT_EXIT(sensors);
+DECLARE_SCMI_INIT_EXIT(bus);
+
+#define DEFINE_SCMI_PROTOCOL_INIT_EXIT(id, name)	\
+int __init scmi_##name##_init(void)			\
+{ \
+	return scmi_protocol_register((id), &scmi_##name##_protocol_init); \
+} \
+\
+void __exit scmi_##name##_exit(void) \
+{ \
+	scmi_protocol_unregister((id)); \
+}
+
 /* SCMI Transport */
 /**
  * struct scmi_chan_info - Structure representing a SCMI channel information
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index f4d9601c053f..2a1396b74fa5 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -931,6 +931,11 @@ static struct platform_driver scmi_driver = {
 static int __init scmi_driver_init(void)
 {
 	scmi_bus_init();
+	scmi_clock_init();
+	scmi_perf_init();
+	scmi_power_init();
+	scmi_reset_init();
+	scmi_sensors_init();
 
 	return platform_driver_register(&scmi_driver);
 }
@@ -939,6 +944,11 @@ module_init(scmi_driver_init);
 static void __exit scmi_driver_exit(void)
 {
 	scmi_bus_exit();
+	scmi_clock_exit();
+	scmi_perf_exit();
+	scmi_power_exit();
+	scmi_reset_exit();
+	scmi_sensors_exit();
 
 	return platform_driver_unregister(&scmi_driver);
 }
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 3e1e87012c95..e0ec1a78d2ad 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -890,9 +890,4 @@ static int scmi_perf_protocol_init(struct scmi_handle *handle)
 	return 0;
 }
 
-static int __init scmi_perf_init(void)
-{
-	return scmi_protocol_register(SCMI_PROTOCOL_PERF,
-				      &scmi_perf_protocol_init);
-}
-subsys_initcall(scmi_perf_init);
+DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_PERF, perf)
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 46f213644c49..54926d1f381c 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -301,9 +301,4 @@ static int scmi_power_protocol_init(struct scmi_handle *handle)
 	return 0;
 }
 
-static int __init scmi_power_init(void)
-{
-	return scmi_protocol_register(SCMI_PROTOCOL_POWER,
-				      &scmi_power_protocol_init);
-}
-subsys_initcall(scmi_power_init);
+DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_POWER, power)
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 3691bafca057..4b91d39cfe21 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -313,9 +313,4 @@ static int scmi_reset_protocol_init(struct scmi_handle *handle)
 	return 0;
 }
 
-static int __init scmi_reset_init(void)
-{
-	return scmi_protocol_register(SCMI_PROTOCOL_RESET,
-				      &scmi_reset_protocol_init);
-}
-subsys_initcall(scmi_reset_init);
+DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_RESET, reset)
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 1af0ad362e82..37dc187cafea 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -365,9 +365,4 @@ static int scmi_sensors_protocol_init(struct scmi_handle *handle)
 	return 0;
 }
 
-static int __init scmi_sensors_init(void)
-{
-	return scmi_protocol_register(SCMI_PROTOCOL_SENSOR,
-				      &scmi_sensors_protocol_init);
-}
-subsys_initcall(scmi_sensors_init);
+DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_SENSOR, sensors)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] firmware: arm_scmi: Enable building as a single module

[Sudeep Holla]

Now, with all the plumbing in place to enable building scmi as a module
instead of built-in modules, let us enable the same.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/Kconfig           | 2 +-
 drivers/firmware/Makefile          | 2 +-
 drivers/firmware/arm_scmi/Makefile | 4 +++-
 include/linux/scmi_protocol.h      | 2 +-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index fbd785dd0513..afdbebba628a 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -7,7 +7,7 @@
 menu "Firmware Drivers"
 
 config ARM_SCMI_PROTOCOL
-	bool "ARM System Control and Management Interface (SCMI) Message Protocol"
+	tristate "ARM System Control and Management Interface (SCMI) Message Protocol"
 	depends on ARM || ARM64 || COMPILE_TEST
 	depends on MAILBOX
 	help
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 99510be9f5ed..5e013b6a3692 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -22,7 +22,7 @@ obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
 
-obj-$(CONFIG_ARM_SCMI_PROTOCOL)	+= arm_scmi/
+obj-y				+= arm_scmi/
 obj-y				+= broadcom/
 obj-y				+= meson/
 obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index 6f9cbc4aef22..0bf0761753db 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -1,9 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-y	= scmi-bus.o scmi-driver.o scmi-protocols.o scmi-transport.o
 scmi-bus-y = bus.o
 scmi-driver-y = driver.o notify.o
 scmi-transport-y = shmem.o
 scmi-transport-$(CONFIG_MAILBOX) += mailbox.o
 scmi-transport-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smc.o
 scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
+scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \
+		    $(scmi-transport-y)
+obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
 obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 7e5dd7d1e221..935a8f77b5a0 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -335,7 +335,7 @@ struct scmi_driver {
 
 #define to_scmi_driver(d) container_of(d, struct scmi_driver, driver)
 
-#ifdef CONFIG_ARM_SCMI_PROTOCOL
+#if IS_REACHABLE(CONFIG_ARM_SCMI_PROTOCOL)
 int scmi_driver_register(struct scmi_driver *driver,
 			 struct module *owner, const char *mod_name);
 void scmi_driver_unregister(struct scmi_driver *driver);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] firmware: arm_scmi: Enable building SCMI as module

[Sudeep Holla]

On Mon, Sep 07, 2020 at 12:29:17PM +0100, Sudeep Holla wrote:
> Hi,
> 
> Though it was initially developed as module, so some reason(I can't
> recollect why apart from some structuring arounf the way bus and
> protocols were initialised), it was merged as a built-in only driver.
> 
> Now, there is a need to build this as modules. This is mainly needed
> by virtio transport. This also aligns well with GKI modularisation
> efforts.
> 

I forgot to add another patch [1] which was in my tree when I tested
this and completely forgot about the dependency.

-- 
Regards,
Sudeep

[1] https://lore.kernel.org/linux-devicetree/20200829170923.29949-5-sudeep.holla@arm.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] firmware: arm_scmi: Enable building SCMI as module

[Cristian Marussi]

Hi Sudeep,

On Mon, Sep 07, 2020 at 12:29:17PM +0100, Sudeep Holla wrote:
> Hi,
> 
> Though it was initially developed as module, so some reason(I can't
> recollect why apart from some structuring arounf the way bus and
> protocols were initialised), it was merged as a built-in only driver.
> 
> Now, there is a need to build this as modules. This is mainly needed
> by virtio transport. This also aligns well with GKI modularisation
> efforts.
> 

This works for me as a module, but it gave me issues when compiled builtin
since the some SCMI drivers (hwmon, cpufreq) look for the SCMI bus too early
when both them and the core are compiled as builtins.

[    2.226029] rtc-efi rtc-efi.0: setting system clock to 2020-09-07T10:51:36 UTC (1599475896)
[    2.235091] rtc-pl031 1c170000.rtc: registered as rtc1
[    2.240767] i2c /dev entries driver
[    2.246522] Driver 'scmi-hwmon' was unable to register with bus_type 'scmi_protocol' because the bus was not initialized.
[    2.259037] sp805-wdt 1c0f0000.wdt: registration successful
[    2.265464] Driver 'scmi-cpufreq' was unable to register with bus_type 'scmi_protocol' because the bus was not initialized.
[    2.278905] mmci-pl18x 1c050000.mmci: mmc0: PL180 manf 41 rev0 at 0x1c050000 irq 8,0 (pio)

This dirty trick below solves for me though the builtin issue (and still runs
fine when modularized):

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 2a1396b74fa5..b69bb174344d 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -939,7 +939,11 @@ static int __init scmi_driver_init(void)
 
        return platform_driver_register(&scmi_driver);
 }
+#ifdef MODULE
 module_init(scmi_driver_init);
+#else
+subsys_initcall(scmi_driver_init);
+#endif
 
 static void __exit scmi_driver_exit(void)
 {

Thanks

Cristian

> --
> Regards,
> Sudeep
> 
> Sudeep Holla (3):
>   firmware: arm_scmi: Move scmi bus init and exit calls into the driver
>   firmware: arm_scmi: Move scmi protocols initialisation into the driver
>   firmware: arm_scmi: Enable building as a single module
> 
>  drivers/firmware/Kconfig            |  2 +-
>  drivers/firmware/Makefile           |  2 +-
>  drivers/firmware/arm_scmi/Makefile  |  4 +++-
>  drivers/firmware/arm_scmi/bus.c     |  6 ++----
>  drivers/firmware/arm_scmi/clock.c   |  7 +------
>  drivers/firmware/arm_scmi/common.h  | 24 ++++++++++++++++++++++++
>  drivers/firmware/arm_scmi/driver.c  | 26 +++++++++++++++++++++++++-
>  drivers/firmware/arm_scmi/perf.c    |  7 +------
>  drivers/firmware/arm_scmi/power.c   |  7 +------
>  drivers/firmware/arm_scmi/reset.c   |  7 +------
>  drivers/firmware/arm_scmi/sensors.c |  7 +------
>  include/linux/scmi_protocol.h       |  2 +-
>  12 files changed, 62 insertions(+), 39 deletions(-)
> 
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] firmware: arm_scmi: Enable building SCMI as module

[Sudeep Holla]

On Mon, Sep 07, 2020 at 04:25:13PM +0100, Cristian Marussi wrote:
> Hi Sudeep,
> 
> On Mon, Sep 07, 2020 at 12:29:17PM +0100, Sudeep Holla wrote:
> > Hi,
> > 
> > Though it was initially developed as module, so some reason(I can't
> > recollect why apart from some structuring arounf the way bus and
> > protocols were initialised), it was merged as a built-in only driver.
> > 
> > Now, there is a need to build this as modules. This is mainly needed
> > by virtio transport. This also aligns well with GKI modularisation
> > efforts.
> > 
> 
> This works for me as a module, but it gave me issues when compiled builtin
> since the some SCMI drivers (hwmon, cpufreq) look for the SCMI bus too early
> when both them and the core are compiled as builtins.
>

Thanks for testing.

> [    2.226029] rtc-efi rtc-efi.0: setting system clock to 2020-09-07T10:51:36 UTC (1599475896)
> [    2.235091] rtc-pl031 1c170000.rtc: registered as rtc1
> [    2.240767] i2c /dev entries driver
> [    2.246522] Driver 'scmi-hwmon' was unable to register with bus_type 'scmi_protocol' because the bus was not initialized.
> [    2.259037] sp805-wdt 1c0f0000.wdt: registration successful
> [    2.265464] Driver 'scmi-cpufreq' was unable to register with bus_type 'scmi_protocol' because the bus was not initialized.
> [    2.278905] mmci-pl18x 1c050000.mmci: mmc0: PL180 manf 41 rev0 at 0x1c050000 irq 8,0 (pio)
> 
> This dirty trick below solves for me though the builtin issue (and still runs
> fine when modularized):
>

In fact the bus init was subsys previously, so make sense to move that
too. I don't think it is a hack. Since scmi_bus needs to be available
for all scmi_drivers to be registered, it looks valid for me.

> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 2a1396b74fa5..b69bb174344d 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -939,7 +939,11 @@ static int __init scmi_driver_init(void)
>  
>         return platform_driver_register(&scmi_driver);
>  }
> +#ifdef MODULE
>  module_init(scmi_driver_init);
> +#else
> +subsys_initcall(scmi_driver_init);
> +#endif
>  

Indeed, just subsys_initcall will suffice. It is module_init when built as
module, so no need for us to define that explicitly.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] firmware: arm_scmi: Move scmi protocols initialisation into the driver

[Cristian Marussi]

On Mon, Sep 07, 2020 at 12:29:19PM +0100, Sudeep Holla wrote:
> In preparation to enable building SCMI as a single module, let us move
> the SCMI protocol initialisation call into the driver. This enables us
> to also add de-initialisation of the SCMI protocols.
> 
> The main reason for this is to keep it simple instead of maintaining
> it as separate modules and dealing with all possible initcall races
> and deferred probe handling. We can move it as separate modules if
> needed in future.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/clock.c   |  7 +------
>  drivers/firmware/arm_scmi/common.h  | 21 +++++++++++++++++++++
>  drivers/firmware/arm_scmi/driver.c  | 10 ++++++++++
>  drivers/firmware/arm_scmi/perf.c    |  7 +------
>  drivers/firmware/arm_scmi/power.c   |  7 +------
>  drivers/firmware/arm_scmi/reset.c   |  7 +------
>  drivers/firmware/arm_scmi/sensors.c |  7 +------
>  7 files changed, 36 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index 75e39882746e..606396f748f0 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -364,9 +364,4 @@ static int scmi_clock_protocol_init(struct scmi_handle *handle)
>  	return 0;
>  }
>  
> -static int __init scmi_clock_init(void)
> -{
> -	return scmi_protocol_register(SCMI_PROTOCOL_CLOCK,
> -				      &scmi_clock_protocol_init);
> -}
> -subsys_initcall(scmi_clock_init);
> +DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_CLOCK, clock)
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 5fa42eba6de7..6d98a6c47005 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -159,6 +159,27 @@ int scmi_base_protocol_init(struct scmi_handle *h);
>  int __init scmi_bus_init(void);
>  void __exit scmi_bus_exit(void);
>  
> +#define DECLARE_SCMI_INIT_EXIT(func)		\
> +	int __init scmi_##func##_init(void);	\
> +	void __exit scmi_##func##_exit(void)
> +DECLARE_SCMI_INIT_EXIT(clock);
> +DECLARE_SCMI_INIT_EXIT(perf);
> +DECLARE_SCMI_INIT_EXIT(power);
> +DECLARE_SCMI_INIT_EXIT(reset);
> +DECLARE_SCMI_INIT_EXIT(sensors);
> +DECLARE_SCMI_INIT_EXIT(bus);
> +

Can we call these protocols' functions (and related macros) something like:

	scmi_##PROTO##_load/_unload or _register/_unregister 

given that in SCMI stack we usually intend something else with protocol
initialization and in fact each protocol has its own dedicated protocol_init
function which is called at a different time.


> +#define DEFINE_SCMI_PROTOCOL_INIT_EXIT(id, name)	\
> +int __init scmi_##name##_init(void)			\
> +{ \
> +	return scmi_protocol_register((id), &scmi_##name##_protocol_init); \
> +} \
> +\
> +void __exit scmi_##name##_exit(void) \
> +{ \
> +	scmi_protocol_unregister((id)); \
> +}
> +
>  /* SCMI Transport */
>  /**
>   * struct scmi_chan_info - Structure representing a SCMI channel information
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index f4d9601c053f..2a1396b74fa5 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -931,6 +931,11 @@ static struct platform_driver scmi_driver = {
>  static int __init scmi_driver_init(void)
>  {
>  	scmi_bus_init();
> +	scmi_clock_init();
> +	scmi_perf_init();
> +	scmi_power_init();
> +	scmi_reset_init();
> +	scmi_sensors_init();
>  
>  	return platform_driver_register(&scmi_driver);
>  }
> @@ -939,6 +944,11 @@ module_init(scmi_driver_init);
>  static void __exit scmi_driver_exit(void)
>  {
>  	scmi_bus_exit();

Shouldn't this bus_exit() be issued in reverse oerder at the end
after protocols have being _exited() ?

Cheers,

Cristian

> +	scmi_clock_exit();
> +	scmi_perf_exit();
> +	scmi_power_exit();
> +	scmi_reset_exit();
> +	scmi_sensors_exit();
>  
>  	return platform_driver_unregister(&scmi_driver);
>  }
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index 3e1e87012c95..e0ec1a78d2ad 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -890,9 +890,4 @@ static int scmi_perf_protocol_init(struct scmi_handle *handle)
>  	return 0;
>  }
>  
> -static int __init scmi_perf_init(void)
> -{
> -	return scmi_protocol_register(SCMI_PROTOCOL_PERF,
> -				      &scmi_perf_protocol_init);
> -}
> -subsys_initcall(scmi_perf_init);
> +DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_PERF, perf)
> diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
> index 46f213644c49..54926d1f381c 100644
> --- a/drivers/firmware/arm_scmi/power.c
> +++ b/drivers/firmware/arm_scmi/power.c
> @@ -301,9 +301,4 @@ static int scmi_power_protocol_init(struct scmi_handle *handle)
>  	return 0;
>  }
>  
> -static int __init scmi_power_init(void)
> -{
> -	return scmi_protocol_register(SCMI_PROTOCOL_POWER,
> -				      &scmi_power_protocol_init);
> -}
> -subsys_initcall(scmi_power_init);
> +DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_POWER, power)
> diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
> index 3691bafca057..4b91d39cfe21 100644
> --- a/drivers/firmware/arm_scmi/reset.c
> +++ b/drivers/firmware/arm_scmi/reset.c
> @@ -313,9 +313,4 @@ static int scmi_reset_protocol_init(struct scmi_handle *handle)
>  	return 0;
>  }
>  
> -static int __init scmi_reset_init(void)
> -{
> -	return scmi_protocol_register(SCMI_PROTOCOL_RESET,
> -				      &scmi_reset_protocol_init);
> -}
> -subsys_initcall(scmi_reset_init);
> +DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_RESET, reset)
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index 1af0ad362e82..37dc187cafea 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -365,9 +365,4 @@ static int scmi_sensors_protocol_init(struct scmi_handle *handle)
>  	return 0;
>  }
>  
> -static int __init scmi_sensors_init(void)
> -{
> -	return scmi_protocol_register(SCMI_PROTOCOL_SENSOR,
> -				      &scmi_sensors_protocol_init);
> -}
> -subsys_initcall(scmi_sensors_init);
> +DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_SENSOR, sensors)
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] firmware: arm_scmi: Move scmi protocols initialisation into the driver

[Sudeep Holla]

On Mon, Sep 07, 2020 at 07:06:01PM +0100, Cristian Marussi wrote:
> On Mon, Sep 07, 2020 at 12:29:19PM +0100, Sudeep Holla wrote:
> > In preparation to enable building SCMI as a single module, let us move
> > the SCMI protocol initialisation call into the driver. This enables us
> > to also add de-initialisation of the SCMI protocols.
> > 
> > The main reason for this is to keep it simple instead of maintaining
> > it as separate modules and dealing with all possible initcall races
> > and deferred probe handling. We can move it as separate modules if
> > needed in future.
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/clock.c   |  7 +------
> >  drivers/firmware/arm_scmi/common.h  | 21 +++++++++++++++++++++
> >  drivers/firmware/arm_scmi/driver.c  | 10 ++++++++++
> >  drivers/firmware/arm_scmi/perf.c    |  7 +------
> >  drivers/firmware/arm_scmi/power.c   |  7 +------
> >  drivers/firmware/arm_scmi/reset.c   |  7 +------
> >  drivers/firmware/arm_scmi/sensors.c |  7 +------
> >  7 files changed, 36 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> > index 75e39882746e..606396f748f0 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -364,9 +364,4 @@ static int scmi_clock_protocol_init(struct scmi_handle *handle)
> >  	return 0;
> >  }
> >  
> > -static int __init scmi_clock_init(void)
> > -{
> > -	return scmi_protocol_register(SCMI_PROTOCOL_CLOCK,
> > -				      &scmi_clock_protocol_init);
> > -}
> > -subsys_initcall(scmi_clock_init);
> > +DEFINE_SCMI_PROTOCOL_INIT_EXIT(SCMI_PROTOCOL_CLOCK, clock)
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index 5fa42eba6de7..6d98a6c47005 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -159,6 +159,27 @@ int scmi_base_protocol_init(struct scmi_handle *h);
> >  int __init scmi_bus_init(void);
> >  void __exit scmi_bus_exit(void);
> >  
> > +#define DECLARE_SCMI_INIT_EXIT(func)		\
> > +	int __init scmi_##func##_init(void);	\
> > +	void __exit scmi_##func##_exit(void)
> > +DECLARE_SCMI_INIT_EXIT(clock);
> > +DECLARE_SCMI_INIT_EXIT(perf);
> > +DECLARE_SCMI_INIT_EXIT(power);
> > +DECLARE_SCMI_INIT_EXIT(reset);
> > +DECLARE_SCMI_INIT_EXIT(sensors);
> > +DECLARE_SCMI_INIT_EXIT(bus);
> > +
> 
> Can we call these protocols' functions (and related macros) something like:
> 
> 	scmi_##PROTO##_load/_unload or _register/_unregister 
> 
> given that in SCMI stack we usually intend something else with protocol
> initialization and in fact each protocol has its own dedicated protocol_init
> function which is called at a different time.
>

Agreed, will fix it.

> 
> > +#define DEFINE_SCMI_PROTOCOL_INIT_EXIT(id, name)	\
> > +int __init scmi_##name##_init(void)			\
> > +{ \
> > +	return scmi_protocol_register((id), &scmi_##name##_protocol_init); \
> > +} \
> > +\
> > +void __exit scmi_##name##_exit(void) \
> > +{ \
> > +	scmi_protocol_unregister((id)); \
> > +}
> > +
> >  /* SCMI Transport */
> >  /**
> >   * struct scmi_chan_info - Structure representing a SCMI channel information
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index f4d9601c053f..2a1396b74fa5 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -931,6 +931,11 @@ static struct platform_driver scmi_driver = {
> >  static int __init scmi_driver_init(void)
> >  {
> >  	scmi_bus_init();
> > +	scmi_clock_init();
> > +	scmi_perf_init();
> > +	scmi_power_init();
> > +	scmi_reset_init();
> > +	scmi_sensors_init();
> >  
> >  	return platform_driver_register(&scmi_driver);
> >  }
> > @@ -939,6 +944,11 @@ module_init(scmi_driver_init);
> >  static void __exit scmi_driver_exit(void)
> >  {
> >  	scmi_bus_exit();
> 
> Shouldn't this bus_exit() be issued in reverse oerder at the end
> after protocols have being _exited() ?
> 

Ah right, will fix this too.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel