Re: [PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone driver

[Cristian Marussi]

On Mon, Jul 08, 2024 at 10:59:33AM -0700, Nikunj Kela wrote:
> 
> On 7/8/2024 8:47 AM, Cristian Marussi wrote:
> > On Mon, Jul 08, 2024 at 08:23:56AM -0700, Nikunj Kela wrote:
> >> On 7/8/2024 7:27 AM, Cristian Marussi wrote:
> >>> On Sun, Jul 07, 2024 at 09:52:49AM -0700, Nikunj Kela wrote:
> >>>> On 7/6/2024 5:20 PM, Cristian Marussi wrote:
> >>>>> Make SCMI SMC transport a standalone driver that can be optionally
> >>>>> loaded as a module.
> >>>>>
> >>>>> CC: Peng Fan <peng.fan@xxxxxxx>
> >>>>> CC: Nikunj Kela <quic_nkela@xxxxxxxxxxx>
> >>>>> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> >>>>> ---
> >>>>>  drivers/firmware/arm_scmi/Kconfig             |  4 ++-
> >>>>>  drivers/firmware/arm_scmi/Makefile            |  2 +-
> >>>>>  drivers/firmware/arm_scmi/common.h            |  3 --
> >>>>>  drivers/firmware/arm_scmi/driver.c            |  5 ---
> >>>>>  .../arm_scmi/{smc.c => scmi_transport_smc.c}  | 31 +++++++++++++++----
> >>>>>  5 files changed, 29 insertions(+), 16 deletions(-)
> >>>>>  rename drivers/firmware/arm_scmi/{smc.c => scmi_transport_smc.c} (89%)
> >>>>>

Hi Nikunj,

> >>>>> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig

[snip]

> >>>
> >>>> Would it make sense to associate scmi descriptor(scmi_smc_desc) with
> >>>> compatible as driver/platform data so we have flexibility to replicate
> >>>> it and modify parameters such as max_timeout_ms etc. for our platform?
> >>>>
> >>> Mmmm...not sure to have understood, because the scmi_smc_desc is
> >>> effecetively passed from this driver to the core via a bit of
> >>> (questionable) magic in the mega-macro
> >>>
> >>> DEFINE_SCMI_TRANSPORT_DRIVER(scmi_smc, scmi_of_match, &core);
> >>>
> >>> ...and it will end-up being set into the dev.platform_data and then
> >>> retrieved by the core SCMI stack driver in scmi_probe...
> >>>
> >>> ...OR...do you mean being able to somehow define 3 different
> >>> scmi_smc_desc* and then associate them to the different compatibles
> >>> and then, depending on which compatible is matched by this isame driver
> >>> at probe time, passing the related platform-specific desc to the core...
> >>>
> >>> ...in this latter case I suppose I can do it by playing with the macros
> >>> defs but maybe it is also the case to start thinking about splitting out
> >>> configuration stuff from the transport descriptor...
> >>>
> >>> I'll give it a go at passing the data around, and see how it plays out
> >>> if you confirm that this is what you meant...
> >> Hi Cristian,
> >>
> > Hi,
> >
> >> I wanted to send a patch for review(with older driver code) that will
> >> allow us to override transport parameters(e.g. max_timeout_ms,
> >> max_msg_size etc.) on Qualcomm platform. There could be multiple
> >> approaches- 1) add callbacks (similar to get_msg_size) in transport_ops
> >> and override the default or 2) replicate the descriptors for different
> >> compatible and change those values as needed. I was going with the
> >> second option but then I saw your patch and thought of throwing this at
> > :P 
> >
> >> you ;) I don't want you to hold your patch series for this but if you
> >> have a better way to achieve or a preferred way between the two
> >> mentioned before, please let me know. If you do want to add this feature
> >> in this patch series, that would be great!
> >>
> > Interesting, because there is also this thread flying around from Peng:
> >
> > https://lore.kernel.org/linux-arm-kernel/20240703031715.379815-1-peng.fan@xxxxxxxxxxx/
> Thanks Cristian for the link. I was under the impression that DT binding
> that don't describe HW are not acceptable to DT maintainer. But if this
> patch goes through, I am fine using it. I would also like to have
> max_msg_size(and maybe max_msg) configurable.
> >
> > ...which I was planning in embedding in this series; Peng's proposal is
> > limited to timeout and based on DT (and title is a bit misleading...the
> > series is not mailbox-only related)....
> >
> > ...now I am NOT saying that we should dump all configs into the DT, but
> > just that this issue about configurability of transports is already sort
> > of a known-problem, and it is a while that it floats in the back of my mind...
> > i.e. the fact that the transport runtime configurations should be *somehow*
> > independent of the transport descriptor and *somehow* configurable....so now
> > only remains to *somehow* figure out how to do it :D
> >
> > ... I'll have a though and maybe cannibalize your ideas and Peng's one and then
> > we could have a discussion around this on the list to address eveybody's needs...
> >
> > ...and maybe, in the meantime, you could also post your proposed series about
> > this even though based on the old code, but as an RFC, just to make a point and
> > detail the needs...but yeah only if it does not require a bunch of extra work from
> > you given that it is only to be used as a basis for discussion ....
> >
> > ...up to you what do you prefer.

I am in the middle of some rework around these SCMI transport characteristics,
(max_msg / max_msg_size) that by itself currently are not so well defined in
terms of semantic and usage in the codebase, so I am trying to remove any
ambiguity and fix innaccuracies. (i.e. even before any DT transport-related
binding is introduced).

I was trying to understand better how you will use this new binding....
...I mean, of course you will use it to describe new sizes for the shmem areas
and the number of in-flight messages, but is this related to the RIDE platform
setup where you use a high number of SCMI instances ?

...if it is, should I guess that you will tailor somehow all of these shmem
areas' size to the specific/limited use of those scmi nodes ? (in terms of
protocols and messages I mean)...

...or you just need more space in general so you bumped the existing shmem
descriptors depending on the platform and you need to align max_msg_size
accordingly to the transport needs for that platform ?

..or I am overthinking ? :D

IOW what are your use-cases...so that I can try to fit them all in a general
way (possibly)...

Thanks,
Cristian