Re: [RFC v2 1/3] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[Oleksii Moisieiev]

On Fri, May 12, 2023 at 02:32:25PM +0200, Michal Simek wrote:
> 
> 
> On 5/12/23 14:31, Oleksii Moisieiev wrote:
> > On Fri, May 12, 2023 at 09:55:53AM +0100, Cristian Marussi wrote:
> > > On Fri, May 12, 2023 at 08:38:06AM +0000, Oleksii Moisieiev wrote:
> > > > On Fri, May 05, 2023 at 08:52:03PM +0100, Cristian Marussi wrote:
> > > > > On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote:
> > > > > > scmi: Introduce pinctrl SCMI protocol driver
> > > > > > 
> > > > > > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > > > > > excluding GPIO support. All pinctrl related callbacks and operations
> > > > > > are exposed in the include/linux/scmi_protocol.h
> > > > > > 
> > > > > 
> > > > > Hi Oleksii,
> > > > > 
> > > > > Thanks for this.
> > > > > 
> > > > > I tried out this in an emulated setup and found just a minor issue from
> > > > > the spec/functional point of view...then I reworked the extended names
> > > > > support using a modified hops->extended_name_get helper (as said the core
> > > > > SCMI support needed a small modification to support PINCTRL): I'll reply
> > > > > to this mail thread with such core SCMI modification patch, so you can
> > > > > include this patch of mine in your next V3 and use it in your series.
> > > > > 
> > > > > Moreover, given that I wanted to test such rework of mine and a bunch
> > > > > of other cleanups I did (as detailed down below), and it seemed silly
> > > > > to throw all away just to then having to detail all to you, I'll also
> > > > > include in another distinct reply the raw diff of what I changed in
> > > > > your series to use the new extended_name support and a few other cleanups,
> > > > > so that, if you want, you can just quickly merge that into your V3 patch
> > > > > (of course if you like it and tests fine also for you...)...these are
> > > > > small changes, if you take it, no need to bother with authorship and
> > > > > attribution from my point of view.
> > > > > 
> > > > 
> > > > Hi Cristian,
> > > > 
> > > 
> > > Hi,
> > > 
> > > > Thank you for the patches. I've applied them and tested with powerpc,
> > > > mx68 and clang environments (as test-robot complained about).
> > > > 
> > > 
> > > Yes, sure, they were just tentative fixes, needed cleanup.
> > > I forgot to add the RFC tag on my proposed fixes to avoid triggering the bots.
> > > 
> > > > > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> > > > > > ---
> > > > > >   MAINTAINERS                           |   6 +
> > > > 
> > > > [snip]
> > > > 
> > > > > >   SYSTEM RESET/SHUTDOWN DRIVERS
> > > > > >   M:	Sebastian Reichel <sre@xxxxxxxxxx>
> > > > > >   L:	linux-pm@xxxxxxxxxxxxxxx
> > > > > > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > > > > > index b31d78fa66cc..071ac65f22b9 100644
> > > > > > --- a/drivers/firmware/arm_scmi/Makefile
> > > > > > +++ b/drivers/firmware/arm_scmi/Makefile
> > > > > > @@ -3,6 +3,7 @@ scmi-bus-y = bus.o
> > > > > >   scmi-core-objs := $(scmi-bus-y)
> > > > > >   scmi-driver-y = driver.o notify.o
> > > > > > +
> > > > > 
> > > > > Do not add spurios lines.
> > > > > 
> > > > 
> > > > Thanks, removed
> > > > 
> > > > > >   scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
> > > > > >   scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
> > > > > >   scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
> > > > > > @@ -10,7 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> > > > > >   scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> > > > > >   scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > > > > >   scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > > > > > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > > > > > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> > > > > >   scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
> > > > 
> > > > I've applied patches you provided and made a small fixes. I'm going to
> > > > make patch:
> > > > "firmware: arm_scmi: Add optional flags to extended names helper"
> > > > as a separate and squach "Misc Fixes and refactor" to my changes in V3 if you
> > > > don't mind.
> > > 
> > > Sure, that's what I meant: include my general extended fixes at the
> > > start of your series and just squash the misc_fixes (additionally fixed
> > > by you :D) in your series.
> > > 
> > > A small nitpick I noticed later in scmi_protocol_ops Dox comment
> > > 
> > > + * struct scmi_pinctrl_protocol_ops - represents the various operations provided
> > > 
> > > should be
> > >    * struct scmi_pinctrl_proto_ops
> > > 
> > > Thanks,
> > > Cristian
> > > 
> > 
> > Hi Cristian,
> > 
> > Thank you very much for your help and your patches. I'm in the finishing
> > line with V3. I'll send to you the unit tests (when I fix them to work with the
> > latest changes) I'm using to test my changes and driver for ATF.
> > Hope it will help you to test your environment.
> 
> Is that ATF driver available somewhere?
> 
> Thanks,
> Michal
> 
> 
Hi Michal,

I've posted a link in the v3 series. But be aware it's not ready for
upstream so may be a bit ugly.

Oleksii.