* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Sebastian Reichel @ 2018-05-08 13:40 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180508064351.ioczgw2v4jtryr3x@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]
Hi,
On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > This properly unmaps DMA SG on device shutdown.
> >
> > Reported-by: Nandor Han <nandor.han@ge.com>
> > Suggested-by: Nandor Han <nandor.han@ge.com>
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > ---
> > drivers/tty/serial/imx.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 3ca767b1162a..6c53e74244ec 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > u32 ucr1, ucr2;
> >
> > if (sport->dma_is_enabled) {
> > - sport->dma_is_rxing = 0;
> > - sport->dma_is_txing = 0;
> > dmaengine_terminate_sync(sport->dma_chan_tx);
> > + if (sport->dma_is_txing) {
> > + dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> > + sport->dma_tx_nents, DMA_TO_DEVICE);
> > + sport->dma_is_txing = 0;
> > + }
>
> did you find this because the kernel crashed or consumed more and more
> memory, or is this "only" a finding of reading the source code? If the
> former it would be great to point out in the commit log, if the latter,
> I wonder if this is a real problem that warrants a stable backport.
A bit of both. One of Collabora's customers had a (scarce) kernel crash
in imx-serial and modified multiple things in the driver. The crash is
gone, but it's not clear which change fixed it. I could not
reproduce the crash so far and I'm currently rebasing and splitting
their changes into upstreamable portions with proper patch
descriptions. From reading the source this looked like a real issue.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Uwe Kleine-König @ 2018-05-08 18:46 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180508134047.zocurxwelw3a24ti@earth.universe>
Hello Sebastian,
On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > This properly unmaps DMA SG on device shutdown.
> > >
> > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > ---
> > > drivers/tty/serial/imx.c | 12 ++++++++++--
> > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index 3ca767b1162a..6c53e74244ec 100644
> > > --- a/drivers/tty/serial/imx.c
> > > +++ b/drivers/tty/serial/imx.c
> > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > > u32 ucr1, ucr2;
> > >
> > > if (sport->dma_is_enabled) {
> > > - sport->dma_is_rxing = 0;
> > > - sport->dma_is_txing = 0;
> > > dmaengine_terminate_sync(sport->dma_chan_tx);
> > > + if (sport->dma_is_txing) {
> > > + dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> > > + sport->dma_tx_nents, DMA_TO_DEVICE);
> > > + sport->dma_is_txing = 0;
> > > + }
> >
> > did you find this because the kernel crashed or consumed more and more
> > memory, or is this "only" a finding of reading the source code? If the
> > former it would be great to point out in the commit log, if the latter,
> > I wonder if this is a real problem that warrants a stable backport.
>
> A bit of both. One of Collabora's customers had a (scarce) kernel crash
> in imx-serial and modified multiple things in the driver. The crash is
> gone, but it's not clear which change fixed it. I could not
> reproduce the crash so far and I'm currently rebasing and splitting
> their changes into upstreamable portions with proper patch
> descriptions. From reading the source this looked like a real issue.
In which context (kernel version, operating mode (e.g. rs485)) did these
happen? What does "crash" mean? The kernel did just hang or produced an
oops? If the latter, can you show it/them?
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: Jacek Anaszewski @ 2018-05-08 19:33 UTC (permalink / raw)
To: Uwe Kleine-König, Greg Kroah-Hartman, Jiri Slaby,
Johan Hovold, Pavel Machek
Cc: linux-serial, linux-leds, linux-can, kernel, One Thousand Gnomes,
Florian Fainelli, Mathieu Poirier, linux-kernel, Robin Murphy,
linux-arm-kernel
In-Reply-To: <20180508100543.12559-2-u.kleine-koenig@pengutronix.de>
Hi Uwe,
Thank you for the patch. It looks fine, but please split
the drivers/net/can/led.c related changes into a separate one.
Best regards,
Jacek Anaszewski
On 05/08/2018 12:05 PM, Uwe Kleine-König wrote:
> 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.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> drivers/leds/led-triggers.c | 84 +++++++++++++++++++++++++++----------
> drivers/net/can/led.c | 6 +--
> include/linux/leds.h | 30 +++++++------
> 3 files changed, 81 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 431123b048a2..5d8bb504b07b 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -175,18 +175,34 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
> }
> EXPORT_SYMBOL_GPL(led_trigger_set_default);
>
> -void led_trigger_rename_static(const char *name, struct led_trigger *trig)
> +int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...)
> {
> - /* new name must be on a temporary string to prevent races */
> - BUG_ON(name == trig->name);
> + const char *prevname;
> + const char *newname;
> + va_list args;
> +
> + if (!trig)
> + return 0;
> +
> + va_start(args, fmt);
> + newname = kvasprintf_const(GFP_KERNEL, fmt, args);
> + va_end(args);
> +
> + if (!newname) {
> + pr_err("Failed to allocate new name for trigger %s\n", trig->name);
> + return -ENOMEM;
> + }
>
> down_write(&triggers_list_lock);
> - /* this assumes that trig->name was originaly allocated to
> - * non constant storage */
> - strcpy((char *)trig->name, name);
> + prevname = trig->name;
> + trig->name = newname;
> up_write(&triggers_list_lock);
> +
> + kfree_const(prevname);
> +
> + return 0;
> }
> -EXPORT_SYMBOL_GPL(led_trigger_rename_static);
> +EXPORT_SYMBOL_GPL(led_trigger_rename);
>
> /* LED Trigger Interface */
>
> @@ -333,34 +349,56 @@ void led_trigger_blink_oneshot(struct led_trigger *trig,
> }
> EXPORT_SYMBOL_GPL(led_trigger_blink_oneshot);
>
> -void led_trigger_register_simple(const char *name, struct led_trigger **tp)
> +int led_trigger_register_format(struct led_trigger **tp, const char *fmt, ...)
> {
> + va_list args;
> struct led_trigger *trig;
> - int err;
> + int err = -ENOMEM;
> + const char *name;
> +
> + va_start(args, fmt);
> + name = kvasprintf_const(GFP_KERNEL, fmt, args);
> + va_end(args);
>
> trig = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
>
> - if (trig) {
> - trig->name = name;
> - err = led_trigger_register(trig);
> - if (err < 0) {
> - kfree(trig);
> - trig = NULL;
> - pr_warn("LED trigger %s failed to register (%d)\n",
> - name, err);
> - }
> - } else {
> - pr_warn("LED trigger %s failed to register (no memory)\n",
> - name);
> - }
> + if (!name || !trig)
> + goto err;
> +
> + trig->name = name;
> +
> + err = led_trigger_register(trig);
> + if (err < 0)
> + goto err;
> +
> *tp = trig;
> +
> + return 0;
> +
> +err:
> + kfree(trig);
> + kfree_const(name);
> +
> + *tp = NULL;
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(led_trigger_register_format);
> +
> +void led_trigger_register_simple(const char *name, struct led_trigger **tp)
> +{
> + int ret = led_trigger_register_format(tp, "%s", name);
> + if (ret < 0)
> + pr_warn("LED trigger %s failed to register (%d)\n", name, ret);
> }
> EXPORT_SYMBOL_GPL(led_trigger_register_simple);
>
> void led_trigger_unregister_simple(struct led_trigger *trig)
> {
> - if (trig)
> + if (trig) {
> led_trigger_unregister(trig);
> + kfree_const(trig->name);
> + }
> kfree(trig);
> }
> EXPORT_SYMBOL_GPL(led_trigger_unregister_simple);
> diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
> index c1b667675fa1..2d7d1b0d20f9 100644
> --- a/drivers/net/can/led.c
> +++ b/drivers/net/can/led.c
> @@ -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);
> }
>
> return NOTIFY_DONE;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index b7e82550e655..e706c28bb35b 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -275,6 +275,8 @@ extern void led_trigger_unregister(struct led_trigger *trigger);
> extern int devm_led_trigger_register(struct device *dev,
> struct led_trigger *trigger);
>
> +extern int led_trigger_register_format(struct led_trigger **trigger,
> + const char *fmt, ...);
> extern void led_trigger_register_simple(const char *name,
> struct led_trigger **trigger);
> extern void led_trigger_unregister_simple(struct led_trigger *trigger);
> @@ -298,28 +300,25 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
> }
>
> /**
> - * led_trigger_rename_static - rename a trigger
> - * @name: the new trigger name
> + * led_trigger_rename - rename a trigger
> * @trig: the LED trigger to rename
> + * @fmt: format string for new name
> *
> - * Change a LED trigger name by copying the string passed in
> - * name into current trigger name, which MUST be large
> - * enough for the new string.
> - *
> - * Note that name must NOT point to the same string used
> - * during LED registration, as that could lead to races.
> - *
> - * This is meant to be used on triggers with statically
> - * allocated name.
> + * rebaptize the given trigger.
> */
> -extern void led_trigger_rename_static(const char *name,
> - struct led_trigger *trig);
> +extern int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...);
>
> #else
>
> /* Trigger has no members */
> struct led_trigger {};
>
> +static inline int led_trigger_register_format(struct led_trigger **trigger,
> + const char *fmt, ...)
> +{
> + return 0;
> +}
> +
> /* Trigger inline empty functions */
> static inline void led_trigger_register_simple(const char *name,
> struct led_trigger **trigger) {}
> @@ -342,6 +341,11 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
> return NULL;
> }
>
> +static inline int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...)
> +{
> + return 0;
> +}
> +
> #endif /* CONFIG_LEDS_TRIGGERS */
>
> /* Trigger specific functions */
>
^ permalink raw reply
* Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()
From: Uwe Kleine-König @ 2018-05-08 20:17 UTC (permalink / raw)
To: Jacek Anaszewski, Marc Kleine-Budde
Cc: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Pavel Machek,
One Thousand Gnomes, Florian Fainelli, linux-serial,
Mathieu Poirier, linux-kernel, linux-can, linux-arm-kernel,
kernel, Robin Murphy, linux-leds
In-Reply-To: <ea34ce99-d446-03d2-b38d-d40a22480337@gmail.com>
Hello Jacek,
On Tue, May 08, 2018 at 09:33:14PM +0200, Jacek Anaszewski wrote:
> Thank you for the patch. It looks fine, but please split
> the drivers/net/can/led.c related changes into a separate one.
I renamed led_trigger_rename_static() to led_trigger_rename() (and
changed the parameters). The can change just adapts the only user of
led_trigger_rename_static() to use the new one.
It's not impossible to separate this patches, but I wonder if it's worth
the effort.
The first patch would be like the patch under discussion, just without
the can bits and introducing something like:
/*
* compat stuff to be removed once the only caller is converted
*/
static inline led_trigger_rename_static(const char *name, struct led_trigger *trig)
{
(void)led_trigger_rename(trig, "%s", name);
}
Then the second patch would just be the 6-line can hunk. And a third
patch would remove the compat function. (Maybe I'd choose to squash the
two can patches together then, but this doesn't reduce the overhead
considerably.) The only upside I can see here is that it increases my
patch count, but it's otherwise not worth the effort for such an easy
change. Further more as there is a strict dependency on these three
patches this either delays the cleanup or (IMHO more likely) the can
change would go in via the led tree anyhow. (Mark already acked patch 2
of this series and in private confirmed that the agrees to let this
change go in via the led tree, too.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)
From: Johan Hovold @ 2018-05-09 9:18 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Johan Hovold, Tony Lindgren, 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-pm
In-Reply-To: <20180508155608.3bzcbepsmoskhlox@earth.universe>
[ Changing the subject line as this is thread is no longer about DT
bindings.
Also adding linux-serial and linux-pm while keeping some context. ]
On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> Hi,
>
> On Mon, May 07, 2018 at 06:34:39PM +0200, Johan Hovold wrote:
> > On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [180507 03:03]:
> > > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > >
> > > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > > us to enable runtime PM by default (once implemented), since we know
> > > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > > with sideband wakeup gpios).
> > > >
> > > > I'm not sure we want generic runtime-pm support for the controllers in
> > > > the sense that the slave device state is always reflected by the serial
> > > > controller. Similar as for i2c and spi, we really only want to keep the
> > > > controller active when we are doing I/O, but we may want to keep a
> > > > client active for longer.
> > >
> > > Yeah i2c seems to do the right thing where the bus takes care
> > > of runtime PM.
> >
> > Yeah, but since serial is async in contrast to i2c/spi, we may not be
> > able to push this entirely into core. The serdev drivers may need to
> > indicate when they expect or need to do I/O by opening and closing the
> > port. And this is how I implemented these first couple of gnss drivers.
> >
> > Also note that most serial driver do not do runtime pm while the port is
> > open (as OMAP does), and doing so also has the drawbacks of lost
> > characters etc. as Sebastian mentioned.
>
> I think using open/close for runtime pm is good enough for GPS,
> since it regularly sends data and draws lots of power anyways.
> But devices, that have an out-of-band wakeup signal can do proper
> runtime PM of the serial port without loosing characters.
Yeah, there may be some applications where this is possible. And this is
not the case for GPS, but not just because of a generally higher power
consumption, but due to the fact that we cannot afford having the first
message in every report burst be dropped.
> Note, that OMAP does not reach deep idle states with active
> serial port. This is not acceptable for low power devices.
Sure, but note that OMAP is the only serial driver which currently
implements this kind of aggressive runtime PM (besides a couple of
usb-serial drivers). This means that a serdev driver can never rely on
this being the case, and therefore needs to be restrictive about how
long the port is kept open if it cares about power at all.
> > > > Take the u-blox driver in this series for example. As I'm using runtime
> > > > PM to manage device power, user-space can chose to prevent the receiver
> > > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > > in setups without a backup battery (by means of the power/control
> > > > attribute).
> > >
> > > Sorry I don't seem to have that one, care to paste the subject
> > > line of that patch?
> >
> > "[PATCH 5/7] gnss: add driver for u-blox receivers"
> >
> > https://lkml.kernel.org/r/20180424163458.11947-6-johan@kernel.org
> >
> > > > Note that serdev not enabling runtime pm for controllers is roughly
> > > > equivalent to setting the .ignore_children flag, which is what we do for
> > > > i2c and spi controller, and possibly what we want here too.
>
> For I2C/SPI this works, since receive operations are initiated by
> the controller. Unfortunately its harder to implement for async
> serial. But I agree, that we may want to have runtime PM for the
> serdev client and .ignore_children is the way to go.
It's really about adding runtime PM support to serdev controllers.
Serdev client drivers can already use runtime PM (as mentioned above).
The ignore_children flag would then allow the controller RPM state to be
managed independently of the child/slave device state when more fine
grained RPM control is desired.
> I think the client API should allow two things:
>
> 1. minimal runtime PM support: The controller is runtime enabled
> on serdev open and disabled on serdev close. This may be enough
> for some clients and useful for writing new drivers.
Right, and we already have something like this today by means of how the
serial driver implement runtime PM (i.e. they are generally active while
the port is open).
> 2. full runtime PM support: The controller is sleeping by default
> even with serdev open. The calls to write/change port settings/...
> automatically enables the device, similar to i2c/spi. But there
> must be additional functions to enable/disable runtime PM based
> on a wakeup gpio or similar out-of-band information. It may be
> enough to just provide:
>
> int serdev_pm_runtime_get_sync(struct serdev_device *serdev) {
> pm_runtime_get_sync(&serdev->dev);
> }
>
> int serdev_pm_runtime_put_autosuspend(struct serdev_device *serdev) {
> pm_runtime_put_autosuspend(&serdev->dev);
> }
I'm not a big fan of rpm wrappers and prefer using the RPM interface
directly, but that's a side note. In the above case we really want to
manage the controller (&serdev->ctrl->dev) rather than the client
however (.ignore_children should be set, remember).
Also note that a serial driver implementing aggressive RPM (e.g. using
autosuspend) would manage the RPM counts itself when changing terminal
settings etc, so the only thing that would be needed is for the
client/slave device to resume the controller pro-actively when it is
expecting incoming data.
To make this more concrete; an example could be a device with an OOB
wake-up signal, but using a request-response type protocol so that the
client driver knows when it's safe to allow the controller to again
suspend.
We can model this similarly to how we do it for usb-serial, namely that
the core takes an extra RPM reference at open, which a sufficiently
power aware driver can then chose to drop in order to allow for more
aggressive controller runtime PM (should the underlying device and driver
support it).
I've cooked up a patch which I'll be sending as a reply to this mail.
Thanks,
Johan
^ permalink raw reply
* [PATCH 1/2] serdev: add controller runtime PM support
From: Johan Hovold @ 2018-05-09 9:44 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rob Herring, Sebastian Reichel, Tony Lindgren,
H. Nikolaus Schaller, Andreas Kemnade, Mark Rutland,
Arnd Bergmann, Pavel Machek, linux-kernel, linux-serial, linux-pm,
Johan Hovold
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>
---
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
^ permalink raw reply related
* [PATCH EXAMPLE 2/2] dbg: gnss: sirf: allow aggressive controller runtime PM
From: Johan Hovold @ 2018-05-09 9:44 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rob Herring, Sebastian Reichel, Tony Lindgren,
H. Nikolaus Schaller, Andreas Kemnade, Mark Rutland,
Arnd Bergmann, Pavel Machek, linux-kernel, linux-serial, linux-pm,
Johan Hovold
In-Reply-To: <20180509094419.13470-1-johan@kernel.org>
This is just an example of how a serdev driver could go about to allow
aggressive controller runtime PM by dropping the RPM reference taken by
serdev core in serdev_device_open().
Note that for most GNSS devices this does not make any sense, as
allowing the controller to suspend this way would cause the
first message of every report burst to be corrupted (and discarded).
This one applies on top of the GNSS series available here:
https://lkml.kernel.org/r/20180424163458.11947-1-johan@kernel.org
Not-Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/gnss/sirf.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 497f8eb8467f..31b2cbccd194 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -57,6 +57,9 @@ static int sirf_open(struct gnss_device *gdev)
goto err_close;
}
+ /* Allow aggresive controller runtime PM. */
+ pm_runtime_put(&serdev->ctrl->dev);
+
return 0;
err_close:
@@ -70,6 +73,9 @@ static void sirf_close(struct gnss_device *gdev)
struct sirf_data *data = gnss_get_drvdata(gdev);
struct serdev_device *serdev = data->serdev;
+ /* Balance the put in open() */
+ pm_runtime_get(&serdev->ctrl->dev);
+
serdev_device_close(serdev);
pm_runtime_put(&serdev->dev);
--
2.17.0
^ permalink raw reply related
* Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)
From: Johan Hovold @ 2018-05-09 9:49 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Johan Hovold, Tony Lindgren, 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-pm
In-Reply-To: <20180509091831.GA2285@localhost>
On Wed, May 09, 2018 at 11:18:31AM +0200, Johan Hovold wrote:
> I've cooked up a patch which I'll be sending as a reply to this mail.
Forgot to add the in-reply-to header of course. For the interested, the
patch can be found here:
https://lkml.kernel.org/r/20180509094419.13470-1-johan@kernel.org
Johan
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Sebastian Reichel @ 2018-05-09 10:20 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180508184612.iixr3psap4ik5fdr@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 9210 bytes --]
Hi,
On Tue, May 08, 2018 at 08:46:12PM +0200, Uwe Kleine-König wrote:
> On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> > On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > > This properly unmaps DMA SG on device shutdown.
> > > >
> > > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > > ---
> > > > drivers/tty/serial/imx.c | 12 ++++++++++--
> > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index 3ca767b1162a..6c53e74244ec 100644
> > > > --- a/drivers/tty/serial/imx.c
> > > > +++ b/drivers/tty/serial/imx.c
> > > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > > > u32 ucr1, ucr2;
> > > >
> > > > if (sport->dma_is_enabled) {
> > > > - sport->dma_is_rxing = 0;
> > > > - sport->dma_is_txing = 0;
> > > > dmaengine_terminate_sync(sport->dma_chan_tx);
> > > > + if (sport->dma_is_txing) {
> > > > + dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> > > > + sport->dma_tx_nents, DMA_TO_DEVICE);
> > > > + sport->dma_is_txing = 0;
> > > > + }
> > >
> > > did you find this because the kernel crashed or consumed more and more
> > > memory, or is this "only" a finding of reading the source code? If the
> > > former it would be great to point out in the commit log, if the latter,
> > > I wonder if this is a real problem that warrants a stable backport.
> >
> > A bit of both. One of Collabora's customers had a (scarce) kernel crash
> > in imx-serial and modified multiple things in the driver. The crash is
> > gone, but it's not clear which change fixed it. I could not
> > reproduce the crash so far and I'm currently rebasing and splitting
> > their changes into upstreamable portions with proper patch
> > descriptions. From reading the source this looked like a real issue.
>
> In which context (kernel version, operating mode (e.g. rs485)) did these
> happen? What does "crash" mean? The kernel did just hang or produced an
> oops? If the latter, can you show it/them?
I pasted the oops, that triggered writing the patches (Linux 4.8, no
rs485, 4MHz baudrate). I think, that the actual issue has already been
fixed upstream between 4.13 and current master.
-- Sebastian
...
[ 302.516696] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0938000
[ 302.524394] pgd = 80004000
[ 302.527111] [f0938000] *pgd=de81a811, *pte=53fc0653, *ppte=53fc0453
[ 302.533451] Internal error: : 1008 [#1] SMP ARM
[ 302.537994] Modules linked in: smsc95xx usbnet atmel_mxt_ts
[ 302.543651] CPU: 0 PID: 357 Comm: _ACFRead Not tainted 4.8.0 #1
[ 302.549578] Hardware name: Freescale i.MX53 (Device Tree Support)
[ 302.555679] task: edbf1f00 task.stack: ed5ce000
[ 302.560228] PC is at imx_rxint+0x5c/0x228
[ 302.564261] LR is at lock_acquired+0x494/0x57c
...
[ 303.055953] Backtrace:
[ 303.058430] [<804b3b48>] (imx_rxint) from [<804b4df8>] (imx_int+0x2ec/0x404)
[ 303.065484] r10:00000000 r9:00000000 r8:00000030 r7:00000030 r6:00005099 r5:00007240
[ 303.073406] r4:eeafdc10
[ 303.075975] [<804b4b0c>] (imx_int) from [<80182f10>] (__handle_irq_event_percpu+0x4c/0x3e8)
[ 303.084331] r10:00000000 r9:810025c4 r8:00000030 r7:00000001 r6:ee81c210 r5:ee81c200
[ 303.092252] r4:edcbe040
[ 303.094814] [<80182ec4>] (__handle_irq_event_percpu) from [<801832d8>] (handle_irq_event_percpu+0x2
c/0x68)
[ 303.104472] r10:1240dc08 r9:00000001 r8:ee81e400 r7:00000001 r6:ee81c210 r5:ee81c200
[ 303.112393] r4:ee81c200
[ 303.114954] [<801832ac>] (handle_irq_event_percpu) from [<8018335c>] (handle_irq_event+0x48/0x6c)
[ 303.123832] r5:ee81c260 r4:ee81c200
[ 303.127454] [<80183314>] (handle_irq_event) from [<80186ba0>] (handle_level_irq+0xb8/0x154)
[ 303.135810] r7:00000001 r6:ee81c210 r5:ee81c260 r4:ee81c200
[ 303.141547] [<80186ae8>] (handle_level_irq) from [<80182420>] (generic_handle_irq+0x30/0x44)
[ 303.149990] r7:00000001 r6:00000000 r5:00000030 r4:80ff1e8c
[ 303.155728] [<801823f0>] (generic_handle_irq) from [<801827a0>] (__handle_domain_irq+0x60/0xc8)
[ 303.164439] [<80182740>] (__handle_domain_irq) from [<80101530>] (tzic_handle_irq+0x74/0x9c)
[ 303.172882] r9:00000001 r8:ed5cfae8 r7:00000001 r6:00000020 r5:8108a50c r4:00000000
[ 303.180734] [<801014bc>] (tzic_handle_irq) from [<8094c630>] (__irq_svc+0x70/0x98)
[ 303.188311] Exception stack(0xed5cfae8 to 0xed5cfb30)
[ 303.193374] fae0: 00000283 edbf23c0 edbf1f00 600b0113 edbf2458 00000004
[ 303.201563] fb00: 00000014 00000004 818762ec edbf23c8 1240dc08 ed5cfb94 eee55200 ed5cfb38
[ 303.209748] fb20: 00000282 80177d1c 600b0113 ffffffff
[ 303.214805] r9:ed5ce000 r8:818762ec r7:ed5cfb1c r6:ffffffff r5:600b0113 r4:80177d1c
[ 303.222649] [<80177a50>] (lock_release) from [<8094bba0>] (_raw_spin_unlock+0x28/0x34)
[ 303.230572] r10:00000008 r9:ed5a5500 r8:eeafdc10 r7:ef07c000 r6:00000000 r5:edb86f40
[ 303.238493] r4:8101bd68
[ 303.241057] [<8094bb78>] (_raw_spin_unlock) from [<8025fdb4>] (remove_vm_area+0x54/0x70)
[ 303.249153] r5:edb86f40 r4:edb86100
[ 303.252771] [<8025fd60>] (remove_vm_area) from [<8025fdfc>] (__vunmap+0x2c/0xf8)
[ 303.260173] r5:f0c0b000 r4:f0c0b000
[ 303.263791] [<8025fdd0>] (__vunmap) from [<8026000c>] (vunmap+0x50/0x5c)
[ 303.270497] r7:ef07c000 r6:00001000 r5:f0c0b000 r4:00000000
[ 303.276240] [<8025ffbc>] (vunmap) from [<805262e4>] (dma_common_free_remap+0x64/0x74)
[ 303.284076] r5:20000008 r4:f0c0b000
[ 303.287697] [<80526280>] (dma_common_free_remap) from [<80117404>] (cma_allocator_free+0x7c/0x84)
[ 303.296574] r7:ef07c000 r6:00000000 r5:00001000 r4:effd9f80
[ 303.302312] [<80117388>] (cma_allocator_free) from [<80116dcc>] (__arm_dma_free+0xf0/0x13c)
[ 303.310668] r7:ef07c000 r6:f0c0b000 r5:f0c0b000 r4:edb869c0
[ 303.316405] [<80116cdc>] (__arm_dma_free) from [<80116e78>] (arm_dma_free+0x2c/0x34)
[ 303.324153] r5:edcc2010 r4:80116e4c
[ 303.327779] [<80116e4c>] (arm_dma_free) from [<8047dba8>] (sdma_free_chan_resources+0xc4/0x110)
[ 303.336490] [<8047dae4>] (sdma_free_chan_resources) from [<8047a9d8>] (dma_chan_put+0x88/0xbc)
[ 303.345107] r7:600b0113 r6:00000000 r5:edcc07ec r4:edcc07ec
[ 303.350845] [<8047a950>] (dma_chan_put) from [<8047aa44>] (dma_release_channel+0x38/0xa8)
[ 303.359028] r5:edcc07ec r4:edcc07ec
[ 303.362647] [<8047aa0c>] (dma_release_channel) from [<804b3e1c>] (imx_uart_dma_exit+0x50/0xfc)
[ 303.371263] r5:edcc07ec r4:eeafdc10
[ 303.374882] [<804b3dcc>] (imx_uart_dma_exit) from [<804b5028>] (imx_shutdown+0x118/0x20c)
[ 303.383065] r5:00000b01 r4:eeafdc10
[ 303.386689] [<804b4f10>] (imx_shutdown) from [<804ae834>] (uart_shutdown+0x120/0x17c)
[ 303.394525] r7:81031774 r6:ee8863a4 r5:eeafdc10 r4:ee886258
[ 303.400264] [<804ae714>] (uart_shutdown) from [<804b0624>] (uart_close+0x164/0x254)
[ 303.407926] r9:ed5a5500 r8:ee8863ac r7:ee886310 r6:eeafdc10 r5:edba9800 r4:ee886258
[ 303.415768] [<804b04c0>] (uart_close) from [<80492300>] (tty_release+0x104/0x498)
[ 303.423256] r9:ed5a5500 r8:00000000 r7:ee00e000 r6:00000000 r5:eeac2e60 r4:edba9800
[ 303.431100] [<804921fc>] (tty_release) from [<80271404>] (__fput+0x98/0x1e8)
[ 303.438154] r10:00000008 r9:eeac2e60 r8:00000000 r7:ee00e000 r6:edebea10 r5:eeac2e60
[ 303.446075] r4:ed5a5500
[ 303.448635] [<8027136c>] (__fput) from [<802715c4>] (____fput+0x18/0x1c)
[ 303.455342] r10:ed5cfedc r9:ed496780 r8:edbf1f00 r7:edbf2340 r6:8108b054 r5:00000000
[ 303.463263] r4:edbf2300
[ 303.465832] [<802715ac>] (____fput) from [<80146034>] (task_work_run+0xc8/0xf8)
[ 303.473165] [<80145f6c>] (task_work_run) from [<801265e0>] (do_exit+0x330/0xb74)
[ 303.480567] r9:edfd07dc r8:00000000 r7:81084dcc r6:810025c4 r5:81083a25 r4:edbf1f00
[ 303.488408] [<801262b0>] (do_exit) from [<80128678>] (do_group_exit+0x4c/0xcc)
[ 303.495636] r7:00000009
[ 303.498205] [<8012862c>] (do_group_exit) from [<80135450>] (get_signal+0x2a8/0x990)
[ 303.505866] r7:00000009 r6:00418004 r5:ed5cfec8 r4:ed5eeca0
[ 303.511616] [<801351a8>] (get_signal) from [<8010c6e4>] (do_signal+0xd8/0x47c)
[ 303.518844] r10:00000000 r9:ed5ce000 r8:00000001 r7:766d443c r6:766d4438 r5:ed5cfec8
[ 303.526764] r4:ed5cffb0
[ 303.529326] [<8010c60c>] (do_signal) from [<8010cc78>] (do_work_pending+0xc0/0xd0)
[ 303.536901] r10:00000000 r9:ed5ce000 r8:801086c4 r7:801086c4 r6:ed5cffb0 r5:ed5ce000
[ 303.544824] r4:00000001
[ 303.547385] [<8010cbb8>] (do_work_pending) from [<80108548>] (slow_work_pending+0xc/0x20)
[ 303.555568] r7:0000008e r6:747ad088 r5:747ad188 r4:747ad188
[ 303.561306] Code: e5943094 e594202c e2833001 e5843094 (e592a000)
[ 303.567419] ---[ end trace 741daf1a1655e1be ]---
[ 303.572048] Kernel panic - not syncing: Fatal exception in interrupt
[ 303.578432] ---[ end Kernel panic - not syncing: Fatal exception in interrupt
...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Uwe Kleine-König @ 2018-05-09 10:42 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180509102058.6mshag4y7aq4w2n2@earth.universe>
On Wed, May 09, 2018 at 12:20:58PM +0200, Sebastian Reichel wrote:
> Hi,
>
> On Tue, May 08, 2018 at 08:46:12PM +0200, Uwe Kleine-König wrote:
> > On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> > > On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > > > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > > > This properly unmaps DMA SG on device shutdown.
> > > > >
> > > > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > > > ---
> > > > > drivers/tty/serial/imx.c | 12 ++++++++++--
> > > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > > index 3ca767b1162a..6c53e74244ec 100644
> > > > > --- a/drivers/tty/serial/imx.c
> > > > > +++ b/drivers/tty/serial/imx.c
> > > > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > > > > u32 ucr1, ucr2;
> > > > >
> > > > > if (sport->dma_is_enabled) {
> > > > > - sport->dma_is_rxing = 0;
> > > > > - sport->dma_is_txing = 0;
> > > > > dmaengine_terminate_sync(sport->dma_chan_tx);
> > > > > + if (sport->dma_is_txing) {
> > > > > + dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> > > > > + sport->dma_tx_nents, DMA_TO_DEVICE);
> > > > > + sport->dma_is_txing = 0;
> > > > > + }
> > > >
> > > > did you find this because the kernel crashed or consumed more and more
> > > > memory, or is this "only" a finding of reading the source code? If the
> > > > former it would be great to point out in the commit log, if the latter,
> > > > I wonder if this is a real problem that warrants a stable backport.
> > >
> > > A bit of both. One of Collabora's customers had a (scarce) kernel crash
> > > in imx-serial and modified multiple things in the driver. The crash is
> > > gone, but it's not clear which change fixed it. I could not
> > > reproduce the crash so far and I'm currently rebasing and splitting
> > > their changes into upstreamable portions with proper patch
> > > descriptions. From reading the source this looked like a real issue.
> >
> > In which context (kernel version, operating mode (e.g. rs485)) did these
> > happen? What does "crash" mean? The kernel did just hang or produced an
> > oops? If the latter, can you show it/them?
>
> I pasted the oops, that triggered writing the patches (Linux 4.8, no
> rs485, 4MHz baudrate). I think, that the actual issue has already been
> fixed upstream between 4.13 and current master.
>
> -- Sebastian
>
> ...
> [ 302.516696] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0938000
This is usually a missing clk. Alternatively RX is disabled and the
RXDATA register is read. Scrolling through
git log v4.13..linus/master -- drivers/tty/serial/imx.c
I didn't find a candidate for fixing that.
> [ 302.524394] pgd = 80004000
> [ 302.527111] [f0938000] *pgd=de81a811, *pte=53fc0653, *ppte=53fc0453
> [ 302.533451] Internal error: : 1008 [#1] SMP ARM
> [ 302.537994] Modules linked in: smsc95xx usbnet atmel_mxt_ts
> [ 302.543651] CPU: 0 PID: 357 Comm: _ACFRead Not tainted 4.8.0 #1
> [ 302.549578] Hardware name: Freescale i.MX53 (Device Tree Support)
> [ 302.555679] task: edbf1f00 task.stack: ed5ce000
> [ 302.560228] PC is at imx_rxint+0x5c/0x228
> [ 302.564261] LR is at lock_acquired+0x494/0x57c
If you still have this kernel, disabling imx_rxint would be helpful.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Sebastian Reichel @ 2018-05-09 11:28 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180509104256.slkzauicnhsktqqf@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 3680 bytes --]
Hi,
On Wed, May 09, 2018 at 12:42:56PM +0200, Uwe Kleine-König wrote:
> On Wed, May 09, 2018 at 12:20:58PM +0200, Sebastian Reichel wrote:
> > Hi,
> >
> > On Tue, May 08, 2018 at 08:46:12PM +0200, Uwe Kleine-König wrote:
> > > On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> > > > On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > > > > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > > > > This properly unmaps DMA SG on device shutdown.
> > > > > >
> > > > > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > > > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > > > > ---
> > > > > > drivers/tty/serial/imx.c | 12 ++++++++++--
> > > > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > > > index 3ca767b1162a..6c53e74244ec 100644
> > > > > > --- a/drivers/tty/serial/imx.c
> > > > > > +++ b/drivers/tty/serial/imx.c
> > > > > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > > > > > u32 ucr1, ucr2;
> > > > > >
> > > > > > if (sport->dma_is_enabled) {
> > > > > > - sport->dma_is_rxing = 0;
> > > > > > - sport->dma_is_txing = 0;
> > > > > > dmaengine_terminate_sync(sport->dma_chan_tx);
> > > > > > + if (sport->dma_is_txing) {
> > > > > > + dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> > > > > > + sport->dma_tx_nents, DMA_TO_DEVICE);
> > > > > > + sport->dma_is_txing = 0;
> > > > > > + }
> > > > >
> > > > > did you find this because the kernel crashed or consumed more and more
> > > > > memory, or is this "only" a finding of reading the source code? If the
> > > > > former it would be great to point out in the commit log, if the latter,
> > > > > I wonder if this is a real problem that warrants a stable backport.
> > > >
> > > > A bit of both. One of Collabora's customers had a (scarce) kernel crash
> > > > in imx-serial and modified multiple things in the driver. The crash is
> > > > gone, but it's not clear which change fixed it. I could not
> > > > reproduce the crash so far and I'm currently rebasing and splitting
> > > > their changes into upstreamable portions with proper patch
> > > > descriptions. From reading the source this looked like a real issue.
> > >
> > > In which context (kernel version, operating mode (e.g. rs485)) did these
> > > happen? What does "crash" mean? The kernel did just hang or produced an
> > > oops? If the latter, can you show it/them?
> >
> > I pasted the oops, that triggered writing the patches (Linux 4.8, no
> > rs485, 4MHz baudrate). I think, that the actual issue has already been
> > fixed upstream between 4.13 and current master.
> >
> > -- Sebastian
> >
> > ...
> > [ 302.516696] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0938000
>
> This is usually a missing clk. Alternatively RX is disabled and the
> RXDATA register is read. Scrolling through
>
> git log v4.13..linus/master -- drivers/tty/serial/imx.c
>
> I didn't find a candidate for fixing that.
It happens while the port is being torn apart. I think the following
patches from you are very good candidates. Especially since the
remaining diff fixing the issue in the customer's old kernel has a
smilar change:
76821e222c18 - serial: imx: ensure that RX irqs are off if RX is off (9 weeks ago) <Uwe Kleine-König>
dedc64e02f5d - serial: imx: Stop to receive in .stop_rx() (9 weeks ago) <Uwe Kleine-König>
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 0/3] serial: uartps: Add run time support for more IPs than hardcoded 2
From: Michal Simek @ 2018-05-09 12:09 UTC (permalink / raw)
To: Maarten Brock, Michal Simek
Cc: linux-kernel, monstr, gnomes, Alexander Graf, devicetree,
Jiri Slaby, Greg Kroah-Hartman, Rob Herring, linux-serial,
Frank Rowand, linux-arm-kernel, linux-serial-owner
In-Reply-To: <1d3bc2082972bb1d4abc2c15db396257@vanmierlo.com>
Hi,
On 5.5.2018 15:10, Maarten Brock wrote:
> On 2018-04-26 16:08, Michal Simek wrote:
>> Hi,
>>
>> this series is trying to address discussion I had with Alan in past
>> https://patchwork.kernel.org/patch/9738445/.
>>
>> It is moving uart_register_driver() to probe function like it is done in
>> pl011 driver.
>> And also introducing new function for alias compatibility checking to
>> resolve cases where some IPs have alias and some of them not.
>> This case is detected in pl011_probe_dt_alias() but not properly solved.
>>
>> Also keep status of free ids/minor numbers in bitmap to know what's the
>> next unallocated number.
>>
>> The same solution can be used in pl011, uart16550 and uartlite to really
>> get unified kernel.
>>
>> Tested on these use cases:
>> Notes:
>> ff000000 is the first PS uart listed in dtsi
>> ff010000 is the second PS uart listed in dtsi
>>
>> Standard zcu102 setting
>> serial0 = &uart0;
>> serial1 = &uart1;
>> [ 0.196628] ff000000.serial: ttyPS0 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.642542] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> Swapped zcu102 setting
>> serial0 = &uart1;
>> serial1 = &uart0;
>> [ 0.196472] ff000000.serial: ttyPS1 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.196824] ff010000.serial: ttyPS0 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> uart0 on alias higher then MAX uart ports
>> serial0 = &uart1;
>> serial200 = &uart0;
>> [ 0.176793] ff000000.serial: ttyPS200 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.177288] ff010000.serial: ttyPS0 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> Both uarts on higher aliases
>> serial1 = &uart1;
>> serial2 = &uart0;
>> [ 0.196470] ff000000.serial: ttyPS2 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.196823] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> uart0 not listed but it is probed first that's why should be ttyPS0 but
>> there is uart1 via alias
>> serial0 = &uart1;
>> [ 0.176656] xuartps ff000000.serial: No serial alias passed. Using
>> the first free id
>> [ 0.176671] xuartps ff000000.serial: Validate id 0
>> [ 0.176680] xuartps ff000000.serial: The empty id is 0
>> [ 0.176692] xuartps ff000000.serial: ID 0 already taken - skipped
>> [ 0.176701] xuartps ff000000.serial: Validate id 1
>> [ 0.176710] xuartps ff000000.serial: The empty id is 1
>> [ 0.176719] xuartps ff000000.serial: Selected ID 1 allocation passed
>> [ 0.176760] ff000000.serial: ttyPS1 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.177104] ff010000.serial: ttyPS0 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> uart0 not listed but it is probed first that's why should be ttyPS0
>> serial1 = &uart1;
>> [ 0.176661] xuartps ff000000.serial: No serial alias passed. Using
>> the first free id
>> [ 0.176676] xuartps ff000000.serial: Validate id 0
>> [ 0.176686] xuartps ff000000.serial: The empty id is 0
>> [ 0.176696] xuartps ff000000.serial: Selected ID 0 allocation passed
>> [ 0.176737] ff000000.serial: ttyPS0 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.177069] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> uarts not listed in aliases list
>> [ 0.176673] xuartps ff000000.serial: No serial alias passed. Using
>> the first free id
>> [ 0.176687] xuartps ff000000.serial: Validate id 0
>> [ 0.176697] xuartps ff000000.serial: The empty id is 0
>> [ 0.176707] xuartps ff000000.serial: Selected ID 0 allocation passed
>> [ 0.176746] ff000000.serial: ttyPS0 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.177057] xuartps ff010000.serial: No serial alias passed. Using
>> the first free id
>> [ 0.177070] xuartps ff010000.serial: Validate id 0
>> [ 0.177079] xuartps ff010000.serial: The empty id is 0
>> [ 0.177089] xuartps ff010000.serial: Selected ID 0 allocation failed
>> [ 0.177098] xuartps ff010000.serial: Validate id 1
>> [ 0.177107] xuartps ff010000.serial: The empty id is 1
>> [ 0.177116] xuartps ff010000.serial: Selected ID 1 allocation passed
>> [ 0.177149] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> Thanks,
>> Michal
>
> Hello Michal,
>
> How will this interact with ns16550 based UARTs?
>
> Can we have both /dev/ttyS0 and /dev/ttyPS0?
> Currently we can't unless we create *no* serialN alias for the ns16550.
> And there is no other means to lock the /dev/ttySx name to a device either.
>
> Or will the xuartps driver eventually use /dev/ttySx as well?
I am not changing any current behavior in this case. uartps is using
ttyPS. ns16550 is using ttyS and on xilinx devices we are using also
ttyUL for uartlite. Other drivers are using different names.
Thanks,
Michal
^ permalink raw reply
* OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))
From: Johan Hovold @ 2018-05-09 13:10 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: <20180508164904.GZ98604@atomide.com>
[ Updating subject and adding linux-serial, linux-pm and linux-omap. ]
On Tue, May 08, 2018 at 09:49:04AM -0700, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [180508 08:54]:
> > * Tony Lindgren <tony@atomide.com> [180508 08:47]:
> > > * Tony Lindgren <tony@atomide.com> [180508 08:22]:
> > > > * Johan Hovold <johan@kernel.org> [180508 07:00]:
> > > > > With the negative autosuspend set in both omap drivers probe functions,
> > > > > this is the expected behaviour. Which I think we must fix.
> > > >
> > > > Yes indeed. I've been using my script for years now and have
> > > > completely missed the fact that the unused ports are not idled
> > > > at all on start-up when unused.
> > >
> > > This might be all that's needed, care to try it and if it works
> > > I'll send out two separate proper patches?
> > >
> > > I'm seeing this now on my bbb after temporarily disabling my
> > > UART idle init script:
> > >
> > > # rwmem -s32 0x44e004b4 # uart 1 on l4_wkup
> > > 0x44e004b4 = 0x00000002
> > >
> > > # rwmem -s32 0x44e0006c+0x10 # uart 2 - 5 on l4_per
> > > 0x44e0006c = 0x00030000
> > > 0x44e00070 = 0x00030000
> > > 0x44e00074 = 0x00030000
> > > 0x44e00078 = 0x00030000
> > >
> > > # rwmem -s32 0x44e00038 # uart 6 on l4_per
> > > 0x44e00038 = 0x00030000
> >
> > Hmm I forgot to remove status = "disabled" from the other ports,
> > still not idling the unused ports with the patch below it seems.
>
> No need to enable/disable autosuspend except in startup and shutdown
> I think. The patch below works for me, now includes removal of the
> status = "disabled" flags too. Only tested with 8250_omap on bbb
> so far.
>
> I wonder if other places still need fixing for autosuspend
> like console support?
While this seems to fix the idling of closed ports here too, I'm not
sure we can move use_autosuspend to startup() like this.
First, this flag should be set before registering the tty so that udev
can be used to update the attributes.
Second, this prevents setting the autosuspend delay through sysfs when
the port is closed (when autosuspend is disabled).
It seems we really should not be using the negative autosuspend to
configure the RPM behaviour the way these drivers do. Perhaps a new
mechanism is needed.
But I'm afraid I don't have time to look at this today.
Thanks,
Johan
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -605,6 +605,7 @@ static int omap_8250_startup(struct uart_port *port)
> return ret;
> }
>
> + pm_runtime_use_autosuspend(port->dev);
> pm_runtime_get_sync(port->dev);
>
> up->mcr = 0;
> @@ -685,8 +686,8 @@ static void omap_8250_shutdown(struct uart_port *port)
> serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
> serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
>
> - pm_runtime_mark_last_busy(port->dev);
> - pm_runtime_put_autosuspend(port->dev);
> + pm_runtime_dont_use_autosuspend(port->dev);
> + pm_runtime_put_sync(port->dev);
> free_irq(port->irq, port);
> dev_pm_clear_wake_irq(port->dev);
> }
> @@ -1226,7 +1227,7 @@ static int omap8250_probe(struct platform_device *pdev)
> spin_lock_init(&priv->rx_dma_lock);
>
> device_init_wakeup(&pdev->dev, true);
> - pm_runtime_use_autosuspend(&pdev->dev);
> + /* See omap_8250_startup and shutdown for autosuspend */
> pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
>
> pm_runtime_irq_safe(&pdev->dev);
> @@ -1265,8 +1266,8 @@ static int omap8250_probe(struct platform_device *pdev)
> }
> priv->line = ret;
> platform_set_drvdata(pdev, priv);
> - pm_runtime_mark_last_busy(&pdev->dev);
> - pm_runtime_put_autosuspend(&pdev->dev);
> + pm_runtime_put_sync(&pdev->dev);
> +
> return 0;
> err:
^ 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-09 13:57 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
In-Reply-To: <20180509131003.GC2285@localhost>
* Johan Hovold <johan@kernel.org> [180509 13:12]:
> While this seems to fix the idling of closed ports here too, I'm not
> sure we can move use_autosuspend to startup() like this.
>
> First, this flag should be set before registering the tty so that udev
> can be used to update the attributes.
>
> Second, this prevents setting the autosuspend delay through sysfs when
> the port is closed (when autosuspend is disabled).
Yeah I noticed that too yesterday.
> It seems we really should not be using the negative autosuspend to
> configure the RPM behaviour the way these drivers do. Perhaps a new
> mechanism is needed.
Hmm well simply defaulting to "on" instead of "auto" and setting the
autosuspend_ms to 3000 by default might be doable. I think that way
we can keep use_autosuspend() in probe. Let's hope there are no
existing use cases that would break with that.
> But I'm afraid I don't have time to look at this today.
Sure np thanks,
Tony
^ permalink raw reply
* Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)
From: Tony Lindgren @ 2018-05-09 14:05 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-pm
In-Reply-To: <20180509091831.GA2285@localhost>
* Johan Hovold <johan@kernel.org> [180509 09:20]:
> On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> > I think using open/close for runtime pm is good enough for GPS,
> > since it regularly sends data and draws lots of power anyways.
> > But devices, that have an out-of-band wakeup signal can do proper
> > runtime PM of the serial port without loosing characters.
>
> Yeah, there may be some applications where this is possible. And this is
> not the case for GPS, but not just because of a generally higher power
> consumption, but due to the fact that we cannot afford having the first
> message in every report burst be dropped.
Well most of the phone implementations use one or two out of band
GPIOs to first wake the UART before any data is sent. For serdev
this can be called from the serdev consumer write function for TX.
For RX, the serdev consumer needs to implement an interrupt handler
and wake up the parent UART before serdev RX.
> > Note, that OMAP does not reach deep idle states with active
> > serial port. This is not acceptable for low power devices.
>
> Sure, but note that OMAP is the only serial driver which currently
> implements this kind of aggressive runtime PM (besides a couple of
> usb-serial drivers). This means that a serdev driver can never rely on
> this being the case, and therefore needs to be restrictive about how
> long the port is kept open if it cares about power at all.
Well by default we don't allow lossy UART. It needs to be manually
configured via /sys for the timeout. With serdev, this can all be
done with no /sys configuration needed for the cases with GPIO
wake irqs :)
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()
From: Jacek Anaszewski @ 2018-05-09 19:57 UTC (permalink / raw)
To: Uwe Kleine-König, Marc Kleine-Budde
Cc: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Pavel Machek,
One Thousand Gnomes, Florian Fainelli, linux-serial,
Mathieu Poirier, linux-kernel, linux-can, linux-arm-kernel,
kernel, Robin Murphy, linux-leds
In-Reply-To: <20180508201725.3alpkcpaxebupzqv@pengutronix.de>
Hi Uwe,
On 05/08/2018 10:17 PM, Uwe Kleine-König wrote:
> Hello Jacek,
>
> On Tue, May 08, 2018 at 09:33:14PM +0200, Jacek Anaszewski wrote:
>> Thank you for the patch. It looks fine, but please split
>> the drivers/net/can/led.c related changes into a separate one.
>
> I renamed led_trigger_rename_static() to led_trigger_rename() (and
> changed the parameters). The can change just adapts the only user of
> led_trigger_rename_static() to use the new one.
>
> It's not impossible to separate this patches, but I wonder if it's worth
> the effort.
>
> The first patch would be like the patch under discussion, just without
> the can bits and introducing something like:
>
> /*
> * compat stuff to be removed once the only caller is converted
> */
> static inline led_trigger_rename_static(const char *name, struct led_trigger *trig)
> {
> (void)led_trigger_rename(trig, "%s", name);
> }
>
> Then the second patch would just be the 6-line can hunk. And a third
> patch would remove the compat function. (Maybe I'd choose to squash the
> two can patches together then, but this doesn't reduce the overhead
> considerably.) The only upside I can see here is that it increases my
> patch count, but it's otherwise not worth the effort for such an easy
> change. Further more as there is a strict dependency on these three
> patches this either delays the cleanup or (IMHO more likely) the can
> change would go in via the led tree anyhow. (Mark already acked patch 2
> of this series and in private confirmed that the agrees to let this
> change go in via the led tree, too.)
OK, makes sense. I'll wait also for ack on 3/3 since it should
go through the LED tree as well - uses a new led_trigger_register_format().
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* [PATCH] tty: serial: msm_geni_serial: Fix TX infinite loop
From: Evan Green @ 2018-05-09 20:44 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Karthikeyan Ramasubramanian, Doug Anderson
Cc: Evan Green
The GENI serial driver handled transmit by leaving stuff in the
common circular buffer until it had completely caught up to the head,
then clearing it out all at once. This is a suboptimal way to do
transmit, as it leaves data in the circular buffer that could be
freed. Moreover, the logic implementing it is wrong, and it is easy to
get into a situation where the UART infinitely writes out the same buffer.
I could reproduce infinite serial output of the same buffer by running
dmesg, then hitting Ctrl-C. I believe what happened is xmit_size was
something large, marching towards a larger value. Then the generic OS
code flushed out the buffer and replaced it with two characters. Now the
xmit_size is a large value marching towards a small value, which it wasn't
expecting. The driver subtracts xmit_size (very large) from
uart_circ_chars_pending (2), underflows, and repeats ad nauseum. The
locking isn't wrong here, as the locks are held whenever the buffer is
manipulated, it's just that the driver wasn't expecting the buffer to be
flushed out from underneath it in between transmits.
This change reworks transmit to grab what it can from the circular buffer,
and then update ->tail, both fixing the underflow and freeing up space
for a smoother circular experience.
Signed-off-by: Evan Green <evgreen@chromium.org>
---
Note: This patch applies on top of Karthik's series of 8 fixup patches,
which seem basically ready to go, at:
https://www.spinics.net/lists/linux-arm-msm/msg36561.html
Karthik had some concerns here in that apparently he had done it the way it
was on purpose in order to avoid a watchdog timeout with very large kernel
logs. Doug's and my best interpretation of his explanation is that maybe the
UART is so fast, and the FIFO is potentially only 16 bytes wide, so by the
time the data has been loaded up into the FIFO and the interrupt handler
returns, the UART has finished and the interrupt fires again immediately.
His original solution works because the buffer fills up completely,
and handle_tx calls stop_tx and uart_write_wakeup, which usually happens to
take enough time to let the interrupting core schedule something.
The old way this driver was doing it, artificially letting the circular buffer
balloon up and eventually pop, still seems like the wrong approach to me.
Perhaps this means that DMA mode should be considered, or a threaded irq,
or at least a larger FIFO. Or perhaps smaller kernel logs.
drivers/tty/serial/qcom_geni_serial.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 9d773a991369..f296a62bd811 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -98,7 +98,6 @@ struct qcom_geni_serial_port {
enum geni_se_xfer_mode xfer_mode;
bool setup;
int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
- unsigned int xmit_size;
unsigned int baud;
unsigned int tx_bytes_pw;
unsigned int rx_bytes_pw;
@@ -462,7 +461,6 @@ static void qcom_geni_serial_stop_tx(struct uart_port *uport)
writel_relaxed(0, uport->membase +
SE_GENI_TX_WATERMARK_REG);
}
- port->xmit_size = 0;
writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
status = readl_relaxed(uport->membase + SE_GENI_STATUS);
/* Possible stop tx is called multiple times. */
@@ -592,16 +590,13 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport)
chunk = uart_circ_chars_pending(xmit);
status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
/* Both FIFO and framework buffer are drained */
- if (chunk == port->xmit_size && !status) {
- port->xmit_size = 0;
- uart_circ_clear(xmit);
+ if (!chunk && !status) {
qcom_geni_serial_stop_tx(uport);
goto out_write_wakeup;
}
- chunk -= port->xmit_size;
avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
- tail = (xmit->tail + port->xmit_size) & (UART_XMIT_SIZE - 1);
+ tail = xmit->tail;
chunk = min3((size_t)chunk, (size_t)(UART_XMIT_SIZE - tail), avail);
if (!chunk)
goto out_write_wakeup;
@@ -622,14 +617,16 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport)
iowrite32_rep(uport->membase + SE_GENI_TX_FIFOn, buf, 1);
i += tx_bytes;
- tail = (tail + tx_bytes) & (UART_XMIT_SIZE - 1);
+ tail += tx_bytes;
uport->icount.tx += tx_bytes;
remaining -= tx_bytes;
}
+
+ xmit->tail = tail & (UART_XMIT_SIZE - 1);
qcom_geni_serial_poll_tx_done(uport);
- port->xmit_size += chunk;
out_write_wakeup:
- uart_write_wakeup(uport);
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(uport);
}
static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
--
2.17.0.441.gb46fe60e1d-goog
^ permalink raw reply related
* Re: [PATCH v2] tty: implement led triggers
From: Pavel Machek @ 2018-05-10 11:14 UTC (permalink / raw)
To: Johan Hovold
Cc: Uwe Kleine-König, linux-serial, One Thousand Gnomes,
Florian Fainelli, Mathieu Poirier, Greg Kroah-Hartman,
linux-kernel, kernel, linux-arm-kernel, Robin Murphy
In-Reply-To: <20180507092710.GQ2285@localhost>
[-- Attachment #1: Type: text/plain, Size: 952 bytes --]
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...)
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()
From: Pavel Machek @ 2018-05-10 11:21 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Jacek Anaszewski,
linux-serial, linux-leds, linux-can, kernel, One Thousand Gnomes,
Florian Fainelli, Mathieu Poirier, linux-kernel, Robin Murphy,
linux-arm-kernel
In-Reply-To: <20180508100543.12559-2-u.kleine-koenig@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]
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.
I believe we should have triggers "net-rx, net-tx" and it should have
parameter "which device it acts on".
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()
From: Pavel Machek @ 2018-05-10 11:22 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Jacek Anaszewski,
linux-serial, linux-leds, linux-can, kernel, One Thousand Gnomes,
Florian Fainelli, Mathieu Poirier, linux-kernel, Robin Murphy,
linux-arm-kernel
In-Reply-To: <20180510112101.GD6977@amd>
[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]
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.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v2] tty: implement led triggers
From: Robin Murphy @ 2018-05-10 11:25 UTC (permalink / raw)
To: Pavel Machek, Johan Hovold
Cc: Uwe Kleine-König, linux-serial, One Thousand Gnomes,
Florian Fainelli, Mathieu Poirier, Greg Kroah-Hartman,
linux-kernel, kernel, linux-arm-kernel
In-Reply-To: <20180510111434.GC6977@amd>
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.
Robin.
^ permalink raw reply
* [PATCH] serial: 8250_dw: Fix runtime PM handling
From: Phil Edworthy @ 2018-05-10 14:13 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman
Cc: Jiri Slaby, Joshua Scott, Philipp Zabel, Stefan Potyra, Ed Blake,
Michel Pollet, linux-serial, linux-kernel, Phil Edworthy
When using kgdb, you get an abort when accessing the UART registers.
This is because the driver has already entered runtime PM and so turned
off the bus clock needed to access the registers.
To fix this, set the capability indicating Runtime PM is active while idle.
Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
drivers/tty/serial/8250/8250_dw.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 0529b5c..aff04f1 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -576,6 +576,10 @@ static int dw8250_probe(struct platform_device *pdev)
if (!data->skip_autocfg)
dw8250_setup_port(p);
+#ifdef CONFIG_PM
+ uart.capabilities |= UART_CAP_RPM;
+#endif
+
/* If we have a valid fifosize, try hooking up DMA */
if (p->fifosize) {
data->dma.rxconf.src_maxburst = p->fifosize / 4;
--
2.7.4
^ permalink raw reply related
* RE: [patch v18 0/4] JTAG driver introduction
From: Oleksandr Shamray @ 2018-05-10 15:13 UTC (permalink / raw)
To: Florian Fainelli, gregkh@linuxfoundation.org, arnd@arndb.de
Cc: linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
openbmc@lists.ozlabs.org, joel@jms.id.au, jiri@resnulli.us,
tklauser@distanz.ch, linux-serial@vger.kernel.org,
Vadim Pasternak, system-sw-low-level, robh+dt@kernel.org,
openocd-devel-owner@lists.sourceforge.net,
linux-api@vger.kernel.org, davem@davemloft.net
In-Reply-To: <ff739c26-d3b2-3df7-e751-fbf037ae96d1@gmail.com>
Hi Florian.
>> We propose to implement general JTAG interface and infrastructure
>> to communicate with user layer application. In such way, we can
>> have the standard JTAG interface core part and separation from
>> specific HW implementation.
>
>Well, the framework in its current shape is still extremely
>simplistic, therefore leaving a lot of room (read: bugs,
>inconsistencies) within the hands of the driver, so while the
>user-space interface is standard through the proposed character
>device, the user experience, likely might not.
Well, the framework is intentionally simply and provide the very basic
interface. It supposed that interface can be extended in the future, if
necessary. Current kernel does not provide a framework for JTAG interface,
and we believe this is good starting point to allow such framework. This
provide minimal, but sufficient interface and any future extension will be
of course welcomed.
At the moment we have one controller driver, which works well with this
infrastructure. And we have very reasonable use case - this driver allows
flashing for all the programmable devices, connected to JTAG interface,
where the image for these device is provided with the standard SVF format.
>> This allow new capability to debug the CPU or program system's
>> device via BMC without additional devices nor cost.
>If that is the case, should not we leverage the kernel's device
>driver model and expect the JTAG framework to create specific devices
>for the different pieces of HW discovered on the scan chain? That
>would also
>presumably allow the core JTAG framework to retain the necessary
>state
>changes in order to address one particular device within the scan chain.
For the device programming use case, the flashing image will contain
jtag chain topology. If for example, jtag domain contains x devices, but
image contains less, the only relevant devices will be flashed and there is
no need to distinct between them.
In case there are several domains, jtag interface is configured to be
connected to the relevant one.
For the CPU debug use case, jtag interface is configured to be connected
to CPU only.
System should be able to provide jtag selection according to the needs
and this kind of operations should be out of the JTAG driver scope.
>> This patch purpose is to add JTAG master core infrastructure by
>> defining new JTAG class and provide generic JTAG interface to allow
>> hardware specific drivers to connect this interface.
>> This will enable all JTAG drivers to use the common interface part
>> and will have separate for hardware implementation.
>Let's consider I want to get rid of OpenOCD, or rather, move its
>driver interface within the kernel and replace it on the OpenOCD side
>with a generic character device interface. I could presumably
>amortize the costly operations which are currently I/O and/or system
>call limiting when running in user-space, what would it look like
>with your proposed framework, have you given some thoughts about that?
The JTAG driver using SDR/SIR transactions to send/receive data.
It can send/receive multiple data bits by a single transaction.
For the bit-banging style drivers it gives you an advantage of sending
Bits stream by one system call vs per-bit system call.
So, instead of updating GPIO pins a few time through user space interface
(sysfs) for sending single bit, an application can send multiple bits in a
stream by one call. And all GPIO operations will be performed than in a
kernel space.
It'll be necessary to have bit-banging driver, using the JTAG infrastructure,
which should be configured according to the particular system during
initialization. And we are planning to develop such kind of driver.
It definitely reduces system calls and saves time for JTAG operations.
For the system equipped with the JTAG master there the same advantages
plus hardware acceleration for pin access (all pins can be accessed by
one shot).
Driver supports universal transactions (include/uapi/linux/jtag.h),
which are sent through IOCTL interface JTAG_IOCRUNTEST, JTAG_IOCXFER
struct struct jtag_xfer {
__u8 type;
__u8 direction;
__u8 endstate;
__u8 padding;
__u32 length;
__u64 tdio;
};
jtag_run_test_idle {
__u8 reset;
__u8 endstate;
__u8 tck;
};
You can see usage example on https://github.com/mellanoxbmc/mellanox-bmc-tools/tree/master/mlnx_cpldprog
Best Regards,
Oleksandr Shamray
> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: 31 января 2018 г. 5:03
> To: Oleksandr Shamray <oleksandrs@mellanox.com>;
> gregkh@linuxfoundation.org; arnd@arndb.de
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; openbmc@lists.ozlabs.org; joel@jms.id.au;
> jiri@resnulli.us; tklauser@distanz.ch; linux-serial@vger.kernel.org; Vadim
> Pasternak <vadimp@mellanox.com>; system-sw-low-level <system-sw-low-
> level@mellanox.com>; robh+dt@kernel.org; openocd-devel-
> owner@lists.sourceforge.net; linux-api@vger.kernel.org;
> davem@davemloft.net; mchehab@kernel.org
> Subject: Re: [patch v18 0/4] JTAG driver introduction
>
> On 01/29/2018 06:31 AM, Oleksandr Shamray wrote:
> > When a need raise up to use JTAG interface for system's devices
> > programming or CPU debugging, usually the user layer application
> > implements jtag protocol by bit-bang or using a proprietary connection
> > to vendor hardware.
> > This method can be slow and not generic.
> >
> > We propose to implement general JTAG interface and infrastructure to
> > communicate with user layer application. In such way, we can have the
> > standard JTAG interface core part and separation from specific HW
> > implementation.
> > This allow new capability to debug the CPU or program system's device
> > via BMC without additional devices nor cost.
>
> Oleksandr, you have completed dodged my questions here:
>
> https://lkml.org/lkml/2017/12/25/163
>
> can you try to respond to some of these questions please?
> --
> Florian
^ permalink raw reply
* [patch v19 0/4] JTAG driver introduction
From: Oleksandr Shamray @ 2018-05-10 15:44 UTC (permalink / raw)
To: gregkh, arnd
Cc: linux-kernel, linux-arm-kernel, devicetree, openbmc, joel, jiri,
tklauser, linux-serial, vadimp, system-sw-low-level, robh+dt,
openocd-devel-owner, linux-api, davem, mchehab, Oleksandr Shamray
When a need raise up to use JTAG interface for system's devices
programming or CPU debugging, usually the user layer
application implements jtag protocol by bit-bang or using a
proprietary connection to vendor hardware.
This method can be slow and not generic.
We propose to implement general JTAG interface and infrastructure
to communicate with user layer application. In such way, we can
have the standard JTAG interface core part and separation from
specific HW implementation.
This allow new capability to debug the CPU or program system's
device via BMC without additional devices nor cost.
This patch purpose is to add JTAG master core infrastructure by
defining new JTAG class and provide generic JTAG interface
to allow hardware specific drivers to connect this interface.
This will enable all JTAG drivers to use the common interface
part and will have separate for hardware implementation.
The JTAG (Joint Test Action Group) core driver provides minimal generic
JTAG interface, which can be used by hardware specific JTAG master
controllers. By providing common interface for the JTAG controllers,
user space device programing is hardware independent.
Modern SoC which in use for embedded system' equipped with
internal JTAG master interface.
This interface is used for programming and debugging system's
hardware components, like CPLD, FPGA, CPU, voltage and
industrial controllers.
Firmware for such devices can be upgraded through JTAG interface during
Runtime. The JTAG standard support for multiple devices programming,
is in case their lines are daisy-chained together.
For example, systems which equipped with host CPU, BMC SoC or/and
number of programmable devices are capable to connect a pin and
select system components dynamically for programming and debugging,
This is using by the BMC which is equipped with internal SoC master
controller.
For example:
BMC JTAG master --> pin selected to CPLDs chain for programming (filed
upgrade, production)
BMC JTAG master --> pin selected to voltage monitors for programming
(field upgrade, production)
BMC JTAG master --> pin selected to host CPU (on-site debugging
and developers debugging)
For example, we can have application in user space which using calls
to JTAG driver executes CPLD programming directly from SVF file
The JTAG standard (IEEE 1149.1) defines the next connector pins:
- TDI (Test Data In);
- TDO (Test Data Out);
- TCK (Test Clock);
- TMS (Test Mode Select);
- TRST (Test Reset) (Optional);
The SoC equipped with JTAG master controller, performs
device programming on command or vector level. For example
a file in a standard SVF (Serial Vector Format) that contains
boundary scan vectors, can be used by sending each vector
to the JTAG interface and the JTAG controller will execute
the programming.
Initial version provides the system calls set for:
- SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
- SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
- RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
number of clocks.
SoC which are not equipped with JTAG master interface, can be built
on top of JTAG core driver infrastructure, by applying bit-banging of
TDI, TDO, TCK and TMS pins within the hardware specific driver.
Oleksandr Shamray (4):
drivers: jtag: Add JTAG core driver
drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master
driver
Documentation: jtag: Add bindings for Aspeed SoC 24xx and 25xx
families JTAG master driver
Documentation: jtag: Add ABI documentation
Documentation/ABI/testing/jtag-dev | 27 +
.../devicetree/bindings/jtag/aspeed-jtag.txt | 22 +
Documentation/ioctl/ioctl-number.txt | 2 +
Documentation/jtag/overview | 31 +
Documentation/jtag/transactions | 108 +++
MAINTAINERS | 10 +
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/jtag/Kconfig | 30 +
drivers/jtag/Makefile | 2 +
drivers/jtag/jtag-aspeed.c | 786 ++++++++++++++++++++
drivers/jtag/jtag.c | 273 +++++++
include/linux/jtag.h | 41 +
include/uapi/linux/jtag.h | 105 +++
14 files changed, 1440 insertions(+), 0 deletions(-)
create mode 100644 Documentation/ABI/testing/jtag-dev
create mode 100644 Documentation/devicetree/bindings/jtag/aspeed-jtag.txt
create mode 100644 Documentation/jtag/overview
create mode 100644 Documentation/jtag/transactions
create mode 100644 drivers/jtag/Kconfig
create mode 100644 drivers/jtag/Makefile
create mode 100644 drivers/jtag/jtag-aspeed.c
create mode 100644 drivers/jtag/jtag.c
create mode 100644 include/linux/jtag.h
create mode 100644 include/uapi/linux/jtag.h
^ permalink raw reply
* [patch v19 1/4] drivers: jtag: Add JTAG core driver
From: Oleksandr Shamray @ 2018-05-10 15:44 UTC (permalink / raw)
To: gregkh, arnd
Cc: linux-kernel, linux-arm-kernel, devicetree, openbmc, joel, jiri,
tklauser, linux-serial, vadimp, system-sw-low-level, robh+dt,
openocd-devel-owner, linux-api, davem, mchehab, Oleksandr Shamray,
Jiri Pirko
In-Reply-To: <1525967064-10760-1-git-send-email-oleksandrs@mellanox.com>
Initial patch for JTAG driver
JTAG class driver provide infrastructure to support hardware/software
JTAG platform drivers. It provide user layer API interface for flashing
and debugging external devices which equipped with JTAG interface
using standard transactions.
Driver exposes set of IOCTL to user space for:
- XFER:
- SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
- SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
- RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
number of clocks).
- SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.
Driver core provides set of internal APIs for allocation and
registration:
- jtag_register;
- jtag_unregister;
- jtag_alloc;
- jtag_free;
Platform driver on registration with jtag-core creates the next
entry in dev folder:
/dev/jtagX
Signed-off-by: Oleksandr Shamray <oleksandrs@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Acked-by: Philippe Ombredanne <pombredanne@nexb.com>
---
v18->v19
Comments pointed by Julia Cartwright <juliac@eso.teric.us>
- Fix memory leak on jtag_alloc exit
v17->v18
Comments pointed by Julia Cartwright <juliac@eso.teric.us>
- Change to return -EOPNOTSUPP in case of error in JTAG_GIOCFREQ
- Add ops callbacks check to jtag_alloc
- Add err check for copy_to_user
- Move the kfree() above the if (err) in JTAG_IOCXFER
- remove unnecessary check for error after put_user
- add padding to struct jtag_xfer
v16->v17
Comments pointed by Julia Cartwright <juliac@eso.teric.us>
- Fix memory allocation on jtag alloc
- Move out unnecessary form lock on jtag open
- Rework jtag register behavior
v15->v16
Commen ts pointed by Florian Fainelli <f.fainelli@gmail.com>
- move check jtag->ops->* in ioctl before get_user()
- change error type -EINVAL --> -EBUSY on open already opened jtag
- remove unnecessary ARCH_DMA_MINALIGN flag from kzalloc
- remove define ARCH_DMA_MINALIGN
v14->v15
v13->v14
Comments pointed by Philippe Ombredanne <pombredanne@nexb.com>
- Change style of head block comment from /**/ to //
v12->v13
Comments pointed by Philippe Ombredanne <pombredanne@nexb.com>
- Change jtag.c licence type to
SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
and reorder line with license in description
v11->v12
Comments pointed by Greg KH <gregkh@linuxfoundation.org>
- Change jtag.h licence type to
SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
and reorder line with license in description
Chip Bilbrey <chip@bilbrey.org>
- Remove Apeed reference from uapi jtag.h header
- Remove access mode from xfer and idle transactions
- Add new ioctl JTAG_SIOCMODE for set hw mode
- Add only one open per JTAG port blocking with mutex blocking
v10->v11
Notifications from kbuild test robot <lkp@intel.com>
- include types.h headeri to jtag.h
- fix incompatible type of xfer callback
- remove rdundant class defination
- Fix return order in case of xfer error
V9->v10
Comments pointed by Greg KH <gregkh@linuxfoundation.org>
- remove unnecessary alignment for pirv data
- move jtag_copy_to_user and jtag_copy_from_user code just to ioctl
- move int jtag_run_test_idle_op and jtag_xfer_op code
just to ioctl
- change return error codes to more applicable
- add missing error checks
- fix error check order in ioctl
- remove unnecessary blank lines
- add param validation to ioctl
- remove compat_ioctl
- remove only one open per JTAG port blocking.
User will care about this.
- Fix idr memory leak on jtag_exit
- change cdev device type to misc
V8->v9
Comments pointed by Arnd Bergmann <arnd@arndb.de>
- use get_user() instead of __get_user().
- change jtag->open type from int to atomic_t
- remove spinlock on jtg_open
- remove mutex on jtag_register
- add unregister_chrdev_region on jtag_init err
- add unregister_chrdev_region on jtag_exit
- remove unnecessary pointer casts
- add *data parameter to xfer function prototype
v7->v8
Comments pointed by Moritz Fischer <moritz.fischer@ettus.com>
- Fix misspelling s/friver/driver
v6->v7
Notifications from kbuild test robot <lkp@intel.com>
- Remove include asm/types.h from jtag.h
- Add include <linux/types.h> to jtag.c
v5->v6
v4->v5
v3->v4
Comments pointed by Arnd Bergmann <arnd@arndb.de>
- change transaction pointer tdio type to __u64
- change internal status type from enum to __u32
- reorder jtag_xfer members to avoid the implied padding
- add __packed attribute to jtag_xfer and jtag_run_test_idle
v2->v3
Notifications from kbuild test robot <lkp@intel.com>
- Change include path to <linux/types.h> in jtag.h
v1->v2
Comments pointed by Greg KH <gregkh@linuxfoundation.org>
- Change license type from GPLv2/BSD to GPLv2
- Change type of variables which crossed user/kernel to __type
- Remove "default n" from Kconfig
Comments pointed by Andrew Lunn <andrew@lunn.ch>
- Change list_add_tail in jtag_unregister to list_del
Comments pointed by Neil Armstrong <narmstrong@baylibre.com>
- Add SPDX-License-Identifier instead of license text
Comments pointed by Arnd Bergmann <arnd@arndb.de>
- Change __copy_to_user to memdup_user
- Change __put_user to put_user
- Change type of variables to __type for compatible 32 and 64-bit systems
- Add check for maximum xfer data size
- Change lookup data mechanism to get jtag data from inode
- Add .compat_ioctl to file ops
- Add mem alignment for jtag priv data
Comments pointed by Tobias Klauser <tklauser@distanz.ch>
- Change function names to avoid match with variable types
- Fix description for jtag_ru_test_idle in uapi jtag.h
- Fix misprints IDEL/IDLE, trough/through
---
Documentation/ioctl/ioctl-number.txt | 2 +
MAINTAINERS | 10 ++
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/jtag/Kconfig | 16 ++
drivers/jtag/Makefile | 1 +
drivers/jtag/jtag.c | 273 ++++++++++++++++++++++++++++++++++
include/linux/jtag.h | 41 +++++
include/uapi/linux/jtag.h | 105 +++++++++++++
9 files changed, 451 insertions(+), 0 deletions(-)
create mode 100644 drivers/jtag/Kconfig
create mode 100644 drivers/jtag/Makefile
create mode 100644 drivers/jtag/jtag.c
create mode 100644 include/linux/jtag.h
create mode 100644 include/uapi/linux/jtag.h
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 7f7413e..c5548b5 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -318,6 +318,8 @@ Code Seq#(hex) Include File Comments
0xB0 all RATIO devices in development:
<mailto:vgo@ratio.de>
0xB1 00-1F PPPoX <mailto:mostrows@styx.uwaterloo.ca>
+0xB2 00-0f linux/jtag.h JTAG driver
+ <mailto:oleksandrs@mellanox.com>
0xB3 00 linux/mmc/ioctl.h
0xB4 00-0F linux/gpio.h <mailto:linux-gpio@vger.kernel.org>
0xB5 00-0F uapi/linux/rpmsg.h <mailto:linux-remoteproc@vger.kernel.org>
diff --git a/MAINTAINERS b/MAINTAINERS
index 79bb02f..2fd22d6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7610,6 +7610,16 @@ L: linux-serial@vger.kernel.org
S: Maintained
F: drivers/tty/serial/jsm/
+JTAG SUBSYSTEM
+M: Oleksandr Shamray <oleksandrs@mellanox.com>
+M: Vadim Pasternak <vadimp@mellanox.com>
+S: Maintained
+F: include/linux/jtag.h
+F: include/uapi/linux/jtag.h
+F: drivers/jtag/
+F: Documentation/devicetree/bindings/jtag/
+F: Documentation/ABI/testing/jtag-cdev
+
K10TEMP HARDWARE MONITORING DRIVER
M: Clemens Ladisch <clemens@ladisch.de>
L: linux-hwmon@vger.kernel.org
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 95b9ccc..bb71e48 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -217,4 +217,6 @@ source "drivers/siox/Kconfig"
source "drivers/slimbus/Kconfig"
+source "drivers/jtag/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 24cd470..c92636b 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -185,3 +185,4 @@ obj-$(CONFIG_TEE) += tee/
obj-$(CONFIG_MULTIPLEXER) += mux/
obj-$(CONFIG_UNISYS_VISORBUS) += visorbus/
obj-$(CONFIG_SIOX) += siox/
+obj-$(CONFIG_JTAG) += jtag/
diff --git a/drivers/jtag/Kconfig b/drivers/jtag/Kconfig
new file mode 100644
index 0000000..5e2c75b
--- /dev/null
+++ b/drivers/jtag/Kconfig
@@ -0,0 +1,16 @@
+menuconfig JTAG
+ tristate "JTAG support"
+ help
+ This provides basic core functionality support for jtag class devices
+ Hardware equipped with JTAG microcontroller which can be built
+ on top of this drivers. Driver exposes the set of IOCTL to the
+ user space for:
+ SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
+ SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
+ RUNTEST (Forces IEEE 1149.1 bus to a run state for specified
+ number of clocks).
+
+ If you want this support, you should say Y here.
+
+ To compile this driver as a module, choose M here: the module will
+ be called jtag.
diff --git a/drivers/jtag/Makefile b/drivers/jtag/Makefile
new file mode 100644
index 0000000..af37493
--- /dev/null
+++ b/drivers/jtag/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_JTAG) += jtag.o
diff --git a/drivers/jtag/jtag.c b/drivers/jtag/jtag.c
new file mode 100644
index 0000000..7f8dca7
--- /dev/null
+++ b/drivers/jtag/jtag.c
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// drivers/jtag/jtag.c
+//
+// Copyright (c) 2018 Mellanox Technologies. All rights reserved.
+// Copyright (c) 2018 Oleksandr Shamray <oleksandrs@mellanox.com>
+
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/jtag.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/rtnetlink.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <uapi/linux/jtag.h>
+
+#define JTAG_NAME "jtag0"
+#define MAX_JTAG_NAME_LEN (sizeof("jtag") + 5)
+
+struct jtag {
+ struct miscdevice miscdev;
+ struct device *dev;
+ const struct jtag_ops *ops;
+ int id;
+ bool opened;
+ struct mutex open_lock;
+ unsigned long priv[0];
+};
+
+static DEFINE_IDA(jtag_ida);
+
+void *jtag_priv(struct jtag *jtag)
+{
+ return jtag->priv;
+}
+EXPORT_SYMBOL_GPL(jtag_priv);
+
+static long jtag_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct jtag *jtag = file->private_data;
+ struct jtag_run_test_idle idle;
+ struct jtag_xfer xfer;
+ u8 *xfer_data;
+ u32 data_size;
+ u32 value;
+ int err;
+
+ if (!arg)
+ return -EINVAL;
+
+ switch (cmd) {
+ case JTAG_GIOCFREQ:
+ if (!jtag->ops->freq_get)
+ return -EOPNOTSUPP;
+
+ err = jtag->ops->freq_get(jtag, &value);
+ if (err)
+ break;
+
+ if (put_user(value, (__u32 *)arg))
+ err = -EFAULT;
+ break;
+
+ case JTAG_SIOCFREQ:
+ if (!jtag->ops->freq_set)
+ return -EOPNOTSUPP;
+
+ if (get_user(value, (__u32 *)arg))
+ return -EFAULT;
+ if (value == 0)
+ return -EINVAL;
+
+ err = jtag->ops->freq_set(jtag, value);
+ break;
+
+ case JTAG_IOCRUNTEST:
+ if (copy_from_user(&idle, (void *)arg,
+ sizeof(struct jtag_run_test_idle)))
+ return -EFAULT;
+
+ if (idle.endstate > JTAG_STATE_PAUSEDR)
+ return -EINVAL;
+
+ err = jtag->ops->idle(jtag, &idle);
+ break;
+
+ case JTAG_IOCXFER:
+ if (copy_from_user(&xfer, (void *)arg,
+ sizeof(struct jtag_xfer)))
+ return -EFAULT;
+
+ if (xfer.length >= JTAG_MAX_XFER_DATA_LEN)
+ return -EINVAL;
+
+ if (xfer.type > JTAG_SDR_XFER)
+ return -EINVAL;
+
+ if (xfer.direction > JTAG_WRITE_XFER)
+ return -EINVAL;
+
+ if (xfer.endstate > JTAG_STATE_PAUSEDR)
+ return -EINVAL;
+
+ data_size = DIV_ROUND_UP(xfer.length, BITS_PER_BYTE);
+ xfer_data = memdup_user(u64_to_user_ptr(xfer.tdio), data_size);
+
+ if (IS_ERR(xfer_data))
+ return -EFAULT;
+
+ err = jtag->ops->xfer(jtag, &xfer, xfer_data);
+ if (err) {
+ kfree(xfer_data);
+ return -EFAULT;
+ }
+
+ err = copy_to_user(u64_to_user_ptr(xfer.tdio),
+ (void *)(xfer_data), data_size);
+ kfree(xfer_data);
+ if (err)
+ return -EFAULT;
+
+ if (copy_to_user((void *)arg, &xfer, sizeof(struct jtag_xfer)))
+ return -EFAULT;
+ break;
+
+ case JTAG_GIOCSTATUS:
+ err = jtag->ops->status_get(jtag, &value);
+ if (err)
+ break;
+
+ err = put_user(value, (__u32 *)arg);
+ break;
+ case JTAG_SIOCMODE:
+ if (get_user(value, (__u32 *)arg))
+ return -EFAULT;
+ if (value == 0)
+ return -EINVAL;
+
+ err = jtag->ops->mode_set(jtag, value);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+ return err;
+}
+
+static int jtag_open(struct inode *inode, struct file *file)
+{
+ struct jtag *jtag = container_of(file->private_data, struct jtag,
+ miscdev);
+
+ if (mutex_lock_interruptible(&jtag->open_lock))
+ return -ERESTARTSYS;
+
+ if (jtag->opened) {
+ mutex_unlock(&jtag->open_lock);
+ return -EBUSY;
+ }
+ jtag->opened = true;
+ mutex_unlock(&jtag->open_lock);
+
+ nonseekable_open(inode, file);
+ file->private_data = jtag;
+ return 0;
+}
+
+static int jtag_release(struct inode *inode, struct file *file)
+{
+ struct jtag *jtag = file->private_data;
+
+ mutex_lock(&jtag->open_lock);
+ jtag->opened = false;
+ mutex_unlock(&jtag->open_lock);
+ return 0;
+}
+
+static const struct file_operations jtag_fops = {
+ .owner = THIS_MODULE,
+ .open = jtag_open,
+ .release = jtag_release,
+ .llseek = noop_llseek,
+ .unlocked_ioctl = jtag_ioctl,
+};
+
+struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops)
+{
+ struct jtag *jtag;
+
+ if (!ops)
+ return NULL;
+
+ if (!ops->idle || !ops->mode_set || !ops->status_get || !ops->xfer)
+ return NULL;
+
+ jtag = kzalloc(sizeof(*jtag) + priv_size, GFP_KERNEL);
+ if (!jtag)
+ return NULL;
+
+ jtag->ops = ops;
+ return jtag;
+}
+EXPORT_SYMBOL_GPL(jtag_alloc);
+
+void jtag_free(struct jtag *jtag)
+{
+ kfree(jtag);
+}
+EXPORT_SYMBOL_GPL(jtag_free);
+
+int jtag_register(struct jtag *jtag)
+{
+ char *name;
+ int err;
+ int id;
+
+ id = ida_simple_get(&jtag_ida, 0, 0, GFP_KERNEL);
+ if (id < 0)
+ return id;
+
+ jtag->id = id;
+ jtag->opened = false;
+
+ name = kzalloc(MAX_JTAG_NAME_LEN, GFP_KERNEL);
+ if (!name) {
+ err = -ENOMEM;
+ goto err_jtag_alloc;
+ }
+
+ err = snprintf(name, MAX_JTAG_NAME_LEN, "jtag%d", id);
+ if (err < 0)
+ goto err_jtag_name;
+
+ mutex_init(&jtag->open_lock);
+ jtag->miscdev.fops = &jtag_fops;
+ jtag->miscdev.minor = MISC_DYNAMIC_MINOR;
+ jtag->miscdev.name = name;
+
+ err = misc_register(&jtag->miscdev);
+ if (err) {
+ dev_err(jtag->dev, "Unable to register device\n");
+ goto err_jtag_name;
+ }
+ return 0;
+
+err_jtag_name:
+ kfree(name);
+err_jtag_alloc:
+ ida_simple_remove(&jtag_ida, id);
+ return err;
+}
+EXPORT_SYMBOL_GPL(jtag_register);
+
+void jtag_unregister(struct jtag *jtag)
+{
+ misc_deregister(&jtag->miscdev);
+ kfree(jtag->miscdev.name);
+ ida_simple_remove(&jtag_ida, jtag->id);
+}
+EXPORT_SYMBOL_GPL(jtag_unregister);
+
+static void __exit jtag_exit(void)
+{
+ ida_destroy(&jtag_ida);
+}
+
+module_exit(jtag_exit);
+
+MODULE_AUTHOR("Oleksandr Shamray <oleksandrs@mellanox.com>");
+MODULE_DESCRIPTION("Generic jtag support");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/jtag.h b/include/linux/jtag.h
new file mode 100644
index 0000000..80a887f
--- /dev/null
+++ b/include/linux/jtag.h
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+// include/linux/jtag.h - JTAG class driver
+//
+// Copyright (c) 2018 Mellanox Technologies. All rights reserved.
+// Copyright (c) 2018 Oleksandr Shamray <oleksandrs@mellanox.com>
+
+#ifndef __JTAG_H
+#define __JTAG_H
+
+#include <uapi/linux/jtag.h>
+
+#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
+
+#define JTAG_MAX_XFER_DATA_LEN 65535
+
+struct jtag;
+/**
+ * struct jtag_ops - callbacks for jtag control functions:
+ *
+ * @freq_get: get frequency function. Filled by device driver
+ * @freq_set: set frequency function. Filled by device driver
+ * @status_get: set status function. Filled by device driver
+ * @idle: set JTAG to idle state function. Filled by device driver
+ * @xfer: send JTAG xfer function. Filled by device driver
+ */
+struct jtag_ops {
+ int (*freq_get)(struct jtag *jtag, u32 *freq);
+ int (*freq_set)(struct jtag *jtag, u32 freq);
+ int (*status_get)(struct jtag *jtag, u32 *state);
+ int (*idle)(struct jtag *jtag, struct jtag_run_test_idle *idle);
+ int (*xfer)(struct jtag *jtag, struct jtag_xfer *xfer, u8 *xfer_data);
+ int (*mode_set)(struct jtag *jtag, u32 mode_mask);
+};
+
+void *jtag_priv(struct jtag *jtag);
+int jtag_register(struct jtag *jtag);
+void jtag_unregister(struct jtag *jtag);
+struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops);
+void jtag_free(struct jtag *jtag);
+
+#endif /* __JTAG_H */
diff --git a/include/uapi/linux/jtag.h b/include/uapi/linux/jtag.h
new file mode 100644
index 0000000..ecd02a0
--- /dev/null
+++ b/include/uapi/linux/jtag.h
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0
+// include/uapi/linux/jtag.h - JTAG class driver uapi
+//
+// Copyright (c) 2018 Mellanox Technologies. All rights reserved.
+// Copyright (c) 2018 Oleksandr Shamray <oleksandrs@mellanox.com>
+
+#ifndef __UAPI_LINUX_JTAG_H
+#define __UAPI_LINUX_JTAG_H
+
+#include <linux/types.h>
+/*
+ * JTAG_XFER_HW_MODE: JTAG hardware mode. Used to set HW drived or bitbang
+ * mode. This is bitmask param of ioctl JTAG_SIOCMODE command
+ */
+#define JTAG_XFER_HW_MODE 1
+
+/**
+ * enum jtag_endstate:
+ *
+ * @JTAG_STATE_IDLE: JTAG state machine IDLE state
+ * @JTAG_STATE_PAUSEIR: JTAG state machine PAUSE_IR state
+ * @JTAG_STATE_PAUSEDR: JTAG state machine PAUSE_DR state
+ */
+enum jtag_endstate {
+ JTAG_STATE_IDLE,
+ JTAG_STATE_PAUSEIR,
+ JTAG_STATE_PAUSEDR,
+};
+
+/**
+ * enum jtag_xfer_type:
+ *
+ * @JTAG_SIR_XFER: SIR transfer
+ * @JTAG_SDR_XFER: SDR transfer
+ */
+enum jtag_xfer_type {
+ JTAG_SIR_XFER,
+ JTAG_SDR_XFER,
+};
+
+/**
+ * enum jtag_xfer_direction:
+ *
+ * @JTAG_READ_XFER: read transfer
+ * @JTAG_WRITE_XFER: write transfer
+ */
+enum jtag_xfer_direction {
+ JTAG_READ_XFER,
+ JTAG_WRITE_XFER,
+};
+
+/**
+ * struct jtag_run_test_idle - forces JTAG state machine to
+ * RUN_TEST/IDLE state
+ *
+ * @reset: 0 - run IDLE/PAUSE from current state
+ * 1 - go through TEST_LOGIC/RESET state before IDLE/PAUSE
+ * @end: completion flag
+ * @tck: clock counter
+ *
+ * Structure represents interface to JTAG device for jtag idle
+ * execution.
+ */
+struct jtag_run_test_idle {
+ __u8 reset;
+ __u8 endstate;
+ __u8 tck;
+};
+
+/**
+ * struct jtag_xfer - jtag xfer:
+ *
+ * @type: transfer type
+ * @direction: xfer direction
+ * @length: xfer bits len
+ * @tdio : xfer data array
+ * @endir: xfer end state
+ *
+ * Structure represents interface to JTAG device for jtag sdr xfer
+ * execution.
+ */
+struct jtag_xfer {
+ __u8 type;
+ __u8 direction;
+ __u8 endstate;
+ __u8 padding;
+ __u32 length;
+ __u64 tdio;
+};
+
+/* ioctl interface */
+#define __JTAG_IOCTL_MAGIC 0xb2
+
+#define JTAG_IOCRUNTEST _IOW(__JTAG_IOCTL_MAGIC, 0,\
+ struct jtag_run_test_idle)
+#define JTAG_SIOCFREQ _IOW(__JTAG_IOCTL_MAGIC, 1, unsigned int)
+#define JTAG_GIOCFREQ _IOR(__JTAG_IOCTL_MAGIC, 2, unsigned int)
+#define JTAG_IOCXFER _IOWR(__JTAG_IOCTL_MAGIC, 3, struct jtag_xfer)
+#define JTAG_GIOCSTATUS _IOWR(__JTAG_IOCTL_MAGIC, 4, enum jtag_endstate)
+#define JTAG_SIOCMODE _IOW(__JTAG_IOCTL_MAGIC, 5, unsigned int)
+
+#define JTAG_FIRST_MINOR 0
+#define JTAG_MAX_DEVICES 32
+
+#endif /* __UAPI_LINUX_JTAG_H */
--
1.7.1
^ permalink raw reply related
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