* Re: [PATCH 4/7] tty: serial: lantiq: Always use readl()/writel()
From: Wu, Songjun @ 2018-06-14 7:05 UTC (permalink / raw)
To: Andy Shevchenko
Cc: hua.ma, yixin.zhu, chuanhua.lei, Linux MIPS Mailing List,
qi-ming.wu, linux-clk, open list:SERIAL DRIVERS, devicetree,
James Hogan, Jiri Slaby, Linux Kernel Mailing List,
Greg Kroah-Hartman, Ralf Baechle
In-Reply-To: <CAHp75Vc_EVgfj1MpuA_hKHgy6SiQdU7JQDdtVHfOT1sZ_K-nRQ@mail.gmail.com>
On 6/12/2018 4:13 PM, Andy Shevchenko wrote:
> On Tue, Jun 12, 2018 at 8:40 AM, Songjun Wu <songjun.wu@linux.intel.com> wrote:
>> Previous implementation uses platform-dependent functions
>> ltq_w32()/ltq_r32() to access registers. Those functions are not
>> available for other SoC which uses the same IP.
>> Change to OS provided readl()/writel() and readb()/writeb(), so
>> that different SoCs can use the same driver.
> This patch consists 2 or even 3 ones:
> - whitespace shuffling (indentation fixes, blank lines), I dunno if
> it's needed at all
> - some new registers / bits
> - actual switch to readl() / writel()
>
> Please, split.
It will be split to four patches, coding style, new registers,
readl()/writel() and asc_update_bits.
>> +#define asc_w32_mask(clear, set, reg) \
>> + ({ typeof(reg) reg_ = (reg); \
>> + writel((readl(reg_) & ~(clear)) | (set), reg_); })
> This would be better as a static inline helper, and name is completely
> misleading, it doesn't mask the register bits, it _updates_ them.
It will be changed to asc_update_bits.
^ permalink raw reply
* [PATCH v3 2/7] serdev: add dev_pm_domain_attach|detach()
From: sean.wang @ 2018-06-14 7:14 UTC (permalink / raw)
To: robh+dt, mark.rutland, marcel, johan.hedberg
Cc: devicetree, Ulf Hansson, Rob Herring, Greg Kroah-Hartman,
Sean Wang, linux-kernel, linux-bluetooth, linux-mediatek,
linux-serial, Jiri Slaby, linux-arm-kernel
In-Reply-To: <cover.1528960095.git.sean.wang@mediatek.com>
From: Sean Wang <sean.wang@mediatek.com>
In order to open up the required power gate before any operation can be
effectively performed over the serial bus between CPU and serdev, it's
clearly essential to add common attach functions for PM domains to serdev
at the probe phase.
Similarly, the relevant dettach function for the PM domains should be
properly and reversely added at the remove phase.
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-serial@vger.kernel.org
---
drivers/tty/serdev/core.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index df93b72..c93d8ee 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_domain.h>
#include <linux/serdev.h>
#include <linux/slab.h>
@@ -330,8 +331,16 @@ EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
static int serdev_drv_probe(struct device *dev)
{
const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
+ int ret;
+
+ ret = dev_pm_domain_attach(dev, true);
+ if (ret != -EPROBE_DEFER) {
+ ret = sdrv->probe(to_serdev_device(dev));
+ if (ret)
+ dev_pm_domain_detach(dev, true);
+ }
- return sdrv->probe(to_serdev_device(dev));
+ return ret;
}
static int serdev_drv_remove(struct device *dev)
@@ -339,6 +348,9 @@ static int serdev_drv_remove(struct device *dev)
const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
if (sdrv->remove)
sdrv->remove(to_serdev_device(dev));
+
+ dev_pm_domain_detach(dev, true);
+
return 0;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v7 3/6] mfd: at91-usart: added mfd driver for usart
From: Ludovic Desroches @ 2018-06-14 7:58 UTC (permalink / raw)
To: Radu Pirea
Cc: broonie, nicolas.ferre, alexandre.belloni, lee.jones,
richard.genoud, robh+dt, mark.rutland, gregkh, devicetree,
linux-kernel, linux-spi, linux-serial, linux-arm-kernel
In-Reply-To: <20180613163621.23995-4-radu.pirea@microchip.com>
On Wed, Jun 13, 2018 at 07:36:18PM +0300, Radu Pirea wrote:
> This mfd driver is just a wrapper over atmel_serial driver and
> spi-at91-usart driver. Selection of one of the drivers is based on a
> property from device tree. If the property is not specified, the default
> driver is atmel_serial.
>
> Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> ---
> drivers/mfd/Kconfig | 9 ++++++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/at91-usart.c | 69 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 79 insertions(+)
> create mode 100644 drivers/mfd/at91-usart.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b860eb5aa194..a886672b960d 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -99,6 +99,15 @@ config MFD_AAT2870_CORE
> additional drivers must be enabled in order to use the
> functionality of the device.
>
> +config MFD_AT91_USART
> + tristate "AT91 USART Driver"
> + select MFD_CORE
> + help
> + Select this to get support for AT91 USART IP. This is a wrapper
> + over at91-usart-serial driver and usart-spi-driver. Only one function
> + can be used at a time. The choice is done at boot time by the probe
> + function of this MFD driver according to a device tree property.
> +
> config MFD_ATMEL_FLEXCOM
> tristate "Atmel Flexcom (Flexible Serial Communication Unit)"
> select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index d9d2cf0d32ef..db1332aa96db 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -185,6 +185,7 @@ obj-$(CONFIG_MFD_SPMI_PMIC) += qcom-spmi-pmic.o
> obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
> obj-$(CONFIG_MFD_TPS65090) += tps65090.o
> obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o
> +obj-$(CONFIG_MFD_AT91_USART) += at91-usart.o
> obj-$(CONFIG_MFD_ATMEL_FLEXCOM) += atmel-flexcom.o
> obj-$(CONFIG_MFD_ATMEL_HLCDC) += atmel-hlcdc.o
> obj-$(CONFIG_MFD_ATMEL_SMC) += atmel-smc.o
> diff --git a/drivers/mfd/at91-usart.c b/drivers/mfd/at91-usart.c
> new file mode 100644
> index 000000000000..3014ce532644
> --- /dev/null
> +++ b/drivers/mfd/at91-usart.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for AT91 USART
> + *
> + * Copyright (C) 2018 Microchip Technology
> + *
> + * Author: Radu Pirea <radu.pirea@microchip.com>
> + *
> + */
> +
> +#include <dt-bindings/mfd/at91-usart.h>
> +
> +#include <linux/module.h>
> +#include <linux/mfd/core.h>
> +#include <linux/property.h>
> +
> +static struct mfd_cell at91_usart_spi_subdev = {
> + .name = "at91_usart_spi",
> + .of_compatible = "microchip,at91sam9g45-usart-spi",
> + };
> +
> +static struct mfd_cell at91_usart_serial_subdev = {
> + .name = "atmel_usart_serial",
> + .of_compatible = "atmel,at91rm9200-usart-serial",
> + };
> +
> +static int at91_usart_mode_probe(struct platform_device *pdev)
> +{
> + struct mfd_cell cell;
> + u32 opmode = AT91_USART_MODE_SERIAL;
> +
> + device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> +
> + switch (opmode) {
> + case AT91_USART_MODE_SPI:
> + cell = at91_usart_spi_subdev;
> + break;
> + case AT91_USART_MODE_SERIAL:
> + cell = at91_usart_serial_subdev;
> + break;
> + default:
> + break;
If there is an invalid opmode from the DT, you will pass a non initialized cell
to mfd_add_device().
Regards
Ludovic
> + }
> +
> + return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO, &cell, 1,
> + NULL, 0, NULL);
> +}
> +
> +static const struct of_device_id at91_usart_mode_of_match[] = {
> + { .compatible = "atmel,at91rm9200-usart" },
> + { .compatible = "atmel,at91sam9260-usart" },
> + { /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, at91_flexcom_of_match);
> +
> +static struct platform_driver at91_usart_mfd = {
> + .probe = at91_usart_mode_probe,
> + .driver = {
> + .name = "at91_usart_mode",
> + .of_match_table = at91_usart_mode_of_match,
> + },
> +};
> +
> +module_platform_driver(at91_usart_mfd);
> +
> +MODULE_AUTHOR("Radu Pirea <radu.pirea@microchip.com>");
> +MODULE_DESCRIPTION("AT91 USART MFD driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/7] MIPS: intel: Add initial support for Intel MIPS SoCs
From: Hua Ma @ 2018-06-14 8:01 UTC (permalink / raw)
To: Rob Herring, Songjun Wu
Cc: yixin.zhu, chuanhua.lei, linux-mips, qi-ming.wu, linux-clk,
linux-serial, devicetree, James Hogan, linux-kernel, Ralf Baechle,
Mark Rutland
In-Reply-To: <20180612223153.GB2197@rob-hp-laptop>
On 6/13/2018 6:31 AM, Rob Herring wrote:
> On Tue, Jun 12, 2018 at 01:40:30PM +0800, Songjun Wu wrote:
>> From: Hua Ma <hua.ma@linux.intel.com>
>>
>> Add initial support for Intel MIPS interAptiv SoCs made by Intel.
>> This series will add support for the GRX500 family.
>>
>> The series allows booting a minimal system using a initramfs.
>>
>> Signed-off-by: Hua ma <hua.ma@linux.intel.com>
>> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
>> ---
>>
>> arch/mips/Kbuild.platforms | 1 +
>> arch/mips/Kconfig | 36 ++++
>> arch/mips/boot/dts/Makefile | 1 +
>> arch/mips/boot/dts/intel-mips/Makefile | 3 +
>> arch/mips/boot/dts/intel-mips/easy350_anywan.dts | 20 +++
>> arch/mips/boot/dts/intel-mips/xrx500.dtsi | 196 +++++++++++++++++++++
> Please split dts files to separate patch.
Thanks,
it will be split into separate patches: one for dts, one for mips codes
and one for the document.
>> diff --git a/arch/mips/boot/dts/intel-mips/easy350_anywan.dts b/arch/mips/boot/dts/intel-mips/easy350_anywan.dts
>> new file mode 100644
>> index 000000000000..40177f6cee1e
>> --- /dev/null
>> +++ b/arch/mips/boot/dts/intel-mips/easy350_anywan.dts
>> @@ -0,0 +1,20 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/dts-v1/;
>> +
>> +#include <dt-bindings/interrupt-controller/mips-gic.h>
>> +#include <dt-bindings/clock/intel,grx500-clk.h>
>> +
>> +#include "xrx500.dtsi"
>> +
>> +/ {
>> + model = "EASY350 ANYWAN (GRX350) Main model";
> A board should have a board specific compatible, too.
The board compatible will be added.
>
>> + chosen {
>> + bootargs = "earlycon=lantiq,0x16600000 clk_ignore_unused";
>> + stdout-path = "serial0";
>> + };
>> +
>> + memory@0 {
> memory@20000000
The memory address will be changed to @20000000.
>
>> + device_type = "memory";
>> + reg = <0x20000000 0x0e000000>;
>> + };
>> +};
>> diff --git a/arch/mips/boot/dts/intel-mips/xrx500.dtsi b/arch/mips/boot/dts/intel-mips/xrx500.dtsi
>> new file mode 100644
>> index 000000000000..04a068d6d96b
>> --- /dev/null
>> +++ b/arch/mips/boot/dts/intel-mips/xrx500.dtsi
>> @@ -0,0 +1,196 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +/ {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + compatible = "intel,xrx500";
> This needs to be documented.
The compatible will be updated in the document.
^ permalink raw reply
* Re: [PATCH v7 3/6] mfd: at91-usart: added mfd driver for usart
From: Radu Pirea @ 2018-06-14 8:22 UTC (permalink / raw)
To: broonie, nicolas.ferre, alexandre.belloni, lee.jones,
richard.genoud, robh+dt, mark.rutland, gregkh, devicetree,
linux-kernel, linux-spi, linux-serial, linux-arm-kernel
In-Reply-To: <20180614075849.dfiqwcu6rtzgpzq7@rfolt0960.corp.atmel.com>
On 06/14/2018 10:58 AM, Ludovic Desroches wrote:
> On Wed, Jun 13, 2018 at 07:36:18PM +0300, Radu Pirea wrote:
>> This mfd driver is just a wrapper over atmel_serial driver and
>> spi-at91-usart driver. Selection of one of the drivers is based on a
>> property from device tree. If the property is not specified, the default
>> driver is atmel_serial.
>>
>> Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
>> ---
>> drivers/mfd/Kconfig | 9 ++++++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/at91-usart.c | 69 ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 79 insertions(+)
>> create mode 100644 drivers/mfd/at91-usart.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index b860eb5aa194..a886672b960d 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -99,6 +99,15 @@ config MFD_AAT2870_CORE
>> additional drivers must be enabled in order to use the
>> functionality of the device.
>>
>> +config MFD_AT91_USART
>> + tristate "AT91 USART Driver"
>> + select MFD_CORE
>> + help
>> + Select this to get support for AT91 USART IP. This is a wrapper
>> + over at91-usart-serial driver and usart-spi-driver. Only one function
>> + can be used at a time. The choice is done at boot time by the probe
>> + function of this MFD driver according to a device tree property.
>> +
>> config MFD_ATMEL_FLEXCOM
>> tristate "Atmel Flexcom (Flexible Serial Communication Unit)"
>> select MFD_CORE
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index d9d2cf0d32ef..db1332aa96db 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -185,6 +185,7 @@ obj-$(CONFIG_MFD_SPMI_PMIC) += qcom-spmi-pmic.o
>> obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
>> obj-$(CONFIG_MFD_TPS65090) += tps65090.o
>> obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o
>> +obj-$(CONFIG_MFD_AT91_USART) += at91-usart.o
>> obj-$(CONFIG_MFD_ATMEL_FLEXCOM) += atmel-flexcom.o
>> obj-$(CONFIG_MFD_ATMEL_HLCDC) += atmel-hlcdc.o
>> obj-$(CONFIG_MFD_ATMEL_SMC) += atmel-smc.o
>> diff --git a/drivers/mfd/at91-usart.c b/drivers/mfd/at91-usart.c
>> new file mode 100644
>> index 000000000000..3014ce532644
>> --- /dev/null
>> +++ b/drivers/mfd/at91-usart.c
>> @@ -0,0 +1,69 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for AT91 USART
>> + *
>> + * Copyright (C) 2018 Microchip Technology
>> + *
>> + * Author: Radu Pirea <radu.pirea@microchip.com>
>> + *
>> + */
>> +
>> +#include <dt-bindings/mfd/at91-usart.h>
>> +
>> +#include <linux/module.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/property.h>
>> +
>> +static struct mfd_cell at91_usart_spi_subdev = {
>> + .name = "at91_usart_spi",
>> + .of_compatible = "microchip,at91sam9g45-usart-spi",
>> + };
>> +
>> +static struct mfd_cell at91_usart_serial_subdev = {
>> + .name = "atmel_usart_serial",
>> + .of_compatible = "atmel,at91rm9200-usart-serial",
>> + };
>> +
>> +static int at91_usart_mode_probe(struct platform_device *pdev)
>> +{
>> + struct mfd_cell cell;
>> + u32 opmode = AT91_USART_MODE_SERIAL;
>> +
>> + device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
>> +
>> + switch (opmode) {
>> + case AT91_USART_MODE_SPI:
>> + cell = at91_usart_spi_subdev;
>> + break;
>> + case AT91_USART_MODE_SERIAL:
>> + cell = at91_usart_serial_subdev;
>> + break;
>> + default:
>> + break;
>
> If there is an invalid opmode from the DT, you will pass a non initialized cell
> to mfd_add_device().
>
> Regards
>
> Ludovic
Hi Ludovic,
Tnx. That's true. How is better to do if atmel,usart-mode has an invalid
value? To initialize cell with at91_usart_serial_subdev or to print an
error message and return -EINVAL?
Regards.
Radu Pirea
>
>> + }
>> +
>> + return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO, &cell, 1,
>> + NULL, 0, NULL);
>> +}
>> +
>> +static const struct of_device_id at91_usart_mode_of_match[] = {
>> + { .compatible = "atmel,at91rm9200-usart" },
>> + { .compatible = "atmel,at91sam9260-usart" },
>> + { /* sentinel */ }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, at91_flexcom_of_match);
>> +
>> +static struct platform_driver at91_usart_mfd = {
>> + .probe = at91_usart_mode_probe,
>> + .driver = {
>> + .name = "at91_usart_mode",
>> + .of_match_table = at91_usart_mode_of_match,
>> + },
>> +};
>> +
>> +module_platform_driver(at91_usart_mfd);
>> +
>> +MODULE_AUTHOR("Radu Pirea <radu.pirea@microchip.com>");
>> +MODULE_DESCRIPTION("AT91 USART MFD driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.17.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 3/6] mfd: at91-usart: added mfd driver for usart
From: Alexandre Belloni @ 2018-06-14 8:32 UTC (permalink / raw)
To: Radu Pirea
Cc: broonie, nicolas.ferre, lee.jones, richard.genoud, robh+dt,
mark.rutland, gregkh, devicetree, linux-kernel, linux-spi,
linux-serial, linux-arm-kernel
In-Reply-To: <1086330e-40e4-8d9b-50aa-1ae9bf48aa22@microchip.com>
On 14/06/2018 11:22:25+0300, Radu Pirea wrote:
> On 06/14/2018 10:58 AM, Ludovic Desroches wrote:
> > > +static int at91_usart_mode_probe(struct platform_device *pdev)
> > > +{
> > > + struct mfd_cell cell;
> > > + u32 opmode = AT91_USART_MODE_SERIAL;
> > > +
> > > + device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> > > +
> > > + switch (opmode) {
> > > + case AT91_USART_MODE_SPI:
> > > + cell = at91_usart_spi_subdev;
> > > + break;
> > > + case AT91_USART_MODE_SERIAL:
> > > + cell = at91_usart_serial_subdev;
> > > + break;
> > > + default:
> > > + break;
> >
> > If there is an invalid opmode from the DT, you will pass a non initialized cell
> > to mfd_add_device().
> >
> > Regards
> >
> > Ludovic
>
> Hi Ludovic,
>
> Tnx. That's true. How is better to do if atmel,usart-mode has an invalid
> value? To initialize cell with at91_usart_serial_subdev or to print an error
> message and return -EINVAL?
>
Returning an error is probably the correct choice because it means the
DT has an invalid value (something was done and is wrong).
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH 2/7] clk: intel: Add clock driver for GRX500 SoC
From: yixin zhu @ 2018-06-14 8:40 UTC (permalink / raw)
To: Rob Herring, Songjun Wu
Cc: hua.ma, chuanhua.lei, linux-mips, qi-ming.wu, linux-clk,
linux-serial, devicetree, Michael Turquette, Stephen Boyd,
linux-kernel, Mark Rutland
In-Reply-To: <20180612223725.GC2197@rob-hp-laptop>
On 6/13/2018 6:37 AM, Rob Herring wrote:
> On Tue, Jun 12, 2018 at 01:40:29PM +0800, Songjun Wu wrote:
>> From: Yixin Zhu <yixin.zhu@linux.intel.com>
>>
>> PLL of GRX500 provide clock to DDR, CPU, and peripherals as show below
>>
>> +---------+
>> |--->| LCPLL3 0|--PCIe clk-->
>> XO | +---------+
>> +-----------|
>> | +---------+
>> | | 3|--PAE clk-->
>> |--->| PLL0B 2|--GSWIP clk-->
>> | | 1|--DDR clk-->DDR PHY clk-->
>> | | 0|--CPU1 clk--+ +-----+
>> | +---------+ |--->0 |
>> | | MUX |--CPU clk-->
>> | +---------+ |--->1 |
>> | | 0|--CPU0 clk--+ +-----+
>> |--->| PLLOA 1|--SSX4 clk-->
>> | 2|--NGI clk-->
>> | 3|--CBM clk-->
>> +---------+
>>
>> VCO of all PLLs of GRX500 is not supposed to be reprogrammed.
>> DDR PHY clock is created to show correct clock rate in software
>> point of view.
>> CPU clock of 1Ghz from PLL0B otherwise from PLL0A.
>> Signed-off-by: Yixin Zhu <yixin.zhu@linux.intel.com>
>>
>> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
>
> Need a blank line before the SoB's and not one in the middle.
Thanks.
Blank line in the middle of sign-off will be removed.
A new blank line will be added before SoB's.
>
>> ---
>>
>> .../devicetree/bindings/clock/intel,grx500-clk.txt | 46 ++
>
> Please split bindings to separate patch.
>
>> drivers/clk/Kconfig | 1 +
>> drivers/clk/Makefile | 1 +
>> drivers/clk/intel/Kconfig | 21 +
>> drivers/clk/intel/Makefile | 7 +
>> drivers/clk/intel/clk-cgu-api.c | 676 +++++++++++++++++++++
>> drivers/clk/intel/clk-cgu-api.h | 120 ++++
>> drivers/clk/intel/clk-grx500.c | 236 +++++++
>> include/dt-bindings/clock/intel,grx500-clk.h | 61 ++
>> 9 files changed, 1169 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/intel,grx500-clk.txt
>> create mode 100644 drivers/clk/intel/Kconfig
>> create mode 100644 drivers/clk/intel/Makefile
>> create mode 100644 drivers/clk/intel/clk-cgu-api.c
>> create mode 100644 drivers/clk/intel/clk-cgu-api.h
>> create mode 100644 drivers/clk/intel/clk-grx500.c
>> create mode 100644 include/dt-bindings/clock/intel,grx500-clk.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/intel,grx500-clk.txt b/Documentation/devicetree/bindings/clock/intel,grx500-clk.txt
>> new file mode 100644
>> index 000000000000..dd761d900dc9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/intel,grx500-clk.txt
>> @@ -0,0 +1,46 @@
>> +Device Tree Clock bindings for GRX500 PLL controller.
>> +
>> +This binding uses the common clock binding:
>> + Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +This GRX500 PLL controller provides the 5 main clock domain of the SoC: CPU/DDR, XBAR,
>> +Voice, WLAN, PCIe and gate clocks for HW modules.
>> +
>> +Required properties for osc clock node
>> +- compatible: Should be intel,grx500-xxx-clk
>
> These would need to be enumerated with all possible values. However, see
> below.
All possible values will be listed.
>
>> +- reg: offset address of the controller memory area.
>> +- clocks: phandle of the external reference clock
>> +- #clock-cells: can be one or zero.
>> +- clock-output-names: Names of the output clocks.
>> +
>> +Example:
>> + pll0aclk: pll0aclk {
>> + #clock-cells = <1>;
>> + compatible = "intel,grx500-pll0a-clk";
>> + clocks = <&pll0a>;
>> + reg = <0x8>;
>> + clock-output-names = "cbmclk", "ngiclk", "ssx4clk", "cpu0clk";
>> + };
>> +
>> + cpuclk: cpuclk {
>> + #clock-cells = <0>;
>> + compatible = "intel,grx500-cpu-clk";
>> + clocks = <&pll0aclk CPU0_CLK>, <&pll0bclk CPU1_CLK>;
>> + reg = <0x8>;
>> + clock-output-names = "cpu";
>> + };
>> +
>> +Required properties for gate node:
>> +- compatible: Should be intel,grx500-gatex-clk
>> +- reg: offset address of the controller memory area.
>> +- #clock-cells: Should be <1>
>> +- clock-output-names: Names of the output clocks.
>> +
>> +Example:
>> + clkgate0: clkgate0 {
>> + #clock-cells = <1>;
>> + compatible = "intel,grx500-gate0-clk";
>> + reg = <0x114>;
>> + clock-output-names = "gate_xbar0", "gate_xbar1", "gate_xbar2",
>> + "gate_xbar3", "gate_xbar6", "gate_xbar7";
>> + };
>
> We generally don't do a clock node per clock or few clocks but rather 1
> clock node per clock controller block. See any recent clock bindings.
>
> Rob
Do you mean only one example is needed per clock controller block?
cpuclk is not needed in the document?
^ permalink raw reply
* Re: [PATCH v4 0/7] add virt-dma support for imx-sdma
From: Lucas Stach @ 2018-06-14 8:53 UTC (permalink / raw)
To: Robin Gong, vkoul, s.hauer, dan.j.williams, gregkh, jslaby
Cc: dmaengine, linux-imx, linux-kernel, linux-serial,
linux-arm-kernel
In-Reply-To: <1528984982-5074-1-git-send-email-yibin.gong@nxp.com>
Hi Robin,
I just gave this series a spin and it seems there is even more locking
fun, see the lockdep output below. After taking a short look it seems
this is caused by using the wrong spinlock variants in
sdma_int_handler(), those should also use the _irqsave ones. When
fixing this you might want to reconsider patch 7/7, as it's probably
not needed at all.
Regards,
Lucas
[ 20.479401] ========================================================
[ 20.485769] WARNING: possible irq lock inversion dependency detected
[ 20.492140] 4.17.0+ #1538 Not tainted
[ 20.495814] --------------------------------------------------------
[ 20.502181] systemd/1 just changed the state of lock:
[ 20.507247] c0abdafc (&(&substream->self_group.lock)->rlock){-...}, at: snd_pcm_stream_lock+0x54/0x60
[ 20.516523] but this lock took another, HARDIRQ-unsafe lock in the past:
[ 20.523234] (fs_reclaim){+.+.}
[ 20.523253]
[ 20.523253]
[ 20.523253] and interrupts could create inverse lock ordering between them.
[ 20.523253]
[ 20.537804]
[ 20.537804] other info that might help us debug this:
[ 20.544344] Possible interrupt unsafe locking scenario:
[ 20.544344]
[ 20.551144] CPU0 CPU1
[ 20.555686] ---- ----
[ 20.560224] lock(fs_reclaim);
[ 20.563386] local_irq_disable();
[ 20.569315] lock(&(&substream->self_group.lock)->rlock);
[ 20.577337] lock(fs_reclaim);
[ 20.583014] <Interrupt>
[ 20.585643] lock(&(&substream->self_group.lock)->rlock);
[ 20.591322]
[ 20.591322] *** DEADLOCK ***
[ 20.591322]
[ 20.597260] 1 lock held by systemd/1:
[ 20.607806] #0: ab23d11c (snd_pcm_link_rwlock){.-..}, at: snd_pcm_stream_lock+0x4c/0x60
[ 20.615951]
[ 20.615951] the shortest dependencies between 2nd lock and 1st lock:
[ 20.623799] -> (fs_reclaim){+.+.} ops: 248474 {
[ 20.628456] HARDIRQ-ON-W at:
[ 20.631716] lock_acquire+0x260/0x29c
[ 20.637223] fs_reclaim_acquire+0x48/0x58
[ 20.643075] kmem_cache_alloc_trace+0x3c/0x364
[ 20.649366] alloc_worker.constprop.15+0x28/0x64
[ 20.655824] init_rescuer.part.5+0x20/0xa4
[ 20.661764] workqueue_init+0x124/0x1f8
[ 20.667446] kernel_init_freeable+0x60/0x550
[ 20.673561] kernel_init+0x18/0x120
[ 20.678890] ret_from_fork+0x14/0x20
[ 20.684299] (null)
[ 20.688408] SOFTIRQ-ON-W at:
[ 20.691659] lock_acquire+0x260/0x29c
[ 20.697158] fs_reclaim_acquire+0x48/0x58
[ 20.703006] kmem_cache_alloc_trace+0x3c/0x364
[ 20.709288] alloc_worker.constprop.15+0x28/0x64
[ 20.709301] init_rescuer.part.5+0x20/0xa4
[ 20.720717] workqueue_init+0x124/0x1f8
[ 20.720729] kernel_init_freeable+0x60/0x550
[ 20.720738] kernel_init+0x18/0x120
[ 20.720746] ret_from_fork+0x14/0x20
[ 20.720751] (null)
[ 20.720756] INITIAL USE at:
[ 20.720770] lock_acquire+0x260/0x29c
[ 20.720782] fs_reclaim_acquire+0x48/0x58
[ 20.774374] kmem_cache_alloc_trace+0x3c/0x364
[ 20.780574] alloc_worker.constprop.15+0x28/0x64
[ 20.786945] init_rescuer.part.5+0x20/0xa4
[ 20.792794] workqueue_init+0x124/0x1f8
[ 20.798384] kernel_init_freeable+0x60/0x550
[ 20.804406] kernel_init+0x18/0x120
[ 20.809648] ret_from_fork+0x14/0x20
[ 20.814971] (null)
[ 20.818992] }
[ 20.820768] ... key at: [<80e22074>] __fs_reclaim_map+0x0/0x10
[ 20.827220] ... acquired at:
[ 20.830289] fs_reclaim_acquire+0x48/0x58
[ 20.834487] kmem_cache_alloc_trace+0x3c/0x364
[ 20.839123] sdma_transfer_init+0x44/0x130
[ 20.843409] sdma_prep_dma_cyclic+0x78/0x21c
[ 20.847869] snd_dmaengine_pcm_trigger+0xdc/0x184
[ 20.852764] soc_pcm_trigger+0x164/0x190
[ 20.856876] snd_pcm_do_start+0x34/0x40
[ 20.860902] snd_pcm_action_single+0x48/0x74
[ 20.865360] snd_pcm_action+0x34/0xfc
[ 20.869213] snd_pcm_ioctl+0x910/0x10ec
[ 20.873241] vfs_ioctl+0x30/0x44
[ 20.876657] do_vfs_ioctl+0xac/0x974
[ 20.880421] ksys_ioctl+0x48/0x64
[ 20.883923] sys_ioctl+0x18/0x1c
[ 20.887340] ret_fast_syscall+0x0/0x28
[ 20.891277] 0x7289bcbc
[ 20.893909]
[ 20.895410] -> (&(&substream->self_group.lock)->rlock){-...} ops: 59 {
[ 20.901977] IN-HARDIRQ-W at:
[ 20.905143] lock_acquire+0x260/0x29c
[ 20.910473] _raw_spin_lock+0x48/0x58
[ 20.915801] snd_pcm_stream_lock+0x54/0x60
[ 20.921561] _snd_pcm_stream_lock_irqsave+0x40/0x48
[ 20.928107] snd_pcm_period_elapsed+0x2c/0xa4
[ 20.934127] dmaengine_pcm_dma_complete+0x54/0x58
[ 20.940498] sdma_int_handler+0x1dc/0x2a8
[ 20.946179] __handle_irq_event_percpu+0x1fc/0x498
[ 20.952635] handle_irq_event_percpu+0x38/0x8c
[ 20.958742] handle_irq_event+0x48/0x6c
[ 20.964242] handle_fasteoi_irq+0xc4/0x138
[ 20.970006] generic_handle_irq+0x28/0x38
[ 20.975681] __handle_domain_irq+0xb0/0xc4
[ 20.981443] gic_handle_irq+0x68/0xa0
[ 20.986769] __irq_svc+0x70/0xb0
[ 20.991662] _raw_spin_unlock_irq+0x38/0x6c
[ 20.997511] task_work_run+0x90/0xb8
[ 21.002751] do_work_pending+0xc8/0xd0
[ 21.008164] slow_work_pending+0xc/0x20
[ 21.013661] 0x76c77e86
[ 21.017768] INITIAL USE at:
[ 21.020844] lock_acquire+0x260/0x29c
[ 21.026086] _raw_spin_lock+0x48/0x58
[ 21.031328] snd_pcm_stream_lock+0x54/0x60
[ 21.037002] snd_pcm_stream_lock_irq+0x38/0x3c
[ 21.043023] snd_pcm_sync_ptr+0x214/0x260
[ 21.048609] snd_pcm_ioctl+0xbe0/0x10ec
[ 21.054027] vfs_ioctl+0x30/0x44
[ 21.058832] do_vfs_ioctl+0xac/0x974
[ 21.063984] ksys_ioctl+0x48/0x64
[ 21.068875] sys_ioctl+0x18/0x1c
[ 21.073679] ret_fast_syscall+0x0/0x28
[ 21.079004] 0x7e9026dc
[ 21.083023] }
[ 21.084710] ... key at: [<8162a6e4>] __key.31798+0x0/0x8
[ 21.090552] ... acquired at:
[ 21.093537] mark_lock+0x3a4/0x69c
[ 21.097128] __lock_acquire+0x420/0x16d4
[ 21.101239] lock_acquire+0x260/0x29c
[ 21.105091] _raw_spin_lock+0x48/0x58
[ 21.108940] snd_pcm_stream_lock+0x54/0x60
[ 21.113226] _snd_pcm_stream_lock_irqsave+0x40/0x48
[ 21.118296] snd_pcm_period_elapsed+0x2c/0xa4
[ 21.122841] dmaengine_pcm_dma_complete+0x54/0x58
[ 21.127735] sdma_int_handler+0x1dc/0x2a8
[ 21.131937] __handle_irq_event_percpu+0x1fc/0x498
[ 21.136915] handle_irq_event_percpu+0x38/0x8c
[ 21.141547] handle_irq_event+0x48/0x6c
[ 21.145570] handle_fasteoi_irq+0xc4/0x138
[ 21.149854] generic_handle_irq+0x28/0x38
[ 21.154052] __handle_domain_irq+0xb0/0xc4
[ 21.158335] gic_handle_irq+0x68/0xa0
[ 21.162184] __irq_svc+0x70/0xb0
[ 21.165601] _raw_spin_unlock_irq+0x38/0x6c
[ 21.169973] task_work_run+0x90/0xb8
[ 21.173735] do_work_pending+0xc8/0xd0
[ 21.177670] slow_work_pending+0xc/0x20
[ 21.181691] 0x76c77e86
[ 21.184320]
[ 21.185821]
[ 21.185821] stack backtrace:
[ 21.190198] CPU: 0 PID: 1 Comm: systemd Not tainted 4.17.0+ #1538
[ 21.196303] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[ 21.202841] Backtrace:
[ 21.205314] [<8010e318>] (dump_backtrace) from [<8010e604>] (show_stack+0x20/0x24)
[ 21.212900] r7:80e9f3d0 r6:00000000 r5:60070193 r4:80e9f3d0
[ 21.218581] [<8010e5e4>] (show_stack) from [<8099b660>] (dump_stack+0xa4/0xd8)
[ 21.225825] [<8099b5bc>] (dump_stack) from [<8017b52c>] (print_irq_inversion_bug+0x15c/0x1fc)
[ 21.234368] r9:814da818 r8:00000001 r7:ee926c00 r6:00000000 r5:ee915bb0 r4:814da818
[ 21.242133] [<8017b3d0>] (print_irq_inversion_bug) from [<8017b6dc>] (check_usage_forwards+0x110/0x13c)
[ 21.251544] r9:00000002 r8:80bfd3e2 r7:ee926c00 r6:ee927148 r5:80e08488 r4:00000001
[ 21.259306] [<8017b5cc>] (check_usage_forwards) from [<8017c2a4>] (mark_lock+0x3a4/0x69c)
[ 21.267500] r9:ee926c00 r8:80a03cd8 r7:00000101 r6:00000002 r5:00000000 r4:ee927148
[ 21.275263] [<8017bf00>] (mark_lock) from [<8017cf68>] (__lock_acquire+0x420/0x16d4)
[ 21.283023] r10:ee927148 r9:ed4620e4 r8:ee926c00 r7:00000000 r6:00000001 r5:00000001
[ 21.290863] r4:00000490
[ 21.293416] [<8017cb48>] (__lock_acquire) from [<8017ed58>] (lock_acquire+0x260/0x29c)
[ 21.301350] r10:00000001 r9:80e084a4 r8:00000000 r7:00000000 r6:00000000 r5:ed4620e4
[ 21.309189] r4:00000000
[ 21.311746] [<8017eaf8>] (lock_acquire) from [<809b74f0>] (_raw_spin_lock+0x48/0x58)
[ 21.319506] r10:ee0a4714 r9:ed457100 r8:ee0a46c8 r7:ee0a4714 r6:ee0a4010 r5:807847b0
[ 21.327346] r4:ed4620d4
[ 21.329902] [<809b74a8>] (_raw_spin_lock) from [<807847b0>] (snd_pcm_stream_lock+0x54/0x60)
[ 21.338265] r5:ed462000 r4:ed462000
[ 21.341863] [<8078475c>] (snd_pcm_stream_lock) from [<80784838>] (_snd_pcm_stream_lock_irqsave+0x40/0x48)
[ 21.351440] r5:ed462000 r4:60070193
[ 21.355042] [<807847f8>] (_snd_pcm_stream_lock_irqsave) from [<8078b044>] (snd_pcm_period_elapsed+0x2c/0xa4)
[ 21.364881] r5:ee3ef000 r4:ed462000
[ 21.368478] [<8078b018>] (snd_pcm_period_elapsed) from [<8078d7b4>] (dmaengine_pcm_dma_complete+0x54/0x58)
[ 21.378148] r7:ee0a4714 r6:ee0a4010 r5:00000007 r4:ee0a46bc
[ 21.383827] [<8078d760>] (dmaengine_pcm_dma_complete) from [<80504c0c>] (sdma_int_handler+0x1dc/0x2a8)
[ 21.393157] [<80504a30>] (sdma_int_handler) from [<8018cd28>] (__handle_irq_event_percpu+0x1fc/0x498)
[ 21.402393] r10:00000000 r9:eeafd400 r8:80e084a4 r7:00000038 r6:00000038 r5:80ea3c12
[ 21.410233] r4:ee2b5d40
[ 21.412787] [<8018cb2c>] (__handle_irq_event_percpu) from [<8018cffc>] (handle_irq_event_percpu+0x38/0x8c)
[ 21.422457] r10:00000000 r9:ee914000 r8:ee81d400 r7:00000038 r6:eeafd400 r5:eeafd464
[ 21.430296] r4:80e08488
[ 21.432852] [<8018cfc4>] (handle_irq_event_percpu) from [<8018d098>] (handle_irq_event+0x48/0x6c)
[ 21.441736] r6:eeafd464 r5:eeafd464 r4:eeafd400
[ 21.446374] [<8018d050>] (handle_irq_event) from [<8019146c>] (handle_fasteoi_irq+0xc4/0x138)
[ 21.454912] r7:00000038 r6:eeafd464 r5:80e10a60 r4:eeafd400
[ 21.460589] [<801913a8>] (handle_fasteoi_irq) from [<8018bd9c>] (generic_handle_irq+0x28/0x38)
[ 21.469214] r7:00000038 r6:80d92ae4 r5:00000000 r4:00000000
[ 21.474893] [<8018bd74>] (generic_handle_irq) from [<8018c48c>] (__handle_domain_irq+0xb0/0xc4)
[ 21.483611] [<8018c3dc>] (__handle_domain_irq) from [<80102330>] (gic_handle_irq+0x68/0xa0)
[ 21.491978] r9:ee914000 r8:f4001100 r7:80e5c6bc r6:ee915f00 r5:80e08c3c r4:f4000100
[ 21.499738] [<801022c8>] (gic_handle_irq) from [<801019f0>] (__irq_svc+0x70/0xb0)
[ 21.507233] Exception stack(0xee915f00 to 0xee915f48)
[ 21.512303] 5f00: 00000001 00000004 00000000 ee926c00 ee9270a8 ee926c00 ecc45e00 ee9270a8
[ 21.520498] 5f20: 80ec23b0 ee914000 00000000 ee915f64 ee915f20 ee915f50 8017c7c0 809b78ac
[ 21.528687] 5f40: 20070013 ffffffff
[ 21.532193] r9:ee914000 r8:80ec23b0 r7:ee915f34 r6:ffffffff r5:20070013 r4:809b78ac
[ 21.539959] [<809b7874>] (_raw_spin_unlock_irq) from [<8014e98c>] (task_work_run+0x90/0xb8)
[ 21.548321] r5:ee926c00 r4:ecc45e00
[ 21.551913] [<8014e8fc>] (task_work_run) from [<8010da3c>] (do_work_pending+0xc8/0xd0)
[ 21.559848] r9:ee914000 r8:801011c4 r7:ee915fb0 r6:ffffe000 r5:00000004 r4:801011c4
[ 21.567608] [<8010d974>] (do_work_pending) from [<80101034>] (slow_work_pending+0xc/0x20)
[ 21.575797] Exception stack(0xee915fb0 to 0xee915ff8)
[ 21.580864] 5fa0: 00000000 020c34e8 46059f00 00000000
[ 21.589059] 5fc0: 00000002 76f133a4 020df680 00000006 76e6e168 00000035 7ef81778 00000035
[ 21.597253] 5fe0: 00000006 7ef816a0 76c75d67 76c77e86 20070030 00000039
[ 21.603883] r7:00000006 r6:020df680 r5:76f133a4 r4:00000002
Am Donnerstag, den 14.06.2018, 22:02 +0800 schrieb Robin Gong:
> The legacy sdma driver has below limitations or drawbacks:
> 1. Hardcode the max BDs number as "PAGE_SIZE / sizeof(*)", and alloc
> one page size for one channel regardless of only few BDs needed
> most time. But in few cases, the max PAGE_SIZE maybe not enough.
> 2. One SDMA channel can't stop immediatley once channel disabled which
> means SDMA interrupt may come in after this channel terminated.There
> are some patches for this corner case such as commit "2746e2c389f9",
> but not cover non-cyclic.
>
> The common virt-dma overcomes the above limitations. It can alloc bd
> dynamically and free bd once this tx transfer done. No memory wasted or
> maximum limititation here, only depends on how many memory can be requested
> from kernel. For No.2, such issue can be workaround by checking if there
> is available descript("sdmac->desc") now once the unwanted interrupt
> coming. At last the common virt-dma is easier for sdma driver maintain.
>
> Change from v3:
> 1. add two uart patches which impacted by this patchset.
> 2. unlock 'vc.lock' before cyclic dma callback and lock again after
> it because some driver such as uart will call dmaengine_tx_status
> which will acquire 'vc.lock' again and dead lock comes out.
> 3. remove 'Revert commit' stuff since that patch is not wrong and
> combine two patch into one patch as Sascha's comment.
>
> Change from v2:
> 1. include Sascha's patch to make the main patch easier to review.
> Thanks Sacha.
> 2. remove useless 'desc'/'chan' in struct sdma_channe.
>
> Change from v1:
> 1. split v1 patch into 5 patches.
> 2. remove some unnecessary condition check.
> 3. remove unnecessary 'pending' list.
>
> Robin Gong (6):
> tty: serial: imx: correct dma cookie status
> dmaengine: imx-sdma: add virt-dma support
> dmaengine: imx-sdma: remove useless 'lock' and 'enabled' in 'struct
> sdma_channel'
> dmaengine: imx-sdma: remove the maximum limitation for bd numbers
> dmaengine: imx-sdma: add sdma_transfer_init to decrease code overlap
> tty: serial: imx: split all dma setup operations out of 'port.lock'
> protector
>
> Sascha Hauer (1):
> dmaengine: imx-sdma: factor out a struct sdma_desc from struct
> sdma_channel
>
> drivers/dma/Kconfig | 1 +
> drivers/dma/imx-sdma.c | 394 +++++++++++++++++++++++++++--------------------
> drivers/tty/serial/imx.c | 99 ++++++------
> 3 files changed, 282 insertions(+), 212 deletions(-)
>
^ permalink raw reply
* Re: [PATCH v3 2/7] serdev: add dev_pm_domain_attach|detach()
From: Ulf Hansson @ 2018-06-14 8:58 UTC (permalink / raw)
To: sean.wang
Cc: Rob Herring, Mark Rutland, Marcel Holtmann, Johan Hedberg,
devicetree, linux-bluetooth, Linux ARM, linux-mediatek,
Linux Kernel Mailing List, Rob Herring, Greg Kroah-Hartman,
Jiri Slaby, linux-serial
In-Reply-To: <e338118bb6b9f560560887eb974eb250a6c89984.1528960095.git.sean.wang@mediatek.com>
On Thu, 14 Jun 2018 at 09:14, <sean.wang@mediatek.com> wrote:
>
> From: Sean Wang <sean.wang@mediatek.com>
>
> In order to open up the required power gate before any operation can be
> effectively performed over the serial bus between CPU and serdev, it's
> clearly essential to add common attach functions for PM domains to serdev
> at the probe phase.
>
> Similarly, the relevant dettach function for the PM domains should be
> properly and reversely added at the remove phase.
>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: linux-serial@vger.kernel.org
> ---
> drivers/tty/serdev/core.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index df93b72..c93d8ee 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_domain.h>
> #include <linux/serdev.h>
> #include <linux/slab.h>
>
> @@ -330,8 +331,16 @@ EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
> static int serdev_drv_probe(struct device *dev)
> {
> const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
> + int ret;
> +
> + ret = dev_pm_domain_attach(dev, true);
> + if (ret != -EPROBE_DEFER) {
>From 4.18 rc1 via commit 919b7308fcc4, dev_pm_domain_attach() will
return better error codes.
I suggest to change the above error path to:
if (ret)
return ret;
Then continue with the probing below.
> + ret = sdrv->probe(to_serdev_device(dev));
> + if (ret)
> + dev_pm_domain_detach(dev, true);
> + }
>
> - return sdrv->probe(to_serdev_device(dev));
> + return ret;
> }
>
> static int serdev_drv_remove(struct device *dev)
> @@ -339,6 +348,9 @@ static int serdev_drv_remove(struct device *dev)
> const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
> if (sdrv->remove)
> sdrv->remove(to_serdev_device(dev));
> +
> + dev_pm_domain_detach(dev, true);
> +
> return 0;
> }
>
> --
> 2.7.4
>
Otherwise, this makes sense to me!
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 3/7] MIPS: intel: Add initial support for Intel MIPS SoCs
From: yixin zhu @ 2018-06-14 9:24 UTC (permalink / raw)
To: James Hogan, Songjun Wu
Cc: hua.ma, chuanhua.lei, linux-mips, qi-ming.wu, linux-clk,
linux-serial, devicetree, linux-kernel, Rob Herring, Ralf Baechle,
Mark Rutland
In-Reply-To: <20180612112346.GA8748@jamesdev>
On 6/12/2018 7:23 PM, James Hogan wrote:
> Hi,
>
> Good to see this patch!
>
> On Tue, Jun 12, 2018 at 01:40:30PM +0800, Songjun Wu wrote:
>> diff --git a/arch/mips/Kbuild.platforms b/arch/mips/Kbuild.platforms
>> index ac7ad54f984f..bcd647060f3e 100644
>> --- a/arch/mips/Kbuild.platforms
>> +++ b/arch/mips/Kbuild.platforms
>> @@ -12,6 +12,7 @@ platforms += cobalt
>> platforms += dec
>> platforms += emma
>> platforms += generic
>> +platforms += intel-mips
>
> What are the main things preventing this from moving to the generic
> platform? Is it mainly the use of EVA (which generic doesn't yet
> support)?
>
Yes. It's mainly because of EVA.
>> diff --git a/arch/mips/include/asm/mach-intel-mips/kernel-entry-init.h b/arch/mips/include/asm/mach-intel-mips/kernel-entry-init.h
>> new file mode 100644
>> index 000000000000..3893855b60c6
>> --- /dev/null
>> +++ b/arch/mips/include/asm/mach-intel-mips/kernel-entry-init.h
> ...
>> + /*
>> + * Get Config.K0 value and use it to program
>> + * the segmentation registers
>
> Please can you describe (maybe with a table) the segment layout in human
> readable terms so the reader doesn't need to decode the SegCtl registers
> to understand where everything is in the virtual address space?
>
Will add detailed EVA mapping description in code comments.
>> diff --git a/arch/mips/boot/dts/intel-mips/Makefile b/arch/mips/boot/dts/intel-mips/Makefile
>> new file mode 100644
>> index 000000000000..b16c0081639c
>> --- /dev/null
>> +++ b/arch/mips/boot/dts/intel-mips/Makefile
>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +dtb-$(CONFIG_DTB_INTEL_MIPS_GRX500) += easy350_anywan.dtb
>> +obj-y += $(patsubst %.dtb, %.dtb.o, $(dtb-y))
>
> This needs updating to obj-$(CONFIG_BUILTIN_DTB) as per commit
> fca3aa166422 ("MIPS: dts: Avoid unneeded built-in.a in DTS dirs") in
> linux-next.
>
>> diff --git a/arch/mips/intel-mips/Makefile b/arch/mips/intel-mips/Makefile
>> new file mode 100644
>> index 000000000000..9f272d06eecd
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/Makefile
>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +obj-$(CONFIG_INTEL_MIPS) += prom.o irq.o time.o
>
> You can use obj-y, since this Makefile is only included if
> CONFIG_INTEL_MIPS=y (i.e. via the platform-$(CONFIG_INTEL_MIPS) below).
>
> Also please split each file onto separate "obj-y += whatever.o" lines.
>
Will use obj-y.
Will split it into per line per .o.
>> diff --git a/arch/mips/intel-mips/Platform b/arch/mips/intel-mips/Platform
>> new file mode 100644
>> index 000000000000..b34750eeaeb0
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/Platform
>> @@ -0,0 +1,11 @@
>> +#
>> +# MIPs SoC platform
>> +#
>> +
>> +platform-$(CONFIG_INTEL_MIPS) += intel-mips/
>
> ^^^ (this is what ensures the Makefile is only included for this
> platform)
>
>> diff --git a/arch/mips/intel-mips/irq.c b/arch/mips/intel-mips/irq.c
>> new file mode 100644
>> index 000000000000..00637a5cdd20
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/irq.c
>> @@ -0,0 +1,36 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2016 Intel Corporation.
>> + */
>> +#include <linux/init.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/of_irq.h>
>> +#include <asm/irq.h>
>> +
>> +#include <asm/irq_cpu.h>
>> +
>> +void __init arch_init_irq(void)
>> +{
>> + struct device_node *intc_node;
>> +
>> + pr_info("EIC is %s\n", cpu_has_veic ? "on" : "off");
>> + pr_info("VINT is %s\n", cpu_has_vint ? "on" : "off");
>> +
>> + intc_node = of_find_compatible_node(NULL, NULL,
>> + "mti,cpu-interrupt-controller");
>> + if (!cpu_has_veic && !intc_node)
>> + mips_cpu_irq_init();
>> +
>> + irqchip_init();
>> +}
>> +
>> +int get_c0_perfcount_int(void)
>> +{
>> + return gic_get_c0_perfcount_int();
>> +}
>> +EXPORT_SYMBOL_GPL(get_c0_perfcount_int);
>> +
>> +unsigned int get_c0_compare_int(void)
>> +{
>> + return gic_get_c0_compare_int();
>> +}
>
> Worth having get_c0_fdc_int() too for the "Fast Debug Channel"?
>
FDC not used in our product.
>> diff --git a/arch/mips/intel-mips/prom.c b/arch/mips/intel-mips/prom.c
>> new file mode 100644
>> index 000000000000..9407858ddc94
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/prom.c
>> @@ -0,0 +1,184 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2014 Lei Chuanhua <Chuanhua.lei@lantiq.com>
>> + * Copyright (C) 2016 Intel Corporation.
>> + */
>> +#include <linux/init.h>
>> +#include <linux/export.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_fdt.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <asm/mips-cps.h>
>> +#include <asm/smp-ops.h>
>> +#include <asm/dma-coherence.h>
>> +#include <asm/prom.h>
>> +
>> +#define IOPORT_RESOURCE_START 0x10000000
>> +#define IOPORT_RESOURCE_END 0xffffffff
>> +#define IOMEM_RESOURCE_START 0x10000000
>> +#define IOMEM_RESOURCE_END 0xffffffff
>
> The _END ones seem to be unused?
>
Will remove unused _END macros.
>> +static void __init prom_init_cmdline(void)
>> +{
>> + int i;
>> + int argc;
>> + char **argv;
>> +
>> + /*
>> + * If u-boot pass parameters, it is ok, however, if without u-boot
>> + * JTAG or other tool has to reset all register value before it goes
>> + * emulation most likely belongs to this category
>> + */
>> + if (fw_arg0 == 0 || fw_arg1 == 0)
>> + return;
>> +
>> + argc = fw_arg0;
>> + argv = (char **)KSEG1ADDR(fw_arg1);
>> +
>> + arcs_cmdline[0] = '\0';
>> +
>> + for (i = 0; i < argc; i++) {
>> + char *p = (char *)KSEG1ADDR(argv[i]);
>> +
>> + if (argv[i] && *p) {
>> + strlcat(arcs_cmdline, p, sizeof(arcs_cmdline));
>> + strlcat(arcs_cmdline, " ", sizeof(arcs_cmdline));
>> + }
>> + }
>
> Please describe the boot register protocol in the commit message and/or
> a comment in this function.
>Will add boot register protocol description in code comment.
> Is it compatible with the UHI boot protocol, such that this could
> potentially be converted to use the generic platform in future?
>
It is compatible with the UHI boot protocol.
>> +}
>> +
>> +static int __init plat_enable_iocoherency(void)
>> +{
>> + int supported = 0;
>> +
>> + if (mips_cps_numiocu(0) != 0) {
>> + /* Nothing special needs to be done to enable coherency */
>> + pr_info("Coherence Manager IOCU detected\n");
>> + /* Second IOCU for MPE or other master access register */
>> + write_gcr_reg0_base(0xa0000000);
>> + write_gcr_reg0_mask(0xf8000000 | CM_GCR_REGn_MASK_CMTGT_IOCU1);
>> + supported = 1;
>> + }
>> +
>> + /* hw_coherentio = supported; */
>> +
>> + return supported;
>> +}
>> +
>> +static void __init plat_setup_iocoherency(void)
>> +{
>> +#ifdef CONFIG_DMA_NONCOHERENT
>> + /*
>> + * Kernel has been configured with software coherency
>> + * but we might choose to turn it off and use hardware
>> + * coherency instead.
>
> That sounds a big risky. Software coherency will I think perform cache
> line invalidation when syncing buffers from device to CPU (see
> __dma_sync_virtual), so that the underlying RAM written by the
> supposedly incoherent DMA is visible. If its coherent afterall then it
> may be sat in a dirty line in the cache, and not have been written back
> to RAM yet before it gets invalidated?
>
The comment in code is not accurate.
HW coherence is not supported and software coherence is always enabled.
Will correct the comments and clean the code.
>> + */
>> + if (plat_enable_iocoherency()) {
>> + if (coherentio == IO_COHERENCE_DISABLED)
>> + pr_info("Hardware DMA cache coherency disabled\n");
>> + else
>> + pr_info("Hardware DMA cache coherency enabled\n");
>> + } else {
>> + if (coherentio == IO_COHERENCE_ENABLED)
>> + pr_info("Hardware DMA cache coherency unsupported, but enabled from command line!\n");
>> + else
>> + pr_info("Software DMA cache coherency enabled\n");
>> + }
>> +#else
>> + if (!plat_enable_iocoherency())
>> + panic("Hardware DMA cache coherency not supported!");
>> +#endif
>> +}
>
>
>> +void __init device_tree_init(void)
>> +{
>> + if (!initial_boot_params)
>> + return;
>
> Redundant check. unflatten_and_copy_device_tree() now handles that and
> emits a message.
>
Will remove the redundant check.
>> +
>> + unflatten_and_copy_device_tree();
>> +}
>> +
>> +#define CPC_BASE_ADDR 0x12310000
>
> Please put this at the top of the file with other #defines.
>
Will move this to the top with other #defines.
>> +
>> +phys_addr_t mips_cpc_default_phys_base(void)
>> +{
>> + return CPC_BASE_ADDR;
>> +}
>
>> diff --git a/arch/mips/intel-mips/time.c b/arch/mips/intel-mips/time.c
>> new file mode 100644
>> index 000000000000..77ad4014fe9d
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/time.c
>> @@ -0,0 +1,56 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2016 Intel Corporation.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clocksource.h>
>> +#include <linux/of.h>
>> +
>> +#include <asm/time.h>
>> +
>> +static inline u32 get_counter_resolution(void)
>> +{
>> + u32 res;
>> +
>> + __asm__ __volatile__(".set push\n"
>
> Preferably each line of asm should end \n\t and the final line doesn't
> need \n or \t. Look at the .s compiler output to see the difference.
>
>> + ".set mips32r2\n"
>> + "rdhwr %0, $3\n"
>
> Hmm, it'd be nice to abstract this in mipsregs.h, but that can always
> wait. You could use MIPS_HWR_CCRES though (i.e. off top of my head
> something like "%0, $%1\n" and have a "i" (MIPS_HWR_CCRES) input.
>
>> + ".set pop\n"
>> + : "=&r" (res)
>
> I don't think you strictly need an early clobber there since there are
> no register inputs, "=r" should do fine.
>
>> + : /* no input */
>> + : "memory");
>
> I don't think that operation clobbers any memory?
>
Will replace the assembly code with fixed value 2 as the chip resolution
is the half of the clock.
> Thanks
> James
>
Thanks
Yixin
^ permalink raw reply
* Re: [PATCH 1/7] MIPS: dts: Add aliases node for lantiq danube serial
From: Arnd Bergmann @ 2018-06-14 10:03 UTC (permalink / raw)
To: Songjun Wu
Cc: hua.ma, yixin.zhu, chuanhua.lei,
open list:RALINK MIPS ARCHITECTURE, qi-ming.wu, linux-clk,
linux-serial, DTML, James Hogan, Linux Kernel Mailing List,
Thomas Gleixner, Philippe Ombredanne, Rob Herring, Kate Stewart,
Greg Kroah-Hartman, Mark Rutland, Ralf Baechle
In-Reply-To: <20180612054034.4969-2-songjun.wu@linux.intel.com>
On Tue, Jun 12, 2018 at 7:40 AM, Songjun Wu <songjun.wu@linux.intel.com> wrote:
> Previous implementation uses a hard-coded register value to check if
> the current serial entity is the console entity.
> Now the lantiq serial driver uses the aliases for the index of the
> serial port.
> The lantiq danube serial dts are updated with aliases to support this.
>
> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
> ---
>
> arch/mips/boot/dts/lantiq/danube.dtsi | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/boot/dts/lantiq/danube.dtsi b/arch/mips/boot/dts/lantiq/danube.dtsi
> index 2dd950181f8a..7a9e15da6bd0 100644
> --- a/arch/mips/boot/dts/lantiq/danube.dtsi
> +++ b/arch/mips/boot/dts/lantiq/danube.dtsi
> @@ -4,6 +4,10 @@
> #size-cells = <1>;
> compatible = "lantiq,xway", "lantiq,danube";
>
> + aliases {
> + serial0 = &asc1;
> + };
> +
You generally want the aliases to be part of the board specific file,
not every board numbers their serial ports in the same way.
Arnd
^ permalink raw reply
* Re: [PATCH 4/7] tty: serial: lantiq: Always use readl()/writel()
From: Arnd Bergmann @ 2018-06-14 10:07 UTC (permalink / raw)
To: Songjun Wu
Cc: hua.ma, yixin.zhu, chuanhua.lei,
open list:RALINK MIPS ARCHITECTURE, qi-ming.wu, linux-clk,
linux-serial, DTML, James Hogan, Jiri Slaby,
Linux Kernel Mailing List, Greg Kroah-Hartman, Ralf Baechle
In-Reply-To: <20180612054034.4969-5-songjun.wu@linux.intel.com>
On Tue, Jun 12, 2018 at 7:40 AM, Songjun Wu <songjun.wu@linux.intel.com> wrote:
> Previous implementation uses platform-dependent functions
> ltq_w32()/ltq_r32() to access registers. Those functions are not
> available for other SoC which uses the same IP.
> Change to OS provided readl()/writel() and readb()/writeb(), so
> that different SoCs can use the same driver.
>
> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
Are there any big-endian machines using this driver? The original definition
of ltq_r32() uses non-byteswapping __raw_readl() etc, which suggests
that the registers might be wired up in a way that matches the CPU
endianess (this is usally a bad idea in hardware design, but nothing
we can influence in the OS).
When you change it to readl(), that will breaks all machines that rely
on the old behavior on big-endian kernels.
Arnd
^ permalink raw reply
* Re: [PATCH v4 0/7] add virt-dma support for imx-sdma
From: Robin Gong @ 2018-06-14 10:09 UTC (permalink / raw)
To: l.stach@pengutronix.de, s.hauer@pengutronix.de, vkoul@kernel.org,
dan.j.williams@intel.com, gregkh@linuxfoundation.org,
jslaby@suse.com
Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dl-linux-imx,
linux-serial@vger.kernel.org
In-Reply-To: <1528966400.21021.26.camel@pengutronix.de>
Hi Lucas,
Could you double check again? Still can reproduce lockdep
warning on UART if change
spin_lock_lockirqsave/spinlock_unlock_irqrestore to
spin_lock/spin_unlock in sdma_int_handler as you said without patch7/7.
Would you please ask below two more questions?
1. Does your uart case pass now with my v4 patchset?
2. Do you make some code change in your local('I just gave this series
a spin')to reproduce your below issue? If yes, could you post your
change?
On 四, 2018-06-14 at 10:53 +0200, Lucas Stach wrote:
> Hi Robin,
>
> I just gave this series a spin and it seems there is even more
> locking
> fun, see the lockdep output below. After taking a short look it seems
> this is caused by using the wrong spinlock variants in
> sdma_int_handler(), those should also use the _irqsave ones. When
> fixing this you might want to reconsider patch 7/7, as it's probably
> not needed at all.
>
> Regards,
> Lucas
>
> [ 20.479401]
> ========================================================
> [ 20.485769] WARNING: possible irq lock inversion dependency
> detected
> [ 20.492140] 4.17.0+ #1538 Not tainted
> [ 20.495814] ------------------------------------------------------
> --
> [ 20.502181] systemd/1 just changed the state of lock:
> [ 20.507247] c0abdafc (&(&substream->self_group.lock)->rlock){-
> ...}, at: snd_pcm_stream_lock+0x54/0x60
> [ 20.516523] but this lock took another, HARDIRQ-unsafe lock in the
> past:
> [ 20.523234] (fs_reclaim){+.+.}
> [ 20.523253]
> [ 20.523253]
> [ 20.523253] and interrupts could create inverse lock ordering
> between them.
> [ 20.523253]
> [ 20.537804]
> [ 20.537804] other info that might help us debug this:
> [ 20.544344] Possible interrupt unsafe locking scenario:
> [ 20.544344]
> [ 20.551144] CPU0 CPU1
> [ 20.555686] ---- ----
> [ 20.560224] lock(fs_reclaim);
> [ 20.563386] local_irq_disable();
> [ 20.569315] lock(&(&substream-
> >self_group.lock)->rlock);
> [ 20.577337] lock(fs_reclaim);
> [ 20.583014] <Interrupt>
> [ 20.585643] lock(&(&substream->self_group.lock)->rlock);
> [ 20.591322]
> [ 20.591322] *** DEADLOCK ***
> [ 20.591322]
> [ 20.597260] 1 lock held by systemd/1:
> [ 20.607806] #0: ab23d11c (snd_pcm_link_rwlock){.-..}, at:
> snd_pcm_stream_lock+0x4c/0x60
> [ 20.615951]
> [ 20.615951] the shortest dependencies between 2nd lock and 1st
> lock:
> [ 20.623799] -> (fs_reclaim){+.+.} ops: 248474 {
> [ 20.628456] HARDIRQ-ON-W at:
> [ 20.631716] lock_acquire+0x260/0x29c
> [ 20.637223] fs_reclaim_acquire+0x48/0x58
> [ 20.643075] kmem_cache_alloc_trace+0x3c/0x36
> 4
> [ 20.649366] alloc_worker.constprop.15+0x28/0
> x64
> [ 20.655824] init_rescuer.part.5+0x20/0xa4
> [ 20.661764] workqueue_init+0x124/0x1f8
> [ 20.667446] kernel_init_freeable+0x60/0x550
> [ 20.673561] kernel_init+0x18/0x120
> [ 20.678890] ret_from_fork+0x14/0x20
> [ 20.684299] (null)
> [ 20.688408] SOFTIRQ-ON-W at:
> [ 20.691659] lock_acquire+0x260/0x29c
> [ 20.697158] fs_reclaim_acquire+0x48/0x58
> [ 20.703006] kmem_cache_alloc_trace+0x3c/0x36
> 4
> [ 20.709288] alloc_worker.constprop.15+0x28/0
> x64
> [ 20.709301] init_rescuer.part.5+0x20/0xa4
> [ 20.720717] workqueue_init+0x124/0x1f8
> [ 20.720729] kernel_init_freeable+0x60/0x550
> [ 20.720738] kernel_init+0x18/0x120
> [ 20.720746] ret_from_fork+0x14/0x20
> [ 20.720751] (null)
> [ 20.720756] INITIAL USE at:
> [ 20.720770] lock_acquire+0x260/0x29c
> [ 20.720782] fs_reclaim_acquire+0x48/0x58
> [ 20.774374] kmem_cache_alloc_trace+0x3c/0x364
> [ 20.780574] alloc_worker.constprop.15+0x28/0x
> 64
> [ 20.786945] init_rescuer.part.5+0x20/0xa4
> [ 20.792794] workqueue_init+0x124/0x1f8
> [ 20.798384] kernel_init_freeable+0x60/0x550
> [ 20.804406] kernel_init+0x18/0x120
> [ 20.809648] ret_from_fork+0x14/0x20
> [ 20.814971] (null)
> [ 20.818992] }
> [ 20.820768] ... key at: [<80e22074>]
> __fs_reclaim_map+0x0/0x10
> [ 20.827220] ... acquired at:
> [ 20.830289] fs_reclaim_acquire+0x48/0x58
> [ 20.834487] kmem_cache_alloc_trace+0x3c/0x364
> [ 20.839123] sdma_transfer_init+0x44/0x130
> [ 20.843409] sdma_prep_dma_cyclic+0x78/0x21c
> [ 20.847869] snd_dmaengine_pcm_trigger+0xdc/0x184
> [ 20.852764] soc_pcm_trigger+0x164/0x190
> [ 20.856876] snd_pcm_do_start+0x34/0x40
> [ 20.860902] snd_pcm_action_single+0x48/0x74
> [ 20.865360] snd_pcm_action+0x34/0xfc
> [ 20.869213] snd_pcm_ioctl+0x910/0x10ec
> [ 20.873241] vfs_ioctl+0x30/0x44
> [ 20.876657] do_vfs_ioctl+0xac/0x974
> [ 20.880421] ksys_ioctl+0x48/0x64
> [ 20.883923] sys_ioctl+0x18/0x1c
> [ 20.887340] ret_fast_syscall+0x0/0x28
> [ 20.891277] 0x7289bcbc
> [ 20.893909]
> [ 20.895410] -> (&(&substream->self_group.lock)->rlock){-...} ops:
> 59 {
> [ 20.901977] IN-HARDIRQ-W at:
> [ 20.905143] lock_acquire+0x260/0x29c
> [ 20.910473] _raw_spin_lock+0x48/0x58
> [ 20.915801] snd_pcm_stream_lock+0x54/0x60
> [ 20.921561] _snd_pcm_stream_lock_irqsave+0x40/
> 0x48
> [ 20.928107] snd_pcm_period_elapsed+0x2c/0xa4
> [ 20.934127] dmaengine_pcm_dma_complete+0x54/0x
> 58
> [ 20.940498] sdma_int_handler+0x1dc/0x2a8
> [ 20.946179] __handle_irq_event_percpu+0x1fc/0x
> 498
> [ 20.952635] handle_irq_event_percpu+0x38/0x8c
> [ 20.958742] handle_irq_event+0x48/0x6c
> [ 20.964242] handle_fasteoi_irq+0xc4/0x138
> [ 20.970006] generic_handle_irq+0x28/0x38
> [ 20.975681] __handle_domain_irq+0xb0/0xc4
> [ 20.981443] gic_handle_irq+0x68/0xa0
> [ 20.986769] __irq_svc+0x70/0xb0
> [ 20.991662] _raw_spin_unlock_irq+0x38/0x6c
> [ 20.997511] task_work_run+0x90/0xb8
> [ 21.002751] do_work_pending+0xc8/0xd0
> [ 21.008164] slow_work_pending+0xc/0x20
> [ 21.013661] 0x76c77e86
> [ 21.017768] INITIAL USE at:
> [ 21.020844] lock_acquire+0x260/0x29c
> [ 21.026086] _raw_spin_lock+0x48/0x58
> [ 21.031328] snd_pcm_stream_lock+0x54/0x60
> [ 21.037002] snd_pcm_stream_lock_irq+0x38/0x3c
> [ 21.043023] snd_pcm_sync_ptr+0x214/0x260
> [ 21.048609] snd_pcm_ioctl+0xbe0/0x10ec
> [ 21.054027] vfs_ioctl+0x30/0x44
> [ 21.058832] do_vfs_ioctl+0xac/0x974
> [ 21.063984] ksys_ioctl+0x48/0x64
> [ 21.068875] sys_ioctl+0x18/0x1c
> [ 21.073679] ret_fast_syscall+0x0/0x28
> [ 21.079004] 0x7e9026dc
> [ 21.083023] }
> [ 21.084710] ... key at: [<8162a6e4>] __key.31798+0x0/0x8
> [ 21.090552] ... acquired at:
> [ 21.093537] mark_lock+0x3a4/0x69c
> [ 21.097128] __lock_acquire+0x420/0x16d4
> [ 21.101239] lock_acquire+0x260/0x29c
> [ 21.105091] _raw_spin_lock+0x48/0x58
> [ 21.108940] snd_pcm_stream_lock+0x54/0x60
> [ 21.113226] _snd_pcm_stream_lock_irqsave+0x40/0x48
> [ 21.118296] snd_pcm_period_elapsed+0x2c/0xa4
> [ 21.122841] dmaengine_pcm_dma_complete+0x54/0x58
> [ 21.127735] sdma_int_handler+0x1dc/0x2a8
> [ 21.131937] __handle_irq_event_percpu+0x1fc/0x498
> [ 21.136915] handle_irq_event_percpu+0x38/0x8c
> [ 21.141547] handle_irq_event+0x48/0x6c
> [ 21.145570] handle_fasteoi_irq+0xc4/0x138
> [ 21.149854] generic_handle_irq+0x28/0x38
> [ 21.154052] __handle_domain_irq+0xb0/0xc4
> [ 21.158335] gic_handle_irq+0x68/0xa0
> [ 21.162184] __irq_svc+0x70/0xb0
> [ 21.165601] _raw_spin_unlock_irq+0x38/0x6c
> [ 21.169973] task_work_run+0x90/0xb8
> [ 21.173735] do_work_pending+0xc8/0xd0
> [ 21.177670] slow_work_pending+0xc/0x20
> [ 21.181691] 0x76c77e86
> [ 21.184320]
> [ 21.185821]
> [ 21.185821] stack backtrace:
> [ 21.190198] CPU: 0 PID: 1 Comm: systemd Not tainted 4.17.0+ #1538
> [ 21.196303] Hardware name: Freescale i.MX6 Quad/DualLite (Device
> Tree)
> [ 21.202841] Backtrace:
> [ 21.205314] [<8010e318>] (dump_backtrace) from [<8010e604>]
> (show_stack+0x20/0x24)
> [ 21.212900] r7:80e9f3d0 r6:00000000 r5:60070193 r4:80e9f3d0
> [ 21.218581] [<8010e5e4>] (show_stack) from [<8099b660>]
> (dump_stack+0xa4/0xd8)
> [ 21.225825] [<8099b5bc>] (dump_stack) from [<8017b52c>]
> (print_irq_inversion_bug+0x15c/0x1fc)
> [ 21.234368] r9:814da818 r8:00000001 r7:ee926c00 r6:00000000
> r5:ee915bb0 r4:814da818
> [ 21.242133] [<8017b3d0>] (print_irq_inversion_bug) from
> [<8017b6dc>] (check_usage_forwards+0x110/0x13c)
> [ 21.251544] r9:00000002 r8:80bfd3e2 r7:ee926c00 r6:ee927148
> r5:80e08488 r4:00000001
> [ 21.259306] [<8017b5cc>] (check_usage_forwards) from [<8017c2a4>]
> (mark_lock+0x3a4/0x69c)
> [ 21.267500] r9:ee926c00 r8:80a03cd8 r7:00000101 r6:00000002
> r5:00000000 r4:ee927148
> [ 21.275263] [<8017bf00>] (mark_lock) from [<8017cf68>]
> (__lock_acquire+0x420/0x16d4)
> [ 21.283023] r10:ee927148 r9:ed4620e4 r8:ee926c00 r7:00000000
> r6:00000001 r5:00000001
> [ 21.290863] r4:00000490
> [ 21.293416] [<8017cb48>] (__lock_acquire) from [<8017ed58>]
> (lock_acquire+0x260/0x29c)
> [ 21.301350] r10:00000001 r9:80e084a4 r8:00000000 r7:00000000
> r6:00000000 r5:ed4620e4
> [ 21.309189] r4:00000000
> [ 21.311746] [<8017eaf8>] (lock_acquire) from [<809b74f0>]
> (_raw_spin_lock+0x48/0x58)
> [ 21.319506] r10:ee0a4714 r9:ed457100 r8:ee0a46c8 r7:ee0a4714
> r6:ee0a4010 r5:807847b0
> [ 21.327346] r4:ed4620d4
> [ 21.329902] [<809b74a8>] (_raw_spin_lock) from [<807847b0>]
> (snd_pcm_stream_lock+0x54/0x60)
> [ 21.338265] r5:ed462000 r4:ed462000
> [ 21.341863] [<8078475c>] (snd_pcm_stream_lock) from [<80784838>]
> (_snd_pcm_stream_lock_irqsave+0x40/0x48)
> [ 21.351440] r5:ed462000 r4:60070193
> [ 21.355042] [<807847f8>] (_snd_pcm_stream_lock_irqsave) from
> [<8078b044>] (snd_pcm_period_elapsed+0x2c/0xa4)
> [ 21.364881] r5:ee3ef000 r4:ed462000
> [ 21.368478] [<8078b018>] (snd_pcm_period_elapsed) from
> [<8078d7b4>] (dmaengine_pcm_dma_complete+0x54/0x58)
> [ 21.378148] r7:ee0a4714 r6:ee0a4010 r5:00000007 r4:ee0a46bc
> [ 21.383827] [<8078d760>] (dmaengine_pcm_dma_complete) from
> [<80504c0c>] (sdma_int_handler+0x1dc/0x2a8)
> [ 21.393157] [<80504a30>] (sdma_int_handler) from [<8018cd28>]
> (__handle_irq_event_percpu+0x1fc/0x498)
> [ 21.402393] r10:00000000 r9:eeafd400 r8:80e084a4 r7:00000038
> r6:00000038 r5:80ea3c12
> [ 21.410233] r4:ee2b5d40
> [ 21.412787] [<8018cb2c>] (__handle_irq_event_percpu) from
> [<8018cffc>] (handle_irq_event_percpu+0x38/0x8c)
> [ 21.422457] r10:00000000 r9:ee914000 r8:ee81d400 r7:00000038
> r6:eeafd400 r5:eeafd464
> [ 21.430296] r4:80e08488
> [ 21.432852] [<8018cfc4>] (handle_irq_event_percpu) from
> [<8018d098>] (handle_irq_event+0x48/0x6c)
> [ 21.441736] r6:eeafd464 r5:eeafd464 r4:eeafd400
> [ 21.446374] [<8018d050>] (handle_irq_event) from [<8019146c>]
> (handle_fasteoi_irq+0xc4/0x138)
> [ 21.454912] r7:00000038 r6:eeafd464 r5:80e10a60 r4:eeafd400
> [ 21.460589] [<801913a8>] (handle_fasteoi_irq) from [<8018bd9c>]
> (generic_handle_irq+0x28/0x38)
> [ 21.469214] r7:00000038 r6:80d92ae4 r5:00000000 r4:00000000
> [ 21.474893] [<8018bd74>] (generic_handle_irq) from [<8018c48c>]
> (__handle_domain_irq+0xb0/0xc4)
> [ 21.483611] [<8018c3dc>] (__handle_domain_irq) from [<80102330>]
> (gic_handle_irq+0x68/0xa0)
> [ 21.491978] r9:ee914000 r8:f4001100 r7:80e5c6bc r6:ee915f00
> r5:80e08c3c r4:f4000100
> [ 21.499738] [<801022c8>] (gic_handle_irq) from [<801019f0>]
> (__irq_svc+0x70/0xb0)
> [ 21.507233] Exception stack(0xee915f00 to 0xee915f48)
> [ 21.512303] 5f00: 00000001 00000004 00000000 ee926c00 ee9270a8
> ee926c00 ecc45e00 ee9270a8
> [ 21.520498] 5f20: 80ec23b0 ee914000 00000000 ee915f64 ee915f20
> ee915f50 8017c7c0 809b78ac
> [ 21.528687] 5f40: 20070013 ffffffff
> [ 21.532193] r9:ee914000 r8:80ec23b0 r7:ee915f34 r6:ffffffff
> r5:20070013 r4:809b78ac
> [ 21.539959] [<809b7874>] (_raw_spin_unlock_irq) from [<8014e98c>]
> (task_work_run+0x90/0xb8)
> [ 21.548321] r5:ee926c00 r4:ecc45e00
> [ 21.551913] [<8014e8fc>] (task_work_run) from [<8010da3c>]
> (do_work_pending+0xc8/0xd0)
> [ 21.559848] r9:ee914000 r8:801011c4 r7:ee915fb0 r6:ffffe000
> r5:00000004 r4:801011c4
> [ 21.567608] [<8010d974>] (do_work_pending) from [<80101034>]
> (slow_work_pending+0xc/0x20)
> [ 21.575797] Exception stack(0xee915fb0 to 0xee915ff8)
> [ 21.580864] 5fa0: 00000000
> 020c34e8 46059f00 00000000
> [ 21.589059] 5fc0: 00000002 76f133a4 020df680 00000006 76e6e168
> 00000035 7ef81778 00000035
> [ 21.597253] 5fe0: 00000006 7ef816a0 76c75d67 76c77e86 20070030
> 00000039
> [ 21.603883] r7:00000006 r6:020df680 r5:76f133a4 r4:00000002
>
> Am Donnerstag, den 14.06.2018, 22:02 +0800 schrieb Robin Gong:
> >
> > The legacy sdma driver has below limitations or drawbacks:
> > 1. Hardcode the max BDs number as "PAGE_SIZE / sizeof(*)", and
> > alloc
> > one page size for one channel regardless of only few BDs
> > needed
> > most time. But in few cases, the max PAGE_SIZE maybe not
> > enough.
> > 2. One SDMA channel can't stop immediatley once channel disabled
> > which
> > means SDMA interrupt may come in after this channel
> > terminated.There
> > are some patches for this corner case such as commit
> > "2746e2c389f9",
> > but not cover non-cyclic.
> >
> > The common virt-dma overcomes the above limitations. It can alloc
> > bd
> > dynamically and free bd once this tx transfer done. No memory
> > wasted or
> > maximum limititation here, only depends on how many memory can be
> > requested
> > from kernel. For No.2, such issue can be workaround by checking if
> > there
> > is available descript("sdmac->desc") now once the unwanted
> > interrupt
> > coming. At last the common virt-dma is easier for sdma driver
> > maintain.
> >
> > Change from v3:
> > 1. add two uart patches which impacted by this patchset.
> > 2. unlock 'vc.lock' before cyclic dma callback and lock again
> > after
> > it because some driver such as uart will call
> > dmaengine_tx_status
> > which will acquire 'vc.lock' again and dead lock comes out.
> > 3. remove 'Revert commit' stuff since that patch is not wrong and
> > combine two patch into one patch as Sascha's comment.
> >
> > Change from v2:
> > 1. include Sascha's patch to make the main patch easier to
> > review.
> > Thanks Sacha.
> > 2. remove useless 'desc'/'chan' in struct sdma_channe.
> >
> > Change from v1:
> > 1. split v1 patch into 5 patches.
> > 2. remove some unnecessary condition check.
> > 3. remove unnecessary 'pending' list.
> >
> > Robin Gong (6):
> > tty: serial: imx: correct dma cookie status
> > dmaengine: imx-sdma: add virt-dma support
> > dmaengine: imx-sdma: remove useless 'lock' and 'enabled' in
> > 'struct
> > sdma_channel'
> > dmaengine: imx-sdma: remove the maximum limitation for bd numbers
> > dmaengine: imx-sdma: add sdma_transfer_init to decrease code
> > overlap
> > tty: serial: imx: split all dma setup operations out of
> > 'port.lock'
> > protector
> >
> > Sascha Hauer (1):
> > dmaengine: imx-sdma: factor out a struct sdma_desc from struct
> > sdma_channel
> >
> > drivers/dma/Kconfig | 1 +
> > drivers/dma/imx-sdma.c | 394 +++++++++++++++++++++++++++------
> > --------------
> > drivers/tty/serial/imx.c | 99 ++++++------
> > 3 files changed, 282 insertions(+), 212 deletions(-)
> >
^ permalink raw reply
* Re: [PATCH v4 0/7] add virt-dma support for imx-sdma
From: Lucas Stach @ 2018-06-14 10:22 UTC (permalink / raw)
To: Robin Gong, s.hauer@pengutronix.de, vkoul@kernel.org,
dan.j.williams@intel.com, gregkh@linuxfoundation.org,
jslaby@suse.com
Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dl-linux-imx,
linux-serial@vger.kernel.org
In-Reply-To: <1528999731.10947.20.camel@nxp.com>
Am Donnerstag, den 14.06.2018, 10:09 +0000 schrieb Robin Gong:
> Hi Lucas,
> Could you double check again? Still can reproduce lockdep
> warning on UART if change
> spin_lock_lockirqsave/spinlock_unlock_irqrestore to
> spin_lock/spin_unlock in sdma_int_handler as you said without
> patch7/7.
Yes, I can reproduce the lockdep issue with _irqsave variants in the
IRQ handler and 7/7 reverted. It fixes the pcm issue though.
> Would you please ask below two more questions?
> 1. Does your uart case pass now with my v4 patchset?
It seems to work, but the change seems to impact the non-DMA serial
somehow. On several boots the system was unable to present a login
prompt, so patch 7/7 seems to break other stuff and it's a pretty
invasive change to the serial driver, which is known to be non-trivial.
I don't have the bandwidth to dig through this patch currently, but I
wonder if it couldn't be simplified with the spinlock stuff in the sdma
driver fixed.
> 2. Do you make some code change in your local('I just gave this
> series
> a spin')to reproduce your below issue? If yes, could you post your
> change?
The lockdep splat below was without any changes to your series.
Regards,
Lucas
> On 四, 2018-06-14 at 10:53 +0200, Lucas Stach wrote:
> > Hi Robin,
> >
> > I just gave this series a spin and it seems there is even more
> > locking
> > fun, see the lockdep output below. After taking a short look it
> > seems
> > this is caused by using the wrong spinlock variants in
> > sdma_int_handler(), those should also use the _irqsave ones. When
> > fixing this you might want to reconsider patch 7/7, as it's
> > probably
> > not needed at all.
> >
> > Regards,
> > Lucas
> >
> > [ 20.479401]
> > ========================================================
> > [ 20.485769] WARNING: possible irq lock inversion dependency
> > detected
> > [ 20.492140] 4.17.0+ #1538 Not tainted
> > [ 20.495814] ----------------------------------------------------
> > --
> > --
> > [ 20.502181] systemd/1 just changed the state of lock:
> > [ 20.507247] c0abdafc (&(&substream->self_group.lock)->rlock){-
> > ...}, at: snd_pcm_stream_lock+0x54/0x60
> > [ 20.516523] but this lock took another, HARDIRQ-unsafe lock in
> > the
> > past:
> > [ 20.523234] (fs_reclaim){+.+.}
> > [ 20.523253]
> > [ 20.523253]
> > [ 20.523253] and interrupts could create inverse lock ordering
> > between them.
> > [ 20.523253]
> > [ 20.537804]
> > [ 20.537804] other info that might help us debug this:
> > [ 20.544344] Possible interrupt unsafe locking scenario:
> > [ 20.544344]
> > [ 20.551144] CPU0 CPU1
> > [ 20.555686] ---- ----
> > [ 20.560224] lock(fs_reclaim);
> > [ 20.563386] local_irq_disable();
> > [ 20.569315] lock(&(&substream-
> > > self_group.lock)->rlock);
> >
> > [ 20.577337] lock(fs_reclaim);
> > [ 20.583014] <Interrupt>
> > [ 20.585643] lock(&(&substream->self_group.lock)->rlock);
> > [ 20.591322]
> > [ 20.591322] *** DEADLOCK ***
> > [ 20.591322]
> > [ 20.597260] 1 lock held by systemd/1:
> > [ 20.607806] #0: ab23d11c (snd_pcm_link_rwlock){.-..}, at:
> > snd_pcm_stream_lock+0x4c/0x60
> > [ 20.615951]
> > [ 20.615951] the shortest dependencies between 2nd lock and 1st
> > lock:
> > [ 20.623799] -> (fs_reclaim){+.+.} ops: 248474 {
> > [ 20.628456] HARDIRQ-ON-W at:
> > [ 20.631716] lock_acquire+0x260/0x29c
> > [ 20.637223] fs_reclaim_acquire+0x48/0x58
> > [ 20.643075] kmem_cache_alloc_trace+0x3c/0x
> > 36
> > 4
> > [ 20.649366] alloc_worker.constprop.15+0x28
> > /0
> > x64
> > [ 20.655824] init_rescuer.part.5+0x20/0xa4
> > [ 20.661764] workqueue_init+0x124/0x1f8
> > [ 20.667446] kernel_init_freeable+0x60/0x55
> > 0
> > [ 20.673561] kernel_init+0x18/0x120
> > [ 20.678890] ret_from_fork+0x14/0x20
> > [ 20.684299] (null)
> > [ 20.688408] SOFTIRQ-ON-W at:
> > [ 20.691659] lock_acquire+0x260/0x29c
> > [ 20.697158] fs_reclaim_acquire+0x48/0x58
> > [ 20.703006] kmem_cache_alloc_trace+0x3c/0x
> > 36
> > 4
> > [ 20.709288] alloc_worker.constprop.15+0x28
> > /0
> > x64
> > [ 20.709301] init_rescuer.part.5+0x20/0xa4
> > [ 20.720717] workqueue_init+0x124/0x1f8
> > [ 20.720729] kernel_init_freeable+0x60/0x55
> > 0
> > [ 20.720738] kernel_init+0x18/0x120
> > [ 20.720746] ret_from_fork+0x14/0x20
> > [ 20.720751] (null)
> > [ 20.720756] INITIAL USE at:
> > [ 20.720770] lock_acquire+0x260/0x29c
> > [ 20.720782] fs_reclaim_acquire+0x48/0x58
> > [ 20.774374] kmem_cache_alloc_trace+0x3c/0x3
> > 64
> > [ 20.780574] alloc_worker.constprop.15+0x28/
> > 0x
> > 64
> > [ 20.786945] init_rescuer.part.5+0x20/0xa4
> > [ 20.792794] workqueue_init+0x124/0x1f8
> > [ 20.798384] kernel_init_freeable+0x60/0x550
> > [ 20.804406] kernel_init+0x18/0x120
> > [ 20.809648] ret_from_fork+0x14/0x20
> > [ 20.814971] (null)
> > [ 20.818992] }
> > [ 20.820768] ... key at: [<80e22074>]
> > __fs_reclaim_map+0x0/0x10
> > [ 20.827220] ... acquired at:
> > [ 20.830289] fs_reclaim_acquire+0x48/0x58
> > [ 20.834487] kmem_cache_alloc_trace+0x3c/0x364
> > [ 20.839123] sdma_transfer_init+0x44/0x130
> > [ 20.843409] sdma_prep_dma_cyclic+0x78/0x21c
> > [ 20.847869] snd_dmaengine_pcm_trigger+0xdc/0x184
> > [ 20.852764] soc_pcm_trigger+0x164/0x190
> > [ 20.856876] snd_pcm_do_start+0x34/0x40
> > [ 20.860902] snd_pcm_action_single+0x48/0x74
> > [ 20.865360] snd_pcm_action+0x34/0xfc
> > [ 20.869213] snd_pcm_ioctl+0x910/0x10ec
> > [ 20.873241] vfs_ioctl+0x30/0x44
> > [ 20.876657] do_vfs_ioctl+0xac/0x974
> > [ 20.880421] ksys_ioctl+0x48/0x64
> > [ 20.883923] sys_ioctl+0x18/0x1c
> > [ 20.887340] ret_fast_syscall+0x0/0x28
> > [ 20.891277] 0x7289bcbc
> > [ 20.893909]
> > [ 20.895410] -> (&(&substream->self_group.lock)->rlock){-...}
> > ops:
> > 59 {
> > [ 20.901977] IN-HARDIRQ-W at:
> > [ 20.905143] lock_acquire+0x260/0x29c
> > [ 20.910473] _raw_spin_lock+0x48/0x58
> > [ 20.915801] snd_pcm_stream_lock+0x54/0x60
> > [ 20.921561] _snd_pcm_stream_lock_irqsave+0x4
> > 0/
> > 0x48
> > [ 20.928107] snd_pcm_period_elapsed+0x2c/0xa4
> > [ 20.934127] dmaengine_pcm_dma_complete+0x54/
> > 0x
> > 58
> > [ 20.940498] sdma_int_handler+0x1dc/0x2a8
> > [ 20.946179] __handle_irq_event_percpu+0x1fc/
> > 0x
> > 498
> > [ 20.952635] handle_irq_event_percpu+0x38/0x8
> > c
> > [ 20.958742] handle_irq_event+0x48/0x6c
> > [ 20.964242] handle_fasteoi_irq+0xc4/0x138
> > [ 20.970006] generic_handle_irq+0x28/0x38
> > [ 20.975681] __handle_domain_irq+0xb0/0xc4
> > [ 20.981443] gic_handle_irq+0x68/0xa0
> > [ 20.986769] __irq_svc+0x70/0xb0
> > [ 20.991662] _raw_spin_unlock_irq+0x38/0x6c
> > [ 20.997511] task_work_run+0x90/0xb8
> > [ 21.002751] do_work_pending+0xc8/0xd0
> > [ 21.008164] slow_work_pending+0xc/0x20
> > [ 21.013661] 0x76c77e86
> > [ 21.017768] INITIAL USE at:
> > [ 21.020844] lock_acquire+0x260/0x29c
> > [ 21.026086] _raw_spin_lock+0x48/0x58
> > [ 21.031328] snd_pcm_stream_lock+0x54/0x60
> > [ 21.037002] snd_pcm_stream_lock_irq+0x38/0x3c
> > [ 21.043023] snd_pcm_sync_ptr+0x214/0x260
> > [ 21.048609] snd_pcm_ioctl+0xbe0/0x10ec
> > [ 21.054027] vfs_ioctl+0x30/0x44
> > [ 21.058832] do_vfs_ioctl+0xac/0x974
> > [ 21.063984] ksys_ioctl+0x48/0x64
> > [ 21.068875] sys_ioctl+0x18/0x1c
> > [ 21.073679] ret_fast_syscall+0x0/0x28
> > [ 21.079004] 0x7e9026dc
> > [ 21.083023] }
> > [ 21.084710] ... key at: [<8162a6e4>] __key.31798+0x0/0x8
> > [ 21.090552] ... acquired at:
> > [ 21.093537] mark_lock+0x3a4/0x69c
> > [ 21.097128] __lock_acquire+0x420/0x16d4
> > [ 21.101239] lock_acquire+0x260/0x29c
> > [ 21.105091] _raw_spin_lock+0x48/0x58
> > [ 21.108940] snd_pcm_stream_lock+0x54/0x60
> > [ 21.113226] _snd_pcm_stream_lock_irqsave+0x40/0x48
> > [ 21.118296] snd_pcm_period_elapsed+0x2c/0xa4
> > [ 21.122841] dmaengine_pcm_dma_complete+0x54/0x58
> > [ 21.127735] sdma_int_handler+0x1dc/0x2a8
> > [ 21.131937] __handle_irq_event_percpu+0x1fc/0x498
> > [ 21.136915] handle_irq_event_percpu+0x38/0x8c
> > [ 21.141547] handle_irq_event+0x48/0x6c
> > [ 21.145570] handle_fasteoi_irq+0xc4/0x138
> > [ 21.149854] generic_handle_irq+0x28/0x38
> > [ 21.154052] __handle_domain_irq+0xb0/0xc4
> > [ 21.158335] gic_handle_irq+0x68/0xa0
> > [ 21.162184] __irq_svc+0x70/0xb0
> > [ 21.165601] _raw_spin_unlock_irq+0x38/0x6c
> > [ 21.169973] task_work_run+0x90/0xb8
> > [ 21.173735] do_work_pending+0xc8/0xd0
> > [ 21.177670] slow_work_pending+0xc/0x20
> > [ 21.181691] 0x76c77e86
> > [ 21.184320]
> > [ 21.185821]
> > [ 21.185821] stack backtrace:
> > [ 21.190198] CPU: 0 PID: 1 Comm: systemd Not tainted 4.17.0+
> > #1538
> > [ 21.196303] Hardware name: Freescale i.MX6 Quad/DualLite (Device
> > Tree)
> > [ 21.202841] Backtrace:
> > [ 21.205314] [<8010e318>] (dump_backtrace) from [<8010e604>]
> > (show_stack+0x20/0x24)
> > [ 21.212900] r7:80e9f3d0 r6:00000000 r5:60070193 r4:80e9f3d0
> > [ 21.218581] [<8010e5e4>] (show_stack) from [<8099b660>]
> > (dump_stack+0xa4/0xd8)
> > [ 21.225825] [<8099b5bc>] (dump_stack) from [<8017b52c>]
> > (print_irq_inversion_bug+0x15c/0x1fc)
> > [ 21.234368] r9:814da818 r8:00000001 r7:ee926c00 r6:00000000
> > r5:ee915bb0 r4:814da818
> > [ 21.242133] [<8017b3d0>] (print_irq_inversion_bug) from
> > [<8017b6dc>] (check_usage_forwards+0x110/0x13c)
> > [ 21.251544] r9:00000002 r8:80bfd3e2 r7:ee926c00 r6:ee927148
> > r5:80e08488 r4:00000001
> > [ 21.259306] [<8017b5cc>] (check_usage_forwards) from
> > [<8017c2a4>]
> > (mark_lock+0x3a4/0x69c)
> > [ 21.267500] r9:ee926c00 r8:80a03cd8 r7:00000101 r6:00000002
> > r5:00000000 r4:ee927148
> > [ 21.275263] [<8017bf00>] (mark_lock) from [<8017cf68>]
> > (__lock_acquire+0x420/0x16d4)
> > [ 21.283023] r10:ee927148 r9:ed4620e4 r8:ee926c00 r7:00000000
> > r6:00000001 r5:00000001
> > [ 21.290863] r4:00000490
> > [ 21.293416] [<8017cb48>] (__lock_acquire) from [<8017ed58>]
> > (lock_acquire+0x260/0x29c)
> > [ 21.301350] r10:00000001 r9:80e084a4 r8:00000000 r7:00000000
> > r6:00000000 r5:ed4620e4
> > [ 21.309189] r4:00000000
> > [ 21.311746] [<8017eaf8>] (lock_acquire) from [<809b74f0>]
> > (_raw_spin_lock+0x48/0x58)
> > [ 21.319506] r10:ee0a4714 r9:ed457100 r8:ee0a46c8 r7:ee0a4714
> > r6:ee0a4010 r5:807847b0
> > [ 21.327346] r4:ed4620d4
> > [ 21.329902] [<809b74a8>] (_raw_spin_lock) from [<807847b0>]
> > (snd_pcm_stream_lock+0x54/0x60)
> > [ 21.338265] r5:ed462000 r4:ed462000
> > [ 21.341863] [<8078475c>] (snd_pcm_stream_lock) from [<80784838>]
> > (_snd_pcm_stream_lock_irqsave+0x40/0x48)
> > [ 21.351440] r5:ed462000 r4:60070193
> > [ 21.355042] [<807847f8>] (_snd_pcm_stream_lock_irqsave) from
> > [<8078b044>] (snd_pcm_period_elapsed+0x2c/0xa4)
> > [ 21.364881] r5:ee3ef000 r4:ed462000
> > [ 21.368478] [<8078b018>] (snd_pcm_period_elapsed) from
> > [<8078d7b4>] (dmaengine_pcm_dma_complete+0x54/0x58)
> > [ 21.378148] r7:ee0a4714 r6:ee0a4010 r5:00000007 r4:ee0a46bc
> > [ 21.383827] [<8078d760>] (dmaengine_pcm_dma_complete) from
> > [<80504c0c>] (sdma_int_handler+0x1dc/0x2a8)
> > [ 21.393157] [<80504a30>] (sdma_int_handler) from [<8018cd28>]
> > (__handle_irq_event_percpu+0x1fc/0x498)
> > [ 21.402393] r10:00000000 r9:eeafd400 r8:80e084a4 r7:00000038
> > r6:00000038 r5:80ea3c12
> > [ 21.410233] r4:ee2b5d40
> > [ 21.412787] [<8018cb2c>] (__handle_irq_event_percpu) from
> > [<8018cffc>] (handle_irq_event_percpu+0x38/0x8c)
> > [ 21.422457] r10:00000000 r9:ee914000 r8:ee81d400 r7:00000038
> > r6:eeafd400 r5:eeafd464
> > [ 21.430296] r4:80e08488
> > [ 21.432852] [<8018cfc4>] (handle_irq_event_percpu) from
> > [<8018d098>] (handle_irq_event+0x48/0x6c)
> > [ 21.441736] r6:eeafd464 r5:eeafd464 r4:eeafd400
> > [ 21.446374] [<8018d050>] (handle_irq_event) from [<8019146c>]
> > (handle_fasteoi_irq+0xc4/0x138)
> > [ 21.454912] r7:00000038 r6:eeafd464 r5:80e10a60 r4:eeafd400
> > [ 21.460589] [<801913a8>] (handle_fasteoi_irq) from [<8018bd9c>]
> > (generic_handle_irq+0x28/0x38)
> > [ 21.469214] r7:00000038 r6:80d92ae4 r5:00000000 r4:00000000
> > [ 21.474893] [<8018bd74>] (generic_handle_irq) from [<8018c48c>]
> > (__handle_domain_irq+0xb0/0xc4)
> > [ 21.483611] [<8018c3dc>] (__handle_domain_irq) from [<80102330>]
> > (gic_handle_irq+0x68/0xa0)
> > [ 21.491978] r9:ee914000 r8:f4001100 r7:80e5c6bc r6:ee915f00
> > r5:80e08c3c r4:f4000100
> > [ 21.499738] [<801022c8>] (gic_handle_irq) from [<801019f0>]
> > (__irq_svc+0x70/0xb0)
> > [ 21.507233] Exception stack(0xee915f00 to 0xee915f48)
> > [ 21.512303] 5f00: 00000001 00000004 00000000 ee926c00 ee9270a8
> > ee926c00 ecc45e00 ee9270a8
> > [ 21.520498] 5f20: 80ec23b0 ee914000 00000000 ee915f64 ee915f20
> > ee915f50 8017c7c0 809b78ac
> > [ 21.528687] 5f40: 20070013 ffffffff
> > [ 21.532193] r9:ee914000 r8:80ec23b0 r7:ee915f34 r6:ffffffff
> > r5:20070013 r4:809b78ac
> > [ 21.539959] [<809b7874>] (_raw_spin_unlock_irq) from
> > [<8014e98c>]
> > (task_work_run+0x90/0xb8)
> > [ 21.548321] r5:ee926c00 r4:ecc45e00
> > [ 21.551913] [<8014e8fc>] (task_work_run) from [<8010da3c>]
> > (do_work_pending+0xc8/0xd0)
> > [ 21.559848] r9:ee914000 r8:801011c4 r7:ee915fb0 r6:ffffe000
> > r5:00000004 r4:801011c4
> > [ 21.567608] [<8010d974>] (do_work_pending) from [<80101034>]
> > (slow_work_pending+0xc/0x20)
> > [ 21.575797] Exception stack(0xee915fb0 to 0xee915ff8)
> > [ 21.580864] 5fa0: 00000000
> > 020c34e8 46059f00 00000000
> > [ 21.589059] 5fc0: 00000002 76f133a4 020df680 00000006 76e6e168
> > 00000035 7ef81778 00000035
> > [ 21.597253] 5fe0: 00000006 7ef816a0 76c75d67 76c77e86 20070030
> > 00000039
> > [ 21.603883] r7:00000006 r6:020df680 r5:76f133a4 r4:00000002
> >
> > Am Donnerstag, den 14.06.2018, 22:02 +0800 schrieb Robin Gong:
> > >
> > > The legacy sdma driver has below limitations or drawbacks:
> > > 1. Hardcode the max BDs number as "PAGE_SIZE / sizeof(*)", and
> > > alloc
> > > one page size for one channel regardless of only few BDs
> > > needed
> > > most time. But in few cases, the max PAGE_SIZE maybe not
> > > enough.
> > > 2. One SDMA channel can't stop immediatley once channel
> > > disabled
> > > which
> > > means SDMA interrupt may come in after this channel
> > > terminated.There
> > > are some patches for this corner case such as commit
> > > "2746e2c389f9",
> > > but not cover non-cyclic.
> > >
> > > The common virt-dma overcomes the above limitations. It can alloc
> > > bd
> > > dynamically and free bd once this tx transfer done. No memory
> > > wasted or
> > > maximum limititation here, only depends on how many memory can be
> > > requested
> > > from kernel. For No.2, such issue can be workaround by checking
> > > if
> > > there
> > > is available descript("sdmac->desc") now once the unwanted
> > > interrupt
> > > coming. At last the common virt-dma is easier for sdma driver
> > > maintain.
> > >
> > > Change from v3:
> > > 1. add two uart patches which impacted by this patchset.
> > > 2. unlock 'vc.lock' before cyclic dma callback and lock again
> > > after
> > > it because some driver such as uart will call
> > > dmaengine_tx_status
> > > which will acquire 'vc.lock' again and dead lock comes out.
> > > 3. remove 'Revert commit' stuff since that patch is not wrong
> > > and
> > > combine two patch into one patch as Sascha's comment.
> > >
> > > Change from v2:
> > > 1. include Sascha's patch to make the main patch easier to
> > > review.
> > > Thanks Sacha.
> > > 2. remove useless 'desc'/'chan' in struct sdma_channe.
> > >
> > > Change from v1:
> > > 1. split v1 patch into 5 patches.
> > > 2. remove some unnecessary condition check.
> > > 3. remove unnecessary 'pending' list.
> > >
> > > Robin Gong (6):
> > > tty: serial: imx: correct dma cookie status
> > > dmaengine: imx-sdma: add virt-dma support
> > > dmaengine: imx-sdma: remove useless 'lock' and 'enabled' in
> > > 'struct
> > > sdma_channel'
> > > dmaengine: imx-sdma: remove the maximum limitation for bd
> > > numbers
> > > dmaengine: imx-sdma: add sdma_transfer_init to decrease code
> > > overlap
> > > tty: serial: imx: split all dma setup operations out of
> > > 'port.lock'
> > > protector
> > >
> > > Sascha Hauer (1):
> > > dmaengine: imx-sdma: factor out a struct sdma_desc from struct
> > > sdma_channel
> > >
> > > drivers/dma/Kconfig | 1 +
> > > drivers/dma/imx-sdma.c | 394 +++++++++++++++++++++++++++------
> > > --------------
> > > drivers/tty/serial/imx.c | 99 ++++++------
> > > 3 files changed, 282 insertions(+), 212 deletions(-)
^ permalink raw reply
* Re: [PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*
From: Johan Hovold @ 2018-06-14 10:48 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: linux-kernel, linux-serial, Rob Herring, Johan Hovold,
Andy Shevchenko
In-Reply-To: <20180611115240.32606-1-ricardo.ribalda@gmail.com>
Hi Ricardo,
On Mon, Jun 11, 2018 at 01:52:16PM +0200, Ricardo Ribalda Delgado wrote:
> There are some situations where it is interesting to load/remove serdev
> devices dynamically, like during board bring-up or when we are
> developing a new driver or for devices that are neither described via
> ACPI or device tree.
First of all, this would be more appropriately labeled an RFC as this is
far from being in a mergeable state. Besides some implementation issues,
we need to determine if this approach is at all viable.
Second, I wonder how you tested this given that this series breaks RX
(and write-wakeup signalling) for serial ports (patch 19/24)?
Third, and as Marcel already suggested, you need to limit your scope
here. Aim at ten patches or so, and use a representative serdev driver
as an example of the kind of driver updates that would be needed. It
also looks like some patches should be squashed (e.g. the ones
introducing new fields and the first one actually using them).
> This implementation allows the creation of serdev devices via sysfs,
> in a similar way as the i2c bus allows sysfs instantiation [1].
Note that this is a legacy interface and not necessarily something that
new interfaces should be modelled after.
Johan
^ permalink raw reply
* Re: [PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*
From: Ricardo Ribalda Delgado @ 2018-06-14 11:06 UTC (permalink / raw)
To: Johan Hovold; +Cc: LKML, open list:SERIAL DRIVERS, Rob Herring, Andy Shevchenko
In-Reply-To: <20180614104820.GC32411@localhost>
Hi Johan
On Thu, Jun 14, 2018 at 12:48 PM Johan Hovold <johan@kernel.org> wrote:
>
> Hi Ricardo,
>
> On Mon, Jun 11, 2018 at 01:52:16PM +0200, Ricardo Ribalda Delgado wrote:
> > There are some situations where it is interesting to load/remove serdev
> > devices dynamically, like during board bring-up or when we are
> > developing a new driver or for devices that are neither described via
> > ACPI or device tree.
>
> First of all, this would be more appropriately labeled an RFC as this is
> far from being in a mergeable state. Besides some implementation issues,
> we need to determine if this approach is at all viable.
>From previous conversations with Greg it seemed that RFC labels was
something to avoid, but I do not mind reseding it as RFC on v3.
http://lists.kernelnewbies.org/pipermail/kernelnewbies/2018-March/018844.html
>
> Second, I wonder how you tested this given that this series breaks RX
> (and write-wakeup signalling) for serial ports (patch 19/24)?
I have a serdev device (led ctrls) that is dynamically enumerated with
something very similar to:
https://github.com/ribalda/linux/commit/415bb3f0076c2b846ebe5409589b8e1e3004f55a
and then I have a script that does adds and removes. For standard
serial port I was not testing the data path, just that the ttyS* was
enumerated fine.
But yesterday I believe that we found the bug that you mentioned and
we have fixed it (check end of mail). I will patch the series and
resend after I get more feedback and also implement what Marcel
suggested.
WIP is at
https://github.com/ribalda/linux/tree/serdev3
Besides this bug, we have used the new driver for over a week now with
no issues.
>
> Third, and as Marcel already suggested, you need to limit your scope
> here. Aim at ten patches or so, and use a representative serdev driver
> as an example of the kind of driver updates that would be needed. It
> also looks like some patches should be squashed (e.g. the ones
> introducing new fields and the first one actually using them).
>
> > This implementation allows the creation of serdev devices via sysfs,
> > in a similar way as the i2c bus allows sysfs instantiation [1].
>
> Note that this is a legacy interface and not necessarily something that
> new interfaces should be modelled after.
I would not consider it legacy, it is the only way to use an i2c
module without writing your own driver and/or modifying ACPI/DT table.
Just google around for i2c linux...
Thanks!
>
> Johan
Author: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Date: Thu Jun 14 11:30:27 2018 +0200
serdev-ttydev: Restore/change ttyport client ops
diff --git a/drivers/tty/serdev/serdev-ttydev.c
b/drivers/tty/serdev/serdev-ttydev.c
index 180035e101dc..b151c9645a1d 100644
--- a/drivers/tty/serdev/serdev-ttydev.c
+++ b/drivers/tty/serdev/serdev-ttydev.c
@@ -16,14 +16,23 @@ static int ttydev_serdev_probe(struct serdev_device *serdev)
struct serdev_controller *ctrl = serdev->ctrl;
struct serport *serport;
struct device *dev;
+ const struct tty_port_client_operations *serdev_ops;
if (!ctrl->is_ttyport)
return -ENODEV;
serport = serdev_controller_get_drvdata(ctrl);
+ serdev_ops = serport->port->client_ops;
+
+ serport->port->client_ops = serport->tty_ops;
dev = tty_register_device_attr(serport->tty_drv, serport->tty_idx,
- &serdev->dev, NULL, NULL);
+ ctrl->dev.parent, NULL, NULL);
+
+ if (IS_ERR(dev))
+ serport->port->client_ops = serdev_ops;
+ else
+ serdev_device_set_drvdata(serdev, (void *)serdev_ops);
return dev ? 0 : PTR_ERR(dev);
}
@@ -35,6 +44,7 @@ static void ttydev_serdev_remove(struct serdev_device *serdev)
serport = serdev_controller_get_drvdata(ctrl);
tty_unregister_device(serport->tty_drv, serport->tty_idx);
+ serport->port->client_ops = serdev_device_get_drvdata(serdev);
}
--
Ricardo Ribalda
^ permalink raw reply related
* Re: [RFC PATCH 5/6] arm64: dts: ti: Add Support for AM654 SoC
From: Tony Lindgren @ 2018-06-14 12:38 UTC (permalink / raw)
To: Nishanth Menon
Cc: Rob Herring, Santosh Shilimkar, Will Deacon, Catalin Marinas,
Greg Kroah-Hartman, Mark Rutland, open list:SERIAL DRIVERS,
linux-kernel@vger.kernel.org, devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Vignesh R, Tero Kristo, Russell King, Sudeep Holla
In-Reply-To: <20180607233853.p7iw7nlxxuyi66og@kahuna>
Hi,
Some comments on the ranges below.
* Nishanth Menon <nm@ti.com> [180607 16:41]:
> + soc0: soc0 {
> + compatible = "simple-bus";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
I suggest you leave out the soc0, that's not real. Just make
the cbass@0 the top level interconnect. It can then provide
ranges to mcu interconnect which can provide ranges to the wkup
interconnect. So just model it after what's in the hardware :)
I found the following ranges based on a quick look at the TRM,
they could be split further if needed for power domains for
genpd for example.
main covers
0x0000000000 - 0x5402000000
main provides at least the following ranges for mcu
0x0028380000 - 0x002bc00000
0x0040080000 - 0x0041c80000
0x0045100000 - 0x0045180000
0x0045600000 - 0x0045640000
0x0045810000 - 0x0045860000
0x0045950000 - 0x0045950400
0x0045a50000 - 0x0045a50400
0x0045b04000 - 0x0045b06400
0x0045d10000 - 0x0045d24000
0x0046000000 - 0x0060000000
0x0400000000 - 0x0800000000
0x4c3c020000 - 0x4c3c030000
0x4c3e000000 - 0x4c3e040000
0x5400000000 - 0x5402000000
then mcu provides the following ranges for wkup
0x0042000000 - 0x0044410020
0x0045000000 - 0x0045030000
0x0045080000 - 0x00450a0000
0x0045808000 - 0x0045808800
0x0045b00000 - 0x0045b02400
This based on looking at "figure 1-1. device top-level
block diagram" and the memory map in TRM.
Regards,
Tony
^ permalink raw reply
* Re: [RFC PATCH 5/6] arm64: dts: ti: Add Support for AM654 SoC
From: Nishanth Menon @ 2018-06-14 13:04 UTC (permalink / raw)
To: Tony Lindgren
Cc: Mark Rutland, devicetree, Sudeep Holla, Vignesh R,
Catalin Marinas, Santosh Shilimkar, Will Deacon,
linux-kernel@vger.kernel.org, Russell King, Tero Kristo,
Rob Herring, open list:SERIAL DRIVERS, Greg Kroah-Hartman,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <20180614123805.GF112168@atomide.com>
On 12:38-20180614, Tony Lindgren wrote:
> Some comments on the ranges below.
Thanks for reviewing in detail (I understand we are in the middle of
merge window, so thanks for the extra effort).
>
> * Nishanth Menon <nm@ti.com> [180607 16:41]:
> > + soc0: soc0 {
> > + compatible = "simple-bus";
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges;
>
> I suggest you leave out the soc0, that's not real. Just make
Why is that so, on a more complex board representation with multiple
SoCs, this is a clear node indicating what the main SoC is in the final
dtb representation.
> the cbass@0 the top level interconnect. It can then provide
> ranges to mcu interconnect which can provide ranges to the wkup
> interconnect. So just model it after what's in the hardware :)
That might blow up things quite a bit - it is like the comment in:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/dra7.dtsi#n141
The trees are pretty deep with many interconnections (example main does
have direct connections to wkup as well, which is simplified off in
top level diagram) - basically it is not a direct one dimensional
relationship. But then, the same is the case for other SoCs..
we can represent NAVSS as a bus segment as well.
>
> I found the following ranges based on a quick look at the TRM,
> they could be split further if needed for power domains for
> genpd for example.
genpd is not really an issue, since it is handled in system firmware and
OSes dont have a visibility into the permitted ranges that the OS is
allowed to use.
I think it is just how accurate a representation is it worth.
>
> main covers
> 0x0000000000 - 0x5402000000
>
> main provides at least the following ranges for mcu
> 0x0028380000 - 0x002bc00000
> 0x0040080000 - 0x0041c80000
> 0x0045100000 - 0x0045180000
> 0x0045600000 - 0x0045640000
> 0x0045810000 - 0x0045860000
> 0x0045950000 - 0x0045950400
> 0x0045a50000 - 0x0045a50400
> 0x0045b04000 - 0x0045b06400
> 0x0045d10000 - 0x0045d24000
> 0x0046000000 - 0x0060000000
> 0x0400000000 - 0x0800000000
> 0x4c3c020000 - 0x4c3c030000
> 0x4c3e000000 - 0x4c3e040000
> 0x5400000000 - 0x5402000000
>
> then mcu provides the following ranges for wkup
> 0x0042000000 - 0x0044410020
> 0x0045000000 - 0x0045030000
> 0x0045080000 - 0x00450a0000
> 0x0045808000 - 0x0045808800
> 0x0045b00000 - 0x0045b02400
>
> This based on looking at "figure 1-1. device top-level
> block diagram" and the memory map in TRM.
Thanks for researching. I did debate something like:
>From A53 view, a more accurate view might be - from an interconnect
view of the world (still simplified - i have ignored the sub bus
segments in the representations below):
msmc {
navss_main {
cbass_main{
cbass_mcu {
navss_mcu {
};
cbass_wkup{
};
};
};
};
};
>From R5 view, the view will be very different ofcourse:
view of the world (still simplified):
cbass_mcu {
navss_mcu {
};
cbass_wkup{
};
cbass_main{
navss_main {
msmc {
};
};
};
};
Do we really need this level of representation, I am not sure I had seen
this detailed a representation in other aarch64 SoCs (I am sure they are
as complex as TI SoCs as well).
I am trying to understand the direction and logic why we'd want to have
such a detailed representation.
A more flatter representation of just the main segments allow for dts
reuse between r5 and a53 as well (but that is minor).
Thoughts?
--
Regards,
Nishanth Menon
^ permalink raw reply
* RE: [PATCH v4 0/7] add virt-dma support for imx-sdma
From: Robin Gong @ 2018-06-14 13:12 UTC (permalink / raw)
To: Lucas Stach, s.hauer@pengutronix.de, vkoul@kernel.org,
dan.j.williams@intel.com, gregkh@linuxfoundation.org,
jslaby@suse.com
Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dl-linux-imx,
linux-serial@vger.kernel.org
In-Reply-To: <1528971740.21021.36.camel@pengutronix.de>
Hi Lucas,
I didn't met your unstable console issue before, anyway, I'll do more test and look into Audio issue.
-----Original Message-----
From: Lucas Stach [mailto:l.stach@pengutronix.de]
Sent: 2018年6月14日 18:22
To: Robin Gong <yibin.gong@nxp.com>; s.hauer@pengutronix.de; vkoul@kernel.org; dan.j.williams@intel.com; gregkh@linuxfoundation.org; jslaby@suse.com
Cc: dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>; linux-serial@vger.kernel.org
Subject: Re: [PATCH v4 0/7] add virt-dma support for imx-sdma
Am Donnerstag, den 14.06.2018, 10:09 +0000 schrieb Robin Gong:
> Hi Lucas,
> Could you double check again? Still can reproduce lockdep warning on
> UART if change spin_lock_lockirqsave/spinlock_unlock_irqrestore to
> spin_lock/spin_unlock in sdma_int_handler as you said without
> patch7/7.
Yes, I can reproduce the lockdep issue with _irqsave variants in the IRQ handler and 7/7 reverted. It fixes the pcm issue though.
> Would you please ask below two more questions?
> 1. Does your uart case pass now with my v4 patchset?
It seems to work, but the change seems to impact the non-DMA serial somehow. On several boots the system was unable to present a login prompt, so patch 7/7 seems to break other stuff and it's a pretty invasive change to the serial driver, which is known to be non-trivial.
I don't have the bandwidth to dig through this patch currently, but I wonder if it couldn't be simplified with the spinlock stuff in the sdma driver fixed.
> 2. Do you make some code change in your local('I just gave this series
> a spin')to reproduce your below issue? If yes, could you post your
> change?
The lockdep splat below was without any changes to your series.
Regards,
Lucas
> On 四, 2018-06-14 at 10:53 +0200, Lucas Stach wrote:
> > Hi Robin,
> >
> > I just gave this series a spin and it seems there is even more
> > locking fun, see the lockdep output below. After taking a short look
> > it seems this is caused by using the wrong spinlock variants in
> > sdma_int_handler(), those should also use the _irqsave ones. When
> > fixing this you might want to reconsider patch 7/7, as it's probably
> > not needed at all.
> >
> > Regards,
> > Lucas
> >
> > [ 20.479401]
> > ========================================================
> > [ 20.485769] WARNING: possible irq lock inversion dependency
> > detected [ 20.492140] 4.17.0+ #1538 Not tainted [ 20.495814]
> > ----------------------------------------------------
> > --
> > --
> > [ 20.502181] systemd/1 just changed the state of lock:
> > [ 20.507247] c0abdafc (&(&substream->self_group.lock)->rlock){-
> > ...}, at: snd_pcm_stream_lock+0x54/0x60 [ 20.516523] but this lock
> > took another, HARDIRQ-unsafe lock in the
> > past:
> > [ 20.523234] (fs_reclaim){+.+.}
> > [ 20.523253]
> > [ 20.523253]
> > [ 20.523253] and interrupts could create inverse lock ordering
> > between them.
> > [ 20.523253]
> > [ 20.537804]
> > [ 20.537804] other info that might help us debug this:
> > [ 20.544344] Possible interrupt unsafe locking scenario:
> > [ 20.544344]
> > [ 20.551144] CPU0 CPU1 [ 20.555686]
> > ---- ---- [ 20.560224] lock(fs_reclaim); [
> > 20.563386] local_irq_disable(); [
> > 20.569315] lock(&(&substream-
> > > self_group.lock)->rlock);
> >
> > [ 20.577337] lock(fs_reclaim); [
> > 20.583014] <Interrupt> [ 20.585643]
> > lock(&(&substream->self_group.lock)->rlock);
> > [ 20.591322]
> > [ 20.591322] *** DEADLOCK ***
> > [ 20.591322]
> > [ 20.597260] 1 lock held by systemd/1:
> > [ 20.607806] #0: ab23d11c (snd_pcm_link_rwlock){.-..}, at:
> > snd_pcm_stream_lock+0x4c/0x60
> > [ 20.615951]
> > [ 20.615951] the shortest dependencies between 2nd lock and 1st
> > lock:
> > [ 20.623799] -> (fs_reclaim){+.+.} ops: 248474 { [ 20.628456]
> > HARDIRQ-ON-W at:
> > [ 20.631716] lock_acquire+0x260/0x29c [
> > 20.637223] fs_reclaim_acquire+0x48/0x58 [
> > 20.643075] kmem_cache_alloc_trace+0x3c/0x
> > 36
> > 4
> > [ 20.649366] alloc_worker.constprop.15+0x28
> > /0
> > x64
> > [ 20.655824] init_rescuer.part.5+0x20/0xa4 [
> > 20.661764] workqueue_init+0x124/0x1f8 [
> > 20.667446] kernel_init_freeable+0x60/0x55
> > 0
> > [ 20.673561] kernel_init+0x18/0x120 [
> > 20.678890] ret_from_fork+0x14/0x20 [
> > 20.684299] (null) [ 20.688408]
> > SOFTIRQ-ON-W at:
> > [ 20.691659] lock_acquire+0x260/0x29c [
> > 20.697158] fs_reclaim_acquire+0x48/0x58 [
> > 20.703006] kmem_cache_alloc_trace+0x3c/0x
> > 36
> > 4
> > [ 20.709288] alloc_worker.constprop.15+0x28
> > /0
> > x64
> > [ 20.709301] init_rescuer.part.5+0x20/0xa4 [
> > 20.720717] workqueue_init+0x124/0x1f8 [
> > 20.720729] kernel_init_freeable+0x60/0x55
> > 0
> > [ 20.720738] kernel_init+0x18/0x120 [
> > 20.720746] ret_from_fork+0x14/0x20 [
> > 20.720751] (null) [ 20.720756] INITIAL
> > USE at:
> > [ 20.720770] lock_acquire+0x260/0x29c [
> > 20.720782] fs_reclaim_acquire+0x48/0x58 [
> > 20.774374] kmem_cache_alloc_trace+0x3c/0x3
> > 64
> > [ 20.780574] alloc_worker.constprop.15+0x28/
> > 0x
> > 64
> > [ 20.786945] init_rescuer.part.5+0x20/0xa4 [
> > 20.792794] workqueue_init+0x124/0x1f8 [
> > 20.798384] kernel_init_freeable+0x60/0x550 [
> > 20.804406] kernel_init+0x18/0x120 [
> > 20.809648] ret_from_fork+0x14/0x20 [
> > 20.814971] (null) [ 20.818992] } [
> > 20.820768] ... key at: [<80e22074>]
> > __fs_reclaim_map+0x0/0x10
> > [ 20.827220] ... acquired at:
> > [ 20.830289] fs_reclaim_acquire+0x48/0x58 [ 20.834487]
> > kmem_cache_alloc_trace+0x3c/0x364 [ 20.839123]
> > sdma_transfer_init+0x44/0x130 [ 20.843409]
> > sdma_prep_dma_cyclic+0x78/0x21c [ 20.847869]
> > snd_dmaengine_pcm_trigger+0xdc/0x184
> > [ 20.852764] soc_pcm_trigger+0x164/0x190 [ 20.856876]
> > snd_pcm_do_start+0x34/0x40 [ 20.860902]
> > snd_pcm_action_single+0x48/0x74 [ 20.865360]
> > snd_pcm_action+0x34/0xfc [ 20.869213]
> > snd_pcm_ioctl+0x910/0x10ec [ 20.873241] vfs_ioctl+0x30/0x44 [
> > 20.876657] do_vfs_ioctl+0xac/0x974 [ 20.880421]
> > ksys_ioctl+0x48/0x64 [ 20.883923] sys_ioctl+0x18/0x1c [
> > 20.887340] ret_fast_syscall+0x0/0x28 [ 20.891277] 0x7289bcbc
> > [ 20.893909] [ 20.895410] ->
> > (&(&substream->self_group.lock)->rlock){-...}
> > ops:
> > 59 {
> > [ 20.901977] IN-HARDIRQ-W at:
> > [ 20.905143] lock_acquire+0x260/0x29c [
> > 20.910473] _raw_spin_lock+0x48/0x58 [
> > 20.915801] snd_pcm_stream_lock+0x54/0x60 [
> > 20.921561] _snd_pcm_stream_lock_irqsave+0x4 0/
> > 0x48
> > [ 20.928107] snd_pcm_period_elapsed+0x2c/0xa4
> > [ 20.934127] dmaengine_pcm_dma_complete+0x54/
> > 0x
> > 58
> > [ 20.940498] sdma_int_handler+0x1dc/0x2a8 [
> > 20.946179] __handle_irq_event_percpu+0x1fc/ 0x
> > 498
> > [ 20.952635] handle_irq_event_percpu+0x38/0x8
> > c [ 20.958742] handle_irq_event+0x48/0x6c [
> > 20.964242] handle_fasteoi_irq+0xc4/0x138 [
> > 20.970006] generic_handle_irq+0x28/0x38 [
> > 20.975681] __handle_domain_irq+0xb0/0xc4 [
> > 20.981443] gic_handle_irq+0x68/0xa0 [
> > 20.986769] __irq_svc+0x70/0xb0 [ 20.991662]
> > _raw_spin_unlock_irq+0x38/0x6c [ 20.997511]
> > task_work_run+0x90/0xb8 [ 21.002751]
> > do_work_pending+0xc8/0xd0 [ 21.008164]
> > slow_work_pending+0xc/0x20 [ 21.013661]
> > 0x76c77e86 [ 21.017768] INITIAL USE at:
> > [ 21.020844] lock_acquire+0x260/0x29c [
> > 21.026086] _raw_spin_lock+0x48/0x58 [
> > 21.031328] snd_pcm_stream_lock+0x54/0x60 [
> > 21.037002] snd_pcm_stream_lock_irq+0x38/0x3c [
> > 21.043023] snd_pcm_sync_ptr+0x214/0x260 [
> > 21.048609] snd_pcm_ioctl+0xbe0/0x10ec [
> > 21.054027] vfs_ioctl+0x30/0x44 [ 21.058832]
> > do_vfs_ioctl+0xac/0x974 [ 21.063984]
> > ksys_ioctl+0x48/0x64 [ 21.068875]
> > sys_ioctl+0x18/0x1c [ 21.073679]
> > ret_fast_syscall+0x0/0x28 [ 21.079004]
> > 0x7e9026dc [ 21.083023] } [ 21.084710] ... key at:
> > [<8162a6e4>] __key.31798+0x0/0x8 [ 21.090552] ... acquired at:
> > [ 21.093537] mark_lock+0x3a4/0x69c [ 21.097128]
> > __lock_acquire+0x420/0x16d4 [ 21.101239]
> > lock_acquire+0x260/0x29c [ 21.105091] _raw_spin_lock+0x48/0x58
> > [ 21.108940] snd_pcm_stream_lock+0x54/0x60 [ 21.113226]
> > _snd_pcm_stream_lock_irqsave+0x40/0x48
> > [ 21.118296] snd_pcm_period_elapsed+0x2c/0xa4 [ 21.122841]
> > dmaengine_pcm_dma_complete+0x54/0x58
> > [ 21.127735] sdma_int_handler+0x1dc/0x2a8 [ 21.131937]
> > __handle_irq_event_percpu+0x1fc/0x498
> > [ 21.136915] handle_irq_event_percpu+0x38/0x8c [ 21.141547]
> > handle_irq_event+0x48/0x6c [ 21.145570]
> > handle_fasteoi_irq+0xc4/0x138 [ 21.149854]
> > generic_handle_irq+0x28/0x38 [ 21.154052]
> > __handle_domain_irq+0xb0/0xc4 [ 21.158335]
> > gic_handle_irq+0x68/0xa0 [ 21.162184] __irq_svc+0x70/0xb0 [
> > 21.165601] _raw_spin_unlock_irq+0x38/0x6c [ 21.169973]
> > task_work_run+0x90/0xb8 [ 21.173735] do_work_pending+0xc8/0xd0
> > [ 21.177670] slow_work_pending+0xc/0x20 [ 21.181691]
> > 0x76c77e86 [ 21.184320] [ 21.185821] [ 21.185821] stack
> > backtrace:
> > [ 21.190198] CPU: 0 PID: 1 Comm: systemd Not tainted 4.17.0+
> > #1538
> > [ 21.196303] Hardware name: Freescale i.MX6 Quad/DualLite (Device
> > Tree)
> > [ 21.202841] Backtrace:
> > [ 21.205314] [<8010e318>] (dump_backtrace) from [<8010e604>]
> > (show_stack+0x20/0x24)
> > [ 21.212900] r7:80e9f3d0 r6:00000000 r5:60070193 r4:80e9f3d0 [
> > 21.218581] [<8010e5e4>] (show_stack) from [<8099b660>]
> > (dump_stack+0xa4/0xd8)
> > [ 21.225825] [<8099b5bc>] (dump_stack) from [<8017b52c>]
> > (print_irq_inversion_bug+0x15c/0x1fc)
> > [ 21.234368] r9:814da818 r8:00000001 r7:ee926c00 r6:00000000
> > r5:ee915bb0 r4:814da818
> > [ 21.242133] [<8017b3d0>] (print_irq_inversion_bug) from
> > [<8017b6dc>] (check_usage_forwards+0x110/0x13c)
> > [ 21.251544] r9:00000002 r8:80bfd3e2 r7:ee926c00 r6:ee927148
> > r5:80e08488 r4:00000001
> > [ 21.259306] [<8017b5cc>] (check_usage_forwards) from [<8017c2a4>]
> > (mark_lock+0x3a4/0x69c)
> > [ 21.267500] r9:ee926c00 r8:80a03cd8 r7:00000101 r6:00000002
> > r5:00000000 r4:ee927148
> > [ 21.275263] [<8017bf00>] (mark_lock) from [<8017cf68>]
> > (__lock_acquire+0x420/0x16d4)
> > [ 21.283023] r10:ee927148 r9:ed4620e4 r8:ee926c00 r7:00000000
> > r6:00000001 r5:00000001
> > [ 21.290863] r4:00000490
> > [ 21.293416] [<8017cb48>] (__lock_acquire) from [<8017ed58>]
> > (lock_acquire+0x260/0x29c)
> > [ 21.301350] r10:00000001 r9:80e084a4 r8:00000000 r7:00000000
> > r6:00000000 r5:ed4620e4
> > [ 21.309189] r4:00000000
> > [ 21.311746] [<8017eaf8>] (lock_acquire) from [<809b74f0>]
> > (_raw_spin_lock+0x48/0x58)
> > [ 21.319506] r10:ee0a4714 r9:ed457100 r8:ee0a46c8 r7:ee0a4714
> > r6:ee0a4010 r5:807847b0
> > [ 21.327346] r4:ed4620d4
> > [ 21.329902] [<809b74a8>] (_raw_spin_lock) from [<807847b0>]
> > (snd_pcm_stream_lock+0x54/0x60)
> > [ 21.338265] r5:ed462000 r4:ed462000 [ 21.341863] [<8078475c>]
> > (snd_pcm_stream_lock) from [<80784838>]
> > (_snd_pcm_stream_lock_irqsave+0x40/0x48)
> > [ 21.351440] r5:ed462000 r4:60070193 [ 21.355042] [<807847f8>]
> > (_snd_pcm_stream_lock_irqsave) from [<8078b044>]
> > (snd_pcm_period_elapsed+0x2c/0xa4)
> > [ 21.364881] r5:ee3ef000 r4:ed462000 [ 21.368478] [<8078b018>]
> > (snd_pcm_period_elapsed) from [<8078d7b4>]
> > (dmaengine_pcm_dma_complete+0x54/0x58)
> > [ 21.378148] r7:ee0a4714 r6:ee0a4010 r5:00000007 r4:ee0a46bc [
> > 21.383827] [<8078d760>] (dmaengine_pcm_dma_complete) from
> > [<80504c0c>] (sdma_int_handler+0x1dc/0x2a8) [ 21.393157]
> > [<80504a30>] (sdma_int_handler) from [<8018cd28>]
> > (__handle_irq_event_percpu+0x1fc/0x498)
> > [ 21.402393] r10:00000000 r9:eeafd400 r8:80e084a4 r7:00000038
> > r6:00000038 r5:80ea3c12
> > [ 21.410233] r4:ee2b5d40
> > [ 21.412787] [<8018cb2c>] (__handle_irq_event_percpu) from
> > [<8018cffc>] (handle_irq_event_percpu+0x38/0x8c)
> > [ 21.422457] r10:00000000 r9:ee914000 r8:ee81d400 r7:00000038
> > r6:eeafd400 r5:eeafd464
> > [ 21.430296] r4:80e08488
> > [ 21.432852] [<8018cfc4>] (handle_irq_event_percpu) from
> > [<8018d098>] (handle_irq_event+0x48/0x6c) [ 21.441736]
> > r6:eeafd464 r5:eeafd464 r4:eeafd400 [ 21.446374] [<8018d050>]
> > (handle_irq_event) from [<8019146c>]
> > (handle_fasteoi_irq+0xc4/0x138)
> > [ 21.454912] r7:00000038 r6:eeafd464 r5:80e10a60 r4:eeafd400 [
> > 21.460589] [<801913a8>] (handle_fasteoi_irq) from [<8018bd9c>]
> > (generic_handle_irq+0x28/0x38)
> > [ 21.469214] r7:00000038 r6:80d92ae4 r5:00000000 r4:00000000 [
> > 21.474893] [<8018bd74>] (generic_handle_irq) from [<8018c48c>]
> > (__handle_domain_irq+0xb0/0xc4)
> > [ 21.483611] [<8018c3dc>] (__handle_domain_irq) from [<80102330>]
> > (gic_handle_irq+0x68/0xa0)
> > [ 21.491978] r9:ee914000 r8:f4001100 r7:80e5c6bc r6:ee915f00
> > r5:80e08c3c r4:f4000100 [ 21.499738] [<801022c8>] (gic_handle_irq)
> > from [<801019f0>]
> > (__irq_svc+0x70/0xb0)
> > [ 21.507233] Exception stack(0xee915f00 to 0xee915f48) [
> > 21.512303] 5f00: 00000001 00000004 00000000 ee926c00 ee9270a8
> > ee926c00 ecc45e00 ee9270a8
> > [ 21.520498] 5f20: 80ec23b0 ee914000 00000000 ee915f64 ee915f20
> > ee915f50 8017c7c0 809b78ac
> > [ 21.528687] 5f40: 20070013 ffffffff [ 21.532193] r9:ee914000
> > r8:80ec23b0 r7:ee915f34 r6:ffffffff
> > r5:20070013 r4:809b78ac
> > [ 21.539959] [<809b7874>] (_raw_spin_unlock_irq) from [<8014e98c>]
> > (task_work_run+0x90/0xb8)
> > [ 21.548321] r5:ee926c00 r4:ecc45e00 [ 21.551913] [<8014e8fc>]
> > (task_work_run) from [<8010da3c>]
> > (do_work_pending+0xc8/0xd0)
> > [ 21.559848] r9:ee914000 r8:801011c4 r7:ee915fb0 r6:ffffe000
> > r5:00000004 r4:801011c4
> > [ 21.567608] [<8010d974>] (do_work_pending) from [<80101034>]
> > (slow_work_pending+0xc/0x20)
> > [ 21.575797] Exception stack(0xee915fb0 to 0xee915ff8) [
> > 21.580864] 5fa0: 00000000
> > 020c34e8 46059f00 00000000
> > [ 21.589059] 5fc0: 00000002 76f133a4 020df680 00000006 76e6e168
> > 00000035 7ef81778 00000035
> > [ 21.597253] 5fe0: 00000006 7ef816a0 76c75d67 76c77e86 20070030
> > 00000039
> > [ 21.603883] r7:00000006 r6:020df680 r5:76f133a4 r4:00000002
> >
> > Am Donnerstag, den 14.06.2018, 22:02 +0800 schrieb Robin Gong:
> > >
> > > The legacy sdma driver has below limitations or drawbacks:
> > > 1. Hardcode the max BDs number as "PAGE_SIZE / sizeof(*)", and
> > > alloc
> > > one page size for one channel regardless of only few BDs
> > > needed
> > > most time. But in few cases, the max PAGE_SIZE maybe not
> > > enough.
> > > 2. One SDMA channel can't stop immediatley once channel disabled
> > > which
> > > means SDMA interrupt may come in after this channel
> > > terminated.There
> > > are some patches for this corner case such as commit
> > > "2746e2c389f9",
> > > but not cover non-cyclic.
> > >
> > > The common virt-dma overcomes the above limitations. It can alloc
> > > bd dynamically and free bd once this tx transfer done. No memory
> > > wasted or maximum limititation here, only depends on how many
> > > memory can be requested from kernel. For No.2, such issue can be
> > > workaround by checking if there is available
> > > descript("sdmac->desc") now once the unwanted interrupt coming. At
> > > last the common virt-dma is easier for sdma driver maintain.
> > >
> > > Change from v3:
> > > 1. add two uart patches which impacted by this patchset.
> > > 2. unlock 'vc.lock' before cyclic dma callback and lock again
> > > after
> > > it because some driver such as uart will call
> > > dmaengine_tx_status
> > > which will acquire 'vc.lock' again and dead lock comes out.
> > > 3. remove 'Revert commit' stuff since that patch is not wrong
> > > and
> > > combine two patch into one patch as Sascha's comment.
> > >
> > > Change from v2:
> > > 1. include Sascha's patch to make the main patch easier to
> > > review.
> > > Thanks Sacha.
> > > 2. remove useless 'desc'/'chan' in struct sdma_channe.
> > >
> > > Change from v1:
> > > 1. split v1 patch into 5 patches.
> > > 2. remove some unnecessary condition check.
> > > 3. remove unnecessary 'pending' list.
> > >
> > > Robin Gong (6):
> > > tty: serial: imx: correct dma cookie status
> > > dmaengine: imx-sdma: add virt-dma support
> > > dmaengine: imx-sdma: remove useless 'lock' and 'enabled' in
> > > 'struct
> > > sdma_channel'
> > > dmaengine: imx-sdma: remove the maximum limitation for bd
> > > numbers
> > > dmaengine: imx-sdma: add sdma_transfer_init to decrease code
> > > overlap
> > > tty: serial: imx: split all dma setup operations out of
> > > 'port.lock'
> > > protector
> > >
> > > Sascha Hauer (1):
> > > dmaengine: imx-sdma: factor out a struct sdma_desc from struct
> > > sdma_channel
> > >
> > > drivers/dma/Kconfig | 1 +
> > > drivers/dma/imx-sdma.c | 394 +++++++++++++++++++++++++++------
> > > --------------
> > > drivers/tty/serial/imx.c | 99 ++++++------
> > > 3 files changed, 282 insertions(+), 212 deletions(-)
^ permalink raw reply
* Re: [PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*
From: Johan Hovold @ 2018-06-14 13:33 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Johan Hovold, LKML, open list:SERIAL DRIVERS, Rob Herring,
Andy Shevchenko
In-Reply-To: <CAPybu_0TSdQFOL1ptv67-aKktAftYWPwnSnevmBd_b1T0W7LYg@mail.gmail.com>
On Thu, Jun 14, 2018 at 01:06:59PM +0200, Ricardo Ribalda Delgado wrote:
> Hi Johan
> On Thu, Jun 14, 2018 at 12:48 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > Hi Ricardo,
> >
> > On Mon, Jun 11, 2018 at 01:52:16PM +0200, Ricardo Ribalda Delgado wrote:
> > > There are some situations where it is interesting to load/remove serdev
> > > devices dynamically, like during board bring-up or when we are
> > > developing a new driver or for devices that are neither described via
> > > ACPI or device tree.
> >
> > First of all, this would be more appropriately labeled an RFC as this is
> > far from being in a mergeable state. Besides some implementation issues,
> > we need to determine if this approach is at all viable.
>
> From previous conversations with Greg it seemed that RFC labels was
> something to avoid, but I do not mind reseding it as RFC on v3.
>
> http://lists.kernelnewbies.org/pipermail/kernelnewbies/2018-March/018844.html
Yeah, Greg uses that to triage his insane workload, but RFCs still have
their use (and are still mentioned in submitting-patches.rst).
> > Second, I wonder how you tested this given that this series breaks RX
> > (and write-wakeup signalling) for serial ports (patch 19/24)?
>
> I have a serdev device (led ctrls) that is dynamically enumerated with
> something very similar to:
>
> https://github.com/ribalda/linux/commit/415bb3f0076c2b846ebe5409589b8e1e3004f55a
>
> and then I have a script that does adds and removes. For standard
> serial port I was not testing the data path, just that the ttyS* was
> enumerated fine.
Well there's more to serial ports than just enumeration, you typically
want to send and receive data as well. ;)
> But yesterday I believe that we found the bug that you mentioned and
> we have fixed it (check end of mail). I will patch the series and
> resend after I get more feedback and also implement what Marcel
> suggested.
>
> WIP is at
> https://github.com/ribalda/linux/tree/serdev3
>
> Besides this bug, we have used the new driver for over a week now with
> no issues.
Hmm... No issues when not testing the main functionality of serial
ports, you mean?
And there are more issues with the series which are less apparent than
the rx (and partial tx) regression.
> > Third, and as Marcel already suggested, you need to limit your scope
> > here. Aim at ten patches or so, and use a representative serdev driver
> > as an example of the kind of driver updates that would be needed. It
> > also looks like some patches should be squashed (e.g. the ones
> > introducing new fields and the first one actually using them).
>
> >
> > > This implementation allows the creation of serdev devices via sysfs,
> > > in a similar way as the i2c bus allows sysfs instantiation [1].
> >
> > Note that this is a legacy interface and not necessarily something that
> > new interfaces should be modelled after.
>
> I would not consider it legacy, it is the only way to use an i2c
> module without writing your own driver and/or modifying ACPI/DT table.
It's legacy as in old, and to be used for one-off hacks and such. But
sure, that is also what this series aims at even if that doesn't mean
you *have to* copy the interface.
> Author: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> Date: Thu Jun 14 11:30:27 2018 +0200
>
> serdev-ttydev: Restore/change ttyport client ops
Yep, you found the source of the broken serial port rx/tx.
Johan
^ permalink raw reply
* [PATCH v4 0/7] add virt-dma support for imx-sdma
From: Robin Gong @ 2018-06-14 14:02 UTC (permalink / raw)
To: vkoul, s.hauer, l.stach, dan.j.williams, gregkh, jslaby
Cc: linux-serial, dmaengine, linux-kernel, linux-arm-kernel,
linux-imx
The legacy sdma driver has below limitations or drawbacks:
1. Hardcode the max BDs number as "PAGE_SIZE / sizeof(*)", and alloc
one page size for one channel regardless of only few BDs needed
most time. But in few cases, the max PAGE_SIZE maybe not enough.
2. One SDMA channel can't stop immediatley once channel disabled which
means SDMA interrupt may come in after this channel terminated.There
are some patches for this corner case such as commit "2746e2c389f9",
but not cover non-cyclic.
The common virt-dma overcomes the above limitations. It can alloc bd
dynamically and free bd once this tx transfer done. No memory wasted or
maximum limititation here, only depends on how many memory can be requested
from kernel. For No.2, such issue can be workaround by checking if there
is available descript("sdmac->desc") now once the unwanted interrupt
coming. At last the common virt-dma is easier for sdma driver maintain.
Change from v3:
1. add two uart patches which impacted by this patchset.
2. unlock 'vc.lock' before cyclic dma callback and lock again after
it because some driver such as uart will call dmaengine_tx_status
which will acquire 'vc.lock' again and dead lock comes out.
3. remove 'Revert commit' stuff since that patch is not wrong and
combine two patch into one patch as Sascha's comment.
Change from v2:
1. include Sascha's patch to make the main patch easier to review.
Thanks Sacha.
2. remove useless 'desc'/'chan' in struct sdma_channe.
Change from v1:
1. split v1 patch into 5 patches.
2. remove some unnecessary condition check.
3. remove unnecessary 'pending' list.
Robin Gong (6):
tty: serial: imx: correct dma cookie status
dmaengine: imx-sdma: add virt-dma support
dmaengine: imx-sdma: remove useless 'lock' and 'enabled' in 'struct
sdma_channel'
dmaengine: imx-sdma: remove the maximum limitation for bd numbers
dmaengine: imx-sdma: add sdma_transfer_init to decrease code overlap
tty: serial: imx: split all dma setup operations out of 'port.lock'
protector
Sascha Hauer (1):
dmaengine: imx-sdma: factor out a struct sdma_desc from struct
sdma_channel
drivers/dma/Kconfig | 1 +
drivers/dma/imx-sdma.c | 394 +++++++++++++++++++++++++++--------------------
drivers/tty/serial/imx.c | 99 ++++++------
3 files changed, 282 insertions(+), 212 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH v4 1/7] tty: serial: imx: correct dma cookie status
From: Robin Gong @ 2018-06-14 14:02 UTC (permalink / raw)
To: vkoul, s.hauer, l.stach, dan.j.williams, gregkh, jslaby
Cc: linux-serial, dmaengine, linux-kernel, linux-arm-kernel,
linux-imx
In-Reply-To: <1528984982-5074-1-git-send-email-yibin.gong@nxp.com>
Correct to check the right rx dma cookie status in spit of it
works because only one cookie is running in the current sdma.
But it will not once sdma driver support multi cookies
running based on virt-dma.
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
drivers/tty/serial/imx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index c2fc6be..b83bc2c 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1051,7 +1051,7 @@ static void imx_uart_dma_rx_callback(void *data)
unsigned int r_bytes;
unsigned int bd_size;
- status = dmaengine_tx_status(chan, (dma_cookie_t)0, &state);
+ status = dmaengine_tx_status(chan, sport->rx_cookie, &state);
if (status == DMA_ERROR) {
imx_uart_clear_rx_errors(sport);
--
2.7.4
^ permalink raw reply related
* [PATCH v4 2/7] dmaengine: imx-sdma: factor out a struct sdma_desc from struct sdma_channel
From: Robin Gong @ 2018-06-14 14:02 UTC (permalink / raw)
To: vkoul, s.hauer, l.stach, dan.j.williams, gregkh, jslaby
Cc: linux-serial, dmaengine, linux-kernel, linux-arm-kernel,
linux-imx
In-Reply-To: <1528984982-5074-1-git-send-email-yibin.gong@nxp.com>
From: Sascha Hauer <s.hauer@pengutronix.de>
This is a preparation step to make the adding of virt-dma easier.
We create a struct sdma_desc, move some fields from struct sdma_channel
there and add a pointer from the former to the latter. For now we
allocate the data statically in struct sdma_channel, but with
virt-dma support it will be dynamically allocated.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
drivers/dma/imx-sdma.c | 137 ++++++++++++++++++++++++++++++-------------------
1 file changed, 83 insertions(+), 54 deletions(-)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index ccd03c3..556d087 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -296,6 +296,30 @@ struct sdma_context_data {
struct sdma_engine;
/**
+ * struct sdma_desc - descriptor structor for one transfer
+ * @vd descriptor for virt dma
+ * @num_bd max NUM_BD. number of descriptors currently handling
+ * @buf_tail ID of the buffer that was processed
+ * @buf_ptail ID of the previous buffer that was processed
+ * @period_len period length, used in cyclic.
+ * @chn_real_count the real count updated from bd->mode.count
+ * @chn_count the transfer count setuped
+ * @sdmac sdma_channel pointer
+ * @bd pointer of alloced bd
+ */
+struct sdma_desc {
+ unsigned int num_bd;
+ dma_addr_t bd_phys;
+ unsigned int buf_tail;
+ unsigned int buf_ptail;
+ unsigned int period_len;
+ unsigned int chn_real_count;
+ unsigned int chn_count;
+ struct sdma_channel *sdmac;
+ struct sdma_buffer_descriptor *bd;
+};
+
+/**
* struct sdma_channel - housekeeping for a SDMA channel
*
* @sdma pointer to the SDMA engine for this channel
@@ -305,11 +329,10 @@ struct sdma_engine;
* @event_id0 aka dma request line
* @event_id1 for channels that use 2 events
* @word_size peripheral access size
- * @buf_tail ID of the buffer that was processed
- * @buf_ptail ID of the previous buffer that was processed
- * @num_bd max NUM_BD. number of descriptors currently handling
*/
struct sdma_channel {
+ struct sdma_desc *desc;
+ struct sdma_desc _desc;
struct sdma_engine *sdma;
unsigned int channel;
enum dma_transfer_direction direction;
@@ -317,12 +340,6 @@ struct sdma_channel {
unsigned int event_id0;
unsigned int event_id1;
enum dma_slave_buswidth word_size;
- unsigned int buf_tail;
- unsigned int buf_ptail;
- unsigned int num_bd;
- unsigned int period_len;
- struct sdma_buffer_descriptor *bd;
- dma_addr_t bd_phys;
unsigned int pc_from_device, pc_to_device;
unsigned int device_to_device;
unsigned long flags;
@@ -332,10 +349,8 @@ struct sdma_channel {
u32 shp_addr, per_addr;
struct dma_chan chan;
spinlock_t lock;
- struct dma_async_tx_descriptor desc;
+ struct dma_async_tx_descriptor txdesc;
enum dma_status status;
- unsigned int chn_count;
- unsigned int chn_real_count;
struct tasklet_struct tasklet;
struct imx_dma_data data;
bool enabled;
@@ -398,6 +413,8 @@ struct sdma_engine {
u32 spba_start_addr;
u32 spba_end_addr;
unsigned int irq;
+ dma_addr_t bd0_phys;
+ struct sdma_buffer_descriptor *bd0;
};
static struct sdma_driver_data sdma_imx31 = {
@@ -632,7 +649,7 @@ static int sdma_run_channel0(struct sdma_engine *sdma)
static int sdma_load_script(struct sdma_engine *sdma, void *buf, int size,
u32 address)
{
- struct sdma_buffer_descriptor *bd0 = sdma->channel[0].bd;
+ struct sdma_buffer_descriptor *bd0 = sdma->bd0;
void *buf_virt;
dma_addr_t buf_phys;
int ret;
@@ -707,7 +724,9 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
* call callback function.
*/
while (1) {
- bd = &sdmac->bd[sdmac->buf_tail];
+ struct sdma_desc *desc = sdmac->desc;
+
+ bd = &desc->bd[desc->buf_tail];
if (bd->mode.status & BD_DONE)
break;
@@ -723,11 +742,11 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
* the number of bytes present in the current buffer descriptor.
*/
- sdmac->chn_real_count = bd->mode.count;
+ desc->chn_real_count = bd->mode.count;
bd->mode.status |= BD_DONE;
- bd->mode.count = sdmac->period_len;
- sdmac->buf_ptail = sdmac->buf_tail;
- sdmac->buf_tail = (sdmac->buf_tail + 1) % sdmac->num_bd;
+ bd->mode.count = desc->period_len;
+ desc->buf_ptail = desc->buf_tail;
+ desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
/*
* The callback is called from the interrupt context in order
@@ -736,7 +755,7 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
* executed.
*/
- dmaengine_desc_get_callback_invoke(&sdmac->desc, NULL);
+ dmaengine_desc_get_callback_invoke(&sdmac->txdesc, NULL);
if (error)
sdmac->status = old_status;
@@ -749,17 +768,17 @@ static void mxc_sdma_handle_channel_normal(unsigned long data)
struct sdma_buffer_descriptor *bd;
int i, error = 0;
- sdmac->chn_real_count = 0;
+ sdmac->desc->chn_real_count = 0;
/*
* non loop mode. Iterate over all descriptors, collect
* errors and call callback function
*/
- for (i = 0; i < sdmac->num_bd; i++) {
- bd = &sdmac->bd[i];
+ for (i = 0; i < sdmac->desc->num_bd; i++) {
+ bd = &sdmac->desc->bd[i];
if (bd->mode.status & (BD_DONE | BD_RROR))
error = -EIO;
- sdmac->chn_real_count += bd->mode.count;
+ sdmac->desc->chn_real_count += bd->mode.count;
}
if (error)
@@ -767,9 +786,9 @@ static void mxc_sdma_handle_channel_normal(unsigned long data)
else
sdmac->status = DMA_COMPLETE;
- dma_cookie_complete(&sdmac->desc);
+ dma_cookie_complete(&sdmac->txdesc);
- dmaengine_desc_get_callback_invoke(&sdmac->desc, NULL);
+ dmaengine_desc_get_callback_invoke(&sdmac->txdesc, NULL);
}
static irqreturn_t sdma_int_handler(int irq, void *dev_id)
@@ -897,7 +916,7 @@ static int sdma_load_context(struct sdma_channel *sdmac)
int channel = sdmac->channel;
int load_address;
struct sdma_context_data *context = sdma->context;
- struct sdma_buffer_descriptor *bd0 = sdma->channel[0].bd;
+ struct sdma_buffer_descriptor *bd0 = sdma->bd0;
int ret;
unsigned long flags;
@@ -1100,18 +1119,22 @@ static int sdma_set_channel_priority(struct sdma_channel *sdmac,
static int sdma_request_channel(struct sdma_channel *sdmac)
{
struct sdma_engine *sdma = sdmac->sdma;
+ struct sdma_desc *desc;
int channel = sdmac->channel;
int ret = -EBUSY;
- sdmac->bd = dma_zalloc_coherent(NULL, PAGE_SIZE, &sdmac->bd_phys,
+ sdmac->desc = &sdmac->_desc;
+ desc = sdmac->desc;
+
+ desc->bd = dma_zalloc_coherent(NULL, PAGE_SIZE, &desc->bd_phys,
GFP_KERNEL);
- if (!sdmac->bd) {
+ if (!desc->bd) {
ret = -ENOMEM;
goto out;
}
- sdma->channel_control[channel].base_bd_ptr = sdmac->bd_phys;
- sdma->channel_control[channel].current_bd_ptr = sdmac->bd_phys;
+ sdma->channel_control[channel].base_bd_ptr = desc->bd_phys;
+ sdma->channel_control[channel].current_bd_ptr = desc->bd_phys;
sdma_set_channel_priority(sdmac, MXC_SDMA_DEFAULT_PRIORITY);
return 0;
@@ -1176,10 +1199,10 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
if (ret)
goto disable_clk_ahb;
- dma_async_tx_descriptor_init(&sdmac->desc, chan);
- sdmac->desc.tx_submit = sdma_tx_submit;
+ dma_async_tx_descriptor_init(&sdmac->txdesc, chan);
+ sdmac->txdesc.tx_submit = sdma_tx_submit;
/* txd.flags will be overwritten in prep funcs */
- sdmac->desc.flags = DMA_CTRL_ACK;
+ sdmac->txdesc.flags = DMA_CTRL_ACK;
return 0;
@@ -1194,6 +1217,7 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
{
struct sdma_channel *sdmac = to_sdma_chan(chan);
struct sdma_engine *sdma = sdmac->sdma;
+ struct sdma_desc *desc = sdmac->desc;
sdma_disable_channel(chan);
@@ -1207,7 +1231,7 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
sdma_set_channel_priority(sdmac, 0);
- dma_free_coherent(NULL, PAGE_SIZE, sdmac->bd, sdmac->bd_phys);
+ dma_free_coherent(NULL, PAGE_SIZE, desc->bd, desc->bd_phys);
clk_disable(sdma->clk_ipg);
clk_disable(sdma->clk_ahb);
@@ -1223,6 +1247,7 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
int ret, i, count;
int channel = sdmac->channel;
struct scatterlist *sg;
+ struct sdma_desc *desc = sdmac->desc;
if (sdmac->status == DMA_IN_PROGRESS)
return NULL;
@@ -1230,9 +1255,9 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
sdmac->flags = 0;
- sdmac->buf_tail = 0;
- sdmac->buf_ptail = 0;
- sdmac->chn_real_count = 0;
+ desc->buf_tail = 0;
+ desc->buf_ptail = 0;
+ desc->chn_real_count = 0;
dev_dbg(sdma->dev, "setting up %d entries for channel %d.\n",
sg_len, channel);
@@ -1249,9 +1274,9 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
goto err_out;
}
- sdmac->chn_count = 0;
+ desc->chn_count = 0;
for_each_sg(sgl, sg, sg_len, i) {
- struct sdma_buffer_descriptor *bd = &sdmac->bd[i];
+ struct sdma_buffer_descriptor *bd = &desc->bd[i];
int param;
bd->buffer_addr = sg->dma_address;
@@ -1266,7 +1291,7 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
}
bd->mode.count = count;
- sdmac->chn_count += count;
+ desc->chn_count += count;
if (sdmac->word_size > DMA_SLAVE_BUSWIDTH_4_BYTES) {
ret = -EINVAL;
@@ -1307,10 +1332,10 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
bd->mode.status = param;
}
- sdmac->num_bd = sg_len;
- sdma->channel_control[channel].current_bd_ptr = sdmac->bd_phys;
+ desc->num_bd = sg_len;
+ sdma->channel_control[channel].current_bd_ptr = desc->bd_phys;
- return &sdmac->desc;
+ return &sdmac->txdesc;
err_out:
sdmac->status = DMA_ERROR;
return NULL;
@@ -1326,6 +1351,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
int num_periods = buf_len / period_len;
int channel = sdmac->channel;
int ret, i = 0, buf = 0;
+ struct sdma_desc *desc = sdmac->desc;
dev_dbg(sdma->dev, "%s channel: %d\n", __func__, channel);
@@ -1334,10 +1360,10 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
sdmac->status = DMA_IN_PROGRESS;
- sdmac->buf_tail = 0;
- sdmac->buf_ptail = 0;
- sdmac->chn_real_count = 0;
- sdmac->period_len = period_len;
+ desc->buf_tail = 0;
+ desc->buf_ptail = 0;
+ desc->chn_real_count = 0;
+ desc->period_len = period_len;
sdmac->flags |= IMX_DMA_SG_LOOP;
sdmac->direction = direction;
@@ -1358,7 +1384,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
}
while (buf < buf_len) {
- struct sdma_buffer_descriptor *bd = &sdmac->bd[i];
+ struct sdma_buffer_descriptor *bd = &desc->bd[i];
int param;
bd->buffer_addr = dma_addr;
@@ -1389,10 +1415,10 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
i++;
}
- sdmac->num_bd = num_periods;
- sdma->channel_control[channel].current_bd_ptr = sdmac->bd_phys;
+ desc->num_bd = num_periods;
+ sdma->channel_control[channel].current_bd_ptr = desc->bd_phys;
- return &sdmac->desc;
+ return &sdmac->txdesc;
err_out:
sdmac->status = DMA_ERROR;
return NULL;
@@ -1431,13 +1457,14 @@ static enum dma_status sdma_tx_status(struct dma_chan *chan,
struct dma_tx_state *txstate)
{
struct sdma_channel *sdmac = to_sdma_chan(chan);
+ struct sdma_desc *desc = sdmac->desc;
u32 residue;
if (sdmac->flags & IMX_DMA_SG_LOOP)
- residue = (sdmac->num_bd - sdmac->buf_ptail) *
- sdmac->period_len - sdmac->chn_real_count;
+ residue = (desc->num_bd - desc->buf_ptail) *
+ desc->period_len - desc->chn_real_count;
else
- residue = sdmac->chn_count - sdmac->chn_real_count;
+ residue = desc->chn_count - desc->chn_real_count;
dma_set_tx_state(txstate, chan->completed_cookie, chan->cookie,
residue);
@@ -1661,6 +1688,8 @@ static int sdma_init(struct sdma_engine *sdma)
if (ret)
goto err_dma_alloc;
+ sdma->bd0 = sdma->channel[0].desc->bd;
+
sdma_config_ownership(&sdma->channel[0], false, true, false);
/* Set Command Channel (Channel Zero) */
--
2.7.4
^ permalink raw reply related
* [PATCH v4 3/7] dmaengine: imx-sdma: add virt-dma support
From: Robin Gong @ 2018-06-14 14:02 UTC (permalink / raw)
To: vkoul, s.hauer, l.stach, dan.j.williams, gregkh, jslaby
Cc: linux-serial, dmaengine, linux-kernel, linux-arm-kernel,
linux-imx
In-Reply-To: <1528984982-5074-1-git-send-email-yibin.gong@nxp.com>
The legacy sdma driver has below limitations or drawbacks:
1. Hardcode the max BDs number as "PAGE_SIZE / sizeof(*)", and alloc
one page size for one channel regardless of only few BDs needed
most time. But in few cases, the max PAGE_SIZE maybe not enough.
2. One SDMA channel can't stop immediatley once channel disabled which
means SDMA interrupt may come in after this channel terminated.There
are some patches for this corner case such as commit "2746e2c389f9",
but not cover non-cyclic.
The common virt-dma overcomes the above limitations. It can alloc bd
dynamically and free bd once this tx transfer done. No memory wasted or
maximum limititation here, only depends on how many memory can be requested
from kernel. For No.2, such issue can be workaround by checking if there
is available descript("sdmac->desc") now once the unwanted interrupt
coming. At last the common virt-dma is easier for sdma driver maintain.
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
drivers/dma/Kconfig | 1 +
drivers/dma/imx-sdma.c | 261 ++++++++++++++++++++++++++++++++-----------------
2 files changed, 170 insertions(+), 92 deletions(-)
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 6d61cd0..78715a2 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -257,6 +257,7 @@ config IMX_SDMA
tristate "i.MX SDMA support"
depends on ARCH_MXC
select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
help
Support the i.MX SDMA engine. This engine is integrated into
Freescale i.MX25/31/35/51/53/6 chips.
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 556d087..719bf9f 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -48,6 +48,7 @@
#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
#include "dmaengine.h"
+#include "virt-dma.h"
/* SDMA registers */
#define SDMA_H_C0PTR 0x000
@@ -308,6 +309,7 @@ struct sdma_engine;
* @bd pointer of alloced bd
*/
struct sdma_desc {
+ struct virt_dma_desc vd;
unsigned int num_bd;
dma_addr_t bd_phys;
unsigned int buf_tail;
@@ -331,8 +333,8 @@ struct sdma_desc {
* @word_size peripheral access size
*/
struct sdma_channel {
+ struct virt_dma_chan vc;
struct sdma_desc *desc;
- struct sdma_desc _desc;
struct sdma_engine *sdma;
unsigned int channel;
enum dma_transfer_direction direction;
@@ -347,11 +349,8 @@ struct sdma_channel {
unsigned long event_mask[2];
unsigned long watermark_level;
u32 shp_addr, per_addr;
- struct dma_chan chan;
spinlock_t lock;
- struct dma_async_tx_descriptor txdesc;
enum dma_status status;
- struct tasklet_struct tasklet;
struct imx_dma_data data;
bool enabled;
};
@@ -705,6 +704,35 @@ static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event)
writel_relaxed(val, sdma->regs + chnenbl);
}
+static struct sdma_desc *to_sdma_desc(struct dma_async_tx_descriptor *t)
+{
+ return container_of(t, struct sdma_desc, vd.tx);
+}
+
+static void sdma_start_desc(struct sdma_channel *sdmac)
+{
+ struct virt_dma_desc *vd = vchan_next_desc(&sdmac->vc);
+ struct sdma_desc *desc;
+ struct sdma_engine *sdma = sdmac->sdma;
+ int channel = sdmac->channel;
+
+ if (!vd) {
+ sdmac->desc = NULL;
+ return;
+ }
+ sdmac->desc = desc = to_sdma_desc(&vd->tx);
+ /*
+ * Do not delete the node in desc_issued list in cyclic mode, otherwise
+ * the desc alloced will never be freed in vchan_dma_desc_free_list
+ */
+ if (!(sdmac->flags & IMX_DMA_SG_LOOP))
+ list_del(&vd->node);
+
+ sdma->channel_control[channel].base_bd_ptr = desc->bd_phys;
+ sdma->channel_control[channel].current_bd_ptr = desc->bd_phys;
+ sdma_enable_channel(sdma, sdmac->channel);
+}
+
static void sdma_update_channel_loop(struct sdma_channel *sdmac)
{
struct sdma_buffer_descriptor *bd;
@@ -723,7 +751,7 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
* loop mode. Iterate over descriptors, re-setup them and
* call callback function.
*/
- while (1) {
+ while (sdmac->desc) {
struct sdma_desc *desc = sdmac->desc;
bd = &desc->bd[desc->buf_tail];
@@ -754,15 +782,16 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
* SDMA transaction status by the time the client tasklet is
* executed.
*/
-
- dmaengine_desc_get_callback_invoke(&sdmac->txdesc, NULL);
+ spin_unlock(&sdmac->vc.lock);
+ dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
+ spin_lock(&sdmac->vc.lock);
if (error)
sdmac->status = old_status;
}
}
-static void mxc_sdma_handle_channel_normal(unsigned long data)
+static void mxc_sdma_handle_channel_normal(struct sdma_channel *data)
{
struct sdma_channel *sdmac = (struct sdma_channel *) data;
struct sdma_buffer_descriptor *bd;
@@ -785,10 +814,6 @@ static void mxc_sdma_handle_channel_normal(unsigned long data)
sdmac->status = DMA_ERROR;
else
sdmac->status = DMA_COMPLETE;
-
- dma_cookie_complete(&sdmac->txdesc);
-
- dmaengine_desc_get_callback_invoke(&sdmac->txdesc, NULL);
}
static irqreturn_t sdma_int_handler(int irq, void *dev_id)
@@ -804,12 +829,21 @@ static irqreturn_t sdma_int_handler(int irq, void *dev_id)
while (stat) {
int channel = fls(stat) - 1;
struct sdma_channel *sdmac = &sdma->channel[channel];
+ struct sdma_desc *desc;
+
+ spin_lock(&sdmac->vc.lock);
+ desc = sdmac->desc;
+ if (desc) {
+ if (sdmac->flags & IMX_DMA_SG_LOOP) {
+ sdma_update_channel_loop(sdmac);
+ } else {
+ mxc_sdma_handle_channel_normal(sdmac);
+ vchan_cookie_complete(&desc->vd);
+ sdma_start_desc(sdmac);
+ }
+ }
- if (sdmac->flags & IMX_DMA_SG_LOOP)
- sdma_update_channel_loop(sdmac);
- else
- tasklet_schedule(&sdmac->tasklet);
-
+ spin_unlock(&sdmac->vc.lock);
__clear_bit(channel, &stat);
}
@@ -965,7 +999,7 @@ static int sdma_load_context(struct sdma_channel *sdmac)
static struct sdma_channel *to_sdma_chan(struct dma_chan *chan)
{
- return container_of(chan, struct sdma_channel, chan);
+ return container_of(chan, struct sdma_channel, vc.chan);
}
static int sdma_disable_channel(struct dma_chan *chan)
@@ -987,7 +1021,16 @@ static int sdma_disable_channel(struct dma_chan *chan)
static int sdma_disable_channel_with_delay(struct dma_chan *chan)
{
+ struct sdma_channel *sdmac = to_sdma_chan(chan);
+ unsigned long flags;
+ LIST_HEAD(head);
+
sdma_disable_channel(chan);
+ spin_lock_irqsave(&sdmac->vc.lock, flags);
+ vchan_get_all_descriptors(&sdmac->vc, &head);
+ sdmac->desc = NULL;
+ spin_unlock_irqrestore(&sdmac->vc.lock, flags);
+ vchan_dma_desc_free_list(&sdmac->vc, &head);
/*
* According to NXP R&D team a delay of one BD SDMA cost time
@@ -1116,46 +1159,56 @@ static int sdma_set_channel_priority(struct sdma_channel *sdmac,
return 0;
}
-static int sdma_request_channel(struct sdma_channel *sdmac)
+static int sdma_request_channel0(struct sdma_engine *sdma)
{
- struct sdma_engine *sdma = sdmac->sdma;
- struct sdma_desc *desc;
- int channel = sdmac->channel;
int ret = -EBUSY;
- sdmac->desc = &sdmac->_desc;
- desc = sdmac->desc;
-
- desc->bd = dma_zalloc_coherent(NULL, PAGE_SIZE, &desc->bd_phys,
+ sdma->bd0 = dma_zalloc_coherent(NULL, PAGE_SIZE, &sdma->bd0_phys,
GFP_KERNEL);
- if (!desc->bd) {
+ if (!sdma->bd0) {
ret = -ENOMEM;
goto out;
}
- sdma->channel_control[channel].base_bd_ptr = desc->bd_phys;
- sdma->channel_control[channel].current_bd_ptr = desc->bd_phys;
+ sdma->channel_control[0].base_bd_ptr = sdma->bd0_phys;
+ sdma->channel_control[0].current_bd_ptr = sdma->bd0_phys;
- sdma_set_channel_priority(sdmac, MXC_SDMA_DEFAULT_PRIORITY);
+ sdma_set_channel_priority(&sdma->channel[0], MXC_SDMA_DEFAULT_PRIORITY);
return 0;
out:
return ret;
}
-static dma_cookie_t sdma_tx_submit(struct dma_async_tx_descriptor *tx)
+
+static int sdma_alloc_bd(struct sdma_desc *desc)
{
- unsigned long flags;
- struct sdma_channel *sdmac = to_sdma_chan(tx->chan);
- dma_cookie_t cookie;
+ u32 bd_size = desc->num_bd * sizeof(struct sdma_buffer_descriptor);
+ int ret = 0;
- spin_lock_irqsave(&sdmac->lock, flags);
+ desc->bd = dma_zalloc_coherent(NULL, bd_size, &desc->bd_phys,
+ GFP_ATOMIC);
+ if (!desc->bd) {
+ ret = -ENOMEM;
+ goto out;
+ }
+out:
+ return ret;
+}
- cookie = dma_cookie_assign(tx);
+static void sdma_free_bd(struct sdma_desc *desc)
+{
+ u32 bd_size = desc->num_bd * sizeof(struct sdma_buffer_descriptor);
- spin_unlock_irqrestore(&sdmac->lock, flags);
+ dma_free_coherent(NULL, bd_size, desc->bd, desc->bd_phys);
+}
- return cookie;
+static void sdma_desc_free(struct virt_dma_desc *vd)
+{
+ struct sdma_desc *desc = container_of(vd, struct sdma_desc, vd);
+
+ sdma_free_bd(desc);
+ kfree(desc);
}
static int sdma_alloc_chan_resources(struct dma_chan *chan)
@@ -1191,19 +1244,10 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
if (ret)
goto disable_clk_ipg;
- ret = sdma_request_channel(sdmac);
- if (ret)
- goto disable_clk_ahb;
-
ret = sdma_set_channel_priority(sdmac, prio);
if (ret)
goto disable_clk_ahb;
- dma_async_tx_descriptor_init(&sdmac->txdesc, chan);
- sdmac->txdesc.tx_submit = sdma_tx_submit;
- /* txd.flags will be overwritten in prep funcs */
- sdmac->txdesc.flags = DMA_CTRL_ACK;
-
return 0;
disable_clk_ahb:
@@ -1217,9 +1261,8 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
{
struct sdma_channel *sdmac = to_sdma_chan(chan);
struct sdma_engine *sdma = sdmac->sdma;
- struct sdma_desc *desc = sdmac->desc;
- sdma_disable_channel(chan);
+ sdma_disable_channel_with_delay(chan);
if (sdmac->event_id0)
sdma_event_disable(sdmac, sdmac->event_id0);
@@ -1231,8 +1274,6 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
sdma_set_channel_priority(sdmac, 0);
- dma_free_coherent(NULL, PAGE_SIZE, desc->bd, desc->bd_phys);
-
clk_disable(sdma->clk_ipg);
clk_disable(sdma->clk_ahb);
}
@@ -1247,7 +1288,7 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
int ret, i, count;
int channel = sdmac->channel;
struct scatterlist *sg;
- struct sdma_desc *desc = sdmac->desc;
+ struct sdma_desc *desc;
if (sdmac->status == DMA_IN_PROGRESS)
return NULL;
@@ -1255,23 +1296,34 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
sdmac->flags = 0;
+ desc = kzalloc((sizeof(*desc)), GFP_KERNEL);
+ if (!desc)
+ goto err_out;
+
desc->buf_tail = 0;
desc->buf_ptail = 0;
+ desc->sdmac = sdmac;
+ desc->num_bd = sg_len;
desc->chn_real_count = 0;
+ if (sdma_alloc_bd(desc)) {
+ kfree(desc);
+ goto err_out;
+ }
+
dev_dbg(sdma->dev, "setting up %d entries for channel %d.\n",
sg_len, channel);
sdmac->direction = direction;
ret = sdma_load_context(sdmac);
if (ret)
- goto err_out;
+ goto err_bd_out;
if (sg_len > NUM_BD) {
dev_err(sdma->dev, "SDMA channel %d: maximum number of sg exceeded: %d > %d\n",
channel, sg_len, NUM_BD);
ret = -EINVAL;
- goto err_out;
+ goto err_bd_out;
}
desc->chn_count = 0;
@@ -1287,7 +1339,7 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
dev_err(sdma->dev, "SDMA channel %d: maximum bytes for sg entry exceeded: %d > %d\n",
channel, count, 0xffff);
ret = -EINVAL;
- goto err_out;
+ goto err_bd_out;
}
bd->mode.count = count;
@@ -1295,25 +1347,25 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
if (sdmac->word_size > DMA_SLAVE_BUSWIDTH_4_BYTES) {
ret = -EINVAL;
- goto err_out;
+ goto err_bd_out;
}
switch (sdmac->word_size) {
case DMA_SLAVE_BUSWIDTH_4_BYTES:
bd->mode.command = 0;
if (count & 3 || sg->dma_address & 3)
- return NULL;
+ goto err_bd_out;
break;
case DMA_SLAVE_BUSWIDTH_2_BYTES:
bd->mode.command = 2;
if (count & 1 || sg->dma_address & 1)
- return NULL;
+ goto err_bd_out;
break;
case DMA_SLAVE_BUSWIDTH_1_BYTE:
bd->mode.command = 1;
break;
default:
- return NULL;
+ goto err_bd_out;
}
param = BD_DONE | BD_EXTD | BD_CONT;
@@ -1332,10 +1384,10 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
bd->mode.status = param;
}
- desc->num_bd = sg_len;
- sdma->channel_control[channel].current_bd_ptr = desc->bd_phys;
-
- return &sdmac->txdesc;
+ return vchan_tx_prep(&sdmac->vc, &desc->vd, flags);
+err_bd_out:
+ sdma_free_bd(desc);
+ kfree(desc);
err_out:
sdmac->status = DMA_ERROR;
return NULL;
@@ -1351,7 +1403,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
int num_periods = buf_len / period_len;
int channel = sdmac->channel;
int ret, i = 0, buf = 0;
- struct sdma_desc *desc = sdmac->desc;
+ struct sdma_desc *desc;
dev_dbg(sdma->dev, "%s channel: %d\n", __func__, channel);
@@ -1360,27 +1412,39 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
sdmac->status = DMA_IN_PROGRESS;
+ desc = kzalloc((sizeof(*desc)), GFP_KERNEL);
+ if (!desc)
+ goto err_out;
+
desc->buf_tail = 0;
desc->buf_ptail = 0;
+ desc->sdmac = sdmac;
+ desc->num_bd = num_periods;
desc->chn_real_count = 0;
desc->period_len = period_len;
sdmac->flags |= IMX_DMA_SG_LOOP;
sdmac->direction = direction;
+
+ if (sdma_alloc_bd(desc)) {
+ kfree(desc);
+ goto err_bd_out;
+ }
+
ret = sdma_load_context(sdmac);
if (ret)
- goto err_out;
+ goto err_bd_out;
if (num_periods > NUM_BD) {
dev_err(sdma->dev, "SDMA channel %d: maximum number of sg exceeded: %d > %d\n",
channel, num_periods, NUM_BD);
- goto err_out;
+ goto err_bd_out;
}
if (period_len > 0xffff) {
dev_err(sdma->dev, "SDMA channel %d: maximum period size exceeded: %zu > %d\n",
channel, period_len, 0xffff);
- goto err_out;
+ goto err_bd_out;
}
while (buf < buf_len) {
@@ -1392,7 +1456,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
bd->mode.count = period_len;
if (sdmac->word_size > DMA_SLAVE_BUSWIDTH_4_BYTES)
- goto err_out;
+ goto err_bd_out;
if (sdmac->word_size == DMA_SLAVE_BUSWIDTH_4_BYTES)
bd->mode.command = 0;
else
@@ -1415,10 +1479,10 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
i++;
}
- desc->num_bd = num_periods;
- sdma->channel_control[channel].current_bd_ptr = desc->bd_phys;
-
- return &sdmac->txdesc;
+ return vchan_tx_prep(&sdmac->vc, &desc->vd, flags);
+err_bd_out:
+ sdma_free_bd(desc);
+ kfree(desc);
err_out:
sdmac->status = DMA_ERROR;
return NULL;
@@ -1457,14 +1521,31 @@ static enum dma_status sdma_tx_status(struct dma_chan *chan,
struct dma_tx_state *txstate)
{
struct sdma_channel *sdmac = to_sdma_chan(chan);
- struct sdma_desc *desc = sdmac->desc;
+ struct sdma_desc *desc;
u32 residue;
+ struct virt_dma_desc *vd;
+ enum dma_status ret;
+ unsigned long flags;
- if (sdmac->flags & IMX_DMA_SG_LOOP)
- residue = (desc->num_bd - desc->buf_ptail) *
- desc->period_len - desc->chn_real_count;
- else
- residue = desc->chn_count - desc->chn_real_count;
+ ret = dma_cookie_status(chan, cookie, txstate);
+ if (ret == DMA_COMPLETE || !txstate)
+ return ret;
+
+ spin_lock_irqsave(&sdmac->vc.lock, flags);
+ vd = vchan_find_desc(&sdmac->vc, cookie);
+ if (vd) {
+ desc = to_sdma_desc(&vd->tx);
+ if (sdmac->flags & IMX_DMA_SG_LOOP)
+ residue = (desc->num_bd - desc->buf_ptail) *
+ desc->period_len - desc->chn_real_count;
+ else
+ residue = desc->chn_count - desc->chn_real_count;
+ } else if (sdmac->desc && sdmac->desc->vd.tx.cookie == cookie) {
+ residue = sdmac->desc->chn_count - sdmac->desc->chn_real_count;
+ } else {
+ residue = 0;
+ }
+ spin_unlock_irqrestore(&sdmac->vc.lock, flags);
dma_set_tx_state(txstate, chan->completed_cookie, chan->cookie,
residue);
@@ -1475,10 +1556,12 @@ static enum dma_status sdma_tx_status(struct dma_chan *chan,
static void sdma_issue_pending(struct dma_chan *chan)
{
struct sdma_channel *sdmac = to_sdma_chan(chan);
- struct sdma_engine *sdma = sdmac->sdma;
+ unsigned long flags;
- if (sdmac->status == DMA_IN_PROGRESS)
- sdma_enable_channel(sdma, sdmac->channel);
+ spin_lock_irqsave(&sdmac->vc.lock, flags);
+ if (vchan_issue_pending(&sdmac->vc) && !sdmac->desc)
+ sdma_start_desc(sdmac);
+ spin_unlock_irqrestore(&sdmac->vc.lock, flags);
}
#define SDMA_SCRIPT_ADDRS_ARRAY_SIZE_V1 34
@@ -1684,12 +1767,10 @@ static int sdma_init(struct sdma_engine *sdma)
for (i = 0; i < MAX_DMA_CHANNELS; i++)
writel_relaxed(0, sdma->regs + SDMA_CHNPRI_0 + i * 4);
- ret = sdma_request_channel(&sdma->channel[0]);
+ ret = sdma_request_channel0(sdma);
if (ret)
goto err_dma_alloc;
- sdma->bd0 = sdma->channel[0].desc->bd;
-
sdma_config_ownership(&sdma->channel[0], false, true, false);
/* Set Command Channel (Channel Zero) */
@@ -1850,20 +1931,15 @@ static int sdma_probe(struct platform_device *pdev)
sdmac->sdma = sdma;
spin_lock_init(&sdmac->lock);
- sdmac->chan.device = &sdma->dma_device;
- dma_cookie_init(&sdmac->chan);
sdmac->channel = i;
-
- tasklet_init(&sdmac->tasklet, mxc_sdma_handle_channel_normal,
- (unsigned long) sdmac);
+ sdmac->vc.desc_free = sdma_desc_free;
/*
* Add the channel to the DMAC list. Do not add channel 0 though
* because we need it internally in the SDMA driver. This also means
* that channel 0 in dmaengine counting matches sdma channel 1.
*/
if (i)
- list_add_tail(&sdmac->chan.device_node,
- &sdma->dma_device.channels);
+ vchan_init(&sdmac->vc, &sdma->dma_device);
}
ret = sdma_init(sdma);
@@ -1968,7 +2044,8 @@ static int sdma_remove(struct platform_device *pdev)
for (i = 0; i < MAX_DMA_CHANNELS; i++) {
struct sdma_channel *sdmac = &sdma->channel[i];
- tasklet_kill(&sdmac->tasklet);
+ tasklet_kill(&sdmac->vc.task);
+ sdma_free_chan_resources(&sdmac->vc.chan);
}
platform_set_drvdata(pdev, NULL);
--
2.7.4
^ permalink raw reply related
* [PATCH v4 4/7] dmaengine: imx-sdma: remove useless 'lock' and 'enabled' in 'struct sdma_channel'
From: Robin Gong @ 2018-06-14 14:02 UTC (permalink / raw)
To: vkoul, s.hauer, l.stach, dan.j.williams, gregkh, jslaby
Cc: linux-serial, dmaengine, linux-kernel, linux-arm-kernel,
linux-imx
In-Reply-To: <1528984982-5074-1-git-send-email-yibin.gong@nxp.com>
Since 'sdmac->vc.lock' and 'sdmac->desc' can be used as 'lock' and
'enabled' in 'struct sdma_channel sdmac', remove them.
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
drivers/dma/imx-sdma.c | 23 -----------------------
1 file changed, 23 deletions(-)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 719bf9f..27b76eb 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -349,10 +349,8 @@ struct sdma_channel {
unsigned long event_mask[2];
unsigned long watermark_level;
u32 shp_addr, per_addr;
- spinlock_t lock;
enum dma_status status;
struct imx_dma_data data;
- bool enabled;
};
#define IMX_DMA_SG_LOOP BIT(0)
@@ -613,14 +611,7 @@ static int sdma_config_ownership(struct sdma_channel *sdmac,
static void sdma_enable_channel(struct sdma_engine *sdma, int channel)
{
- unsigned long flags;
- struct sdma_channel *sdmac = &sdma->channel[channel];
-
writel(BIT(channel), sdma->regs + SDMA_H_START);
-
- spin_lock_irqsave(&sdmac->lock, flags);
- sdmac->enabled = true;
- spin_unlock_irqrestore(&sdmac->lock, flags);
}
/*
@@ -738,14 +729,6 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
struct sdma_buffer_descriptor *bd;
int error = 0;
enum dma_status old_status = sdmac->status;
- unsigned long flags;
-
- spin_lock_irqsave(&sdmac->lock, flags);
- if (!sdmac->enabled) {
- spin_unlock_irqrestore(&sdmac->lock, flags);
- return;
- }
- spin_unlock_irqrestore(&sdmac->lock, flags);
/*
* loop mode. Iterate over descriptors, re-setup them and
@@ -1007,15 +990,10 @@ static int sdma_disable_channel(struct dma_chan *chan)
struct sdma_channel *sdmac = to_sdma_chan(chan);
struct sdma_engine *sdma = sdmac->sdma;
int channel = sdmac->channel;
- unsigned long flags;
writel_relaxed(BIT(channel), sdma->regs + SDMA_H_STATSTOP);
sdmac->status = DMA_ERROR;
- spin_lock_irqsave(&sdmac->lock, flags);
- sdmac->enabled = false;
- spin_unlock_irqrestore(&sdmac->lock, flags);
-
return 0;
}
@@ -1929,7 +1907,6 @@ static int sdma_probe(struct platform_device *pdev)
struct sdma_channel *sdmac = &sdma->channel[i];
sdmac->sdma = sdma;
- spin_lock_init(&sdmac->lock);
sdmac->channel = i;
sdmac->vc.desc_free = sdma_desc_free;
--
2.7.4
^ 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