* Re: [PATCH v2] ARM: dts: Add missing CPU frequencies for Exynos5422/5800
From: Markus Reichl @ 2016-12-15 13:55 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
Cc: Kukjin Kim, Javier Martinez Canillas, Rob Herring, Mark Rutland,
Russell King, Doug Anderson, Andreas Faerber, Thomas Abraham,
Ben Gamari, Arjun K V, linux-samsung-soc, linux-arm-kernel,
linux-pm, devicetree, linux-kernel
In-Reply-To: <10512254.nyUcL0zgTP@amdc3058>
Hi Bartlomiej,
Am 15.12.2016 um 12:55 schrieb Bartlomiej Zolnierkiewicz:
> Add missing 2000MHz & 1900MHz OPPs (for A15 cores) and 1400MHz OPP
> (for A7 cores). Also update common Odroid-XU3 Lite/XU3/XU4 thermal
> cooling maps to account for new OPPs.
>
> Since new OPPs are not available on all Exynos5422/5800 boards modify
> dts files for Odroid-XU3 Lite (limited to 1.8 GHz / 1.3 GHz) & Peach
> Pi (limited to 2.0 GHz / 1.3 GHz) accordingly.
>
> Tested on Odroid-XU3 and XU3 Lite.
>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> Cc: Andreas Faerber <afaerber@suse.de>
> Cc: Thomas Abraham <thomas.ab@samsung.com>
> Cc: Ben Gamari <ben@smart-cactus.org>
> Cc: Arjun K V <arjun.kv@samsung.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
> v2:
> - added comments about limitations of SoC revisions used by Odroid-XU3 Lite and
> Peach Pi boards (suggested by Javier)
> - removed redundant opp_a7_14 label
> - added Arjun to Cc:
>
> Javier, could you test it on Peach Pi board?
>
> arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi | 14 ++++++-------
> arch/arm/boot/dts/exynos5422-odroidxu3-lite.dts | 22 +++++++++++++++++++++
> arch/arm/boot/dts/exynos5800-peach-pi.dts | 9 ++++++++
> arch/arm/boot/dts/exynos5800.dtsi | 15 ++++++++++++++
> 4 files changed, 53 insertions(+), 7 deletions(-)
>
> Index: b/arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi
> ===================================================================
> --- a/arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi 2016-12-15 12:43:54.365955950 +0100
> +++ b/arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi 2016-12-15 12:43:54.361955949 +0100
> @@ -118,7 +118,7 @@
> /*
> * When reaching cpu_alert3, reduce CPU
> * by 2 steps. On Exynos5422/5800 that would
> - * be: 1600 MHz and 1100 MHz.
> + * (usually) be: 1800 MHz and 1200 MHz.
> */
> map3 {
> trip = <&cpu_alert3>;
> @@ -131,16 +131,16 @@
>
> /*
> * When reaching cpu_alert4, reduce CPU
> - * further, down to 600 MHz (11 steps for big,
> - * 7 steps for LITTLE).
> + * further, down to 600 MHz (13 steps for big,
> + * 8 steps for LITTLE).
> */
> - map5 {
> + cooling_map5: map5 {
> trip = <&cpu_alert4>;
> - cooling-device = <&cpu0 3 7>;
> + cooling-device = <&cpu0 3 8>;
> };
> - map6 {
> + cooling_map6: map6 {
> trip = <&cpu_alert4>;
> - cooling-device = <&cpu4 3 11>;
> + cooling-device = <&cpu4 3 13>;
> };
> };
> };
> Index: b/arch/arm/boot/dts/exynos5422-odroidxu3-lite.dts
> ===================================================================
> --- a/arch/arm/boot/dts/exynos5422-odroidxu3-lite.dts 2016-12-15 12:43:54.365955950 +0100
> +++ b/arch/arm/boot/dts/exynos5422-odroidxu3-lite.dts 2016-12-15 12:43:54.361955949 +0100
> @@ -21,6 +21,28 @@
> compatible = "hardkernel,odroid-xu3-lite", "samsung,exynos5800", "samsung,exynos5";
> };
>
> +/*
> + * Odroid XU3-Lite board uses SoC revision with lower maximum frequencies
> + * than Odroid XU3/XU4 boards: 1.8 GHz for A15 cores & 1.3 GHz for A7 cores.
> + * Therefore we need to update OPPs tables and thermal maps accordingly.
> + */
> +&cluster_a15_opp_table {
> + /delete-node/opp@2000000000;
> + /delete-node/opp@1900000000;
> +};
> +
> +&cluster_a7_opp_table {
> + /delete-node/opp@1400000000;
> +};
> +
> +&cooling_map5 {
> + cooling-device = <&cpu0 3 7>;
> +};
> +
> +&cooling_map6 {
> + cooling-device = <&cpu4 3 11>;
> +};
> +
> &pwm {
> /*
> * PWM 0 -- fan
> Index: b/arch/arm/boot/dts/exynos5800-peach-pi.dts
> ===================================================================
> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts 2016-12-15 12:43:54.365955950 +0100
> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts 2016-12-15 12:43:54.361955949 +0100
> @@ -146,6 +146,15 @@
> vdd-supply = <&ldo9_reg>;
> };
>
> +/*
> + * Peach Pi board uses SoC revision with lower maximum frequency for A7 cores
> + * (1.3 GHz instead of 1.4 GHz) than Odroid XU3/XU4 boards. Thus we need to
> + * update A7 OPPs table accordingly.
> + */
> +&cluster_a7_opp_table {
> + /delete-property/opp@1400000000;
> +};
> +
> &cpu0 {
> cpu-supply = <&buck2_reg>;
> };
> Index: b/arch/arm/boot/dts/exynos5800.dtsi
> ===================================================================
> --- a/arch/arm/boot/dts/exynos5800.dtsi 2016-12-15 12:43:54.365955950 +0100
> +++ b/arch/arm/boot/dts/exynos5800.dtsi 2016-12-15 12:43:54.361955949 +0100
> @@ -24,6 +24,16 @@
> };
>
> &cluster_a15_opp_table {
> + opp@2000000000 {
> + opp-hz = /bits/ 64 <2000000000>;
> + opp-microvolt = <1250000>;
> + clock-latency-ns = <140000>;
> + };
> + opp@1900000000 {
> + opp-hz = /bits/ 64 <1900000000>;
> + opp-microvolt = <1250000>;
> + clock-latency-ns = <140000>;
> + };
> opp@1700000000 {
> opp-microvolt = <1250000>;
> };
> @@ -85,6 +95,11 @@
> };
>
> &cluster_a7_opp_table {
> + opp@1400000000 {
> + opp-hz = /bits/ 64 <1400000000>;
> + opp-microvolt = <1250000>;
> + clock-latency-ns = <140000>;
> + };
> opp@1300000000 {
> opp-microvolt = <1250000>;
> };
>
On Odroid XU4, XU3 and XU3-lite:
Tested-by: Markus Reichl <m.reichl@fivetechno.de>
Thanks,
--
Markus Reichl
^ permalink raw reply
* Re: [PATCH 2/2] xilinx_dma: Add reset support
From: Laurent Pinchart @ 2016-12-15 13:56 UTC (permalink / raw)
To: Ramiro Oliveira
Cc: dmaengine-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
vinod.koul-ral2JQCrhuEAvxtiuMwx3w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, michal.simek-gjFFaj9aHVfQT0dZR+AlfA,
soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA,
appana.durga.rao-gjFFaj9aHVfQT0dZR+AlfA,
anuragku-gjFFaj9aHVfQT0dZR+AlfA,
dan.j.williams-ral2JQCrhuEAvxtiuMwx3w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w, Philipp Zabel
In-Reply-To: <380d1b11-814d-0164-3d49-e976f2deb3f9-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
Hi Ramiro,
(CC'ing Philipp Zabel)
On Thursday 15 Dec 2016 11:26:54 Ramiro Oliveira wrote:
> On 12/14/2016 8:16 PM, Laurent Pinchart wrote:
> > Hi Ramiro,
> >
> > Thank you for the patch.
> >
> > On Wednesday 14 Dec 2016 17:18:24 Ramiro Oliveira wrote:
> >> Add a DT property to control an optional external reset line
> >>
> >> Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> >> ---
> >>
> >> drivers/dma/xilinx/xilinx_dma.c | 23 +++++++++++++++++++++++
> >> 1 file changed, 23 insertions(+)
> >>
> >> diff --git a/drivers/dma/xilinx/xilinx_dma.c
> >> b/drivers/dma/xilinx/xilinx_dma.c index 5c9f11b..b845224 100644
> >> --- a/drivers/dma/xilinx/xilinx_dma.c
> >> +++ b/drivers/dma/xilinx/xilinx_dma.c
> >> @@ -46,6 +46,7 @@
> >> #include <linux/slab.h>
> >> #include <linux/clk.h>
> >> #include <linux/io-64-nonatomic-lo-hi.h>
> >> +#include <linux/reset.h>
> >
> > I had neatly sorted the header alphabetically until someone added clk.h
> > and io-64-nonatomic-lo-hi.h :-( Could you please move reset.h just before
> > slab.h ?
>
> Sure. Actually I was tempted to reorder it, but I decided not to do it. I'll
> do it now
Yeah, I'll sleep better tonight :-D
> >> #include "../dmaengine.h"
> >>
> >> @@ -409,6 +410,7 @@ struct xilinx_dma_device {
> >> struct clk *rxs_clk;
> >> u32 nr_channels;
> >> u32 chan_id;
> >> + struct reset_control *rst;
> >> };
> >>
> >> /* Macros */
> >> @@ -2543,6 +2545,27 @@ static int xilinx_dma_probe(struct platform_device
> >> *pdev) if (IS_ERR(xdev->regs))
> >> return PTR_ERR(xdev->regs);
> >>
> >> + xdev->rst = devm_reset_control_get_optional(&pdev->dev, "reset");
> >
> > devm_reset_control_get_optional() is deprecated as explained in
> > linux/reset.h, you should use devm_reset_control_get_optional_exclusive()
> > or devm_reset_control_get_optional_shared() instead, as applicable.
> >
> > This being said, I'm wondering why the optional versions of those
> > functions exist, as they do exactly the same as the non-optional versions.
> > The API feels wrong, it should have been modelled like the GPIO API. Feel
> > free to fix it if you want :-) But that's out of scope for this patch.
>
> I missed the comment stating that devm_reset_control_get_optional() was
> deprecated.
>
> I could fix it. Your sugestion is modelling these functions like the GPIO
> API?
I think it would be better for driver if the _get_optional functions would
return an ERR_PTR() for errors and NULL when reset control is not available,
and then have the rest of the reset controller API accept NULL as a no-op.
Your implementation would then be
xdev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
"reset");
if (IS_ERR(xdev->rst)) {
err = PTR_ERR(xdev->rst);
if (err != -EPROBE_DEFER)
dev_err(xdev->dev, "error getting reset %d\n", err);
return err;
}
err = reset_control_deassert(xdev->rst);
if (err) {
dev_err(xdev->dev, "failed to deassert reset: %d\n", err);
return err;
}
That requires modifying the reset controller API, so it's a bit out-of-scope,
but if you could give it a go, it would be great.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2] i2c: designware: Cleaning and comment style fixes.
From: Luis Oliveira @ 2016-12-15 14:38 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
The purpose of this commit is to fix some comments and styling issues in the
existing code due to the need of reuse this code. What is being made here is:
- Sorted the headers files
- Corrected some comments style
- Reverse tree in the variables declaration
- Add/remove empty lines and tabs where needed
- Fix of a misspelled word
Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V1->V2: (Andy Shevchenko)
- Removed all the unneeded dots as suggested
- I checked and "acknowledgement" and "acknowledgment"" are both correct
drivers/i2c/busses/i2c-designware-core.c | 36 ++++++++++++++---------------
drivers/i2c/busses/i2c-designware-core.h | 2 +-
drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++++++++++-----------
3 files changed, 33 insertions(+), 32 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 6d81c56184d3..254d82bb6bc3 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -21,17 +21,17 @@
* ----------------------------------------------------------------------------
*
*/
+#include <linux/delay.h>
#include <linux/export.h>
#include <linux/errno.h>
#include <linux/err.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
#include <linux/io.h>
-#include <linux/pm_runtime.h>
-#include <linux/delay.h>
#include <linux/module.h>
-#include "i2c-designware-core.h"
+#include <linux/pm_runtime.h>
+#include "i2c-designware-core.h"
/*
* Registers offset
*/
@@ -98,7 +98,7 @@
#define DW_IC_ERR_TX_ABRT 0x1
-#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
+#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
#define DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH (BIT(2) | BIT(3))
#define DW_IC_COMP_PARAM_1_SPEED_MODE_MASK GENMASK(3, 2)
@@ -113,10 +113,10 @@
#define TIMEOUT 20 /* ms */
/*
- * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
+ * Hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
*
- * only expected abort codes are listed here
- * refer to the datasheet for the full list
+ * Only expected abort codes are listed here
+ * refer to the datasheet for the full list.
*/
#define ABRT_7B_ADDR_NOACK 0
#define ABRT_10ADDR1_NOACK 1
@@ -338,14 +338,14 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
reg = dw_readl(dev, DW_IC_COMP_TYPE);
if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
- /* Configure register endianess access */
+ /* Configure register endianness access */
dev->accessor_flags |= ACCESS_SWAP;
} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
/* Configure register access mode 16bit */
dev->accessor_flags |= ACCESS_16BIT;
} else if (reg != DW_IC_COMP_TYPE_VALUE) {
- dev_err(dev->dev, "Unknown Synopsys component type: "
- "0x%08x\n", reg);
+ dev_err(dev->dev,
+ "Unknown Synopsys component type: 0x%08x\n", reg);
i2c_dw_release_lock(dev);
return -ENODEV;
}
@@ -355,7 +355,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
/* Disable the adapter */
__i2c_dw_enable_and_wait(dev, false);
- /* set standard and fast speed deviders for high/low periods */
+ /* Set standard and fast speed deviders for high/low periods */
sda_falling_time = dev->sda_falling_time ?: 300; /* ns */
scl_falling_time = dev->scl_falling_time ?: 300; /* ns */
@@ -444,7 +444,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
- /* configure the i2c master */
+ /* Configure the I2C master */
dw_writel(dev, dev->master_cfg , DW_IC_CON);
i2c_dw_release_lock(dev);
@@ -480,7 +480,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
/* Disable the adapter */
__i2c_dw_enable_and_wait(dev, false);
- /* if the slave address is ten bit address, enable 10BITADDR */
+ /* If the slave address is ten bit address, enable 10BITADDR */
if (dev->dynamic_tar_update_enabled) {
/*
* If I2C_DYNAMIC_TAR_UPDATE is set, the 10-bit addressing
@@ -505,7 +505,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
*/
dw_writel(dev, msgs[dev->msg_write_idx].addr | ic_tar, DW_IC_TAR);
- /* enforce disabled interrupts (due to HW issues) */
+ /* Enforce disabled interrupts (due to HW issues) */
i2c_dw_disable_int(dev);
/* Enable the adapter */
@@ -539,7 +539,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
u32 flags = msgs[dev->msg_write_idx].flags;
/*
- * if target address has changed, we need to
+ * If target address has changed, we need to
* reprogram the target address in the i2c
* adapter when we are done with this transfer
*/
@@ -601,7 +601,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
- /* avoid rx buffer overrun */
+ /* Avoid rx buffer overrun */
if (dev->rx_outstanding >= dev->rx_fifo_depth)
break;
@@ -905,7 +905,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
/*
* Anytime TX_ABRT is set, the contents of the tx/rx
- * buffers are flushed. Make sure to skip them.
+ * buffers are flushed. Make sure to skip them.
*/
dw_writel(dev, 0, DW_IC_INTR_MASK);
goto tx_aborted;
@@ -927,7 +927,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err)
complete(&dev->cmd_complete);
else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
- /* workaround to trigger pending interrupt */
+ /* Workaround to trigger pending interrupt */
stat = dw_readl(dev, DW_IC_INTR_MASK);
i2c_dw_disable_int(dev);
dw_writel(dev, stat, DW_IC_INTR_MASK);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 26250b425e2f..3cb81fca7738 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -36,7 +36,7 @@
#define DW_IC_CON_SPEED_FAST 0x4
#define DW_IC_CON_SPEED_HIGH 0x6
#define DW_IC_CON_SPEED_MASK 0x6
-#define DW_IC_CON_10BITADDR_MASTER 0x10
+#define DW_IC_CON_10BITADDR_MASTER 0x10
#define DW_IC_CON_RESTART_EN 0x20
#define DW_IC_CON_SLAVE_DISABLE 0x40
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 08153ea4d848..886e39753fc2 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -21,26 +21,27 @@
* ----------------------------------------------------------------------------
*
*/
-#include <linux/kernel.h>
-#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/clk-provider.h>
+#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/dmi.h>
-#include <linux/i2c.h>
-#include <linux/clk.h>
-#include <linux/clk-provider.h>
-#include <linux/errno.h>
-#include <linux/sched.h>
#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
#include <linux/of.h>
+#include <linux/platform_data/i2c-designware.h>
#include <linux/platform_device.h>
#include <linux/pm.h>
#include <linux/pm_runtime.h>
#include <linux/property.h>
-#include <linux/io.h>
+#include <linux/sched.h>
#include <linux/slab.h>
-#include <linux/acpi.h>
-#include <linux/platform_data/i2c-designware.h>
+
#include "i2c-designware-core.h"
static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
@@ -153,11 +154,11 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
static int dw_i2c_plat_probe(struct platform_device *pdev)
{
struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
- struct dw_i2c_dev *dev;
struct i2c_adapter *adap;
+ struct dw_i2c_dev *dev;
+ u32 acpi_speed, ht = 0;
struct resource *mem;
int irq, r;
- u32 acpi_speed, ht = 0;
irq = platform_get_irq(pdev, 0);
if (irq < 0)
@@ -354,7 +355,7 @@ static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
#define DW_I2C_DEV_PMOPS NULL
#endif
-/* work with hotplug and coldplug */
+/* Work with hotplug and coldplug */
MODULE_ALIAS("platform:i2c_designware");
static struct platform_driver dw_i2c_driver = {
--
2.11.0
^ permalink raw reply related
* [GIT PULL] DeviceTree updates for 4.10
From: Rob Herring @ 2016-12-15 14:51 UTC (permalink / raw)
To: Linus Torvalds
Cc: Frank Rowand, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Hi Linus,
Please pull. There's a trivial conflict in vendor-prefixes.txt. The
resolution is take both lines, but 'nvd' is also not sorted correctly
and it should come after 'nuvoton'.
Rob
The following changes since commit bc33b0ca11e3df467777a4fa7639ba488c9d4911:
Linux 4.9-rc4 (2016-11-05 16:23:36 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
tags/devicetree-for-4.10
for you to fetch changes up to 61eb3a04f9789c5e4f7498a2f171b82a567cea6b:
dt: pwm: bcm2835: fix typo in clocks property name (2016-12-09 15:37:58 -0600)
----------------------------------------------------------------
DeviceTree updates for 4.10:
- Add various vendor prefixes.
- Fix NUMA node handling when "numa=off" is passed on kernel command
line.
- Coding style Clean-up of overlay handling code.
- DocBook fixes in DT platform driver code
- Altera SoCFPGA binding addtions for freeze bridge, arria10 FPGA
manager and FPGA bridges.
- Couple of printk message fixes.
----------------------------------------------------------------
Alan Tull (3):
ARM: socfpga: add bindings document for fpga bridge drivers
ARM: socfpga: add bindings doc for arria10 fpga manager
add bindings document for altera freeze bridge
David Daney (1):
of, numa: Return NUMA_NO_NODE from disable of_node_to_nid() if
nid not possible.
Dinh Nguyen (3):
dt-bindings: Add Macnica Americas vendor prefix
dt-bindings: Add vendor prefix for Terasic Inc.
dt-bindings: Add vendor prefix for Samtec
Fabio Estevam (1):
dt-bindings: Add vendor prefix for Udoo
Frank Rowand (12):
of: Remove comments that state the obvious, to reduce clutter
of: Remove excessive printks to reduce clutter.
of: Convert comparisons to zero or NULL to logical expressions
of: Rename functions to more accurately reflect what they do
of: Remove prefix "__of_" from local function names
of: Rename variables to better reflect purpose or follow convention
of: Update structure of code to be clearer, also remove BUG_ON()
of: Remove redundant size check
of: Update comments to reflect changes and increase clarity
of: Add back an error message, restructured
of: Move setting of pointer to beside test for non-null
of: Remove unused variable overlay_symbols
Geert Uytterhoeven (2):
dt/bindings: arm-boards: Remove skeleton.dtsi inclusion from example
devicetree: bindings: Add vendor prefix for Oki
Greentime Hu (1):
devicetree: bindings: Add vendor prefix for Andes Technology Corporation
Johan Hovold (2):
of/platform: fix of_platform_device_destroy comment
of/platform: clarify of_find_device_by_node refcounting
Marcin Nowakowski (1):
drivers/of: fix missing pr_cont()s in of_print_phandle_args
Marek Vasut (1):
of: Add vendor prefix for Aries Embedded GmbH
Moritz Fischer (1):
of: Fix issue where code would fall through to error case.
Nathan Sullivan (1):
devicetree: add vendor prefix for National Instruments
Rob Herring (1):
Revert "of: base: add support to get machine model name"
Sudeep Holla (1):
of: base: add support to get machine model name
Vladimir Zapolskiy (2):
dt-bindings: add MYIR Tech hardware vendor prefix
dt: pwm: bcm2835: fix typo in clocks property name
Documentation/devicetree/bindings/arm/arm-boards | 3 +-
.../bindings/fpga/altera-fpga2sdram-bridge.txt | 16 +
.../bindings/fpga/altera-freeze-bridge.txt | 23 ++
.../bindings/fpga/altera-hps2fpga-bridge.txt | 39 +++
.../bindings/fpga/altera-socfpga-a10-fpga-mgr.txt | 19 ++
.../devicetree/bindings/pwm/pwm-bcm2835.txt | 2 +-
.../devicetree/bindings/vendor-prefixes.txt | 9 +
drivers/of/base.c | 9 +-
drivers/of/of_numa.c | 7 +-
drivers/of/platform.c | 6 +-
drivers/of/resolver.c | 358 +++++++++------------
11 files changed, 277 insertions(+), 214 deletions(-)
create mode 100644
Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
create mode 100644
Documentation/devicetree/bindings/fpga/altera-freeze-bridge.txt
create mode 100644
Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
create mode 100644
Documentation/devicetree/bindings/fpga/altera-socfpga-a10-fpga-mgr.txt
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] i2c: designware: Cleaning and comment style fixes.
From: Andy Shevchenko @ 2016-12-15 14:56 UTC (permalink / raw)
To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <d7b5a22ce09539c2af343c21ca69675bef6a0a10.1481812563.git.lolivei@synopsys.com>
On Thu, 2016-12-15 at 14:38 +0000, Luis Oliveira wrote:
> The purpose of this commit is to fix some comments and styling issues
> in the
> existing code due to the need of reuse this code. What is being made
> here is:
>
> - Sorted the headers files
> - Corrected some comments style
> - Reverse tree in the variables declaration
> - Add/remove empty lines and tabs where needed
> - Fix of a misspelled word
>
So, I'm okay with the change as long as no-one objecting it. As I
mentioned earlier it might rise concerns and better if you put the
answers to the commit message.
Also, comment below.
> @@ -113,10 +113,10 @@
> #define TIMEOUT 20 /* ms */
>
> /*
> - * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
> + * Hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
> *
> - * only expected abort codes are listed here
> - * refer to the datasheet for the full list
> + * Only expected abort codes are listed here
> + * refer to the datasheet for the full list.
Feels either comma is missing, or they are two different sentences.
> */
> #define ABRT_7B_ADDR_NOACK 0
> #define ABRT_10ADDR1_NOACK 1
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH] of/platform: depopulate devices in the reverse order of creation
From: Rob Herring @ 2016-12-15 14:57 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Frank Rowand, Pawel Moll, Grant Likely,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20161214214012.GB6947@obsidianresearch.com>
On Wed, Dec 14, 2016 at 3:40 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Dec 14, 2016 at 02:54:02PM -0600, Rob Herring wrote:
>> On Mon, Dec 12, 2016 at 12:39 PM, Jason Gunthorpe
>> <jgunthorpe@obsidianresearch.com> wrote:
>> > If the DT has inter-dependencies, then the devices need to be removed
>> > in the right order to avoid removal problems.
>> >
>> > Assuming the DT is constructed so that EPROBE_DEFER doesn't happen
>> > during creating then a good way to avoid removal problems is reversing
>> > the order during depopulation.
>>
>> I assume you mean by sorting the nodes to get lucky with the init
>> order.
>
> Not quite, the init order doesn't matter, just that the device list/DT
> is ordered topologically. The driver bind order can still be
> randomized. No luck needed.
I meant init in a generic sense
>
>> > In my specific case I have a gpio driver, followed by a i2c bitbang
>> > using that driver. So gpiolib prints an error if it the gpio driver is
>> > removed before the gpio client..
>>
>> Good news, functional dependencies are going into 4.10. Let's fix GPIO
>> and use that.
>
> Sure, but it will be some time until that is used everwhere..
>
> You don't think there is some value in 'hiding' the problem with
> ordering until that work is done? IIRC that was essentially how
> EPROBE_DEFER was introduced?
>
> Jason
^ permalink raw reply
* Re: [PATCH 1/2] devicetree: power: add bindings for GPIO-driven power switches
From: Rob Herring @ 2016-12-15 15:05 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Mark Rutland,
linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-devicetree, LKML, Kevin Hilman, Patrick Titiano,
Neil Armstrong, Linus Walleij, Alexandre Courbot, linux-gpio,
Sebastian Reichel, linux-pm, Mark Brown, Liam Girdwood
In-Reply-To: <CAMpxmJWw3wCE4ch8TVik+2b3BC8Lvxbi13HGd9GxruSRLu95Mg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Dec 14, 2016 at 10:58 AM, Bartosz Golaszewski
<bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
> 2016-12-13 20:27 GMT+01:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>> On Sun, Dec 11, 2016 at 11:21:44PM +0100, Bartosz Golaszewski wrote:
>>> Some boards are equipped with simple, GPIO-driven power load switches.
>>> An example of such ICs is the TI tps229* series.
>>
>> How is this different than a GPIO regulator? The input and output
>> voltages just happen to be the same. I could be convinced this is
>> different enough to have a different compatible, but it somewhat seems
>> you want to use this for IIO, so you are creating a different binding
>> for that usecase.
>>
>
> It's more of a fixed regulator I suppose. Do you mean adding a new
> compatible to the fixed-regulator binding (e.g. "gpio-power-switch" or
> "simple-power-switch") and then providing an iio driver for toggling
> the switch?
Yes, at least the first part. I view the switch as just a more
specific subtype of a fixed-regulator. Whether an IIO driver is a
separate discussion which is happening.
Rob
P.S. I really don't like compatibles with "simple".
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v3] i2c: designware: Cleaning and comment style fixes.
From: Luis Oliveira @ 2016-12-15 15:09 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
The purpose of this commit is to fix some comments and styling issues in the
existing code due to the need of reuse this code. What is being made here is:
- Sorted the headers files
- Corrected some comments style
- Reverse tree in the variables declaration
- Add/remove empty lines and tabs where needed
- Fix of a misspelled word
The value of this, besides the rules of coding style, is because I
will use this code after and it will make my future patch a lot bigger and
complicated to review. The work here won't bring any additional work to
backported fixes because is just style and reordering.
Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V2->V3: (Andy Shevchenko)
- Added the answers in the commit message.
- Added a comma, it seems to me it is the same sentence.
drivers/i2c/busses/i2c-designware-core.c | 36 ++++++++++++++---------------
drivers/i2c/busses/i2c-designware-core.h | 2 +-
drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++++++++++-----------
3 files changed, 33 insertions(+), 32 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 6d81c56184d3..9d724a6a7ec8 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -21,17 +21,17 @@
* ----------------------------------------------------------------------------
*
*/
+#include <linux/delay.h>
#include <linux/export.h>
#include <linux/errno.h>
#include <linux/err.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
#include <linux/io.h>
-#include <linux/pm_runtime.h>
-#include <linux/delay.h>
#include <linux/module.h>
-#include "i2c-designware-core.h"
+#include <linux/pm_runtime.h>
+#include "i2c-designware-core.h"
/*
* Registers offset
*/
@@ -98,7 +98,7 @@
#define DW_IC_ERR_TX_ABRT 0x1
-#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
+#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
#define DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH (BIT(2) | BIT(3))
#define DW_IC_COMP_PARAM_1_SPEED_MODE_MASK GENMASK(3, 2)
@@ -113,10 +113,10 @@
#define TIMEOUT 20 /* ms */
/*
- * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
+ * Hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
*
- * only expected abort codes are listed here
- * refer to the datasheet for the full list
+ * Only expected abort codes are listed here,
+ * refer to the datasheet for the full list.
*/
#define ABRT_7B_ADDR_NOACK 0
#define ABRT_10ADDR1_NOACK 1
@@ -338,14 +338,14 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
reg = dw_readl(dev, DW_IC_COMP_TYPE);
if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
- /* Configure register endianess access */
+ /* Configure register endianness access */
dev->accessor_flags |= ACCESS_SWAP;
} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
/* Configure register access mode 16bit */
dev->accessor_flags |= ACCESS_16BIT;
} else if (reg != DW_IC_COMP_TYPE_VALUE) {
- dev_err(dev->dev, "Unknown Synopsys component type: "
- "0x%08x\n", reg);
+ dev_err(dev->dev,
+ "Unknown Synopsys component type: 0x%08x\n", reg);
i2c_dw_release_lock(dev);
return -ENODEV;
}
@@ -355,7 +355,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
/* Disable the adapter */
__i2c_dw_enable_and_wait(dev, false);
- /* set standard and fast speed deviders for high/low periods */
+ /* Set standard and fast speed deviders for high/low periods */
sda_falling_time = dev->sda_falling_time ?: 300; /* ns */
scl_falling_time = dev->scl_falling_time ?: 300; /* ns */
@@ -444,7 +444,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
- /* configure the i2c master */
+ /* Configure the I2C master */
dw_writel(dev, dev->master_cfg , DW_IC_CON);
i2c_dw_release_lock(dev);
@@ -480,7 +480,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
/* Disable the adapter */
__i2c_dw_enable_and_wait(dev, false);
- /* if the slave address is ten bit address, enable 10BITADDR */
+ /* If the slave address is ten bit address, enable 10BITADDR */
if (dev->dynamic_tar_update_enabled) {
/*
* If I2C_DYNAMIC_TAR_UPDATE is set, the 10-bit addressing
@@ -505,7 +505,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
*/
dw_writel(dev, msgs[dev->msg_write_idx].addr | ic_tar, DW_IC_TAR);
- /* enforce disabled interrupts (due to HW issues) */
+ /* Enforce disabled interrupts (due to HW issues) */
i2c_dw_disable_int(dev);
/* Enable the adapter */
@@ -539,7 +539,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
u32 flags = msgs[dev->msg_write_idx].flags;
/*
- * if target address has changed, we need to
+ * If target address has changed, we need to
* reprogram the target address in the i2c
* adapter when we are done with this transfer
*/
@@ -601,7 +601,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
- /* avoid rx buffer overrun */
+ /* Avoid rx buffer overrun */
if (dev->rx_outstanding >= dev->rx_fifo_depth)
break;
@@ -905,7 +905,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
/*
* Anytime TX_ABRT is set, the contents of the tx/rx
- * buffers are flushed. Make sure to skip them.
+ * buffers are flushed. Make sure to skip them.
*/
dw_writel(dev, 0, DW_IC_INTR_MASK);
goto tx_aborted;
@@ -927,7 +927,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err)
complete(&dev->cmd_complete);
else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
- /* workaround to trigger pending interrupt */
+ /* Workaround to trigger pending interrupt */
stat = dw_readl(dev, DW_IC_INTR_MASK);
i2c_dw_disable_int(dev);
dw_writel(dev, stat, DW_IC_INTR_MASK);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 26250b425e2f..3cb81fca7738 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -36,7 +36,7 @@
#define DW_IC_CON_SPEED_FAST 0x4
#define DW_IC_CON_SPEED_HIGH 0x6
#define DW_IC_CON_SPEED_MASK 0x6
-#define DW_IC_CON_10BITADDR_MASTER 0x10
+#define DW_IC_CON_10BITADDR_MASTER 0x10
#define DW_IC_CON_RESTART_EN 0x20
#define DW_IC_CON_SLAVE_DISABLE 0x40
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 08153ea4d848..886e39753fc2 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -21,26 +21,27 @@
* ----------------------------------------------------------------------------
*
*/
-#include <linux/kernel.h>
-#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/clk-provider.h>
+#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/dmi.h>
-#include <linux/i2c.h>
-#include <linux/clk.h>
-#include <linux/clk-provider.h>
-#include <linux/errno.h>
-#include <linux/sched.h>
#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
#include <linux/of.h>
+#include <linux/platform_data/i2c-designware.h>
#include <linux/platform_device.h>
#include <linux/pm.h>
#include <linux/pm_runtime.h>
#include <linux/property.h>
-#include <linux/io.h>
+#include <linux/sched.h>
#include <linux/slab.h>
-#include <linux/acpi.h>
-#include <linux/platform_data/i2c-designware.h>
+
#include "i2c-designware-core.h"
static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
@@ -153,11 +154,11 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
static int dw_i2c_plat_probe(struct platform_device *pdev)
{
struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
- struct dw_i2c_dev *dev;
struct i2c_adapter *adap;
+ struct dw_i2c_dev *dev;
+ u32 acpi_speed, ht = 0;
struct resource *mem;
int irq, r;
- u32 acpi_speed, ht = 0;
irq = platform_get_irq(pdev, 0);
if (irq < 0)
@@ -354,7 +355,7 @@ static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
#define DW_I2C_DEV_PMOPS NULL
#endif
-/* work with hotplug and coldplug */
+/* Work with hotplug and coldplug */
MODULE_ALIAS("platform:i2c_designware");
static struct platform_driver dw_i2c_driver = {
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 1/1] of: of_reserved_mem: Ensure cma reserved region not cross the low/high memory
From: Jason Liu @ 2016-12-15 15:10 UTC (permalink / raw)
To: Rob Herring
Cc: Laura Abbott, Jason Liu, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Frank Rowand
In-Reply-To: <CAL_JsqJygJK_VmXKad8n63-jLH+G_xQ=NwX8JVmE=xSuYMof=Q@mail.gmail.com>
2016-12-15 21:54 GMT+08:00 Rob Herring <robh+dt@kernel.org>:
> On Wed, Dec 14, 2016 at 4:21 PM, Laura Abbott <labbott@redhat.com> wrote:
>> On 12/14/2016 12:45 PM, Rob Herring wrote:
>>> On Wed, Nov 23, 2016 at 5:37 AM, Jason Liu <jason.hui.liu@nxp.com> wrote:
>>>> Need ensure the cma reserved region not cross the low/high memory boundary
>>>> when using the dynamic allocation methond through device-tree, otherwise,
>>>> kernel will fail to boot up when cma reserved region cross how/high mem.
>>>
>>> The kernel command line code setting CMA already deals with this. Why
>>> don't we just call the CMA code (cma_declare_contiguous) to deal with
>>> this?
>>>
>>> Rob
>>>
>>
>> That was proposed in the first version[1] but I think this is a generic
>> problem not specific to CMA. Even non-CMA reservations trying to span
>> zones could cause problems so the devicetree allocation code should
>> restrict reservations to a single zone.
>
> Fair enough, but that's not what this patch does. It's only for CMA.
I'm only certain that the CMA reservation from the device-tree is not
working now.
and if you guys think that this is not only the CMA but also other
non-CMA reservations
should also have this restriction on the device-tree method. I can
change the patch the patch
as the followings.
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 366d8c3..7b8857d 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -31,11 +31,15 @@ static int reserved_mem_count;
#if defined(CONFIG_HAVE_MEMBLOCK)
#include <linux/memblock.h>
-int __init __weak early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
- phys_addr_t align, phys_addr_t start, phys_addr_t end, bool nomap,
- phys_addr_t *res_base)
+int __init __weak early_init_dt_alloc_reserved_memory_arch(unsigned long node,
+ phys_addr_t size, phys_addr_t align, phys_addr_t start, phys_addr_t end,
+ bool nomap, phys_addr_t *res_base)
{
phys_addr_t base;
+ phys_addr_t highmem_start;
+
+ highmem_start = __pa(high_memory - 1) + 1;
+
/*
* We use __memblock_alloc_base() because memblock_alloc_base()
* panic()s on allocation failure.
@@ -53,15 +57,29 @@ int __init __weak
early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
return -ENOMEM;
}
+ /*
+ * Sanity check for the reserved region:If the reserved region
+ * crosses the low/high memory boundary, try to fix it up and then
+ * fall back to allocate region from the low mememory space.
+ */
+
+ if (base < highmem_start && (base + size) > highmem_start) {
+ memblock_free(base, size);
+ base = memblock_alloc_range(size, align, start,
+ highmem_start, MEMBLOCK_NONE);
+ if (!base)
+ return -ENOMEM;
+ }
+
if you guys have good idea, please share or post your patch. This is
really an issue
that reserve memory from the device-tree is broken.
>
> Rob
^ permalink raw reply related
* Re: [PATCH v2] i2c: designware: Cleaning and comment style fixes.
From: Peter Rosin @ 2016-12-15 15:17 UTC (permalink / raw)
To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
andriy.shevchenko, mika.westerberg, linux-i2c, devicetree,
linux-kernel
Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <d7b5a22ce09539c2af343c21ca69675bef6a0a10.1481812563.git.lolivei@synopsys.com>
On 2016-12-15 15:38, Luis Oliveira wrote:
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 26250b425e2f..3cb81fca7738 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -36,7 +36,7 @@
> #define DW_IC_CON_SPEED_FAST 0x4
> #define DW_IC_CON_SPEED_HIGH 0x6
> #define DW_IC_CON_SPEED_MASK 0x6
> -#define DW_IC_CON_10BITADDR_MASTER 0x10
> +#define DW_IC_CON_10BITADDR_MASTER 0x10
How is this an improvement?
Cheers,
peda
^ permalink raw reply
* Re: [PATCH v6 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2016-12-15 15:26 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: devicetree, Alexandre Torgue, Wolfram Sang, linux-kernel,
Linus Walleij, Patrice Chotard, linux, Rob Herring, linux-i2c,
Maxime Coquelin, linux-arm-kernel
In-Reply-To: <20161213092031.d2ax2pnpzzicriel@pengutronix.de>
Hello,
Thanks for this review, it will help.
2016-12-13 10:20 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Mon, Dec 12, 2016 at 05:15:39PM +0100, M'boumba Cedric Madianga wrote:
>> This patch adds support for the STM32F4 I2C controller.
>>
>> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
>> ---
>> drivers/i2c/busses/Kconfig | 10 +
>> drivers/i2c/busses/Makefile | 1 +
>> drivers/i2c/busses/i2c-stm32f4.c | 849 +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 860 insertions(+)
>> create mode 100644 drivers/i2c/busses/i2c-stm32f4.c
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 0cdc844..2719208 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -886,6 +886,16 @@ config I2C_ST
>> This driver can also be built as module. If so, the module
>> will be called i2c-st.
>>
>> +config I2C_STM32F4
>> + tristate "STMicroelectronics STM32F4 I2C support"
>> + depends on ARCH_STM32 || COMPILE_TEST
>> + help
>> + Enable this option to add support for STM32 I2C controller embedded
>> + in STM32F4 SoCs.
>> +
>> + This driver can also be built as module. If so, the module
>> + will be called i2c-stm32f4.
>> +
>> config I2C_STU300
>> tristate "ST Microelectronics DDC I2C interface"
>> depends on MACH_U300
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 1c1bac8..a2c6ff5 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -85,6 +85,7 @@ obj-$(CONFIG_I2C_SH_MOBILE) += i2c-sh_mobile.o
>> obj-$(CONFIG_I2C_SIMTEC) += i2c-simtec.o
>> obj-$(CONFIG_I2C_SIRF) += i2c-sirf.o
>> obj-$(CONFIG_I2C_ST) += i2c-st.o
>> +obj-$(CONFIG_I2C_STM32F4) += i2c-stm32f4.o
>> obj-$(CONFIG_I2C_STU300) += i2c-stu300.o
>> obj-$(CONFIG_I2C_SUN6I_P2WI) += i2c-sun6i-p2wi.o
>> obj-$(CONFIG_I2C_TEGRA) += i2c-tegra.o
>> diff --git a/drivers/i2c/busses/i2c-stm32f4.c b/drivers/i2c/busses/i2c-stm32f4.c
>> new file mode 100644
>> index 0000000..89ad579
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-stm32f4.c
>> @@ -0,0 +1,849 @@
>> +/*
>> + * Driver for STMicroelectronics STM32 I2C controller
>> + *
>> + * Copyright (C) M'boumba Cedric Madianga 2015
>> + * Author: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
>> + *
>> + * This driver is based on i2c-st.c
>> + *
>> + * License terms: GNU General Public License (GPL), version 2
>> + */
>
> If there is a public description available for the device, a link here
> would be great.
The device is described in the reference manual of the STM32F429/439 SoC.
As this reference manual is public, I will add it at the beginning of
the driver as requested.
>
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +
>> +/* STM32F4 I2C offset registers */
>> +#define STM32F4_I2C_CR1 0x00
>> +#define STM32F4_I2C_CR2 0x04
>> +#define STM32F4_I2C_DR 0x10
>> +#define STM32F4_I2C_SR1 0x14
>> +#define STM32F4_I2C_SR2 0x18
>> +#define STM32F4_I2C_CCR 0x1C
>> +#define STM32F4_I2C_TRISE 0x20
>> +#define STM32F4_I2C_FLTR 0x24
>> +
>> +/* STM32F4 I2C control 1*/
>> +#define STM32F4_I2C_CR1_SWRST BIT(15)
>> +#define STM32F4_I2C_CR1_POS BIT(11)
>> +#define STM32F4_I2C_CR1_ACK BIT(10)
>> +#define STM32F4_I2C_CR1_STOP BIT(9)
>> +#define STM32F4_I2C_CR1_START BIT(8)
>> +#define STM32F4_I2C_CR1_PE BIT(0)
>> +
>> +/* STM32F4 I2C control 2 */
>> +#define STM32F4_I2C_CR2_FREQ_MASK GENMASK(5, 0)
>> +#define STM32F4_I2C_CR2_FREQ(n) ((n & STM32F4_I2C_CR2_FREQ_MASK))
>
> This should better be ((n) & STM32F4_I2C_CR2_FREQ_MASK). There a few
> more constants that need the same fix.
OK I will fix it in the V7. Thanks.
>
>> +#define STM32F4_I2C_CR2_ITBUFEN BIT(10)
>> +#define STM32F4_I2C_CR2_ITEVTEN BIT(9)
>> +#define STM32F4_I2C_CR2_ITERREN BIT(8)
>> +#define STM32F4_I2C_CR2_IRQ_MASK (STM32F4_I2C_CR2_ITBUFEN \
>> + | STM32F4_I2C_CR2_ITEVTEN \
>> + | STM32F4_I2C_CR2_ITERREN)
>
> I'd layout this like:
>
> #define STM32F4_I2C_CR2_IRQ_MASK (STM32F4_I2C_CR2_ITBUFEN | \
> STM32F4_I2C_CR2_ITEVTEN | \
> STM32F4_I2C_CR2_ITERREN)
>
> which is more usual I think.
OK I will fix it in the V7. Thanks.
>
>> +/* STM32F4 I2C Status 1 */
>> +#define STM32F4_I2C_SR1_AF BIT(10)
>> +#define STM32F4_I2C_SR1_ARLO BIT(9)
>> +#define STM32F4_I2C_SR1_BERR BIT(8)
>> +#define STM32F4_I2C_SR1_TXE BIT(7)
>> +#define STM32F4_I2C_SR1_RXNE BIT(6)
>> +#define STM32F4_I2C_SR1_BTF BIT(2)
>> +#define STM32F4_I2C_SR1_ADDR BIT(1)
>> +#define STM32F4_I2C_SR1_SB BIT(0)
>> +#define STM32F4_I2C_SR1_ITEVTEN_MASK (STM32F4_I2C_SR1_BTF \
>> + | STM32F4_I2C_SR1_ADDR \
>> + | STM32F4_I2C_SR1_SB)
>> +#define STM32F4_I2C_SR1_ITBUFEN_MASK (STM32F4_I2C_SR1_TXE \
>> + | STM32F4_I2C_SR1_RXNE)
>> +#define STM32F4_I2C_SR1_ITERREN_MASK (STM32F4_I2C_SR1_AF \
>> + | STM32F4_I2C_SR1_ARLO \
>> + | STM32F4_I2C_SR1_BERR)
>> +
>> +/* STM32F4 I2C Status 2 */
>> +#define STM32F4_I2C_SR2_BUSY BIT(1)
>> +
>> +/* STM32F4 I2C Control Clock */
>> +#define STM32F4_I2C_CCR_CCR_MASK GENMASK(11, 0)
>> +#define STM32F4_I2C_CCR_CCR(n) ((n & STM32F4_I2C_CCR_CCR_MASK))
>> +#define STM32F4_I2C_CCR_FS BIT(15)
>> +#define STM32F4_I2C_CCR_DUTY BIT(14)
>> +
>> +/* STM32F4 I2C Trise */
>> +#define STM32F4_I2C_TRISE_VALUE_MASK GENMASK(5, 0)
>> +#define STM32F4_I2C_TRISE_VALUE(n) ((n & STM32F4_I2C_TRISE_VALUE_MASK))
>> +
>> +/* STM32F4 I2C Filter */
>> +#define STM32F4_I2C_FLTR_DNF_MASK GENMASK(3, 0)
>> +#define STM32F4_I2C_FLTR_DNF(n) ((n & STM32F4_I2C_FLTR_DNF_MASK))
>> +#define STM32F4_I2C_FLTR_ANOFF BIT(4)
>> +
>> +#define STM32F4_I2C_MIN_FREQ 2U
>> +#define STM32F4_I2C_MAX_FREQ 42U
>> +#define FAST_MODE_MAX_RISE_TIME 1000
>> +#define STD_MODE_MAX_RISE_TIME 300
>
> Are these supposed to be the values "rise time of both SDA and SCL
> signals" from the i2c specification? If so, you got it wrong, fast mode
> has the smaller value.
Yes you are right. My mistake. Thanks
> Maybe these constants could get a home in a more central place?
Yes we probably could add these constants in a include file like i2c.h
or someting like that.
Wolfram, what is your opinion regarding this proposal ?
> Also I'd add /* ns */ to the definition.
Ok
>
>> +#define MHZ_TO_HZ 1000000
>> +
>> +enum stm32f4_i2c_speed {
>> + STM32F4_I2C_SPEED_STANDARD, /* 100 kHz */
>> + STM32F4_I2C_SPEED_FAST, /* 400 kHz */
>> + STM32F4_I2C_SPEED_END,
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_timings - per-Mode tuning parameters
>> + * @duty: Fast mode duty cycle
>> + * @mul_ccr: Value to be multiplied to CCR to reach 100Khz/400Khz SCL frequency
>> + * @min_ccr: Minimum clock ctrl reg value to reach 100Khz/400Khz SCL frequency
>> + */
>> +struct stm32f4_i2c_timings {
>> + u32 rate;
>
> rate is undocumented and unused.
Good point. I will remove it. Thanks.
>
>> + u32 duty;
>> + u32 mul_ccr;
>> + u32 min_ccr;
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_msg - client specific data
>> + * @addr: 8-bit slave addr, including r/w bit
>> + * @count: number of bytes to be transferred
>> + * @buf: data buffer
>> + * @result: result of the transfer
>> + * @stop: last I2C msg to be sent, i.e. STOP to be generated
>> + */
>> +struct stm32f4_i2c_msg {
>> + u8 addr;
>> + u32 count;
>> + u8 *buf;
>> + int result;
>> + bool stop;
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_dev - private data of the controller
>> + * @adap: I2C adapter for this controller
>> + * @dev: device for this controller
>> + * @base: virtual memory area
>> + * @complete: completion of I2C message
>> + * @irq_event: interrupt event line for the controller
>> + * @irq_error: interrupt error line for the controller
>> + * @clk: hw i2c clock
>> + * speed: I2C clock frequency of the controller. Standard or Fast only supported
>> + * @msg: I2C transfer information
>> + */
>> +struct stm32f4_i2c_dev {
>> + struct i2c_adapter adap;
>> + struct device *dev;
>> + void __iomem *base;
>> + struct completion complete;
>> + int irq_event;
>> + int irq_error;
>
> You only use irq_event in the probe function. So there is no need to
> remember this one and you could use a local variable instead.
Ok, I will fix it in the V7. Thanks
>
>> + struct clk *clk;
>> + int speed;
>> + struct stm32f4_i2c_msg msg;
>> +};
>> +
>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> + [STM32F4_I2C_SPEED_STANDARD] = {
>> + .mul_ccr = 1,
>> + .min_ccr = 4,
>> + .duty = 0,
>> + },
>> + [STM32F4_I2C_SPEED_FAST] = {
>> + .mul_ccr = 16,
>> + .min_ccr = 1,
>> + .duty = 1,
>> + },
>
> Are these values from the datasheet?
Yes, these values come from I2C IP datasheet.
>
>> +};
>> +
>> +static inline void stm32f4_i2c_set_bits(void __iomem *reg, u32 mask)
>> +{
>> + writel_relaxed(readl_relaxed(reg) | mask, reg);
>> +}
>> +
>> +static inline void stm32f4_i2c_clr_bits(void __iomem *reg, u32 mask)
>> +{
>> + writel_relaxed(readl_relaxed(reg) & ~mask, reg);
>> +}
>> +
>> +static void stm32f4_i2c_soft_reset(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_SWRST);
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_SWRST);
>
> Not very critical, but you're doing an unneeded register access here
> because the register is read twice.
>
> Also I think readability would improve if you dropped
> stm32f4_i2c_{set,clr}_bits and do their logic explicitly in the callers.
>
> stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_SWRST);
> stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_SWRST);
>
> vs
>
> val = readl_relaxed(reg);
> writel_relaxed(val | STM32F4_I2C_CR1_SWRST, reg);
> writel_relaxed(val, reg);
>
If it is just for this function, I don't have any objection to use
your proposal.
But if your request, it is to drop all calls of
stm32f4_i2c_{set,clr}_bits, I am wondering if it is something really
critical ?
Indeed, this is a big impact in this driver and I would prefer to avoid it.
>> +}
>> +
>> +static void stm32f4_i2c_disable_it(struct stm32f4_i2c_dev *i2c_dev)
>
> What is "it"? If it stands for "interrupt" the more usual abbrev is
> "irq".
Yes it stands for interrupt.
So, I will replace it by irq in the next version.
>
>> +{
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_IRQ_MASK);
>> +}
>> +
>> +static void stm32f4_i2c_set_periph_clk_freq(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 clk_rate, cr2, freq;
>> +
>> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
>> + clk_rate = clk_get_rate(i2c_dev->clk);
>> + freq = clk_rate / MHZ_TO_HZ;
>> + freq = clamp(freq, STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ);
>> + cr2 |= STM32F4_I2C_CR2_FREQ(freq);
>> + writel_relaxed(cr2, i2c_dev->base + STM32F4_I2C_CR2);
>
> Can you quote the data sheet enough in a comment here to make it obvious
> that your calculation is right?
Ok I will add it.
>
> Would it be more sensible to error out if clk_rate / MHZ_TO_HZ isn't in
> the interval [STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ]?
Yes, input clock must be between 2 MHz and 42 Mhz to achieve standard
or fast mode mode I²C frequencies.
If it is not the case, the I2C signal integrity is not guarantee and
could lead to communication issue between devices on the I2C bus.
>
> Usually I would expect that you need to use
> DIV_ROUND_UP(clk_rate, MHZ_TO_HZ) instead of a plain division.
Ok, I will use this macro in the next version
>
>> +}
>> +
>> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 trise, freq, cr2, val;
>> +
>> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
>> +
>> + trise = readl_relaxed(i2c_dev->base + STM32F4_I2C_TRISE);
>> + trise &= ~STM32F4_I2C_TRISE_VALUE_MASK;
>
> Are you required to use rmw for STM32F4_I2C_TRISE? I'd prefer
>
> writel_relaxed(STM32F4_I2C_TRISE_VALUE(..), i2c_dev->base + STM32F4_I2C_TRISE);
>
> unless the datasheet requires rmw.
>
>> + /* Maximum rise time computation */
>> + if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD) {
>> + trise |= STM32F4_I2C_TRISE_VALUE((freq + 1));
>
> A single pair of parenthesis is enough when you fix
> STM32F4_I2C_TRISE_VALUE as suggested above.
The datasheet does not required to use rmw so I will use your proposal
in the next version. Thanks.
>
>> + } else {
>> + val = freq * FAST_MODE_MAX_RISE_TIME / STD_MODE_MAX_RISE_TIME;
>> + trise |= STM32F4_I2C_TRISE_VALUE((val + 1));
>
> val could be local to this branch.
>
> Or make it shorter using:
>
> freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> if (i2c_dev->speed == STM32F4_I2C_SPEED_FAST)
> freq = freq * FAST_MODE_MAX_RISE_TIME / STD_MODE_MAX_RISE_TIME;
>
> writel_relaxed(STM32F4_I2C_TRISE_VALUE(freq + 1), ...);
>
> A quote from the data sheet about the algorithm would be good here, too.
Ok, I will use a shorter way to compute freq and add a quote from the datasheet
>
>> + }
>> +
>> + writel_relaxed(trise, i2c_dev->base + STM32F4_I2C_TRISE);
>> +}
>> +
>> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_timings *t = &i2c_timings[i2c_dev->speed];
>> + u32 ccr, clk_rate;
>> + int val;
>> +
>> + ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
>> + ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
>> + STM32F4_I2C_CCR_CCR_MASK);
>> +
>> + clk_rate = clk_get_rate(i2c_dev->clk);
>> + val = clk_rate / MHZ_TO_HZ * t->mul_ccr;
>
> Is the rounding done right? Again please describe the hardware in a
> comment.
Ok I will use DIV_ROUND_UP here and add a comment from datasheet
>
>> + if (val < t->min_ccr)
>> + val = t->min_ccr;
>> + ccr |= STM32F4_I2C_CCR_CCR(val);
>> +
>> + if (t->duty)
>> + ccr |= STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY;
>> +
>> + writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
>> +}
>> +[...]
>> +
>> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 status;
>> + int ret;
>> +
>> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> + status,
>> + !(status & STM32F4_I2C_SR2_BUSY),
>> + 10, 1000);
>> + if (ret) {
>> + dev_err(i2c_dev->dev, "bus not free\n");
>> + ret = -EBUSY;
>
> I'm not sure if "bus not free" deserves an error message. Wolfram?
>
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +[...]
>> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + u32 rbuf;
>> +
>> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>> + *msg->buf++ = (u8)rbuf & 0xff;
>
> unneeded cast (or unneeded & 0xff).
Ok thanks
>
>> + msg->count--;
>> +}
>> +
>> +[...]
>> +/**
>> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + switch (msg->count) {
>> + case 1:
>> + stm32f4_i2c_disable_it(i2c_dev);
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + complete(&i2c_dev->complete);
>> + break;
>> + case 2:
>> + case 3:
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> + break;
>> + default:
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + }
>
> It looks wrong that you don't call stm32f4_i2c_read_msg if msg->count is
> 2 or 3. I guess that's because these cases are handled in
> stm32f4_i2c_handle_rx_btf? Maybe you can simplify the logic a bit?
stm32f4_i2c_handle_read is called when RXNE bit is set due to new data
present in DR register.
stm32f4_i2c_handle_rx_btf is called when BTF bit is set.
This bit is set when there is new data in shift register whereas data
in DR register has not been read yet.
The datasheet requires to wait for BTF bit for 2 byte reception and
for the 3 last bytes to be read for N bytes reception (with N > 2)
That's why, in these cases (2 and 3), I clear the ITBUF interrupt in
order to not be preempted again for RXNE event as I know that I have
to wait for BTF event.
So, this is not a wrong case but I will add a comment to explain that.
>
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
>> + * in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> + u32 mask;
>> + int i;
>> +
>> + switch (msg->count) {
>
> I don't understand why the handling depends on the number of messages.
Please see my above comment
>
>> + case 2:
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + /* Generate STOP or REPSTART */
>
> I stumbled about "REPSTART" and would spell it out as "repeated Start".
Ok
>
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +
>> + /* Read two last data bytes */
>> + for (i = 2; i > 0; i--)
>> + stm32f4_i2c_read_msg(i2c_dev);
>> +
>> + /* Disable EVT and ERR interrupt */
>> + reg = i2c_dev->base + STM32F4_I2C_CR2;
>> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>> + stm32f4_i2c_clr_bits(reg, mask);
>> +
>> + complete(&i2c_dev->complete);
>> + break;
>> + case 3:
>> + /* Enable ACK and read data */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + break;
>> + default:
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
>> + * master receiver
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> +
>> + switch (msg->count) {
>> + case 0:
>> + stm32f4_i2c_terminate_xfer(i2c_dev);
>> + /* Clear ADDR flag */
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + break;
>> + case 1:
>> + /*
>> + * Single byte reception:
>> + * Enable NACK, clear ADDR flag and generate STOP or RepSTART
>> + */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> + break;
>> + case 2:
>> + /*
>> + * 2-byte reception:
>> + * Enable NACK and PEC Position Ack and clear ADDR flag
>
> What is PEC?
PEC stands for Packet Error Checking.
This feature is used is SMbus mode in order to guarantee data
integrity during an I2C transaction thanks to packet error code
comparaison.
The POS bit is used in reception to indicate that the next byte
received in the shift register will be ACK by hardware.
So, this is a wrong comment I would say POS ACK and I will fix it in
the next version.
>
>> + */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + break;
>> +
>> + default:
>> + /* N-byte reception: Enable ACK and clear ADDR flag */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + break;
>> + }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_isr_event() - Interrupt routine for I2C bus event
>> + * @irq: interrupt number
>> + * @data: Controller's private data
>> + */
>> +static irqreturn_t stm32f4_i2c_isr_event(int irq, void *data)
>> +{
>> +[...]
>> + real_status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
>
> s/real_status/status/ ?
Ok. Thanks
>
>> +
>> + if (!(real_status & possible_status)) {
>> + dev_dbg(i2c_dev->dev,
>> + "spurious evt it (status=0x%08x, ien=0x%08x)\n",
>> + real_status, ien);
>
> s/it/irq/
Ok. Thanks
>
>> + return IRQ_NONE;
>> + }
>> +
>> + /* Use __fls() to check error bits first */
>> + flag = __fls(real_status & possible_status);
>
> If you get several events reported you only handle a single one. Is this
> effective?
You are right, if several events occur, I will execute this irq
routines for each event.
I will rework this in the next version. Thanks
>
>> + switch (1 << flag) {
>> + case STM32F4_I2C_SR1_SB:
>> + stm32f4_i2c_write_byte(i2c_dev, msg->addr);
>> + break;
>> +
>> + case STM32F4_I2C_SR1_ADDR:
>> + if (msg->addr & I2C_M_RD)
>> + stm32f4_i2c_handle_rx_addr(i2c_dev);
>> + else
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> +
>> + /* Enable ITBUF interrupts */
>
> What is ITBUF?
ITBUF is an interrupt generated when RxNE or TxE flag is set
>
>> + reg = i2c_dev->base + STM32F4_I2C_CR2;
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> + break;
>> +
>> + case STM32F4_I2C_SR1_BTF:
>> + if (msg->addr & I2C_M_RD)
>> + stm32f4_i2c_handle_rx_btf(i2c_dev);
>> + else
>> + stm32f4_i2c_handle_write(i2c_dev);
>> + break;
>> +
>> + case STM32F4_I2C_SR1_TXE:
>> + stm32f4_i2c_handle_write(i2c_dev);
>> + break;
>> +
>> + case STM32F4_I2C_SR1_RXNE:
>> + stm32f4_i2c_handle_read(i2c_dev);
>> + break;
>> +
>> + default:
>> + dev_err(i2c_dev->dev,
>> + "evt it unhandled: status=0x%08x)\n", real_status);
>
> s/it/irq/
ok
>
>> + return IRQ_NONE;
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +[...]
>> +static int stm32f4_i2c_xfer_msg(struct stm32f4_i2c_dev *i2c_dev,
>> + struct i2c_msg *msg, bool is_first,
>> + bool is_last)
>> +{
>> +[...]
>> + /* Enable ITEVT and ITERR interrupts */
>
> This comment isn't helpful. Mentioning their meaning would be great
> instead.
ok I will add it (ITEVT = event interrupt and ITERR = error interrupt)
>
>> +[...]
>> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
>> + int num)
>> +{
>> + struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>> + int ret, i;
>> +
>> + ret = clk_enable(i2c_dev->clk);
>> + if (ret) {
>> + dev_err(i2c_dev->dev, "Failed to enable clock\n");
>> + return ret;
>> + }
>> +
>> + stm32f4_i2c_hw_config(i2c_dev);
>> +
>> + for (i = 0; i < num && !ret; i++)
>> + ret = stm32f4_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0,
>> + i == num - 1);
>> +
>> + clk_disable(i2c_dev->clk);
>> +
>> + return (ret < 0) ? ret : i;
>
> using num instead of i would be a bit more obvious.
ok
>
>> +static int stm32f4_i2c_probe(struct platform_device *pdev)
>> +{
>> +[...]
>> + i2c_dev->speed = STM32F4_I2C_SPEED_STANDARD;
>> + ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
>> + if ((!ret) && (clk_rate == 400000))
>> + i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
>
> I'd use
>
> if (!ret && clk_rate >= 400000)
> i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
>
> . That's less parenthesis and a more robust selection of the bus
> frequency.
ok
>
>> +
>> + i2c_dev->dev = &pdev->dev;
>> +
>> + ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_event,
>> + NULL, stm32f4_i2c_isr_event,
>> + IRQF_ONESHOT, pdev->name, i2c_dev);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to request irq %i\n",
>> + i2c_dev->irq_error);
>
> That's wrong. Requesting irq_event failed.
Ok Thanks.
>
>> + goto clk_free;
>> + }
>> +
>> + ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_error,
>> + NULL, stm32f4_i2c_isr_error,
>> + IRQF_ONESHOT, pdev->name, i2c_dev);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to request irq %i\n",
>> + i2c_dev->irq_error);
>> + goto clk_free;
>
> It would also be nice to know for which type of irq this failed. I.e.
> please point out if this is the error irq or the event irq in the
> message. Ditto for checking the return type of irq_of_parse_and_map.
Ok I will fix it in the next version.
>
>> + }
>> +
>> + adap = &i2c_dev->adap;
>> + i2c_set_adapdata(adap, i2c_dev);
>> + snprintf(adap->name, sizeof(adap->name), "STM32 I2C(%pa)", &res->start);
>> + adap->owner = THIS_MODULE;
>> + adap->timeout = 2 * HZ;
>> + adap->retries = 0;
>> + adap->algo = &stm32f4_i2c_algo;
>> + adap->dev.parent = &pdev->dev;
>> + adap->dev.of_node = pdev->dev.of_node;
>> +
>> + init_completion(&i2c_dev->complete);
>> +
>> + ret = i2c_add_adapter(adap);
>> + if (ret)
>> + goto clk_free;
>> +
>> + platform_set_drvdata(pdev, i2c_dev);
>> +
>> + dev_info(i2c_dev->dev, "STM32F4 I2C driver initialized\n");
>
> This is wrong. The driver is bound now to a device, not initialized.
Ok I will replaced initialized by registered. Thanks.
>
>> +static const struct of_device_id stm32f4_i2c_match[] = {
>> + { .compatible = "st,stm32f4-i2c", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32f4_i2c_match);
>> +
>> +static struct platform_driver stm32f4_i2c_driver = {
>> + .driver = {
>> + .name = "stm32f4-i2c",
>> + .of_match_table = stm32f4_i2c_match,
>
> Is this needed?
Without of_match_table, I could not match an I2C device instance from
DT with this driver.
So maybe, there is a misunderstanding.
Could you please clarify your question ?
>
>> + },
>> + .probe = stm32f4_i2c_probe,
>> + .remove = stm32f4_i2c_remove,
>> +};
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
Thanks again
Best regards,
Cedric
_______________________________________________
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 v4 2/6] [media] rc-main: split setup and unregister functions
From: Sean Young @ 2016-12-15 15:50 UTC (permalink / raw)
To: Andi Shyti
Cc: Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Richard Purdie,
Jacek Anaszewski, Heiner Kallweit, linux-media, devicetree,
linux-leds, linux-kernel, Andi Shyti
In-Reply-To: <20161214140030.28537-3-andi.shyti@samsung.com>
Hi Andi,
This patch breaks all rc devices, none of them have input devices any
more (see below).
On Wed, Dec 14, 2016 at 11:00:26PM +0900, Andi Shyti wrote:
> Move the input device allocation, map and protocol handling to
> different functions.
>
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> Reviewed-by: Sean Young <sean@mess.org>
> ---
> drivers/media/rc/rc-main.c | 143 +++++++++++++++++++++++++--------------------
> 1 file changed, 81 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index a6bbceb..59fac96 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1436,16 +1436,12 @@ struct rc_dev *devm_rc_allocate_device(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(devm_rc_allocate_device);
>
> -int rc_register_device(struct rc_dev *dev)
> +static int rc_setup_rx_device(struct rc_dev *dev)
> {
> - static bool raw_init = false; /* raw decoders loaded? */
> - struct rc_map *rc_map;
> - const char *path;
> - int attr = 0;
> - int minor;
> int rc;
> + struct rc_map *rc_map;
>
> - if (!dev || !dev->map_name)
> + if (!dev->map_name)
> return -EINVAL;
>
> rc_map = rc_map_get(dev->map_name);
> @@ -1454,6 +1450,19 @@ int rc_register_device(struct rc_dev *dev)
> if (!rc_map || !rc_map->scan || rc_map->size == 0)
> return -EINVAL;
>
> + rc = ir_setkeytable(dev, rc_map);
> + if (rc)
> + return rc;
> +
> + if (dev->change_protocol) {
> + u64 rc_type = (1ll << rc_map->rc_type);
> +
> + rc = dev->change_protocol(dev, &rc_type);
> + if (rc < 0)
> + goto out_table;
> + dev->enabled_protocols = rc_type;
> + }
> +
> set_bit(EV_KEY, dev->input_dev->evbit);
> set_bit(EV_REP, dev->input_dev->evbit);
> set_bit(EV_MSC, dev->input_dev->evbit);
> @@ -1463,6 +1472,61 @@ int rc_register_device(struct rc_dev *dev)
> if (dev->close)
> dev->input_dev->close = ir_close;
>
> + /*
> + * Default delay of 250ms is too short for some protocols, especially
> + * since the timeout is currently set to 250ms. Increase it to 500ms,
> + * to avoid wrong repetition of the keycodes. Note that this must be
> + * set after the call to input_register_device().
> + */
> + dev->input_dev->rep[REP_DELAY] = 500;
> +
> + /*
> + * As a repeat event on protocols like RC-5 and NEC take as long as
> + * 110/114ms, using 33ms as a repeat period is not the right thing
> + * to do.
> + */
> + dev->input_dev->rep[REP_PERIOD] = 125;
> +
> + /* rc_open will be called here */
> + rc = input_register_device(dev->input_dev);
> + if (rc)
> + goto out_table;
> +
> + dev->input_dev->dev.parent = &dev->dev;
> + memcpy(&dev->input_dev->id, &dev->input_id, sizeof(dev->input_id));
> + dev->input_dev->phys = dev->input_phys;
> + dev->input_dev->name = dev->input_name;
I was testing your changes, and with this patch none of my rc devices
have input devices associated with them. The problem is that you've changed
the order: input_register_device() should happen AFTER the preceding
4 lines.
> +
> + return 0;
> +
> +out_table:
> + ir_free_table(&dev->rc_map);
> +
> + return rc;
> +}
> +
> +static void rc_free_rx_device(struct rc_dev *dev)
> +{
> + if (!dev)
> + return;
> +
> + ir_free_table(&dev->rc_map);
> +
> + input_unregister_device(dev->input_dev);
> + dev->input_dev = NULL;
> +}
> +
> +int rc_register_device(struct rc_dev *dev)
> +{
> + static bool raw_init = false; /* raw decoders loaded? */
> + const char *path;
> + int attr = 0;
> + int minor;
> + int rc;
> +
> + if (!dev)
> + return -EINVAL;
> +
> minor = ida_simple_get(&rc_ida, 0, RC_DEV_MAX, GFP_KERNEL);
> if (minor < 0)
> return minor;
> @@ -1486,39 +1550,15 @@ int rc_register_device(struct rc_dev *dev)
> if (rc)
> goto out_unlock;
>
> - rc = ir_setkeytable(dev, rc_map);
> - if (rc)
> - goto out_dev;
See the original order here.
> -
> - dev->input_dev->dev.parent = &dev->dev;
> - memcpy(&dev->input_dev->id, &dev->input_id, sizeof(dev->input_id));
> - dev->input_dev->phys = dev->input_phys;
> - dev->input_dev->name = dev->input_name;
> -
> - rc = input_register_device(dev->input_dev);
> - if (rc)
> - goto out_table;
> -
> - /*
> - * Default delay of 250ms is too short for some protocols, especially
> - * since the timeout is currently set to 250ms. Increase it to 500ms,
> - * to avoid wrong repetition of the keycodes. Note that this must be
> - * set after the call to input_register_device().
> - */
> - dev->input_dev->rep[REP_DELAY] = 500;
> -
> - /*
> - * As a repeat event on protocols like RC-5 and NEC take as long as
> - * 110/114ms, using 33ms as a repeat period is not the right thing
> - * to do.
> - */
> - dev->input_dev->rep[REP_PERIOD] = 125;
> -
> path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL);
> dev_info(&dev->dev, "%s as %s\n",
> dev->input_name ?: "Unspecified device", path ?: "N/A");
> kfree(path);
>
> + rc = rc_setup_rx_device(dev);
> + if (rc)
> + goto out_dev;
> +
> if (dev->driver_type == RC_DRIVER_IR_RAW) {
> if (!raw_init) {
> request_module_nowait("ir-lirc-codec");
> @@ -1526,36 +1566,20 @@ int rc_register_device(struct rc_dev *dev)
> }
> rc = ir_raw_event_register(dev);
> if (rc < 0)
> - goto out_input;
> - }
> -
> - if (dev->change_protocol) {
> - u64 rc_type = (1ll << rc_map->rc_type);
> - rc = dev->change_protocol(dev, &rc_type);
> - if (rc < 0)
> - goto out_raw;
> - dev->enabled_protocols = rc_type;
> + goto out_rx;
> }
>
> /* Allow the RC sysfs nodes to be accessible */
> atomic_set(&dev->initialized, 1);
>
> - IR_dprintk(1, "Registered rc%u (driver: %s, remote: %s, mode %s)\n",
> + IR_dprintk(1, "Registered rc%u (driver: %s)\n",
> dev->minor,
> - dev->driver_name ? dev->driver_name : "unknown",
> - rc_map->name ? rc_map->name : "unknown",
> - dev->driver_type == RC_DRIVER_IR_RAW ? "raw" : "cooked");
> + dev->driver_name ? dev->driver_name : "unknown");
>
> return 0;
>
> -out_raw:
> - if (dev->driver_type == RC_DRIVER_IR_RAW)
> - ir_raw_event_unregister(dev);
> -out_input:
> - input_unregister_device(dev->input_dev);
> - dev->input_dev = NULL;
> -out_table:
> - ir_free_table(&dev->rc_map);
> +out_rx:
> + rc_free_rx_device(dev);
> out_dev:
> device_del(&dev->dev);
> out_unlock:
> @@ -1601,12 +1625,7 @@ void rc_unregister_device(struct rc_dev *dev)
> if (dev->driver_type == RC_DRIVER_IR_RAW)
> ir_raw_event_unregister(dev);
>
> - /* Freeing the table should also call the stop callback */
> - ir_free_table(&dev->rc_map);
> - IR_dprintk(1, "Freed keycode table\n");
> -
> - input_unregister_device(dev->input_dev);
> - dev->input_dev = NULL;
> + rc_free_rx_device(dev);
>
> device_del(&dev->dev);
>
> --
> 2.10.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] i2c: designware: Cleaning and comment style fixes.
From: Luis Oliveira @ 2016-12-15 15:53 UTC (permalink / raw)
To: Peter Rosin, Luis Oliveira, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <f11794ec-439b-5fcd-65f4-c14c319cd627-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 15-Dec-16 15:17, Peter Rosin wrote:
> On 2016-12-15 15:38, Luis Oliveira wrote:
>> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
>> index 26250b425e2f..3cb81fca7738 100644
>> --- a/drivers/i2c/busses/i2c-designware-core.h
>> +++ b/drivers/i2c/busses/i2c-designware-core.h
>> @@ -36,7 +36,7 @@
>> #define DW_IC_CON_SPEED_FAST 0x4
>> #define DW_IC_CON_SPEED_HIGH 0x6
>> #define DW_IC_CON_SPEED_MASK 0x6
>> -#define DW_IC_CON_10BITADDR_MASTER 0x10
>> +#define DW_IC_CON_10BITADDR_MASTER 0x10
>
> How is this an improvement?
It is not, I will fix it.
Thanks,
Luis
>
> Cheers,
> peda
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 1/2] Documentation: omap-usb-host: fix OMAP OHCI/EHCI file names
From: yegorslists-gM/Ye1E23mwN+BqQ9rBEUg @ 2016-12-15 15:56 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, Yegor Yefremov
From: Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
OMAP related files are actually named ehci-omap.txt and ohci-omap3.txt.
Also add full path to ohci-omap3.txt.
Signed-off-by: Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
Documentation/devicetree/bindings/mfd/omap-usb-host.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt
index 4721b2d..aa1eaa5 100644
--- a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt
+++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt
@@ -64,8 +64,8 @@ Required properties if child node exists:
Properties for children:
The OMAP HS USB Host subsystem contains EHCI and OHCI controllers.
-See Documentation/devicetree/bindings/usb/omap-ehci.txt and
-omap3-ohci.txt
+See Documentation/devicetree/bindings/usb/ehci-omap.txt and
+Documentation/devicetree/bindings/usb/ohci-omap3.txt.
Example for OMAP4:
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 2/2] Documentation: ehci-omap: remove the unnecessary newline
From: yegorslists @ 2016-12-15 15:56 UTC (permalink / raw)
To: linux-kernel; +Cc: devicetree, robh+dt, mark.rutland, Yegor Yefremov
In-Reply-To: <1481817370-28242-1-git-send-email-yegorslists@googlemail.com>
From: Yegor Yefremov <yegorslists@googlemail.com>
Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
---
Documentation/devicetree/bindings/usb/ehci-omap.txt | 1 -
1 file changed, 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/usb/ehci-omap.txt b/Documentation/devicetree/bindings/usb/ehci-omap.txt
index 3dc231c..d77e11a 100644
--- a/Documentation/devicetree/bindings/usb/ehci-omap.txt
+++ b/Documentation/devicetree/bindings/usb/ehci-omap.txt
@@ -29,4 +29,3 @@ usbhsehci: ehci@4a064c00 {
&usbhsehci {
phys = <&hsusb1_phy 0 &hsusb3_phy>;
};
-
--
2.1.4
^ permalink raw reply related
* Re: [PATCH v4 2/6] [media] rc-main: split setup and unregister functions
From: Andi Shyti @ 2016-12-15 16:01 UTC (permalink / raw)
To: Sean Young
Cc: Andi Shyti, Mauro Carvalho Chehab, Rob Herring, Mark Rutland,
Richard Purdie, Jacek Anaszewski, Heiner Kallweit, linux-media,
devicetree, linux-leds, linux-kernel, Andi Shyti
In-Reply-To: <20161215155049.GA23320@gofer.mess.org>
> > + /* rc_open will be called here */
> > + rc = input_register_device(dev->input_dev);
> > + if (rc)
> > + goto out_table;
> > +
> > + dev->input_dev->dev.parent = &dev->dev;
> > + memcpy(&dev->input_dev->id, &dev->input_id, sizeof(dev->input_id));
> > + dev->input_dev->phys = dev->input_phys;
> > + dev->input_dev->name = dev->input_name;
>
> I was testing your changes, and with this patch none of my rc devices
> have input devices associated with them. The problem is that you've changed
> the order: input_register_device() should happen AFTER the preceding
> 4 lines.
This must have been a copy paste error and I don't have
transmitters to test it. Thanks for testing it. I will send it
again.
Andi
^ permalink raw reply
* [PATCHv4 1/2] regulator: fixed: dt: Allow an optional over current pin
From: Axel Haslam @ 2016-12-15 16:29 UTC (permalink / raw)
To: broonie-DgEjT+Ai2ygdnm+yROfE0A, lgirdwood-Re5JQEeQqe8AvxtiuMwx3w,
khilman-rdvid1DuHRBWk0Htik3J/w, nsekhar-l0cyMroinI0,
david-nq/r/kbU++upp/zk7JDF2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Axel Haslam,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161215162955.3940-1-ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Add support for an optional over current input pin which
can be used to send an over current event to the regulator
consumer.
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
Documentation/devicetree/bindings/regulator/fixed-regulator.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
index 4fae41d..b145abb 100644
--- a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
@@ -11,6 +11,7 @@ If this property is missing, the default assumed is Active low.
- gpio-open-drain: GPIO is open drain type.
If this property is missing then default assumption is false.
-vin-supply: Input supply name.
+- over-current-gpios: Input gpio that signal an over current condition.
Any property defined as part of the core regulator
binding, defined in regulator.txt, can also be used.
@@ -26,6 +27,7 @@ Example:
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
gpio = <&gpio1 16 0>;
+ over-current-gpios = <&gpio1 18 0>;
startup-delay-us = <70000>;
enable-active-high;
regulator-boot-on;
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399
From: Doug Anderson @ 2016-12-15 16:34 UTC (permalink / raw)
To: Frank Wang
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Elaine Zhang, Tao Huang, Heiko Stübner, Xing Zheng,
Catalin Marinas, Brian Norris, Will Deacon,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, daniel.meng,
Kever Yang, open list:ARM/Rockchip SoC..., Rob Herring,
William wu, Dmitry Torokhov, Jianqun Xu,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Caesar Wang
In-Reply-To: <991221a4-3962-1bcb-7863-72f5553eba40-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Hi,
On Wed, Dec 14, 2016 at 10:41 PM, Frank Wang <frank.wang-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> Hi Brain, Doug and Heiko,
>
> I would like to summarize why this story was constructed.
>
> The ehci/ohci-platform suspend process are blocked due to UTMI clock which
> directly output from usb-phy has been disabled, and why the UTMI clock was
> disabled?
>
> UTMI clock and 480m clock all output from the same internal PLL of usb-phy,
> and there is only one bit can use to control this PLL on or off, which we
> named "otg_commononn"(GRF, offset 0x0e450/0x0e460 bit4 ) in RK3399 TRM.
>
> When system boot up, ehci/ohci-platform probe function invoke
> phy_power_on(), further invoke rockchip_usb2phy_power_on() to enable 480m
> clock, actually, it sets the otg_commononn bit on, and then usb-phy will go
> to (auto)suspend if there is no devices plug-in after 1 minute, the
> rockchip_usb2phy_power_off() will be invoked and the 480m clock may be
> disabled in the (auto)suspend process. As a result, the otg_commononn bit
> may be turned off, and all output clock of usb-phy will be disabled.
> However, ehci/ohci-platform PM suspend operation (read/write controller
> register) are based on the UTMI clock.
>
> So we introduced "clk_usbphy0_480m_src"/"clk_usbphy1_480m_src" as one input
> clock for ehci/ohci-platform, in this way, the otg_commononn bit is not
> turned off until ehci/ohci-platform go to PM suspend.
I still need to digest all of the things that were added to this
thread overnight, but nothing I've seen so far indicates that you need
the post-gated clock. AKA I still think you need to redo your patch
to replace:
clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
<&cru SCLK_USBPHY0_480M_SRC>;
with:
clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
<&u2phy0>;
Can you please comment on that?
-Doug
^ permalink raw reply
* [PATCH v5 2/9] doc: DT: venus: binding document for Qualcomm video driver
From: Stanimir Varbanov @ 2016-12-15 17:22 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Hans Verkuil
Cc: Andy Gross, Bjorn Andersson, Stephen Boyd, Srinivas Kandagatla,
linux-media, linux-kernel, linux-arm-msm, Stanimir Varbanov,
Rob Herring, Mark Rutland, devicetree
In-Reply-To: <1481822544-29900-1-git-send-email-stanimir.varbanov@linaro.org>
Add binding document for Venus video encoder/decoder driver
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
.../devicetree/bindings/media/qcom,venus.txt | 68 ++++++++++++++++++++++
1 file changed, 68 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/qcom,venus.txt
diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
new file mode 100644
index 000000000000..7b77dff52b0f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
@@ -0,0 +1,68 @@
+* Qualcomm Venus video encode/decode accelerator
+
+- compatible:
+ Usage: required
+ Value type: <stringlist>
+ Definition: Value should contain one of:
+ - "qcom,msm8916-venus"
+ - "qcom,msm8996-venus"
+- reg:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: Register base address and length of the register map.
+- interrupts:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: Should contain interrupt line number.
+- clocks:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: A List of phandle and clock specifier pairs as listed
+ in clock-names property.
+- clock-names:
+ Usage: required for msm8916
+ Value type: <stringlist>
+ Definition: Should contain the following entries:
+ - "core" Core video accelerator clock
+ - "iface" Video accelerator AHB clock
+ - "bus" Video accelerator AXI clock
+- clock-names:
+ Usage: required for msm8996
+ Value type: <stringlist>
+ Definition: Should contain the following entries:
+ - "core" Core video accelerator clock
+ - "iface" Video accelerator AHB clock
+ - "bus" Video accelerator AXI clock
+ - "subcore0" Subcore0 (decoder) video accelerator clock
+ - "subcore1" Subcore1 (encoder) video accelerator clock
+ - "mbus" Video MAXI clock
+- power-domains:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: A phandle and power domain specifier pairs to the
+ power domain which is responsible for collapsing
+ and restoring power to the peripheral.
+- rproc:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: A phandle to remote processor responsible for
+ firmware loading and processor booting.
+
+- iommus:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: A list of phandle and IOMMU specifier pairs.
+
+* An Example
+ video-codec@1d00000 {
+ compatible = "qcom,msm8916-venus";
+ reg = <0x01d00000 0xff000>;
+ interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&gcc GCC_VENUS0_VCODEC0_CLK>,
+ <&gcc GCC_VENUS0_AHB_CLK>,
+ <&gcc GCC_VENUS0_AXI_CLK>;
+ clock-names = "core", "iface", "bus";
+ power-domains = <&gcc VENUS_GDSC>;
+ rproc = <&venus_rproc>;
+ iommus = <&apps_iommu 5>;
+ };
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v3] dt-bindings: power: supply: bq24735: reverse the polarity of ac-detect
From: Stephen Warren @ 2016-12-15 17:32 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Sebastian Reichel, Rob Herring, Mark Rutland,
linux-pm, devicetree, linux-tegra, Jon Hunter
In-Reply-To: <1481804479-8711-1-git-send-email-peda@axentia.se>
On 12/15/2016 05:21 AM, Peter Rosin wrote:
> The ACOK pin on the bq24735 is active-high, of course meaning that when
> AC is OK the pin is high. However, all Tegra dts files have incorrectly
> specified active-high even though the signal is inverted on the Tegra
> boards. This has worked since the Linux driver has also inverted the
> meaning of the GPIO. Fix this situation by simply specifying in the
> bindings what everybody else agrees on; that the ti,ac-detect-gpios is
> active on AC adapter absence.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>
> Hi!
>
> This patch is the result of this discussion:
> http://marc.info/?t=148152531800002
>
> I don't like how it changes the one thing that is seems correct, but
> what to do?
I haven't followed this thread so hopefully what I say is relevant. My
take is:
If the DT binding is correct or reasonable, keep it.
If the Tegra DTs contain incorrect content, and never worked correctly
in this aspect, then fix them. We do need to maintain DT
ABI/compatibility, but I believe only with stuff that actually worked
correctly. If the DT has a bug, just fix it.
That said, if ti,ac-detect-gpios is describing a host GPIO, then it's
entirely arbitrary which polarity it should have, i.e. the polarity is
not something specified by the bq24735 HW. In that case, feel free to
change either the binding to match the DT or the DT to match the
binding. Changing the DT to match the binding might still be better
since there could be other users you're not aware of, and they might
have written their DT correctly, and you don't want to break them.
^ permalink raw reply
* Re: [PATCH/RFC v2 3/7] spi: core: Add support for registering SPI slave controllers
From: Mark Brown @ 2016-12-15 17:46 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Geert Uytterhoeven, Rob Herring, Mark Rutland, Magnus Damm,
Wolfram Sang, Hiromitsu Yamasaki, linux-spi,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux-Renesas,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Fengguang Wu
In-Reply-To: <CAMuHMdWhuqSg+qUnDUqad-SBJOY5rOuRGuG7oUfTXiQA_rUtoA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 299 bytes --]
On Sun, Sep 18, 2016 at 11:04:18AM +0200, Geert Uytterhoeven wrote:
> This is caused by moving the setup of master->dev.class.
> To fix this, I can
> 1) Introduce a separate spi_alloc_slave() function, which sets up
> spi_slave_class instead of spi_master class,
This seems more idiomatic.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v3] dt-bindings: power: supply: bq24735: reverse the polarity of ac-detect
From: Peter Rosin @ 2016-12-15 17:50 UTC (permalink / raw)
To: Stephen Warren
Cc: linux-kernel, Sebastian Reichel, Rob Herring, Mark Rutland,
linux-pm, devicetree, linux-tegra, Jon Hunter
In-Reply-To: <5f5fe01a-7c7a-58f7-2171-5f6879392ea7@wwwdotorg.org>
On 2016-12-15 18:32, Stephen Warren wrote:
> On 12/15/2016 05:21 AM, Peter Rosin wrote:
>> The ACOK pin on the bq24735 is active-high, of course meaning that when
>> AC is OK the pin is high. However, all Tegra dts files have incorrectly
>> specified active-high even though the signal is inverted on the Tegra
>> boards. This has worked since the Linux driver has also inverted the
>> meaning of the GPIO. Fix this situation by simply specifying in the
>> bindings what everybody else agrees on; that the ti,ac-detect-gpios is
>> active on AC adapter absence.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>
>> Hi!
>>
>> This patch is the result of this discussion:
>> http://marc.info/?t=148152531800002
>>
>> I don't like how it changes the one thing that is seems correct, but
>> what to do?
>
> I haven't followed this thread so hopefully what I say is relevant. My
> take is:
>
> If the DT binding is correct or reasonable, keep it.
>
> If the Tegra DTs contain incorrect content, and never worked correctly
> in this aspect, then fix them. We do need to maintain DT
> ABI/compatibility, but I believe only with stuff that actually worked
> correctly. If the DT has a bug, just fix it.
>
> That said, if ti,ac-detect-gpios is describing a host GPIO, then it's
> entirely arbitrary which polarity it should have, i.e. the polarity is
> not something specified by the bq24735 HW. In that case, feel free to
> change either the binding to match the DT or the DT to match the
> binding. Changing the DT to match the binding might still be better
> since there could be other users you're not aware of, and they might
> have written their DT correctly, and you don't want to break them.
The bindings are fine.
The Tegra dts files are buggy, but the driver is also buggy, so those
two bugs cancel each other. So, the option is to either introduce
regressions by fixing the two bugs thus creating a flag day where
the kernel and dt needs to match. Or, just document what is going on
and change the bindings even if they are not wrong.
I suggest you read the discussion. We covered all this already, and it
is also in the commit message.
Cheers,
peda
^ permalink raw reply
* Re: [PATCH/RFC v2 3/7] spi: core: Add support for registering SPI slave controllers
From: Mark Brown @ 2016-12-15 17:53 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rob Herring, Mark Rutland, Magnus Damm, Wolfram Sang,
Hisashi Nakamura, Hiromitsu Yamasaki, linux-spi, devicetree,
linux-renesas-soc, linux-kernel
In-Reply-To: <1473713446-30366-4-git-send-email-geert+renesas@glider.be>
[-- Attachment #1: Type: text/plain, Size: 509 bytes --]
On Mon, Sep 12, 2016 at 10:50:42PM +0200, Geert Uytterhoeven wrote:
> TBD:
> - s/spi_master/spi_controller/ where appropriate,
> - Provide wrappers (e.g. "#define spi_master spi_controller" until all
> SPI drivers have been converted),
> - Do we want a separate spi_register_slave() instead?
This basically looks fine to me - there's these TBDs so I might be
missing things and we probably need some GPIO chip select handling but
that's a separate thing. Sorry it took me so long to review this.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] Bluetooth: btusb: Configure Marvel to use one of the pins for oob wakeup
From: Rajat Jain @ 2016-12-15 18:04 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Rob Herring, Mark Rutland, Marcel Holtmann, Gustavo Padovan,
Johan Hedberg, Amitkumar Karwar, Wei-Ning Huang, Xinming Hu,
netdev, devicetree, linux-bluetooth, Brian Norris, Rajat Jain
In-Reply-To: <87shppk32a.fsf@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 6813 bytes --]
On Thu, Dec 15, 2016 at 12:29 AM, Gregory CLEMENT <
gregory.clement@free-electrons.com> wrote:
> Hi Rajat,
>
> On mer., déc. 14 2016, Rajat Jain <rajatja@google.com> wrote:
>
> In your title unless you speak about the comic books you should do a
> s/Marvel/Marvell/ :)
>
Oops :-) Will do, thanks!
>
> Gregory
>
> > The Marvell devices may have many gpio pins, and hence for wakeup
> > on these out-of-band pins, the chip needs to be told which pin is
> > to be used for wakeup, using an hci command.
> >
> > Thus, we read the pin number etc from the device tree node and send
> > a command to the chip.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > Note that while I would have liked to name the compatible string as more
> > like "marvell, usb8997-bt", the devicetrees/bindings/usb/usb-device.txt
> > requires the compatible property to be of the form "usbVID,PID".
> >
> > .../{marvell-bt-sd8xxx.txt => marvell-bt-8xxx.txt} | 25 ++++++++-
> > drivers/bluetooth/btusb.c | 59
> ++++++++++++++++++++++
> > 2 files changed, 82 insertions(+), 2 deletions(-)
> > rename Documentation/devicetree/bindings/net/{marvell-bt-sd8xxx.txt =>
> marvell-bt-8xxx.txt} (76%)
> >
> > diff --git a/Documentation/devicetree/bindings/net/marvell-bt-sd8xxx.txt
> b/Documentation/devicetree/bindings/net/marvell-bt-8xxx.txt
> > similarity index 76%
> > rename from Documentation/devicetree/bindings/net/marvell-bt-sd8xxx.txt
> > rename to Documentation/devicetree/bindings/net/marvell-bt-8xxx.txt
> > index 6a9a63c..471bef8 100644
> > --- a/Documentation/devicetree/bindings/net/marvell-bt-sd8xxx.txt
> > +++ b/Documentation/devicetree/bindings/net/marvell-bt-8xxx.txt
> > @@ -1,4 +1,4 @@
> > -Marvell 8897/8997 (sd8897/sd8997) bluetooth SDIO devices
> > +Marvell 8897/8997 (sd8897/sd8997) bluetooth devices (SDIO or USB based)
> > ------
> >
> > Required properties:
> > @@ -6,11 +6,13 @@ Required properties:
> > - compatible : should be one of the following:
> > * "marvell,sd8897-bt"
> > * "marvell,sd8997-bt"
> > + * "usb1286,204e"
> >
> > Optional properties:
> >
> > - marvell,cal-data: Calibration data downloaded to the device during
> > initialization. This is an array of 28 values(u8).
> > + This is only applicable to SDIO devices.
> >
> > - marvell,wakeup-pin: It represents wakeup pin number of the
> bluetooth chip.
> > firmware will use the pin to wakeup host system
> (u16).
> > @@ -29,7 +31,9 @@ Example:
> > IRQ pin 119 is used as system wakeup source interrupt.
> > wakeup pin 13 and gap 100ms are configured so that firmware can wakeup
> host
> > using this device side pin and wakeup latency.
> > -calibration data is also available in below example.
> > +
> > +Example for SDIO device follows (calibration data is also available in
> > +below example).
> >
> > &mmc3 {
> > status = "okay";
> > @@ -54,3 +58,20 @@ calibration data is also available in below example.
> > marvell,wakeup-gap-ms = /bits/ 16 <0x64>;
> > };
> > };
> > +
> > +Example for USB device:
> > +
> > +&usb_host1_ohci {
> > + status = "okay";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + mvl_bt1: bt@1 {
> > + compatible = "usb1286,204e";
> > + reg = <1>;
> > + interrupt-parent = <&gpio0>;
> > + interrupts = <119 IRQ_TYPE_LEVEL_LOW>;
> > + marvell,wakeup-pin = /bits/ 16 <0x0d>;
> > + marvell,wakeup-gap-ms = /bits/ 16 <0x64>;
> > + };
> > +};
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 32a6f22..99d7f6d 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -2343,6 +2343,58 @@ static int btusb_shutdown_intel(struct hci_dev
> *hdev)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_PM
> > +static const struct of_device_id mvl_oob_wake_match_table[] = {
> > + { .compatible = "usb1286,204e" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, mvl_oob_wake_match_table);
> > +
> > +/* Configure an out-of-band gpio as wake-up pin, if specified in device
> tree */
> > +static int marvell_config_oob_wake(struct hci_dev *hdev)
> > +{
> > + struct sk_buff *skb;
> > + struct btusb_data *data = hci_get_drvdata(hdev);
> > + struct device *dev = &data->udev->dev;
> > + u16 pin, gap, opcode;
> > + int ret;
> > + u8 cmd[5];
> > +
> > + if (!of_match_device(mvl_oob_wake_match_table, dev))
> > + return 0;
> > +
> > + if (of_property_read_u16(dev->of_node, "marvell,wakeup-pin",
> &pin) ||
> > + of_property_read_u16(dev->of_node, "marvell,wakeup-gap-ms",
> &gap))
> > + return -EINVAL;
> > +
> > + /* Vendor specific command to configure a GPIO as wake-up pin */
> > + opcode = hci_opcode_pack(0x3F, 0x59);
> > + cmd[0] = opcode & 0xFF;
> > + cmd[1] = opcode >> 8;
> > + cmd[2] = 2; /* length of parameters that follow */
> > + cmd[3] = pin;
> > + cmd[4] = gap; /* time in ms, for which wakeup pin should be
> asserted */
> > +
> > + skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
> > + if (!skb) {
> > + bt_dev_err(hdev, "%s: No memory\n", __func__);
> > + return -ENOMEM;
> > + }
> > +
> > + memcpy(skb_put(skb, sizeof(cmd)), cmd, sizeof(cmd));
> > + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> > +
> > + ret = btusb_send_frame(hdev, skb);
> > + if (ret) {
> > + bt_dev_err(hdev, "%s: configuration failed\n", __func__);
> > + kfree_skb(skb);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > static int btusb_set_bdaddr_marvell(struct hci_dev *hdev,
> > const bdaddr_t *bdaddr)
> > {
> > @@ -2917,6 +2969,13 @@ static int btusb_probe(struct usb_interface *intf,
> > err = btusb_config_oob_wake(hdev);
> > if (err)
> > goto out_free_dev;
> > +
> > + /* Marvel devices may need a specific chip configuration */
> > + if (id->driver_info & BTUSB_MARVELL && data->oob_wake_irq) {
> > + err = marvell_config_oob_wake(hdev);
> > + if (err)
> > + goto out_free_dev;
> > + }
> > #endif
> > if (id->driver_info & BTUSB_CW6622)
> > set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
> > --
> > 2.8.0.rc3.226.g39d4020
> >
>
> --
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com
>
[-- Attachment #2: Type: text/html, Size: 9023 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399
From: Heiko Stuebner @ 2016-12-15 18:18 UTC (permalink / raw)
To: Doug Anderson
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Elaine Zhang, Tao Huang, Xing Zheng, Frank Wang, Catalin Marinas,
Brian Norris, Will Deacon,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, daniel.meng,
Kever Yang, open list:ARM/Rockchip SoC..., Rob Herring,
William wu, Dmitry Torokhov, Jianqun Xu,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Caesar Wang
In-Reply-To: <CAD=FV=XKQaqRS4jUM7NpN2KEV8USj_cVWbh7q4274n3jBtwORg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Am Donnerstag, 15. Dezember 2016, 08:34:09 CET schrieb Doug Anderson:
> Hi,
>
> On Wed, Dec 14, 2016 at 10:41 PM, Frank Wang <frank.wang-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
wrote:
> > Hi Brain, Doug and Heiko,
> >
> > I would like to summarize why this story was constructed.
> >
> > The ehci/ohci-platform suspend process are blocked due to UTMI clock which
> > directly output from usb-phy has been disabled, and why the UTMI clock was
> > disabled?
> >
> > UTMI clock and 480m clock all output from the same internal PLL of
> > usb-phy,
> > and there is only one bit can use to control this PLL on or off, which we
> > named "otg_commononn"(GRF, offset 0x0e450/0x0e460 bit4 ) in RK3399 TRM.
> >
> > When system boot up, ehci/ohci-platform probe function invoke
> > phy_power_on(), further invoke rockchip_usb2phy_power_on() to enable 480m
> > clock, actually, it sets the otg_commononn bit on, and then usb-phy will
> > go
> > to (auto)suspend if there is no devices plug-in after 1 minute, the
> > rockchip_usb2phy_power_off() will be invoked and the 480m clock may be
> > disabled in the (auto)suspend process. As a result, the otg_commononn bit
> > may be turned off, and all output clock of usb-phy will be disabled.
> > However, ehci/ohci-platform PM suspend operation (read/write controller
> > register) are based on the UTMI clock.
> >
> > So we introduced "clk_usbphy0_480m_src"/"clk_usbphy1_480m_src" as one
> > input
> > clock for ehci/ohci-platform, in this way, the otg_commononn bit is not
> > turned off until ehci/ohci-platform go to PM suspend.
>
> I still need to digest all of the things that were added to this
> thread overnight, but nothing I've seen so far indicates that you need
> the post-gated clock. AKA I still think you need to redo your patch
> to replace:
>
> clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
> <&cru SCLK_USBPHY0_480M_SRC>;
>
> with:
>
> clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
> <&u2phy0>;
>
> Can you please comment on that?
Also, with the change, the ehci will keep the clock (and thus the phy) always
on. Does the phy-autosuspend even save anything now?
In any case, could we make the clock-names entry sound nicer than usbphy0_480m
please? bindings/usb/atmel-usb.txt calls its UTMI clock simply "usb_clk", but
something like "utmi" should also work.
While at it you could also fix up the other clock names to something like
"host" and "arbiter" or so?.
Heiko
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox