* Re: 答复: Re: [PATCH] tty: max3100: Fix oops while 'cat/proc/tty/driver/ttyMAX'
From: Jiri Slaby @ 2018-09-14 11:28 UTC (permalink / raw)
To: chen.lin5; +Cc: gregkh, linux-kernel, linux-serial
In-Reply-To: <201809131432456792094@zte.com.cn>
On 09/13/2018, 08:32 AM, chen.lin5@zte.com.cn wrote:
>
>
> but 'get_mctrl' is already protected by the upper layer by spin lock,
> so, will the races happen?
>
>
> for example: in /drivers/tty/serial/serial_core.c
>
> spin_lock_irq(&uport->lock);
>
> result |= uport->ops->get_mctrl(uport);
>
> spin_unlock_irq(&uport->lock);
Right, but creation and destroy of the workqueue is not locked as far as
I can see. Am I missing something?
thanks,.
--
js
suse labs
^ permalink raw reply
* [GIT PULL] TTY/Serial fixes for 4.19-rc4
From: Greg KH @ 2018-09-14 11:26 UTC (permalink / raw)
To: Linus Torvalds, Jiri Slaby
Cc: Stephen Rothwell, Andrew Morton, linux-kernel, linux-serial
The following changes since commit 11da3a7f84f19c26da6f86af878298694ede0804:
Linux 4.19-rc3 (2018-09-09 17:26:43 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tags/tty-4.19-rc4
for you to fetch changes up to 7f2bf7840b74a160f908db83bc8829f8de10629b:
tty: hvc: hvc_write() fix break condition (2018-09-10 18:04:31 +0200)
----------------------------------------------------------------
TTY fixes for 4.19-rc4
Here are 3 small HVC tty driver fixes to resolve a reported regression
from 4.19-rc1.
All of these have been in linux-next for a while with no reported
issues.
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
----------------------------------------------------------------
Nicholas Piggin (3):
tty: hvc: hvc_poll() fix read loop hang
tty: hvc: hvc_poll() fix read loop batching
tty: hvc: hvc_write() fix break condition
drivers/tty/hvc/hvc_console.c | 38 +++++++++++++++++++++++++++++++++-----
1 file changed, 33 insertions(+), 5 deletions(-)
^ permalink raw reply
* [PATCH] serial: cpm_uart: return immediately from console poll
From: Christophe Leroy @ 2018-09-14 10:32 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: linux-kernel, linuxppc-dev, linux-serial, stable, Jason Wessel
kgdb expects poll function to return immediately and
returning NO_POLL_CHAR when no character is available.
Fixes: f5316b4aea024 ("kgdb,8250,pl011: Return immediately from console poll")
Cc: Jason Wessel <jason.wessel@windriver.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
drivers/tty/serial/cpm_uart/cpm_uart_core.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_core.c b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
index cd3f3fc4e0a5..280acc4dfa90 100644
--- a/drivers/tty/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
@@ -1093,8 +1093,8 @@ static int poll_wait_key(char *obuf, struct uart_cpm_port *pinfo)
/* Get the address of the host memory buffer.
*/
bdp = pinfo->rx_cur;
- while (bdp->cbd_sc & BD_SC_EMPTY)
- ;
+ if (bdp->cbd_sc & BD_SC_EMPTY)
+ return NO_POLL_CHAR;
/* If the buffer address is in the CPM DPRAM, don't
* convert it.
@@ -1129,7 +1129,11 @@ static int cpm_get_poll_char(struct uart_port *port)
poll_chars = 0;
}
if (poll_chars <= 0) {
- poll_chars = poll_wait_key(poll_buf, pinfo);
+ int ret = poll_wait_key(poll_buf, pinfo);
+
+ if (ret == NO_POLL_CHAR)
+ return ret;
+ poll_chars = ret;
pollp = poll_buf;
}
poll_chars--;
--
2.13.3
^ permalink raw reply related
* Re: [PATCH v8 0/4] gpiolib: speed up GPIO array processing
From: Linus Walleij @ 2018-09-13 9:22 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Jonathan Corbet, Miguel Ojeda Sandonis, Peter Korsgaard,
Peter Rosin, Ulf Hansson, Andrew Lunn, Florian Fainelli,
David S. Miller, Dominik Brodowski, Greg KH, kishon,
Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald, Jiri Slaby, Willy Tarreau,
Geert Uytterhoeven, Sebastien Bourdelin
In-Reply-To: <20180905215008.1649-1-jmkrzyszt@gmail.com>
On Wed, Sep 5, 2018 at 11:49 PM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> The goal is to boost performance of get/set array functions while
> processing GPIO arrays which represent pins of a signle chip in
> hardware order. If resulting performance is close to PIO, GPIO API
> can be used for data I/O without much loss of speed.
I applied the v8 to an immutable branch and pushed to kernelorg
so the build servers can churn it a bit, and if it works fine
then we can merge this into the devel branch and also set up
that as something other subsystems can pull in if they need it.
I'm really excited to merge this!
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] tty: max3100: Fix oops while 'cat /proc/tty/driver/ttyMAX'
From: Jiri Slaby @ 2018-09-13 5:33 UTC (permalink / raw)
To: chen.lin5, gregkh; +Cc: linux-kernel, linux-serial
In-Reply-To: <201809131038370132780@zte.com.cn>
On 09/13/2018, 04:38 AM, chen.lin5@zte.com.cn wrote:
> Before wq 's->workqueue' be initialized in function 'max3100_startup',
>
> 'cat /proc/tty/driver/ttyMAX' will cause oops.
>
>
> Oops: Kernel access of bad area, sig: 11 [#1]
>
> ...
>
> NIP [c0049070] __queue_work+0x24/0x268
>
> LR [c0049308] queue_work_on+0x54/0x70
>
> Call Trace:
>
> [db44dba0] [c0049308] queue_work_on+0x54/0x70
>
> [db44dbb0] [c036befc] max3100_get_mctrl+0x18/0x40
>
> [db44dbc0] [c03614a4] uart_proc_show+0xfc/0x448
>
> [db44dc40] [c0111aa8] seq_read+0x198/0x514
>
> [db44dc90] [c01436e4] proc_reg_read+0x58/0x94
>
> [db44dca0] [c00ee804] do_iter_read+0x190/0x1e0
>
> [db44dcc0] [c00f0040] vfs_readv+0x58/0x7c
>
> [db44dd40] [c011a2f4] default_file_splice_read+0x160/0x27c
>
> [db44de30] [c0119f74] splice_direct_to_actor+0xec/0x23c
>
> [db44de80] [c011a158] do_splice_direct+0x94/0xd0
>
> [db44dec0] [c00eeee8] do_sendfile+0x1e0/0x380
>
> [db44df10] [c00f0424] sys_sendfile64+0xc0/0xd4
>
> [db44df40] [c000e260] ret_from_syscall+0x0/0x3c
>
>
> Signed-off-by: Chen Lin <chen.lin5@zte.com.cn>
>
> ---
>
> drivers/tty/serial/max3100.c | 3 ++-
>
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
>
> index 371569a..0429c2d 100644
>
> --- a/drivers/tty/serial/max3100.c
>
> +++ b/drivers/tty/serial/max3100.c
>
> @@ -391,7 +391,8 @@ static unsigned int max3100_get_mctrl(struct
> uart_port *port)
>
> dev_dbg(&s->spi->dev, "%s\n", __func__);
>
>
> /* may not be truly up-to-date */
>
> - max3100_dowork(s);
>
> + if (s->workqueue)
>
> + max3100_dowork(s);
This does not look right in the sense of races (shutdown can kill the
workqueue right after the if. Or even before the if, as s->workqueue =
NULL is only after destroy_workqueue in max3100_shutdown).
If get_mctrl needs the queue, it shall not be created and destroyed ad hoc.
> /* always assert DCD and DSR since these lines are not wired */
>
> return (s->cts ? TIOCM_CTS : 0) | TIOCM_DSR | TIOCM_CAR;
>
> }
thanks,
--
js
suse labs
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Radu Pirea @ 2018-09-12 19:36 UTC (permalink / raw)
To: Lee Jones, Alexandre Belloni
Cc: Geert Uytterhoeven, Rob Herring, Mark Rutland, Nicolas Ferre,
Greg KH, Mark Brown, Jiri Slaby, Richard Genoud, David S. Miller,
Mauro Carvalho Chehab, Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
In-Reply-To: <20180912131248.GA21544@dell>
On Wed, 2018-09-12 at 14:12 +0100, Lee Jones wrote:
> On Wed, 12 Sep 2018, Alexandre Belloni wrote:
>
> > On 12/09/2018 12:43:52+0100, Lee Jones wrote:
> > > > > But ... we can't have it both ways. *Either* it's a true
> > > > > MFD, in
> > > > > which case it can/should have 2 separate compatible strings
> > > > > which can
> > > > > be specified directly from the DT. *Or* it's not an MFD. In
> > > > > the
> > > > > latter case, which I think we're all agreeing on (else we'd
> > > > > have 2
> > > > > compatible strings), MFD is not the place to handle this (my
> > > > > original
> > > > > point).
> > > > >
> > > >
> > > > If that is what bothers you, then let's move it out of mfd.
> > >
> > > As I've already mentioned. I don't just want it moved out of MFD
> > > and
> > > shoved somewhere else. My aim is to fix this properly.
> > >
> >
> > If it is out of MFD, then I'm not sure why you would care too much
> > about
> > it as you won't be maintaining that code. And I still this what was
> > done
> > was correct but I'm open to test what you suggest.
>
> I care for the kernel in general, not just the areas I'm responsible
> for. I guess I'm just that kinda guy! ;)
Well, Lee, like you, I think this driver should not be a MFD driver,
but Alex has a good point of view.
>
> > > > > So ... this is a USART device which can do SPI, right?
> > > > >
> > > > > My current thinking is that; as this is a USART device first
> > > > > &
> > > > > foremost, the USART should be probed in the first instance
> > > > > regardless,
> > > > > then if SPI mode is specified it (the USART driver) registers
> > > > > the SPI
> > > > > platform driver (as MFD does currently) and exits gracefully,
> > > > > allowing
> > > > > the SPI driver to take over.
> > > > >
> > > > > Spanner in the works: is it physically possible to change the
> > > > > mode at
> > > > > run-time? :s
> > > >
> > > > Yes it is possible but on Linux that will not happen without
> > > > probing
> > > > the drivers again.
> > >
> > > Not sure I understand what you mean.
> >
> > I was just commenting on changing the mode at runtime.
>
> Oh I see. My question was relating to whether the H/W is physically
> capable of changing modes on-the-fly, rather than how Linux would
> handle that. If this is something we'd wish to support, then it
> would
> have to be a single driver, which is why I was asking. By separating
> the drivers this way, we are blocking that as a
> possibility. Although
> I guess the OP has already thought about that and made the decision
> not to support it.
Is possible to change modes on-the-fly, but you have no reason to do
that. On the PCB you will have a SPI slave or a serial console :)
Anyway, the current form of the driver, and through this I want to say
"this ugly hack", allows the user to switch from serial to SPI mode by
adding only one property to the device tree node of USART. If the
driver were in his first form, a simple SPI driver, how you will make a
dtsi file for an IP like this? You will add two nodes for the same IP
in dtsi and will take care to enable correct node in dts?
I think this driver is only a tradeoff between having an ugly hack in
kernel or having an messy device tree.
>
> > > I'm suggesting that you use the same platform_* interfaces MFD
> > > uses to
> > > register the SPI driver if SPI mode has been selected. Only do
> > > so
> > > from the appropriate driver i.e. USART.
> >
> > Yeah, I understood that but I didn't comment because I'm not sure
> > this
> > will work yet.
>
> Other drivers already do this.
Can you give me an example please?
I am open to suggestions.
Sorry for that acked-by. There was a lot of "reviewed-by", "acked-by",
etc in a single version and I messed up :).
^ permalink raw reply
* Re: [PATCH 00/25] Change tty_port(standard)_install's return type
From: Alan Cox @ 2018-09-12 14:41 UTC (permalink / raw)
To: Jaejoong Kim
Cc: linux-um, netdev, linux-mmc, linux-s390, devel, greybus-dev,
linuxppc-dev, linux-serial, sparclinux, linux-usb,
linux-bluetooth, linux-kernel, Greg Kroah-Hartman, Jiri Slaby
In-Reply-To: <1536029091-4426-1-git-send-email-climbbb.kim@gmail.com>
On Tue, 4 Sep 2018 11:44:26 +0900
Jaejoong Kim <climbbb.kim@gmail.com> wrote:
> Many drivers with tty use the tty_stand_install(). But, there is no
> need to handle the error, since it always returns 0.
And what happens if another change means it can fail again. It's just a
property of the current implementation that it can't. It used to fail.
This seems to be a ton of unneccessary churn that will end up just having
to be reversed again some day in the future.
Alan
^ permalink raw reply
* [PATCH 2/2] sc16is7xx: Fix for "Unexpected interrupt: 8"
From: Phil Elwell @ 2018-09-12 14:31 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Alexander Graf, Stefan Wahren
Cc: Phil Elwell
In-Reply-To: <1536762716-30673-1-git-send-email-phil@raspberrypi.org>
The SC16IS752 has an Enhanced Feature Register which is aliased at the
same address as the Interrupt Identification Register; accessing it
requires that a magic value is written to the Line Configuration
Register. If an interrupt is raised while the EFR is mapped in then
the ISR won't be able to access the IIR, leading to the "Unexpected
interrupt" error messages.
Avoid the problem by claiming a mutex around accesses to the EFR
register, also claiming the mutex in the interrupt handler work
item (this is equivalent to disabling interrupts to interlock against
a non-threaded interrupt handler).
See: https://github.com/raspberrypi/linux/issues/2529
Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
drivers/tty/serial/sc16is7xx.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 47b4115..2680986 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -328,6 +328,7 @@ struct sc16is7xx_port {
struct kthread_worker kworker;
struct task_struct *kworker_task;
struct kthread_work irq_work;
+ struct mutex efr_lock;
struct sc16is7xx_one p[0];
};
@@ -499,6 +500,21 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
div /= 4;
}
+ /* In an amazing feat of design, the Enhanced Features Register shares
+ * the address of the Interrupt Identification Register, and is
+ * switched in by writing a magic value (0xbf) to the Line Control
+ * Register. Any interrupt firing during this time will see the EFR
+ * where it expects the IIR to be, leading to "Unexpected interrupt"
+ * messages.
+ *
+ * Prevent this possibility by claiming a mutex while accessing the
+ * EFR, and claiming the same mutex from within the interrupt handler.
+ * This is similar to disabling the interrupt, but that doesn't work
+ * because the bulk of the interrupt processing is run as a workqueue
+ * job in thread context.
+ */
+ mutex_lock(&s->efr_lock);
+
lcr = sc16is7xx_port_read(port, SC16IS7XX_LCR_REG);
/* Open the LCR divisors for configuration */
@@ -514,6 +530,8 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
/* Put LCR back to the normal mode */
sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
+ mutex_unlock(&s->efr_lock);
+
sc16is7xx_port_update(port, SC16IS7XX_MCR_REG,
SC16IS7XX_MCR_CLKSEL_BIT,
prescaler);
@@ -696,6 +714,8 @@ static void sc16is7xx_ist(struct kthread_work *ws)
{
struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
+ mutex_lock(&s->efr_lock);
+
while (1) {
bool keep_polling = false;
int i;
@@ -705,6 +725,8 @@ static void sc16is7xx_ist(struct kthread_work *ws)
if (!keep_polling)
break;
}
+
+ mutex_unlock(&s->efr_lock);
}
static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
@@ -899,6 +921,9 @@ static void sc16is7xx_set_termios(struct uart_port *port,
if (!(termios->c_cflag & CREAD))
port->ignore_status_mask |= SC16IS7XX_LSR_BRK_ERROR_MASK;
+ /* As above, claim the mutex while accessing the EFR. */
+ mutex_lock(&s->efr_lock);
+
sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
SC16IS7XX_LCR_CONF_MODE_B);
@@ -920,6 +945,8 @@ static void sc16is7xx_set_termios(struct uart_port *port,
/* Update LCR register */
sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
+ mutex_unlock(&s->efr_lock);
+
/* Get baud rate generator configuration */
baud = uart_get_baud_rate(port, termios, old,
port->uartclk / 16 / 4 / 0xffff,
@@ -1185,6 +1212,7 @@ static int sc16is7xx_probe(struct device *dev,
s->regmap = regmap;
s->devtype = devtype;
dev_set_drvdata(dev, s);
+ mutex_init(&s->efr_lock);
kthread_init_worker(&s->kworker);
kthread_init_work(&s->irq_work, sc16is7xx_ist);
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] sc16is7xx: Fix for multi-channel stall
From: Phil Elwell @ 2018-09-12 14:31 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Alexander Graf, Stefan Wahren
Cc: Phil Elwell
In-Reply-To: <1536762716-30673-1-git-send-email-phil@raspberrypi.org>
The SC16IS752 is a dual-channel device. The two channels are largely
independent, but the IRQ signals are wired together as an open-drain,
active low signal which will be driven low while either of the
channels requires attention, which can be for significant periods of
time until operations complete and the interrupt can be acknowledged.
In that respect it is should be treated as a true level-sensitive IRQ.
The kernel, however, needs to be able to exit interrupt context in
order to use I2C or SPI to access the device registers (which may
involve sleeping). Therefore the interrupt needs to be masked out or
paused in some way.
The usual way to manage sleeping from within an interrupt handler
is to use a threaded interrupt handler - a regular interrupt routine
does the minimum amount of work needed to triage the interrupt before
waking the interrupt service thread. If the threaded IRQ is marked as
IRQF_ONESHOT the kernel will automatically mask out the interrupt
until the thread runs to completion. The sc16is7xx driver used to
use a threaded IRQ, but a patch switched to using a kthread_worker
in order to set realtime priorities on the handler thread and for
other optimisations. The end result is non-threaded IRQ that
schedules some work then returns IRQ_HANDLED, making the kernel
think that all IRQ processing has completed.
The work-around to prevent a constant stream of interrupts is to
mark the interrupt as edge-sensitive rather than level-sensitive,
but interpreting an active-low source as a falling-edge source
requires care to prevent a total cessation of interrupts. Whereas
an edge-triggering source will generate a new edge for every interrupt
condition a level-triggering source will keep the signal at the
interrupting level until it no longer requires attention; in other
words, the host won't see another edge until all interrupt conditions
are cleared. It is therefore vital that the interrupt handler does not
exit with an outstanding interrupt condition, otherwise the kernel
will not receive another interrupt unless some other operation causes
the interrupt state on the device to be cleared.
The existing sc16is7xx driver has a very simple interrupt "thread"
(kthread_work job) that processes interrupts on each channel in turn
until there are no more. If both channels are active and the first
channel starts interrupting while the handler for the second channel
is running then it will not be detected and an IRQ stall ensues. This
could be handled easily if there was a shared IRQ status register, or
a convenient way to determine if the IRQ had been deasserted for any
length of time, but both appear to be lacking.
Avoid this problem (or at least make it much less likely to happen)
by reducing the granularity of per-channel interrupt processing
to one condition per iteration, only exiting the overall loop when
both channels are no longer interrupting.
Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
drivers/tty/serial/sc16is7xx.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 243c960..47b4115 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -657,7 +657,7 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
uart_write_wakeup(port);
}
-static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
+static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
{
struct uart_port *port = &s->p[portno].port;
@@ -666,7 +666,7 @@ static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
if (iir & SC16IS7XX_IIR_NO_INT_BIT)
- break;
+ return false;
iir &= SC16IS7XX_IIR_ID_MASK;
@@ -688,16 +688,23 @@ static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
port->line, iir);
break;
}
- } while (1);
+ } while (0);
+ return true;
}
static void sc16is7xx_ist(struct kthread_work *ws)
{
struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
- int i;
- for (i = 0; i < s->devtype->nr_uart; ++i)
- sc16is7xx_port_irq(s, i);
+ while (1) {
+ bool keep_polling = false;
+ int i;
+
+ for (i = 0; i < s->devtype->nr_uart; ++i)
+ keep_polling |= sc16is7xx_port_irq(s, i);
+ if (!keep_polling)
+ break;
+ }
}
static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
--
2.7.4
^ permalink raw reply related
* [PATCH 0/2] sc16is7xx interrupt fixes
From: Phil Elwell @ 2018-09-12 14:31 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Alexander Graf, Stefan Wahren
Cc: Phil Elwell
The interrupt handling of the sc16is7xx driver is broken in a number of ways,
as observed by multiple Raspberry Pi users. The attached patches attempt to
address its failings, with apparent success.
The first is a workaround for a side-effect of the switch away from using a
thread IRQ, a change which has necessitated using what is actually a
level-triggered interrupt as if it were edge-triggered. Doing so is fraught
with potential race conditions, but the patch makes them much less likely.
The second is a workaround for a bug in the design of the SC16IS752 which
requires mutual exclusion between the interrupt handler and access to the
Enhanced Features Register.
Phil Elwell (2):
sc16is7xx: Fix for multi-channel stall
sc16is7xx: Fix for "Unexpected interrupt: 8"
drivers/tty/serial/sc16is7xx.c | 50 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 44 insertions(+), 6 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Lee Jones @ 2018-09-12 13:12 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180912121420.GE2760@piout.net>
On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> On 12/09/2018 12:43:52+0100, Lee Jones wrote:
> > > > But ... we can't have it both ways. *Either* it's a true MFD, in
> > > > which case it can/should have 2 separate compatible strings which can
> > > > be specified directly from the DT. *Or* it's not an MFD. In the
> > > > latter case, which I think we're all agreeing on (else we'd have 2
> > > > compatible strings), MFD is not the place to handle this (my original
> > > > point).
> > > >
> > >
> > > If that is what bothers you, then let's move it out of mfd.
> >
> > As I've already mentioned. I don't just want it moved out of MFD and
> > shoved somewhere else. My aim is to fix this properly.
> >
>
> If it is out of MFD, then I'm not sure why you would care too much about
> it as you won't be maintaining that code. And I still this what was done
> was correct but I'm open to test what you suggest.
I care for the kernel in general, not just the areas I'm responsible
for. I guess I'm just that kinda guy! ;)
> > > > So ... this is a USART device which can do SPI, right?
> > > >
> > > > My current thinking is that; as this is a USART device first &
> > > > foremost, the USART should be probed in the first instance regardless,
> > > > then if SPI mode is specified it (the USART driver) registers the SPI
> > > > platform driver (as MFD does currently) and exits gracefully, allowing
> > > > the SPI driver to take over.
> > > >
> > > > Spanner in the works: is it physically possible to change the mode at
> > > > run-time? :s
> > >
> > > Yes it is possible but on Linux that will not happen without probing
> > > the drivers again.
> >
> > Not sure I understand what you mean.
>
> I was just commenting on changing the mode at runtime.
Oh I see. My question was relating to whether the H/W is physically
capable of changing modes on-the-fly, rather than how Linux would
handle that. If this is something we'd wish to support, then it would
have to be a single driver, which is why I was asking. By separating
the drivers this way, we are blocking that as a possibility. Although
I guess the OP has already thought about that and made the decision
not to support it.
> > I'm suggesting that you use the same platform_* interfaces MFD uses to
> > register the SPI driver if SPI mode has been selected. Only do so
> > from the appropriate driver i.e. USART.
>
> Yeah, I understood that but I didn't comment because I'm not sure this
> will work yet.
Other drivers already do this.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Alexandre Belloni @ 2018-09-12 12:14 UTC (permalink / raw)
To: Lee Jones
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180912114352.GT4185@dell>
On 12/09/2018 12:43:52+0100, Lee Jones wrote:
> > > But ... we can't have it both ways. *Either* it's a true MFD, in
> > > which case it can/should have 2 separate compatible strings which can
> > > be specified directly from the DT. *Or* it's not an MFD. In the
> > > latter case, which I think we're all agreeing on (else we'd have 2
> > > compatible strings), MFD is not the place to handle this (my original
> > > point).
> > >
> >
> > If that is what bothers you, then let's move it out of mfd.
>
> As I've already mentioned. I don't just want it moved out of MFD and
> shoved somewhere else. My aim is to fix this properly.
>
If it is out of MFD, then I'm not sure why you would care too much about
it as you won't be maintaining that code. And I still this what was done
was correct but I'm open to test what you suggest.
> > > So ... this is a USART device which can do SPI, right?
> > >
> > > My current thinking is that; as this is a USART device first &
> > > foremost, the USART should be probed in the first instance regardless,
> > > then if SPI mode is specified it (the USART driver) registers the SPI
> > > platform driver (as MFD does currently) and exits gracefully, allowing
> > > the SPI driver to take over.
> > >
> > > Spanner in the works: is it physically possible to change the mode at
> > > run-time? :s
> >
> > Yes it is possible but on Linux that will not happen without probing
> > the drivers again.
>
> Not sure I understand what you mean.
>
I was just commenting on changing the mode at runtime.
> I'm suggesting that you use the same platform_* interfaces MFD uses to
> register the SPI driver if SPI mode has been selected. Only do so
> from the appropriate driver i.e. USART.
>
Yeah, I understood that but I didn't comment because I'm not sure this
will work yet.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Lee Jones @ 2018-09-12 11:43 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180912111757.GC2760@piout.net>
On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> On 12/09/2018 11:54:07+0100, Lee Jones wrote:
> > On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> > > On Wed, Sep 12, 2018 at 10:41 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > > > >
> > > > > > > > USART doc starting p572, registers p621.
> > > > > >
> > > > > > After looking at the datasheet, I don't see any reason why one of the
> > > > > > two drivers can't be selected using different compatible strings.
> > > > >
> > > > > Because there is only one IP and we don't use the device tree to selecet
> > > > > linux specific drivers.
> > > >
> > > > We do it all the time. There are loads of MFDs (def: same IP, with
> > > > different functions) which have separate compatibles for their various
> > > > functions. If you wish this IP to operate as an SPI controller, it
> > > > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > > > then it should have a UART compatible. It's what we do for most of
> > > > the other MFDs in the kernel.
> > >
> > > There is a big difference: MFD functions are(more or less) independent
> > > functions, which can be used at the same time. It makes perfect sense for a
> > > single IP block that has both SPI and UART interfaces, that can be used at
> > > the same time.
> > >
> > > In this case, there is a single piece of hardware that can perform
> > > different functions, but not at the same time. Performing a different
> > > function means configuring the hardware for that function, hence using a
> > > different driver (from a different subsystem).
> >
> > Yes, I can see that PoV.
> >
> > But ... we can't have it both ways. *Either* it's a true MFD, in
> > which case it can/should have 2 separate compatible strings which can
> > be specified directly from the DT. *Or* it's not an MFD. In the
> > latter case, which I think we're all agreeing on (else we'd have 2
> > compatible strings), MFD is not the place to handle this (my original
> > point).
> >
>
> If that is what bothers you, then let's move it out of mfd.
As I've already mentioned. I don't just want it moved out of MFD and
shoved somewhere else. My aim is to fix this properly.
> > So ... this is a USART device which can do SPI, right?
> >
> > My current thinking is that; as this is a USART device first &
> > foremost, the USART should be probed in the first instance regardless,
> > then if SPI mode is specified it (the USART driver) registers the SPI
> > platform driver (as MFD does currently) and exits gracefully, allowing
> > the SPI driver to take over.
> >
> > Spanner in the works: is it physically possible to change the mode at
> > run-time? :s
>
> Yes it is possible but on Linux that will not happen without probing
> the drivers again.
Not sure I understand what you mean.
I'm suggesting that you use the same platform_* interfaces MFD uses to
register the SPI driver if SPI mode has been selected. Only do so
from the appropriate driver i.e. USART.
> I think DT overlays will be the only possible use
> case because on SPI, you'd still have to provide nodes for the connected
> SPI devices.
Since SPI is a function of the USART you should describe is as such
via a child node.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Alexandre Belloni @ 2018-09-12 11:17 UTC (permalink / raw)
To: Lee Jones
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180912105407.GR4185@dell>
On 12/09/2018 11:54:07+0100, Lee Jones wrote:
> On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> > On Wed, Sep 12, 2018 at 10:41 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > > >
> > > > > > > USART doc starting p572, registers p621.
> > > > >
> > > > > After looking at the datasheet, I don't see any reason why one of the
> > > > > two drivers can't be selected using different compatible strings.
> > > >
> > > > Because there is only one IP and we don't use the device tree to selecet
> > > > linux specific drivers.
> > >
> > > We do it all the time. There are loads of MFDs (def: same IP, with
> > > different functions) which have separate compatibles for their various
> > > functions. If you wish this IP to operate as an SPI controller, it
> > > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > > then it should have a UART compatible. It's what we do for most of
> > > the other MFDs in the kernel.
> >
> > There is a big difference: MFD functions are(more or less) independent
> > functions, which can be used at the same time. It makes perfect sense for a
> > single IP block that has both SPI and UART interfaces, that can be used at
> > the same time.
> >
> > In this case, there is a single piece of hardware that can perform
> > different functions, but not at the same time. Performing a different
> > function means configuring the hardware for that function, hence using a
> > different driver (from a different subsystem).
>
> Yes, I can see that PoV.
>
> But ... we can't have it both ways. *Either* it's a true MFD, in
> which case it can/should have 2 separate compatible strings which can
> be specified directly from the DT. *Or* it's not an MFD. In the
> latter case, which I think we're all agreeing on (else we'd have 2
> compatible strings), MFD is not the place to handle this (my original
> point).
>
If that is what bothers you, then let's move it out of mfd.
> So ... this is a USART device which can do SPI, right?
>
> My current thinking is that; as this is a USART device first &
> foremost, the USART should be probed in the first instance regardless,
> then if SPI mode is specified it (the USART driver) registers the SPI
> platform driver (as MFD does currently) and exits gracefully, allowing
> the SPI driver to take over.
>
> Spanner in the works: is it physically possible to change the mode at
> run-time? :s
Yes it is possible but on Linux that will not happen without probing
the drivers again. I think DT overlays will be the only possible use
case because on SPI, you'd still have to provide nodes for the connected
SPI devices.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Lee Jones @ 2018-09-12 10:54 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Alexandre Belloni, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <CAMuHMdWefU0pw=c9TKOa+wGHchpg8JRfE0OnC4+wKDt-qrbH1A@mail.gmail.com>
On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> On Wed, Sep 12, 2018 at 10:41 AM Lee Jones <lee.jones@linaro.org> wrote:
> > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > >
> > > > > > USART doc starting p572, registers p621.
> > > >
> > > > After looking at the datasheet, I don't see any reason why one of the
> > > > two drivers can't be selected using different compatible strings.
> > >
> > > Because there is only one IP and we don't use the device tree to selecet
> > > linux specific drivers.
> >
> > We do it all the time. There are loads of MFDs (def: same IP, with
> > different functions) which have separate compatibles for their various
> > functions. If you wish this IP to operate as an SPI controller, it
> > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > then it should have a UART compatible. It's what we do for most of
> > the other MFDs in the kernel.
>
> There is a big difference: MFD functions are(more or less) independent
> functions, which can be used at the same time. It makes perfect sense for a
> single IP block that has both SPI and UART interfaces, that can be used at
> the same time.
>
> In this case, there is a single piece of hardware that can perform
> different functions, but not at the same time. Performing a different
> function means configuring the hardware for that function, hence using a
> different driver (from a different subsystem).
Yes, I can see that PoV.
But ... we can't have it both ways. *Either* it's a true MFD, in
which case it can/should have 2 separate compatible strings which can
be specified directly from the DT. *Or* it's not an MFD. In the
latter case, which I think we're all agreeing on (else we'd have 2
compatible strings), MFD is not the place to handle this (my original
point).
So ... this is a USART device which can do SPI, right?
My current thinking is that; as this is a USART device first &
foremost, the USART should be probed in the first instance regardless,
then if SPI mode is specified it (the USART driver) registers the SPI
platform driver (as MFD does currently) and exits gracefully, allowing
the SPI driver to take over.
Spanner in the works: is it physically possible to change the mode at
run-time? :s
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Geert Uytterhoeven @ 2018-09-12 9:43 UTC (permalink / raw)
To: Lee Jones
Cc: Alexandre Belloni, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180912084143.GN4185@dell>
Hi Lee,
On Wed, Sep 12, 2018 at 10:41 AM Lee Jones <lee.jones@linaro.org> wrote:
> On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > >
> > > > > USART doc starting p572, registers p621.
> > >
> > > After looking at the datasheet, I don't see any reason why one of the
> > > two drivers can't be selected using different compatible strings.
> >
> > Because there is only one IP and we don't use the device tree to selecet
> > linux specific drivers.
>
> We do it all the time. There are loads of MFDs (def: same IP, with
> different functions) which have separate compatibles for their various
> functions. If you wish this IP to operate as an SPI controller, it
> should have an SPI compatible, if you wish it to operate as a U(S)ART,
> then it should have a UART compatible. It's what we do for most of
> the other MFDs in the kernel.
There is a big difference: MFD functions are(more or less) independent
functions, which can be used at the same time. It makes perfect sense for a
single IP block that has both SPI and UART interfaces, that can be used at
the same time.
In this case, there is a single piece of hardware that can perform
different functions, but not at the same time. Performing a different
function means configuring the hardware for that function, hence using a
different driver (from a different subsystem).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Lee Jones @ 2018-09-12 8:41 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180912073355.GB2557@piout.net>
On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > >
> > > > USART doc starting p572, registers p621.
> >
> > After looking at the datasheet, I don't see any reason why one of the
> > two drivers can't be selected using different compatible strings.
>
> Because there is only one IP and we don't use the device tree to selecet
> linux specific drivers.
We do it all the time. There are loads of MFDs (def: same IP, with
different functions) which have separate compatibles for their various
functions. If you wish this IP to operate as an SPI controller, it
should have an SPI compatible, if you wish it to operate as a U(S)ART,
then it should have a UART compatible. It's what we do for most of
the other MFDs in the kernel.
> If you are not happy having that in MFD, I guess we can move it out
> somewhere else.
My issue isn't pertaining to where the hack lives, it's that there is
a hack in the first place.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Lee Jones @ 2018-09-12 8:36 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180912073056.GA2557@piout.net>
On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> On 11/09/2018 23:43:02+0100, Lee Jones wrote:
> > > I haven't read it, but I believe it's not unlike Renesas SCIF, which is
> > > served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
> > > But the latter is not used from DT, so we haven't experienced (and solved)
> > > the similar issue yet.
> > >
> > > Would it work if the UART driver and SPI driver would match against the
> > > same compatible value, but the UART driver would do in its probe()
> > > function:
> > >
> > > device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> > > if (opmode != AT91_USART_MODE_SERIAL)
> > > return ENODEV;
> > >
> > > while the SPI driver would do:
> > >
> > > device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> > > if (opmode != AT91_USART_MODE_SPI)
> > > return ENODEV;
> > >
> > > ? No MFD driver involved.
> >
> > I haven't looked at the code in a while, but if memory serves I
> > believe platform code gives up once it has found its first match, so
> > by doing this, one of the drivers will never be matched/probed.
> >
> > It's midnight here, so cracking out the datasheet isn't going to
> > happen just now, but it's my current belief that if the IP serves 2
> > very different modes of operation, even if the registers are in a
> > shared space, they could have their own compatible strings in DT.
> >
> > That is what the MFD driver provides after all. Why would it be okay
> > to allocate different compatible strings from the MFD, but not in the
> > Device Tree?
> >
> > It would be the easiest solution.
> >
> > Has Rob commented on this yet?
>
> V4 of the bindings were acked by Rob and you:
> https://patchwork.kernel.org/patch/10428087/
We didn't Ack the bindings. We Acked the location change.
I mean, has Rob specifically spoken out about using a compatible
string for each function.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Alexandre Belloni @ 2018-09-12 7:33 UTC (permalink / raw)
To: Lee Jones
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180911225440.GL4185@dell>
On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > >
> > > USART doc starting p572, registers p621.
>
> After looking at the datasheet, I don't see any reason why one of the
> two drivers can't be selected using different compatible strings.
Because there is only one IP and we don't use the device tree to selecet
linux specific drivers.
If you are not happy having that in MFD, I guess we can move it out
somewhere else.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Alexandre Belloni @ 2018-09-12 7:30 UTC (permalink / raw)
To: Lee Jones
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180911224302.GJ4185@dell>
On 11/09/2018 23:43:02+0100, Lee Jones wrote:
> > I haven't read it, but I believe it's not unlike Renesas SCIF, which is
> > served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
> > But the latter is not used from DT, so we haven't experienced (and solved)
> > the similar issue yet.
> >
> > Would it work if the UART driver and SPI driver would match against the
> > same compatible value, but the UART driver would do in its probe()
> > function:
> >
> > device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> > if (opmode != AT91_USART_MODE_SERIAL)
> > return ENODEV;
> >
> > while the SPI driver would do:
> >
> > device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> > if (opmode != AT91_USART_MODE_SPI)
> > return ENODEV;
> >
> > ? No MFD driver involved.
>
> I haven't looked at the code in a while, but if memory serves I
> believe platform code gives up once it has found its first match, so
> by doing this, one of the drivers will never be matched/probed.
>
> It's midnight here, so cracking out the datasheet isn't going to
> happen just now, but it's my current belief that if the IP serves 2
> very different modes of operation, even if the registers are in a
> shared space, they could have their own compatible strings in DT.
>
> That is what the MFD driver provides after all. Why would it be okay
> to allocate different compatible strings from the MFD, but not in the
> Device Tree?
>
> It would be the easiest solution.
>
> Has Rob commented on this yet?
>
V4 of the bindings were acked by Rob and you:
https://patchwork.kernel.org/patch/10428087/
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Lee Jones @ 2018-09-11 22:54 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180911224418.GK4185@dell>
On Tue, 11 Sep 2018, Lee Jones wrote:
> On Tue, 11 Sep 2018, Alexandre Belloni wrote:
>
> > On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > >
> > > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > > Radu Pirea (6):
> > > > > > > > > MAINTAINERS: add at91 usart mfd driver
> > > > > > > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > > > mfd: at91-usart: added mfd driver for usart
> > > > > > > > > MAINTAINERS: add at91 usart spi driver
> > > > > > > > > spi: at91-usart: add driver for at91-usart as spi
> > > > > > > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > > > > >
> > > > > > > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > > > > > > MAINTAINERS | 16 +
> > > > > > > > > drivers/mfd/Kconfig | 9 +
> > > > > > > > > drivers/mfd/Makefile | 1 +
> > > > > > > > > drivers/mfd/at91-usart.c | 71 +++
> > > > > > > > > drivers/spi/Kconfig | 8 +
> > > > > > > > > drivers/spi/Makefile | 1 +
> > > > > > > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > > > > > > drivers/tty/serial/Kconfig | 1 +
> > > > > > > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > > > > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > > > > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > > > > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > > >
> > > > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > > > the liberty to peruse back into its history to figure out where things
> > > > > > > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > > > > > > I did!
> > > > > > > >
> > > > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > > > why the code hasn't caught my eye before today. I reviewed the
> > > > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > > > in v4 and nothing else. It appears as though you mistakenly added it
> > > > > > > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > > > > > > told you I'd already reviewed the binding document.
> > > > > > > >
> > > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > > > that I do not like what I see. Besides the missing header file and
> > > > > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > > > use-case.
> > > > > > > >
> > > > > > > > What's stopping you from just using the compatible string directly to
> > > > > > > > select which driver you need to probe?
> > > > > > > >
> > > > > > >
> > > > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > > > big no-no.
> > > > > >
> > > > > > It's still the same hardware device, isn't?
> > > > > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > > >
> > > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > > hardware description in DT? If that's impossible (I didn't look into detail
> > > > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > > > a mode property should be added?
> > > > >
> > > > > Yes, this is exactly what is done:
> > > > >
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > > >
> > > > OK.
> > > >
> > > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > > which thus requires having additional compatible values?
> > > >
> > > > I think those can just be removed. AFAICS, the SPI and serial drivers already
> > > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> > > > names?
> > >
> > > The hackish part of this driver is that it's using MFD for something
> > > which is clearly not an MFD. It's a USART device. Nothing more,
> > > nothing less.
> > >
> > > Does anyone have the datasheet to hand?
> > >
> >
> > It is not a simple usart, it is either a usart or a full blown SPI
> > controller with registers changing layout depending on the selected
> > mode. Otherwise, I'm not sure how you would get a USART to do SPI.
>
> Make up your mind. Either the IP is different, or it's not. ;)
>
> > Datasheet here:
>
> Great. Thank you.
>
> > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> >
> > USART doc starting p572, registers p621.
After looking at the datasheet, I don't see any reason why one of the
two drivers can't be selected using different compatible strings.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Lee Jones @ 2018-09-11 22:44 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180911185839.GA25212@piout.net>
On Tue, 11 Sep 2018, Alexandre Belloni wrote:
> On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> >
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > > <alexandre.belloni@bootlin.com> wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > Radu Pirea (6):
> > > > > > > > MAINTAINERS: add at91 usart mfd driver
> > > > > > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > > mfd: at91-usart: added mfd driver for usart
> > > > > > > > MAINTAINERS: add at91 usart spi driver
> > > > > > > > spi: at91-usart: add driver for at91-usart as spi
> > > > > > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > > > >
> > > > > > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > > > > > MAINTAINERS | 16 +
> > > > > > > > drivers/mfd/Kconfig | 9 +
> > > > > > > > drivers/mfd/Makefile | 1 +
> > > > > > > > drivers/mfd/at91-usart.c | 71 +++
> > > > > > > > drivers/spi/Kconfig | 8 +
> > > > > > > > drivers/spi/Makefile | 1 +
> > > > > > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > > > > > drivers/tty/serial/Kconfig | 1 +
> > > > > > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > > > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > > > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > > > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > >
> > > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > > the liberty to peruse back into its history to figure out where things
> > > > > > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > > > > > I did!
> > > > > > >
> > > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > > why the code hasn't caught my eye before today. I reviewed the
> > > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > > in v4 and nothing else. It appears as though you mistakenly added it
> > > > > > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > > > > > told you I'd already reviewed the binding document.
> > > > > > >
> > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > > that I do not like what I see. Besides the missing header file and
> > > > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > > use-case.
> > > > > > >
> > > > > > > What's stopping you from just using the compatible string directly to
> > > > > > > select which driver you need to probe?
> > > > > > >
> > > > > >
> > > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > >
> > > OK.
> > >
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > >
> > > I think those can just be removed. AFAICS, the SPI and serial drivers already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> > > names?
> >
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD. It's a USART device. Nothing more,
> > nothing less.
> >
> > Does anyone have the datasheet to hand?
> >
>
> It is not a simple usart, it is either a usart or a full blown SPI
> controller with registers changing layout depending on the selected
> mode. Otherwise, I'm not sure how you would get a USART to do SPI.
Make up your mind. Either the IP is different, or it's not. ;)
> Datasheet here:
Great. Thank you.
> http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
>
> USART doc starting p572, registers p621.
>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Lee Jones @ 2018-09-11 22:43 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Alexandre Belloni, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <CAMuHMdUMt=HJGCDZvJF99=45mecn5AjnTr-tA_LioxWrNKM2ag@mail.gmail.com>
On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> On Tue, Sep 11, 2018 at 8:40 PM Lee Jones <lee.jones@linaro.org> wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > > <alexandre.belloni@bootlin.com> wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > Radu Pirea (6):
> > > > > > > > MAINTAINERS: add at91 usart mfd driver
> > > > > > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > > mfd: at91-usart: added mfd driver for usart
> > > > > > > > MAINTAINERS: add at91 usart spi driver
> > > > > > > > spi: at91-usart: add driver for at91-usart as spi
> > > > > > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > > > >
> > > > > > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > > > > > MAINTAINERS | 16 +
> > > > > > > > drivers/mfd/Kconfig | 9 +
> > > > > > > > drivers/mfd/Makefile | 1 +
> > > > > > > > drivers/mfd/at91-usart.c | 71 +++
> > > > > > > > drivers/spi/Kconfig | 8 +
> > > > > > > > drivers/spi/Makefile | 1 +
> > > > > > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > > > > > drivers/tty/serial/Kconfig | 1 +
> > > > > > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > > > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > > > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > > > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > >
> > > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > > the liberty to peruse back into its history to figure out where things
> > > > > > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > > > > > I did!
> > > > > > >
> > > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > > why the code hasn't caught my eye before today. I reviewed the
> > > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > > in v4 and nothing else. It appears as though you mistakenly added it
> > > > > > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > > > > > told you I'd already reviewed the binding document.
> > > > > > >
> > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > > that I do not like what I see. Besides the missing header file and
> > > > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > > use-case.
> > > > > > >
> > > > > > > What's stopping you from just using the compatible string directly to
> > > > > > > select which driver you need to probe?
> > > > > > >
> > > > > >
> > > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > >
> > > OK.
> > >
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > >
> > > I think those can just be removed. AFAICS, the SPI and serial drivers already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> > > names?
> >
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD. It's a USART device. Nothing more,
> > nothing less.
> >
> > Does anyone have the datasheet to hand?
>
> I haven't read it, but I believe it's not unlike Renesas SCIF, which is
> served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
> But the latter is not used from DT, so we haven't experienced (and solved)
> the similar issue yet.
>
> Would it work if the UART driver and SPI driver would match against the
> same compatible value, but the UART driver would do in its probe()
> function:
>
> device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> if (opmode != AT91_USART_MODE_SERIAL)
> return ENODEV;
>
> while the SPI driver would do:
>
> device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> if (opmode != AT91_USART_MODE_SPI)
> return ENODEV;
>
> ? No MFD driver involved.
I haven't looked at the code in a while, but if memory serves I
believe platform code gives up once it has found its first match, so
by doing this, one of the drivers will never be matched/probed.
It's midnight here, so cracking out the datasheet isn't going to
happen just now, but it's my current belief that if the IP serves 2
very different modes of operation, even if the registers are in a
shared space, they could have their own compatible strings in DT.
That is what the MFD driver provides after all. Why would it be okay
to allocate different compatible strings from the MFD, but not in the
Device Tree?
It would be the easiest solution.
Has Rob commented on this yet?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Geert Uytterhoeven @ 2018-09-11 19:04 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Lee Jones, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi <linux-spi@
In-Reply-To: <20180911185839.GA25212@piout.net>
Hi Alexandre,
On Tue, Sep 11, 2018 at 8:58 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> >
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > > <alexandre.belloni@bootlin.com> wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > >
> > > OK.
> > >
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > >
> > > I think those can just be removed. AFAICS, the SPI and serial drivers already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> > > names?
> >
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD. It's a USART device. Nothing more,
> > nothing less.
> >
> > Does anyone have the datasheet to hand?
>
> It is not a simple usart, it is either a usart or a full blown SPI
> controller with registers changing layout depending on the selected
> mode. Otherwise, I'm not sure how you would get a USART to do SPI.
Note the "S" in USART. SPI is just synchronous serial with a shared clock
for transmit and receive. So the hardware is not that unrelated.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
From: Geert Uytterhoeven @ 2018-09-11 18:59 UTC (permalink / raw)
To: Lee Jones
Cc: Alexandre Belloni, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <20180911181838.GI4185@dell>
Hi Lee,
On Tue, Sep 11, 2018 at 8:40 PM Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > Radu Pirea (6):
> > > > > > > MAINTAINERS: add at91 usart mfd driver
> > > > > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > mfd: at91-usart: added mfd driver for usart
> > > > > > > MAINTAINERS: add at91 usart spi driver
> > > > > > > spi: at91-usart: add driver for at91-usart as spi
> > > > > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > > >
> > > > > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > > > > MAINTAINERS | 16 +
> > > > > > > drivers/mfd/Kconfig | 9 +
> > > > > > > drivers/mfd/Makefile | 1 +
> > > > > > > drivers/mfd/at91-usart.c | 71 +++
> > > > > > > drivers/spi/Kconfig | 8 +
> > > > > > > drivers/spi/Makefile | 1 +
> > > > > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > > > > drivers/tty/serial/Kconfig | 1 +
> > > > > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > >
> > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > the liberty to peruse back into its history to figure out where things
> > > > > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > > > > I did!
> > > > > >
> > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > why the code hasn't caught my eye before today. I reviewed the
> > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > in v4 and nothing else. It appears as though you mistakenly added it
> > > > > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > > > > told you I'd already reviewed the binding document.
> > > > > >
> > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > that I do not like what I see. Besides the missing header file and
> > > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > use-case.
> > > > > >
> > > > > > What's stopping you from just using the compatible string directly to
> > > > > > select which driver you need to probe?
> > > > > >
> > > > >
> > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > big no-no.
> > > >
> > > > It's still the same hardware device, isn't?
> > > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > >
> > > > Hence shouldn't the software derive the hardware mode from the full
> > > > hardware description in DT? If that's impossible (I didn't look into detail
> > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > a mode property should be added?
> > >
> > > Yes, this is exactly what is done:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> >
> > OK.
> >
> > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > which thus requires having additional compatible values?
> >
> > I think those can just be removed. AFAICS, the SPI and serial drivers already
> > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> > names?
>
> The hackish part of this driver is that it's using MFD for something
> which is clearly not an MFD. It's a USART device. Nothing more,
> nothing less.
>
> Does anyone have the datasheet to hand?
I haven't read it, but I believe it's not unlike Renesas SCIF, which is
served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
But the latter is not used from DT, so we haven't experienced (and solved)
the similar issue yet.
Would it work if the UART driver and SPI driver would match against the
same compatible value, but the UART driver would do in its probe()
function:
device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
if (opmode != AT91_USART_MODE_SERIAL)
return ENODEV;
while the SPI driver would do:
device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
if (opmode != AT91_USART_MODE_SPI)
return ENODEV;
? No MFD driver involved.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox