Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-05-14 Thread Sudeep Holla 



On 14/05/15 08:30, Jassi Brar wrote:

On Thu, May 14, 2015 at 12:32 PM, Jassi Brar  wrote:


BTW is scpi_protocol.c meant/tested to work over arm_mhu.c? The spec
says so but I don't see how because you pass 'struct scpi_xfer*' as
the message whereas arm_mhu.c expects u32*



Yes it's tested using arm_mhu.c, and I have even sent updates to the
binding that's incomplete as of now and *must* be pulled into v4.1.
Please make sure it gets in. Otherwise clocks are optional but the
driver fails to probe without that. I was initially wondering why the
MHU probe is not called.

scpi_xfer has the slot as first element which will have the right
doorbell bit(in this case slot#0) set always.


It seems your remote doesn't interpret the value in STAT register...
so it just works.


Not exactly. If you look at Figure 2-1 in the spec, it shows how STAT
(along with SET/CLEAR) is used to identify the protocol. The remote
can implement multiple protocol. E.g. SCPI(on Slot#0 - main topic of
this series), ACPI(PCC/CPPC say on Slot#1), ABC(on Slot#X)...etc.


However the SCPI spec recommends seeing STAT register as '31 slots'
... maybe we should try to support that.



Correct but only Slot#0 is used for SCPI.

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-05-14 Thread Jassi Brar 
On Thu, May 14, 2015 at 12:32 PM, Jassi Brar  wrote:
>
> BTW is scpi_protocol.c meant/tested to work over arm_mhu.c? The spec
> says so but I don't see how because you pass 'struct scpi_xfer*' as
> the message whereas arm_mhu.c expects u32*
>
It seems your remote doesn't interpret the value in STAT register...
so it just works.
However the SCPI spec recommends seeing STAT register as '31 slots'
... maybe we should try to support that.

thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-05-14 Thread Jassi Brar 
On Wed, May 13, 2015 at 10:39 PM, Sudeep Holla  wrote:
> On 13/05/15 17:52, Jassi Brar wrote:
>>
>>> This patch adds support for System Control and Power Interface (SCPI)
>>> Message Protocol used between the Application Cores(AP) and the System
>>> Control Processor(SCP). The MHU peripheral provides a mechanism for
>>> inter-processor communication between SCP's M3 processor and AP.
>>>
>>> SCP offers control and management of the core/cluster power states,
>>> various power domain DVFS including the core/cluster, certain system
>>> clocks configuration, thermal sensors and many others.
>>>
>>> This protocol driver provides interface for all the client drivers using
>>> SCPI to make use of the features offered by the SCP.
>>>
>> Is the SCPI specification available somewhere to look into?
>>
>
> Yes sorry posted the link separately(as reply to Tixy in the cover
> letter) since it was not available when I posted the patches.
> You can grab the protocol @[1] or [2]
>
Thanks for the link. I wish I had access to the spec earlier.

>>>   .../devicetree/bindings/mailbox/arm,scpi.txt   | 121 
>>>   drivers/mailbox/Kconfig|  19 +
>>>   drivers/mailbox/Makefile   |   2 +
>>>   drivers/mailbox/scpi_protocol.c| 694
>>> +
>>>
>> Why in drivers/mailbox/ ? This is a 'consumer' driver and seems
>> Juno(ARM) specific.
>>
>
> Not just JUNO alone though it's first one to use, it will used in next
> few platforms(foreseeable future) from ARM Ltd.
>
> I have put that in drivers/mailbox for 2 reasons:
> 1. It's mailbox protocol :)
>
client/protocol drivers don't usually reside with controller drivers.
drivers/firmware/ seems more appropriate.

> 2. ARM64 doesn't have platform code like ARM32 and moreover it's
>strictly not specific to JUNO or any single platform. It may
>get reused on other platforms.
>
drivers/firmware/ should do too.

BTW is scpi_protocol.c meant/tested to work over arm_mhu.c? The spec
says so but I don't see how because you pass 'struct scpi_xfer*' as
the message whereas arm_mhu.c expects u32*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-05-14 Thread Jassi Brar 
On Wed, May 13, 2015 at 10:39 PM, Sudeep Holla sudeep.ho...@arm.com wrote:
 On 13/05/15 17:52, Jassi Brar wrote:

 This patch adds support for System Control and Power Interface (SCPI)
 Message Protocol used between the Application Cores(AP) and the System
 Control Processor(SCP). The MHU peripheral provides a mechanism for
 inter-processor communication between SCP's M3 processor and AP.

 SCP offers control and management of the core/cluster power states,
 various power domain DVFS including the core/cluster, certain system
 clocks configuration, thermal sensors and many others.

 This protocol driver provides interface for all the client drivers using
 SCPI to make use of the features offered by the SCP.

 Is the SCPI specification available somewhere to look into?


 Yes sorry posted the link separately(as reply to Tixy in the cover
 letter) since it was not available when I posted the patches.
 You can grab the protocol @[1] or [2]

Thanks for the link. I wish I had access to the spec earlier.

   .../devicetree/bindings/mailbox/arm,scpi.txt   | 121 
   drivers/mailbox/Kconfig|  19 +
   drivers/mailbox/Makefile   |   2 +
   drivers/mailbox/scpi_protocol.c| 694
 +

 Why in drivers/mailbox/ ? This is a 'consumer' driver and seems
 Juno(ARM) specific.


 Not just JUNO alone though it's first one to use, it will used in next
 few platforms(foreseeable future) from ARM Ltd.

 I have put that in drivers/mailbox for 2 reasons:
 1. It's mailbox protocol :)

client/protocol drivers don't usually reside with controller drivers.
drivers/firmware/ seems more appropriate.

 2. ARM64 doesn't have platform code like ARM32 and moreover it's
strictly not specific to JUNO or any single platform. It may
get reused on other platforms.

drivers/firmware/ should do too.

BTW is scpi_protocol.c meant/tested to work over arm_mhu.c? The spec
says so but I don't see how because you pass 'struct scpi_xfer*' as
the message whereas arm_mhu.c expects u32*
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-05-14 Thread Jassi Brar 
On Thu, May 14, 2015 at 12:32 PM, Jassi Brar jassisinghb...@gmail.com wrote:

 BTW is scpi_protocol.c meant/tested to work over arm_mhu.c? The spec
 says so but I don't see how because you pass 'struct scpi_xfer*' as
 the message whereas arm_mhu.c expects u32*

It seems your remote doesn't interpret the value in STAT register...
so it just works.
However the SCPI spec recommends seeing STAT register as '31 slots'
... maybe we should try to support that.

thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-05-14 Thread Sudeep Holla 



On 14/05/15 08:30, Jassi Brar wrote:

On Thu, May 14, 2015 at 12:32 PM, Jassi Brar jassisinghb...@gmail.com wrote:


BTW is scpi_protocol.c meant/tested to work over arm_mhu.c? The spec
says so but I don't see how because you pass 'struct scpi_xfer*' as
the message whereas arm_mhu.c expects u32*



Yes it's tested using arm_mhu.c, and I have even sent updates to the
binding that's incomplete as of now and *must* be pulled into v4.1.
Please make sure it gets in. Otherwise clocks are optional but the
driver fails to probe without that. I was initially wondering why the
MHU probe is not called.

scpi_xfer has the slot as first element which will have the right
doorbell bit(in this case slot#0) set always.


It seems your remote doesn't interpret the value in STAT register...
so it just works.


Not exactly. If you look at Figure 2-1 in the spec, it shows how STAT
(along with SET/CLEAR) is used to identify the protocol. The remote
can implement multiple protocol. E.g. SCPI(on Slot#0 - main topic of
this series), ACPI(PCC/CPPC say on Slot#1), ABC(on Slot#X)...etc.


However the SCPI spec recommends seeing STAT register as '31 slots'
... maybe we should try to support that.



Correct but only Slot#0 is used for SCPI.

Regards,
Sudeep
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-05-13 Thread Sudeep Holla 



On 13/05/15 17:52, Jassi Brar wrote:

On Mon, Apr 27, 2015 at 5:10 PM, Sudeep Holla  wrote:

This patch adds support for System Control and Power Interface (SCPI)
Message Protocol used between the Application Cores(AP) and the System
Control Processor(SCP). The MHU peripheral provides a mechanism for
inter-processor communication between SCP's M3 processor and AP.

SCP offers control and management of the core/cluster power states,
various power domain DVFS including the core/cluster, certain system
clocks configuration, thermal sensors and many others.

This protocol driver provides interface for all the client drivers using
SCPI to make use of the features offered by the SCP.


Is the SCPI specification available somewhere to look into?



Yes sorry posted the link separately(as reply to Tixy in the cover
letter) since it was not available when I posted the patches.
You can grab the protocol @[1] or [2]


Signed-off-by: Sudeep Holla 
Cc: Rob Herring 
Cc: Mark Rutland 
CC: Jassi Brar 
Cc: Liviu Dudau 
Cc: Lorenzo Pieralisi 
Cc: Jon Medhurst (Tixy) 
Cc: devicet...@vger.kernel.org
---
  .../devicetree/bindings/mailbox/arm,scpi.txt   | 121 
  drivers/mailbox/Kconfig|  19 +
  drivers/mailbox/Makefile   |   2 +
  drivers/mailbox/scpi_protocol.c| 694 +


Why in drivers/mailbox/ ? This is a 'consumer' driver and seems
Juno(ARM) specific.



Not just JUNO alone though it's first one to use, it will used in next
few platforms(foreseeable future) from ARM Ltd.

I have put that in drivers/mailbox for 2 reasons:
1. It's mailbox protocol :)
2. ARM64 doesn't have platform code like ARM32 and moreover it's
   strictly not specific to JUNO or any single platform. It may
   get reused on other platforms.

Regards,
Sudeep

[1] 
http://community.arm.com/servlet/JiveServlet/download/8401-40-18262/DUI0922A_scp_message_interface.pdf
[2] 
https://wiki.linaro.org/ARM/Juno?action=AttachFile=get=DUI0922A_scp_message_interface.pdf

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-05-13 Thread Jassi Brar 
On Mon, Apr 27, 2015 at 5:10 PM, Sudeep Holla  wrote:
> This patch adds support for System Control and Power Interface (SCPI)
> Message Protocol used between the Application Cores(AP) and the System
> Control Processor(SCP). The MHU peripheral provides a mechanism for
> inter-processor communication between SCP's M3 processor and AP.
>
> SCP offers control and management of the core/cluster power states,
> various power domain DVFS including the core/cluster, certain system
> clocks configuration, thermal sensors and many others.
>
> This protocol driver provides interface for all the client drivers using
> SCPI to make use of the features offered by the SCP.
>
Is the SCPI specification available somewhere to look into?

> Signed-off-by: Sudeep Holla 
> Cc: Rob Herring 
> Cc: Mark Rutland 
> CC: Jassi Brar 
> Cc: Liviu Dudau 
> Cc: Lorenzo Pieralisi 
> Cc: Jon Medhurst (Tixy) 
> Cc: devicet...@vger.kernel.org
> ---
>  .../devicetree/bindings/mailbox/arm,scpi.txt   | 121 
>  drivers/mailbox/Kconfig|  19 +
>  drivers/mailbox/Makefile   |   2 +
>  drivers/mailbox/scpi_protocol.c| 694 
> +
>
Why in drivers/mailbox/ ? This is a 'consumer' driver and seems
Juno(ARM) specific.

-Jassi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-05-13 Thread Jassi Brar 
On Mon, Apr 27, 2015 at 5:10 PM, Sudeep Holla sudeep.ho...@arm.com wrote:
 This patch adds support for System Control and Power Interface (SCPI)
 Message Protocol used between the Application Cores(AP) and the System
 Control Processor(SCP). The MHU peripheral provides a mechanism for
 inter-processor communication between SCP's M3 processor and AP.

 SCP offers control and management of the core/cluster power states,
 various power domain DVFS including the core/cluster, certain system
 clocks configuration, thermal sensors and many others.

 This protocol driver provides interface for all the client drivers using
 SCPI to make use of the features offered by the SCP.

Is the SCPI specification available somewhere to look into?

 Signed-off-by: Sudeep Holla sudeep.ho...@arm.com
 Cc: Rob Herring robh...@kernel.org
 Cc: Mark Rutland mark.rutl...@arm.com
 CC: Jassi Brar jassisinghb...@gmail.com
 Cc: Liviu Dudau liviu.du...@arm.com
 Cc: Lorenzo Pieralisi lorenzo.pieral...@arm.com
 Cc: Jon Medhurst (Tixy) t...@linaro.org
 Cc: devicet...@vger.kernel.org
 ---
  .../devicetree/bindings/mailbox/arm,scpi.txt   | 121 
  drivers/mailbox/Kconfig|  19 +
  drivers/mailbox/Makefile   |   2 +
  drivers/mailbox/scpi_protocol.c| 694 
 +

Why in drivers/mailbox/ ? This is a 'consumer' driver and seems
Juno(ARM) specific.

-Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-05-13 Thread Sudeep Holla 



On 13/05/15 17:52, Jassi Brar wrote:

On Mon, Apr 27, 2015 at 5:10 PM, Sudeep Holla sudeep.ho...@arm.com wrote:

This patch adds support for System Control and Power Interface (SCPI)
Message Protocol used between the Application Cores(AP) and the System
Control Processor(SCP). The MHU peripheral provides a mechanism for
inter-processor communication between SCP's M3 processor and AP.

SCP offers control and management of the core/cluster power states,
various power domain DVFS including the core/cluster, certain system
clocks configuration, thermal sensors and many others.

This protocol driver provides interface for all the client drivers using
SCPI to make use of the features offered by the SCP.


Is the SCPI specification available somewhere to look into?



Yes sorry posted the link separately(as reply to Tixy in the cover
letter) since it was not available when I posted the patches.
You can grab the protocol @[1] or [2]


Signed-off-by: Sudeep Holla sudeep.ho...@arm.com
Cc: Rob Herring robh...@kernel.org
Cc: Mark Rutland mark.rutl...@arm.com
CC: Jassi Brar jassisinghb...@gmail.com
Cc: Liviu Dudau liviu.du...@arm.com
Cc: Lorenzo Pieralisi lorenzo.pieral...@arm.com
Cc: Jon Medhurst (Tixy) t...@linaro.org
Cc: devicet...@vger.kernel.org
---
  .../devicetree/bindings/mailbox/arm,scpi.txt   | 121 
  drivers/mailbox/Kconfig|  19 +
  drivers/mailbox/Makefile   |   2 +
  drivers/mailbox/scpi_protocol.c| 694 +


Why in drivers/mailbox/ ? This is a 'consumer' driver and seems
Juno(ARM) specific.



Not just JUNO alone though it's first one to use, it will used in next
few platforms(foreseeable future) from ARM Ltd.

I have put that in drivers/mailbox for 2 reasons:
1. It's mailbox protocol :)
2. ARM64 doesn't have platform code like ARM32 and moreover it's
   strictly not specific to JUNO or any single platform. It may
   get reused on other platforms.

Regards,
Sudeep

[1] 
http://community.arm.com/servlet/JiveServlet/download/8401-40-18262/DUI0922A_scp_message_interface.pdf
[2] 
https://wiki.linaro.org/ARM/Juno?action=AttachFiledo=gettarget=DUI0922A_scp_message_interface.pdf

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-30 Thread Jon Medhurst (Tixy) 
On Wed, 2015-04-29 at 13:25 +0100, Jon Medhurst (Tixy) wrote:
> diff --git a/drivers/mailbox/scpi_protocol.c
> b/drivers/mailbox/scpi_protocol.c
> index c74575b..5818d9b 100644
> --- a/drivers/mailbox/scpi_protocol.c
> +++ b/drivers/mailbox/scpi_protocol.c
> @@ -286,14 +286,23 @@ static void scpi_tx_prepare(struct mbox_client
> *c, void *msg)
> struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
> struct scpi_shared_mem *mem = (struct scpi_shared_mem
> *)ch->tx_payload;
>  
> -   mem->command = cpu_to_le32(t->cmd);
> if (t->tx_buf)
> memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
> if (t->rx_buf) {
> +   int token;
> spin_lock_irqsave(>rx_lock, flags);
> +   /*
> +* Presumably we can do this token setting outside
> +* spinlock and still be safe from concurrency?
> +*/

To answer my own question, yes, the four lines below can be moved up
above the spin_lock_irqsave. Because we had better be safe from
concurrency here as we are also writing to the channel's shared memory
area.

> +   do
> +   token = (++ch->token) & CMD_TOKEN_ID_MASK;
> +   while(!token);
> +   t->cmd |= token << CMD_TOKEN_ID_SHIFT;
> list_add_tail(>node, >rx_pending);
> spin_unlock_irqrestore(>rx_lock, flags);
> }
> +   mem->command = cpu_to_le32(t->cmd);
>  }
>  
>  static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)

-- 
Tixy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-30 Thread Jon Medhurst (Tixy) 
On Wed, 2015-04-29 at 13:25 +0100, Jon Medhurst (Tixy) wrote:
 diff --git a/drivers/mailbox/scpi_protocol.c
 b/drivers/mailbox/scpi_protocol.c
 index c74575b..5818d9b 100644
 --- a/drivers/mailbox/scpi_protocol.c
 +++ b/drivers/mailbox/scpi_protocol.c
 @@ -286,14 +286,23 @@ static void scpi_tx_prepare(struct mbox_client
 *c, void *msg)
 struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
 struct scpi_shared_mem *mem = (struct scpi_shared_mem
 *)ch-tx_payload;
  
 -   mem-command = cpu_to_le32(t-cmd);
 if (t-tx_buf)
 memcpy_toio(mem-payload, t-tx_buf, t-tx_len);
 if (t-rx_buf) {
 +   int token;
 spin_lock_irqsave(ch-rx_lock, flags);
 +   /*
 +* Presumably we can do this token setting outside
 +* spinlock and still be safe from concurrency?
 +*/

To answer my own question, yes, the four lines below can be moved up
above the spin_lock_irqsave. Because we had better be safe from
concurrency here as we are also writing to the channel's shared memory
area.

 +   do
 +   token = (++ch-token)  CMD_TOKEN_ID_MASK;
 +   while(!token);
 +   t-cmd |= token  CMD_TOKEN_ID_SHIFT;
 list_add_tail(t-node, ch-rx_pending);
 spin_unlock_irqrestore(ch-rx_lock, flags);
 }
 +   mem-command = cpu_to_le32(t-cmd);
  }
  
  static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)

