-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: Monday, September 10, 2018 3:04 PM
To: A.s. Dong <aisheng.dong@nxp.com>
Cc: Jassi Brar <jassisinghbrar@gmail.com>; dongas86 at gmail.com;
kernel at pengutronix.de; shawnguo at kernel.org; Fabio Estevam
[off-list ref]; dl-linux-imx [off-list ref];
linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH V5 0/5] soc: imx: add scu firmware api support

On Thu, Sep 06, 2018 at 03:21:39AM +0000, A.s. Dong wrote:

quoted

Hi Sasha,

quoted

-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: Monday, September 3, 2018 7:45 PM

On Mon, Sep 03, 2018 at 08:57:36AM +0000, A.s. Dong wrote:

quoted

Hi Sascha,

quoted

-----Original Message-----
From: A.s. Dong
Sent: Wednesday, August 29, 2018 4:35 PM
To: Sascha Hauer <s.hauer@pengutronix.de>

[...]

quoted

quoted

quoted

The original point is to separate the SCU service API
implementation from the client drivers. Client drivers don't
have to know the internal details of the API implementation,
they just use the service API provided by SCU firmware. API
implementation and client users will be

maintained independently.

I would buy this argument if the 'API implementation' was more
than a shim layer that directly translates between function
arguments and a

message struct.

quoted

With this you effectively can't change the API implementation
without changing the API you provide to the client drivers.

In reality, the API implementation could change without changing
the API prototype.
The API is defined in a more generic way and less possible to change.
But the internal implementation is allowed to change if the
firmware

updates.

quoted

quoted

e.g. protocol params layout and size and etc.

This way give us a clear separation between API internals and
client users which are more like to be maintained independently.
And it may also help if we want to support old firmware due to
API internal implementation changes. (And note that SCU firmware
supports many functions, distribute them into various drivers
may cause a bit mess. Some of them may get troubles on finding a
proper place to put.)

Examples?

Take sc_misc_set_control as example which is widely used by various client

drivers.

quoted

Encoding message in driver may cause many duplicated code.
Cscope tag: sc_misc_set_control
   #   line  filename / context / line
   1    206  drivers/clk/imx/clk-divider-scu.c

<<clk_divider3_scu_set_rate>>

quoted

             sci_err = sc_misc_set_control(ccm_ipc_handle,
clk->rsrc_id,

You can still create a helper function for these calls. I was mainly aiming for the
oneshot usages of several other functions.

Okay. That seems good to me.
So you mean we can still put the generic SVC APIs (e.g. sc_misc_set/get_control) in
a generic place. e.g. under drivers/soc/imx/sc/svc/ 
But put those oneshot usages APIs into individuals drivers. Right?
We can do that way.

quoted

quoted

quoted

quoted

So aren't those more valuable comparing to move SCU APIs into
client

drivers?

quoted

quoted

And current kernel users (arm scpi/scmi, ti sci) already do like
this... why not i.MX?

Sorry if I still not get your point.
Please help clarify a bit more if I missed something..

Please let me know if you still believe moving SCU API
implementation into each client driver is a more reasonable way to
go, then i will do it as I trust your professionality.

Yes, I still think that. I still think the 1:1 mapping between
messages and function calls is an unnecessary overhead.

Okay, let's do that way first if you still think it's proper.
BTW, do you have any comment about the MU usage in Patch 2/5 ?
[V5,2/5] soc: imx: add SC firmware IPC and APIs
https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat

chwork.kernel.org%2Fpatch%2F10570551%2F&data=02%7C01%7Caishe
ng.don

quoted

g%40nxp.com%7C195b82e11bc04a2a342a08d616eb906e%7C686ea1d3bc2b
4c6fa92cd

quoted

99c5c301635%7C0%7C0%7C636721598339072295&sdata=dYWNSEOoll
1%2BCq2i1

quoted

5uBxwelooUbAkNE6S9fnMOzf2I%3D&reserved=0

The MU usage looks fine from a first glance.

The way you share a global scu handle with your clients still looks ridiculous
though.

What's your suggestion?

I checked other references:
ARM SCPI, it also shares a global scpi_ops, no much difference than handler.
struct scpi_ops *get_scpi_ops(void)
{
        return scpi_info ? scpi_info->scpi_ops : NULL;
}               
EXPORT_SYMBOL_GPL(get_scpi_ops);

TI also indirectly shares a global handler, they put it in a global ti_sci_list.
And allocate it dynamically (there's actually only one SCI instance in current kernel)
const struct ti_sci_handle *ti_sci_get_handle(struct device *dev)
{
}

It looks like they have no too much difference from i.MX.
And we have 5 MU instances, the scu handle used by client can be different later.

Regards
Dong Aisheng

Sascha

--
Pengutronix e.K.                           |
|
Industrial Linux Solutions                 |
https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7C19
5b82e11bc04a2a342a08d616eb906e%7C686ea1d3bc2b4c6fa92cd99c5c3016
35%7C0%7C0%7C636721598339072295&sdata=dZKI%2FX4zoUTDTchK8
7DEpRzXIFUI1IR8kwUlA3t3HlY%3D&reserved=0  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
|
Amtsgericht Hildesheim, HRA 2686           | Fax:
+49-5121-206917-5555 |