Message-ID: <CAKfTPtD5EwmDQJvx2yhG3cd6hKh-qJYW15DpW5cdPkgUc2tFfA@mail.gmail.com>
Date:   Wed, 27 Jan 2021 17:21:15 +0100
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Cristian Marussi <cristian.marussi@....com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        LAK <linux-arm-kernel@...ts.infradead.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Lukasz Luba <lukasz.luba@....com>, james.quinlan@...adcom.com,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Etienne Carriere <etienne.carriere@...aro.org>,
        Thara Gopinath <thara.gopinath@...aro.org>,
        Souvik Chakravarty <souvik.chakravarty@....com>
Subject: Re: [PATCH v5 0/36] SCMI vendor protocols and modularization

Hi Cristian,


On Tue, 12 Jan 2021 at 20:20, Cristian Marussi <cristian.marussi@....com> wrote:
>
> Hi all,
>
> The current SCMI implementation does not provide an interface to easily
> develop and include a custom vendor protocol implementation as prescribed
> by the SCMI standard, also because, there is not currently any custom
> protocol in the upstream to justify the development of a custom interface
> and its maintenance.
>
> Moreover the current interface exposes protocol operations to the SCMI
> driver users attaching per-protocol operations directly to the handle
> structure, which, in this way, tends to grow indefinitely for each new
> protocol addition.
>
> Beside this, protocols private data are also exposed via handle *_priv
> pointers, making such private data accessible also to the SCMI drivers
> even if neither really needed nor advisable.
>
> This series wants to address this by simplifying the SCMI protocols
> interface and reducing it, roughly, to these common generic operations:
>
>         - handle->devm_get_protocol()
>         -  handle->devm_put_protocol()
>         - handle->notify_ops()
>
> All protocols' private data pointers are removed from handle too and made
> accessible only to the protocols code through dedicated internal helpers.
>
> The concept of protocol handle is also introduced in the SCMI protocol code
> to represent a protocol instance initialized against a specific SCMI
> instance (handle), so that all the new protocol code uses such protocol
> handles wherever previously SCMI handle was used: this enable tigther
> control of what is exposed to the protocol code vs the SCMI drivers.
>
> Moreover protocol initialization is moved away from device probe and now
> happens on demand when the first user shows up (first .get_protocol), while
> de-initialization is performed once the last user of the protocol, even in
> terms of registered notifications callback, is gone, with the SCMI core
> taking care to perform all the needed underlying resource accounting.
>
> This way any new future standard or custom protocol implementation will
> expose a common unified interface which does not need to be extended
> endlessly: no need to maintain a custom interface only for vendor protos.
> SCMI drivers written on top of standard or custom protocols will use this
> same common interface to access any protocol operations.
>
> All existent upstream SCMI drivers are converted to this new interface.
>
> In order to make this migration painless and to avoid the need of a big
> un-mergeable jumbo patch touching all over the protocols and drivers (like
> it was in v2), since v3 the migration process has been heavily split with a
> bit of transient code added along the way (to preserve bisectability) and
> finally removed towards the ends of the series.
> Protocols and SCMI drivers migration to the new interface happens along
> patches 10->29.
>
> Note that even in v5 all the related SCMI drivers maintainers are still NOT
> CC'ed given I am still sort of gather consensus about the interface itself.
>
> Leveraging this new centralized and common initialization flow we took
> care also to refactor and simplify protocol-events registration and remove
> *notify_priv from the handle interface making it accessible only to the
> notification core.
>
> Patch 35 builds on top of this new interface and introduces a mechanism to
> define an SCMI protocol as a full blown module (possibly loadable) while
> leaving the core dealing with proper resource accounting.
> Standard protocols are still kept as builtins in this series, though.
>
> Finally, patch 36 introduces dynamic SCMI devices creation to avoid having
> to update the static module device table in the core each time a new driver
> is added.
>
> The whole SCMI stack can still be built alternatively as a module, with all
> the standard protocols included in scmi-module.ko in such a case.
>
> On top of this series an example SCMI Custom protocol 0x99 and related
> SCMI Custom Dummy driver has been built and it is available at [1] as a
> series of DEBUG patches on top this same series.
>
> The series is currently based on for-next/scmi [2] on top of:
>
> commit 6054d97ab512 ("MAINTAINERS: Update ARM SCMI entry")
>
> Any feedback welcome.

I have tested your patchset on my setup which includes:

- scmi performance protocol
- scmi power domain protocol
- scmi clock protocol
- scmi sensor protocol
- scmi notification
- registration of a custom scmi module (still under dev)
- 2 scmi channels

And everything worked fine. So FWIW,

Tested-by: Vincent Guittot <vincent.guittot@...aro.org>

