Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683316 > unrolled thread

[PATCH 3/9] firmware: arm_scmi: add basic driver infrastructure for SCMI

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/9] firmware: arm_scmi: add basic driver infrastructure for SCMI Jassi Brar <jassisinghbrar@gmail.com> - 2017-07-07 19:00 +0200
    Re: [PATCH 3/9] firmware: arm_scmi: add basic driver infrastructure  for SCMI Sudeep Holla <sudeep.holla@arm.com> - 2017-07-07 19:40 +0200
      Re: [PATCH 3/9] firmware: arm_scmi: add basic driver infrastructure  for SCMI Jassi Brar <jassisinghbrar@gmail.com> - 2017-07-08 07:40 +0200

#1683316 — [PATCH 3/9] firmware: arm_scmi: add basic driver infrastructure for SCMI

Hi Arnd, Hi Rob, Hi Mark,

[CC'ing only those who I have the email id of]

>+/**
>+ * scmi_do_xfer() - Do one transfer
>+ *
>+ * @info: Pointer to SCMI entity information
>+ * @xfer: Transfer to initiate and wait for response
>+ *
>+ * Return: -ETIMEDOUT in case of no response, if transmit error,
>+ *   return corresponding error, else if all goes well,
>+ *   return 0.
>+ */
>+int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
>+{
>+	int ret;
>+	int timeout;
>+	struct scmi_info *info = handle_to_scmi_info(handle);
>+	struct device *dev = info->dev;
>+
>+	ret = mbox_send_message(info->tx_chan, xfer);
>+
>
The api is

    int mbox_send_message(struct mbox_chan *chan, void *mssg)

where each controller driver defines its own format in which it accepts
the 'mssg' to be transmitted.

For example :-
ti_msgmgr_send_data(struct mbox_chan *, struct ti_msgmgr_message *)
rockchip_mbox_send_data(struct mbox_chan *, struct rockchip_mbox_msg *)
....and so on...  you get the idea.

Some controller driver may ignore the 'mssg' because only an interrupt line
is shared with the remote and not some register/fifo.
For example,
  sti_mbox_send_data(struct mbox_chan *, void *ignored)

Out of 14 controller drivers today, this SCMI implementation will work with
only 5 (and that too by abusing the api). The other 9 controllers (and many
in future) are perfectly able to support SCMI (its just another protocol).
All we need is what the SCMI specification calls "Transport Layer".
Of course that will be a thin platform/controller specific code that will
encapsulate the 'struct scmi_xfer *xfer' from SCMI, into controller specified
format and actually call the mbox_send_message().

 To achieve that, we can either do similar to what platforms with DWC3
do - generic dwc3 child node of platform specific parent node. Or we can have
the SCMI protocol implemented as a library that platforms can init and use.
Or any other better way.

 I just think it is a bad idea to rule out all but one classes of mailbox controllers.

Regards.

[toc] | [next] | [standalone]

#1683340 — Re: [PATCH 3/9] firmware: arm_scmi: add basic driver infrastructure for SCMI

On 07/07/17 17:52, Jassi Brar wrote:
> Hi Arnd, Hi Rob, Hi Mark,
> 
> [CC'ing only those who I have the email id of]
> 
>> +/**
>> + * scmi_do_xfer() - Do one transfer
>> + *
>> + * @info: Pointer to SCMI entity information
>> + * @xfer: Transfer to initiate and wait for response
>> + *
>> + * Return: -ETIMEDOUT in case of no response, if transmit error,
>> + *   return corresponding error, else if all goes well,
>> + *   return 0.
>> + */
>> +int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
>> +{
>> +	int ret;
>> +	int timeout;
>> +	struct scmi_info *info = handle_to_scmi_info(handle);
>> +	struct device *dev = info->dev;
>> +
>> +	ret = mbox_send_message(info->tx_chan, xfer);
>> +
>>
> The api is
> 
>     int mbox_send_message(struct mbox_chan *chan, void *mssg)
> 
> where each controller driver defines its own format in which it accepts
> the 'mssg' to be transmitted.
> 

Yes they can continue that, but SCMI just doesn't depend on that.

> For example :-
> ti_msgmgr_send_data(struct mbox_chan *, struct ti_msgmgr_message *)
> rockchip_mbox_send_data(struct mbox_chan *, struct rockchip_mbox_msg *)
> ....and so on...  you get the idea.
> 

Yes I am aware of that.

> Some controller driver may ignore the 'mssg' because only an interrupt line
> is shared with the remote and not some register/fifo.
> For example,
>   sti_mbox_send_data(struct mbox_chan *, void *ignored)
> 

Exactly, now with SCMI, every controller *can do* that, as we just care
about the signaling which in other terms I have so far referred as
"doorbell". The data passed should not be used for any other purpose
as there's no need to even by the remote firmware implementing SCMI.

If you read the SCMI specification, it's designed to avoid all such
issues with wide variety of mailbox controller we have in the wild.
And hence all the command and other information are passed in the shared
memory.

> Out of 14 controller drivers today, this SCMI implementation will work with
> only 5 (and that too by abusing the api).

What do you mean by abusing the API ?

> The other 9 controllers (and many
> in future) are perfectly able to support SCMI (its just another protocol).

Exactly, if and only if they adhere to the specification.

> All we need is what the SCMI specification calls "Transport Layer".
> Of course that will be a thin platform/controller specific code that will
> encapsulate the 'struct scmi_xfer *xfer' from SCMI, into controller specified
> format and actually call the mbox_send_message().

OK, if platform had to send some platform specific data, I agree. But
can you point me from the SCPI specification the need for the platforms
to do that. That's the whole point. Transport is just a doorbell nothing
more.

You are trying to fit the existing platform implementation into SCMI
which won't work.

E.g. if TI/Rockchip implements SCMI, they can continue using these
drivers as along as it does the job of "ringing the doorbell".

They may be passing some data today because their existing remote
firmware expects it. With SCMI, they don't have to. These existing
drivers are doing additional job in the current form: passing some data
along with triggering the remote doorbell. We just need latter in SCMI
case as we don't need any data in the transport as it's all part of shmem.

Why do you think that won't work ? Remember all the platform specific
stuff(if platform implement own protocols) in SCMI is moved to the
shared memory and transport just needs to do "doorbell". So I disagree
with your concern unless I am missing something.

> 
>  To achieve that, we can either do similar to what platforms with DWC3
> do - generic dwc3 child node of platform specific parent node. Or we can have
> the SCMI protocol implemented as a library that platforms can init and use.
> Or any other better way.
> 
>  I just think it is a bad idea to rule out all but one classes of mailbox controllers.
> 

Definitely neither SCMI specification nor SCMI driver is ruling out any
class of mailbox controllers as long as there are capable of signaling
the other end that "hey look you have something to check in shared
memory" which I think is the case with most of the controllers.

To be honest this whole mailbox framework is somewhat complex for simple
doorbell kind of this, but hey I am not complaining ;).

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1683540 — Re: [PATCH 3/9] firmware: arm_scmi: add basic driver infrastructure for SCMI

Hi Roy, Matt, Nishant, Harb Abdulhamid, Loc,

I have a gut feeling you guys were part of the SCMI spec committee. If
so, could you please chime in?


On Fri, Jul 7, 2017 at 11:09 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 07/07/17 17:52, Jassi Brar wrote:
>> Hi Arnd, Hi Rob, Hi Mark,
>>
>> [CC'ing only those who I have the email id of]
>>
>>> +/**
>>> + * scmi_do_xfer() - Do one transfer
>>> + *
>>> + * @info: Pointer to SCMI entity information
>>> + * @xfer: Transfer to initiate and wait for response
>>> + *
>>> + * Return: -ETIMEDOUT in case of no response, if transmit error,
>>> + *   return corresponding error, else if all goes well,
>>> + *   return 0.
>>> + */
>>> +int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
>>> +{
>>> +    int ret;
>>> +    int timeout;
>>> +    struct scmi_info *info = handle_to_scmi_info(handle);
>>> +    struct device *dev = info->dev;
>>> +
>>> +    ret = mbox_send_message(info->tx_chan, xfer);
>>> +
>>>
>> The api is
>>
>>     int mbox_send_message(struct mbox_chan *chan, void *mssg)
>>
>> where each controller driver defines its own format in which it accepts
>> the 'mssg' to be transmitted.
>>
>
> Yes they can continue that, but SCMI just doesn't depend on that.
>
>> For example :-
>> ti_msgmgr_send_data(struct mbox_chan *, struct ti_msgmgr_message *)
>> rockchip_mbox_send_data(struct mbox_chan *, struct rockchip_mbox_msg *)
>> ....and so on...  you get the idea.
>>
>
> Yes I am aware of that.
>
>> Some controller driver may ignore the 'mssg' because only an interrupt line
>> is shared with the remote and not some register/fifo.
>> For example,
>>   sti_mbox_send_data(struct mbox_chan *, void *ignored)
>>
>
> Exactly, now with SCMI, every controller *can do* that, as we just care
> about the signaling which in other terms I have so far referred as
> "doorbell".
>
No, the controllers can not ... unless you clone and adapt the 9
drivers+bindings to conform to the expectations of SCMI (like you
attempted with MHU recently). Also, then mandate every future
controller driver must emulate "doorbell" channels.

As the mailbox maintainer, I am open to suggestions that would allow
every controller to support SCMI.
But compared to the options of scmi-as-a-library and
scmi-as-child-node-of-platform-parent, this does not even qualify as
an option.

Why? Because SCMI is but one protocol that provides 4 features ATM,
and certainly can not provide for every whim and quirk of future
platforms.  Among the sane requirements are watchdog,
suspend/resume/hibernation and thermal _control_ (not just sensor
readings) and among the weird are video, network and storage over
mailbox api. And even a filesystem backed by read/write over mailbox!!
 And these are only that I have worked on first hand.

  The point is : you can not assume SCMI to be the only protocol
running over a controller _and_ you can not dictate other protocols to
not touch certain bits of the signal register/fifo.

In simplest terms, controller driver can not cater to only a
particular client. That's the reason we have the controller driver
define the message format and clients conform to it.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web