Re: [PATCH v7 2/7] mailbox: arm_mhu: add driver for ARM MHU controller

[Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Wed, Mar 18, 2015 at 3:27 PM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Vincent,
>
> I see that this driver is queued but most of my earlier comments were
> not addressed.
>
Some of your comments have been addressed as changes and some as replies.

> *No ACK from DT binding maintainers too*.
>
Yeah it would have been great to have some ack. It has been many
months now, but I see its not uncommon for bindings go in via
maintainers. I assume folks find it just normal.


>> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>> @@ -0,0 +1,43 @@
>> +ARM MHU Mailbox Driver
>> +======================
>> +
>> +The ARM's Message-Handling-Unit (MHU) is a mailbox controller that has
>> +3 independent channels/links to communicate with remote processor(s).
>> + MHU links are hardwired on a platform. A link raises interrupt for any
>
>
> As I had mentioned before it's just one implementation, the IP had
> provision bi-directional interrupts and the binding *has to consider*
> that instead of having to workaround that later.
>
I will reply in yet another way.... the spec I have don't mention that
(and it makes sense) and your platform doesn't do that either. I would
have had to do something about it, had there been such a weird setup,
but there's none known.

 So let us please avoid getting into bikeshedding discussion yet
again. Otherwise almost every binding is broken because there could
always be some platform that does weird things with irqs.

>> +The last channel is specified to be a 'Secure' resource, hence can't be
>> +used by Linux running NS.
>
> Correct so drop the support for secure channel until the first user
> comes up.
>
In your last post you said:
  "Sorry my statement was not clear, I wanted to ask you to add that info
to the binding."
  Not that it makes sense to say the obvious, I added it just to
address your comment.

> I am worried it might generate abort if someone tries to
> access it in ARM64 which always runs in non-secure.
>
Abort should have happened years ago if someone _tries_ to access a
secure resource from non-secure mode ;)
How do you protect such people from "trying to" dereference NULL
pointers?  (you made me repeat Nth time).

>> +
>> +static struct amba_id mhu_ids[] = {
>> +       {
>> +               .id     = 0x1bb098,
>
>
> As I mentioned couple of times this is broken. Please refer my earlier mail
> for details. You are skipping a field to compare which incorrect.
> You are just trying to get it working with existing AMBA driver without any
> changes matching the IDs partially.
>
I did not miss your last mail. Let me try to reply in even simpler
terms... PID[0,3] identification is fine grained enough. If/when we
have some platform with non-ARM MHU, we could catch that in this
driver. Most likely even then the reg-map won't change drastically, if
at all. So we could ignore the PID4, otherwise we check PID4 in this
driver and adapt the behavior.

-Jassi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html