-- 
Tixy

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-29 Thread Sudeep Holla 



On 29/04/15 13:25, Jon Medhurst (Tixy) wrote:

On Wed, 2015-04-29 at 12:43 +0100, Jon Medhurst (Tixy) wrote:

On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:

On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:

On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:

[...]

+ int ret;
+ u8 token, chan;
+ struct scpi_xfer *msg;
+ struct scpi_chan *scpi_chan;
+
+ chan = atomic_inc_return(_info->next_chan) % scpi_info->num_chans;
+ scpi_chan = scpi_info->channels + chan;
+
+ msg = get_scpi_xfer(scpi_chan);
+ if (!msg)
+ return -ENOMEM;
+
+ token = atomic_inc_return(_chan->token) & CMD_TOKEN_ID_MASK;


So, this 8 bit token is what's used to 'uniquely' identify a pending
command. But as it's just an incrementing value, then if one command
gets delayed for long enough that 256 more are issued then we will have
a non-unique value and scpi_process_cmd can go wrong.



IMO by the time 256 message are queued up and serviced we would timeout
on the initial command. Moreover the core mailbox has sent the mailbox
length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
remote chance of hit the corner case.


The corner case can be hit even if the queue length is only 2, because
other processes/cpus can use the other message we don't own here and
they can send then receive a message using that, 256 times. The corner
case doesn't require 256 simultaneous outstanding requests.

That is the reason I suggested that rather than using an incrementing
value for the 'unique' token, that each message instead contain the
value of the token to use with it.


Of course, I failed to mention that this solution to this problem makes
thing worse for the situation where we timeout messages, because the
same token will get reused quicker in that case. So, in practice, if we
have timeouts, and a unchangeable protocol limitation of 256 tokens,
then using those tokens in order, for each message sent is probably the
best we can do.



I agree, I think we must be happy with that for now :)


Perhaps that's the clue, generate and add the token to the message, just
before transmission via the MHU, at a point where we know no other
request can overtake us. In scpi_tx_prepare? Perhaps, it might also be
good to only use up a token if we are expecting a response, and use zero
for other messages?

Something like this totally untested patch...



Looks good and best we can do with the limitations we have IMO

Regards,
Sudeep

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-29 Thread Sudeep Holla 

Hi Tixy,

On 29/04/15 12:43, Jon Medhurst (Tixy) wrote:

On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:

On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:

On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:

[...]

+ int ret;
+ u8 token, chan;
+ struct scpi_xfer *msg;
+ struct scpi_chan *scpi_chan;
+
+ chan = atomic_inc_return(_info->next_chan) % scpi_info->num_chans;
+ scpi_chan = scpi_info->channels + chan;
+
+ msg = get_scpi_xfer(scpi_chan);
+ if (!msg)
+ return -ENOMEM;
+
+ token = atomic_inc_return(_chan->token) & CMD_TOKEN_ID_MASK;


So, this 8 bit token is what's used to 'uniquely' identify a pending
command. But as it's just an incrementing value, then if one command
gets delayed for long enough that 256 more are issued then we will have
a non-unique value and scpi_process_cmd can go wrong.



IMO by the time 256 message are queued up and serviced we would timeout
on the initial command. Moreover the core mailbox has sent the mailbox
length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
remote chance of hit the corner case.


The corner case can be hit even if the queue length is only 2, because
other processes/cpus can use the other message we don't own here and
they can send then receive a message using that, 256 times. The corner
case doesn't require 256 simultaneous outstanding requests.



Good point, I missed it completely.


That is the reason I suggested that rather than using an incrementing
value for the 'unique' token, that each message instead contain the
value of the token to use with it.




Note, this delay doesn't just have to be at the SCPI end. We could get
preempted here (?) before actually sending the command to the SCP and
other kernel threads or processes could send those other 256 commands
before we get to run again.



Agreed, but we would still timeout after 3 jiffies max.


But we haven't started any timeout yet, the 3 jiffies won't start until
we get scheduled again and call wait_for_completion_timeout below.


Agreed.




Wouldn't it be better instead to have scpi_alloc_xfer_list add a unique
number to each struct scpi_xfer.



One of reason using it part of command is that SCP gives it back in the
response to compare.


Can't we fill the token in the command from the value stored in the
struct scpi_xfer we are using to send that command?



Yes we can but 256 limitation still exists but solve some issues at-least.


+
+ msg->slot = BIT(SCPI_SLOT);
+ msg->cmd = PACK_SCPI_CMD(cmd, token, len);
+ msg->tx_buf = tx_buf;
+ msg->tx_len = len;
+ msg->rx_buf = rx_buf;
+ init_completion(>done);
+
+ ret = mbox_send_message(scpi_chan->chan, msg);
+ if (ret < 0 || !rx_buf)
+ goto out;
+
+ if (!wait_for_completion_timeout(>done, MAX_RX_TIMEOUT))
+ ret = -ETIMEDOUT;
+ else
+ /* first status word */
+ ret = le32_to_cpu(msg->status);
+out:
+ if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */


So, even with my suggestion that the unique message identifies are
fixed values stored in struct scpi_xfer, we can still have the situation
where we timeout a request, that scpi_xfer then getting used for another
request, and finally the SCP completes the request that we timed out,
which has the same 'unique' value as the later one.



As explained above I can't imagine hitting this condition. I will think
more on that again.


I can imagine :-) If we timeout and discard messages, and reuse it's
unique id, there is always the possibility of this confusion occurring.
No amount of coding in the kernel can get around that. The only thing
you can do to get out of this quandary is make assumptions about how the
SCP firmware behaves.



Agreed again.




One way to handle that it to not have any timeout on requests and assume
the firmware isn't buggy.



That's something I can't do ;) based on my experience so far. It's good
to assume firmware *can be buggy* and handle all possible errors.


I'm inclined to agree.



Thanks :)


  Think
about the development firmware using this driver. This has been very
useful when I was testing the development versions. Even under stress
conditions I still see timeouts(very rarely though), so my personal
preference is to have them.


But the SCPI protocol unfortunately doesn't seem to allow us to robustly
handle timeouts. Well, we could keep a list of tokens used in timed out
messages, and not reuse them. But if, as you say, timeouts do occur,
then with only 256 available, we are likely to run out.



Yes :(


When I brought this up 9 months ago, it was pointed out that the
limitation of an 8-bit token for a message because was because the
protocol designers had were cramming it into the 32-bit value poked into
the MHU register. The new finished protocol spec doesn't use the MHU
register any more for this data, but the limitations we're kept by
specifying the same command data format

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-29 Thread Jon Medhurst (Tixy) 
On Wed, 2015-04-29 at 12:43 +0100, Jon Medhurst (Tixy) wrote:
> On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:
> > On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:
> > > On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
> [...]
> > >> + int ret;
> > >> + u8 token, chan;
> > >> + struct scpi_xfer *msg;
> > >> + struct scpi_chan *scpi_chan;
> > >> +
> > >> + chan = atomic_inc_return(_info->next_chan) % 
> > >> scpi_info->num_chans;
> > >> + scpi_chan = scpi_info->channels + chan;
> > >> +
> > >> + msg = get_scpi_xfer(scpi_chan);
> > >> + if (!msg)
> > >> + return -ENOMEM;
> > >> +
> > >> + token = atomic_inc_return(_chan->token) & CMD_TOKEN_ID_MASK;
> > >
> > > So, this 8 bit token is what's used to 'uniquely' identify a pending
> > > command. But as it's just an incrementing value, then if one command
> > > gets delayed for long enough that 256 more are issued then we will have
> > > a non-unique value and scpi_process_cmd can go wrong.
> > >
> > 
> > IMO by the time 256 message are queued up and serviced we would timeout
> > on the initial command. Moreover the core mailbox has sent the mailbox
> > length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
> > remote chance of hit the corner case.
> 
> The corner case can be hit even if the queue length is only 2, because
> other processes/cpus can use the other message we don't own here and
> they can send then receive a message using that, 256 times. The corner
> case doesn't require 256 simultaneous outstanding requests.
> 
> That is the reason I suggested that rather than using an incrementing
> value for the 'unique' token, that each message instead contain the
> value of the token to use with it.

Of course, I failed to mention that this solution to this problem makes
thing worse for the situation where we timeout messages, because the
same token will get reused quicker in that case. So, in practice, if we
have timeouts, and a unchangeable protocol limitation of 256 tokens,
then using those tokens in order, for each message sent is probably the
best we can do.

Perhaps that's the clue, generate and add the token to the message, just
before transmission via the MHU, at a point where we know no other
request can overtake us. In scpi_tx_prepare? Perhaps, it might also be
good to only use up a token if we are expecting a response, and use zero
for other messages?

Something like this totally untested patch...

diff --git a/drivers/mailbox/scpi_protocol.c b/drivers/mailbox/scpi_protocol.c
index c74575b..5818d9b 100644
--- a/drivers/mailbox/scpi_protocol.c
+++ b/drivers/mailbox/scpi_protocol.c
@@ -286,14 +286,23 @@ static void scpi_tx_prepare(struct mbox_client *c, void 
*msg)
struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
 
-   mem->command = cpu_to_le32(t->cmd);
if (t->tx_buf)
memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
if (t->rx_buf) {
+   int token;
spin_lock_irqsave(>rx_lock, flags);
+   /*
+* Presumably we can do this token setting outside
+* spinlock and still be safe from concurrency?
+*/
+   do
+   token = (++ch->token) & CMD_TOKEN_ID_MASK;
+   while(!token);
+   t->cmd |= token << CMD_TOKEN_ID_SHIFT;
list_add_tail(>node, >rx_pending);
spin_unlock_irqrestore(>rx_lock, flags);
}
+   mem->command = cpu_to_le32(t->cmd);
 }
 
 static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
@@ -322,7 +331,7 @@ static int
 scpi_send_message(u8 cmd, void *tx_buf, unsigned int len, void *rx_buf)
 {
int ret;
-   u8 token, chan;
+   u8 chan;
struct scpi_xfer *msg;
struct scpi_chan *scpi_chan;
 
@@ -333,10 +342,8 @@ scpi_send_message(u8 cmd, void *tx_buf, unsigned int len, 
void *rx_buf)
if (!msg)
return -ENOMEM;
 
-   token = atomic_inc_return(_chan->token) & CMD_TOKEN_ID_MASK;
-
msg->slot = BIT(SCPI_SLOT);
-   msg->cmd = PACK_SCPI_CMD(cmd, token, len);
+   msg->cmd = PACK_SCPI_CMD(cmd, 0, len);
msg->tx_buf = tx_buf;
msg->tx_len = len;
msg->rx_buf = rx_buf;


-- 
Tixy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-29 Thread Jon Medhurst (Tixy) 
On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:
> On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:
> > On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
[...]
> >> + int ret;
> >> + u8 token, chan;
> >> + struct scpi_xfer *msg;
> >> + struct scpi_chan *scpi_chan;
> >> +
> >> + chan = atomic_inc_return(_info->next_chan) % 
> >> scpi_info->num_chans;
> >> + scpi_chan = scpi_info->channels + chan;
> >> +
> >> + msg = get_scpi_xfer(scpi_chan);
> >> + if (!msg)
> >> + return -ENOMEM;
> >> +
> >> + token = atomic_inc_return(_chan->token) & CMD_TOKEN_ID_MASK;
> >
> > So, this 8 bit token is what's used to 'uniquely' identify a pending
> > command. But as it's just an incrementing value, then if one command
> > gets delayed for long enough that 256 more are issued then we will have
> > a non-unique value and scpi_process_cmd can go wrong.
> >
> 
> IMO by the time 256 message are queued up and serviced we would timeout
> on the initial command. Moreover the core mailbox has sent the mailbox
> length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
> remote chance of hit the corner case.

The corner case can be hit even if the queue length is only 2, because
other processes/cpus can use the other message we don't own here and
they can send then receive a message using that, 256 times. The corner
case doesn't require 256 simultaneous outstanding requests.

That is the reason I suggested that rather than using an incrementing
value for the 'unique' token, that each message instead contain the
value of the token to use with it.

> 
> > Note, this delay doesn't just have to be at the SCPI end. We could get
> > preempted here (?) before actually sending the command to the SCP and
> > other kernel threads or processes could send those other 256 commands
> > before we get to run again.
> >
> 
> Agreed, but we would still timeout after 3 jiffies max.

But we haven't started any timeout yet, the 3 jiffies won't start until
we get scheduled again and call wait_for_completion_timeout below.
> 
> > Wouldn't it be better instead to have scpi_alloc_xfer_list add a unique
> > number to each struct scpi_xfer.
> >
> 
> One of reason using it part of command is that SCP gives it back in the
> response to compare.

Can't we fill the token in the command from the value stored in the
struct scpi_xfer we are using to send that command?

> >> +
> >> + msg->slot = BIT(SCPI_SLOT);
> >> + msg->cmd = PACK_SCPI_CMD(cmd, token, len);
> >> + msg->tx_buf = tx_buf;
> >> + msg->tx_len = len;
> >> + msg->rx_buf = rx_buf;
> >> + init_completion(>done);
> >> +
> >> + ret = mbox_send_message(scpi_chan->chan, msg);
> >> + if (ret < 0 || !rx_buf)
> >> + goto out;
> >> +
> >> + if (!wait_for_completion_timeout(>done, MAX_RX_TIMEOUT))
> >> + ret = -ETIMEDOUT;
> >> + else
> >> + /* first status word */
> >> + ret = le32_to_cpu(msg->status);
> >> +out:
> >> + if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
> >
> > So, even with my suggestion that the unique message identifies are
> > fixed values stored in struct scpi_xfer, we can still have the situation
> > where we timeout a request, that scpi_xfer then getting used for another
> > request, and finally the SCP completes the request that we timed out,
> > which has the same 'unique' value as the later one.
> >
> 
> As explained above I can't imagine hitting this condition. I will think
> more on that again.

I can imagine :-) If we timeout and discard messages, and reuse it's
unique id, there is always the possibility of this confusion occurring.
No amount of coding in the kernel can get around that. The only thing
you can do to get out of this quandary is make assumptions about how the
SCP firmware behaves.

> 
> > One way to handle that it to not have any timeout on requests and assume
> > the firmware isn't buggy.
> >
> 
> That's something I can't do ;) based on my experience so far. It's good
> to assume firmware *can be buggy* and handle all possible errors.

I'm inclined to agree.

>  Think
> about the development firmware using this driver. This has been very
> useful when I was testing the development versions. Even under stress
> conditions I still see timeouts(very rarely though), so my personal
> preference is to have them.

But the SCPI protocol unfortunately doesn't seem to allow us to robustly
handle timeouts. Well, we could keep a list of tokens used in timed out
messages, and not reuse them. But if, as you say, timeouts do occur,
then with only 256 available, we are likely to run out.

When I brought this up 9 months ago, it was pointed out that the
limitation of an 8-bit token for a message because was because the
protocol designers had were cramming it into the 32-bit value poked into
the MHU register. The new finished protocol spec doesn't use the MHU
register any more for this data, but the limitations

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-29 Thread Sudeep Holla 



On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:

On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:

This patch adds support for System Control and Power Interface (SCPI)
Message Protocol used between the Application Cores(AP) and the System
Control Processor(SCP). The MHU peripheral provides a mechanism for
inter-processor communication between SCP's M3 processor and AP.

SCP offers control and management of the core/cluster power states,
various power domain DVFS including the core/cluster, certain system
clocks configuration, thermal sensors and many others.

This protocol driver provides interface for all the client drivers using
SCPI to make use of the features offered by the SCP.

Signed-off-by: Sudeep Holla 
Cc: Rob Herring 
Cc: Mark Rutland 
CC: Jassi Brar 
Cc: Liviu Dudau 
Cc: Lorenzo Pieralisi 
Cc: Jon Medhurst (Tixy) 
Cc: devicet...@vger.kernel.org
---


There are several spelling errors but I won't point out each, sure you
can find them with a spellcheck ;-) I'll just comment on the code...



OK :)


[...]

+++ b/drivers/mailbox/scpi_protocol.c
@@ -0,0 +1,694 @@
+/*
+ * System Control and Power Interface (SCPI) Message Protocol driver
+ *
+ * SCPI Message Protocol is used between the System Control Processor(SCP)
+ * and the Application Processors(AP). The Message Handling Unit(MHU)
+ * provides a mechanism for inter-processor communication between SCP's
+ * Cortex M3 and AP.
+ *
+ * SCP offers control and management of the core/cluster power states,
+ * various power domain DVFS including the core/cluster, certain system
+ * clocks configuration, thermal sensors and many others.
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program. If not, see .
+ */
+


[...]


+
+static inline int scpi_to_linux_errno(int errno)
+{
+ if (errno >= SCPI_SUCCESS && errno < SCPI_ERR_MAX)
+ return scpi_linux_errmap[errno];
+ return -EIO;
+}
+
+static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
+{
+ unsigned long flags;
+ struct scpi_xfer *t, *match = NULL;
+
+ spin_lock_irqsave(>rx_lock, flags);
+ if (list_empty(>rx_pending)) {
+ spin_unlock_irqrestore(>rx_lock, flags);
+ return;
+ }
+
+ list_for_each_entry(t, >rx_pending, node)
+ if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {


So if UNIQ here isn't actually unique amongst all pending requests, its
possible we'll pick the wrong one. There's a couple of scenarios where
that can happen, comments further down about that.


+ list_del(>node);
+ match = t;
+ break;
+ }
+ /* check if wait_for_completion is in progress or timed-out */
+ if (match && !completion_done(>done)) {
+ struct scpi_shared_mem *mem = ch->rx_payload;
+
+ match->status = le32_to_cpu(mem->status);
+ memcpy_fromio(match->rx_buf, mem->payload, CMD_SIZE(cmd));
+ complete(>done);
+ }
+ spin_unlock_irqrestore(>rx_lock, flags);
+}
+
+static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
+{
+ struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
+ struct scpi_shared_mem *mem = ch->rx_payload;
+ u32 cmd = le32_to_cpu(mem->command);
+
+ scpi_process_cmd(ch, cmd);
+}
+
+static void scpi_tx_prepare(struct mbox_client *c, void *msg)
+{
+ unsigned long flags;
+ struct scpi_xfer *t = msg;
+ struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
+ struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
+
+ mem->command = cpu_to_le32(t->cmd);
+ if (t->tx_buf)
+ memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
+ if (t->rx_buf) {
+ spin_lock_irqsave(>rx_lock, flags);
+ list_add_tail(>node, >rx_pending);
+ spin_unlock_irqrestore(>rx_lock, flags);
+ }
+}
+
+static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
+{
+ struct scpi_xfer *t;
+
+ mutex_lock(>xfers_lock);
+ if (list_empty(>xfers_list)) {
+ mutex_unlock(>xfers_lock);
+ return NULL;
+ }
+ t = list_first_entry(>xfers_list, struct scpi_xfer, node);
+ list_del(>node);
+ mutex_unlock(>xfers_lock);
+ return t;
+}
+
+static void put_scpi_xfer(struct scpi_xfer *t, struct scpi_chan *ch)
+{
+ mutex_lock(>xfers_lock);
+ list_add_tail(>node, >xfers_list);
+

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-29 Thread Jon Medhurst (Tixy) 
On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:
 On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:
  On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
[...]
  + int ret;
  + u8 token, chan;
  + struct scpi_xfer *msg;
  + struct scpi_chan *scpi_chan;
  +
  + chan = atomic_inc_return(scpi_info-next_chan) % 
  scpi_info-num_chans;
  + scpi_chan = scpi_info-channels + chan;
  +
  + msg = get_scpi_xfer(scpi_chan);
  + if (!msg)
  + return -ENOMEM;
  +
  + token = atomic_inc_return(scpi_chan-token)  CMD_TOKEN_ID_MASK;
 
  So, this 8 bit token is what's used to 'uniquely' identify a pending
  command. But as it's just an incrementing value, then if one command
  gets delayed for long enough that 256 more are issued then we will have
  a non-unique value and scpi_process_cmd can go wrong.
 
 
 IMO by the time 256 message are queued up and serviced we would timeout
 on the initial command. Moreover the core mailbox has sent the mailbox
 length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
 remote chance of hit the corner case.

The corner case can be hit even if the queue length is only 2, because
other processes/cpus can use the other message we don't own here and
they can send then receive a message using that, 256 times. The corner
case doesn't require 256 simultaneous outstanding requests.

That is the reason I suggested that rather than using an incrementing
value for the 'unique' token, that each message instead contain the
value of the token to use with it.

 
  Note, this delay doesn't just have to be at the SCPI end. We could get
  preempted here (?) before actually sending the command to the SCP and
  other kernel threads or processes could send those other 256 commands
  before we get to run again.
 
 
 Agreed, but we would still timeout after 3 jiffies max.

But we haven't started any timeout yet, the 3 jiffies won't start until
we get scheduled again and call wait_for_completion_timeout below.
 
  Wouldn't it be better instead to have scpi_alloc_xfer_list add a unique
  number to each struct scpi_xfer.
 
 
 One of reason using it part of command is that SCP gives it back in the
 response to compare.

Can't we fill the token in the command from the value stored in the
struct scpi_xfer we are using to send that command?

  +
  + msg-slot = BIT(SCPI_SLOT);
  + msg-cmd = PACK_SCPI_CMD(cmd, token, len);
  + msg-tx_buf = tx_buf;
  + msg-tx_len = len;
  + msg-rx_buf = rx_buf;
  + init_completion(msg-done);
  +
  + ret = mbox_send_message(scpi_chan-chan, msg);
  + if (ret  0 || !rx_buf)
  + goto out;
  +
  + if (!wait_for_completion_timeout(msg-done, MAX_RX_TIMEOUT))
  + ret = -ETIMEDOUT;
  + else
  + /* first status word */
  + ret = le32_to_cpu(msg-status);
  +out:
  + if (ret  0  rx_buf) /* remove entry from the list if timed-out */
 
  So, even with my suggestion that the unique message identifies are
  fixed values stored in struct scpi_xfer, we can still have the situation
  where we timeout a request, that scpi_xfer then getting used for another
  request, and finally the SCP completes the request that we timed out,
  which has the same 'unique' value as the later one.
 
 
 As explained above I can't imagine hitting this condition. I will think
 more on that again.

I can imagine :-) If we timeout and discard messages, and reuse it's
unique id, there is always the possibility of this confusion occurring.
No amount of coding in the kernel can get around that. The only thing
you can do to get out of this quandary is make assumptions about how the
SCP firmware behaves.

 
  One way to handle that it to not have any timeout on requests and assume
  the firmware isn't buggy.
 
 
 That's something I can't do ;) based on my experience so far. It's good
 to assume firmware *can be buggy* and handle all possible errors.

I'm inclined to agree.

  Think
 about the development firmware using this driver. This has been very
 useful when I was testing the development versions. Even under stress
 conditions I still see timeouts(very rarely though), so my personal
 preference is to have them.

But the SCPI protocol unfortunately doesn't seem to allow us to robustly
handle timeouts. Well, we could keep a list of tokens used in timed out
messages, and not reuse them. But if, as you say, timeouts do occur,
then with only 256 available, we are likely to run out.

When I brought this up 9 months ago, it was pointed out that the
limitation of an 8-bit token for a message because was because the
protocol designers had were cramming it into the 32-bit value poked into
the MHU register. The new finished protocol spec doesn't use the MHU
register any more for this data, but the limitations we're kept by
specifying the same command data format but just stored in the shared
memory. Pity the opportunity wasn't taken to expand the token size to
something that allowed

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-29 Thread Sudeep Holla 



On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:

On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:

This patch adds support for System Control and Power Interface (SCPI)
Message Protocol used between the Application Cores(AP) and the System
Control Processor(SCP). The MHU peripheral provides a mechanism for
inter-processor communication between SCP's M3 processor and AP.

SCP offers control and management of the core/cluster power states,
various power domain DVFS including the core/cluster, certain system
clocks configuration, thermal sensors and many others.

This protocol driver provides interface for all the client drivers using
SCPI to make use of the features offered by the SCP.

Signed-off-by: Sudeep Holla sudeep.ho...@arm.com
Cc: Rob Herring robh...@kernel.org
Cc: Mark Rutland mark.rutl...@arm.com
CC: Jassi Brar jassisinghb...@gmail.com
Cc: Liviu Dudau liviu.du...@arm.com
Cc: Lorenzo Pieralisi lorenzo.pieral...@arm.com
Cc: Jon Medhurst (Tixy) t...@linaro.org
Cc: devicet...@vger.kernel.org
---


There are several spelling errors but I won't point out each, sure you
can find them with a spellcheck ;-) I'll just comment on the code...



OK :)


[...]

+++ b/drivers/mailbox/scpi_protocol.c
@@ -0,0 +1,694 @@
+/*
+ * System Control and Power Interface (SCPI) Message Protocol driver
+ *
+ * SCPI Message Protocol is used between the System Control Processor(SCP)
+ * and the Application Processors(AP). The Message Handling Unit(MHU)
+ * provides a mechanism for inter-processor communication between SCP's
+ * Cortex M3 and AP.
+ *
+ * SCP offers control and management of the core/cluster power states,
+ * various power domain DVFS including the core/cluster, certain system
+ * clocks configuration, thermal sensors and many others.
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program. If not, see http://www.gnu.org/licenses/.
+ */
+


[...]


