Linux Serial subsystem development
 help / color / mirror / Atom feed
* [PATCH v2 1/4] dt-bindings: arm: Add bindings for Mediatek MT8183 SoC Platform
From: Erin Lo @ 2018-05-14 10:22 UTC (permalink / raw)
  To: Matthias Brugger, Rob Herring, Mark Rutland, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Greg Kroah-Hartman
  Cc: devicetree, srv_heupstream, linux-kernel, linux-serial,
	linux-mediatek, linux-arm-kernel, yingjoe.chen, erin.lo,
	mars.cheng
In-Reply-To: <1526293351-32794-1-git-send-email-erin.lo@mediatek.com>

This adds dt-binding documentation of cpu for Mediatek MT8183.

Signed-off-by: Erin Lo <erin.lo@mediatek.com>
---
 Documentation/devicetree/bindings/arm/mediatek.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/mediatek.txt b/Documentation/devicetree/bindings/arm/mediatek.txt
index 7d21ab3..2754535 100644
--- a/Documentation/devicetree/bindings/arm/mediatek.txt
+++ b/Documentation/devicetree/bindings/arm/mediatek.txt
@@ -19,6 +19,7 @@ compatible: Must contain one of
    "mediatek,mt8127"
    "mediatek,mt8135"
    "mediatek,mt8173"
+   "mediatek,mt8183"
 
 
 Supported boards:
@@ -73,3 +74,6 @@ Supported boards:
 - MTK mt8173 tablet EVB:
     Required root node properties:
       - compatible = "mediatek,mt8173-evb", "mediatek,mt8173";
+- Evaluation board for MT8183:
+    Required root node properties:
+      - compatible = "mediatek,mt8183-evb", "mediatek,mt8183";
-- 
1.9.1

^ permalink raw reply related

* [PATCH v2 0/4] Add basic support for Mediatek MT8183 SoC
From: Erin Lo @ 2018-05-14 10:22 UTC (permalink / raw)
  To: Matthias Brugger, Rob Herring, Mark Rutland, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Greg Kroah-Hartman
  Cc: devicetree, srv_heupstream, linux-kernel, linux-serial,
	linux-mediatek, linux-arm-kernel, yingjoe.chen, erin.lo,
	mars.cheng

MT8183 is a SoC based on 64bit ARMv8 architecture.
It contains 4 CA53 and 4 CA73 cores.
MT8183 share many HW IP with MT65xx series.
This patchset was tested on MT8183 evaluation board, and boot to shell ok.

This series contains document bindings, device tree including interrupt, uart.

Change in v2:
1. Split dt-bindings into different patches
2. Correct bindings for supported SoCs (mtk-uart.txt)

Ben Ho (1):
  arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and
    Makefile

Erin Lo (3):
  dt-bindings: arm: Add bindings for Mediatek MT8183 SoC Platform
  dt-bindings: mtk-sysirq: Add compatible for Mediatek MT8183
  dt-bindings: serial: Add compatible for Mediatek MT8183

 Documentation/devicetree/bindings/arm/mediatek.txt |   4 +
 .../interrupt-controller/mediatek,sysirq.txt       |   1 +
 .../devicetree/bindings/serial/mtk-uart.txt        |   1 +
 arch/arm64/boot/dts/mediatek/Makefile              |   1 +
 arch/arm64/boot/dts/mediatek/mt8183-evb.dts        |  31 ++++
 arch/arm64/boot/dts/mediatek/mt8183.dtsi           | 178 +++++++++++++++++++++
 6 files changed, 216 insertions(+)
 create mode 100644 arch/arm64/boot/dts/mediatek/mt8183-evb.dts
 create mode 100644 arch/arm64/boot/dts/mediatek/mt8183.dtsi

--
1.9.1

^ permalink raw reply

* Re: [PATCH v2] tty: pl011: Avoid spuriously stuck-off interrupts
From: Wei Xu @ 2018-05-14  9:18 UTC (permalink / raw)
  To: Dave Martin, linux-arm-kernel
  Cc: Peter Maydell, Andrew Jones, Ciro Santilli, Linus Walleij,
	Russell King, Linuxarm, linux-serial, Greg KH
In-Reply-To: <1525972103-26691-1-git-send-email-Dave.Martin@arm.com>

Hi Dave,

On 2018/5/10 18:08, Dave Martin wrote:
> Commit 9b96fbacda34 ("serial: PL011: clear pending interrupts")
> clears the RX and receive timeout interrupts on pl011 startup, to
> avoid a screaming-interrupt scenario that can occur when the
> firmware or bootloader leaves these interrupts asserted.
> 
> This has been noted as an issue when running Linux on qemu [1].
> 
> Unfortunately, the above fix seems to lead to potential
> misbehaviour if the RX FIFO interrupt is asserted _non_ spuriously
> on driver startup, if the RX FIFO is also already full to the
> trigger level.
> 
> Clearing the RX FIFO interrupt does not change the FIFO fill level.
> In this scenario, because the interrupt is now clear and because
> the FIFO is already full to the trigger level, no new assertion of
> the RX FIFO interrupt can occur unless the FIFO is drained back
> below the trigger level.  This never occurs because the pl011
> driver is waiting for an RX FIFO interrupt to tell it that there is
> something to read, and does not read the FIFO at all until that
> interrupt occurs.
> 
> Thus, simply clearing "spurious" interrupts on startup may be
> misguided, since there is no way to be sure that the interrupts are
> truly spurious, and things can go wrong if they are not.
> 
> This patch instead clears the interrupt condition by draining the
> RX FIFO during UART startup, after clearing any potentially
> spurious interrupt.  This should ensure that an interrupt will
> definitely be asserted if the RX FIFO subsequently becomes
> sufficiently full.
> 
> The drain is done at the point of enabling interrupts only.  This
> means that it will occur any time the UART is newly opened through
> the tty layer.  It will not apply to polled-mode use of the UART by
> kgdboc: since that scenario cannot use interrupts by design, this
> should not matter.  kgdboc will interact badly with "normal" use of
> the UART in any case: this patch makes no attempt to paper over
> such issues.
> 
> This patch does not attempt to address the case where the RX FIFO
> fills faster than it can be drained: that is a pathological
> hardware design problem that is beyond the scope of the driver to
> work around.  As a failsafe, the number of poll iterations for
> draining the FIFO is limited to twice the FIFO size.  This will
> ensure that the kernel at least boots even if it is impossible to
> drain the FIFO for some reason.
> 
> [1] [Qemu-devel] [Qemu-arm] [PATCH] pl011: do not put into fifo
> before enabled the interruption
> https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06446.html
> 
> Reported-by: Wei Xu <xuwei5@hisilicon.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 9b96fbacda34 ("serial: PL011: clear pending interrupts")
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

Thanks!
Tested on hisilicon D05 board.

Tested-by: Wei Xu <xuwei5@hisilicon.com>

Best Regards,
Wei

> 
> ---
> 
> Changes since v1 [1]
> 
>  * Deleted Andrew Jones' Reviewed/Tested-bys due to the following
>    change.
> 
>    If you can please retest that the updated patch fixes your
>    issue that would be appreciated.
> 
> Suggested by Russell King:
> 
>  * Only drain 2 * FIFO size, to avoid going into an infinite spin
>    if something does wrong.  If the FIFO still couldn't be drained
>    sufficiently then pl011 RX won't work properly, but the kernel
>    will at least boot.
> 
> 
> [1] [PATCH] tty: pl011: Avoid spuriously stuck-off interrupts
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/574362.html
> 
> ---
>  drivers/tty/serial/amba-pl011.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 4b40a5b..ebd33c0 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1727,10 +1727,26 @@ static int pl011_allocate_irq(struct uart_amba_port *uap)
>   */
>  static void pl011_enable_interrupts(struct uart_amba_port *uap)
>  {
> +	unsigned int i;
> +
>  	spin_lock_irq(&uap->port.lock);
>  
>  	/* Clear out any spuriously appearing RX interrupts */
>  	pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
> +
> +	/*
> +	 * RXIS is asserted only when the RX FIFO transitions from below
> +	 * to above the trigger threshold.  If the RX FIFO is already
> +	 * full to the threshold this can't happen and RXIS will now be
> +	 * stuck off.  Drain the RX FIFO explicitly to fix this:
> +	 */
> +	for (i = 0; i < uap->fifosize * 2; ++i) {
> +		if (pl011_read(uap, REG_FR) & UART01x_FR_RXFE)
> +			break;
> +
> +		pl011_read(uap, REG_DR);
> +	}
> +
>  	uap->im = UART011_RTIM;
>  	if (!pl011_dma_rx_running(uap))
>  		uap->im |= UART011_RXIM;
> 

^ permalink raw reply

* Re: [PATCH 6/8] serial: Add Tegra Combined UART driver
From: Mikko Perttunen @ 2018-05-14  7:36 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mikko Perttunen, Rob Herring, Mark Rutland, Jassi Brar,
	Greg Kroah-Hartman, Thierry Reding, Jon Hunter, araza, devicetree,
	open list:SERIAL DRIVERS, linux-tegra, linux-arm Mailing List,
	Linux Kernel Mailing List
In-Reply-To: <CAHp75VeJcqaiWUVr=oXEH5YurY3yCDdE1eeoWumo6jCa_4PWEQ@mail.gmail.com>

On 14.05.2018 01:20, Andy Shevchenko wrote:
> On Sun, May 13, 2018 at 9:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote:
>> On 05/13/2018 05:16 PM, Andy Shevchenko wrote:
>>>
>>> On Tue, May 8, 2018 at 2:44 PM, Mikko Perttunen <mperttunen@nvidia.com>
>>> wrote:
>>>>
>>>> The Tegra Combined UART (TCU) is a mailbox-based mechanism that allows
>>>> multiplexing multiple "virtual UARTs" into a single hardware serial
>>>> port. The TCU is the primary serial port on Tegra194 devices.
>>>>
>>>> Add a TCU driver utilizing the mailbox framework, as the used mailboxes
>>>> are part of Tegra HSP blocks that are already controlled by the Tegra
>>>> HSP mailbox driver.
>
>>>> +static void tegra_tcu_uart_set_mctrl(struct uart_port *port, unsigned
>>>> int mctrl)
>>>> +{
>>>
>>>
>>>> +       (void)port;
>>>> +       (void)mctrl;
>>>
>>>
>>> Huh?
>>
>>
>> The serial core calls these callbacks without checking if they are set. They
>> don't make sense for this driver so they are stubbed out.
>
> My question why do you need these ugly lines? I'm pretty sure no other
> driver with stubs using such style.

It's my personal style, being explicit about unused variables in this 
way - I don't consider them ugly. But I can certainly remove them for 
the next version.

>
>>>> +}
>
>>>> +               if (written == 3) {
>>>> +                       value |= 3 << 24;
>>>> +                       value |= BIT(26);
>>>> +                       mbox_send_message(tcu->tx, &value);
>>>
>>>
>>>> +               }
>>>
>>>
>>> (1)
>>>
>>>> +       }
>>>> +
>>>> +       if (written) {
>>>> +               value |= written << 24;
>>>> +               value |= BIT(26);
>>>> +               mbox_send_message(tcu->tx, &value);
>>>> +       }
>>>
>>>
>>> (2)
>>>
>>> These are code duplications.
>>
>>
>> Indeed - the length of the duplicated code is so short, and the instances
>> are so close to each other, that I don't find it necessary (or clearer) to
>> have an extra function.
>
> It makes sense. Consider to refactor other way w/o duplication then.

