* Re: [PATCH v3 5/6] spi: at91-usart: add driver for at91-usart as spi
From: Alexandre Belloni @ 2018-05-24 17:56 UTC (permalink / raw)
To: Radu Pirea
Cc: Mark Brown, Andy Shevchenko, 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, Nicolas Ferre
In-Reply-To: <0e6e71e2-f8ac-7889-0d81-8d8a4c15223d@microchip.com>
Hi,
On 24/05/2018 19:04:11+0300, Radu Pirea wrote:
>
>
> On 05/17/2018 07:54 AM, Mark Brown wrote:
> > On Tue, May 15, 2018 at 12:22:24PM +0300, Radu Pirea wrote:
> > > On Mon, 2018-05-14 at 20:38 +0300, Andy Shevchenko wrote:
> >
> > > > So, what is not going as expected in "SPI core takes care of CSs"
> > > > case?
> > > > Did you use oscilloscope for that?
> >
> > > Yes, I used and CSs was not asserted. Anyway, I will will try again.
> >
> > If the core chip select handling is not working properly for some reason
> > then the core chip select handling should be fixed rather than just open
> > coding in your driver - probably it's also broken for other users.
> >
>
> Hi Mark,
>
> I found the fix for cs-gpios. If I change spi_add_device function like
> this(see below) everything is ok.
>
> int spi_add_device(struct spi_device *spi)
>
> ...
>
> if (ctlr->cs_gpios){
> spi->cs_gpio = ctlr->cs_gpios[spi->chip_select];
> if(gpio_is_valid(spi->cs_gpio))
> gpio_direction_output(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
>
> }
>
> ...
>
> return status;
> }
>
> In the subsystem gpio direction of pins is never set and gpio_set_value()
> don't set the direction.
> In my opinion gpio_direction_output() set direction should be called in
> spi_add_device. What do you think? Is ok?
Back in 2014, I was suggesting using devm_gpio_request_one() in
of_spi_register_master(). That would take care of setting the direction
of the GPIO:
https://www.spinics.net/lists/arm-kernel/msg351251.html
I never took the time to create the patch and test though.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH 1/2] serial: imx: drop CTS/RTS handling from shutdown
From: Uwe Kleine-König @ 2018-05-24 17:44 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180524173024.20936-2-sebastian.reichel@collabora.co.uk>
On Thu, May 24, 2018 at 07:30:23PM +0200, Sebastian Reichel wrote:
> According to Documentation/serial/driver the shutdown function should
> not disable RTS, so drop it.
>
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Great idea! :-)
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: disable UCR4_OREN on shutdown
From: Uwe Kleine-König @ 2018-05-24 17:43 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180524173024.20936-3-sebastian.reichel@collabora.co.uk>
On Thu, May 24, 2018 at 07:30:24PM +0200, Sebastian Reichel wrote:
> UCR4_OREN is (depending on the configuration) enabled in startup,
> but is never disabled. Fix this by disabling it in shutdown.
>
> Reported-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
> drivers/tty/serial/imx.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index a0fc0327c8a5..31e1df8a8d29 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1422,7 +1422,7 @@ static void imx_uart_shutdown(struct uart_port *port)
> {
> struct imx_port *sport = (struct imx_port *)port;
> unsigned long flags;
> - u32 ucr1, ucr2;
> + u32 ucr1, ucr2, ucr4;
>
> if (sport->dma_is_enabled) {
> dmaengine_terminate_sync(sport->dma_chan_tx);
> @@ -1452,6 +1452,10 @@ static void imx_uart_shutdown(struct uart_port *port)
> ucr2 = imx_uart_readl(sport, UCR2);
> ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
> imx_uart_writel(sport, ucr2, UCR2);
> +
> + ucr4 = imx_uart_readl(sport, UCR4);
> + ucr4 &= ~UCR4_OREN;
> + imx_uart_writel(sport, ucr4, UCR4);
Not sure this is necessary, but I assume it cannot hurt either, so
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* [PATCH 2/2] serial: imx: disable UCR4_OREN on shutdown
From: Sebastian Reichel @ 2018-05-24 17:30 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, linux-serial
Cc: Uwe Kleine-König, Fabio Estevam, Shawn Guo, linux-kernel,
Sebastian Reichel
In-Reply-To: <20180524173024.20936-1-sebastian.reichel@collabora.co.uk>
UCR4_OREN is (depending on the configuration) enabled in startup,
but is never disabled. Fix this by disabling it in shutdown.
Reported-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
drivers/tty/serial/imx.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index a0fc0327c8a5..31e1df8a8d29 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1422,7 +1422,7 @@ static void imx_uart_shutdown(struct uart_port *port)
{
struct imx_port *sport = (struct imx_port *)port;
unsigned long flags;
- u32 ucr1, ucr2;
+ u32 ucr1, ucr2, ucr4;
if (sport->dma_is_enabled) {
dmaengine_terminate_sync(sport->dma_chan_tx);
@@ -1452,6 +1452,10 @@ static void imx_uart_shutdown(struct uart_port *port)
ucr2 = imx_uart_readl(sport, UCR2);
ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
imx_uart_writel(sport, ucr2, UCR2);
+
+ ucr4 = imx_uart_readl(sport, UCR4);
+ ucr4 &= ~UCR4_OREN;
+ imx_uart_writel(sport, ucr4, UCR4);
spin_unlock_irqrestore(&sport->port.lock, flags);
/*
--
2.17.0
^ permalink raw reply related
* [PATCH 1/2] serial: imx: drop CTS/RTS handling from shutdown
From: Sebastian Reichel @ 2018-05-24 17:30 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, linux-serial
Cc: Uwe Kleine-König, Fabio Estevam, Shawn Guo, linux-kernel,
Sebastian Reichel
In-Reply-To: <20180524173024.20936-1-sebastian.reichel@collabora.co.uk>
According to Documentation/serial/driver the shutdown function should
not disable RTS, so drop it.
Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
drivers/tty/serial/imx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 6c53e74244ec..a0fc0327c8a5 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1450,7 +1450,7 @@ static void imx_uart_shutdown(struct uart_port *port)
spin_lock_irqsave(&sport->port.lock, flags);
ucr2 = imx_uart_readl(sport, UCR2);
- ucr2 &= ~(UCR2_TXEN | UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
+ ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
imx_uart_writel(sport, ucr2, UCR2);
spin_unlock_irqrestore(&sport->port.lock, flags);
--
2.17.0
^ permalink raw reply related
* [PATCH 0/2] serial: imx: more shutdown cleanups
From: Sebastian Reichel @ 2018-05-24 17:30 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, linux-serial
Cc: Uwe Kleine-König, Fabio Estevam, Shawn Guo, linux-kernel,
Sebastian Reichel
Hi,
Here are two more cleanup patches for imx-serial. Both
issues were found by code reading, no real-world issues
are known.
-- Sebastian
Sebastian Reichel (2):
serial: imx: drop CTS/RTS handling from shutdown
serial: imx: disable UCR4_OREN on shutdown
drivers/tty/serial/imx.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
--
2.17.0
^ permalink raw reply
* Re: [PATCH v3 5/6] spi: at91-usart: add driver for at91-usart as spi
From: Radu Pirea @ 2018-05-24 16:04 UTC (permalink / raw)
To: Mark Brown
Cc: Andy Shevchenko, 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
In-Reply-To: <20180517045437.GE20254@sirena.org.uk>
On 05/17/2018 07:54 AM, Mark Brown wrote:
> On Tue, May 15, 2018 at 12:22:24PM +0300, Radu Pirea wrote:
>> On Mon, 2018-05-14 at 20:38 +0300, Andy Shevchenko wrote:
>
>>> So, what is not going as expected in "SPI core takes care of CSs"
>>> case?
>>> Did you use oscilloscope for that?
>
>> Yes, I used and CSs was not asserted. Anyway, I will will try again.
>
> If the core chip select handling is not working properly for some reason
> then the core chip select handling should be fixed rather than just open
> coding in your driver - probably it's also broken for other users.
>
Hi Mark,
I found the fix for cs-gpios. If I change spi_add_device function like
this(see below) everything is ok.
int spi_add_device(struct spi_device *spi)
...
if (ctlr->cs_gpios){
spi->cs_gpio = ctlr->cs_gpios[spi->chip_select];
if(gpio_is_valid(spi->cs_gpio))
gpio_direction_output(spi->cs_gpio, !(spi->mode &
SPI_CS_HIGH));
}
...
return status;
}
In the subsystem gpio direction of pins is never set and
gpio_set_value() don't set the direction.
In my opinion gpio_direction_output() set direction should be called in
spi_add_device. What do you think? Is ok?
^ permalink raw reply
* Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))
From: Tony Lindgren @ 2018-05-24 13:32 UTC (permalink / raw)
To: Johan Hovold
Cc: Sebastian Reichel, H. Nikolaus Schaller, Andreas Kemnade,
Mark Rutland, Arnd Bergmann, Pavel Machek,
linux-kernel@vger.kernel.org,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Rob Herring, linux-serial, linux-omap,
linux-pm, Andy Shevchenko
In-Reply-To: <20180524091742.GZ30172@localhost>
* Johan Hovold <johan@kernel.org> [180524 09:20]:
> On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> >
> > Yes the bug for closed ports needs to be fixed for sure.
>
> I did some forensic on this and it seems this problem has "always" been
> there. Specifically, closed ports have never been runtime suspended
> unless a non-negative autosuspend delay has been set by user space since
> fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
> driver") which was merged seven years ago.
>
> So while it would certainly be nice to save some more power by default,
> this would really be a new feature rather than a bug or regression fix
> (which reduces the urgency for this issue somewhat too).
Yes it's been there since the start.
> > > > > 2. aggressive serial RPM, where the controller is allowed to
> > > > > suspend while the port is open even though this may result in
> > > > > lost characters when waking up on incoming data
> > > >
> > > > In this case it seems that the only thing needed is to just
> > > > configure the autosuspend delay for the parent port. The use of
> > > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > > we should just document it. I guess we could also introduce
> > > > pm_runtime_block_autoidle_unless_configured() :)
> > >
> > > The implications of a negative autosuspend delay are already documented
> > > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > > gets it wrong when trying to do things which aren't currently supported
> > > (and never have been).
> > >
> > > So I still think we need a new mechanism for this.
> >
> > Well if you have some better mechanism in mind let's try it out. Short of
> > sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> > right now.
>
> Yeah, that would be too much of a hack and likely wouldn't work either
> (and we really should do away with those _force calls altogether).
>
> I've been thinking a bit too much about this already, but it may be
> possible to use the pm QoS framework for this. A resume latency can be
> set through sysfs where "n/a" is defined to mean "no latency accepted"
> (i.e. controller remains always-on while port is open) and "0" means
> "any latency accepted" (i.e. omap aggressive serial RPM is allowed).
Oh yeah, PM QoS might work here!
> Now, implementing this may get a little tricky as we want to be able to
> change this setting on the fly (consider consoles) and we need to figure
> out the interaction with serdev (user space should probably not be
> allowed to request a resume latency for ports used by serdev).
It sounds like Andy Shevchenko has a series of patches that just might
allow us to make this all generic for Linux serial framework. So adding
Andy to Cc, I don't think he has posted all the patches yet.
Andy, see the PM QoS comment above for console idling :)
> I'd be happy to dig into this some more, but not in my spare time I'm
> afraid.
Indeed.
> > > > > For normal ttys, we need a user-space interface for selecting between
> > > > > the two, and for serdev we may want a way to select the RPM scheme from
> > > > > within the kernel.
> > > > >
> > > > > Note that with my serdev controller runtime PM patch, serdev core could
> > > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > > reference for the controller while the port is open).
> > > >
> > > > So if your serdev controller was to set the parent autosuspend
> > > > delay on open() and set it back on close() this should work?
> > >
> > > Is it really the job of a serdev driver to set the autosuspend delay of
> > > a parent controller? Isn't this somethings which depends on the
> > > characteristics of the controller (possibly configurable by user space)
> > > such as the cost of runtime suspending and resuming?
> >
> > Only in some cases will the serdev driver know it's safe to configure
> > the parent controller. Configuring the parent controller from userspace
> > works just fine as we've seen for years now.
>
> Yes, user space may override the default settings provided by the serial
> driver, but a serdev driver, in contrast, knows nothing about the
> underlying serial hardware.
>
> > > The patch I posted works with what we have today; if a parent serial
> > > controller driver uses aggressive runtime PM by default or after having
> > > been configured through sysfs to do so.
> >
> > Yeah let's stick with configuring the parent controller from userspace
> > for now at least.
>
> Yep, status quo works for the time being (since this isn't a
> regression).
>
> > > What I'm getting at here is that the delay should be set by the serial
> > > driver implementing aggressive runtime PM. Then all we need is a
> > > mechanism to determine whether an extra RPM reference should be taken at
> > > tty open or not (configurable by user space, defaulting to yes).
> >
> > OK yeah some additional on/off switch seems to be missing here.
>
> As mentioned above, PM QoS resume latency may possibly be used, and
> otherwise me may able to define a new (generic) QoS flag for this.
Good idea.
> > > Specifically, the serial drivers themselves would always use
> > > autosuspend and not have to deal with supporting the two RPM schemes
> > > (normal vs aggressive runtime PM).
> >
> > OK. So if I understand your idea right, we could have autosuspend timeout
> > set to 3000ms in the 8250_omap.c but still default to RPM blocked?
> > Then user can enable aggressive PM via /sys as desired, right?
>
> Not RPM blocked; the ports must always be able to suspend when the port
> is closed. But user space should be able to enable the aggressive
> (active) runtime PM via sysfs independently of the autosuspend delay,
> yes.
Yup OK, I like the PM QoS approach.
Regards,
Tony
^ permalink raw reply
* Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))
From: Johan Hovold @ 2018-05-24 9:17 UTC (permalink / raw)
To: Tony Lindgren
Cc: Johan Hovold, Sebastian Reichel, H. Nikolaus Schaller,
Andreas Kemnade, Mark Rutland, Arnd Bergmann, Pavel Machek,
linux-kernel@vger.kernel.org,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Rob Herring, linux-serial, linux-omap,
linux-pm
In-Reply-To: <20180521154832.GY98604@atomide.com>
On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180521 13:50]:
> > On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [180517 10:12]:
> > > Well in that case we should just stick with -1 value for the
> > > autosuspend. And just do pm_runtime_put_sync_suspend() after
> > > probe and on close.
> >
> > That won't work either as a negative autosuspend delay prevents runtime
> > suspend completely (by grabbing an extra RPM reference).
>
> Well so negative autosuspend delay is working as documented then :)
Yes, indeed. All too well. ;)
> > > > I fail to see how we can implement this using the current toolbox. What
> > > > you're after here is really a mechanism for selecting between two
> > > > different runtime PM schemes at runtime:
> > > >
> > > > 1. normal serial RPM, where the controller is active while the
> > > > port is open (this should be the safe default)
> > >
> > > Agreed. And that is the case already.
> >
> > Yes, but it's not really the case today as since omap-serial (and
> > 8250-omap) sets a negative autosuspend at probe and hence does not
> > runtime-suspend when the port is closed. So that's the long-standing bug
> > which needs fixing.
>
> Yes the bug for closed ports needs to be fixed for sure.
I did some forensic on this and it seems this problem has "always" been
there. Specifically, closed ports have never been runtime suspended
unless a non-negative autosuspend delay has been set by user space since
fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
driver") which was merged seven years ago.
So while it would certainly be nice to save some more power by default,
this would really be a new feature rather than a bug or regression fix
(which reduces the urgency for this issue somewhat too).
> > > > 2. aggressive serial RPM, where the controller is allowed to
> > > > suspend while the port is open even though this may result in
> > > > lost characters when waking up on incoming data
> > >
> > > In this case it seems that the only thing needed is to just
> > > configure the autosuspend delay for the parent port. The use of
> > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > we should just document it. I guess we could also introduce
> > > pm_runtime_block_autoidle_unless_configured() :)
> >
> > The implications of a negative autosuspend delay are already documented
> > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > gets it wrong when trying to do things which aren't currently supported
> > (and never have been).
> >
> > So I still think we need a new mechanism for this.
>
> Well if you have some better mechanism in mind let's try it out. Short of
> sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> right now.
Yeah, that would be too much of a hack and likely wouldn't work either
(and we really should do away with those _force calls altogether).
I've been thinking a bit too much about this already, but it may be
possible to use the pm QoS framework for this. A resume latency can be
set through sysfs where "n/a" is defined to mean "no latency accepted"
(i.e. controller remains always-on while port is open) and "0" means
"any latency accepted" (i.e. omap aggressive serial RPM is allowed).
Now, implementing this may get a little tricky as we want to be able to
change this setting on the fly (consider consoles) and we need to figure
out the interaction with serdev (user space should probably not be
allowed to request a resume latency for ports used by serdev).
I'd be happy to dig into this some more, but not in my spare time I'm
afraid.
> > > > For normal ttys, we need a user-space interface for selecting between
> > > > the two, and for serdev we may want a way to select the RPM scheme from
> > > > within the kernel.
> > > >
> > > > Note that with my serdev controller runtime PM patch, serdev core could
> > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > reference for the controller while the port is open).
> > >
> > > So if your serdev controller was to set the parent autosuspend
> > > delay on open() and set it back on close() this should work?
> >
> > Is it really the job of a serdev driver to set the autosuspend delay of
> > a parent controller? Isn't this somethings which depends on the
> > characteristics of the controller (possibly configurable by user space)
> > such as the cost of runtime suspending and resuming?
>
> Only in some cases will the serdev driver know it's safe to configure
> the parent controller. Configuring the parent controller from userspace
> works just fine as we've seen for years now.
Yes, user space may override the default settings provided by the serial
driver, but a serdev driver, in contrast, knows nothing about the
underlying serial hardware.
> > The patch I posted works with what we have today; if a parent serial
> > controller driver uses aggressive runtime PM by default or after having
> > been configured through sysfs to do so.
>
> Yeah let's stick with configuring the parent controller from userspace
> for now at least.
Yep, status quo works for the time being (since this isn't a
regression).
> > What I'm getting at here is that the delay should be set by the serial
> > driver implementing aggressive runtime PM. Then all we need is a
> > mechanism to determine whether an extra RPM reference should be taken at
> > tty open or not (configurable by user space, defaulting to yes).
>
> OK yeah some additional on/off switch seems to be missing here.
As mentioned above, PM QoS resume latency may possibly be used, and
otherwise me may able to define a new (generic) QoS flag for this.
> > Specifically, the serial drivers themselves would always use
> > autosuspend and not have to deal with supporting the two RPM schemes
> > (normal vs aggressive runtime PM).
>
> OK. So if I understand your idea right, we could have autosuspend timeout
> set to 3000ms in the 8250_omap.c but still default to RPM blocked?
> Then user can enable aggressive PM via /sys as desired, right?
Not RPM blocked; the ports must always be able to suspend when the port
is closed. But user space should be able to enable the aggressive
(active) runtime PM via sysfs independently of the autosuspend delay,
yes.
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Tony Lindgren @ 2018-05-23 18:32 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Sebastian Andrzej Siewior, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <20180523175859.GO98604@atomide.com>
* Tony Lindgren <tony@atomide.com> [180523 10:58]:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [180522 21:42]:
> > Have you seen entire series which I keep here:
> > https://bitbucket.org/andy-shev/linux/branch/topic/uart/rpm?
> > Among other things it gets rid of those specific callbacks entirely.
>
> Well I was not Cc:ed on it, I browsed it in some archive and it
> seemed unsafe to me. But if you figured out a way to do it conditionally
> based on console.idle without causing regressions.
Sorry incomplete sentence here.. If you have some branch that
based on console.idle I'd be happy to test it.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Tony Lindgren @ 2018-05-23 18:00 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Sebastian Andrzej Siewior, Andy Shevchenko, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <CAHp75Vf7L3zsSgaj1vfoyNKdPo0w6Nu+mW5HuM97SKAK-1y2Pg@mail.gmail.com>
* Andy Shevchenko <andy.shevchenko@gmail.com> [180522 21:54]:
> On Thu, May 17, 2018 at 10:48 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:38]:
> >> On Thu, May 17, 2018 at 4:48 PM, Tony Lindgren <tony@atomide.com> wrote:
> >> > * Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516 10:49]:
>
> >> > The idea breaking PM seems silly to me considering that we've had
> >> > it working for years already.
> >>
> >> Same question / note. World is much more complex than just being OMAP.
> >
> > Sorry but you are making assumptions about hardware being powered on
> > all the time.
>
> Nope, other way around. The so called "support" _prevents_ our
> hardware to go to sleep.
Hmm sorry now I'm all confused what issues you're having.
I thought you said earlier the issue was that you wanted to keep
the console enabled all the time and never idle?
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Tony Lindgren @ 2018-05-23 17:58 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Sebastian Andrzej Siewior, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <CAHp75Ve56pyEC41XZ9i9yxBNberQe=ee2miQL0v9T4vRtkWPBQ@mail.gmail.com>
* Andy Shevchenko <andy.shevchenko@gmail.com> [180522 21:42]:
> On Thu, May 17, 2018 at 10:30 PM, Tony Lindgren <tony@atomide.com> wrote:
> > So how about add some "noidle" kernel command line parameter for console
> > that calls
> > pm_runtime_forbid() and then you have the UART permanently
> > on.
>
> IIUC _forbid() can be overwritten via sysfs.
> And I would prefer to do other way around, something like console.idle
> and put default for OMAP to yes and no for everything else.
OK yeah console.idle sounds good to me. We should default to a
safe option.
> > Hmm I guess you could make also serial8250_rpm_get() do nothing
> > based on that.
>
> Have you seen entire series which I keep here:
> https://bitbucket.org/andy-shev/linux/branch/topic/uart/rpm?
> Among other things it gets rid of those specific callbacks entirely.
Well I was not Cc:ed on it, I browsed it in some archive and it
seemed unsafe to me. But if you figured out a way to do it conditionally
based on console.idle without causing regressions.
> > I do agree the serial runtime PM has an issue if it depends on
> > pm_runtime_irq_safe() being set.
>
> It's more than an issue. The so called "support" of RPM for UART is
> _based on the hack_.
> I would love to NAK that in the first place if I would have known of it in time.
Hmm well it seems that you too have been patching the 8250_rpm
functions for years and then now what after multiple years you
hit this issue? :)
> >> So, I can, of course just remove callbacks from the console ->write().
> >> Though it will prevent to use kernel console anyway.
> >
> > Please et's not start breaking things, we already see a constant
> > flow of regressions on weekly basis.
>
> Now we are stick with a hack and the case based on that is against
> fixing things.
> This is how it looks from my side.
Sorry yeah I agree there are issues, but let's fix it properly with
no regressions.
Regards,
TOny
^ permalink raw reply
* Re: [PATCH v3 5/6] spi: at91-usart: add driver for at91-usart as spi
From: Mark Brown @ 2018-05-23 8:30 UTC (permalink / raw)
To: Radu Pirea
Cc: devicetree, linux-serial, linux-kernel, linux-arm-kernel,
linux-spi, mark.rutland, robh+dt, lee.jones, gregkh, jslaby,
richard.genoud, alexandre.belloni, nicolas.ferre
In-Reply-To: <6a59e071-3159-4939-8535-6c7a9d491379@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 531 bytes --]
On Wed, May 23, 2018 at 11:10:28AM +0300, Radu Pirea wrote:
> On 05/17/2018 08:04 AM, Mark Brown wrote:
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver for AT91 USART Controllers as SPI
> > > + *
> > > + * Copyright (C) 2018 Microchip Technology Inc.
> > Make the entire block a C++ comment so it looks more intentional rather
> > tha mixing C and C++.
> I know it's ugly, but SPDX license identifier must be in a separate comment
> block.
No, it doesn't - it just needs to be the first line of the file.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v3 5/6] spi: at91-usart: add driver for at91-usart as spi
From: Radu Pirea @ 2018-05-23 8:10 UTC (permalink / raw)
To: Mark Brown
Cc: devicetree, linux-serial, linux-kernel, linux-arm-kernel,
linux-spi, mark.rutland, robh+dt, lee.jones, gregkh, jslaby,
richard.genoud, alexandre.belloni, nicolas.ferre
In-Reply-To: <20180517050406.GF20254@sirena.org.uk>
On 05/17/2018 08:04 AM, Mark Brown wrote:
> On Fri, May 11, 2018 at 01:38:21PM +0300, Radu Pirea wrote:
>
>> +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.
>> +
>
> This looks like there's some tab/space mixing going on here.
>
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for AT91 USART Controllers as SPI
>> + *
>> + * Copyright (C) 2018 Microchip Technology Inc.
>
> Make the entire block a C++ comment so it looks more intentional rather
> tha mixing C and C++.
Hi Mark,
I know it's ugly, but SPDX license identifier must be in a separate
comment block.
>
>> +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--;
>
> Missing braces here - we only write to the FIFO if there's space but we
> unconditionally decrement the counter.
>
Thanks. I will fix it.
>> + } else {
>> + if (at91_usart_spi_tx_ready(aus))
>> + spi_writel(aus, THR, US_DUMMY_TX);
>> + }
>> +}
>
> This looks like you're open coding SPI_CONTROLLER_MUST_TX
>
>> + 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);
>> + }
>
> Similarly for _MUST_RX.
>
>> + controller->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;
>
> You're actually setting both flags... this means that the handling for
> cases with missing TX or RX buffers can't happen.
Sorry. My mistake. I will remove unnecessary code.
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Andy Shevchenko @ 2018-05-22 21:52 UTC (permalink / raw)
To: Tony Lindgren
Cc: Sebastian Andrzej Siewior, Andy Shevchenko, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <20180517194834.GW25808@atomide.com>
On Thu, May 17, 2018 at 10:48 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:38]:
>> On Thu, May 17, 2018 at 4:48 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516 10:49]:
>> > The idea breaking PM seems silly to me considering that we've had
>> > it working for years already.
>>
>> Same question / note. World is much more complex than just being OMAP.
>
> Sorry but you are making assumptions about hardware being powered on
> all the time.
Nope, other way around. The so called "support" _prevents_ our
hardware to go to sleep.
So it's not breaking, it's fixing. It's pity that OMAP solution is just a hack.
> It may have been a safe assumption up to mid-90's and might still be
> valid assumption for hardware providing on MS-DOS boot floppy
> compability.
>
> But that is not a safe assumption for Linux kernel at all. Especially
> for console TX where there's no need to keep it powered unless there
> is actually something to write.
>
> If there are runtime PM issues for consoles, let's just fix them.
Yes, there is an issue to begin with, i.e. irq_safe hack.
Removal of that hack reveals the real issues with the code.
> Then for really seeing console messages on next reboot from a hung
> system, CONFIG_PSTORE_CONSOLE option seems to do a very good job.
How this helps to prevent a more serious system crashes during an
attempt to power UART on?
> With a warm reset after triggered by watchdog the console messages
> are readable most of the time even when using DDR as the back end.
Ditto.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Andy Shevchenko @ 2018-05-22 21:39 UTC (permalink / raw)
To: Tony Lindgren
Cc: Andy Shevchenko, Sebastian Andrzej Siewior, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <20180517193008.GV25808@atomide.com>
On Thu, May 17, 2018 at 10:30 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:41]:
>> On Thu, May 17, 2018 at 4:56 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180516 13:12]:
>> >> On Wed, 2018-05-16 at 12:47 +0200, Sebastian Andrzej Siewior wrote:
>> >> > But since I am on it. You have to enable runtime-PM for the UART. So
>> >> > what is the problem if you simply don't enable it for the UART which
>> >> > used as the kernel console?
>> >>
>> >> How do I know at the ->probe() time that device in question is going to
>> >> be kernel console? Maybe I missed simple way of it.
>> >
>> > Hmm parse the kernel cmdline maybe? :)
>> >
>> > BTW, kernel already has earlycon doing exactly what you're trying to do.
>>
>> I'm sorry, I didn't follow. What exactly earlycon does?
>
> It provides a console very early on, see earlycon in
> Documentation/admin-guide/kernel-parameters.txt
Yes, but what we are talking is to put the kernel console out of power
management due to _real_ issues with it.
>> The problem is in 8250 driver. The issue with runtime PM used in atomic context.
>
> So how about add some "noidle" kernel command line parameter for console
> that calls
> pm_runtime_forbid() and then you have the UART permanently
> on.
IIUC _forbid() can be overwritten via sysfs.
And I would prefer to do other way around, something like console.idle
and put default for OMAP to yes and no for everything else.
> Hmm I guess you could make also serial8250_rpm_get() do nothing
> based on that.
Have you seen entire series which I keep here:
https://bitbucket.org/andy-shev/linux/branch/topic/uart/rpm?
Among other things it gets rid of those specific callbacks entirely.
> I do agree the serial runtime PM has an issue if it depends on
> pm_runtime_irq_safe() being set.
It's more than an issue. The so called "support" of RPM for UART is
_based on the hack_.
I would love to NAK that in the first place if I would have known of it in time.
>> So, I can, of course just remove callbacks from the console ->write().
>> Though it will prevent to use kernel console anyway.
>
> Please et's not start breaking things, we already see a constant
> flow of regressions on weekly basis.
Now we are stick with a hack and the case based on that is against
fixing things.
This is how it looks from my side.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
From: Andy Shevchenko @ 2018-05-22 20:21 UTC (permalink / raw)
To: Oleksandr Shamray
Cc: Greg Kroah-Hartman, Arnd Bergmann, Linux Kernel Mailing List,
linux-arm Mailing List, devicetree, openbmc@lists.ozlabs.org,
Joel Stanley, Jiří Pírko, Tobias Klauser,
open list:SERIAL DRIVERS, Vadim Pasternak, system-sw-low-level,
Rob Herring, openocd-devel-owner@lists.sourceforge.net,
linux-api@vger.kernel.org
In-Reply-To: <AM5PR0501MB244981D83DD33BD29FD64924B1940@AM5PR0501MB2449.eurprd05.prod.outlook.com>
On Tue, May 22, 2018 at 6:00 PM, Oleksandr Shamray
<oleksandrs@mellanox.com> wrote:
> Ok. Changed to:
> #define ASPEED_JTAG_IOUT_LEN(len) \
> (ASPEED_JTAG_CTL_ENG_EN | \
> ASPEED_JTAG_CTL_ENG_OUT_EN | \
> ASPEED_JTAG_CTL_INST_LEN(len))
>
> #define ASPEED_JTAG_DOUT_LEN(len) \
> (ASPEED_JTAG_CTL_ENG_EN | \
> ASPEED_JTAG_CTL_ENG_OUT_EN | \
> ASPEED_JTAG_CTL_DATA_LEN(len))
What about
#define _JTAG_OUT_ENABLE \
( _ENG_EN | _ENG_OUT_EN)
#define _IOUT_LEN(len) \
(_ENABLE | _INST_LEN(len))
#define _DOUT_LEN(len) \
...
?
>> > + apb_frq = clk_get_rate(aspeed_jtag->pclk);
>>
>> > + div = (apb_frq % freq == 0) ? (apb_frq / freq) - 1 :
>> > + (apb_frq / freq);
>>
>> Isn't it the same as
>>
>> div = (apb_frq - 1) / freq;
>>
>> ?
> Seems it is same. Thanks.
Though be careful if apb_frq == 0.
In either case the hw will be screwed, but differently.
>> > + if (xfer->direction == JTAG_READ_XFER)
>> > + tdi = UINT_MAX;
>> > + else
>> > + tdi = data[index];
>>
>> > + if (xfer->direction == JTAG_READ_XFER)
>> > + tdi = UINT_MAX;
>> > + else
>> > + tdi = data[index];
>>
>> Take your time to think how the above duplication can be avoided.
>>
>
> In both cases data[] is different, so I should check it twice, but I will
> change it to, macro like:
>
> #define ASPEED_JTAG_GET_TDI(direction, data) \
> (direction == JTAG_READ_XFER) ? UNIT_MAX : data
Perhaps choose better name for data, b/c in the above you are using data[index].
>> > + dev_err(aspeed_jtag->dev, "irq status:%x\n",
>> > + status);
>> Huh, really?! SPAM.
> I will review and delete redundant debug messages.
Just to be sure you got a point. This is interrupt context. Imagine
what might go wrong.
>> > + err = jtag_register(jtag);
>>
>> Perhaps we might have devm_ variant of this. Check how SPI framework
>> deal with a such.
>>
>
> Jtag driver uses miscdevice and related misc_register and misc_deregister
> calls for creation and destruction. There is no device object prior
> to call to misc_register, which could be used in devm_jtag_register.
Same question as per previous patch.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/8] dt-bindings: serial: Add bindings for nvidia,tegra194-tcu
From: Rob Herring @ 2018-05-22 20:20 UTC (permalink / raw)
To: Mikko Perttunen
Cc: mark.rutland, jassisinghbrar, gregkh, thierry.reding, jonathanh,
araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel
In-Reply-To: <20180508114403.14499-3-mperttunen@nvidia.com>
On Tue, May 08, 2018 at 02:43:57PM +0300, Mikko Perttunen wrote:
> Add bindings for the Tegra Combined UART device used to talk to the
> UART console on Tegra194 systems.
>
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> .../bindings/serial/nvidia,tegra194-tcu.txt | 35 ++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
>
> diff --git a/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt b/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
> new file mode 100644
> index 000000000000..86763bc5d74f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
> @@ -0,0 +1,35 @@
> +NVIDIA Tegra Combined UART (TCU)
> +
> +The TCU is a system for sharing a hardware UART instance among multiple
> +systems withing the Tegra SoC. It is implemented through a mailbox-
s/withing/within/
Otherwise,
Reviewed-by: Rob Herring <robh@kernel.org>
> +based protocol where each "virtual UART" has a pair of mailboxes, one
> +for transmitting and one for receiving, that is used to communicate
> +with the hardware implementing the TCU.
> +
> +Required properties:
> +- name : Should be tcu
> +- compatible
> + Array of strings
> + One of:
> + - "nvidia,tegra194-tcu"
> +- mbox-names:
> + "rx" - Mailbox for receiving data from hardware UART
> + "tx" - Mailbox for transmitting data to hardware UART
> +- mboxes: Mailboxes corresponding to the mbox-names.
> +
> +This node is a mailbox consumer. See the following files for details of
> +the mailbox subsystem, and the specifiers implemented by the relevant
> +provider(s):
> +
> +- .../mailbox/mailbox.txt
> +- .../mailbox/nvidia,tegra186-hsp.txt
> +
> +Example bindings:
> +-----------------
> +
> +tcu: tcu {
> + compatible = "nvidia,tegra194-tcu";
> + mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_SM 0>,
> + <&hsp_aon TEGRA_HSP_MBOX_TYPE_SM 1>;
> + mbox-names = "rx", "tx";
> +};
> --
> 2.16.1
>
^ permalink raw reply
* Re: [PATCH 2/8] dt-bindings: serial: Add bindings for nvidia,tegra194-tcu
From: Rob Herring @ 2018-05-22 20:18 UTC (permalink / raw)
To: Jon Hunter
Cc: Mikko Perttunen, mark.rutland, jassisinghbrar, gregkh,
thierry.reding, araza, devicetree, linux-serial, linux-tegra,
linux-arm-kernel, linux-kernel
In-Reply-To: <3ddaafbd-d8cb-3cca-be4e-8c5c53fd9734@nvidia.com>
On Tue, May 22, 2018 at 04:15:09PM +0100, Jon Hunter wrote:
>
> On 08/05/18 12:43, Mikko Perttunen wrote:
> > Add bindings for the Tegra Combined UART device used to talk to the
> > UART console on Tegra194 systems.
> >
> > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> > ---
> > .../bindings/serial/nvidia,tegra194-tcu.txt | 35 ++++++++++++++++++++++
> > 1 file changed, 35 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
> >
> > diff --git a/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt b/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
> > new file mode 100644
> > index 000000000000..86763bc5d74f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
> > @@ -0,0 +1,35 @@
> > +NVIDIA Tegra Combined UART (TCU)
> > +
> > +The TCU is a system for sharing a hardware UART instance among multiple
> > +systems withing the Tegra SoC. It is implemented through a mailbox-
> > +based protocol where each "virtual UART" has a pair of mailboxes, one
> > +for transmitting and one for receiving, that is used to communicate
> > +with the hardware implementing the TCU.
> > +
> > +Required properties:
> > +- name : Should be tcu
> > +- compatible
> > + Array of strings
> > + One of:
> > + - "nvidia,tegra194-tcu"
>
> Nit. We should say what device the above compatibility is applicable for ...
>
> - "nvidia,tegra194-tcu": for Tegra194
Yeah, everyone seems to do that, but I find it to be redundant.
Rob
^ permalink raw reply
* Re: [PATCH v3 3/4] dt-bindings: serial: Add compatible for Mediatek MT8183
From: Rob Herring @ 2018-05-22 17:15 UTC (permalink / raw)
To: Erin Lo
Cc: Matthias Brugger, Mark Rutland, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Greg Kroah-Hartman, devicetree, srv_heupstream,
linux-kernel, linux-serial, linux-mediatek, linux-arm-kernel,
yingjoe.chen, mars.cheng
In-Reply-To: <1526538126-51497-4-git-send-email-erin.lo@mediatek.com>
On Thu, May 17, 2018 at 02:22:05PM +0800, Erin Lo wrote:
> This adds dt-binding documentation of uart for Mediatek MT8183 SoC
> Platform.
>
> Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> ---
> Documentation/devicetree/bindings/serial/mtk-uart.txt | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 2/4] dt-bindings: mtk-sysirq: Add compatible for Mediatek MT8183
From: Rob Herring @ 2018-05-22 17:15 UTC (permalink / raw)
To: Erin Lo
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Jason Cooper,
srv_heupstream, Marc Zyngier, Greg Kroah-Hartman,
mars.cheng-NuS5LvNUpcJWk0Htik3J/w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Matthias Brugger,
yingjoe.chen-NuS5LvNUpcJWk0Htik3J/w, Thomas Gleixner,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1526538126-51497-3-git-send-email-erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
On Thu, May 17, 2018 at 02:22:04PM +0800, Erin Lo wrote:
> This adds dt-binding documentation of SYSIRQ for Mediatek MT8183 SoC
> Platform.
>
> Signed-off-by: Erin Lo <erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
> .../devicetree/bindings/interrupt-controller/mediatek,sysirq.txt | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: arm: Add bindings for Mediatek MT8183 SoC Platform
From: Rob Herring @ 2018-05-22 17:14 UTC (permalink / raw)
To: Erin Lo
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Jason Cooper,
srv_heupstream, Marc Zyngier, Greg Kroah-Hartman,
mars.cheng-NuS5LvNUpcJWk0Htik3J/w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Matthias Brugger,
yingjoe.chen-NuS5LvNUpcJWk0Htik3J/w, Thomas Gleixner,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1526538126-51497-2-git-send-email-erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
On Thu, May 17, 2018 at 02:22:03PM +0800, Erin Lo wrote:
> This adds dt-binding documentation of cpu for Mediatek MT8183.
>
> Signed-off-by: Erin Lo <erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
> Documentation/devicetree/bindings/arm/mediatek.txt | 4 ++++
> 1 file changed, 4 insertions(+)
Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
^ permalink raw reply
* Re: [PATCH 8/8] arm64: tegra: Mark tcu as primary serial port on Tegra194 P2888
From: Jon Hunter @ 2018-05-22 16:29 UTC (permalink / raw)
To: Mikko Perttunen, robh+dt, mark.rutland, jassisinghbrar, gregkh,
thierry.reding
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel
In-Reply-To: <20180508114403.14499-9-mperttunen@nvidia.com>
On 08/05/18 12:44, Mikko Perttunen wrote:
> The Tegra Combined UART is the proper primary serial port on P2888,
> so use it.
>
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi b/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
> index 859ab5af17c1..95e2433984f7 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
> @@ -10,7 +10,7 @@
> aliases {
> sdhci0 = "/cbb/sdhci@3460000";
> sdhci1 = "/cbb/sdhci@3400000";
> - serial0 = &uartb;
> + serial0 = &tcu;
> i2c0 = "/bpmp/i2c";
> i2c1 = "/cbb/i2c@3160000";
> i2c2 = "/cbb/i2c@c240000";
>
Acked-by: Jon Hunter <jonathanh@nvidia.com>
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH 7/8] arm64: tegra: Add nodes for tcu on Tegra194
From: Jon Hunter @ 2018-05-22 16:28 UTC (permalink / raw)
To: Mikko Perttunen, robh+dt, mark.rutland, jassisinghbrar, gregkh,
thierry.reding
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel
In-Reply-To: <20180508114403.14499-8-mperttunen@nvidia.com>
On 08/05/18 12:44, Mikko Perttunen wrote:
> Add nodes required for communication through the Tegra Combined UART.
> This includes the AON HSP instance, addition of shared interrupts
> for the TOP0 HSP instance, and finally the TCU node itself. Also
> mark the HSP instances as compatible to tegra194-hsp, as the hardware
> is not identical but is compatible to tegra186-hsp.
>
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> arch/arm64/boot/dts/nvidia/tegra194.dtsi | 34 +++++++++++++++++++++++++++++---
> 1 file changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
> index 6d699815a84f..d7f780b06fe2 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
> @@ -217,10 +217,31 @@
> };
>
> hsp_top0: hsp@3c00000 {
> - compatible = "nvidia,tegra186-hsp";
> + compatible = "nvidia,tegra194-hsp", "nvidia,tegra186-hsp";
I might be wrong, but I think we may need to update the binding doc
compatibility property for hsp. For example see
'Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-flowctrl.txt'.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH 5/8] mailbox: tegra-hsp: Add support for shared mailboxes
From: Jon Hunter @ 2018-05-22 16:20 UTC (permalink / raw)
To: Mikko Perttunen, robh+dt, mark.rutland, jassisinghbrar, gregkh,
thierry.reding
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel
In-Reply-To: <20180508114403.14499-6-mperttunen@nvidia.com>
On 08/05/18 12:44, Mikko Perttunen wrote:
> The Tegra HSP block supports 'shared mailboxes' that are simple 32-bit
> registers consisting of a FULL bit in MSB position and 31 bits of data.
> The hardware can be configured to trigger interrupts when a mailbox
> is empty or full. Add support for these shared mailboxes to the HSP
> driver.
>
> The initial use for the mailboxes is the Tegra Combined UART. For this
> purpose, we use interrupts to receive data, and spinning to wait for
> the transmit mailbox to be emptied to minimize unnecessary overhead.
>
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> drivers/mailbox/tegra-hsp.c | 216 +++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 193 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
> index 16eb970f2c9f..77bc8ed7ef15 100644
> --- a/drivers/mailbox/tegra-hsp.c
> +++ b/drivers/mailbox/tegra-hsp.c
> @@ -21,6 +21,11 @@
>
> #include <dt-bindings/mailbox/tegra186-hsp.h>
>
> +#include "mailbox.h"
> +
> +#define HSP_INT0_IE 0x100
> +#define HSP_INT_IR 0x304
> +
> #define HSP_INT_DIMENSIONING 0x380
> #define HSP_nSM_SHIFT 0
> #define HSP_nSS_SHIFT 4
> @@ -34,6 +39,8 @@
> #define HSP_DB_RAW 0x8
> #define HSP_DB_PENDING 0xc
>
> +#define HSP_SM_SHRD_MBOX 0x0
> +
> #define HSP_DB_CCPLEX 1
> #define HSP_DB_BPMP 3
> #define HSP_DB_MAX 7
> @@ -68,6 +75,18 @@ struct tegra_hsp_db_map {
> unsigned int index;
> };
>
> +struct tegra_hsp_mailbox {
> + struct tegra_hsp_channel channel;
> + unsigned int index;
> + bool sending;
> +};
> +
> +static inline struct tegra_hsp_mailbox *
> +channel_to_mailbox(struct tegra_hsp_channel *channel)
> +{
> + return container_of(channel, struct tegra_hsp_mailbox, channel);
> +}
> +
> struct tegra_hsp_soc {
> const struct tegra_hsp_db_map *map;
> };
> @@ -77,6 +96,7 @@ struct tegra_hsp {
> struct mbox_controller mbox;
> void __iomem *regs;
> unsigned int doorbell_irq;
> + unsigned int shared_irq;
> unsigned int num_sm;
> unsigned int num_as;
> unsigned int num_ss;
> @@ -85,6 +105,7 @@ struct tegra_hsp {
> spinlock_t lock;
>
> struct list_head doorbells;
> + struct tegra_hsp_mailbox *mailboxes;
> };
>
> static inline struct tegra_hsp *
> @@ -189,6 +210,35 @@ static irqreturn_t tegra_hsp_doorbell_irq(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t tegra_hsp_shared_irq(int irq, void *data)
> +{
> + struct tegra_hsp_mailbox *mb;
> + struct tegra_hsp *hsp = data;
> + unsigned long bit, mask;
> + u32 value;
> +
> + mask = tegra_hsp_readl(hsp, HSP_INT_IR);
> + /* Only interested in FULL interrupts */
> + mask &= 0xff << 8;
Maybe add some definitions for the above.
Should we qualify 'mask' against the HSP_INT_IE register as well?
> +
> + for_each_set_bit(bit, &mask, 16) {
> + unsigned int mb_i = bit % 8;
If you right-shifted the mask above, you could avoid this modulo.
> +
> + mb = &hsp->mailboxes[mb_i];
> +
> + if (!mb->sending) {
> + value = tegra_hsp_channel_readl(&mb->channel,
> + HSP_SM_SHRD_MBOX);
> + value &= ~BIT(31);
Similarly a definition for bit 31 may add some clarity.
> + mbox_chan_received_data(mb->channel.chan, &value);
> + tegra_hsp_channel_writel(&mb->channel, value,
> + HSP_SM_SHRD_MBOX);
> + }
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> static struct tegra_hsp_channel *
> tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
> unsigned int master, unsigned int index)
> @@ -277,15 +327,58 @@ static void tegra_hsp_doorbell_shutdown(struct tegra_hsp_doorbell *db)
> spin_unlock_irqrestore(&hsp->lock, flags);
> }
>
> +static int tegra_hsp_mailbox_startup(struct tegra_hsp_mailbox *mb)
> +{
> + struct tegra_hsp *hsp = mb->channel.hsp;
> + u32 value;
> +
> + mb->channel.chan->txdone_method = TXDONE_BY_BLOCK;
> +
> + /* Route FULL interrupt to external IRQ 0 */
> + value = tegra_hsp_readl(hsp, HSP_INT0_IE);
> + value |= BIT(mb->index + 8);
> + tegra_hsp_writel(hsp, value, HSP_INT0_IE);
> +
> + return 0;
> +}
> +
> +static int tegra_hsp_mailbox_shutdown(struct tegra_hsp_mailbox *mb)
> +{
> + struct tegra_hsp *hsp = mb->channel.hsp;
> + u32 value;
> +
> + value = tegra_hsp_readl(hsp, HSP_INT0_IE);
> + value &= ~BIT(mb->index + 8);
> + tegra_hsp_writel(hsp, value, HSP_INT0_IE);
> +
> + return 0;
> +}
> +
> static int tegra_hsp_send_data(struct mbox_chan *chan, void *data)
> {
> struct tegra_hsp_channel *channel = chan->con_priv;
> - struct tegra_hsp_doorbell *db;
> + struct tegra_hsp_mailbox *mailbox;
> + uint32_t value;
>
> switch (channel->type) {
> case TEGRA_HSP_MBOX_TYPE_DB:
> - db = channel_to_doorbell(channel);
> - tegra_hsp_channel_writel(&db->channel, 1, HSP_DB_TRIGGER);
> + tegra_hsp_channel_writel(channel, 1, HSP_DB_TRIGGER);
> + return 0;
> + case TEGRA_HSP_MBOX_TYPE_SM:
> + mailbox = channel_to_mailbox(channel);
> + mailbox->sending = true;
> +
> + value = *(uint32_t *)data;
> + /* Mark mailbox full */
> + value |= BIT(31);
> +
> + tegra_hsp_channel_writel(channel, value, HSP_SM_SHRD_MBOX);
> +
> + while (tegra_hsp_channel_readl(channel, HSP_SM_SHRD_MBOX)
> + & BIT(31))
> + cpu_relax();
Could be nice to use readx_poll_timeout() here.
> +
> + return 0;
> }
>
> return -EINVAL;
> @@ -298,6 +391,8 @@ static int tegra_hsp_startup(struct mbox_chan *chan)
> switch (channel->type) {
> case TEGRA_HSP_MBOX_TYPE_DB:
> return tegra_hsp_doorbell_startup(channel_to_doorbell(channel));
> + case TEGRA_HSP_MBOX_TYPE_SM:
> + return tegra_hsp_mailbox_startup(channel_to_mailbox(channel));
> }
>
> return -EINVAL;
> @@ -311,6 +406,9 @@ static void tegra_hsp_shutdown(struct mbox_chan *chan)
> case TEGRA_HSP_MBOX_TYPE_DB:
> tegra_hsp_doorbell_shutdown(channel_to_doorbell(channel));
> break;
> + case TEGRA_HSP_MBOX_TYPE_SM:
> + tegra_hsp_mailbox_shutdown(channel_to_mailbox(channel));
> + break;
> }
> }
>
> @@ -364,7 +462,16 @@ static struct mbox_chan *of_tegra_hsp_xlate(struct mbox_controller *mbox,
>
> switch (type) {
> case TEGRA_HSP_MBOX_TYPE_DB:
> - return tegra_hsp_doorbell_xlate(hsp, param);
> + if (hsp->doorbell_irq)
> + return tegra_hsp_doorbell_xlate(hsp, param);
> + else
> + return ERR_PTR(-EINVAL);
> +
> + case TEGRA_HSP_MBOX_TYPE_SM:
> + if (hsp->shared_irq && param < hsp->num_sm)
> + return hsp->mailboxes[param].channel.chan;
> + else
> + return ERR_PTR(-EINVAL);
>
> default:
> return ERR_PTR(-EINVAL);
> @@ -403,6 +510,31 @@ static int tegra_hsp_add_doorbells(struct tegra_hsp *hsp)
> return 0;
> }
>
> +static int tegra_hsp_add_mailboxes(struct tegra_hsp *hsp, struct device *dev)
> +{
> + int i;
> +
> + hsp->mailboxes = devm_kcalloc(dev, hsp->num_sm, sizeof(*hsp->mailboxes),
> + GFP_KERNEL);
> + if (!hsp->mailboxes)
> + return -ENOMEM;
> +
> + for (i = 0; i < hsp->num_sm; i++) {
> + struct tegra_hsp_mailbox *mb = &hsp->mailboxes[i];
> +
> + mb->index = i;
> + mb->sending = false;
> +
> + mb->channel.hsp = hsp;
> + mb->channel.type = TEGRA_HSP_MBOX_TYPE_SM;
> + mb->channel.regs = hsp->regs + SZ_64K + i * SZ_32K;
> + mb->channel.chan = &hsp->mbox.chans[i];
> + mb->channel.chan->con_priv = &mb->channel;
> + }
> +
> + return 0;
> +}
> +
> static int tegra_hsp_probe(struct platform_device *pdev)
> {
> struct tegra_hsp *hsp;
> @@ -431,14 +563,19 @@ static int tegra_hsp_probe(struct platform_device *pdev)
> hsp->num_si = (value >> HSP_nSI_SHIFT) & HSP_nINT_MASK;
>
> err = platform_get_irq_byname(pdev, "doorbell");
> - if (err < 0) {
> - dev_err(&pdev->dev, "failed to get doorbell IRQ: %d\n", err);
> - return err;
> - }
> + if (err < 0)
> + hsp->doorbell_irq = 0;
It should not be necessary to set to zero as it should already be zero.
> + else
> + hsp->doorbell_irq = err;
>
> - hsp->doorbell_irq = err;
> + err = platform_get_irq_byname(pdev, "shared0");
> + if (err < 0)
> + hsp->shared_irq = 0;
It should not be necessary to set to zero as it should already be zero.
> + else
> + hsp->shared_irq = err;
>
> hsp->mbox.of_xlate = of_tegra_hsp_xlate;
> + /* First nSM are reserved for mailboxes */
> hsp->mbox.num_chans = 32;
> hsp->mbox.dev = &pdev->dev;
> hsp->mbox.txdone_irq = false;
> @@ -451,10 +588,22 @@ static int tegra_hsp_probe(struct platform_device *pdev)
> if (!hsp->mbox.chans)
> return -ENOMEM;
>
> - err = tegra_hsp_add_doorbells(hsp);
> - if (err < 0) {
> - dev_err(&pdev->dev, "failed to add doorbells: %d\n", err);
> - return err;
> + if (hsp->doorbell_irq) {
> + err = tegra_hsp_add_doorbells(hsp);
> + if (err < 0) {
> + dev_err(&pdev->dev, "failed to add doorbells: %d\n",
> + err);
> + return err;
> + }
> + }
> +
> + if (hsp->shared_irq) {
> + err = tegra_hsp_add_mailboxes(hsp, &pdev->dev);
> + if (err < 0) {
> + dev_err(&pdev->dev, "failed to add mailboxes: %d\n",
> + err);
> + goto remove_doorbells;
> + }
> }
>
> platform_set_drvdata(pdev, hsp);
> @@ -462,20 +611,40 @@ static int tegra_hsp_probe(struct platform_device *pdev)
> err = mbox_controller_register(&hsp->mbox);
> if (err) {
> dev_err(&pdev->dev, "failed to register mailbox: %d\n", err);
> - tegra_hsp_remove_doorbells(hsp);
> - return err;
> + goto remove_doorbells;
> }
>
> - err = devm_request_irq(&pdev->dev, hsp->doorbell_irq,
> - tegra_hsp_doorbell_irq, IRQF_NO_SUSPEND,
> - dev_name(&pdev->dev), hsp);
> - if (err < 0) {
> - dev_err(&pdev->dev, "failed to request doorbell IRQ#%u: %d\n",
> - hsp->doorbell_irq, err);
> - return err;
> + if (hsp->doorbell_irq) {
> + err = devm_request_irq(&pdev->dev, hsp->doorbell_irq,
> + tegra_hsp_doorbell_irq, IRQF_NO_SUSPEND,
> + dev_name(&pdev->dev), hsp);
> + if (err < 0) {
> + dev_err(&pdev->dev,
> + "failed to request doorbell IRQ#%u: %d\n",
> + hsp->doorbell_irq, err);
> + return err;
Clean-up?
> + }
> + }
> +
> + if (hsp->shared_irq) {
> + err = devm_request_irq(&pdev->dev, hsp->shared_irq,
> + tegra_hsp_shared_irq, 0,
> + dev_name(&pdev->dev), hsp);
> + if (err < 0) {
> + dev_err(&pdev->dev,
> + "failed to request shared0 IRQ%u: %d\n",
> + hsp->shared_irq, err);
> + return err;
Clean-up?
> + }
> }
>
> return 0;
> +
> +remove_doorbells:
> + if (hsp->doorbell_irq)
> + tegra_hsp_remove_doorbells(hsp);
> +
> + return err;
> }
>
> static int tegra_hsp_remove(struct platform_device *pdev)
> @@ -483,7 +652,8 @@ static int tegra_hsp_remove(struct platform_device *pdev)
> struct tegra_hsp *hsp = platform_get_drvdata(pdev);
>
> mbox_controller_unregister(&hsp->mbox);
> - tegra_hsp_remove_doorbells(hsp);
> + if (hsp->doorbell_irq)
> + tegra_hsp_remove_doorbells(hsp);
>
> return 0;
> }
>
Cheers
Jon
--
nvpublic
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox