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

Groups > linux.kernel > #1203848 > unrolled thread

Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=> device tree] defines

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

  Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=>  device tree] defines Jassi Brar <jassisinghbrar@gmail.com> - 2015-08-10 09:00 +0200
    Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=>  device tree] defines Lee Jones <lee.jones@linaro.org> - 2015-08-10 10:30 +0200
      Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=>  device tree] defines Jassi Brar <jassisinghbrar@gmail.com> - 2015-08-10 10:50 +0200
        Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=>  device tree] defines Lee Jones <lee.jones@linaro.org> - 2015-08-12 11:00 +0200
          Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=>  device tree] defines Jassi Brar <jassisinghbrar@gmail.com> - 2015-08-12 12:20 +0200
            Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=>  device tree] defines Lee Jones <lee.jones@linaro.org> - 2015-08-12 12:50 +0200

#1203848 — Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=> device tree] defines

On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> This header is currently only used for defines pertaining to data
> direction i.e. Rx, Tx or Loopback.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>  create mode 100644 include/dt-bindings/mailbox/mailbox.h
>
> diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
> new file mode 100644
> index 0000000..82e929a
> --- /dev/null
> +++ b/include/dt-bindings/mailbox/mailbox.h
> @@ -0,0 +1,14 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
> +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
> +
> +#define MBOX_TX                0x1
> +#define MBOX_RX                0x2
> +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
> +
Not sure I understand 'loopback'.  Does it mean h/w has some
'loopback' mode for testing purposes? Or it simply means the
controller can send as well as receive messages?

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

[toc] | [next] | [standalone]

#1203889

On Mon, 10 Aug 2015, Jassi Brar wrote:
> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > This header is currently only used for defines pertaining to data
> > direction i.e. Rx, Tx or Loopback.
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >  create mode 100644 include/dt-bindings/mailbox/mailbox.h
> >
> > diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
> > new file mode 100644
> > index 0000000..82e929a
> > --- /dev/null
> > +++ b/include/dt-bindings/mailbox/mailbox.h
> > @@ -0,0 +1,14 @@
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
> > +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
> > +
> > +#define MBOX_TX                0x1
> > +#define MBOX_RX                0x2
> > +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
> > +
> Not sure I understand 'loopback'.  Does it mean h/w has some
> 'loopback' mode for testing purposes? Or it simply means the
> controller can send as well as receive messages?

'loopback' allows firmware to conduct some early function tests.
However, channels are simplex, so we provide protection against
multiple allocation of single channel.  By allocating a LOOPBACK
channel we over-ride this protection and allow a single channel to be
allocated twice, once for Rx and the other for Tx.

This mode was born out of a _real_ use-case.  Some of ST's firmware
running on products actually uses the loopback feature.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1203900

On Mon, Aug 10, 2015 at 1:54 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Mon, 10 Aug 2015, Jassi Brar wrote:
>> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> > This header is currently only used for defines pertaining to data
>> > direction i.e. Rx, Tx or Loopback.
>> >
>> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> > ---
>> >  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
>> >  1 file changed, 14 insertions(+)
>> >  create mode 100644 include/dt-bindings/mailbox/mailbox.h
>> >
>> > diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
>> > new file mode 100644
>> > index 0000000..82e929a
>> > --- /dev/null
>> > +++ b/include/dt-bindings/mailbox/mailbox.h
>> > @@ -0,0 +1,14 @@
>> > +/*
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2 as
>> > + * published by the Free Software Foundation.
>> > + */
>> > +
>> > +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
>> > +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
>> > +
>> > +#define MBOX_TX                0x1
>> > +#define MBOX_RX                0x2
>> > +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
>> > +
>> Not sure I understand 'loopback'.  Does it mean h/w has some
>> 'loopback' mode for testing purposes? Or it simply means the
>> controller can send as well as receive messages?
>
> 'loopback' allows firmware to conduct some early function tests.
> However, channels are simplex, so we provide protection against
> multiple allocation of single channel.  By allocating a LOOPBACK
> channel we over-ride this protection and allow a single channel to be
> allocated twice, once for Rx and the other for Tx.
>
So basically hardware is half-duplex, not simplex. I think maybe you
should simply allow for RX and TX always. It should work. Just
handover any received data before send_data (reflecting the h/w
limitation). That way you don't need any such special flag.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1205717

On Mon, 10 Aug 2015, Jassi Brar wrote:

> On Mon, Aug 10, 2015 at 1:54 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Mon, 10 Aug 2015, Jassi Brar wrote:
> >> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > This header is currently only used for defines pertaining to data
> >> > direction i.e. Rx, Tx or Loopback.
> >> >
> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >> > ---
> >> >  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
> >> >  1 file changed, 14 insertions(+)
> >> >  create mode 100644 include/dt-bindings/mailbox/mailbox.h
> >> >
> >> > diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
> >> > new file mode 100644
> >> > index 0000000..82e929a
> >> > --- /dev/null
> >> > +++ b/include/dt-bindings/mailbox/mailbox.h
> >> > @@ -0,0 +1,14 @@
> >> > +/*
> >> > + * This program is free software; you can redistribute it and/or modify
> >> > + * it under the terms of the GNU General Public License version 2 as
> >> > + * published by the Free Software Foundation.
> >> > + */
> >> > +
> >> > +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
> >> > +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
> >> > +
> >> > +#define MBOX_TX                0x1
> >> > +#define MBOX_RX                0x2
> >> > +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
> >> > +
> >> Not sure I understand 'loopback'.  Does it mean h/w has some
> >> 'loopback' mode for testing purposes? Or it simply means the
> >> controller can send as well as receive messages?
> >
> > 'loopback' allows firmware to conduct some early function tests.
> > However, channels are simplex, so we provide protection against
> > multiple allocation of single channel.  By allocating a LOOPBACK
> > channel we over-ride this protection and allow a single channel to be
> > allocated twice, once for Rx and the other for Tx.
> >
> So basically hardware is half-duplex, not simplex. I think maybe you
> should simply allow for RX and TX always. It should work. Just
> handover any received data before send_data (reflecting the h/w
> limitation). That way you don't need any such special flag.

Unfortunately no, that's not correct.  Only Mailbox 0 is half-duplex.
The others are simplex (Rx only).  Ideally I'd like to keep the
LOOPBACK flag, as it's easier to figure out if what someone is
attempting to do is actually valid.  It is only ever used for
'loopback' testing anyway, so it's a perfectly reasonable
description of the channel configuration.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1205888

On Wed, Aug 12, 2015 at 2:23 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Mon, 10 Aug 2015, Jassi Brar wrote:
>
>> On Mon, Aug 10, 2015 at 1:54 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> > On Mon, 10 Aug 2015, Jassi Brar wrote:
>> >> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> > This header is currently only used for defines pertaining to data
>> >> > direction i.e. Rx, Tx or Loopback.
>> >> >
>> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> >> > ---
>> >> >  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
>> >> >  1 file changed, 14 insertions(+)
>> >> >  create mode 100644 include/dt-bindings/mailbox/mailbox.h
>> >> >
>> >> > diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
>> >> > new file mode 100644
>> >> > index 0000000..82e929a
>> >> > --- /dev/null
>> >> > +++ b/include/dt-bindings/mailbox/mailbox.h
>> >> > @@ -0,0 +1,14 @@
>> >> > +/*
>> >> > + * This program is free software; you can redistribute it and/or modify
>> >> > + * it under the terms of the GNU General Public License version 2 as
>> >> > + * published by the Free Software Foundation.
>> >> > + */
>> >> > +
>> >> > +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
>> >> > +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
>> >> > +
>> >> > +#define MBOX_TX                0x1
>> >> > +#define MBOX_RX                0x2
>> >> > +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
>> >> > +
>> >> Not sure I understand 'loopback'.  Does it mean h/w has some
>> >> 'loopback' mode for testing purposes? Or it simply means the
>> >> controller can send as well as receive messages?
>> >
>> > 'loopback' allows firmware to conduct some early function tests.
>> > However, channels are simplex, so we provide protection against
>> > multiple allocation of single channel.  By allocating a LOOPBACK
>> > channel we over-ride this protection and allow a single channel to be
>> > allocated twice, once for Rx and the other for Tx.
>> >
>> So basically hardware is half-duplex, not simplex. I think maybe you
>> should simply allow for RX and TX always. It should work. Just
>> handover any received data before send_data (reflecting the h/w
>> limitation). That way you don't need any such special flag.
>
> Unfortunately no, that's not correct.  Only Mailbox 0 is half-duplex.
> The others are simplex (Rx only).
>
Assuming that is indeed the case (though code and comments suggest
otherwise), it is still not a matter of choice for clients to 'make' a
channel RX or TX or RXTX. That is the property/constraint of the
controller and the controller driver should simply check for channel
ID to be zero in send_data() and return error if its non-zero.

>  Ideally I'd like to keep the
> LOOPBACK flag, as it's easier to figure out if what someone is
> attempting to do is actually valid.
>
I am not for such paranoia. Provider drivers is not the place to check
for valid user data.
The controller driver should simply reject send_data() request on any
mailbox > 0 while the consumer driver should scream for attention
because that's where the problem is.

Please note, what I suggest will only make the code simpler while not
breaking anything.

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

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

#1205950

On Wed, 12 Aug 2015, Jassi Brar wrote:

> On Wed, Aug 12, 2015 at 2:23 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Mon, 10 Aug 2015, Jassi Brar wrote:
> >
> >> On Mon, Aug 10, 2015 at 1:54 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > On Mon, 10 Aug 2015, Jassi Brar wrote:
> >> >> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> >> > This header is currently only used for defines pertaining to data
> >> >> > direction i.e. Rx, Tx or Loopback.
> >> >> >
> >> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >> >> > ---
> >> >> >  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
> >> >> >  1 file changed, 14 insertions(+)
> >> >> >  create mode 100644 include/dt-bindings/mailbox/mailbox.h
> >> >> >
> >> >> > diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
> >> >> > new file mode 100644
> >> >> > index 0000000..82e929a
> >> >> > --- /dev/null
> >> >> > +++ b/include/dt-bindings/mailbox/mailbox.h
> >> >> > @@ -0,0 +1,14 @@
> >> >> > +/*
> >> >> > + * This program is free software; you can redistribute it and/or modify
> >> >> > + * it under the terms of the GNU General Public License version 2 as
> >> >> > + * published by the Free Software Foundation.
> >> >> > + */
> >> >> > +
> >> >> > +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
> >> >> > +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
> >> >> > +
> >> >> > +#define MBOX_TX                0x1
> >> >> > +#define MBOX_RX                0x2
> >> >> > +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
> >> >> > +
> >> >> Not sure I understand 'loopback'.  Does it mean h/w has some
> >> >> 'loopback' mode for testing purposes? Or it simply means the
> >> >> controller can send as well as receive messages?
> >> >
> >> > 'loopback' allows firmware to conduct some early function tests.
> >> > However, channels are simplex, so we provide protection against
> >> > multiple allocation of single channel.  By allocating a LOOPBACK
> >> > channel we over-ride this protection and allow a single channel to be
> >> > allocated twice, once for Rx and the other for Tx.
> >> >
> >> So basically hardware is half-duplex, not simplex. I think maybe you
> >> should simply allow for RX and TX always. It should work. Just
> >> handover any received data before send_data (reflecting the h/w
> >> limitation). That way you don't need any such special flag.
> >
> > Unfortunately no, that's not correct.  Only Mailbox 0 is half-duplex.
> > The others are simplex (Rx only).
> >
> Assuming that is indeed the case (though code and comments suggest
> otherwise),

Sorry, that should have read "Tx only".  Only the A9 (Mbox 0) can Rx.

> it is still not a matter of choice for clients to 'make' a
> channel RX or TX or RXTX. That is the property/constraint of the
> controller and the controller driver should simply check for channel
> ID to be zero in send_data() and return error if its non-zero.

The client is not 'making' the channel Rx or Tx.  It's requesting a
specific Rx or Tx channel.  It's the controller's responsibility to
tell the client if the configuration it has requested is invalid or
already in use.

You're suggesting I remove configuration checking during
mbox_request_channel(), which is not a good suggestion.  That will
fool the client into thinking it has successfully obtained a channel
with the configuration it desires, only to fail later when it attempts
to actually send a message.  Either that, or it will be waiting
forever for a message that it will never receive because it has
selected a Tx only channel to receive from.

That doesn't sound reasonable to me.

> >  Ideally I'd like to keep the
> > LOOPBACK flag, as it's easier to figure out if what someone is
> > attempting to do is actually valid.
> >
> I am not for such paranoia. Provider drivers is not the place to check
> for valid user data.

No one is checking data here.  We are checking channel confirmation,
which is absolutely a controller's responsibility.

> The controller driver should simply reject send_data() request on any
> mailbox > 0 while the consumer driver should scream for attention
> because that's where the problem is.

That would make debugging very difficult, due to the fact that we've
already insinuated that the client had successfully received the
channel/configuration it desired, when in actual fact, the provider of
that channel would know full well that it was actually invalid.

> Please note, what I suggest will only make the code simpler while not
> breaking anything.

Simpler yes.  We could remove all error checking and the driver would
be simpler still, but the fact remains that it would not be doing its
job and debugging would become neigh impossible.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web