>
> Thanks,
>
> Cristian
>
> ---
> v4 --> v5
> - using standard kernel list instead of ad-hoc lists in 36/36
> - renamed devm_get/put_ops to devm_get/put_protocol
> - dropped RFC patch on non devres get/put_ops
>
> v3 --> v4
> - rebased on sudeep/for-next/scmi v5.11-rc1
> - added a few comments more
>
> v2 --> v3
> - added dynamic SCMI devices creation (getting rid of static device table)
> - heavy split of protocols and drivers migrations to the new interface
> - rebased on top of next-20201201 so migrating also:
>   + SCMIv3.0 Voltage Domain protocol & SCMI Regulator
>   + SCMIv3.0 Sensor Extensions
>
> v1 --> v2
> - rebased on for-next/scmi v5.10-rc1
> - introduced protocol handles
> - added devres managed devm_ variant for protocols operations
> - made all scmi_protocol refs const
> - introduced IDR to handle protocols instead of static array
> - refactored code around fast path
>
> [1]:https://gitlab.arm.com/linux-arm/linux-cm/-/commits/scmi_modules_ext_V5/
> [2]:https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi
>
>
> Cristian Marussi (36):
>   firmware: arm_scmi: review protocol registration interface
>   firmware: arm_scmi: introduce protocol handle definitions
>   firmware: arm_scmi: introduce devres get/put protocols operations
>   firmware: arm_scmi: make notifications aware of protocols users
>   firmware: arm_scmi: introduce new devres notification ops
>   firmware: arm_scmi: refactor events registration
>   firmware: arm_scmi: convert events registration to protocol handles
>   firmware: arm_scmi: add new protocol handle core xfer ops
>   firmware: arm_scmi: add helper to access revision area memory
>   firmware: arm_scmi: port Base protocol to new interface
>   firmware: arm_scmi: port Perf protocol to new protocols interface
>   cpufreq: scmi: port driver to the new scmi_perf_proto_ops interface
>   firmware: arm_scmi: remove legacy scmi_perf_ops protocol interface
>   firmware: arm_scmi: port Power protocol to new protocols interface
>   firmware: arm_scmi: port GenPD driver to the new scmi_power_proto_ops
>     interface
>   firmware: arm_scmi: remove legacy scmi_power_ops protocol interface
>   firmware: arm_scmi: port Clock protocol to new protocols interface
>   clk: scmi: port driver to the new scmi_clk_proto_ops interface
>   firmware: arm_scmi: remove legacy scmi_clk_ops protocol interface
>   firmware: arm_scmi: port Reset protocol to new protocols interface
>   reset: reset-scmi: port driver to the new scmi_reset_proto_ops
>     interface
>   firmware: arm_scmi: remove legacy scmi_reset_ops protocol interface
>   firmware: arm_scmi: port Sensor protocol to new protocols interface
>   hwmon: (scmi) port driver to the new scmi_sensor_proto_ops interface
>   firmware: arm_scmi: remove legacy scmi_sensor_ops protocol interface
>   firmware: arm_scmi: port SystemPower protocol to new protocols
>     interface
>   firmware: arm_scmi: port Voltage protocol to new protocols interface
>   regulator: scmi: port driver to the new scmi_voltage_proto_ops
>     interface
>   firmware: arm_scmi: remove legacy scmi_voltage_ops protocol interface
>   firmware: arm_scmi: make references to handle const
>   firmware: arm_scmi: cleanup legacy protocol init code
>   firmware: arm_scmi: cleanup unused core xfer wrappers
>   firmware: arm_scmi: cleanup events registration transient code
>   firmware: arm_scmi: make notify_priv really private
>   firmware: arm_scmi: add protocol modularization support
>   firmware: arm_scmi: add dynamic scmi devices creation
>
>  drivers/clk/clk-scmi.c                     |  27 +-
>  drivers/cpufreq/scmi-cpufreq.c             |  37 +-
>  drivers/firmware/arm_scmi/base.c           | 140 ++--
>  drivers/firmware/arm_scmi/bus.c            | 100 ++-
>  drivers/firmware/arm_scmi/clock.c          | 129 ++--
>  drivers/firmware/arm_scmi/common.h         | 117 +++-
>  drivers/firmware/arm_scmi/driver.c         | 759 +++++++++++++++++++--
>  drivers/firmware/arm_scmi/notify.c         | 297 ++++++--
>  drivers/firmware/arm_scmi/notify.h         |  38 +-
>  drivers/firmware/arm_scmi/perf.c           | 262 +++----
>  drivers/firmware/arm_scmi/power.c          | 134 ++--
>  drivers/firmware/arm_scmi/reset.c          | 146 ++--
>  drivers/firmware/arm_scmi/scmi_pm_domain.c |  26 +-
>  drivers/firmware/arm_scmi/sensors.c        | 230 ++++---
>  drivers/firmware/arm_scmi/system.c         |  61 +-
>  drivers/firmware/arm_scmi/voltage.c        | 122 ++--
>  drivers/hwmon/scmi-hwmon.c                 |  24 +-
>  drivers/regulator/scmi-regulator.c         |  40 +-
>  drivers/reset/reset-scmi.c                 |  33 +-
>  include/linux/scmi_protocol.h              | 183 ++---
>  20 files changed, 1996 insertions(+), 909 deletions(-)
>
> --
> 2.17.1
>

Powered by blists - more mailing lists