I'll see if I can refactor it out.

>
>>>> +static void tegra_tcu_uart_set_termios(struct uart_port *port,
>>>> +                                      struct ktermios *new,
>>>> +                                      struct ktermios *old)
>>>> +{
>>>> +       (void)port;
>>>> +       (void)new;
>>>> +       (void)old;
>>>> +}
>>>
>>>
>>> Remove those unused stub contents.
>>
>>
>> Sure. I had these here so that we don't get unused parameter warnings, but I
>> can just as well remove the parameter names.
>
> What warnings? How did you get them? We have them disabled as far as I
> know even with W=1.

May be - it's just a habit, maybe from other projects where the warning 
is enabled.

>
>>
>>>
>>>> +       return uart_set_options(&tegra_tcu_uart_port, cons,
>>>> +                               115200, 'n', 8, 'n');
>>>
>>>
>>> Can't it be one line?
>>
>>
>> It would be a total of 81 characters in length on one line, so no.
>
> So, yes. 1 character doesn't prevent us make the readability better.
> Please, put to one line.
>

Ok, I'll change this.

Thanks,
Mikko

^ permalink raw reply

* Re: [PATCH 6/8] serial: Add Tegra Combined UART driver
From: Andy Shevchenko @ 2018-05-13 22:20 UTC (permalink / raw)
  To: Mikko Perttunen
  Cc: Mikko Perttunen, Rob Herring, Mark Rutland, Jassi Brar,
	Greg Kroah-Hartman, Thierry Reding, Jon Hunter, araza, devicetree,
	open list:SERIAL DRIVERS, linux-tegra, linux-arm Mailing List,
	Linux Kernel Mailing List
In-Reply-To: <662ce972-56a9-9daa-d332-faa08b89c05a@kapsi.fi>

On Sun, May 13, 2018 at 9:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote:
> On 05/13/2018 05:16 PM, Andy Shevchenko wrote:
>>
>> On Tue, May 8, 2018 at 2:44 PM, Mikko Perttunen <mperttunen@nvidia.com>
>> wrote:
>>>
>>> The Tegra Combined UART (TCU) is a mailbox-based mechanism that allows
>>> multiplexing multiple "virtual UARTs" into a single hardware serial
>>> port. The TCU is the primary serial port on Tegra194 devices.
>>>
>>> Add a TCU driver utilizing the mailbox framework, as the used mailboxes
>>> are part of Tegra HSP blocks that are already controlled by the Tegra
>>> HSP mailbox driver.

>>> +static void tegra_tcu_uart_set_mctrl(struct uart_port *port, unsigned
>>> int mctrl)
>>> +{
>>
>>
>>> +       (void)port;
>>> +       (void)mctrl;
>>
>>
>> Huh?
>
>
> The serial core calls these callbacks without checking if they are set. They
> don't make sense for this driver so they are stubbed out.

My question why do you need these ugly lines? I'm pretty sure no other
driver with stubs using such style.

>>> +}

>>> +               if (written == 3) {
>>> +                       value |= 3 << 24;
>>> +                       value |= BIT(26);
>>> +                       mbox_send_message(tcu->tx, &value);
>>
>>
>>> +               }
>>
>>
>> (1)
>>
>>> +       }
>>> +
>>> +       if (written) {
>>> +               value |= written << 24;
>>> +               value |= BIT(26);
>>> +               mbox_send_message(tcu->tx, &value);
>>> +       }
>>
>>
>> (2)
>>
>> These are code duplications.
>
>
> Indeed - the length of the duplicated code is so short, and the instances
> are so close to each other, that I don't find it necessary (or clearer) to
> have an extra function.

It makes sense. Consider to refactor other way w/o duplication then.

>>> +static void tegra_tcu_uart_set_termios(struct uart_port *port,
>>> +                                      struct ktermios *new,
>>> +                                      struct ktermios *old)
>>> +{
>>> +       (void)port;
>>> +       (void)new;
>>> +       (void)old;
>>> +}
>>
>>
>> Remove those unused stub contents.
>
>
> Sure. I had these here so that we don't get unused parameter warnings, but I
> can just as well remove the parameter names.

What warnings? How did you get them? We have them disabled as far as I
know even with W=1.

>
>>
>>> +       return uart_set_options(&tegra_tcu_uart_port, cons,
>>> +                               115200, 'n', 8, 'n');
>>
>>
>> Can't it be one line?
>
>
> It would be a total of 81 characters in length on one line, so no.

So, yes. 1 character doesn't prevent us make the readability better.
Please, put to one line.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply

* Re: serial: custom baud rate
From: Alan Cox @ 2018-05-13 19:57 UTC (permalink / raw)
  To: Grant Edwards; +Cc: linux-kernel, linux-serial
In-Reply-To: <pcfka2$gie$1@blaine.gmane.org>

On Thu, 3 May 2018 18:27:14 +0000 (UTC)
Grant Edwards <grant.b.edwards@gmail.com> wrote:

> On 2018-05-03, Muni Sekhar <munisekharrms@gmail.com> wrote:
> 
> > If I need to set a custom baud rates(e.g. 14400, 128000, 256000), does
> > Linux serial framework has any supporting method?  
> 
> Sure, use the termios2 structure instead of the termios structure:
> 
>   #include <linux/termios.h>
> 
>   struct termios2 t;
> 
>   ioctl(fd, TCGETS2, &t)
> 
>   t.c_cflag &= ~CBAUD;
>   t.c_cflag |= BOTHER;
>   t.c_ispeed = baud;
>   t.c_ospeed = baud;
> 
>   ioctl(fd, TCSETS2, &t)
> 
> [Not all devices/drivers support termios2]

That shouldn't be true - all devices get passed ispeed/ospeed and
everything in tree was using the correct fields as far as I could tell
last time I checked this

Alan

^ permalink raw reply

* Re: [PATCH 6/8] serial: Add Tegra Combined UART driver
From: Mikko Perttunen @ 2018-05-13 18:06 UTC (permalink / raw)
  To: Jassi Brar, Mikko Perttunen
  Cc: Rob Herring, Mark Rutland, Greg KH, Thierry Reding, Jon Hunter,
	araza, Devicetree List, linux-serial, linux-tegra,
	", linux-arm-kernel", linux-mediatek, srv_heupstream,
	Linux Kernel Mailing List
In-Reply-To: <CABb+yY3snB9o4e14by4xui+o_Vhqpe2zh6Pp_e0t-DCTKvMHWA@mail.gmail.com>

On 05/13/2018 06:36 PM, Jassi Brar wrote:
> On Tue, May 8, 2018 at 5:14 PM, Mikko Perttunen <mperttunen@nvidia.com> wrote:
> 
> ....
>>
>> +config SERIAL_TEGRA_TCU
>> +       tristate "NVIDIA Tegra Combined UART"
>> +       depends on ARCH_TEGRA && MAILBOX
>> +       select SERIAL_CORE
>> +       help
>> +         Support for the mailbox-based TCU (Tegra Combined UART) serial port.
>> +         TCU is a virtual serial port that allows multiplexing multiple data
>> +         streams into a single hardware serial port.
>> +
> Maybe make it depend upon TEGRA_HSP_MBOX ?

Yeah, that probably makes more sense. MAILBOX is enough to build it but 
it won't be of any use without TEGRA_HSP_MBOX.

> 
> ......
> 
>> +
>> +static void tegra_tcu_write(const char *s, unsigned int count)
>> +{
>> +       struct tegra_tcu *tcu = tegra_tcu_uart_port.private_data;
>> +       unsigned int written = 0, i = 0;
>> +       bool insert_nl = false;
>> +       uint32_t value = 0;
>> +
>> +       while (i < count) {
>> +               if (insert_nl) {
>> +                       value |= '\n' << (written++ * 8);
>> +                       insert_nl = false;
>> +                       i++;
>> +               } else if (s[i] == '\n') {
>> +                       value |= '\r' << (written++ * 8);
>> +                       insert_nl = true;
>> +               } else {
>> +                       value |= s[i++] << (written++ * 8);
>> +               }
>> +
>> +               if (written == 3) {
>> +                       value |= 3 << 24;
>> +                       value |= BIT(26);
>> +                       mbox_send_message(tcu->tx, &value);
>>
> How is this supposed to work? tegra_hsp_doorbell_send_data() ignores
> the second argument.

The previous patch in the series adds support for what are called 
"shared mailboxes" to the tegra-hsp driver. For these the second 
argument is used.

Thanks,
Mikko

> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply

* Re: [PATCH 6/8] serial: Add Tegra Combined UART driver
From: Mikko Perttunen @ 2018-05-13 18:04 UTC (permalink / raw)
  To: Andy Shevchenko, Mikko Perttunen
  Cc: Rob Herring, Mark Rutland, Jassi Brar, Greg Kroah-Hartman,
	Thierry Reding, Jon Hunter, araza, devicetree,
	open list:SERIAL DRIVERS, linux-tegra, linux-arm Mailing List,
	Linux Kernel Mailing List
In-Reply-To: <CAHp75VfYF_u2bZ+temGbReEB9K-t3A75aUNYwffyv9-=zEi0Lw@mail.gmail.com>

On 05/13/2018 05:16 PM, Andy Shevchenko wrote:
> On Tue, May 8, 2018 at 2:44 PM, Mikko Perttunen <mperttunen@nvidia.com> wrote:
>> The Tegra Combined UART (TCU) is a mailbox-based mechanism that allows
>> multiplexing multiple "virtual UARTs" into a single hardware serial
>> port. The TCU is the primary serial port on Tegra194 devices.
>>
>> Add a TCU driver utilizing the mailbox framework, as the used mailboxes
>> are part of Tegra HSP blocks that are already controlled by the Tegra
>> HSP mailbox driver.
> 
> First question, can it be done utilizing SERDEV framework?

Based on some brief research, the SERDEV framework is for devices that 
are behind some UART interface. In this case, this driver implements the 
UART interface itself, so by my understanding SERDEV is not appropriate. 
Please correct me if I'm wrong.

> 
>> +static void tegra_tcu_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>> +{
> 
>> +       (void)port;
>> +       (void)mctrl;
> 
> Huh?

The serial core calls these callbacks without checking if they are set. 
They don't make sense for this driver so they are stubbed out.

> 
>> +}
> 
>> +static void tegra_tcu_uart_stop_tx(struct uart_port *port)
>> +{
>> +       (void)port;
>> +}
> 
> Ditto.
> 
>> +               if (written == 3) {
>> +                       value |= 3 << 24;
>> +                       value |= BIT(26);
>> +                       mbox_send_message(tcu->tx, &value);
> 
>> +               }
> 
> (1)
> 
>> +       }
>> +
>> +       if (written) {
>> +               value |= written << 24;
>> +               value |= BIT(26);
>> +               mbox_send_message(tcu->tx, &value);
>> +       }
> 
> (2)
> 
> These are code duplications.

Indeed - the length of the duplicated code is so short, and the 
instances are so close to each other, that I don't find it necessary (or 
clearer) to have an extra function.

> 
>> +static void tegra_tcu_uart_stop_rx(struct uart_port *port)
>> +{
>> +       (void)port;
>> +}
>> +
>> +static void tegra_tcu_uart_break_ctl(struct uart_port *port, int ctl)
>> +{
>> +       (void)port;
>> +       (void)ctl;
>> +}
>> +
>> +static int tegra_tcu_uart_startup(struct uart_port *port)
>> +{
>> +       (void)port;
>> +
>> +       return 0;
>> +}
>> +
>> +static void tegra_tcu_uart_shutdown(struct uart_port *port)
>> +{
>> +       (void)port;
>> +}
>> +
>> +static void tegra_tcu_uart_set_termios(struct uart_port *port,
>> +                                      struct ktermios *new,
>> +                                      struct ktermios *old)
>> +{
>> +       (void)port;
>> +       (void)new;
>> +       (void)old;
>> +}
> 
> Remove those unused stub contents.

Sure. I had these here so that we don't get unused parameter warnings, 
but I can just as well remove the parameter names.

> 
>> +       return uart_set_options(&tegra_tcu_uart_port, cons,
>> +                               115200, 'n', 8, 'n');
> 
> Can't it be one line?

It would be a total of 81 characters in length on one line, so no.

> 
>> +static void tegra_tcu_receive(struct mbox_client *client, void *msg_p)
>> +{
>> +       struct tty_port *port = &tegra_tcu_uart_port.state->port;
> 
>> +       uint32_t msg = *(uint32_t *)msg_p;
> 
> Redundant casting.

Will remove.

> 
>> +       unsigned int num_bytes;
>> +       int i;
>> +
> 
>> +       num_bytes = (msg >> 24) & 0x3;
> 
> Two magic numbers.

Sure, will add defines.

> 
>> +       for (i = 0; i < num_bytes; i++)
>> +               tty_insert_flip_char(port, (msg >> (i*8)) & 0xff, TTY_NORMAL);
>> +
>> +       tty_flip_buffer_push(port);
>> +}
> 
>> +MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("NVIDIA Tegra Combined UART driver");
>> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
>> index dce5f9dae121..eaf3c303cba6 100644
>> --- a/include/uapi/linux/serial_core.h
>> +++ b/include/uapi/linux/serial_core.h
>> @@ -281,4 +281,7 @@
>>   /* MediaTek BTIF */
>>   #define PORT_MTK_BTIF  117
>>
>> +/* NVIDIA Tegra Combined UART */
>> +#define PORT_TEGRA_TCU 118
> 
> Check if there is an unused gap. IIRC we still have one near to 40ish.
> 

Correct, looks like 41-43 are unused. I'll change this 41.

Thanks for reviewing!

Mikko

^ permalink raw reply

* Re: [PATCH 6/8] serial: Add Tegra Combined UART driver
From: Jassi Brar @ 2018-05-13 15:36 UTC (permalink / raw)
  To: Mikko Perttunen
  Cc: Rob Herring, Mark Rutland, Greg KH, Thierry Reding, Jon Hunter,
	araza, Devicetree List, linux-serial, linux-tegra,
	, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, srv_heupstream,
	Linux Kernel Mailing List
In-Reply-To: <20180508114403.14499-7-mperttunen@nvidia.com>

On Tue, May 8, 2018 at 5:14 PM, Mikko Perttunen <mperttunen@nvidia.com> wrote:

....
>
> +config SERIAL_TEGRA_TCU
> +       tristate "NVIDIA Tegra Combined UART"
> +       depends on ARCH_TEGRA && MAILBOX
> +       select SERIAL_CORE
> +       help
> +         Support for the mailbox-based TCU (Tegra Combined UART) serial port.
> +         TCU is a virtual serial port that allows multiplexing multiple data
> +         streams into a single hardware serial port.
> +
Maybe make it depend upon TEGRA_HSP_MBOX ?

......

> +
> +static void tegra_tcu_write(const char *s, unsigned int count)
> +{
> +       struct tegra_tcu *tcu = tegra_tcu_uart_port.private_data;
> +       unsigned int written = 0, i = 0;
> +       bool insert_nl = false;
> +       uint32_t value = 0;
> +
> +       while (i < count) {
> +               if (insert_nl) {
> +                       value |= '\n' << (written++ * 8);
> +                       insert_nl = false;
> +                       i++;
> +               } else if (s[i] == '\n') {
> +                       value |= '\r' << (written++ * 8);
> +                       insert_nl = true;
> +               } else {
> +                       value |= s[i++] << (written++ * 8);
> +               }
> +
> +               if (written == 3) {
> +                       value |= 3 << 24;
> +                       value |= BIT(26);
> +                       mbox_send_message(tcu->tx, &value);
>
How is this supposed to work? tegra_hsp_doorbell_send_data() ignores
the second argument.

^ permalink raw reply

* Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()
From: Andy Shevchenko @ 2018-05-13 14:34 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Uwe Kleine-König, Greg Kroah-Hartman, Jiri Slaby,
	Johan Hovold, Jacek Anaszewski, open list:SERIAL DRIVERS,
	Linux LED Subsystem, linux-can, Sascha Hauer, One Thousand Gnomes,
	Florian Fainelli, Mathieu Poirier, Linux Kernel Mailing List,
	Robin Murphy, linux-arm Mailing List
In-Reply-To: <20180510112229.GE6977@amd>

On Thu, May 10, 2018 at 2:22 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Thu 2018-05-10 13:21:01, Pavel Machek wrote:

>> I know this is not your fault, but if you have a space or "[]" in
>> netdev names, confusing things will happen.
>
> Hmm. If we are doing this we really should check trigger names for
> forbidden characters. At least "[] " should be forbidden.

So, string_escape_mem() is your helper here.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply

* Re: [PATCH v3 3/3] tty: implement led triggers
From: Andy Shevchenko @ 2018-05-13 14:23 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Jacek Anaszewski,
	Pavel Machek, open list:SERIAL DRIVERS, Linux LED Subsystem,
	linux-can, Sascha Hauer, One Thousand Gnomes, Florian Fainelli,
	Mathieu Poirier, Linux Kernel Mailing List, Robin Murphy,
	linux-arm Mailing List
In-Reply-To: <20180508100543.12559-4-u.kleine-koenig@pengutronix.de>

On Tue, May 8, 2018 at 1:05 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> The rx trigger fires when data is pushed to the ldisc by the driver. This
> is a bit later than the actual receiving of data but has the nice benefit
> that it doesn't need adaption for each driver and isn't in the hot path.
>
> Similarly the tx trigger fires when data was copied from userspace and is
> given to the ldisc.

>  #include <uapi/linux/tty.h>
>  #include <linux/rwsem.h>
>  #include <linux/llist.h>
> +#include <linux/leds.h>

Even for unordered lists of inclusions I would still try to put new
lines in "somehow" ordered positions.
For example here, I would rather put it before llist.h.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply

* Re: [PATCH 6/8] serial: Add Tegra Combined UART driver
From: Andy Shevchenko @ 2018-05-13 14:16 UTC (permalink / raw)
  To: Mikko Perttunen
  Cc: Rob Herring, Mark Rutland, Jassi Brar, Greg Kroah-Hartman,
	Thierry Reding, Jon Hunter, araza, devicetree,
	open list:SERIAL DRIVERS, linux-tegra, linux-arm Mailing List,
	Linux Kernel Mailing List
In-Reply-To: <20180508114403.14499-7-mperttunen@nvidia.com>

On Tue, May 8, 2018 at 2:44 PM, Mikko Perttunen <mperttunen@nvidia.com> wrote:
> The Tegra Combined UART (TCU) is a mailbox-based mechanism that allows
> multiplexing multiple "virtual UARTs" into a single hardware serial
> port. The TCU is the primary serial port on Tegra194 devices.
>
> Add a TCU driver utilizing the mailbox framework, as the used mailboxes
> are part of Tegra HSP blocks that are already controlled by the Tegra
> HSP mailbox driver.

First question, can it be done utilizing SERDEV framework?

> +static void tegra_tcu_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
> +{

> +       (void)port;
> +       (void)mctrl;

Huh?

> +}

> +static void tegra_tcu_uart_stop_tx(struct uart_port *port)
> +{
> +       (void)port;
> +}

Ditto.

> +               if (written == 3) {
> +                       value |= 3 << 24;
> +                       value |= BIT(26);
> +                       mbox_send_message(tcu->tx, &value);

> +               }

(1)

> +       }
> +
> +       if (written) {
> +               value |= written << 24;
> +               value |= BIT(26);
> +               mbox_send_message(tcu->tx, &value);
> +       }

(2)

These are code duplications.

> +static void tegra_tcu_uart_stop_rx(struct uart_port *port)
> +{
> +       (void)port;
> +}
> +
> +static void tegra_tcu_uart_break_ctl(struct uart_port *port, int ctl)
> +{
> +       (void)port;
> +       (void)ctl;
> +}
> +
> +static int tegra_tcu_uart_startup(struct uart_port *port)
> +{
> +       (void)port;
> +
> +       return 0;
> +}
> +
> +static void tegra_tcu_uart_shutdown(struct uart_port *port)
> +{
> +       (void)port;
> +}
> +
> +static void tegra_tcu_uart_set_termios(struct uart_port *port,
> +                                      struct ktermios *new,
> +                                      struct ktermios *old)
> +{
> +       (void)port;
> +       (void)new;
> +       (void)old;
> +}

Remove those unused stub contents.

> +       return uart_set_options(&tegra_tcu_uart_port, cons,
> +                               115200, 'n', 8, 'n');

Can't it be one line?

> +static void tegra_tcu_receive(struct mbox_client *client, void *msg_p)
> +{
> +       struct tty_port *port = &tegra_tcu_uart_port.state->port;

> +       uint32_t msg = *(uint32_t *)msg_p;

Redundant casting.

> +       unsigned int num_bytes;
> +       int i;
> +

> +       num_bytes = (msg >> 24) & 0x3;

Two magic numbers.

> +       for (i = 0; i < num_bytes; i++)
> +               tty_insert_flip_char(port, (msg >> (i*8)) & 0xff, TTY_NORMAL);
> +
> +       tty_flip_buffer_push(port);
> +}

> +MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("NVIDIA Tegra Combined UART driver");
> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> index dce5f9dae121..eaf3c303cba6 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -281,4 +281,7 @@
>  /* MediaTek BTIF */
>  #define PORT_MTK_BTIF  117
>
> +/* NVIDIA Tegra Combined UART */
> +#define PORT_TEGRA_TCU 118

Check if there is an unused gap. IIRC we still have one near to 40ish.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply

* Re: [PATCH v3 5/6] spi: at91-usart: add driver for at91-usart as spi
From: Andy Shevchenko @ 2018-05-13 13:35 UTC (permalink / raw)
  To: Radu Pirea
  Cc: devicetree, open list:SERIAL DRIVERS, Linux Kernel Mailing List,
	linux-arm Mailing List, linux-spi, Mark Rutland, Rob Herring,
	Lee Jones, Greg Kroah-Hartman, Jiri Slaby, Richard Genoud,
	alexandre.belloni, Nicolas Ferre, Mark Brown
In-Reply-To: <CAHp75Ve3Ugnjjm8EZkPQTZSvH1qad1e5SqjOn8zz5syHSQea_g@mail.gmail.com>

> I will refer to above as (1) later on.

> The question is, why you didn't utilize what SPI core provides you?

Here I should have referred to (1).

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply

* Re: [PATCH v3 5/6] spi: at91-usart: add driver for at91-usart as spi
From: Andy Shevchenko @ 2018-05-13 13:33 UTC (permalink / raw)
  To: Radu Pirea
  Cc: devicetree, open list:SERIAL DRIVERS, Linux Kernel Mailing List,
	linux-arm Mailing List, linux-spi, Mark Rutland, Rob Herring,
	Lee Jones, Greg Kroah-Hartman, Jiri Slaby, Richard Genoud,
	alexandre.belloni, Nicolas Ferre, Mark Brown
In-Reply-To: <20180511103822.31698-6-radu.pirea@microchip.com>

On Fri, May 11, 2018 at 1:38 PM, Radu Pirea <radu.pirea@microchip.com> wrote:
> This is the driver for at91-usart in spi mode. The USART IP can be configured
> to work in many modes and one of them is SPI.

> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>

Here is something wrong. You need to use latter one in new code.

> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>

Hmm... Do you need all of them?

> +static inline void at91_usart_spi_cs_activate(struct spi_device *spi)
> +{
...
> +       gpiod_set_value(ausd->npcs_pin, active);
> +       aus->cs_active = true;
> +}
> +
> +static inline void at91_usart_spi_cs_deactivate(struct spi_device *spi)
> +{
...
> +       gpiod_set_value(ausd->npcs_pin, !active);
> +       aus->cs_active = false;
> +}
...
> +       if (!ausd) {
> +               if (gpio_is_valid(spi->cs_gpio)) {
> +                       npcs_pin = gpio_to_desc(spi->cs_gpio);
...
> +               }
...
> +               gpiod_direction_output(npcs_pin, !(spi->mode & SPI_CS_HIGH));
> +
> +               ausd->npcs_pin = npcs_pin;
...
> +       }

I will refer to above as (1) later on.

> +       dev_dbg(&spi->dev, "new message %p submitted for %s\n",
> +               msg, dev_name(&spi->dev));

%p does make a very little sense.

> +       list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +               ret = at91_usart_spi_one_transfer(controller, msg, xfer);
> +               if (ret)
> +                       goto msg_done;
> +       }

Cant SPI core do this for your?

> +static void at91_usart_spi_cleanup(struct spi_device *spi)
> +{
> +       struct at91_usart_spi_device *ausd = spi->controller_state;
> +

> +       if (!ausd)
> +               return;

Is it even possible?

Anyway the code below will work fine even if it's the case.

> +
> +       spi->controller_state = NULL;
> +       kfree(ausd);
> +}

> +static int at91_usart_spi_gpio_cs(struct platform_device *pdev)
> +{
> +       struct spi_controller *controller = platform_get_drvdata(pdev);
> +       struct device_node *np = controller->dev.parent->of_node;
> +       struct gpio_desc *cs_gpio;
> +       int nb;
> +       int i;
> +
> +       if (!np)
> +               return 0;
> +
> +       nb = of_gpio_named_count(np, "cs-gpios");
> +       for (i = 0; i < nb; i++) {
> +               cs_gpio = devm_gpiod_get_from_of_node(&pdev->dev,
> +                                                     pdev->dev.parent->of_node,
> +                                                     "cs-gpios",
> +                                                     i, GPIOD_OUT_HIGH,
> +                                                     dev_name(&pdev->dev));
> +               if (IS_ERR(cs_gpio))
> +                       return PTR_ERR(cs_gpio);
> +       }
> +
> +       controller->num_chipselect = nb;
> +
> +       return 0;
> +}

The question is, why you didn't utilize what SPI core provides you?

> +       spi_writel(aus, MR, US_MR_SPI_MASTER | US_MR_CHRL | US_MR_CLKO |
> +                       US_MR_WRDBT);
> +       spi_writel(aus, CR, US_CR_RXDIS | US_CR_TXDIS | US_CR_RSTRX |
> +                       US_CR_RSTTX);

I didn't check over, but it seems like you might have duplication in
these bitwise ORs. Consider to unify them into another (shorter)
definitions and reuse all over the code.

> +       regs = platform_get_resource(to_platform_device(pdev->dev.parent),
> +                                    IORESOURCE_MEM, 0);
> +       if (!regs)
> +               return -ENXIO;

Strange error code for getting MMIO resource. ENOMEM sounds better.

> +       dev_info(&pdev->dev,
> +                "Atmel USART SPI Controller version 0x%x at 0x%08lx (irq %d)\n",
> +                spi_readl(aus, VERSION),
> +                (unsigned long)regs->start, irq);

If you do explicit casting when printing something you are doing wrong.
Please use %pR or %pr in this case.

> +static struct platform_driver at91_usart_spi_driver = {
> +       .driver = {
> +               .name = "at91_usart_spi",

> +               .of_match_table = of_match_ptr(at91_usart_spi_dt_ids),

Can it work as pure platform driver? If no, of_match_ptr() is redundant.

> +       },
> +       .probe = at91_usart_spi_probe,
> +       .remove = at91_usart_spi_remove, };

Two lines at one. Split.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply

* Re: [PATCH v3 3/6] MAINTAINERS: add at91 usart spi driver
From: Andy Shevchenko @ 2018-05-13 13:14 UTC (permalink / raw)
  To: Radu Pirea
  Cc: devicetree, open list:SERIAL DRIVERS, Linux Kernel Mailing List,
	linux-arm Mailing List, linux-spi, Mark Rutland, Rob Herring,
	Lee Jones, Greg Kroah-Hartman, Jiri Slaby, Richard Genoud,
	alexandre.belloni, Nicolas Ferre, Mark Brown
In-Reply-To: <20180511103822.31698-4-radu.pirea@microchip.com>

On Fri, May 11, 2018 at 1:38 PM, Radu Pirea <radu.pirea@microchip.com> wrote:
> Added entry for at91 usart mfd driver.
>

You are adding a record for not existing file?

> Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ca06c6f58299..9243b9007966 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9199,6 +9199,13 @@ S:       Supported
>  F:     drivers/mfd/at91-usart.c
>  F:     include/dt-bindings/mfd/at91-usart.h
>
> +MICROCHIP AT91 USART SPI DRIVER
> +M:     Radu Pirea <radu.pirea@microchip.com>
> +L:     linux-spi@vger.kernel.org
> +S:     Supported
> +F:     drivers/spi/spi-at91-usart.c
> +F:     Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt
> +
>  MICROCHIP KSZ SERIES ETHERNET SWITCH DRIVER
>  M:     Woojung Huh <Woojung.Huh@microchip.com>
>  M:     Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
> --
> 2.17.0
>



-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply

* Re: [PATCH v2] tty: implement led triggers
From: Uwe Kleine-König @ 2018-05-12 19:00 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Pavel Machek, Johan Hovold, One Thousand Gnomes, Florian Fainelli,
	kernel, Mathieu Poirier, Greg Kroah-Hartman, linux-kernel,
	linux-serial, linux-arm-kernel
In-Reply-To: <01dcbf5c-70a0-b067-20c3-f99e22ed4512@arm.com>

On Thu, May 10, 2018 at 12:25:21PM +0100, Robin Murphy wrote:
> On 10/05/18 12:14, Pavel Machek wrote:
> > Hi!
> > 
> > > > > > @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
> > > > > >   		struct tty_buffer *head = buf->head;
> > > > > >   		struct tty_buffer *next;
> > > > > >   		int count;
> > > > > > +		unsigned long delay = 50 /* ms */;
> > > > > 
> > > > > Comment after the semicolon?
> > > > 
> > > > Given that this comment is about the 50 and not the delay member, I
> > > > prefer it before the ;.
> > > 
> > > Hmm. I personally find it hard to read and can only find about 30
> > > instances of this comment style (for assignments) in the kernel. And
> > > arguably the comment applies equally well to the delay variable in this
> > > case too.
> > 
> > It is not too traditional, but I believe it makes sense....
> > 
> > (and yes, I wish we had kernel in Rust, so we could have real units
> > attached to our variables...)
> 
> Well, the variable itself could always be named "delay_ms" if it's really
> that important.

FTR: That's what I did for v3.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()
From: Uwe Kleine-König @ 2018-05-12 18:59 UTC (permalink / raw)
  To: Pavel Machek
  Cc: linux-arm-kernel, One Thousand Gnomes, Florian Fainelli,
	linux-kernel, kernel, Mathieu Poirier, Greg Kroah-Hartman,
	Johan Hovold, linux-can, Jacek Anaszewski, linux-serial,
	Jiri Slaby, Robin Murphy, linux-leds
In-Reply-To: <20180510112229.GE6977@amd>

On Thu, May 10, 2018 at 01:22:29PM +0200, Pavel Machek wrote:
> On Thu 2018-05-10 13:21:01, Pavel Machek wrote:
> > Hi!
> > 
> > > This allows one to simplify drivers that provide a trigger with a
> > > non-constant name (e.g. one trigger per device with the trigger name
> > > depending on the device's name).
> > > 
> > > Internally the memory the name member of struct led_trigger points to
> > > now always allocated dynamically instead of just taken from the caller.
> > > 
> > > The function led_trigger_rename_static() must be changed accordingly and
> > > was renamed to led_trigger_rename() for consistency, with the only user
> > > adapted.
> > 
> > Well, I'm not sure if we want to have _that_ many trigger. Trigger
> > interface is going to become.. "interesting".
> > 
> > We have 4K limit on total number of triggers. We use rather strange
> > interface to select trigger.
> > 
> > > @@ -115,13 +115,13 @@ static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
> > >  
> > >  	if (msg == NETDEV_CHANGENAME) {
> > >  		snprintf(name, sizeof(name), "%s-tx", netdev->name);
> > > -		led_trigger_rename_static(name, priv->tx_led_trig);
> > > +		led_trigger_rename(priv->tx_led_trig, name);
> > >  
> > >  		snprintf(name, sizeof(name), "%s-rx", netdev->name);
> > > -		led_trigger_rename_static(name, priv->rx_led_trig);
> > > +		led_trigger_rename(priv->rx_led_trig, name);
> > >  
> > >  		snprintf(name, sizeof(name), "%s-rxtx", netdev->name);
> > > -		led_trigger_rename_static(name, priv->rxtx_led_trig);
> > > +		led_trigger_rename(priv->rxtx_led_trig, name);
> > >  	}
> > >  
> > 
> > I know this is not your fault, but if you have a space or "[]" in
> > netdev names, confusing things will happen.
> 
> Hmm. If we are doing this we really should check trigger names for
> forbidden characters. At least "[] " should be forbidden.

I think you don't expect me to change the patch, but to make this
explicit: My patch doesn't make this problem worse, so this would be an
orthogonal change and doesn't affect this one.

Spaces don't seem to be allowed in netdev names:

uwe@taurus:~$ sudo ip link set wlp3s0 name 'la la'
Error: argument "la la" is wrong: "name" not a valid ifname

(Didn't check if only ip forbids that, of if that is a kernel policy.) I
could rename my device to "lala[]" though.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* Re: [PATCH 1/2] serdev: add controller runtime PM support
From: Tony Lindgren @ 2018-05-11 16:00 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Greg Kroah-Hartman, Rob Herring, Sebastian Reichel,
	H. Nikolaus Schaller, Andreas Kemnade, Mark Rutland,
	Arnd Bergmann, Pavel Machek, linux-kernel, linux-serial, linux-pm
In-Reply-To: <20180511131213.GF2285@localhost>

* Johan Hovold <johan@kernel.org> [180511 13:14]:
> On Fri, May 11, 2018 at 05:56:27AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [180511 08:09]:
> > > On Thu, May 10, 2018 at 09:48:31AM -0700, Tony Lindgren wrote:
> > > > If this solution works for GPS then this should also work for modems
> > > > that might produce data. And as long as the serdev consumer driver
> > > > can wake up the UART with pm_runtime_get(&serdev->ctrl->dev) then
> > > > the out of band GPIO wake interrupts will work to. And for TX,
> > > > the serdev consumer driver can toggle the wake GPIO, and then call
> > > > pm_runtime_get(&serdev->ctrl->dev).
> > > 
> > > I don't think any serdev driver action is needed for TX however. The
> > > serial driver itself would know that there's data in the write buffer
> > > and should manage PM itself until the buffer has been drained (which may
> > > never happen due to flow control).
> > 
> > Sure if the serial driver can manage TX wake directly. However, the
> > case I'm thinking needs few hundred milliseconds after toggling the
> > GPIO before we can even attempt to do TX. I guess what I'm saying
> > let's not try to stuff any "application specific" GPIO handling to
> > generic UART code :)
> 
> Ok, I think understand what you have in mind now, but that sounds like
> something which should be handled by the serdev driver which is
> responsible for the power state of the serial-attached device (e.g.
> through a pm_runtime_get_sync(&serdev->dev)).

Yes that makes sense.

> And you're absolutely right that that RPM reference cannot be dropped
> before any written data has reached the device. But note that the serial
> controller responsible for that transfer could potentially runtime
> suspend before the write buffer has been drained (e.g. during long
> periods without I/O due to flow control).
> 
> So I still think that TX is not directly related to this patch and RPM
> support for serdev controllers.

Yes you are right after the device on the serial line has been properly
woken up then TX should just work and at that point the UART driver
runtime PM can take care of things.

Regards,

Tony

^ permalink raw reply

* Re: [PATCH 1/2] serdev: add controller runtime PM support
From: Johan Hovold @ 2018-05-11 13:12 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Johan Hovold, Greg Kroah-Hartman, Rob Herring, Sebastian Reichel,
	H. Nikolaus Schaller, Andreas Kemnade, Mark Rutland,
	Arnd Bergmann, Pavel Machek, linux-kernel, linux-serial, linux-pm
In-Reply-To: <20180511125627.GI98604@atomide.com>

On Fri, May 11, 2018 at 05:56:27AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180511 08:09]:
> > On Thu, May 10, 2018 at 09:48:31AM -0700, Tony Lindgren wrote:
> > > If this solution works for GPS then this should also work for modems
> > > that might produce data. And as long as the serdev consumer driver
> > > can wake up the UART with pm_runtime_get(&serdev->ctrl->dev) then
> > > the out of band GPIO wake interrupts will work to. And for TX,
> > > the serdev consumer driver can toggle the wake GPIO, and then call
> > > pm_runtime_get(&serdev->ctrl->dev).
> > 
> > I don't think any serdev driver action is needed for TX however. The
> > serial driver itself would know that there's data in the write buffer
> > and should manage PM itself until the buffer has been drained (which may
> > never happen due to flow control).
> 
> Sure if the serial driver can manage TX wake directly. However, the
> case I'm thinking needs few hundred milliseconds after toggling the
> GPIO before we can even attempt to do TX. I guess what I'm saying
> let's not try to stuff any "application specific" GPIO handling to
> generic UART code :)

Ok, I think understand what you have in mind now, but that sounds like
something which should be handled by the serdev driver which is
responsible for the power state of the serial-attached device (e.g.
through a pm_runtime_get_sync(&serdev->dev)).

And you're absolutely right that that RPM reference cannot be dropped
before any written data has reached the device. But note that the serial
controller responsible for that transfer could potentially runtime
suspend before the write buffer has been drained (e.g. during long
periods without I/O due to flow control).

So I still think that TX is not directly related to this patch and RPM
support for serdev controllers.

Thanks,
Johan

^ permalink raw reply

* Re: [PATCH 1/2] serdev: add controller runtime PM support
From: Tony Lindgren @ 2018-05-11 12:56 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Greg Kroah-Hartman, Rob Herring, Sebastian Reichel,
	H. Nikolaus Schaller, Andreas Kemnade, Mark Rutland,
	Arnd Bergmann, Pavel Machek, linux-kernel, linux-serial, linux-pm
In-Reply-To: <20180511080725.GE2285@localhost>

* Johan Hovold <johan@kernel.org> [180511 08:09]:
> On Thu, May 10, 2018 at 09:48:31AM -0700, Tony Lindgren wrote:
> > If this solution works for GPS then this should also work for modems
> > that might produce data. And as long as the serdev consumer driver
> > can wake up the UART with pm_runtime_get(&serdev->ctrl->dev) then
> > the out of band GPIO wake interrupts will work to. And for TX,
> > the serdev consumer driver can toggle the wake GPIO, and then call
> > pm_runtime_get(&serdev->ctrl->dev).
> 
> I don't think any serdev driver action is needed for TX however. The
> serial driver itself would know that there's data in the write buffer
> and should manage PM itself until the buffer has been drained (which may
> never happen due to flow control).

Sure if the serial driver can manage TX wake directly. However, the
case I'm thinking needs few hundred milliseconds after toggling the
GPIO before we can even attempt to do TX. I guess what I'm saying
let's not try to stuff any "application specific" GPIO handling to
generic UART code :)

> But either way, the mechanism introduced by this patch is general enough
> that it could in principle be used also for something like that.

Yes I agree your patches should work for both cases.

Regards,

Tony

^ permalink raw reply

* Re: [PATCH 1/2] serdev: add controller runtime PM support
From: Sebastian Reichel @ 2018-05-11 12:35 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Greg Kroah-Hartman, Rob Herring, Tony Lindgren,
	H. Nikolaus Schaller, Andreas Kemnade, Mark Rutland,
	Arnd Bergmann, Pavel Machek, linux-kernel, linux-serial, linux-pm
In-Reply-To: <20180509094419.13470-1-johan@kernel.org>

[-- Attachment #1: Type: text/plain, Size: 4429 bytes --]

Hi Johan,

On Wed, May 09, 2018 at 11:44:18AM +0200, Johan Hovold wrote:
> Add support for controller runtime power management to serdev core. This
> is needed to allow slave drivers to manage the runtime PM state of the
> underlying serial controller when its driver, in turn, implements more
> aggressive runtime power management (e.g. using autosuspend).
> 
> For some applications, for example, where loss off initial data after a
> remote-wakeup event is acceptable or where rx is not used at all,
> aggressive serial controller runtime PM may be used without further
> involvement of the slave driver. But when this is not the case, the
> slave driver must be able to indicate when incoming data is expected in
> order to avoid data loss.
> 
> To facilitate the common case, where the serial controller power state
> is active whenever the port is open (which is the case with just about
> every serial driver), and where data loss is not acceptable and cannot
> even be prevented by explicit controller runtime power management, an
> RPM reference is taken in serdev open and put again at close. This
> reference can later be balanced by any serdev driver which wants and/or
> can handle aggressive controller runtime PM.
> 
> Note that the .ignore_children flag is set for the serdev controller to
> allow the underlying hardware to idle when no I/O is expected, regardless
> of the slave device RPM state.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---

Looks good to me.

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

>  drivers/tty/serdev/core.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index df93b727e984..e5e84303faca 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/serdev.h>
>  #include <linux/slab.h>
>  
> @@ -143,11 +144,28 @@ EXPORT_SYMBOL_GPL(serdev_device_remove);
>  int serdev_device_open(struct serdev_device *serdev)
>  {
>  	struct serdev_controller *ctrl = serdev->ctrl;
> +	int ret;
>  
>  	if (!ctrl || !ctrl->ops->open)
>  		return -EINVAL;
>  
> -	return ctrl->ops->open(ctrl);
> +	ret = ctrl->ops->open(ctrl);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_get_sync(&ctrl->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(&ctrl->dev);
> +		goto err_close;
> +	}
> +
> +	return 0;
> +
> +err_close:
> +	if (ctrl->ops->close)
> +		ctrl->ops->close(ctrl);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_open);
>  
> @@ -158,6 +176,8 @@ void serdev_device_close(struct serdev_device *serdev)
>  	if (!ctrl || !ctrl->ops->close)
>  		return;
>  
> +	pm_runtime_put(&ctrl->dev);
> +
>  	ctrl->ops->close(ctrl);
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_close);
> @@ -416,6 +436,9 @@ struct serdev_controller *serdev_controller_alloc(struct device *parent,
>  
>  	dev_set_name(&ctrl->dev, "serial%d", id);
>  
> +	pm_runtime_no_callbacks(&ctrl->dev);
> +	pm_suspend_ignore_children(&ctrl->dev, true);
> +
>  	dev_dbg(&ctrl->dev, "allocated controller 0x%p id %d\n", ctrl, id);
>  	return ctrl;
>  
> @@ -547,20 +570,23 @@ int serdev_controller_add(struct serdev_controller *ctrl)
>  	if (ret)
>  		return ret;
>  
> +	pm_runtime_enable(&ctrl->dev);
> +
>  	ret_of = of_serdev_register_devices(ctrl);
>  	ret_acpi = acpi_serdev_register_devices(ctrl);
>  	if (ret_of && ret_acpi) {
>  		dev_dbg(&ctrl->dev, "no devices registered: of:%d acpi:%d\n",
>  			ret_of, ret_acpi);
>  		ret = -ENODEV;
> -		goto out_dev_del;
> +		goto err_rpm_disable;
>  	}
>  
>  	dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
>  		ctrl->nr, &ctrl->dev);
>  	return 0;
>  
> -out_dev_del:
> +err_rpm_disable:
> +	pm_runtime_disable(&ctrl->dev);
>  	device_del(&ctrl->dev);
>  	return ret;
>  };
> @@ -591,6 +617,7 @@ void serdev_controller_remove(struct serdev_controller *ctrl)
>  
>  	dummy = device_for_each_child(&ctrl->dev, NULL,
>  				      serdev_remove_device);
> +	pm_runtime_disable(&ctrl->dev);
>  	device_del(&ctrl->dev);
>  }
>  EXPORT_SYMBOL_GPL(serdev_controller_remove);
> -- 
> 2.17.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [PATCH 2/2] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: Matthias Brugger @ 2018-05-11 11:19 UTC (permalink / raw)
  To: Erin Lo
  Cc: Mark Rutland, devicetree, srv_heupstream, Ben Ho, Rob Herring,
	linux-mediatek, linux-serial, linux-arm-kernel
In-Reply-To: <1526035267.13828.13.camel@mtksdaap41>



On 05/11/2018 12:41 PM, Erin Lo wrote:
> On Fri, 2018-05-11 at 10:36 +0200, Matthias Brugger wrote:
>>
>> On 05/11/2018 08:11 AM, Erin Lo wrote:
[...]
>>
>> I wonder if there aren't any other devices which can be supported out of the box.
>> I understand that for now we are missing the clock driver and the pinctrl
>> driver. Are you planning to submit them in the near future?
>>
>> I'm asking because I don't want to bloat the dts with boards that only can boot
>> to an initramfs with a serial console. Especially if there is no HW + datasheet
>> available for anyone in the community who wants to work on this.
>>
>> Regards,
>> Matthias
>>
>> Regards,
>> Matthias
> 
> 
> We have implement clock and pinctrl driver these days, and plan to
> submit them maybe next month.
> After that we will submit other drivers of MT8183 continuously.
> 

I'm happy to hear this. So in this case I'm fine with merging these once you
figured out the dt-bindings.

Thanks a lot,
Matthias

^ permalink raw reply

* Re: [PATCH 2/2] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: Erin Lo @ 2018-05-11 10:41 UTC (permalink / raw)
  To: Matthias Brugger
  Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, srv_heupstream,
	Ben Ho, Rob Herring,
	linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-serial-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <860cba8a-3af8-31d6-ec3f-81706d2d998b-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On Fri, 2018-05-11 at 10:36 +0200, Matthias Brugger wrote:
> 
> On 05/11/2018 08:11 AM, Erin Lo wrote:
> > From: Ben Ho <Ben.Ho-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > 
> > Add basic chip support for Mediatek 8183
> > 
> > Signed-off-by: Ben Ho <Ben.Ho-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Erin Lo <erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> >  arch/arm64/boot/dts/mediatek/Makefile       |   1 +
> >  arch/arm64/boot/dts/mediatek/mt8183-evb.dts |  31 +++++
> >  arch/arm64/boot/dts/mediatek/mt8183.dtsi    | 178 ++++++++++++++++++++++++++++
> >  3 files changed, 210 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8183.dtsi
> > 
> > diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> > index ac17f60..2836261 100644
> > --- a/arch/arm64/boot/dts/mediatek/Makefile
> > +++ b/arch/arm64/boot/dts/mediatek/Makefile
> > @@ -5,3 +5,4 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt6795-evb.dtb
> >  dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-evb.dtb
> >  dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-rfb1.dtb
> >  dtb-$(CONFIG_ARCH_MEDIATEK) += mt8173-evb.dtb
> > +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-evb.dtb
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> > new file mode 100644
> > index 0000000..9a3d6b7
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> > @@ -0,0 +1,31 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright (c) 2017 MediaTek Inc.
> > + * Author: Ben Ho <ben.ho-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > + *	   Erin Lo <erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > + */
> > +
> > +/dts-v1/;
> > +#include "mt8183.dtsi"
> > +
> > +/ {
> > +	model = "MediaTek MT8183 evaluation board";
> > +	compatible = "mediatek,mt8183-evb", "mediatek,mt8183";
> > +
> > +	aliases {
> > +		serial0 = &uart0;
> > +	};
> > +
> > +	memory@40000000 {
> > +		device_type = "memory";
> > +		reg = <0 0x40000000 0 0x80000000>;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0:921600n8";
> > +	};
> > +};
> > +
> > +&uart0 {
> > +	status = "okay";
> > +};
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> > new file mode 100644
> > index 0000000..8564a26
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> > @@ -0,0 +1,178 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright (c) 2017 MediaTek Inc.
> > + * Author: Ben Ho <ben.ho-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > + *	   Erin Lo <erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > + */
> > +
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +/ {
> > +	compatible = "mediatek,mt8183";
> > +	interrupt-parent = <&sysirq>;
> > +	#address-cells = <2>;
> > +	#size-cells = <2>;
> > +
> > +	cpus {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		cpu-map {
> > +			cluster0 {
> > +				core0 {
> > +					cpu = <&cpu0>;
> > +				};
> > +				core1 {
> > +					cpu = <&cpu1>;
> > +				};
> > +				core2 {
> > +					cpu = <&cpu2>;
> > +				};
> > +				core3 {
> > +					cpu = <&cpu3>;
> > +				};
> > +			};
> > +
> > +			cluster1 {
> > +				core0 {
> > +					cpu = <&cpu4>;
> > +				};
> > +				core1 {
> > +					cpu = <&cpu5>;
> > +				};
> > +				core2 {
> > +					cpu = <&cpu6>;
> > +				};
> > +				core3 {
> > +					cpu = <&cpu7>;
> > +				};
> > +			};
> > +		};
> > +
> > +		cpu0: cpu@000 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a53";
> > +			reg = <0x000>;
> > +			enable-method = "psci";
> > +		};
> > +
> > +		cpu1: cpu@001 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a53";
> > +			reg = <0x001>;
> > +			enable-method = "psci";
> > +		};
> > +
> > +		cpu2: cpu@002 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a53";
> > +			reg = <0x002>;
> > +			enable-method = "psci";
> > +		};
> > +
> > +		cpu3: cpu@003 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a53";
> > +			reg = <0x003>;
> > +			enable-method = "psci";
> > +		};
> > +
> > +		cpu4: cpu@100 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a73";
> > +			reg = <0x100>;
> > +			enable-method = "psci";
> > +		};
> > +
> > +		cpu5: cpu@101 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a73";
> > +			reg = <0x101>;
> > +			enable-method = "psci";
> > +		};
> > +
> > +		cpu6: cpu@102 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a73";
> > +			reg = <0x102>;
> > +			enable-method = "psci";
> > +		};
> > +
> > +		cpu7: cpu@103 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a73";
> > +			reg = <0x103>;
> > +			enable-method = "psci";
> > +		};
> > +	};
> > +
> > +	psci {
> > +		compatible      = "arm,psci-1.0";
> > +		method          = "smc";
> > +	};
> > +
> > +	uart_clk: dummy26m {
> > +		compatible = "fixed-clock";
> > +		clock-frequency = <26000000>;
> > +		#clock-cells = <0>;
> > +	};
> > +
> > +	timer {
> > +		compatible = "arm,armv8-timer";
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
> > +			     <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
> > +			     <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
> > +			     <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
> > +	};
> > +
> > +	gic: interrupt-controller@0c000000 {
> > +		compatible = "arm,gic-v3";
> > +		#interrupt-cells = <3>;
> > +		interrupt-parent = <&gic>;
> > +		interrupt-controller;
> > +		reg = <0 0x0c000000 0 0x40000>,  // CID
> > +		      <0 0x0c100000 0 0x200000>; // CIR
> > +		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> > +	};
> > +
> > +	sysirq: intpol-controller@0c530a80 {
> > +		compatible = "mediatek,mt8183-sysirq",
> > +			     "mediatek,mt6577-sysirq";
> > +		interrupt-controller;
> > +		#interrupt-cells = <3>;
> > +		interrupt-parent = <&gic>;
> > +		reg = <0 0x0c530a80 0 0x50>;
> > +	};
> > +
> > +	uart0: serial@11002000 {
> > +		compatible = "mediatek,mt8183-uart",
> > +			     "mediatek,mt6577-uart";
> > +		reg = <0 0x11002000 0 0x1000>;
> > +		interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_LOW>;
> > +		clocks = <&uart_clk>, <&uart_clk>;
> > +		clock-names = "baud", "bus";
> > +		status = "disabled";
> > +	};
> > +
> > +	uart1: serial@11003000 {
> > +		compatible = "mediatek,mt8183-uart",
> > +			     "mediatek,mt6577-uart";
> > +		reg = <0 0x11003000 0 0x1000>;
> > +		interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_LOW>;
> > +		clocks = <&uart_clk>, <&uart_clk>;
> > +		clock-names = "baud", "bus";
> > +		status = "disabled";
> > +	};
> > +
> > +	uart2: serial@11004000 {
> > +		compatible = "mediatek,mt8183-uart",
> > +			     "mediatek,mt6577-uart";
> > +		reg = <0 0x11004000 0 0x1000>;
> > +		interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_LOW>;
> > +		clocks = <&uart_clk>, <&uart_clk>;
> > +		clock-names = "baud", "bus";
> > +		status = "disabled";
> > +	};
> > +};
> > 
> 
> I wonder if there aren't any other devices which can be supported out of the box.
> I understand that for now we are missing the clock driver and the pinctrl
> driver. Are you planning to submit them in the near future?
> 
> I'm asking because I don't want to bloat the dts with boards that only can boot
> to an initramfs with a serial console. Especially if there is no HW + datasheet
> available for anyone in the community who wants to work on this.
> 
> Regards,
> Matthias
> 
> Regards,
> Matthias


We have implement clock and pinctrl driver these days, and plan to
submit them maybe next month.
After that we will submit other drivers of MT8183 continuously.

Regards,
Erin

^ permalink raw reply

* [PATCH v3 6/6] tty/serial: atmel: changed the driver to work under at91-usart mfd
From: Radu Pirea @ 2018-05-11 10:38 UTC (permalink / raw)
  To: devicetree, linux-serial, linux-kernel, linux-arm-kernel,
	linux-spi
  Cc: mark.rutland, robh+dt, lee.jones, gregkh, jslaby, richard.genoud,
	alexandre.belloni, nicolas.ferre, broonie, Radu Pirea
In-Reply-To: <20180511103822.31698-1-radu.pirea@microchip.com>

This patch modifies the place where resources and device tree properties
are searched.

Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
---
 drivers/tty/serial/Kconfig        |  1 +
 drivers/tty/serial/atmel_serial.c | 29 +++++++++++++++--------------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 3682fd3e960c..25e55332f8b1 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -119,6 +119,7 @@ config SERIAL_ATMEL
 	depends on ARCH_AT91 || COMPILE_TEST
 	select SERIAL_CORE
 	select SERIAL_MCTRL_GPIO if GPIOLIB
+	select MFD_AT91_USART
 	help
 	  This enables the driver for the on-chip UARTs of the Atmel
 	  AT91 processors.
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index df46a9e88c34..6b4494352853 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -193,8 +193,8 @@ static struct console atmel_console;
 
 #if defined(CONFIG_OF)
 static const struct of_device_id atmel_serial_dt_ids[] = {
-	{ .compatible = "atmel,at91rm9200-usart" },
-	{ .compatible = "atmel,at91sam9260-usart" },
+	{ .compatible = "atmel,at91rm9200-usart-serial" },
+	{ .compatible = "atmel,at91sam9260-usart-serial" },
 	{ /* sentinel */ }
 };
 #endif
@@ -1631,7 +1631,7 @@ static void atmel_tasklet_tx_func(unsigned long data)
 static void atmel_init_property(struct atmel_uart_port *atmel_port,
 				struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.parent->of_node;
 
 	/* DMA/PDC usage specification */
 	if (of_property_read_bool(np, "atmel,use-dma-rx")) {
@@ -2223,7 +2223,8 @@ static const char *atmel_type(struct uart_port *port)
 static void atmel_release_port(struct uart_port *port)
 {
 	struct platform_device *pdev = to_platform_device(port->dev);
-	int size = pdev->resource[0].end - pdev->resource[0].start + 1;
+	int size = to_platform_device(pdev->dev.parent)->resource[0].end -
+		to_platform_device(pdev->dev.parent)->resource[0].start + 1;
 
 	release_mem_region(port->mapbase, size);
 
@@ -2239,7 +2240,8 @@ static void atmel_release_port(struct uart_port *port)
 static int atmel_request_port(struct uart_port *port)
 {
 	struct platform_device *pdev = to_platform_device(port->dev);
-	int size = pdev->resource[0].end - pdev->resource[0].start + 1;
+	int size = to_platform_device(pdev->dev.parent)->resource[0].end -
+		to_platform_device(pdev->dev.parent)->resource[0].start + 1;
 
 	if (!request_mem_region(port->mapbase, size, "atmel_serial"))
 		return -EBUSY;
@@ -2345,23 +2347,23 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
 	atmel_init_property(atmel_port, pdev);
 	atmel_set_ops(port);
 
-	uart_get_rs485_mode(&pdev->dev, &port->rs485);
+	uart_get_rs485_mode(pdev->dev.parent, &port->rs485);
 
 	port->iotype		= UPIO_MEM;
 	port->flags		= UPF_BOOT_AUTOCONF | UPF_IOREMAP;
 	port->ops		= &atmel_pops;
 	port->fifosize		= 1;
 	port->dev		= &pdev->dev;
-	port->mapbase	= pdev->resource[0].start;
-	port->irq	= pdev->resource[1].start;
+	port->mapbase		= to_platform_device(pdev->dev.parent)->resource[0].start;
+	port->irq		= to_platform_device(pdev->dev.parent)->resource[1].start;
 	port->rs485_config	= atmel_config_rs485;
-	port->membase	= NULL;
+	port->membase		= NULL;
 
 	memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
 
 	/* for console, the clock could already be configured */
 	if (!atmel_port->clk) {
-		atmel_port->clk = clk_get(&pdev->dev, "usart");
+		atmel_port->clk = clk_get(pdev->dev.parent, "usart");
 		if (IS_ERR(atmel_port->clk)) {
 			ret = PTR_ERR(atmel_port->clk);
 			atmel_port->clk = NULL;
@@ -2656,7 +2658,7 @@ static void atmel_serial_probe_fifos(struct atmel_uart_port *atmel_port,
 	atmel_port->rts_low = 0;
 	atmel_port->rts_high = 0;
 
-	if (of_property_read_u32(pdev->dev.of_node,
+	if (of_property_read_u32(pdev->dev.parent->of_node,
 				 "atmel,fifo-size",
 				 &atmel_port->fifo_size))
 		return;
@@ -2694,11 +2696,10 @@ static void atmel_serial_probe_fifos(struct atmel_uart_port *atmel_port,
 static int atmel_serial_probe(struct platform_device *pdev)
 {
 	struct atmel_uart_port *atmel_port;
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.parent->of_node;
 	void *data;
 	int ret = -ENODEV;
 	bool rs485_enabled;
-
 	BUILD_BUG_ON(ATMEL_SERIAL_RINGSIZE & (ATMEL_SERIAL_RINGSIZE - 1));
 
 	ret = of_alias_get_id(np, "serial");
@@ -2845,7 +2846,7 @@ static struct platform_driver atmel_serial_driver = {
 	.suspend	= atmel_serial_suspend,
 	.resume		= atmel_serial_resume,
 	.driver		= {
-		.name			= "atmel_usart",
+		.name			= "atmel_usart_serial",
 		.of_match_table		= of_match_ptr(atmel_serial_dt_ids),
 	},
 };
-- 
2.17.0

^ permalink raw reply related

* [PATCH v3 5/6] spi: at91-usart: add driver for at91-usart as spi
From: Radu Pirea @ 2018-05-11 10:38 UTC (permalink / raw)
  To: devicetree, linux-serial, linux-kernel, linux-arm-kernel,
	linux-spi
  Cc: mark.rutland, robh+dt, lee.jones, gregkh, jslaby, richard.genoud,
	alexandre.belloni, nicolas.ferre, broonie, Radu Pirea
In-Reply-To: <20180511103822.31698-1-radu.pirea@microchip.com>

This is the driver for at91-usart in spi mode. The USART IP can be configured
to work in many modes and one of them is SPI.

The driver was tested on sama5d3-xplained and sama5d4-xplained boards with
enc28j60 ethernet controller as slave.

Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
---
 drivers/spi/Kconfig          |   9 +
 drivers/spi/Makefile         |   1 +
 drivers/spi/spi-at91-usart.c | 544 +++++++++++++++++++++++++++++++++++
 3 files changed, 554 insertions(+)
 create mode 100644 drivers/spi/spi-at91-usart.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 6fb0347a24f2..c675e6b8dd5a 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -77,6 +77,15 @@ config SPI_ATMEL
 	  This selects a driver for the Atmel SPI Controller, present on
 	  many AT91 (ARM) chips.
 
+config SPI_AT91_USART
+        tristate "Atmel USART Controller as SPI"
+	depends on HAS_DMA
+	depends on (ARCH_AT91 || COMPILE_TEST)
+        select MFD_AT91_USART
+	help
+	  This selects a driver for the AT91 USART Controller as SPI Master,
+	  present on AT91 and SAMA5 SoC series.
+
 config SPI_AU1550
 	tristate "Au1550/Au1200/Au1300 SPI Controller"
 	depends on MIPS_ALCHEMY
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 34c5f2832ddf..fb6cb42f4eaa 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST)		+= spi-loopback-test.o
 obj-$(CONFIG_SPI_ALTERA)		+= spi-altera.o
 obj-$(CONFIG_SPI_ARMADA_3700)		+= spi-armada-3700.o
 obj-$(CONFIG_SPI_ATMEL)			+= spi-atmel.o
+obj-$(CONFIG_SPI_AT91_USART)		+= spi-at91-usart.o
 obj-$(CONFIG_SPI_ATH79)			+= spi-ath79.o
 obj-$(CONFIG_SPI_AU1550)		+= spi-au1550.o
 obj-$(CONFIG_SPI_AXI_SPI_ENGINE)	+= spi-axi-spi-engine.o
diff --git a/drivers/spi/spi-at91-usart.c b/drivers/spi/spi-at91-usart.c
new file mode 100644
index 000000000000..79a59759d2ee
--- /dev/null
+++ b/drivers/spi/spi-at91-usart.c
@@ -0,0 +1,544 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for AT91 USART Controllers as SPI
+ *
+ * Copyright (C) 2018 Microchip Technology Inc.
+ * Author: Radu Pirea <radu.pirea@microchip.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+
+#include <linux/pinctrl/consumer.h>
+
+#include <linux/spi/spi.h>
+
+#define US_CR			0x00
+#define US_MR			0x04
+#define US_IER			0x08
+#define US_IDR			0x0C
+#define US_CSR			0x14
+#define US_RHR			0x18
+#define US_THR			0x1C
+#define US_BRGR			0x20
+#define US_VERSION		0xFC
+
+#define US_CR_RSTRX		BIT(2)
+#define US_CR_RSTTX		BIT(3)
+#define US_CR_RXEN		BIT(4)
+#define US_CR_RXDIS		BIT(5)
+#define US_CR_TXEN		BIT(6)
+#define US_CR_TXDIS		BIT(7)
+
+#define US_MR_SPI_MASTER	0x0E
+#define US_MR_CHRL		GENMASK(7, 6)
+#define US_MR_CPHA		BIT(8)
+#define US_MR_CPOL		BIT(16)
+#define US_MR_CLKO		BIT(18)
+#define US_MR_WRDBT		BIT(20)
+#define US_MR_LOOP		BIT(15)
+
+#define US_IR_RXRDY		BIT(0)
+#define US_IR_TXRDY		BIT(1)
+#define US_IR_OVRE		BIT(5)
+
+#define US_BRGR_SIZE		BIT(16)
+
+#define US_MIN_CLK_DIV		0x06
+#define US_MAX_CLK_DIV		BIT(16)
+
+#define US_DUMMY_TX		0xFF
+
+/* Register access macros */
+#define spi_readl(port, reg) \
+	readl_relaxed((port)->regs + US_##reg)
+#define spi_writel(port, reg, value) \
+	writel_relaxed((value), (port)->regs + US_##reg)
+
+#define spi_readb(port, reg) \
+	readb_relaxed((port)->regs + US_##reg)
+#define spi_writeb(port, reg, value) \
+	writeb_relaxed((value), (port)->regs + US_##reg)
+
+struct at91_usart_spi {
+	struct spi_transfer	*current_transfer;
+	void __iomem		*regs;
+	struct device		*dev;
+	struct clk		*clk;
+
+	/*used in interrupt to protect data reading*/
+	spinlock_t		lock;
+
+	int			irq;
+	unsigned int		current_tx_remaining_bytes;
+	unsigned int		current_rx_remaining_bytes;
+	int			done_status;
+
+	u32			spi_clk;
+	u32			status;
+
+	bool			xfer_failed;
+	bool			keep_cs;
+	bool			cs_active;
+};
+
+struct at91_usart_spi_device {
+	struct gpio_desc	*npcs_pin;
+	u32			mr;
+};
+
+static inline u32 at91_usart_spi_tx_ready(struct at91_usart_spi *aus)
+{
+	return aus->status & US_IR_TXRDY;
+}
+
+static inline u32 at91_usart_spi_rx_ready(struct at91_usart_spi *aus)
+{
+	return aus->status & US_IR_RXRDY;
+}
+
+static inline u32 at91_usart_spi_check_overrun(struct at91_usart_spi *aus)
+{
+	return aus->status & US_IR_OVRE;
+}
+
+static inline u32 at91_usart_spi_read_status(struct at91_usart_spi *aus)
+{
+	aus->status = spi_readl(aus, CSR);
+	return aus->status;
+}
+
+static inline void at91_usart_spi_tx(struct at91_usart_spi *aus)
+{
+	unsigned int len = aus->current_transfer->len;
+	unsigned int remaining = aus->current_tx_remaining_bytes;
+	const u8  *tx_buf = aus->current_transfer->tx_buf;
+
+	if (tx_buf && remaining) {
+		if (at91_usart_spi_tx_ready(aus))
+			spi_writel(aus, THR, tx_buf[len - remaining]);
+			aus->current_tx_remaining_bytes--;
+	} else {
+		if (at91_usart_spi_tx_ready(aus))
+			spi_writel(aus, THR, US_DUMMY_TX);
+	}
+}
+
+static inline void at91_usart_spi_rx(struct at91_usart_spi *aus)
+{
+	int len = aus->current_transfer->len;
+	int remaining = aus->current_rx_remaining_bytes;
+	u8  *rx_buf = aus->current_transfer->rx_buf;
+
+	if (aus->current_rx_remaining_bytes) {
+		rx_buf[len - remaining] = spi_readb(aus, RHR);
+		aus->current_rx_remaining_bytes--;
+	} else {
+		spi_readb(aus, RHR);
+	}
+}
+
+static inline void at91_usart_spi_cs_activate(struct spi_device *spi)
+{
+	struct at91_usart_spi_device *ausd = spi->controller_state;
+	struct at91_usart_spi *aus = spi_master_get_devdata(spi->controller);
+	u32 active = spi->mode & SPI_CS_HIGH;
+
+	gpiod_set_value(ausd->npcs_pin, active);
+	aus->cs_active = true;
+}
+
+static inline void at91_usart_spi_cs_deactivate(struct spi_device *spi)
+{
+	struct at91_usart_spi_device *ausd = spi->controller_state;
+	struct at91_usart_spi *aus = spi_master_get_devdata(spi->controller);
+	u32 active = spi->mode & SPI_CS_HIGH;
+
+	gpiod_set_value(ausd->npcs_pin, !active);
+	aus->cs_active = false;
+}
+
+static inline void at91_usart_spi_set_mode_register(struct spi_device *spi)
+{
+	struct at91_usart_spi_device *ausd = spi->controller_state;
+	struct at91_usart_spi *aus = spi_master_get_devdata(spi->controller);
+
+	spi_writel(aus, MR, ausd->mr);
+}
+
+static inline void
+at91_usart_spi_enable_irq_and_hw(struct at91_usart_spi *aus)
+{
+	spi_writel(aus, CR, US_CR_RXEN | US_CR_TXEN);
+	spi_writel(aus, IER, US_IR_OVRE | US_IR_RXRDY);
+}
+
+static inline void
+at91_usart_spi_disable_irq_and_hw(struct at91_usart_spi *aus)
+{
+	spi_writel(aus, CR, US_CR_RXDIS | US_CR_TXDIS |
+		   US_CR_RSTRX | US_CR_RSTTX);
+	spi_writel(aus, IDR, US_IR_OVRE | US_IR_RXRDY);
+}
+
+static inline void
+at91_usart_spi_set_xfer_speed(struct at91_usart_spi *aus,
+			      struct spi_transfer *xfer)
+{
+	spi_writel(aus, BRGR,
+		   DIV_ROUND_UP(aus->spi_clk, xfer->speed_hz));
+}
+
+static irqreturn_t at91_usart_spi_interrupt(int irq, void *dev_id)
+{
+	struct spi_controller *controller = dev_id;
+	struct at91_usart_spi *aus = spi_master_get_devdata(controller);
+
+	spin_lock(&aus->lock);
+
+	at91_usart_spi_read_status(aus);
+
+	if (at91_usart_spi_check_overrun(aus)) {
+		aus->xfer_failed = true;
+		aus->done_status = -EIO;
+		spi_writel(aus, IDR, US_IR_OVRE | US_IR_RXRDY);
+		spin_unlock(&aus->lock);
+		return IRQ_HANDLED;
+	}
+
+	if (at91_usart_spi_rx_ready(aus)) {
+		at91_usart_spi_rx(aus);
+		spin_unlock(&aus->lock);
+		return IRQ_HANDLED;
+	}
+	spin_unlock(&aus->lock);
+
+	return IRQ_NONE;
+}
+
+static int at91_usart_spi_setup(struct spi_device *spi)
+{
+	struct at91_usart_spi *aus = spi_master_get_devdata(spi->controller);
+	struct at91_usart_spi_device *ausd = spi->controller_state;
+	struct gpio_desc *npcs_pin;
+	unsigned int mr = spi_readl(aus, MR);
+	u8 bits = spi->bits_per_word;
+
+	if (bits != 8) {
+		dev_dbg(&spi->dev, "Only 8 bits per word are supported\n");
+		return -EINVAL;
+	}
+
+	if (spi->mode & SPI_CPOL)
+		mr |= US_MR_CPOL;
+	else
+		mr &= ~US_MR_CPOL;
+
+	if (spi->mode & SPI_CPHA)
+		mr |= US_MR_CPHA;
+	else
+		mr &= ~US_MR_CPHA;
+
+	if (spi->mode & SPI_LOOP)
+		mr |= US_MR_LOOP;
+	else
+		mr &= ~US_MR_LOOP;
+
+	if (!ausd) {
+		if (gpio_is_valid(spi->cs_gpio)) {
+			npcs_pin = gpio_to_desc(spi->cs_gpio);
+		} else {
+			dev_dbg(&spi->dev, "Invalid chip select\n");
+			return -EINVAL;
+		}
+
+		ausd = kzalloc(sizeof(*ausd), GFP_KERNEL);
+		if (!ausd)
+			return -ENOMEM;
+		gpiod_direction_output(npcs_pin, !(spi->mode & SPI_CS_HIGH));
+
+		ausd->npcs_pin = npcs_pin;
+		spi->controller_state = ausd;
+	}
+
+	ausd->mr = mr;
+
+	dev_dbg(&spi->dev,
+		"setup: bpw %u mode 0x%x -> mr %d %08x\n",
+		bits, spi->mode, spi->chip_select, mr);
+
+	return 0;
+}
+
+static int at91_usart_spi_one_transfer(struct spi_controller *controller,
+				       struct spi_message *msg,
+				       struct spi_transfer *xfer)
+{
+	struct at91_usart_spi *aus = spi_master_get_devdata(controller);
+	struct spi_device *spi = msg->spi;
+	const u8 *tx_buf = xfer->tx_buf;
+	u8 *rx_buf = xfer->rx_buf;
+
+	if (!(xfer->tx_buf || xfer->rx_buf) && xfer->len) {
+		dev_dbg(&spi->dev, "missing rx and tx buf\n");
+		return -EINVAL;
+	}
+
+	at91_usart_spi_set_xfer_speed(aus, xfer);
+	aus->done_status = 0;
+	aus->xfer_failed = false;
+	aus->current_transfer = xfer;
+	aus->current_tx_remaining_bytes = xfer->len;
+	aus->current_rx_remaining_bytes = xfer->len;
+	if (!tx_buf)
+		aus->current_tx_remaining_bytes = 0;
+	if (!rx_buf)
+		aus->current_rx_remaining_bytes = 0;
+
+	while ((aus->current_tx_remaining_bytes ||
+		aus->current_rx_remaining_bytes) && !aus->xfer_failed) {
+		at91_usart_spi_read_status(aus);
+		at91_usart_spi_tx(aus);
+		cpu_relax();
+	}
+	if (aus->xfer_failed) {
+		dev_err(aus->dev, "Overrun!\n");
+		return -EIO;
+	}
+
+	if (xfer->delay_usecs)
+		udelay(xfer->delay_usecs);
+
+	if (xfer->cs_change) {
+		if (list_is_last(&xfer->transfer_list, &msg->transfers)) {
+			aus->keep_cs = true;
+		} else {
+			aus->cs_active = !aus->cs_active;
+			if (aus->cs_active)
+				at91_usart_spi_cs_activate(spi);
+			else
+				at91_usart_spi_cs_deactivate(spi);
+		}
+	}
+
+	return 0;
+}
+
+static int
+at91_usart_spi_transfer_one_message(struct spi_controller *controller,
+				    struct spi_message *msg)
+{
+	struct at91_usart_spi *aus = spi_master_get_devdata(controller);
+	struct spi_transfer *xfer;
+	struct spi_device *spi = msg->spi;
+	int ret;
+
+	dev_dbg(&spi->dev, "new message %p submitted for %s\n",
+		msg, dev_name(&spi->dev));
+	at91_usart_spi_enable_irq_and_hw(aus);
+	at91_usart_spi_set_mode_register(spi);
+	at91_usart_spi_cs_activate(spi);
+
+	aus->keep_cs = false;
+
+	msg->status = 0;
+	msg->actual_length = 0;
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		ret = at91_usart_spi_one_transfer(controller, msg, xfer);
+		if (ret)
+			goto msg_done;
+	}
+
+msg_done:
+
+	if (!aus->keep_cs)
+		at91_usart_spi_cs_deactivate(spi);
+
+	at91_usart_spi_disable_irq_and_hw(aus);
+
+	msg->status = aus->done_status;
+	spi_finalize_current_message(spi->master);
+
+	return ret;
+}
+
+static void at91_usart_spi_cleanup(struct spi_device *spi)
+{
+	struct at91_usart_spi_device *ausd = spi->controller_state;
+
+	if (!ausd)
+		return;
+
+	spi->controller_state = NULL;
+	kfree(ausd);
+}
+
+static int at91_usart_spi_gpio_cs(struct platform_device *pdev)
+{
+	struct spi_controller *controller = platform_get_drvdata(pdev);
+	struct device_node *np = controller->dev.parent->of_node;
+	struct gpio_desc *cs_gpio;
+	int nb;
+	int i;
+
+	if (!np)
+		return 0;
+
+	nb = of_gpio_named_count(np, "cs-gpios");
+	for (i = 0; i < nb; i++) {
+		cs_gpio = devm_gpiod_get_from_of_node(&pdev->dev,
+						      pdev->dev.parent->of_node,
+						      "cs-gpios",
+						      i, GPIOD_OUT_HIGH,
+						      dev_name(&pdev->dev));
+		if (IS_ERR(cs_gpio))
+			return PTR_ERR(cs_gpio);
+	}
+
+	controller->num_chipselect = nb;
+
+	return 0;
+}
+
+static void at91_usart_spi_init(struct at91_usart_spi *aus)
+{
+	spi_writel(aus, MR, US_MR_SPI_MASTER | US_MR_CHRL | US_MR_CLKO |
+			US_MR_WRDBT);
+	spi_writel(aus, CR, US_CR_RXDIS | US_CR_TXDIS | US_CR_RSTRX |
+			US_CR_RSTTX);
+}
+
+static int at91_usart_spi_probe(struct platform_device *pdev)
+{
+	struct resource *regs;
+	struct spi_controller *controller;
+	struct at91_usart_spi *aus;
+	struct clk *clk;
+	int irq;
+	int ret;
+
+	regs = platform_get_resource(to_platform_device(pdev->dev.parent),
+				     IORESOURCE_MEM, 0);
+	if (!regs)
+		return -ENXIO;
+
+	irq = platform_get_irq(to_platform_device(pdev->dev.parent), 0);
+	if (irq < 0)
+		return irq;
+
+	clk = devm_clk_get(pdev->dev.parent, "usart");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = -ENOMEM;
+	controller = spi_alloc_master(&pdev->dev, sizeof(*aus));
+	if (!controller)
+		goto at91_usart_spi_probe_fail;
+
+	controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_CS_HIGH;
+	controller->dev.of_node = pdev->dev.parent->of_node;
+	controller->bits_per_word_mask = SPI_BPW_MASK(8);
+	controller->num_chipselect = 0;
+	controller->setup = at91_usart_spi_setup;
+	controller->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;
+	controller->transfer_one_message = at91_usart_spi_transfer_one_message;
+	controller->cleanup = at91_usart_spi_cleanup;
+	controller->max_speed_hz = DIV_ROUND_UP(clk_get_rate(clk),
+						US_MIN_CLK_DIV);
+	controller->min_speed_hz = DIV_ROUND_UP(clk_get_rate(clk),
+						US_MAX_CLK_DIV);
+	platform_set_drvdata(pdev, controller);
+
+	aus = spi_master_get_devdata(controller);
+
+	aus->dev = &pdev->dev;
+	aus->regs = devm_ioremap_resource(&pdev->dev, regs);
+	if (IS_ERR(aus->regs)) {
+		ret = PTR_ERR(aus->regs);
+		goto at91_usart_spi_probe_fail;
+	}
+
+	aus->irq = irq;
+	aus->clk = clk;
+
+	ret = at91_usart_spi_gpio_cs(pdev);
+	if (ret)
+		goto at91_usart_spi_probe_fail;
+
+	ret = devm_request_irq(&pdev->dev, irq, at91_usart_spi_interrupt, 0,
+			       dev_name(&pdev->dev), controller);
+	if (ret)
+		goto at91_usart_spi_probe_fail;
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		goto at91_usart_spi_probe_fail;
+
+	aus->spi_clk = clk_get_rate(clk);
+	at91_usart_spi_init(aus);
+
+	spin_lock_init(&aus->lock);
+	ret = devm_spi_register_master(&pdev->dev, controller);
+	if (ret)
+		goto fail_register_master;
+
+	dev_info(&pdev->dev,
+		 "Atmel USART SPI Controller version 0x%x at 0x%08lx (irq %d)\n",
+		 spi_readl(aus, VERSION),
+		 (unsigned long)regs->start, irq);
+
+	return 0;
+
+fail_register_master:
+	clk_disable_unprepare(clk);
+at91_usart_spi_probe_fail:
+	spi_master_put(controller);
+	return ret;
+}
+
+static int at91_usart_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct at91_usart_spi *aus = spi_master_get_devdata(master);
+
+	clk_disable_unprepare(aus->clk);
+
+	return 0;
+}
+
+static const struct of_device_id at91_usart_spi_dt_ids[] = {
+	{ .compatible = "microchip,sama5d3-usart-spi"},
+	{ .compatible = "microchip,sama5d4-usart-spi"},
+	{ .compatible = "microchip,at91sam9g45-usart-spi"},
+	{ /* sentinel */}
+};
+
+MODULE_DEVICE_TABLE(of, at91_usart_spi_dt_ids);
+
+static struct platform_driver at91_usart_spi_driver = {
+	.driver = {
+		.name = "at91_usart_spi",
+		.of_match_table = of_match_ptr(at91_usart_spi_dt_ids),
+	},
+	.probe = at91_usart_spi_probe,
+	.remove = at91_usart_spi_remove, };
+module_platform_driver(at91_usart_spi_driver);
+
+MODULE_DESCRIPTION("Microchip AT91 USART SPI Controller driver");
+MODULE_AUTHOR("Radu Pirea <radu.pirea@microchip.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:at91_usart_spi");
-- 
2.17.0

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox