| Message ID | c1371fc8-ebc2-0d14-e409-02e2fbf3ba94@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 16/04/17 21:13, Heiner Kallweit wrote: > AML_UART_TX_EN is set in meson_uart_startup and there's no place in > the driver where it gets cleared. Therefore we don't have to set it > here. I think this is a leftover of the patches I did when trying to fix the UART driver when using debian/systemd and having strange console problems. The only comment is it might be worth leaving this in as it isn't a high cost to the driver and it is possible someone might accidentally clear it later. > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/tty/serial/meson_uart.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c > index 22857f1e..714b29ad 100644 > --- a/drivers/tty/serial/meson_uart.c > +++ b/drivers/tty/serial/meson_uart.c > @@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s, > } > > val = readl(port->membase + AML_UART_CONTROL); > - val |= AML_UART_TX_EN; > tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN); > writel(tmp, port->membase + AML_UART_CONTROL); > >
On 04/17/2017 03:58 PM, Ben Dooks wrote: > On 16/04/17 21:13, Heiner Kallweit wrote: >> AML_UART_TX_EN is set in meson_uart_startup and there's no place in >> the driver where it gets cleared. Therefore we don't have to set it >> here. > > I think this is a leftover of the patches I did when trying to fix > the UART driver when using debian/systemd and having strange console > problems. > > The only comment is it might be worth leaving this in as it isn't > a high cost to the driver and it is possible someone might accidentally > clear it later. > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/tty/serial/meson_uart.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c >> index 22857f1e..714b29ad 100644 >> --- a/drivers/tty/serial/meson_uart.c >> +++ b/drivers/tty/serial/meson_uart.c >> @@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s, >> } >> >> val = readl(port->membase + AML_UART_CONTROL); >> - val |= AML_UART_TX_EN; >> tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN); >> writel(tmp, port->membase + AML_UART_CONTROL); >> >> > > I'm ok to remove it, having it in uart_startup is enough. Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Am 17.04.2017 um 17:27 schrieb Neil Armstrong: > On 04/17/2017 03:58 PM, Ben Dooks wrote: >> On 16/04/17 21:13, Heiner Kallweit wrote: >>> AML_UART_TX_EN is set in meson_uart_startup and there's no place in >>> the driver where it gets cleared. Therefore we don't have to set it >>> here. >> >> I think this is a leftover of the patches I did when trying to fix >> the UART driver when using debian/systemd and having strange console >> problems. >> >> The only comment is it might be worth leaving this in as it isn't >> a high cost to the driver and it is possible someone might accidentally >> clear it later. >> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> drivers/tty/serial/meson_uart.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c >>> index 22857f1e..714b29ad 100644 >>> --- a/drivers/tty/serial/meson_uart.c >>> +++ b/drivers/tty/serial/meson_uart.c >>> @@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s, >>> } >>> >>> val = readl(port->membase + AML_UART_CONTROL); >>> - val |= AML_UART_TX_EN; >>> tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN); >>> writel(tmp, port->membase + AML_UART_CONTROL); >>> >>> >> >> > > I'm ok to remove it, having it in uart_startup is enough. > > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> Have you checked the earlycon code paths? That does not run through the standard setup function, so it is dependent on what the firmware set last. I believe I am calling into this function through some wrapper. Since we're writing the register anyway, it shouldn't hurt to leave this in. Regards, Andreas
Am 17.04.2017 um 17:37 schrieb Andreas Färber: > Am 17.04.2017 um 17:27 schrieb Neil Armstrong: >> On 04/17/2017 03:58 PM, Ben Dooks wrote: >>> On 16/04/17 21:13, Heiner Kallweit wrote: >>>> AML_UART_TX_EN is set in meson_uart_startup and there's no place in >>>> the driver where it gets cleared. Therefore we don't have to set it >>>> here. >>> >>> I think this is a leftover of the patches I did when trying to fix >>> the UART driver when using debian/systemd and having strange console >>> problems. >>> >>> The only comment is it might be worth leaving this in as it isn't >>> a high cost to the driver and it is possible someone might accidentally >>> clear it later. >>> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>>> --- >>>> drivers/tty/serial/meson_uart.c | 1 - >>>> 1 file changed, 1 deletion(-) >>>> >>>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c >>>> index 22857f1e..714b29ad 100644 >>>> --- a/drivers/tty/serial/meson_uart.c >>>> +++ b/drivers/tty/serial/meson_uart.c >>>> @@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s, >>>> } >>>> >>>> val = readl(port->membase + AML_UART_CONTROL); >>>> - val |= AML_UART_TX_EN; >>>> tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN); >>>> writel(tmp, port->membase + AML_UART_CONTROL); >>>> >>>> >>> >>> >> >> I'm ok to remove it, having it in uart_startup is enough. >> >> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> > > Have you checked the earlycon code paths? That does not run through the > standard setup function, so it is dependent on what the firmware set > last. I believe I am calling into this function through some wrapper. > > Since we're writing the register anyway, it shouldn't hurt to leave this in. > > Regards, > Andreas > Provided that everybody is fine with the other patches of this series: Can you simply omit patch 3 when applying the series or would you prefer sending an updated series w/o patch 3? Heiner
On 04/17/2017 05:37 PM, Andreas Färber wrote: > Am 17.04.2017 um 17:27 schrieb Neil Armstrong: >> On 04/17/2017 03:58 PM, Ben Dooks wrote: >>> On 16/04/17 21:13, Heiner Kallweit wrote: >>>> AML_UART_TX_EN is set in meson_uart_startup and there's no place in >>>> the driver where it gets cleared. Therefore we don't have to set it >>>> here. >>> >>> I think this is a leftover of the patches I did when trying to fix >>> the UART driver when using debian/systemd and having strange console >>> problems. >>> >>> The only comment is it might be worth leaving this in as it isn't >>> a high cost to the driver and it is possible someone might accidentally >>> clear it later. >>> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>>> --- >>>> drivers/tty/serial/meson_uart.c | 1 - >>>> 1 file changed, 1 deletion(-) >>>> >>>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c >>>> index 22857f1e..714b29ad 100644 >>>> --- a/drivers/tty/serial/meson_uart.c >>>> +++ b/drivers/tty/serial/meson_uart.c >>>> @@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s, >>>> } >>>> >>>> val = readl(port->membase + AML_UART_CONTROL); >>>> - val |= AML_UART_TX_EN; >>>> tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN); >>>> writel(tmp, port->membase + AML_UART_CONTROL); >>>> >>>> >>> >>> >> >> I'm ok to remove it, having it in uart_startup is enough. >> >> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> > > Have you checked the earlycon code paths? That does not run through the > standard setup function, so it is dependent on what the firmware set > last. I believe I am calling into this function through some wrapper. > > Since we're writing the register anyway, it shouldn't hurt to leave this in. > > Regards, > Andreas > Indeed, it's on the earlycon path, but it's a hack and should be moved inside meson_serial_early_console_setup() and meson_serial_console_setup() instead. Writing TX_EN at each meson_serial_port_write() is really unnecessary... Neil
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c index 22857f1e..714b29ad 100644 --- a/drivers/tty/serial/meson_uart.c +++ b/drivers/tty/serial/meson_uart.c @@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s, } val = readl(port->membase + AML_UART_CONTROL); - val |= AML_UART_TX_EN; tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN); writel(tmp, port->membase + AML_UART_CONTROL);
AML_UART_TX_EN is set in meson_uart_startup and there's no place in the driver where it gets cleared. Therefore we don't have to set it here. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/tty/serial/meson_uart.c | 1 - 1 file changed, 1 deletion(-)