+
+static inline int scpi_to_linux_errno(int errno)
+{
+ if (errno = SCPI_SUCCESS  errno  SCPI_ERR_MAX)
+ return scpi_linux_errmap[errno];
+ return -EIO;
+}
+
+static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
+{
+ unsigned long flags;
+ struct scpi_xfer *t, *match = NULL;
+
+ spin_lock_irqsave(ch-rx_lock, flags);
+ if (list_empty(ch-rx_pending)) {
+ spin_unlock_irqrestore(ch-rx_lock, flags);
+ return;
+ }
+
+ list_for_each_entry(t, ch-rx_pending, node)
+ if (CMD_XTRACT_UNIQ(t-cmd) == CMD_XTRACT_UNIQ(cmd)) {


So if UNIQ here isn't actually unique amongst all pending requests, its
possible we'll pick the wrong one. There's a couple of scenarios where
that can happen, comments further down about that.


+ list_del(t-node);
+ match = t;
+ break;
+ }
+ /* check if wait_for_completion is in progress or timed-out */
+ if (match  !completion_done(match-done)) {
+ struct scpi_shared_mem *mem = ch-rx_payload;
+
+ match-status = le32_to_cpu(mem-status);
+ memcpy_fromio(match-rx_buf, mem-payload, CMD_SIZE(cmd));
+ complete(match-done);
+ }
+ spin_unlock_irqrestore(ch-rx_lock, flags);
+}
+
+static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
+{
+ struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
+ struct scpi_shared_mem *mem = ch-rx_payload;
+ u32 cmd = le32_to_cpu(mem-command);
+
+ scpi_process_cmd(ch, cmd);
+}
+
+static void scpi_tx_prepare(struct mbox_client *c, void *msg)
+{
+ unsigned long flags;
+ struct scpi_xfer *t = msg;
+ struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
+ struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch-tx_payload;
+
+ mem-command = cpu_to_le32(t-cmd);
+ if (t-tx_buf)
+ memcpy_toio(mem-payload, t-tx_buf, t-tx_len);
+ if (t-rx_buf) {
+ spin_lock_irqsave(ch-rx_lock, flags);
+ list_add_tail(t-node, ch-rx_pending);
+ spin_unlock_irqrestore(ch-rx_lock, flags);
+ }
+}
+
+static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
+{
+ struct scpi_xfer *t;
+
+ mutex_lock(ch-xfers_lock);
+ if (list_empty(ch-xfers_list)) {
+ mutex_unlock(ch-xfers_lock);
+ return NULL;
+ }
+ t = list_first_entry(ch-xfers_list, struct scpi_xfer, node);
+ list_del(t-node);
+ mutex_unlock(ch-xfers_lock);
+ return t;
+}
+

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-29 Thread Jon Medhurst (Tixy) 
On Wed, 2015-04-29 at 12:43 +0100, Jon Medhurst (Tixy) wrote:
 On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:
  On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:
   On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
 [...]
   + int ret;
   + u8 token, chan;
   + struct scpi_xfer *msg;
   + struct scpi_chan *scpi_chan;
   +
   + chan = atomic_inc_return(scpi_info-next_chan) % 
   scpi_info-num_chans;
   + scpi_chan = scpi_info-channels + chan;
   +
   + msg = get_scpi_xfer(scpi_chan);
   + if (!msg)
   + return -ENOMEM;
   +
   + token = atomic_inc_return(scpi_chan-token)  CMD_TOKEN_ID_MASK;
  
   So, this 8 bit token is what's used to 'uniquely' identify a pending
   command. But as it's just an incrementing value, then if one command
   gets delayed for long enough that 256 more are issued then we will have
   a non-unique value and scpi_process_cmd can go wrong.
  
  
  IMO by the time 256 message are queued up and serviced we would timeout
  on the initial command. Moreover the core mailbox has sent the mailbox
  length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
  remote chance of hit the corner case.
 
 The corner case can be hit even if the queue length is only 2, because
 other processes/cpus can use the other message we don't own here and
 they can send then receive a message using that, 256 times. The corner
 case doesn't require 256 simultaneous outstanding requests.
 
 That is the reason I suggested that rather than using an incrementing
 value for the 'unique' token, that each message instead contain the
 value of the token to use with it.

Of course, I failed to mention that this solution to this problem makes
thing worse for the situation where we timeout messages, because the
same token will get reused quicker in that case. So, in practice, if we
have timeouts, and a unchangeable protocol limitation of 256 tokens,
then using those tokens in order, for each message sent is probably the
best we can do.

Perhaps that's the clue, generate and add the token to the message, just
before transmission via the MHU, at a point where we know no other
request can overtake us. In scpi_tx_prepare? Perhaps, it might also be
good to only use up a token if we are expecting a response, and use zero
for other messages?

Something like this totally untested patch...

diff --git a/drivers/mailbox/scpi_protocol.c b/drivers/mailbox/scpi_protocol.c
index c74575b..5818d9b 100644
--- a/drivers/mailbox/scpi_protocol.c
+++ b/drivers/mailbox/scpi_protocol.c
@@ -286,14 +286,23 @@ static void scpi_tx_prepare(struct mbox_client *c, void 
*msg)
struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch-tx_payload;
 
-   mem-command = cpu_to_le32(t-cmd);
if (t-tx_buf)
memcpy_toio(mem-payload, t-tx_buf, t-tx_len);
if (t-rx_buf) {
+   int token;
spin_lock_irqsave(ch-rx_lock, flags);
+   /*
+* Presumably we can do this token setting outside
+* spinlock and still be safe from concurrency?
+*/
+   do
+   token = (++ch-token)  CMD_TOKEN_ID_MASK;
+   while(!token);
+   t-cmd |= token  CMD_TOKEN_ID_SHIFT;
list_add_tail(t-node, ch-rx_pending);
spin_unlock_irqrestore(ch-rx_lock, flags);
}
+   mem-command = cpu_to_le32(t-cmd);
 }
 
 static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
@@ -322,7 +331,7 @@ static int
 scpi_send_message(u8 cmd, void *tx_buf, unsigned int len, void *rx_buf)
 {
int ret;
-   u8 token, chan;
+   u8 chan;
struct scpi_xfer *msg;
struct scpi_chan *scpi_chan;
 
@@ -333,10 +342,8 @@ scpi_send_message(u8 cmd, void *tx_buf, unsigned int len, 
void *rx_buf)
if (!msg)
return -ENOMEM;
 
-   token = atomic_inc_return(scpi_chan-token)  CMD_TOKEN_ID_MASK;
-
msg-slot = BIT(SCPI_SLOT);
-   msg-cmd = PACK_SCPI_CMD(cmd, token, len);
+   msg-cmd = PACK_SCPI_CMD(cmd, 0, len);
msg-tx_buf = tx_buf;
msg-tx_len = len;
msg-rx_buf = rx_buf;


-- 
Tixy

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-29 Thread Sudeep Holla 



On 29/04/15 13:25, Jon Medhurst (Tixy) wrote:

On Wed, 2015-04-29 at 12:43 +0100, Jon Medhurst (Tixy) wrote:

On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:

On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:

On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:

[...]

+ int ret;
+ u8 token, chan;
+ struct scpi_xfer *msg;
+ struct scpi_chan *scpi_chan;
+
+ chan = atomic_inc_return(scpi_info-next_chan) % scpi_info-num_chans;
+ scpi_chan = scpi_info-channels + chan;
+
+ msg = get_scpi_xfer(scpi_chan);
+ if (!msg)
+ return -ENOMEM;
+
+ token = atomic_inc_return(scpi_chan-token)  CMD_TOKEN_ID_MASK;


So, this 8 bit token is what's used to 'uniquely' identify a pending
command. But as it's just an incrementing value, then if one command
gets delayed for long enough that 256 more are issued then we will have
a non-unique value and scpi_process_cmd can go wrong.



IMO by the time 256 message are queued up and serviced we would timeout
on the initial command. Moreover the core mailbox has sent the mailbox
length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
remote chance of hit the corner case.


The corner case can be hit even if the queue length is only 2, because
other processes/cpus can use the other message we don't own here and
they can send then receive a message using that, 256 times. The corner
case doesn't require 256 simultaneous outstanding requests.

That is the reason I suggested that rather than using an incrementing
value for the 'unique' token, that each message instead contain the
value of the token to use with it.


Of course, I failed to mention that this solution to this problem makes
thing worse for the situation where we timeout messages, because the
same token will get reused quicker in that case. So, in practice, if we
have timeouts, and a unchangeable protocol limitation of 256 tokens,
then using those tokens in order, for each message sent is probably the
best we can do.



I agree, I think we must be happy with that for now :)


Perhaps that's the clue, generate and add the token to the message, just
before transmission via the MHU, at a point where we know no other
request can overtake us. In scpi_tx_prepare? Perhaps, it might also be
good to only use up a token if we are expecting a response, and use zero
for other messages?

Something like this totally untested patch...



Looks good and best we can do with the limitations we have IMO

Regards,
Sudeep

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-29 Thread Sudeep Holla 

Hi Tixy,

On 29/04/15 12:43, Jon Medhurst (Tixy) wrote:

On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:

On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:

On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:

[...]

+ int ret;
+ u8 token, chan;
+ struct scpi_xfer *msg;
+ struct scpi_chan *scpi_chan;
+
+ chan = atomic_inc_return(scpi_info-next_chan) % scpi_info-num_chans;
+ scpi_chan = scpi_info-channels + chan;
+
+ msg = get_scpi_xfer(scpi_chan);
+ if (!msg)
+ return -ENOMEM;
+
+ token = atomic_inc_return(scpi_chan-token)  CMD_TOKEN_ID_MASK;


So, this 8 bit token is what's used to 'uniquely' identify a pending
command. But as it's just an incrementing value, then if one command
gets delayed for long enough that 256 more are issued then we will have
a non-unique value and scpi_process_cmd can go wrong.



IMO by the time 256 message are queued up and serviced we would timeout
on the initial command. Moreover the core mailbox has sent the mailbox
length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
remote chance of hit the corner case.


The corner case can be hit even if the queue length is only 2, because
other processes/cpus can use the other message we don't own here and
they can send then receive a message using that, 256 times. The corner
case doesn't require 256 simultaneous outstanding requests.



Good point, I missed it completely.


That is the reason I suggested that rather than using an incrementing
value for the 'unique' token, that each message instead contain the
value of the token to use with it.




Note, this delay doesn't just have to be at the SCPI end. We could get
preempted here (?) before actually sending the command to the SCP and
other kernel threads or processes could send those other 256 commands
before we get to run again.



Agreed, but we would still timeout after 3 jiffies max.


But we haven't started any timeout yet, the 3 jiffies won't start until
we get scheduled again and call wait_for_completion_timeout below.


Agreed.




Wouldn't it be better instead to have scpi_alloc_xfer_list add a unique
number to each struct scpi_xfer.



One of reason using it part of command is that SCP gives it back in the
response to compare.


Can't we fill the token in the command from the value stored in the
struct scpi_xfer we are using to send that command?



Yes we can but 256 limitation still exists but solve some issues at-least.


+
+ msg-slot = BIT(SCPI_SLOT);
+ msg-cmd = PACK_SCPI_CMD(cmd, token, len);
+ msg-tx_buf = tx_buf;
+ msg-tx_len = len;
+ msg-rx_buf = rx_buf;
+ init_completion(msg-done);
+
+ ret = mbox_send_message(scpi_chan-chan, msg);
+ if (ret  0 || !rx_buf)
+ goto out;
+
+ if (!wait_for_completion_timeout(msg-done, MAX_RX_TIMEOUT))
+ ret = -ETIMEDOUT;
+ else
+ /* first status word */
+ ret = le32_to_cpu(msg-status);
+out:
+ if (ret  0  rx_buf) /* remove entry from the list if timed-out */


So, even with my suggestion that the unique message identifies are
fixed values stored in struct scpi_xfer, we can still have the situation
where we timeout a request, that scpi_xfer then getting used for another
request, and finally the SCP completes the request that we timed out,
which has the same 'unique' value as the later one.



As explained above I can't imagine hitting this condition. I will think
more on that again.


I can imagine :-) If we timeout and discard messages, and reuse it's
unique id, there is always the possibility of this confusion occurring.
No amount of coding in the kernel can get around that. The only thing
you can do to get out of this quandary is make assumptions about how the
SCP firmware behaves.



Agreed again.




One way to handle that it to not have any timeout on requests and assume
the firmware isn't buggy.



That's something I can't do ;) based on my experience so far. It's good
to assume firmware *can be buggy* and handle all possible errors.


I'm inclined to agree.



Thanks :)


  Think
about the development firmware using this driver. This has been very
useful when I was testing the development versions. Even under stress
conditions I still see timeouts(very rarely though), so my personal
preference is to have them.


But the SCPI protocol unfortunately doesn't seem to allow us to robustly
handle timeouts. Well, we could keep a list of tokens used in timed out
messages, and not reuse them. But if, as you say, timeouts do occur,
then with only 256 available, we are likely to run out.



Yes :(


When I brought this up 9 months ago, it was pointed out that the
limitation of an 8-bit token for a message because was because the
protocol designers had were cramming it into the 32-bit value poked into
the MHU register. The new finished protocol spec doesn't use the MHU
register any more for this data, but the limitations we're kept by
specifying the same command data format but

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-28 Thread Jon Medhurst (Tixy) 
On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
> This patch adds support for System Control and Power Interface (SCPI)
> Message Protocol used between the Application Cores(AP) and the System
> Control Processor(SCP). The MHU peripheral provides a mechanism for
> inter-processor communication between SCP's M3 processor and AP.
> 
> SCP offers control and management of the core/cluster power states,
> various power domain DVFS including the core/cluster, certain system
> clocks configuration, thermal sensors and many others.
> 
> This protocol driver provides interface for all the client drivers using
> SCPI to make use of the features offered by the SCP.
> 
> Signed-off-by: Sudeep Holla 
> Cc: Rob Herring 
> Cc: Mark Rutland 
> CC: Jassi Brar 
> Cc: Liviu Dudau 
> Cc: Lorenzo Pieralisi 
> Cc: Jon Medhurst (Tixy) 
> Cc: devicet...@vger.kernel.org
> ---

There are several spelling errors but I won't point out each, sure you
can find them with a spellcheck ;-) I'll just comment on the code...

[...]
> +++ b/drivers/mailbox/scpi_protocol.c
> @@ -0,0 +1,694 @@
> +/*
> + * System Control and Power Interface (SCPI) Message Protocol driver
> + *
> + * SCPI Message Protocol is used between the System Control Processor(SCP)
> + * and the Application Processors(AP). The Message Handling Unit(MHU)
> + * provides a mechanism for inter-processor communication between SCP's
> + * Cortex M3 and AP.
> + *
> + * SCP offers control and management of the core/cluster power states,
> + * various power domain DVFS including the core/cluster, certain system
> + * clocks configuration, thermal sensors and many others.
> + *
> + * Copyright (C) 2015 ARM Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program. If not, see .
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define CMD_ID_SHIFT 0
> +#define CMD_ID_MASK  0x7f
> +#define CMD_TOKEN_ID_SHIFT   8
> +#define CMD_TOKEN_ID_MASK0xff
> +#define CMD_DATA_SIZE_SHIFT  16
> +#define CMD_DATA_SIZE_MASK   0x1ff
> +#define PACK_SCPI_CMD(cmd_id, token, tx_sz)  \
> + cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |   \
> + (((token) & CMD_TOKEN_ID_MASK) << CMD_TOKEN_ID_SHIFT) | \
> + (((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
> +
> +#define CMD_SIZE(cmd)(((cmd) >> CMD_DATA_SIZE_SHIFT) & 
> CMD_DATA_SIZE_MASK)
> +#define CMD_UNIQ_MASK(CMD_TOKEN_ID_MASK << CMD_TOKEN_ID_SHIFT | 
> CMD_ID_MASK)
> +#define CMD_XTRACT_UNIQ(cmd) ((cmd) & CMD_UNIQ_MASK)
> +
> +#define SCPI_SLOT0
> +
> +#define MAX_DVFS_DOMAINS 8
> +#define MAX_DVFS_OPPS8
> +#define DVFS_LATENCY(hdr)(le32_to_cpu(hdr) >> 16)
> +#define DVFS_OPP_COUNT(hdr)  ((le32_to_cpu(hdr) >> 8) & 0xff)
> +
> +#define PROTOCOL_REV_MINOR_BITS  16
> +#define PROTOCOL_REV_MINOR_MASK  ((1U << PROTOCOL_REV_MINOR_BITS) - 1)
> +#define PROTOCOL_REV_MAJOR(x)((x) >> PROTOCOL_REV_MINOR_BITS)
> +#define PROTOCOL_REV_MINOR(x)((x) & PROTOCOL_REV_MINOR_MASK)
> +
> +#define FW_REV_MAJOR_BITS24
> +#define FW_REV_MINOR_BITS16
> +#define FW_REV_PATCH_MASK((1U << FW_REV_MINOR_BITS) - 1)
> +#define FW_REV_MINOR_MASK((1U << FW_REV_MAJOR_BITS) - 1)
> +#define FW_REV_MAJOR(x)  ((x) >> FW_REV_MAJOR_BITS)
> +#define FW_REV_MINOR(x)  (((x) & FW_REV_MINOR_MASK) >> 
> FW_REV_MINOR_BITS)
> +#define FW_REV_PATCH(x)  ((x) & FW_REV_PATCH_MASK)
> +
> +#define MAX_RX_TIMEOUT   (msecs_to_jiffies(30))
> +
> +enum scpi_error_codes {
> + SCPI_SUCCESS = 0, /* Success */
> + SCPI_ERR_PARAM = 1, /* Invalid parameter(s) */
> + SCPI_ERR_ALIGN = 2, /* Invalid alignment */
> + SCPI_ERR_SIZE = 3, /* Invalid size */
> + SCPI_ERR_HANDLER = 4, /* Invalid handler/callback */
> + SCPI_ERR_ACCESS = 5, /* Invalid access/permission denied */
> + SCPI_ERR_RANGE = 6, /* Value out of range */
> + SCPI_ERR_TIMEOUT = 7, /* Timeout has occurred */
> + SCPI_ERR_NOMEM = 8, /* Invalid memory area or pointer */
> + SCPI_ERR_PWRSTATE = 9, /* Invalid power state */
> + SCPI_ERR_SUPPORT = 10, /* Not supported or disabled */
> + SCPI_ERR_DEVICE = 11, /* Device error */
> +

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-28 Thread Sudeep Holla 



