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

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.
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>>
             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.

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://patchwork.kernel.org/patch/10570551/

The MU usage looks fine from a first glance.

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

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |