* Re: [PATCH v2] tty: serial: qcom_geni_serial: Fix softlock
From: kbuild test robot @ 2018-11-29 10:17 UTC (permalink / raw)
Cc: kbuild-all, Greg Kroah-Hartman, Jiri Slaby, Evan Green,
Doug Anderson, linux-kernel, linux-serial, Stephen Boyd,
Ryan Case
In-Reply-To: <20181128235459.180940-1-ryandcase@chromium.org>
[-- Attachment #1: Type: text/plain, Size: 4327 bytes --]
Hi Ryan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on v4.20-rc4 next-20181129]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ryan-Case/tty-serial-qcom_geni_serial-Fix-softlock/20181129-174407
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=mips
All warnings (new ones prefixed by >>):
In file included from include/linux/clk.h:16:0,
from drivers/tty/serial/qcom_geni_serial.c:8:
drivers/tty/serial/qcom_geni_serial.c: In function 'qcom_geni_serial_handle_tx':
include/linux/kernel.h:845:29: warning: comparison of distinct pointer types lacks a cast
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
^
include/linux/kernel.h:859:4: note: in expansion of macro '__typecheck'
(__typecheck(x, y) && __no_side_effects(x, y))
^~~~~~~~~~~
include/linux/kernel.h:869:24: note: in expansion of macro '__safe_cmp'
__builtin_choose_expr(__safe_cmp(x, y), \
^~~~~~~~~~
include/linux/kernel.h:878:19: note: in expansion of macro '__careful_cmp'
#define min(x, y) __careful_cmp(x, y, <)
^~~~~~~~~~~~~
include/linux/kernel.h:893:23: note: in expansion of macro 'min'
#define min3(x, y, z) min((typeof(x))min(x, y), z)
^~~
>> drivers/tty/serial/qcom_geni_serial.c:746:10: note: in expansion of macro 'min3'
chunk = min3(avail, pending, (UART_XMIT_SIZE - tail));
^~~~
vim +/min3 +746 drivers/tty/serial/qcom_geni_serial.c
714
715 static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
716 bool active)
717 {
718 struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
719 struct circ_buf *xmit = &uport->state->xmit;
720 size_t avail;
721 size_t remaining;
722 size_t pending;
723 int i;
724 u32 status;
725 unsigned int chunk;
726 int tail;
727
728 status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
729
730 /* Complete the current tx command before taking newly added data */
731 if (active)
732 pending = port->tx_remaining;
733 else
734 pending = uart_circ_chars_pending(xmit);
735
736 /* All data has been transmitted and acknowledged as received */
737 if (!pending && !status && done) {
738 qcom_geni_serial_stop_tx(uport);
739 goto out_write_wakeup;
740 }
741
742 avail = port->tx_fifo_depth - (status & TX_FIFO_WC);
743 avail *= port->tx_bytes_pw;
744
745 tail = xmit->tail;
> 746 chunk = min3(avail, pending, (UART_XMIT_SIZE - tail));
747 if (!chunk)
748 goto out_write_wakeup;
749
750 if (!port->tx_remaining) {
751 qcom_geni_serial_setup_tx(uport, pending);
752 port->tx_remaining = pending;
753 }
754
755 remaining = chunk;
756 for (i = 0; i < chunk; ) {
757 unsigned int tx_bytes;
758 u8 buf[sizeof(u32)];
759 int c;
760
761 memset(buf, 0, ARRAY_SIZE(buf));
762 tx_bytes = min_t(size_t, remaining, port->tx_bytes_pw);
763 for (c = 0; c < tx_bytes ; c++)
764 buf[c] = xmit->buf[tail + c];
765
766 iowrite32_rep(uport->membase + SE_GENI_TX_FIFOn, buf, 1);
767
768 i += tx_bytes;
769 tail += tx_bytes;
770 uport->icount.tx += tx_bytes;
771 remaining -= tx_bytes;
772 port->tx_remaining -= tx_bytes;
773 }
774
775 xmit->tail = tail & (UART_XMIT_SIZE - 1);
776 out_write_wakeup:
777 if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
778 uart_write_wakeup(uport);
779 }
780
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58323 bytes --]
^ permalink raw reply
* Re: [PATCH 02/14] dt-bindings: soc: milbeaut: Add Milbeaut trampoline description
From: Sugaya, Taichi @ 2018-11-29 12:24 UTC (permalink / raw)
To: Stephen Boyd, devicetree, linux-arm-kernel, linux-clk,
linux-kernel, linux-serial
Cc: Michael Turquette, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Daniel Lezcano, Thomas Gleixner, Russell King, Jiri Slaby,
Masami Hiramatsu, Jassi Brar
In-Reply-To: <154337047410.88331.9696178601340675631@swboyd.mtv.corp.google.com>
Hi,
Thank you for your comments.
On 2018/11/28 11:01, Stephen Boyd wrote:
> Quoting Sugaya Taichi (2018-11-18 17:01:07)
>> Add DT bindings document for Milbeaut trampoline.
>>
>> Signed-off-by: Sugaya Taichi<sugaya.taichi@socionext.com>
>> ---
>> .../devicetree/bindings/soc/socionext/socionext,m10v.txt | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
>>
>> diff --git a/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
>> new file mode 100644
>> index 0000000..f5d906c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
>> @@ -0,0 +1,12 @@
>> +Socionext M10V SMP trampoline driver binding
>> +
>> +This is a driver to wait for sub-cores while boot process.
>> +
>> +- compatible: should be "socionext,smp-trampoline"
>> +- reg: should be <0x4C000100 0x100>
>> +
>> +EXAMPLE
>> + trampoline: trampoline@0x4C000100 {
> Drop the 0x part of unit addresses.
Okay.
>> + compatible = "socionext,smp-trampoline";
>> + reg = <0x4C000100 0x100>;
> Looks like a software construct, which we wouldn't want to put into DT
> this way. DT doesn't describe drivers.
We would like to use this node only getting the address of the
trampoline area
in which sub-cores wait. (They have finished to go to this area in previous
bootloader process.)
So should we embed the constant value in source codes instead of getting
from
DT because the address is constant at the moment? Or is there other
approach?
Thanks
Sugaya Taichi
^ permalink raw reply
* Re: [PATCH] dmaengine: at_hdmac: fix memory leak in at_dma_xlate()
From: Ludovic Desroches @ 2018-11-29 13:51 UTC (permalink / raw)
To: Richard Genoud
Cc: Dan Williams, Vinod Koul, Alexandre Belloni, Nicolas Ferre,
Maxime Ripard, Mario Forner, linux-arm-kernel, dmaengine,
linux-kernel, linux-serial, stable
In-Reply-To: <20181127160635.11836-1-richard.genoud@gmail.com>
On Tue, Nov 27, 2018 at 05:06:34PM +0100, Richard Genoud wrote:
> The leak was found when opening/closing a serial port a great number of
> time, increasing kmalloc-32 in slabinfo.
>
> Each time the port was opened, dma_request_slave_channel() was called.
> Then, in at_dma_xlate(), atslave was allocated with devm_kzalloc() and
> never freed. (Well, it was free at module unload, but that's not what we
> want).
> So, here, kzalloc is more suited for the job since it has to be freed in
> atc_free_chan_resources().
>
> Cc: stable@vger.kernel.org
> Fixes: bbe89c8e3d59 ("at_hdmac: move to generic DMA binding")
> Reported-by: Mario Forner <m.forner@be4energy.com>
> Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>
Thanks for handling this issue.
Ludovic
> ---
> drivers/dma/at_hdmac.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index 7cbac6e8c113..1b7f0ca0d5cd 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -1641,6 +1641,12 @@ static void atc_free_chan_resources(struct dma_chan *chan)
> atchan->descs_allocated = 0;
> atchan->status = 0;
>
> + /*
> + * Free atslave allocated in at_dma_xlate()
> + */
> + kfree(chan->private);
> + chan->private = NULL;
> +
> dev_vdbg(chan2dev(chan), "free_chan_resources: done\n");
> }
>
> @@ -1675,7 +1681,7 @@ static struct dma_chan *at_dma_xlate(struct of_phandle_args *dma_spec,
> dma_cap_zero(mask);
> dma_cap_set(DMA_SLAVE, mask);
>
> - atslave = devm_kzalloc(&dmac_pdev->dev, sizeof(*atslave), GFP_KERNEL);
> + atslave = kzalloc(sizeof(*atslave), GFP_KERNEL);
> if (!atslave)
> return NULL;
>
^ permalink raw reply
* Re: [PATCH] dmaengine: at_hdmac: fix module unloading
From: Ludovic Desroches @ 2018-11-29 13:52 UTC (permalink / raw)
To: Richard Genoud
Cc: Dan Williams, Vinod Koul, Alexandre Belloni, Nicolas Ferre,
Maxime Ripard, Mario Forner, linux-arm-kernel, dmaengine,
linux-kernel, linux-serial, stable
In-Reply-To: <20181127160635.11836-2-richard.genoud@gmail.com>
On Tue, Nov 27, 2018 at 05:06:35PM +0100, Richard Genoud wrote:
> of_dma_controller_free() was not called on module onloading.
s/onloading/unloading
> This lead to a soft lockup:
> watchdog: BUG: soft lockup - CPU#0 stuck for 23s!
> Modules linked in: at_hdmac [last unloaded: at_hdmac]
> when of_dma_request_slave_channel() tried to call ofdma->of_dma_xlate().
>
> Cc: stable@vger.kernel.org
> Fixes: bbe89c8e3d59 ("at_hdmac: move to generic DMA binding")
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>
Thanks
Ludovic
> ---
> drivers/dma/at_hdmac.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index 1b7f0ca0d5cd..01d936c9fe89 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -2006,6 +2006,8 @@ static int at_dma_remove(struct platform_device *pdev)
> struct resource *io;
>
> at_dma_off(atdma);
> + if (pdev->dev.of_node)
> + of_dma_controller_free(pdev->dev.of_node);
> dma_async_device_unregister(&atdma->dma_common);
>
> dma_pool_destroy(atdma->memset_pool);
^ permalink raw reply
* Re: [PATCH] dmaengine: at_hdmac: fix memory leak in at_dma_xlate()
From: Vinod Koul @ 2018-11-29 14:20 UTC (permalink / raw)
To: Richard Genoud
Cc: Ludovic Desroches, Dan Williams, Alexandre Belloni, Nicolas Ferre,
Maxime Ripard, Mario Forner, linux-arm-kernel, dmaengine,
linux-kernel, linux-serial, stable
In-Reply-To: <20181127160635.11836-1-richard.genoud@gmail.com>
On 27-11-18, 17:06, Richard Genoud wrote:
> The leak was found when opening/closing a serial port a great number of
> time, increasing kmalloc-32 in slabinfo.
>
> Each time the port was opened, dma_request_slave_channel() was called.
> Then, in at_dma_xlate(), atslave was allocated with devm_kzalloc() and
> never freed. (Well, it was free at module unload, but that's not what we
> want).
> So, here, kzalloc is more suited for the job since it has to be freed in
> atc_free_chan_resources().
Applied, thanks
--
~Vinod
^ permalink raw reply
* Re: [PATCH] dmaengine: at_hdmac: fix module unloading
From: Vinod Koul @ 2018-11-29 14:21 UTC (permalink / raw)
To: Richard Genoud
Cc: Alexandre Belloni, Mario Forner, Maxime Ripard, stable,
linux-kernel, Ludovic Desroches, linux-serial, dmaengine,
Dan Williams, linux-arm-kernel
In-Reply-To: <20181127160635.11836-2-richard.genoud@gmail.com>
On 27-11-18, 17:06, Richard Genoud wrote:
> of_dma_controller_free() was not called on module onloading.
> This lead to a soft lockup:
> watchdog: BUG: soft lockup - CPU#0 stuck for 23s!
> Modules linked in: at_hdmac [last unloaded: at_hdmac]
> when of_dma_request_slave_channel() tried to call ofdma->of_dma_xlate().
Applied, thanks
--
~Vinod
^ permalink raw reply
* Re: [PATCH v2 01/10] mailbox: Support blocking transfers in atomic context
From: Thierry Reding @ 2018-11-29 15:23 UTC (permalink / raw)
To: Jassi Brar
Cc: Devicetree List, Greg KH, mliljeberg, Mikko Perttunen, talho,
linux-serial, jslaby, linux-tegra, ppessi, Jon Hunter,
linux-arm-kernel
In-Reply-To: <CABb+yY1u24JYO68f74qSJ1RpqidJ4H7cnP1Sy=RdqcBHru5XQg@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 5944 bytes --]
On Wed, Nov 28, 2018 at 11:23:36PM -0600, Jassi Brar wrote:
> On Wed, Nov 28, 2018 at 3:43 AM Jon Hunter <jonathanh@nvidia.com> wrote:
> >
> >
> > On 12/11/2018 15:18, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > The mailbox framework supports blocking transfers via completions for
> > > clients that can sleep. In order to support blocking transfers in cases
> > > where the transmission is not permitted to sleep, add a new ->flush()
> > > callback that controller drivers can implement to busy loop until the
> > > transmission has been completed. This will automatically be called when
> > > available and interrupts are disabled for clients that request blocking
> > > transfers.
> > >
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > > drivers/mailbox/mailbox.c | 8 ++++++++
> > > include/linux/mailbox_controller.h | 4 ++++
> > > 2 files changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> > > index 674b35f402f5..0eaf21259874 100644
> > > --- a/drivers/mailbox/mailbox.c
> > > +++ b/drivers/mailbox/mailbox.c
> > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
> > > unsigned long wait;
> > > int ret;
> > >
> > > + if (irqs_disabled() && chan->mbox->ops->flush) {
> > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout);
> > > + if (ret < 0)
> > > + tx_tick(chan, ret);
> > > +
> > > + return ret;
> > > + }
> >
> > It seems to me that if mbox_send_message() is called from an atomic
> > context AND tx_block is true, then if 'flush' is not populated this
> > should be an error condition as we do not wish to call
> > wait_for_completion from an atomic context.
> >
> > I understand that there is some debate about adding such flush support,
> > but irrespective of the above change, it seems to me that if the
> > mbox_send_message() can be called from an atomic context (which it
> > appears to), then it should be detecting if someone is trying to do so
> > with 'tx_block' set as this should be an error.
> >
> Layers within kernel space have to trust each other. A badly written
> client can break the consumer in so many ways, we can not catch every
> possibility.
>
> > Furthermore, if the underlying mailbox driver can support sending a
> > message from an atomic context and busy wait until it is done, surely
> > the mailbox framework should provide a means to support this?
> >
> Being able to put the message on bus in atomic context is a feature -
> which we do support. But busy-waiting in a loop is not a feature, and
> we don't want to encourage that.
I agree that in generally busy-waiting is a bad idea and shouldn't be
encouraged. However, I also think that an exception proves the rule. If
you look at the console drivers in drivers/tty/serial, all of them will
busy loop prior to or after sending a character. This is pretty much
part of the API and as such busy-looping is very much a feature.
The reason why this is done is because on one hand we have an atomic
context and on the other hand we want to make sure that all characters
actually make it to the console when we print them.
As an example how this can break, I've taken your suggestion to
implement a producer/consumer mode in the TCU driver where the console
write function will just stash characters into a circular buffer and a
work queue will then use mbox_send_message() to drain the circular
buffer. While this does work on the surface, I was able to concern both
of the issues that I was concerned about: 1) it is easy to overflow the
circular buffer by just dumping enough data at once to the console and
2) when a crash happens, everything in the kernel stops, including the
consumer workqueue that is supposed to drain the circular buffer and
flush messages to the TCU. The result is that, in my particular case,
the boot log will just stop at a random place in the middle of messages
from much earlier in the boot because the TCU hasn't caught up yet and
there's a lot of characters still in the circular buffer.
Now, 1) can be mitigated by increasing the circular buffer size. A value
that seems to give reliably good results in 2 << CONFIG_LOG_BUF_SHIFT. I
thought that I could also mitigate 2) by busy looping in the TCU driver,
but it turns out that that doesn't work. The reason is that since we are
in atomic context with all interrupts disabled, the mailbox won't ever
consume any new characters, so the read pointer in the circular buffer
won't increment, leaving me with no condition upon which to loop that
would work.
Given that 2) can't be solved makes this implementation of the TCU
completely useless as a console.
All of that said, have you had a chance to look at my other proposed
solution for this? I sent it out yesterday, but you can also find the
series here:
http://patchwork.ozlabs.org/project/linux-tegra/list/?series=78477
The difference to the earlier versions is that the flushing is now
explicit. I think this combines the best of both worlds. On one hand it
makes the mechanism completely opt-in, so nothing gets hidden in the
regular functions. On the other hand, it allows clients to make use of
this functionality very explicitly. A client that doesn't call the
mbox_flush() function will just not busy-loop. But clients that need to
make sure messages are flushed in atomic contexts can now do that. Does
that sound like a more acceptable solution to you? We could even go and
add documentation to mbox_flush() that it should only be used under
special circumstances.
If you still think that's a bad idea, do you have any other suggestions
on how to move forward?
Thierry
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2] tty: serial: qcom_geni_serial: Fix softlock
From: Doug Anderson @ 2018-11-29 22:12 UTC (permalink / raw)
To: ryandcase
Cc: Greg Kroah-Hartman, Jiri Slaby, Evan Green, LKML, linux-serial,
Stephen Boyd
In-Reply-To: <20181128235459.180940-1-ryandcase@chromium.org>
Hi,
On Wed, Nov 28, 2018 at 3:55 PM Ryan Case <ryandcase@chromium.org> wrote:
> @@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
> }
> writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> SE_GENI_M_IRQ_CLEAR);
> + } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
> + /*
> + * It seems we can interrupt existing transfers unless all data
s/It seems we can interrupt/It seems we can't interrupt/
> +static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
> + bool active)
> {
> struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> struct circ_buf *xmit = &uport->state->xmit;
> size_t avail;
> size_t remaining;
> + size_t pending;
> int i;
> u32 status;
> unsigned int chunk;
> int tail;
> - u32 irq_en;
>
> - chunk = uart_circ_chars_pending(xmit);
> status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> - /* Both FIFO and framework buffer are drained */
> - if (!chunk && !status) {
> +
> + /* Complete the current tx command before taking newly added data */
> + if (active)
> + pending = port->tx_remaining;
I almost feel like this should be:
if (port->tx_remaining)
pending = port->tx_remaining
I could imagine active being false but "port->tx_remaining" being
non-zero if we happened to take a long time to handle the interrupt
for some reason. Presumably you could simulator this and see what
breaks. I think what would happen would be "pending" will be larger
than you expect and you could write a few extra bytes into the FIFO
causing it to go beyond the length of the transfer you setup. ...so I
guess you'd drop some bytes?
If it's somehow important for "pending" to be 0 still when we're
active but port->tx_remaining is non-zero, then I guess you could also
write it as:
if (active || port->tx_remaining)
pending = port->tx_remaining
Maybe I'm misunderstanding though.
-Doug
^ permalink raw reply
* Re: [PATCH v2] tty: serial: qcom_geni_serial: Fix softlock
From: Ryan Case @ 2018-11-30 1:51 UTC (permalink / raw)
To: Doug Anderson
Cc: Greg Kroah-Hartman, Jiri Slaby, Evan Green, linux-kernel,
linux-serial, Stephen Boyd
In-Reply-To: <CAD=FV=VTkM+mWZX4jS96E0JdpYpW40q_EkGPfvLxWKW==e+JMw@mail.gmail.com>
On Thu, Nov 29, 2018 at 2:12 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Nov 28, 2018 at 3:55 PM Ryan Case <ryandcase@chromium.org> wrote:
> > @@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
> > }
> > writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> > SE_GENI_M_IRQ_CLEAR);
> > + } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
> > + /*
> > + * It seems we can interrupt existing transfers unless all data
>
> s/It seems we can interrupt/It seems we can't interrupt/
Comment is correct as is, but will reword to make things clearer.
>
>
> > +static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
> > + bool active)
> > {
> > struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> > struct circ_buf *xmit = &uport->state->xmit;
> > size_t avail;
> > size_t remaining;
> > + size_t pending;
> > int i;
> > u32 status;
> > unsigned int chunk;
> > int tail;
> > - u32 irq_en;
> >
> > - chunk = uart_circ_chars_pending(xmit);
> > status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> > - /* Both FIFO and framework buffer are drained */
> > - if (!chunk && !status) {
> > +
> > + /* Complete the current tx command before taking newly added data */
> > + if (active)
> > + pending = port->tx_remaining;
>
> I almost feel like this should be:
>
> if (port->tx_remaining)
> pending = port->tx_remaining
>
> I could imagine active being false but "port->tx_remaining" being
> non-zero if we happened to take a long time to handle the interrupt
> for some reason. Presumably you could simulator this and see what
> breaks. I think what would happen would be "pending" will be larger
> than you expect and you could write a few extra bytes into the FIFO
> causing it to go beyond the length of the transfer you setup. ...so I
> guess you'd drop some bytes?
>
>
> If it's somehow important for "pending" to be 0 still when we're
> active but port->tx_remaining is non-zero, then I guess you could also
> write it as:
>
> if (active || port->tx_remaining)
> pending = port->tx_remaining
>
>
> Maybe I'm misunderstanding though.
Yeah, this flag has different behavior on the hardware than you're
expecting. Active will be set for the duration of the command no
matter the size of the transfer or across how many interrupts the
transfer takes, including after we're done on our side
(port->tx_remaining is zero).
>
>
> -Doug
^ permalink raw reply
* [PATCH v3] tty: serial: qcom_geni_serial: Fix softlock
From: Ryan Case @ 2018-11-30 2:18 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Evan Green, Doug Anderson, linux-kernel, linux-serial,
Stephen Boyd, Ryan Case
Transfers were being divided into device FIFO sized (64 byte max)
operations which would poll for completion within a spin_lock_irqsave /
spin_unlock_irqrestore block. This both made things slow by waiting for
the FIFO to completely drain before adding further data and would also
result in softlocks on large transmissions.
This patch allows larger transfers with continuous FIFO additions as
space becomes available and removes polling from the interrupt handler.
Signed-off-by: Ryan Case <ryandcase@chromium.org>
---
Changes in v3:
- Reworded comment for clarity
- Fixed mips compiler warning
Changes in v2:
- Addressed nits raised by Stephen
drivers/tty/serial/qcom_geni_serial.c | 56 +++++++++++++++++++--------
1 file changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 7ded51081add..bbc7d7a5b8b6 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -117,6 +117,8 @@ struct qcom_geni_serial_port {
u32 *rx_fifo;
u32 loopback;
bool brk;
+
+ unsigned int tx_remaining;
};
static const struct uart_ops qcom_geni_console_pops;
@@ -439,6 +441,7 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
struct qcom_geni_serial_port *port;
bool locked = true;
unsigned long flags;
+ u32 geni_status;
WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
@@ -452,6 +455,8 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
else
spin_lock_irqsave(&uport->lock, flags);
+ geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
+
/* Cancel the current write to log the fault */
if (!locked) {
geni_se_cancel_m_cmd(&port->se);
@@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
}
writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
SE_GENI_M_IRQ_CLEAR);
+ } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
+ /*
+ * It seems we can't interrupt existing transfers if all data
+ * has been sent, in which case we need to look for done first.
+ */
+ qcom_geni_serial_poll_tx_done(uport);
}
__qcom_geni_serial_console_write(uport, s, count);
+
+ if (port->tx_remaining)
+ qcom_geni_serial_setup_tx(uport, port->tx_remaining);
+
if (locked)
spin_unlock_irqrestore(&uport->lock, flags);
}
@@ -701,40 +716,45 @@ static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop)
port->handle_rx(uport, total_bytes, drop);
}
-static void qcom_geni_serial_handle_tx(struct uart_port *uport)
+static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
+ bool active)
{
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
struct circ_buf *xmit = &uport->state->xmit;
size_t avail;
size_t remaining;
+ size_t pending;
int i;
u32 status;
unsigned int chunk;
int tail;
- u32 irq_en;
- chunk = uart_circ_chars_pending(xmit);
status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
- /* Both FIFO and framework buffer are drained */
- if (!chunk && !status) {
+
+ /* Complete the current tx command before taking newly added data */
+ if (active)
+ pending = port->tx_remaining;
+ else
+ pending = uart_circ_chars_pending(xmit);
+
+ /* All data has been transmitted and acknowledged as received */
+ if (!pending && !status && done) {
qcom_geni_serial_stop_tx(uport);
goto out_write_wakeup;
}
- if (!uart_console(uport)) {
- irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
- irq_en &= ~(M_TX_FIFO_WATERMARK_EN);
- writel_relaxed(0, uport->membase + SE_GENI_TX_WATERMARK_REG);
- writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
- }
+ avail = port->tx_fifo_depth - (status & TX_FIFO_WC);
+ avail *= port->tx_bytes_pw;
- avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
tail = xmit->tail;
- chunk = min3((size_t)chunk, (size_t)(UART_XMIT_SIZE - tail), avail);
+ chunk = min3(avail, pending, (size_t)(UART_XMIT_SIZE - tail));
if (!chunk)
goto out_write_wakeup;
- qcom_geni_serial_setup_tx(uport, chunk);
+ if (!port->tx_remaining) {
+ qcom_geni_serial_setup_tx(uport, pending);
+ port->tx_remaining = pending;
+ }
remaining = chunk;
for (i = 0; i < chunk; ) {
@@ -753,11 +773,10 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport)
tail += tx_bytes;
uport->icount.tx += tx_bytes;
remaining -= tx_bytes;
+ port->tx_remaining -= tx_bytes;
}
xmit->tail = tail & (UART_XMIT_SIZE - 1);
- if (uart_console(uport))
- qcom_geni_serial_poll_tx_done(uport);
out_write_wakeup:
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(uport);
@@ -767,6 +786,7 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
{
unsigned int m_irq_status;
unsigned int s_irq_status;
+ unsigned int geni_status;
struct uart_port *uport = dev;
unsigned long flags;
unsigned int m_irq_en;
@@ -780,6 +800,7 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
spin_lock_irqsave(&uport->lock, flags);
m_irq_status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
s_irq_status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
+ geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
m_irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
writel_relaxed(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR);
writel_relaxed(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR);
@@ -794,7 +815,8 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) &&
m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
- qcom_geni_serial_handle_tx(uport);
+ qcom_geni_serial_handle_tx(uport, m_irq_status & M_CMD_DONE_EN,
+ geni_status & M_GENI_CMD_ACTIVE);
if (s_irq_status & S_GP_IRQ_0_EN || s_irq_status & S_GP_IRQ_1_EN) {
if (s_irq_status & S_GP_IRQ_0_EN)
--
2.20.0.rc0.387.gc7a69e6b6c-goog
^ permalink raw reply related
* Re: [PATCH v3] tty: serial: qcom_geni_serial: Fix softlock
From: Doug Anderson @ 2018-11-30 2:22 UTC (permalink / raw)
To: ryandcase
Cc: Greg Kroah-Hartman, Jiri Slaby, Evan Green, LKML, linux-serial,
Stephen Boyd
In-Reply-To: <20181130021840.75861-1-ryandcase@chromium.org>
Hi,
On Thu, Nov 29, 2018 at 6:19 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> Transfers were being divided into device FIFO sized (64 byte max)
> operations which would poll for completion within a spin_lock_irqsave /
> spin_unlock_irqrestore block. This both made things slow by waiting for
> the FIFO to completely drain before adding further data and would also
> result in softlocks on large transmissions.
>
> This patch allows larger transfers with continuous FIFO additions as
> space becomes available and removes polling from the interrupt handler.
>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
> Changes in v3:
> - Reworded comment for clarity
> - Fixed mips compiler warning
>
> Changes in v2:
> - Addressed nits raised by Stephen
>
> drivers/tty/serial/qcom_geni_serial.c | 56 +++++++++++++++++++--------
> 1 file changed, 39 insertions(+), 17 deletions(-)
Looks happy to me now. Thanks and sorry for misunderstanding how
"active" worked on v2.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [PATCH 02/14] dt-bindings: soc: milbeaut: Add Milbeaut trampoline description
From: Stephen Boyd @ 2018-11-30 8:16 UTC (permalink / raw)
To: Sugaya, Taichi, devicetree, linux-arm-kernel, linux-clk,
linux-kernel, linux-serial
Cc: Michael Turquette, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Daniel Lezcano, Thomas Gleixner, Russell King, Jiri Slaby,
Masami Hiramatsu, Jassi Brar
In-Reply-To: <c746a3de-a9e3-9f25-ecb9-703ceb7e62d0@socionext.com>
Quoting Sugaya, Taichi (2018-11-29 04:24:51)
> On 2018/11/28 11:01, Stephen Boyd wrote:
> > Quoting Sugaya Taichi (2018-11-18 17:01:07)
> >> create mode 100644 Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
> >> new file mode 100644
> >> index 0000000..f5d906c
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
> >> @@ -0,0 +1,12 @@
> >> +Socionext M10V SMP trampoline driver binding
> >> +
> >> +This is a driver to wait for sub-cores while boot process.
> >> +
> >> +- compatible: should be "socionext,smp-trampoline"
> >> +- reg: should be <0x4C000100 0x100>
> >> +
> >> +EXAMPLE
> >> + trampoline: trampoline@0x4C000100 {
> > Drop the 0x part of unit addresses.
>
> Okay.
>
>
> >> + compatible = "socionext,smp-trampoline";
> >> + reg = <0x4C000100 0x100>;
> > Looks like a software construct, which we wouldn't want to put into DT
> > this way. DT doesn't describe drivers.
> We would like to use this node only getting the address of the
> trampoline area
> in which sub-cores wait. (They have finished to go to this area in previous
> bootloader process.)
Is this area part of memory, or a special SRAM? If it's part of memory,
I would expect this node to be under the reserved-memory node and
pointed to by some other node that uses this region. Could even be the
CPU nodes.
>
> So should we embed the constant value in source codes instead of getting
> from
> DT because the address is constant at the moment? Or is there other
> approach?
>
If it's constant then that also works. Why does it need to come from DT
at all then?
^ permalink raw reply
* Re: [PATCH 06/14] dt-bindings: clock: milbeaut: add Milbeaut clock description
From: Stephen Boyd @ 2018-11-30 8:19 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-clk, linux-kernel,
linux-serial
Cc: Michael Turquette, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Daniel Lezcano, Thomas Gleixner, Russell King, Jiri Slaby,
Masami Hiramatsu, Jassi Brar, Sugaya Taichi
In-Reply-To: <1542589274-13878-7-git-send-email-sugaya.taichi@socionext.com>
Quoting Sugaya Taichi (2018-11-18 17:01:11)
> Add DT bindings document for Milbeaut clock.
>
> Signed-off-by: Sugaya Taichi <sugaya.taichi@socionext.com>
> ---
> .../devicetree/bindings/clock/milbeaut-clock.txt | 93 ++++++++++++++++++++++
> 1 file changed, 93 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/milbeaut-clock.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/milbeaut-clock.txt b/Documentation/devicetree/bindings/clock/milbeaut-clock.txt
> new file mode 100644
> index 0000000..5c093c8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/milbeaut-clock.txt
> @@ -0,0 +1,93 @@
> +Milbeaut M10V Clock Controller Binding
> +----------------------------------------
> +Milbeaut clock controller is consists of few oscillators, PLL, multiplexer
> +and few divider modules
> +
> +This binding uses common clock bindings
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible: shall be "socionext,milbeaut-m10v-clk-regs"
> +- reg: shall contain base address and length of clock registers
> +- #clock-cells: shall be 0
> +
> +Example:
> + m10v-clk-tree@ {
> + compatible = "socionext,milbeaut-m10v-clk-regs";
> + reg = <0x1d021000 0x4000>;
> +
> + clocks {
> + #address-cells = <0>;
> + #size-cells = <0>;
> +
> + uclk40xi: uclk40xi {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <40000000>;
> + };
> + };
This style of binding is highly discouraged. We don't describe each and
every clk in DT, we describe clk controllers and their outputs and
inputs in DT. The driver is the place where the clock controller
describes the internal clk topology of that controller. Also, fixed
frequency clks are typically oscillators and those would come from the
board dts file, but otherwise I wouldn't expect to see fixed frequency
clks in DT.
> + }
> +
> +The clock consumer shall specify the desired clock-output of the clock
> +controller (as defined in [2]) by specifying output-id in its "clock"
> +phandle cell
> +[2] arch/arm/boot/dts/milbeaut-m10v-clk.h
> +
[...]
> +
> +Example
> + piclk_mux_0: spiclk_mux_0 {
> + compatible = "socionext,m10v-clk-div";
> + #clock-cells = <0>;
> + clocks = <&pll10_div_1_2>;
> + offset = <bSPICLK>;
> + mask = <0x3>;
> + ratios = <4 0x5 2 0x4>;
> + };
> +
> + pll10: pll10 {
> + compatible = "socionext,m10v-pll-fixed-factor";
> + #clock-cells = <0>;
> + clocks = <&uclk40xi>;
> + offset = <10>;
> + clock-div = <5>;
> + clock-mult = <108>;
> + };
> +
> + emmcclk: emmcclk {
> + compatible = "socionext,m10v-clk-div";
> + #clock-cells = <0>;
> + clocks = <&pll11>;
> + offset = <bEMMCCLK>;
> + mask = <0x3>;
> + ratios = <15 0x7 10 0x6 9 0x5 8 0x4>;
Yeah, please no. This whole binding needs a rewrite to not have one node
per clk.
^ permalink raw reply
* Re: [PATCH 07/14] clock: milbeaut: Add Milbeaut M10V clock control
From: Stephen Boyd @ 2018-11-30 8:31 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-clk, linux-kernel,
linux-serial
Cc: Michael Turquette, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Daniel Lezcano, Thomas Gleixner, Russell King, Jiri Slaby,
Masami Hiramatsu, Jassi Brar, Sugaya Taichi
In-Reply-To: <1542589274-13878-8-git-send-email-sugaya.taichi@socionext.com>
Quoting Sugaya Taichi (2018-11-18 17:01:12)
> Add Milbeaut M10V clock ( including PLL ) control.
Please give some more details here.
>
> Signed-off-by: Sugaya Taichi <sugaya.taichi@socionext.com>
> ---
> drivers/clk/Makefile | 1 +
> drivers/clk/clk-m10v.c | 671 +++++++++++++++++++++++++++++++++++++++++++++++++
And this is different from Uniphier? Maybe we need a socionext
directory under drivers/clk/.
> 2 files changed, 672 insertions(+)
> create mode 100644 drivers/clk/clk-m10v.c
>
> diff --git a/drivers/clk/clk-m10v.c b/drivers/clk/clk-m10v.c
> new file mode 100644
> index 0000000..aa92a69
> --- /dev/null
> +++ b/drivers/clk/clk-m10v.c
> @@ -0,0 +1,671 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Socionext Inc.
> + * Copyright (C) 2016 Linaro Ltd.
> + *
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
Is this include used?
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/of.h>
> +#include <linux/spinlock.h>
> +
> +#define CLKSEL1 0x0
> +#define CLKSEL(n) (((n) - 1) * 4 + CLKSEL1)
> +
> +#define PLLCNT1 0x30
> +#define PLLCNT(n) (((n) - 1) * 4 + PLLCNT1)
> +
> +#define CLKSTOP1 0x54
> +#define CLKSTOP(n) (((n) - 1) * 4 + CLKSTOP1)
> +
> +#define CRSWR 0x8c
> +#define CRRRS 0x90
> +#define CRRSM 0x94
> +
> +#define to_m10v_mux(_hw) container_of(_hw, struct m10v_mux, hw)
> +#define to_m10v_gate(_hw) container_of(_hw, struct m10v_gate, hw)
> +#define to_m10v_div(_hw) container_of(_hw, struct m10v_div, hw)
> +#define to_m10v_pll(_hw) container_of(_hw, struct m10v_pll, hw)
> +
> +static void __iomem *clk_base;
> +static struct device_node *np_top;
> +static DEFINE_SPINLOCK(crglock);
Please make more specific names for these global variables by prefixing
with m10v_. Also consider getting rid of the iomem and np_top globals
entirely and associate those with clks differently.
> +
> +static __init void __iomem *m10v_clk_iomap(void)
> +{
> + if (clk_base)
> + return clk_base;
> +
> + np_top = of_find_compatible_node(NULL, NULL,
> + "socionext,milbeaut-m10v-clk-regs");
> + if (!np_top) {
> + pr_err("%s: CLK iomap failed!\n", __func__);
We haven't iomapped yet though.
> + return NULL;
> + }
> +
> + clk_base = of_iomap(np_top, 0);
> + of_node_put(np_top);
Would be nicer to use platform_device APIs instead of OF ones.
> +
> + return clk_base;
> +}
> +
> +struct m10v_mux {
> + struct clk_hw hw;
> + const char *cname;
> + u32 parent;
> +};
> +
> +static u8 m10v_mux_get_parent(struct clk_hw *hw)
> +{
> + struct m10v_mux *mcm = to_m10v_mux(hw);
> + struct clk_hw *parent;
> + int i;
> +
> + i = clk_hw_get_num_parents(hw);
> + while (i--) {
> + parent = clk_hw_get_parent_by_index(hw, i);
> + if (clk_hw_get_rate(parent))
> + break;
> + }
> +
> + if (i < 0) {
> + pr_info("%s:%s no parent?!\n",
> + __func__, mcm->cname);
> + i = 0;
> + }
> +
> + return i;
> +}
> +
> +static int m10v_mux_set_parent(struct clk_hw *hw, u8 index)
> +{
> + struct m10v_mux *mcm = to_m10v_mux(hw);
> +
> + mcm->parent = index;
> + return 0;
> +}
> +
> +static const struct clk_ops m10v_mux_ops = {
> + .get_parent = m10v_mux_get_parent,
> + .set_parent = m10v_mux_set_parent,
> + .determine_rate = __clk_mux_determine_rate,
> +};
> +
> +void __init m10v_clk_mux_setup(struct device_node *node)
> +{
> + const char *clk_name = node->name;
> + struct clk_init_data init;
> + const char **parent_names;
> + struct m10v_mux *mcm;
> + struct clk *clk;
> + int i, parents;
> +
> + if (!m10v_clk_iomap())
> + return;
> +
> + of_property_read_string(node, "clock-output-names", &clk_name);
> +
> + parents = of_clk_get_parent_count(node);
> + if (parents < 2) {
> + pr_err("%s: not a mux\n", clk_name);
How is this possible?
> + return;
> + }
> +
> + parent_names = kzalloc((sizeof(char *) * parents), GFP_KERNEL);
> + if (!parent_names)
> + return;
> +
> + for (i = 0; i < parents; i++)
> + parent_names[i] = of_clk_get_parent_name(node, i);
This is of_clk_parent_fill().
> +
> + mcm = kzalloc(sizeof(*mcm), GFP_KERNEL);
> + if (!mcm)
> + goto err_mcm;
> +
> + init.name = clk_name;
> + init.ops = &m10v_mux_ops;
> + init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
Please don't use CLK_IS_BASIC unless you need it.
> + init.num_parents = parents;
> + init.parent_names = parent_names;
> +
> + mcm->cname = clk_name;
> + mcm->parent = 0;
> + mcm->hw.init = &init;
> +
> + clk = clk_register(NULL, &mcm->hw);
> + if (IS_ERR(clk))
> + goto err_clk;
> +
> + of_clk_add_provider(node, of_clk_src_simple_get, clk);
> + return;
> +
> +err_clk:
> + kfree(mcm);
> +err_mcm:
> + kfree(parent_names);
> +}
> +CLK_OF_DECLARE(m10v_clk_mux, "socionext,milbeaut-m10v-clk-mux",
> + m10v_clk_mux_setup);
Any chance you can use a platform driver?
> +
> +struct m10v_pll {
> + struct clk_hw hw;
> + const char *cname;
> + const struct clk_ops ops;
> + u32 offset;
> + u32 div, mult;
> + bool ro;
> +};
> +
> +#define ST 1
> +#define SEL 2
> +
> +static void _mpg_enable(struct clk_hw *hw, unsigned int enable)
> +{
> + struct m10v_pll *mpg = to_m10v_pll(hw);
> + unsigned long flags;
> + u32 val;
> +
> + if (mpg->ro) {
> + pr_debug("%s:%d %s: read-only\n",
> + __func__, __LINE__, mpg->cname);
> + return;
> + }
> +
> + spin_lock_irqsave(&crglock, flags);
> +
> + val = readl(clk_base + PLLCNT(SEL));
> + if (enable)
> + val |= BIT(mpg->offset);
> + else
> + val &= ~BIT(mpg->offset);
> + writel(val, clk_base + PLLCNT(SEL));
> +
> + spin_unlock_irqrestore(&crglock, flags);
> +}
> +
> +static int mpg_enable(struct clk_hw *hw)
> +{
> + _mpg_enable(hw, 1);
> + return 0;
> +}
> +
> +static void mpg_disable(struct clk_hw *hw)
> +{
> + _mpg_enable(hw, 0);
> +}
> +
> +static int mpg_is_enabled(struct clk_hw *hw)
> +{
> + struct m10v_pll *mpg = to_m10v_pll(hw);
> +
> + return readl(clk_base + PLLCNT(SEL)) & (1 << mpg->offset);
> +}
> +
> +static void _mpg_prepare(struct clk_hw *hw, unsigned int on)
> +{
> + struct m10v_pll *mpg = to_m10v_pll(hw);
> + unsigned long flags;
> + u32 val;
> +
> + if (mpg->ro) {
Should have different RO ops for read-only clks.
> + pr_debug("%s:%d %s: read-only\n",
> + __func__, __LINE__, mpg->cname);
> + return;
> + }
> +
> + val = readl(clk_base + PLLCNT(ST));
> + if (!on == !(val & BIT(mpg->offset)))
> + return;
> +
> + /* disable */
Please remove obvious comments.
> + mpg_disable(hw);
> +
> + spin_lock_irqsave(&crglock, flags);
> +
> + val = readl(clk_base + PLLCNT(ST));
> + if (on)
> + val |= BIT(mpg->offset);
> + else
> + val &= ~BIT(mpg->offset);
> + writel(val, clk_base + PLLCNT(ST));
> +
> + spin_unlock_irqrestore(&crglock, flags);
> +
> + udelay(on ? 200 : 10);
> +}
> +
> +static int mpg_prepare(struct clk_hw *hw)
> +{
> + _mpg_prepare(hw, 1);
> + return 0;
> +}
> +
> +static void mpg_unprepare(struct clk_hw *hw)
> +{
> + _mpg_prepare(hw, 0);
> +}
> +
> +static int mpg_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long prate)
> +{
Why is this implemented then?
> + return 0;
> +}
> +
> +static unsigned long mpg_recalc_rate(struct clk_hw *hw,
> + unsigned long prate)
> +{
> + struct m10v_pll *mpg = to_m10v_pll(hw);
> + unsigned long long rate = prate;
> +
> + if (mpg_is_enabled(hw)) {
> + rate = (unsigned long long)prate * mpg->mult;
> + do_div(rate, mpg->div);
> + }
> +
> + return (unsigned long)rate;
> +}
> +
> +static long mpg_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *prate)
> +{
> + struct m10v_pll *mpg = to_m10v_pll(hw);
> + unsigned long long temp_rate = (unsigned long long)*prate * mpg->mult;
> +
> + if (mpg->ro)
> + return mpg_recalc_rate(hw, *prate);
> +
> + return do_div(temp_rate, mpg->div);
There shouldn't be round_rate implemented at all if the device is
'read-only' or can't change frequency because set_rate op is empty.
> +}
> +
[..]
> +
> +static void mdc_set_div(struct m10v_div *mdc, u32 div)
> +{
> + u32 off, shift, val;
> +
> + off = mdc->offset / 32 * 4;
> + shift = mdc->offset % 32;
> +
> + val = readl(clk_base + CLKSEL1 + off);
> + val &= ~(mdc->mask << shift);
> + val |= (div << shift);
> + writel(val, clk_base + CLKSEL1 + off);
> +
> + if (mdc->waitdchreq) {
> + unsigned int count = 250;
> +
> + writel(1, clk_base + CLKSEL(11));
> +
> + do {
> + udelay(1);
> + } while (--count && readl(clk_base + CLKSEL(11)) & 1);
Use readl_poll_timeout()?
> +
> + if (!count)
> + pr_err("%s:%s CLK(%d) couldn't stabilize\n",
> + __func__, mdc->cname, mdc->offset);
> + }
> +}
> +
[...]
> +
> +void __init m10v_clk_gate_setup(struct device_node *node)
> +{
> + const char *clk_name = node->name;
> + struct clk_init_data init;
> + const char *parent_name;
> + struct m10v_gate *mgc;
> + struct clk *clk;
> + u32 offset;
> + int ret;
> +
> + if (!m10v_clk_iomap())
> + return;
> +
> + of_property_read_string(node, "clock-output-names", &clk_name);
> +
> + ret = of_property_read_u32(node, "offset", &offset);
> + if (ret) {
> + pr_err("%s: missing 'offset' property\n", clk_name);
> + return;
> + }
> +
> + parent_name = of_clk_get_parent_name(node, 0);
> +
> + mgc = kzalloc(sizeof(*mgc), GFP_KERNEL);
> + if (!mgc)
> + return;
> +
> + init.name = clk_name;
> + init.ops = &m10v_gate_ops;
> + init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
> + init.parent_names = &parent_name;
> + init.num_parents = 1;
> +
> + mgc->cname = clk_name;
> + mgc->offset = offset;
> + mgc->hw.init = &init;
> + if (of_get_property(node, "read-only", NULL))
> + mgc->ro = true;
> +
> + clk = clk_register(NULL, &mgc->hw);
Please use clk_hw based registration and provider APIs.
> + if (IS_ERR(clk))
> + kfree(mgc);
> + else
> + of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +}
> +CLK_OF_DECLARE(m10v_clk_gate, "socionext,milbeaut-m10v-clk-gate",
> + m10v_clk_gate_setup);
> --
I suspect this driver will significantly change so I'm not reviewing
any further until it's sent again.
^ permalink raw reply
* Re: [PATCH] dmaengine: at_hdmac: fix memory leak in at_dma_xlate()
From: Mario Forner @ 2018-11-30 10:07 UTC (permalink / raw)
To: Richard Genoud
Cc: Ludovic Desroches, Dan Williams, Vinod Koul, Alexandre Belloni,
Nicolas Ferre, Maxime Ripard, linux-arm-kernel, dmaengine,
linux-kernel, linux-serial, stable
In-Reply-To: <20181127160635.11836-1-richard.genoud@gmail.com>
Am Dienstag, 27. November 2018, 17:06:34 CET schrieb Richard Genoud:
> The leak was found when opening/closing a serial port a great number of
> time, increasing kmalloc-32 in slabinfo.
>
> Each time the port was opened, dma_request_slave_channel() was called.
> Then, in at_dma_xlate(), atslave was allocated with devm_kzalloc() and
> never freed. (Well, it was free at module unload, but that's not what we
> want).
> So, here, kzalloc is more suited for the job since it has to be freed in
> atc_free_chan_resources().
>
> Cc: stable@vger.kernel.org
> Fixes: bbe89c8e3d59 ("at_hdmac: move to generic DMA binding")
> Reported-by: Mario Forner <m.forner@be4energy.com>
> Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
After testing I installed an updated kernel on a production machine, which
worked fine. The memory leak has been repaired successfully. There have been
no adverse side-effects.
Thank you for providing the patch.
> ---
> drivers/dma/at_hdmac.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index 7cbac6e8c113..1b7f0ca0d5cd 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -1641,6 +1641,12 @@ static void atc_free_chan_resources(struct dma_chan *chan)
> atchan->descs_allocated = 0;
> atchan->status = 0;
>
> + /*
> + * Free atslave allocated in at_dma_xlate()
> + */
> + kfree(chan->private);
> + chan->private = NULL;
> +
> dev_vdbg(chan2dev(chan), "free_chan_resources: done\n");
> }
>
> @@ -1675,7 +1681,7 @@ static struct dma_chan *at_dma_xlate(struct of_phandle_args *dma_spec,
> dma_cap_zero(mask);
> dma_cap_set(DMA_SLAVE, mask);
>
> - atslave = devm_kzalloc(&dmac_pdev->dev, sizeof(*atslave), GFP_KERNEL);
> + atslave = kzalloc(sizeof(*atslave), GFP_KERNEL);
> if (!atslave)
> return NULL;
>
>
^ permalink raw reply
* Re: [PATCH] drivers/tty: add missing of_node_put()
From: David Miller @ 2018-12-03 4:55 UTC (permalink / raw)
To: tiny.windzz; +Cc: gregkh, jslaby, sparclinux, linux-serial, linux-kernel
In-Reply-To: <20181121152254.23480-1-tiny.windzz@gmail.com>
From: Yangtao Li <tiny.windzz@gmail.com>
Date: Wed, 21 Nov 2018 10:22:54 -0500
> of_find_node_by_path() acquires a reference to the node
> returned by it and that reference needs to be dropped by its caller.
> This place is not doing this, so fix it.
>
> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] serial/sunsu: add missing of_node_put()
From: David Miller @ 2018-12-03 4:57 UTC (permalink / raw)
To: tiny.windzz; +Cc: gregkh, jslaby, sparclinux, linux-serial, linux-kernel
In-Reply-To: <20181121160615.23666-1-tiny.windzz@gmail.com>
From: Yangtao Li <tiny.windzz@gmail.com>
Date: Wed, 21 Nov 2018 11:06:15 -0500
> of_find_node_by_path() acquires a reference to the node
> returned by it and that reference needs to be dropped by its caller.
> This place is not doing this, so fix it.
>
> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
Several coding style problems and you are adding two new
leaks.
> + struct device_node *tmp;
> const char *keyb = of_get_property(ap, "keyboard", NULL);
> const char *ms = of_get_property(ap, "mouse", NULL);
Always order local variable declarations from longest to
shortest line.
>
> if (keyb) {
> - if (dp == of_find_node_by_path(keyb))
> - return SU_PORT_KBD;
> + tmp = of_find_node_by_path(keyb);
> + if (tmp && dp == tmp){
Always put a space between ")" and the "{" openning a new basic
block.
> + rc = SU_PORT_KBD;
> + goto out;
> + }
> }
> if (ms) {
> - if (dp == of_find_node_by_path(ms))
> - return SU_PORT_MS;
> + tmp = of_find_node_by_path(ms);
> + if (tmp && dp == tmp){
> + rc = SU_PORT_MS;
> + goto out;
> + }
> }
> }
>
> - return SU_PORT_PORT;
> +out:
> + of_node_put(ap);
> + return rc;
Now you have two references, one held by 'ap' and one held by 'tmp'.
You are not releasing the one held by 'tmp', and you must release
'tmp' even if it equals 'dp'.
It's pretty obvious you have no way by which to test these changes and
therefore are not doing so.
^ permalink raw reply
* Re: [PATCH 02/14] dt-bindings: soc: milbeaut: Add Milbeaut trampoline description
From: Sugaya, Taichi @ 2018-12-03 7:42 UTC (permalink / raw)
To: Stephen Boyd, devicetree, linux-arm-kernel, linux-clk,
linux-kernel, linux-serial
Cc: Michael Turquette, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Daniel Lezcano, Thomas Gleixner, Russell King, Jiri Slaby,
Masami Hiramatsu, Jassi Brar
In-Reply-To: <154356579701.88331.5043467509900444879@swboyd.mtv.corp.google.com>
Hi,
On 2018/11/30 17:16, Stephen Boyd wrote:
> Quoting Sugaya, Taichi (2018-11-29 04:24:51)
>> On 2018/11/28 11:01, Stephen Boyd wrote:
>>> Quoting Sugaya Taichi (2018-11-18 17:01:07)
>>>> create mode 100644 Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
>>>> new file mode 100644
>>>> index 0000000..f5d906c
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
>>>> @@ -0,0 +1,12 @@
>>>> +Socionext M10V SMP trampoline driver binding
>>>> +
>>>> +This is a driver to wait for sub-cores while boot process.
>>>> +
>>>> +- compatible: should be "socionext,smp-trampoline"
>>>> +- reg: should be <0x4C000100 0x100>
>>>> +
>>>> +EXAMPLE
>>>> + trampoline: trampoline@0x4C000100 {
>>> Drop the 0x part of unit addresses.
>>
>> Okay.
>>
>>
>>>> + compatible = "socionext,smp-trampoline";
>>>> + reg = <0x4C000100 0x100>;
>>> Looks like a software construct, which we wouldn't want to put into DT
>>> this way. DT doesn't describe drivers.
>> We would like to use this node only getting the address of the
>> trampoline area
>> in which sub-cores wait. (They have finished to go to this area in previous
>> bootloader process.)
>
> Is this area part of memory, or a special SRAM? If it's part of memory,
> I would expect this node to be under the reserved-memory node and
> pointed to by some other node that uses this region. Could even be the
> CPU nodes.
Yes, 0x4C000100 is a part of memory under the reserved-memory node. So
we would like to use the SRAM ( allocated 0x00000000 ) area instead.
BTW, sorry, the trampoline address of this example is simply wrong. We
were going to use a part of the SRAM from the beginning.
>
>>
>> So should we embed the constant value in source codes instead of getting
>> from
>> DT because the address is constant at the moment? Or is there other
>> approach?
>>
>
> If it's constant then that also works. Why does it need to come from DT
> at all then?
We think it is not good to embed constant value in driver codes and do
not have another way...
Are there better ways?
Thanks
Sugaya Taichi
>
^ permalink raw reply
* Re: [PATCH 06/14] dt-bindings: clock: milbeaut: add Milbeaut clock description
From: Sugaya, Taichi @ 2018-12-03 8:08 UTC (permalink / raw)
To: Stephen Boyd, devicetree, linux-arm-kernel, linux-clk,
linux-kernel, linux-serial
Cc: Michael Turquette, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Daniel Lezcano, Thomas Gleixner, Russell King, Jiri Slaby,
Masami Hiramatsu, Jassi Brar
In-Reply-To: <154356596981.88331.14415961625410424962@swboyd.mtv.corp.google.com>
Hi,
Thank you for your comments.
On 2018/11/30 17:19, Stephen Boyd wrote:
> Quoting Sugaya Taichi (2018-11-18 17:01:11)
>> Add DT bindings document for Milbeaut clock.
>>
>> Signed-off-by: Sugaya Taichi <sugaya.taichi@socionext.com>
>> ---
>> .../devicetree/bindings/clock/milbeaut-clock.txt | 93 ++++++++++++++++++++++
>> 1 file changed, 93 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/milbeaut-clock.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/milbeaut-clock.txt b/Documentation/devicetree/bindings/clock/milbeaut-clock.txt
>> new file mode 100644
>> index 0000000..5c093c8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/milbeaut-clock.txt
>> @@ -0,0 +1,93 @@
>> +Milbeaut M10V Clock Controller Binding
>> +----------------------------------------
>> +Milbeaut clock controller is consists of few oscillators, PLL, multiplexer
>> +and few divider modules
>> +
>> +This binding uses common clock bindings
>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +Required properties:
>> +- compatible: shall be "socionext,milbeaut-m10v-clk-regs"
>> +- reg: shall contain base address and length of clock registers
>> +- #clock-cells: shall be 0
>> +
>> +Example:
>> + m10v-clk-tree@ {
>> + compatible = "socionext,milbeaut-m10v-clk-regs";
>> + reg = <0x1d021000 0x4000>;
>> +
>> + clocks {
>> + #address-cells = <0>;
>> + #size-cells = <0>;
>> +
>> + uclk40xi: uclk40xi {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <40000000>;
>> + };
>> + };
>
> This style of binding is highly discouraged. We don't describe each and
> every clk in DT, we describe clk controllers and their outputs and
> inputs in DT. The driver is the place where the clock controller
> describes the internal clk topology of that controller. Also, fixed
> frequency clks are typically oscillators and those would come from the
> board dts file, but otherwise I wouldn't expect to see fixed frequency
> clks in DT.
I understand. Received the similar comment from Rob in the DT part also.
>
>> + }
>> +
>> +The clock consumer shall specify the desired clock-output of the clock
>> +controller (as defined in [2]) by specifying output-id in its "clock"
>> +phandle cell
>> +[2] arch/arm/boot/dts/milbeaut-m10v-clk.h
>> +
> [...]
>> +
>> +Example
>> + piclk_mux_0: spiclk_mux_0 {
>> + compatible = "socionext,m10v-clk-div";
>> + #clock-cells = <0>;
>> + clocks = <&pll10_div_1_2>;
>> + offset = <bSPICLK>;
>> + mask = <0x3>;
>> + ratios = <4 0x5 2 0x4>;
>> + };
>> +
>> + pll10: pll10 {
>> + compatible = "socionext,m10v-pll-fixed-factor";
>> + #clock-cells = <0>;
>> + clocks = <&uclk40xi>;
>> + offset = <10>;
>> + clock-div = <5>;
>> + clock-mult = <108>;
>> + };
>> +
>> + emmcclk: emmcclk {
>> + compatible = "socionext,m10v-clk-div";
>> + #clock-cells = <0>;
>> + clocks = <&pll11>;
>> + offset = <bEMMCCLK>;
>> + mask = <0x3>;
>> + ratios = <15 0x7 10 0x6 9 0x5 8 0x4>;
>
> Yeah, please no. This whole binding needs a rewrite to not have one node
> per clk.
OK, I will renew the binding.
Thanks.
Sugaya Taichi
>
^ permalink raw reply
* Re: [PATCH 02/14] dt-bindings: soc: milbeaut: Add Milbeaut trampoline description
From: Rob Herring @ 2018-12-03 15:49 UTC (permalink / raw)
To: sugaya.taichi
Cc: Stephen Boyd, devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-clk, linux-kernel@vger.kernel.org, open list:SERIAL DRIVERS,
Michael Turquette, Mark Rutland, Greg Kroah-Hartman,
Daniel Lezcano, Thomas Gleixner, Russell King, Jiri Slaby,
Masami Hiramatsu, Jassi Brar
In-Reply-To: <f5917f71-f15b-a92d-649c-e3f5f5f10698@socionext.com>
On Mon, Dec 3, 2018 at 1:42 AM Sugaya, Taichi
<sugaya.taichi@socionext.com> wrote:
>
> Hi,
>
> On 2018/11/30 17:16, Stephen Boyd wrote:
> > Quoting Sugaya, Taichi (2018-11-29 04:24:51)
> >> On 2018/11/28 11:01, Stephen Boyd wrote:
> >>> Quoting Sugaya Taichi (2018-11-18 17:01:07)
> >>>> create mode 100644 Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
> >>>> new file mode 100644
> >>>> index 0000000..f5d906c
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
> >>>> @@ -0,0 +1,12 @@
> >>>> +Socionext M10V SMP trampoline driver binding
> >>>> +
> >>>> +This is a driver to wait for sub-cores while boot process.
> >>>> +
> >>>> +- compatible: should be "socionext,smp-trampoline"
> >>>> +- reg: should be <0x4C000100 0x100>
> >>>> +
> >>>> +EXAMPLE
> >>>> + trampoline: trampoline@0x4C000100 {
> >>> Drop the 0x part of unit addresses.
> >>
> >> Okay.
> >>
> >>
> >>>> + compatible = "socionext,smp-trampoline";
> >>>> + reg = <0x4C000100 0x100>;
> >>> Looks like a software construct, which we wouldn't want to put into DT
> >>> this way. DT doesn't describe drivers.
> >> We would like to use this node only getting the address of the
> >> trampoline area
> >> in which sub-cores wait. (They have finished to go to this area in previous
> >> bootloader process.)
> >
> > Is this area part of memory, or a special SRAM? If it's part of memory,
> > I would expect this node to be under the reserved-memory node and
> > pointed to by some other node that uses this region. Could even be the
> > CPU nodes.
>
> Yes, 0x4C000100 is a part of memory under the reserved-memory node. So
> we would like to use the SRAM ( allocated 0x00000000 ) area instead.
> BTW, sorry, the trampoline address of this example is simply wrong. We
> were going to use a part of the SRAM from the beginning.
>
> >
> >>
> >> So should we embed the constant value in source codes instead of getting
> >> from
> >> DT because the address is constant at the moment? Or is there other
> >> approach?
> >>
> >
> > If it's constant then that also works. Why does it need to come from DT
> > at all then?
>
> We think it is not good to embed constant value in driver codes and do
> not have another way...
> Are there better ways?
If this is just memory, can you use the standard spin-table binding in
the DT spec? There are some requirements like 64-bit values even on
32-bit machines (though this gets violated).
Rob
^ permalink raw reply
* Re: [PATCH v2 06/15] arm: dts: Add devicetree for OrangePi 2G IoT board
From: Rob Herring @ 2018-12-03 15:53 UTC (permalink / raw)
To: Manivannan Sadhasivam, Arnd Bergmann, Olof Johansson
Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, Daniel Lezcano,
Greg Kroah-Hartman, Jiri Slaby, Andreas Färber,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel@vger.kernel.org, devicetree,
open list:SERIAL DRIVERS, Amit Kucheria, Linus Walleij,
zhao_steven, overseas.sales
In-Reply-To: <20181121033652.12247-7-manivannan.sadhasivam@linaro.org>
On Tue, Nov 20, 2018 at 9:38 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> Add initial devicetree support for OrangePi 2G IoT board from Xunlong.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> arch/arm/boot/dts/Makefile | 2 +
> .../boot/dts/rda8810pl-orangepi-2g-iot.dts | 40 +++++++++++++++++++
> 2 files changed, 42 insertions(+)
> create mode 100644 arch/arm/boot/dts/rda8810pl-orangepi-2g-iot.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index b0e966d625b9..a0fdad8f10dd 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -806,6 +806,8 @@ dtb-$(CONFIG_ARCH_QCOM) += \
> qcom-msm8974-sony-xperia-castor.dtb \
> qcom-msm8974-sony-xperia-honami.dtb \
> qcom-mdm9615-wp8548-mangoh-green.dtb
> +dtb-$(CONFIG_ARCH_RDA) += \
> + rda8810pl-orangepi-2g-iot.dtb
> dtb-$(CONFIG_ARCH_REALVIEW) += \
Question for Arnd/Olof: can we *please* start putting new SoC families
in sub-dirs?
Rob
^ permalink raw reply
* Re: [PATCH v3] tty: serial: qcom_geni_serial: Fix softlock
From: Stephen Boyd @ 2018-12-03 16:22 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Evan Green, Doug Anderson, linux-kernel, linux-serial, Ryan Case
In-Reply-To: <20181130021840.75861-1-ryandcase@chromium.org>
Quoting Ryan Case (2018-11-29 18:18:40)
> Transfers were being divided into device FIFO sized (64 byte max)
> operations which would poll for completion within a spin_lock_irqsave /
> spin_unlock_irqrestore block. This both made things slow by waiting for
> the FIFO to completely drain before adding further data and would also
> result in softlocks on large transmissions.
>
> This patch allows larger transfers with continuous FIFO additions as
> space becomes available and removes polling from the interrupt handler.
>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply
* Re: [PATCH v2 06/15] arm: dts: Add devicetree for OrangePi 2G IoT board
From: Olof Johansson @ 2018-12-03 17:11 UTC (permalink / raw)
To: Rob Herring
Cc: Manivannan Sadhasivam, Arnd Bergmann, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Daniel Lezcano, Greg Kroah-Hartman,
Jiri Slaby, Andreas Färber, Linux ARM Mailing List,
Linux Kernel Mailing List, DTML, linux-serial, Amit Kucheria,
LinusW, zhao_steven, overseas.sales
In-Reply-To: <CAL_JsqJ=88ORdgbRDn+VWk9=ydShidqwOxdAMfDbH4W2drnXzA@mail.gmail.com>
On Mon, Dec 3, 2018 at 7:54 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Nov 20, 2018 at 9:38 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > Add initial devicetree support for OrangePi 2G IoT board from Xunlong.
> >
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> > arch/arm/boot/dts/Makefile | 2 +
> > .../boot/dts/rda8810pl-orangepi-2g-iot.dts | 40 +++++++++++++++++++
> > 2 files changed, 42 insertions(+)
> > create mode 100644 arch/arm/boot/dts/rda8810pl-orangepi-2g-iot.dts
> >
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index b0e966d625b9..a0fdad8f10dd 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -806,6 +806,8 @@ dtb-$(CONFIG_ARCH_QCOM) += \
> > qcom-msm8974-sony-xperia-castor.dtb \
> > qcom-msm8974-sony-xperia-honami.dtb \
> > qcom-mdm9615-wp8548-mangoh-green.dtb
> > +dtb-$(CONFIG_ARCH_RDA) += \
> > + rda8810pl-orangepi-2g-iot.dtb
> > dtb-$(CONFIG_ARCH_REALVIEW) += \
>
> Question for Arnd/Olof: can we *please* start putting new SoC families
> in sub-dirs?
I think we're at a point where it's becoming quite awkward to keep it
in a flat directory, yes.
Best way to handle this is usually right before the closing of a merge
window, so let's do it then. It'll be a conflict-ridden mess if we try
to stage something in -next, so I'd rather just do it directly in our
tree as a one-shot thing.
-Olof
^ permalink raw reply
* Re: [PATCH v2 06/15] arm: dts: Add devicetree for OrangePi 2G IoT board
From: Rob Herring @ 2018-12-03 17:21 UTC (permalink / raw)
To: Olof Johansson
Cc: Manivannan Sadhasivam, Arnd Bergmann, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Daniel Lezcano, Greg Kroah-Hartman,
Jiri Slaby, Andreas Färber,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel@vger.kernel.org, devicetree,
open list:SERIAL DRIVERS, Amit Kucheria, Linus Walleij,
zhao_steven, overseas.sales
In-Reply-To: <CAOesGMgSxwcmBLDvxsvcCJ9-NvJ3-wK8fZBZV2ZNuzAa1idi3w@mail.gmail.com>
On Mon, Dec 3, 2018 at 11:11 AM Olof Johansson <olof@lixom.net> wrote:
>
> On Mon, Dec 3, 2018 at 7:54 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Tue, Nov 20, 2018 at 9:38 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > Add initial devicetree support for OrangePi 2G IoT board from Xunlong.
> > >
> > > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > > arch/arm/boot/dts/Makefile | 2 +
> > > .../boot/dts/rda8810pl-orangepi-2g-iot.dts | 40 +++++++++++++++++++
> > > 2 files changed, 42 insertions(+)
> > > create mode 100644 arch/arm/boot/dts/rda8810pl-orangepi-2g-iot.dts
> > >
> > > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > > index b0e966d625b9..a0fdad8f10dd 100644
> > > --- a/arch/arm/boot/dts/Makefile
> > > +++ b/arch/arm/boot/dts/Makefile
> > > @@ -806,6 +806,8 @@ dtb-$(CONFIG_ARCH_QCOM) += \
> > > qcom-msm8974-sony-xperia-castor.dtb \
> > > qcom-msm8974-sony-xperia-honami.dtb \
> > > qcom-mdm9615-wp8548-mangoh-green.dtb
> > > +dtb-$(CONFIG_ARCH_RDA) += \
> > > + rda8810pl-orangepi-2g-iot.dtb
> > > dtb-$(CONFIG_ARCH_REALVIEW) += \
> >
> > Question for Arnd/Olof: can we *please* start putting new SoC families
> > in sub-dirs?
>
> I think we're at a point where it's becoming quite awkward to keep it
> in a flat directory, yes.
>
> Best way to handle this is usually right before the closing of a merge
> window, so let's do it then. It'll be a conflict-ridden mess if we try
> to stage something in -next, so I'd rather just do it directly in our
> tree as a one-shot thing.
While I'd like to see the whole thing converted, I was only asking
about this one platform. That should be doable now, right?
Rob
^ permalink raw reply
* Re: [PATCH v2 06/15] arm: dts: Add devicetree for OrangePi 2G IoT board
From: Olof Johansson @ 2018-12-03 17:53 UTC (permalink / raw)
To: Rob Herring
Cc: Manivannan Sadhasivam, Arnd Bergmann, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Daniel Lezcano, Greg Kroah-Hartman,
Jiri Slaby, Andreas Färber, Linux ARM Mailing List,
Linux Kernel Mailing List, DTML, linux-serial, Amit Kucheria,
LinusW, zhao_steven, overseas.sales
In-Reply-To: <CAL_JsqLZY_5rZ35YSGeH7wmEZoLP1TjG+0n4BWGiUBwOFSq-Vw@mail.gmail.com>
On Mon, Dec 3, 2018 at 9:21 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Dec 3, 2018 at 11:11 AM Olof Johansson <olof@lixom.net> wrote:
> >
> > On Mon, Dec 3, 2018 at 7:54 AM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Tue, Nov 20, 2018 at 9:38 PM Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org> wrote:
> > > >
> > > > Add initial devicetree support for OrangePi 2G IoT board from Xunlong.
> > > >
> > > > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > > arch/arm/boot/dts/Makefile | 2 +
> > > > .../boot/dts/rda8810pl-orangepi-2g-iot.dts | 40 +++++++++++++++++++
> > > > 2 files changed, 42 insertions(+)
> > > > create mode 100644 arch/arm/boot/dts/rda8810pl-orangepi-2g-iot.dts
> > > >
> > > > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > > > index b0e966d625b9..a0fdad8f10dd 100644
> > > > --- a/arch/arm/boot/dts/Makefile
> > > > +++ b/arch/arm/boot/dts/Makefile
> > > > @@ -806,6 +806,8 @@ dtb-$(CONFIG_ARCH_QCOM) += \
> > > > qcom-msm8974-sony-xperia-castor.dtb \
> > > > qcom-msm8974-sony-xperia-honami.dtb \
> > > > qcom-mdm9615-wp8548-mangoh-green.dtb
> > > > +dtb-$(CONFIG_ARCH_RDA) += \
> > > > + rda8810pl-orangepi-2g-iot.dtb
> > > > dtb-$(CONFIG_ARCH_REALVIEW) += \
> > >
> > > Question for Arnd/Olof: can we *please* start putting new SoC families
> > > in sub-dirs?
> >
> > I think we're at a point where it's becoming quite awkward to keep it
> > in a flat directory, yes.
> >
> > Best way to handle this is usually right before the closing of a merge
> > window, so let's do it then. It'll be a conflict-ridden mess if we try
> > to stage something in -next, so I'd rather just do it directly in our
> > tree as a one-shot thing.
>
> While I'd like to see the whole thing converted, I was only asking
> about this one platform. That should be doable now, right?
I'd rather apply this to current scheme and then move everything once,
instead of having maintainers go "Oh, I guess I need to move mine" and
we end up with an onslaught of conflicting pull requests to move
things apart.
-Olof
^ 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