On 28/04/15 08:36, Paul Bolle wrote:

Just one nit: a license mismatch.

On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:

--- /dev/null
+++ b/drivers/mailbox/scpi_protocol.c



+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program. If not, see .


This states the license is GPL v2.


+MODULE_LICENSE("GPL");


And, according to include/linux/module.h, this states the license is GPL
v2 or later. So I think either the comment at the top of this file or
the license ident used in the MODULE_LICENSE() macro should be changed.

Likewise for 2/4 and 4/4.


Thanks for pointing this out. Will fix in the next version.

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-28 Thread Paul Bolle 
Just one nit: a license mismatch.

On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
> --- /dev/null
> +++ b/drivers/mailbox/scpi_protocol.c

> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program. If not, see .

This states the license is GPL v2.

> +MODULE_LICENSE("GPL");

And, according to include/linux/module.h, this states the license is GPL
v2 or later. So I think either the comment at the top of this file or
the license ident used in the MODULE_LICENSE() macro should be changed.

Likewise for 2/4 and 4/4.

Thanks,


Paul Bolle

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-28 Thread Jon Medhurst (Tixy) 
On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
 This patch adds support for System Control and Power Interface (SCPI)
 Message Protocol used between the Application Cores(AP) and the System
 Control Processor(SCP). The MHU peripheral provides a mechanism for
 inter-processor communication between SCP's M3 processor and AP.
 
 SCP offers control and management of the core/cluster power states,
 various power domain DVFS including the core/cluster, certain system
 clocks configuration, thermal sensors and many others.
 
 This protocol driver provides interface for all the client drivers using
 SCPI to make use of the features offered by the SCP.
 
 Signed-off-by: Sudeep Holla sudeep.ho...@arm.com
 Cc: Rob Herring robh...@kernel.org
 Cc: Mark Rutland mark.rutl...@arm.com
 CC: Jassi Brar jassisinghb...@gmail.com
 Cc: Liviu Dudau liviu.du...@arm.com
 Cc: Lorenzo Pieralisi lorenzo.pieral...@arm.com
 Cc: Jon Medhurst (Tixy) t...@linaro.org
 Cc: devicet...@vger.kernel.org
 ---

There are several spelling errors but I won't point out each, sure you
can find them with a spellcheck ;-) I'll just comment on the code...

[...]
 +++ b/drivers/mailbox/scpi_protocol.c
 @@ -0,0 +1,694 @@
 +/*
 + * System Control and Power Interface (SCPI) Message Protocol driver
 + *
 + * SCPI Message Protocol is used between the System Control Processor(SCP)
 + * and the Application Processors(AP). The Message Handling Unit(MHU)
 + * provides a mechanism for inter-processor communication between SCP's
 + * Cortex M3 and AP.
 + *
 + * SCP offers control and management of the core/cluster power states,
 + * various power domain DVFS including the core/cluster, certain system
 + * clocks configuration, thermal sensors and many others.
 + *
 + * Copyright (C) 2015 ARM Ltd.
 + *
 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms and conditions of the GNU General Public License,
 + * version 2, as published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope it will be useful, but WITHOUT
 + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
 + * more details.
 + *
 + * You should have received a copy of the GNU General Public License along
 + * with this program. If not, see http://www.gnu.org/licenses/.
 + */
 +
 +#define pr_fmt(fmt) KBUILD_MODNAME :  fmt
 +
 +#include linux/bitmap.h
 +#include linux/device.h
 +#include linux/err.h
 +#include linux/export.h
 +#include linux/io.h
 +#include linux/kernel.h
 +#include linux/list.h
 +#include linux/mailbox_client.h
 +#include linux/module.h
 +#include linux/of_address.h
 +#include linux/of_platform.h
 +#include linux/printk.h
 +#include linux/scpi_protocol.h
 +#include linux/slab.h
 +#include linux/spinlock.h
 +
 +#define CMD_ID_SHIFT 0
 +#define CMD_ID_MASK  0x7f
 +#define CMD_TOKEN_ID_SHIFT   8
 +#define CMD_TOKEN_ID_MASK0xff
 +#define CMD_DATA_SIZE_SHIFT  16
 +#define CMD_DATA_SIZE_MASK   0x1ff
 +#define PACK_SCPI_CMD(cmd_id, token, tx_sz)  \
 + cmd_id)  CMD_ID_MASK)  CMD_ID_SHIFT) |   \
 + (((token)  CMD_TOKEN_ID_MASK)  CMD_TOKEN_ID_SHIFT) | \
 + (((tx_sz)  CMD_DATA_SIZE_MASK)  CMD_DATA_SIZE_SHIFT))
 +
 +#define CMD_SIZE(cmd)(((cmd)  CMD_DATA_SIZE_SHIFT)  
 CMD_DATA_SIZE_MASK)
 +#define CMD_UNIQ_MASK(CMD_TOKEN_ID_MASK  CMD_TOKEN_ID_SHIFT | 
 CMD_ID_MASK)
 +#define CMD_XTRACT_UNIQ(cmd) ((cmd)  CMD_UNIQ_MASK)
 +
 +#define SCPI_SLOT0
 +
 +#define MAX_DVFS_DOMAINS 8
 +#define MAX_DVFS_OPPS8
 +#define DVFS_LATENCY(hdr)(le32_to_cpu(hdr)  16)
 +#define DVFS_OPP_COUNT(hdr)  ((le32_to_cpu(hdr)  8)  0xff)
 +
 +#define PROTOCOL_REV_MINOR_BITS  16
 +#define PROTOCOL_REV_MINOR_MASK  ((1U  PROTOCOL_REV_MINOR_BITS) - 1)
 +#define PROTOCOL_REV_MAJOR(x)((x)  PROTOCOL_REV_MINOR_BITS)
 +#define PROTOCOL_REV_MINOR(x)((x)  PROTOCOL_REV_MINOR_MASK)
 +
 +#define FW_REV_MAJOR_BITS24
 +#define FW_REV_MINOR_BITS16
 +#define FW_REV_PATCH_MASK((1U  FW_REV_MINOR_BITS) - 1)
 +#define FW_REV_MINOR_MASK((1U  FW_REV_MAJOR_BITS) - 1)
 +#define FW_REV_MAJOR(x)  ((x)  FW_REV_MAJOR_BITS)
 +#define FW_REV_MINOR(x)  (((x)  FW_REV_MINOR_MASK)  
 FW_REV_MINOR_BITS)
 +#define FW_REV_PATCH(x)  ((x)  FW_REV_PATCH_MASK)
 +
 +#define MAX_RX_TIMEOUT   (msecs_to_jiffies(30))
 +
 +enum scpi_error_codes {
 + SCPI_SUCCESS = 0, /* Success */
 + SCPI_ERR_PARAM = 1, /* Invalid parameter(s) */
 + SCPI_ERR_ALIGN = 2, /* Invalid alignment */
 + SCPI_ERR_SIZE = 3, /* Invalid size */
 + SCPI_ERR_HANDLER = 4, /* Invalid handler/callback */
 + SCPI_ERR_ACCESS = 5, /* Invalid access/permission denied */
 + SCPI_ERR_RANGE = 6, /* Value out of range */
 + SCPI_ERR_TIMEOUT = 7, /* Timeout has occurred */
 + SCPI_ERR_NOMEM = 8, /* Invalid

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-28 Thread Sudeep Holla 



On 28/04/15 08:36, Paul Bolle wrote:

Just one nit: a license mismatch.

On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:

--- /dev/null
+++ b/drivers/mailbox/scpi_protocol.c



+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program. If not, see http://www.gnu.org/licenses/.


This states the license is GPL v2.


+MODULE_LICENSE(GPL);


And, according to include/linux/module.h, this states the license is GPL
v2 or later. So I think either the comment at the top of this file or
the license ident used in the MODULE_LICENSE() macro should be changed.

Likewise for 2/4 and 4/4.


Thanks for pointing this out. Will fix in the next version.

Regards,
Sudeep
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mailbox: add support for System Control and Power Interface(SCPI) protocol

2015-04-28 Thread Paul Bolle 
Just one nit: a license mismatch.

On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
 --- /dev/null
 +++ b/drivers/mailbox/scpi_protocol.c

 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms and conditions of the GNU General Public License,
 + * version 2, as published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope it will be useful, but WITHOUT
 + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
 + * more details.
 + *
 + * You should have received a copy of the GNU General Public License along
 + * with this program. If not, see http://www.gnu.org/licenses/.

This states the license is GPL v2.

 +MODULE_LICENSE(GPL);

And, according to include/linux/module.h, this states the license is GPL
v2 or later. So I think either the comment at the top of this file or
the license ident used in the MODULE_LICENSE() macro should be changed.

Likewise for 2/4 and 4/4.

Thanks,


Paul Bolle

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/