* Re: [PATCH -next] PCI: dra7xx: Add missing of_node_put() in dra7xx_pcie_init_irq_domain()
From: Bjorn Helgaas @ 2016-11-14 21:19 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: Wei Yongjun, Bjorn Helgaas, Wei Yongjun,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-pci-u79uwXL29TY76Z2rM5mHXA, Shawn Lin, Heiko Stuebner,
michal.simek-gjFFaj9aHVfQT0dZR+AlfA,
soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA,
bharat.kumar.gogada-gjFFaj9aHVfQT0dZR+AlfA,
robh-DgEjT+Ai2ygdnm+yROfE0A, Frank Rowand,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1d95b915-ddc0-b48f-270b-fffb60ecfe5e-l0cyMroinI0@public.gmane.org>
[+cc Shawn, Heiko, Michal, Soren, Bharat, Rob H, Frank, devicetree@vger]
On Sat, Nov 12, 2016 at 12:39:01PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Saturday 12 November 2016 03:08 AM, Bjorn Helgaas wrote:
> > On Mon, Oct 17, 2016 at 02:54:37PM +0000, Wei Yongjun wrote:
> >> From: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> >>
> >> This node pointer is returned by of_get_next_child() with refcount
> >> incremented in this function. of_node_put() on it before exitting
> >> this function on error.
> >>
> >> This is detected by Coccinelle semantic patch.
> >>
> >> Signed-off-by: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> >
> > Kishon, this looks correct to me, so I applied it to pci/host-dra7xx for
> > v4.10. Let me know if you have any issue with it.
> >
> >> ---
> >> drivers/pci/host/pci-dra7xx.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
> >> index 9595fad..79297e9 100644
> >> --- a/drivers/pci/host/pci-dra7xx.c
> >> +++ b/drivers/pci/host/pci-dra7xx.c
> >> @@ -177,6 +177,7 @@ static int dra7xx_pcie_init_irq_domain(struct pcie_port *pp)
> >> &intx_domain_ops, pp);
> >> if (!pp->irq_domain) {
> >> dev_err(dev, "Failed to get a INTx IRQ domain\n");
> >> + of_node_put(pcie_intc_node);
>
> I think of_node_put should be used for both the error case and non-error case.
Hmm, OK. I don't know what the rules are. Certainly if we made these
drivers modular, I don't think we'd want to leak these references
every time we unload/reload the module. Should we do the put
immediately here, or in the module remove path, or ...?
Adding other driver and DT folks for comment.
I dropped these patches for now (dra7xx, rockchip, xilinx-nwl,
xilinx).
--
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 v3] dts: omap5: board-common: fix wrong SMPS6 (VDD-DDR3) voltage
From: Tony Lindgren @ 2016-11-14 21:17 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Benoît Cousson, Rob Herring, Mark Rutland, Russell King,
linux-omap, devicetree, linux-kernel, kernel
In-Reply-To: <9718ff02c1c863e97bf1247009db8292fcff7d8d.1479124514.git.hns@goldelico.com>
* H. Nikolaus Schaller <hns@goldelico.com> [161114 03:55]:
> DDR3L is usually specified as
>
> JEDEC standard 1.35V(1.28V~1.45V) & 1.5V(1.425V~1.575V)
>
> Therefore setting smps6 regulator to 1.2V is definitively below
> minimum. It appears that real world chips are more forgiving than
> data sheets indicate, but let's set the regulator right.
>
> Note: a board that uses other voltages (DDR with 1.5V) can
> overwrite by referencing &smps6_reg.
Applying into omap-for-v4.9/fixes thanks.
Tony
^ permalink raw reply
* [PATCH v3 2/2] ARM: sunxi: Add the missing clocks to the pinctrl nodes
From: Maxime Ripard @ 2016-11-14 20:53 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot
Cc: linux-gpio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Chen-Yu Tsai,
Maxime Ripard
In-Reply-To: <cover.fa554eb1146d18ec75bf44863543fec4fa4fd3ae.1479156725.git-series.maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
The pin controllers also use the two oscillators for debouncing. Add them
to the DTs.
Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
arch/arm/boot/dts/sun4i-a10.dtsi | 3 ++-
arch/arm/boot/dts/sun5i.dtsi | 3 ++-
arch/arm/boot/dts/sun6i-a31.dtsi | 6 ++++--
arch/arm/boot/dts/sun7i-a20.dtsi | 3 ++-
arch/arm/boot/dts/sun8i-a23-a33.dtsi | 6 ++++--
arch/arm/boot/dts/sun8i-h3.dtsi | 6 ++++--
arch/arm/boot/dts/sun9i-a80.dtsi | 6 ++++--
7 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 7e7dfc2b43db..b14a4281058d 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -967,7 +967,8 @@
compatible = "allwinner,sun4i-a10-pinctrl";
reg = <0x01c20800 0x400>;
interrupts = <28>;
- clocks = <&apb0_gates 5>;
+ clocks = <&apb0_gates 5>, <&osc24M>, <&osc32k>;
+ clock-names = "apb", "hosc", "losc";
gpio-controller;
interrupt-controller;
#interrupt-cells = <3>;
diff --git a/arch/arm/boot/dts/sun5i.dtsi b/arch/arm/boot/dts/sun5i.dtsi
index b4ccee8cfb02..b0fca4ef4dae 100644
--- a/arch/arm/boot/dts/sun5i.dtsi
+++ b/arch/arm/boot/dts/sun5i.dtsi
@@ -547,7 +547,8 @@
pio: pinctrl@01c20800 {
reg = <0x01c20800 0x400>;
interrupts = <28>;
- clocks = <&apb0_gates 5>;
+ clocks = <&apb0_gates 5>, <&osc24M>, <&osc32k>;
+ clock-names = "apb", "hosc", "losc";
gpio-controller;
interrupt-controller;
#interrupt-cells = <3>;
diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 2e8bf93dcfb2..c941662383ee 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -471,7 +471,8 @@
<GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&ccu CLK_APB1_PIO>;
+ clocks = <&ccu CLK_APB1_PIO>, <&osc24M>, <&osc32k>;
+ clock-names = "apb", "hosc", "losc";
gpio-controller;
interrupt-controller;
#interrupt-cells = <3>;
@@ -1051,7 +1052,8 @@
reg = <0x01f02c00 0x400>;
interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&apb0_gates 0>;
+ clocks = <&apb0_gates 0>, <&osc24M>, <&osc32k>;
+ clock-names = "apb", "hosc", "losc";
resets = <&apb0_rst 0>;
gpio-controller;
interrupt-controller;
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 94cf5a1c7172..f7db067b0de0 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -1085,7 +1085,8 @@
compatible = "allwinner,sun7i-a20-pinctrl";
reg = <0x01c20800 0x400>;
interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&apb0_gates 5>;
+ clocks = <&apb0_gates 5>, <&osc24M>, <&osc32k>;
+ clock-names = "apb", "hosc", "losc";
gpio-controller;
interrupt-controller;
#interrupt-cells = <3>;
diff --git a/arch/arm/boot/dts/sun8i-a23-a33.dtsi b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
index 300a1bd5a6ec..e4991a78ad73 100644
--- a/arch/arm/boot/dts/sun8i-a23-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
@@ -266,7 +266,8 @@
/* compatible gets set in SoC specific dtsi file */
reg = <0x01c20800 0x400>;
/* interrupts get set in SoC specific dtsi file */
- clocks = <&ccu CLK_BUS_PIO>;
+ clocks = <&ccu CLK_BUS_PIO>, <&osc24M>, <&osc32k>;
+ clock-names = "apb", "hosc", "losc";
gpio-controller;
interrupt-controller;
#interrupt-cells = <3>;
@@ -575,7 +576,8 @@
compatible = "allwinner,sun8i-a23-r-pinctrl";
reg = <0x01f02c00 0x400>;
interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&apb0_gates 0>;
+ clocks = <&apb0_gates 0>, <&osc24M>, <&osc32k>;
+ clock-names = "apb", "hosc", "losc";
resets = <&apb0_rst 0>;
gpio-controller;
interrupt-controller;
diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index c38b028cac83..3c6596f06ebc 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -321,7 +321,8 @@
reg = <0x01c20800 0x400>;
interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&ccu CLK_BUS_PIO>;
+ clocks = <&ccu CLK_BUS_PIO>, <&osc24M>, <&osc32k>;
+ clock-names = "apb", "hosc", "losc";
gpio-controller;
#gpio-cells = <3>;
interrupt-controller;
@@ -614,7 +615,8 @@
compatible = "allwinner,sun8i-h3-r-pinctrl";
reg = <0x01f02c00 0x400>;
interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&apb0_gates 0>;
+ clocks = <&apb0_gates 0>, <&osc24M>, <&osc32k>;
+ clock-names = "apb", "hosc", "losc";
resets = <&apb0_reset 0>;
gpio-controller;
#gpio-cells = <3>;
diff --git a/arch/arm/boot/dts/sun9i-a80.dtsi b/arch/arm/boot/dts/sun9i-a80.dtsi
index ab6a221027ef..979ad1aacfb1 100644
--- a/arch/arm/boot/dts/sun9i-a80.dtsi
+++ b/arch/arm/boot/dts/sun9i-a80.dtsi
@@ -678,7 +678,8 @@
<GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&apb0_gates 5>;
+ clocks = <&apb0_gates 5>, <&osc24M>, <&osc32k>;
+ clock-names = "apb", "hosc", "losc";
gpio-controller;
interrupt-controller;
#interrupt-cells = <3>;
@@ -902,7 +903,8 @@
reg = <0x08002c00 0x400>;
interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&apbs_gates 0>;
+ clocks = <&apbs_gates 0>, <&osc24M>, <&osc32k>;
+ clock-names = "apb", "hosc", "losc";
resets = <&apbs_rst 0>;
gpio-controller;
interrupt-controller;
--
git-series 0.8.11
--
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 v3 1/2] pinctrl: sunxi: Add support for interrupt debouncing
From: Maxime Ripard @ 2016-11-14 20:53 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot
Cc: linux-gpio, devicetree, Rob Herring, Chen-Yu Tsai, Maxime Ripard
In-Reply-To: <cover.fa554eb1146d18ec75bf44863543fec4fa4fd3ae.1479156725.git-series.maxime.ripard@free-electrons.com>
The pin controller found in the Allwinner SoCs has support for interrupts
debouncing.
However, this is not done per-pin, preventing us from using the generic
pinconf binding for that, but per irq bank, which, depending on the SoC,
ranges from one to five.
Introduce a device-wide property to deal with this using a microsecond
resolution. We can re-use the per-pin input-debounce property for that, so
let's do it!
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt | 14 ++++++++++++-
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
drivers/pinctrl/sunxi/pinctrl-sunxi.h | 7 ++++++-
3 files changed, 105 insertions(+), 0 deletions(-)
diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt
index 1685821eea41..56debe1db4e8 100644
--- a/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt
@@ -28,6 +28,20 @@ Required properties:
- reg: Should contain the register physical address and length for the
pin controller.
+- clocks: phandle to the clocks feeding the pin controller:
+ - "apb": the gated APB parent clock
+ - "hosc": the high frequency oscillator in the system
+ - "losc": the low frequency oscillator in the system
+
+Note: For backward compatibility reasons, the hosc and losc clocks are only
+required if you need to use the optional input-debounce property. Any new
+device tree should set them.
+
+Optional properties:
+ - input-debounce: Array of debouncing periods in microseconds. One period per
+ irq bank found in the controller. 0 if no setup required.
+
+
Please refer to pinctrl-bindings.txt in this directory for details of the
common pinctrl bindings used by client devices.
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 0facbea5f465..b425be2875d4 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -870,6 +870,88 @@ static int sunxi_pinctrl_build_state(struct platform_device *pdev)
return 0;
}
+static int sunxi_pinctrl_get_debounce_div(struct clk *clk, int freq, int *diff)
+{
+ unsigned long clock = clk_get_rate(clk);
+ unsigned int best_diff = ~0, best_div;
+ int i;
+
+ for (i = 0; i < 8; i++) {
+ int cur_diff = abs(freq - (clock >> i));
+
+ if (cur_diff < best_diff) {
+ best_diff = cur_diff;
+ best_div = i;
+ }
+ }
+
+ *diff = best_diff;
+ return best_div;
+}
+
+static int sunxi_pinctrl_setup_debounce(struct sunxi_pinctrl *pctl,
+ struct device_node *node)
+{
+ unsigned int hosc_diff, losc_diff;
+ unsigned int hosc_div, losc_div;
+ struct clk *hosc, *losc;
+ u8 div, src;
+ int i, ret;
+
+ /* Deal with old DTs that didn't have the oscillators */
+ if (of_count_phandle_with_args(node, "clocks", "#clock-cells") != 3)
+ return 0;
+
+ /* If we don't have any setup, bail out */
+ if (!of_find_property(node, "input-debounce", NULL))
+ return 0;
+
+ losc = devm_clk_get(pctl->dev, "losc");
+ if (IS_ERR(losc))
+ return PTR_ERR(losc);
+
+ hosc = devm_clk_get(pctl->dev, "hosc");
+ if (IS_ERR(hosc))
+ return PTR_ERR(hosc);
+
+ for (i = 0; i < pctl->desc->irq_banks; i++) {
+ unsigned long debounce_freq;
+ u32 debounce;
+
+ ret = of_property_read_u32_index(node, "input-debounce",
+ i, &debounce);
+ if (ret)
+ return ret;
+
+ if (!debounce)
+ continue;
+
+ debounce_freq = DIV_ROUND_CLOSEST(USEC_PER_SEC, debounce);
+ losc_div = sunxi_pinctrl_get_debounce_div(losc,
+ debounce_freq,
+ &losc_diff);
+
+ hosc_div = sunxi_pinctrl_get_debounce_div(hosc,
+ debounce_freq,
+ &hosc_diff);
+
+ if (hosc_diff < losc_diff) {
+ div = hosc_div;
+ src = 1;
+ } else {
+ div = losc_div;
+ src = 0;
+ }
+
+ writel(src | div << 4,
+ pctl->membase +
+ sunxi_irq_debounce_reg_from_bank(i,
+ pctl->desc->irq_bank_base));
+ }
+
+ return 0;
+}
+
int sunxi_pinctrl_init(struct platform_device *pdev,
const struct sunxi_pinctrl_desc *desc)
{
@@ -1032,6 +1114,8 @@ int sunxi_pinctrl_init(struct platform_device *pdev,
pctl);
}
+ sunxi_pinctrl_setup_debounce(pctl, node);
+
dev_info(&pdev->dev, "initialized sunXi PIO driver\n");
return 0;
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
index 0afce1ab12d0..c0d97fe58e84 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
@@ -69,6 +69,8 @@
#define IRQ_STATUS_IRQ_BITS 1
#define IRQ_STATUS_IRQ_MASK ((1 << IRQ_STATUS_IRQ_BITS) - 1)
+#define IRQ_DEBOUNCE_REG 0x218
+
#define IRQ_MEM_SIZE 0x20
#define IRQ_EDGE_RISING 0x00
@@ -266,6 +268,11 @@ static inline u32 sunxi_irq_ctrl_offset(u16 irq)
return irq_num * IRQ_CTRL_IRQ_BITS;
}
+static inline u32 sunxi_irq_debounce_reg_from_bank(u8 bank, unsigned bank_base)
+{
+ return IRQ_DEBOUNCE_REG + (bank_base + bank) * IRQ_MEM_SIZE;
+}
+
static inline u32 sunxi_irq_status_reg_from_bank(u8 bank, unsigned bank_base)
{
return IRQ_STATUS_REG + (bank_base + bank) * IRQ_MEM_SIZE;
--
git-series 0.8.11
^ permalink raw reply related
* [PATCH v3 0/2] pinctrl: sunxi: Support the interrupt debouncing
From: Maxime Ripard @ 2016-11-14 20:53 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot
Cc: linux-gpio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Chen-Yu Tsai,
Maxime Ripard
Hi,
The Allwinner pin controllers can setup a different debouncing period based
on two clocks and a prescaler.
This debouncing is applied to the whole IRQ bank, which prevents us from
using the per-pin property that is usually used.
Let me know what you think,
Maxime
Changes from v2:
- Deal with a debounce set to 0
- Change debounce divider function name
- Used DIV_ROUND_CLOSEST instead of manual division
- Convert the R_PIO too
- Added the Acked-by
Changes from v1:
- Changed the resolution of the debouncing property to microseconds, and
switched to the input-debounce instead of a custom one.
Maxime Ripard (2):
pinctrl: sunxi: Add support for interrupt debouncing
ARM: sunxi: Add the missing clocks to the pinctrl nodes
Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt | 14 ++++++++++++-
arch/arm/boot/dts/sun4i-a10.dtsi | 3 ++-
arch/arm/boot/dts/sun5i.dtsi | 3 ++-
arch/arm/boot/dts/sun6i-a31.dtsi | 6 +++--
arch/arm/boot/dts/sun7i-a20.dtsi | 3 ++-
arch/arm/boot/dts/sun8i-a23-a33.dtsi | 6 +++--
arch/arm/boot/dts/sun8i-h3.dtsi | 6 +++--
arch/arm/boot/dts/sun9i-a80.dtsi | 6 +++--
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
drivers/pinctrl/sunxi/pinctrl-sunxi.h | 7 ++++++-
10 files changed, 127 insertions(+), 11 deletions(-)
base-commit: bc5952be2d424b75ed11ff599b70bc9604e98d42
--
git-series 0.8.11
--
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 1/5] pinctrl: core: Use delayed work for hogs
From: Tony Lindgren @ 2016-11-14 20:52 UTC (permalink / raw)
To: Linus Walleij
Cc: Haojian Zhuang, Masahiro Yamada, Grygorii Strashko,
Nishanth Menon, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Linux-OMAP
In-Reply-To: <20161111202650.GI7138@atomide.com>
* Tony Lindgren <tony@atomide.com> [161111 12:27]:
> * Linus Walleij <linus.walleij@linaro.org> [161111 12:17]:
> > On Tue, Oct 25, 2016 at 11:02 PM, Tony Lindgren <tony@atomide.com> wrote:
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> >
> > I don't see why this is necessary?
>
> It's needed because the pin controller driver has not yet
> finished it's probe at this point. We end up calling functions
> in the device driver where no struct pinctrl_dev is yet known
> to the driver. Asking a device driver to do something before
> it's probe is done does not quite follow the Linux driver model :)
>
> > The hogging was placed inside pinctrl_register() so that any hogs
> > would be taken before it returns, so nothing else can take it
> > before the controller itself has the first chance. This semantic
> > needs to be preserved I think.
> >
> > > + schedule_delayed_work(&pctldev->hog_work,
> > > + msecs_to_jiffies(100));
> >
> > If we arbitrarily delay, something else can go in and take the
> > pins used by the hogs before the pinctrl core? That is what
> > we want to avoid.
> >
> > Hm, 100ms seems arbitrarily chosen BTW. Can it be 1 ms?
> > 1 ns?
>
> Yeah well seems like it should not matter but the race we need
> to remove somehow.
>
> > I'm pretty sure that whatever it is that needs to happen before
> > the hog work runs can race with this delayed work under
> > some circumstances (such as slow external expanders
> > on i2c). It should be impossible for that to happen
> > and I don't think it is?
>
> Yes it's totally possible even with delay set to 0.
>
> Maybe we could add some trigger on the first consumer request
> and if that does not happen use the timer?
Below is what I came up with for removing the race for hogs. We
can do it by not registering the pctldev until in the deferred
work, does that seem OK to you?
Regards,
Tony
8<-----------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Tue, 25 Oct 2016 08:33:35 -0700
Subject: [PATCH] pinctrl: core: Use delayed work for hogs
Having the pin control framework call pin controller functions
before it's probe has finished is not nice as the pin controller
device driver does not yet have struct pinctrl_dev handle.
Let's fix this issue by adding deferred work for late init. This is
needed to be able to add pinctrl generic helper functions that expect
to know struct pinctrl_dev handle. Note that we now need to call
create_pinctrl() directly as we don't want to add the pin controller
to the list of controllers until the hogs are claimed.
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
drivers/pinctrl/core.c | 66 ++++++++++++++++++++++++++++++--------------------
drivers/pinctrl/core.h | 2 ++
2 files changed, 42 insertions(+), 26 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1911,6 +1911,43 @@ static int pinctrl_check_ops(struct pinctrl_dev *pctldev)
}
/**
+ * pinctrl_late_init() - finish pin controller device registration
+ * @work: work struct
+ */
+static void pinctrl_late_init(struct work_struct *work)
+{
+ struct pinctrl_dev *pctldev;
+
+ pctldev = container_of(work, struct pinctrl_dev, late_init.work);
+
+ pctldev->p = create_pinctrl(pctldev->dev);
+ if (IS_ERR(pctldev->p))
+ return;
+
+ kref_get(&pctldev->p->users);
+
+ pctldev->hog_default =
+ pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
+ if (IS_ERR(pctldev->hog_default)) {
+ dev_dbg(pctldev->dev, "failed to lookup the default state\n");
+ } else {
+ if (pinctrl_select_state(pctldev->p, pctldev->hog_default))
+ dev_err(pctldev->dev, "failed to select default state\n");
+ }
+
+ pctldev->hog_sleep = pinctrl_lookup_state(pctldev->p,
+ PINCTRL_STATE_SLEEP);
+ if (IS_ERR(pctldev->hog_sleep))
+ dev_dbg(pctldev->dev, "failed to lookup the sleep state\n");
+
+ mutex_lock(&pinctrldev_list_mutex);
+ list_add_tail(&pctldev->node, &pinctrldev_list);
+ mutex_unlock(&pinctrldev_list_mutex);
+
+ pinctrl_init_device_debugfs(pctldev);
+}
+
+/**
* pinctrl_register() - register a pin controller device
* @pctldesc: descriptor for this pin controller
* @dev: parent device for this pin controller
@@ -1941,6 +1978,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
INIT_RADIX_TREE(&pctldev->pin_group_tree, GFP_KERNEL);
INIT_RADIX_TREE(&pctldev->pin_function_tree, GFP_KERNEL);
INIT_LIST_HEAD(&pctldev->gpio_ranges);
+ INIT_DELAYED_WORK(&pctldev->late_init, pinctrl_late_init);
pctldev->dev = dev;
mutex_init(&pctldev->mutex);
@@ -1975,32 +2013,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
goto out_err;
}
- mutex_lock(&pinctrldev_list_mutex);
- list_add_tail(&pctldev->node, &pinctrldev_list);
- mutex_unlock(&pinctrldev_list_mutex);
-
- pctldev->p = pinctrl_get(pctldev->dev);
-
- if (!IS_ERR(pctldev->p)) {
- pctldev->hog_default =
- pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
- if (IS_ERR(pctldev->hog_default)) {
- dev_dbg(dev, "failed to lookup the default state\n");
- } else {
- if (pinctrl_select_state(pctldev->p,
- pctldev->hog_default))
- dev_err(dev,
- "failed to select default state\n");
- }
-
- pctldev->hog_sleep =
- pinctrl_lookup_state(pctldev->p,
- PINCTRL_STATE_SLEEP);
- if (IS_ERR(pctldev->hog_sleep))
- dev_dbg(dev, "failed to lookup the sleep state\n");
- }
-
- pinctrl_init_device_debugfs(pctldev);
+ schedule_delayed_work(&pctldev->late_init, 0);
return pctldev;
@@ -2023,6 +2036,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
if (pctldev == NULL)
return;
+ cancel_delayed_work_sync(&pctldev->late_init);
mutex_lock(&pctldev->mutex);
pinctrl_remove_device_debugfs(pctldev);
mutex_unlock(&pctldev->mutex);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -37,6 +37,7 @@ struct pinctrl_gpio_range;
* @p: result of pinctrl_get() for this device
* @hog_default: default state for pins hogged by this device
* @hog_sleep: sleep state for pins hogged by this device
+ * @late_init: delayed work for pin controller to finish registration
* @mutex: mutex taken on each pin controller specific action
* @device_root: debugfs root for this device
*/
@@ -55,6 +56,7 @@ struct pinctrl_dev {
struct pinctrl *p;
struct pinctrl_state *hog_default;
struct pinctrl_state *hog_sleep;
+ struct delayed_work late_init;
struct mutex mutex;
#ifdef CONFIG_DEBUG_FS
struct dentry *device_root;
--
2.10.2
^ permalink raw reply
* Re: [PATCH RFC] ARM: dts: add support for Turris Omnia
From: Andrew Lunn @ 2016-11-14 20:28 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: tomas.hlavacek-x+rMaJPWets, Mark Rutland, marex-ynQEQJNshbs,
Jason Cooper, Martin Strba??ka, devicetree-u79uwXL29TY76Z2rM5mHXA,
Rob Herring, Gregory Clement,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Sebastian Hesselbarth
In-Reply-To: <20161114201640.rr32iyjf5a53v33t-jgopVnDzZD+b0XQX99//ntPVjbGH4+40kFgPdswSElo@public.gmane.org>
>
> + i2c@7 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <7>;
> +
> + pcawan: gpio@71 {
> + compatible = "nxp,pca9538";
> + reg = <0x71>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pcawan_pins>;
> +
> + interrupt-parent = <&gpio1>;
> + interrupts = <14 IRQ_TYPE_LEVEL_LOW>;
> +
> + gpio-controller;
> + #gpio-cells = <2>;
> +
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> + };
>
> The interrupt-controller part doesn't seem to work though, at least
>
> + interrupt-parent = <&pcawan>;
> + interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
>
> in the phy node gives an error.
Interrupts don't seem to work very well with the nxp,pca9538. Which
is probably why it is disabled by default.
Andrew
--
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 RFC] ARM: dts: add support for Turris Omnia
From: Uwe Kleine-König @ 2016-11-14 20:16 UTC (permalink / raw)
To: tomas.hlavacek-x+rMaJPWets
Cc: Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth,
Martin Strbačka, Rob Herring, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
marex-ynQEQJNshbs
In-Reply-To: <1479126185.15557.5-TAvD023jEQEN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 6108 bytes --]
Hello Tomas,
On Mon, Nov 14, 2016 at 01:23:05PM +0100, tomas.hlavacek-x+rMaJPWets@public.gmane.org wrote:
> On Sat, Nov 5, 2016 at 9:38 PM, Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org>
> wrote:
> > This machine is an open hardware router by cz.nic driven by a
> > Marvell Armada 385.
> >
> > Signed-off-by: Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org>
> > ---
> >
> > Hello,
> >
> > the following components are working:
> >
> > - WAN port
> > - eMMC
>
> But I not not sure about DDR50 mode. At least with kernel 4.4, that we use
> in production, we had to limit to SDR50 to overcome I/O errors and
> communication instability, if I can remember it correctly. So it might need
> more testing with the current kernel.
I didn't test that extensively, but the eMMC serves my rootfs and I
didn't had any problems so far.
> > Still missing is support for the switch. Wireless fails to probe, didn't
> > debug this up to now. SFP is untested as is UART1.
>
> Actually SFP is connected to SGMII interface of eth1, which is routed
> through SERDES 5. The SGMII line is shared between the SFP and metallic PHY
> 88E1514. There is a autonomous high-speed switch connected to the SFPDET
> signal from SFP cage. It disconnects the metallic SFP and connects SGMII to
> SFP once the module is connected.
>
> The SFP is also connected to the I2C mux port 4 and to GPIO expander for
> reading/driving SFPDET, LOS, TXFLT, TXDIS signals:
>
> &i2c0 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins>;
> status = "okay";
> clock-frequency = <100000>;
>
> i2cmux@70 {
> compatible = "nxp,pca9547";
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0x70>;
> status = "okay";
>
> ...
>
> i2c@7 {
> /* SFP+ GPIO expander */
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <7>;
>
> sfpgpio: gpio@71 {
> compatible = "nxp,pca9538";
> reg = <0x71>;
> interrupt-parent = <&gpio1>;
> interrupts = <14 IRQ_TYPE_LEVEL_LOW>;
> gpio-controller;
> #gpio-cells = <2>;
> };
I have authored a nearly identical snippet, mine looks as follows:
+ i2c@7 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <7>;
+
+ pcawan: gpio@71 {
+ compatible = "nxp,pca9538";
+ reg = <0x71>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pcawan_pins>;
+
+ interrupt-parent = <&gpio1>;
+ interrupts = <14 IRQ_TYPE_LEVEL_LOW>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ };
+ };
The interrupt-controller part doesn't seem to work though, at least
+ interrupt-parent = <&pcawan>;
+ interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
in the phy node gives an error.
> };
> };
> };
>
> We have our proprietary support hacked onto mvneta driver for disconnecting
> PHY on the fly. It is a bit nasty, so I suggest to ignore SFP in this DTS
> altogether and let's wait till "phylink based SFP module support" or
> something alike hits upstream, so we can base the SFP support on solid code;
> unless somebody has a better idea, of course.
>
> >
> >
> > diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts
> > b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> > new file mode 100644
> > index 000000000000..d3cd8a4d713d
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> > @@ -0,0 +1,246 @@
> ...
> > +
> > + /* USB part of the eSATA/USB 2.0 port */
>
> This comment is perhaps some error inherited from my development DTS. We do
> not have any eSATA, perhaps PCIe/USB 2.0 slot.
oh right. I changed it for v3.
> >
> > + usb@58000 {
> > + status = "okay";
> > + };
> > +
> > +
> > +ð0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&ge0_rgmii_pins>;
> > + status = "okay";
> > + phy-mode = "rgmii-id";
> > +
> > + fixed-link {
> > + speed = <1000>;
> > + full-duplex;
> > + };
> > +};
> > +
> > +ð1 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&ge1_rgmii_pins>;
> > + status = "okay";
> > + phy-mode = "rgmii-id";
> > +
> > + fixed-link {
> > + speed = <1000>;
> > + full-duplex;
> > + };
> > +};
> > +
>
> Actually eth0 and eth1 (both are RGMII) are connected to the 88E6176 switch.
> The problem is that from what I have read so far the switch can not operate
> in DSA mode with two CPU ports. We currently operate the switch in "normal
> mode" with the eth0 and eth1 set to fixed-link 1000/full and with
> proprietary driver (derived from OpenWRT switch drivers). I would say that
> these records for eth0 and eth1 are therefore redundant, because it does
> nothing without the switch support and it would most likely change once we
> have DSA driver (using only eth0).
Right. They do nothing currently. In my local tree I have a
specification for the switch which allows to read the phy registers of
the lan ports, but communication isn't possible yet. For this AFAIK I
need at least one of these. I mailed a few iterations with Andrew here,
but no success so far. Also dropping one cpu port from the definition
didn't help.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] ARM: dts: sun8i: reference-design-tablet: ldo_io1 is vcc-touchscreen
From: Maxime Ripard @ 2016-11-14 20:08 UTC (permalink / raw)
To: Hans de Goede
Cc: Chen-Yu Tsai, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree
In-Reply-To: <20161113192203.7101-2-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 620 bytes --]
On Sun, Nov 13, 2016 at 08:22:03PM +0100, Hans de Goede wrote:
> On some Q8 and other tablets ldo_io1 is used as vcc-touchscreen,
> config at as such in sun8i-reference-design-tablet.dtsi.
>
> Note that it will only be enabled when it us actually referenced by
> a foo-supply property in the touchscreen node, so for tablets which
> do not use ldo_io1 as vcc-touchscreen, it will be disabled.
>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Applied, thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] ARM: dts: sun5i: Add touchscreen node to reference-design-tablet.dtsi
From: Maxime Ripard @ 2016-11-14 20:08 UTC (permalink / raw)
To: Hans de Goede
Cc: Chen-Yu Tsai, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree
In-Reply-To: <20161113192203.7101-1-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4706 bytes --]
Hi,
On Sun, Nov 13, 2016 at 08:22:02PM +0100, Hans de Goede wrote:
> Just like on sun8i all sun5i tablets use the same interrupt and power
> gpios for their touchscreens. I've checked all known a13 fex files and
> only the UTOO P66 uses a different gpio for the interrupt.
>
> Add a touchscreen node to sun5i-reference-design-tablet.dtsi, which
> fills in the necessary gpios to avoid duplication in the tablet dts files,
> just like we do in sun8i-reference-design-tablet.dtsi.
>
> This will make future patches adding touchscreen nodes to a13 tablets
> simpler.
>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> arch/arm/boot/dts/sun5i-a13-utoo-p66.dts | 38 ++++++++--------------
> .../boot/dts/sun5i-reference-design-tablet.dtsi | 25 ++++++++++++++
> 2 files changed, 39 insertions(+), 24 deletions(-)
>
> diff --git a/arch/arm/boot/dts/sun5i-a13-utoo-p66.dts b/arch/arm/boot/dts/sun5i-a13-utoo-p66.dts
> index a8b0bcc..3d7ff10 100644
> --- a/arch/arm/boot/dts/sun5i-a13-utoo-p66.dts
> +++ b/arch/arm/boot/dts/sun5i-a13-utoo-p66.dts
> @@ -83,22 +83,6 @@
> allwinner,pins = "PG3";
> };
>
> -&i2c1 {
> - icn8318: touchscreen@40 {
> - compatible = "chipone,icn8318";
> - reg = <0x40>;
> - interrupt-parent = <&pio>;
> - interrupts = <6 9 IRQ_TYPE_EDGE_FALLING>; /* EINT9 (PG9) */
> - pinctrl-names = "default";
> - pinctrl-0 = <&ts_wake_pin_p66>;
> - wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
> - touchscreen-size-x = <800>;
> - touchscreen-size-y = <480>;
> - touchscreen-inverted-x;
> - touchscreen-swapped-x-y;
> - };
> -};
> -
> &mmc2 {
> pinctrl-names = "default";
> pinctrl-0 = <&mmc2_pins_a>;
> @@ -121,20 +105,26 @@
> allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> allwinner,pull = <SUN4I_PINCTRL_PULL_UP>;
> };
> -
> - ts_wake_pin_p66: ts_wake_pin@0 {
> - allwinner,pins = "PB3";
> - allwinner,function = "gpio_out";
> - allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> - allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> - };
> -
> };
>
> ®_usb0_vbus {
> gpio = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4 */
> };
>
> +&touchscreen {
> + compatible = "chipone,icn8318";
> + reg = <0x40>;
> + /* The P66 uses a different EINT then the reference design */
> + interrupts = <6 9 IRQ_TYPE_EDGE_FALLING>; /* EINT9 (PG9) */
> + /* The icn8318 binding expects wake-gpios instead of power-gpios */
> + wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
> + touchscreen-size-x = <800>;
> + touchscreen-size-y = <480>;
> + touchscreen-inverted-x;
> + touchscreen-swapped-x-y;
> + status = "okay";
> +};
> +
> &uart1 {
> /* The P66 uses the uart pins as gpios */
> status = "disabled";
> diff --git a/arch/arm/boot/dts/sun5i-reference-design-tablet.dtsi b/arch/arm/boot/dts/sun5i-reference-design-tablet.dtsi
> index 20cc940..7af488a 100644
> --- a/arch/arm/boot/dts/sun5i-reference-design-tablet.dtsi
> +++ b/arch/arm/boot/dts/sun5i-reference-design-tablet.dtsi
> @@ -41,6 +41,7 @@
> */
> #include "sunxi-reference-design-tablet.dtsi"
>
> +#include <dt-bindings/interrupt-controller/irq.h>
> #include <dt-bindings/pwm/pwm.h>
>
> / {
> @@ -84,6 +85,23 @@
> };
>
> &i2c1 {
> + /*
> + * The gsl1680 is rated at 400KHz and it will not work reliable at
> + * 100KHz, this has been confirmed on multiple different q8 tablets.
> + * All other devices on this bus are also rated for 400KHz.
> + */
> + clock-frequency = <400000>;
> +
> + touchscreen: touchscreen {
> + interrupt-parent = <&pio>;
> + interrupts = <6 11 IRQ_TYPE_EDGE_FALLING>; /* EINT11 (PG11) */
> + pinctrl-names = "default";
> + pinctrl-0 = <&ts_power_pin>;
> + power-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
> + /* Tablet dts must provide reg and compatible */
> + status = "disabled";
> + };
> +
> pcf8563: rtc@51 {
> compatible = "nxp,pcf8563";
> reg = <0x51>;
> @@ -125,6 +143,13 @@
> allwinner,pull = <SUN4I_PINCTRL_PULL_UP>;
> };
>
> + ts_power_pin: ts_power_pin {
> + allwinner,pins = "PB3";
> + allwinner,function = "gpio_out";
> + allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> + allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> + };
> +
For the next release, we'll switch to the generic pin mux properties
("pins" and "function"), and we actually implemented the fact that the
drive and pull properties are optional, so you can drop them both.
You'll need next + http://lists.infradead.org/pipermail/linux-arm-kernel/2016-November/467123.html
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 5/5] media: platform: rcar_drif: Add DRIF support
From: Geert Uytterhoeven @ 2016-11-14 20:04 UTC (permalink / raw)
To: Rob Herring
Cc: Laurent Pinchart, Ramesh Shanmugasundaram, Mark Rutland,
Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
Antti Palosaari, Chris Paterson, Geert Uytterhoeven,
Linux Media Mailing List,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux-Renesas
In-Reply-To: <20161114195200.s62ga2bnutsrocyf@rob-hp-laptop>
Hi Rob,
On Mon, Nov 14, 2016 at 8:52 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Nov 10, 2016 at 11:22:20AM +0200, Laurent Pinchart wrote:
>> On Wednesday 09 Nov 2016 15:44:44 Ramesh Shanmugasundaram wrote:
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/media/renesas,drif.txt
>> > @@ -0,0 +1,136 @@
>> > +Example
>> > +--------
>> > +
>> > +SoC common dtsi file
>> > +
>> > + drif00: rif@e6f40000 {
>> > + compatible = "renesas,r8a7795-drif",
>> > + "renesas,rcar-gen3-drif";
>> > + reg = <0 0xe6f40000 0 0x64>;
>> > + interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
>> > + clocks = <&cpg CPG_MOD 515>;
>> > + clock-names = "fck";
>> > + dmas = <&dmac1 0x20>, <&dmac2 0x20>;
>> > + dma-names = "rx", "rx";
>
> rx, rx? That doesn't make sense. While we don't explicitly disallow
> this, I'm thinking we should. I wonder if there's any case this is
> valid. If not, then a dtc check for this could be added.
The device can be used with either dmac1 or dmac2.
Which one is used is decided at run time, based on the availability of
DMA channels per DMAC, which is a limited resource.
>> > + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
>> > + status = "disabled";
>> > + };
>> > +
>> > + drif01: rif@e6f50000 {
>> > + compatible = "renesas,r8a7795-drif",
>> > + "renesas,rcar-gen3-drif";
>> > + reg = <0 0xe6f50000 0 0x64>;
>> > + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
>> > + clocks = <&cpg CPG_MOD 514>;
>> > + clock-names = "fck";
>> > + dmas = <&dmac1 0x22>, <&dmac2 0x22>;
>> > + dma-names = "rx", "rx";
>> > + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
>> > + status = "disabled";
>> > + };
>> > +
>> > + drif0: rif@0 {
>> > + compatible = "renesas,r8a7795-drif",
>> > + "renesas,rcar-gen3-drif";
>> > + sub-channels = <&drif00>, <&drif01>;
>> > + status = "disabled";
>> > + };
>>
>> I'm afraid this really hurts my eyes, especially using the same compatible
>> string for both the channel and sub-channel nodes.
>>
>> We need to decide how to model the hardware in DT. Given that the two channels
>> are mostly independent having separate DT nodes makes sense to me. However, as
>> they share the clock and sync signals, somehow grouping them makes sense. I
>> see three ways to do so, and there might be more.
>>
>> 1. Adding an extra DT node for the channels group, with phandles to the two
>> channels. This is what you're proposing here.
>>
>> 2. Adding an extra DT node for the channels group, as a parent of the two
>> channels.
>>
>> 3. Adding phandles to the channels, pointing to each other, or possibly a
>> phandle from channel 0 pointing to channel 1.
>>
>> Neither of these options seem perfect to me. I don't like option 1 as the
>> group DT node really doesn't describe a hardware block. If we want to use a DT
>> node to convey group information, option 2 seems better to me. However, it
>> somehow abuses the DT parent-child model that is supposed to describe
>> relationships from a control bus point of view. Option 3 has the drawback of
>> not scaling properly, at least with phandles in both channels pointing to the
>> other one.
>>
>> Rob, Geert, tell me you have a fourth idea I haven't thought of that would
>> solve all those problems :-)
>
> What's the purpose/need for grouping them?
>
> I'm fine with Option 2, though I want to make sure it is really needed.
Each half of a DRIF pair is basically an SPI slave controller without TX
capability, sharing clock and chip-select between the two halves.
Hence you can use either one half to receive 1 bit per clock pulse,
or both halves to receive 2 bits per clock pulse.
You cannot use both halves for independent operation due to the signal
sharing.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
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 2/2] clk: hisilicon: add CRG driver for Hi3516CV300 SoC
From: Stephen Boyd @ 2016-11-14 20:03 UTC (permalink / raw)
To: Jiancheng Xue
Cc: mturquette, robh+dt, mark.rutland, linux-kernel, linux-clk,
devicetree, bin.chen, elder, hermit.wangheming, yanhaifeng,
wenpan, howell.yang
In-Reply-To: <0c3b49a4-f58c-253a-fd87-ab52b0b26010@hisilicon.com>
On 11/14, Jiancheng Xue wrote:
> Hi Stephen,
>
> On 2016/11/12 8:04, Stephen Boyd wrote:
> > On 10/29, Jiancheng Xue wrote:
> >
> > Should be a From: Pan Wen here?
> >
> >> Add CRG driver for Hi3516CV300 SoC. CRG(Clock and Reset
> >> Generator) module generates clock and reset signals used
> >> by other module blocks on SoC.
> >>
> >> Signed-off-by: Pan Wen <wenpan@hisilicon.com>
> >
> > And you should have signed it off? Care to resend or state that
> > this is incorrectly attributed to you instead of Pan Wen?
> >
>
> Pan Wen is the main author of this patch. I just made some small modification
> with agreement from him. Do I need to resend this patch if it's better to add my
> signed-off?
>
Sure I'll apply the resent one. But you should also make sure to
retain Pan Wen's authorship. I'll go do that when applying it.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v7 04/14] ARM: dts: Add xo_clock to sdhc nodes on qcom platforms
From: Stephen Boyd @ 2016-11-14 20:01 UTC (permalink / raw)
To: Ritesh Harjani
Cc: ulf.hansson, linux-mmc, adrian.hunter, shawn.lin, andy.gross,
devicetree, linux-clk, david.brown, linux-arm-msm, georgi.djakov,
alex.lemberg, mateusz.nowak, Yuliy.Izrailov, asutoshd, kdorfman,
david.griego, stummala, venkatg, rnayak, pramod.gurav
In-Reply-To: <1479103248-9491-5-git-send-email-riteshh@codeaurora.org>
On 11/14, Ritesh Harjani wrote:
> Add xo_clock to sdhc clock node on all qcom platforms.
>
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
> ---
> arch/arm/boot/dts/qcom-apq8084.dtsi | 14 ++++++++------
> arch/arm/boot/dts/qcom-msm8974.dtsi | 14 ++++++++------
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 10 ++++++----
> arch/arm64/boot/dts/qcom/msm8996.dtsi | 9 +++++----
> 4 files changed, 27 insertions(+), 20 deletions(-)
>
Is there an update to
Documentation/devicetree/bindings/mmc/sdhci-msm.txt as well?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v7 13/14] mmc: sdhci-msm: Add calibration tuning for CDCLP533 circuit
From: Stephen Boyd @ 2016-11-14 19:59 UTC (permalink / raw)
To: Ritesh Harjani
Cc: ulf.hansson, linux-mmc, adrian.hunter, shawn.lin, andy.gross,
devicetree, linux-clk, david.brown, linux-arm-msm, georgi.djakov,
alex.lemberg, mateusz.nowak, Yuliy.Izrailov, asutoshd, kdorfman,
david.griego, stummala, venkatg, rnayak, pramod.gurav
In-Reply-To: <1479103248-9491-14-git-send-email-riteshh@codeaurora.org>
On 11/14, Ritesh Harjani wrote:
> @@ -575,6 +729,15 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
> dev_dbg(mmc_dev(mmc), "%s: clock=%u uhs=%u ctrl_2=0x%x\n",
> mmc_hostname(host->mmc), host->clock, uhs, ctrl_2);
> sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> +
> + spin_unlock_irq(&host->lock);
> + /* CDCLP533 HW calibration is only required for HS400 mode*/
> + if (host->clock > CORE_FREQ_100MHZ &&
> + msm_host->tuning_done && !msm_host->calibration_done &&
> + (mmc->ios.timing == MMC_TIMING_MMC_HS400))
Drop useless parenthesis.
> + if (!sdhci_msm_cdclp533_calibration(host))
> + msm_host->calibration_done = true;
> + spin_lock_irq(&host->lock);
> }
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v7 14/14] sdhci: sdhci-msm: update dll configuration
From: Stephen Boyd @ 2016-11-14 19:57 UTC (permalink / raw)
To: Ritesh Harjani
Cc: ulf.hansson, linux-mmc, adrian.hunter, shawn.lin, andy.gross,
devicetree, linux-clk, david.brown, linux-arm-msm, georgi.djakov,
alex.lemberg, mateusz.nowak, Yuliy.Izrailov, asutoshd, kdorfman,
david.griego, stummala, venkatg, rnayak, pramod.gurav,
Krishna Konda
In-Reply-To: <1479103248-9491-15-git-send-email-riteshh@codeaurora.org>
On 11/14, Ritesh Harjani wrote:
> @@ -903,7 +998,33 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
> config |= CORE_HC_SELECT_IN_EN;
> writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
> }
> + if (!msm_host->clk_rate && !msm_host->use_cdclp533) {
> + /*
> + * Poll on DLL_LOCK or DDR_DLL_LOCK bits in
> + * CORE_DLL_STATUS to be set. This should get set
> + * within 15 us at 200 MHz.
> + */
> + rc = readl_relaxed_poll_timeout(host->ioaddr +
> + CORE_DLL_STATUS,
> + dll_lock,
> + (dll_lock &
> + (CORE_DLL_LOCK |
> + CORE_DDR_DLL_LOCK)), 10,
> + 1000);
> + if (rc == -ETIMEDOUT)
> + pr_err("%s: Unable to get DLL_LOCK/DDR_DLL_LOCK, dll_status: 0x%08x\n",
> + mmc_hostname(host->mmc), dll_lock);
> + }
> } else {
> + if (!msm_host->use_cdclp533) {
> + /* set CORE_PWRSAVE_DLL bit in CORE_VENDOR_SPEC3 */
These types of comments are totally useless. The code says
exactly what is being done, and the comment is actually wrong in
this case. Please remove all these "set/clear bit X in register
Y" comments.
> + config = readl_relaxed(host->ioaddr +
> + CORE_VENDOR_SPEC3);
> + config &= ~CORE_PWRSAVE_DLL;
> + writel_relaxed(config, host->ioaddr +
> + CORE_VENDOR_SPEC3);
> + }
> +
> /* Select the default clock (free running MCLK) */
> config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
> config &= ~CORE_HC_MCLK_SEL_MASK;
> @@ -1100,6 +1221,13 @@ static int sdhci_msm_probe(struct platform_device *pdev)
> msm_host->use_14lpp_dll_reset = true;
>
> /*
> + * SDCC 5 controller with major version 1, minor version 0x34 and later
> + * with HS 400 mode support will use CM DLL instead of CDC LP 533 DLL.
> + */
> + if ((core_major == 1) && (core_minor < 0x34))
Drop useless parenthesis please.
> + msm_host->use_cdclp533 = true;
> +
> + /*
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH] ARM: dts: sun8i: replace enable-sdio-wakeup with wakeup-source for BananaPi M1+
From: Maxime Ripard @ 2016-11-14 19:53 UTC (permalink / raw)
To: Sudeep Holla
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Chen-Yu Tsai
In-Reply-To: <1479138250-17780-4-git-send-email-sudeep.holla-5wv7dgnIgG8@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 762 bytes --]
On Mon, Nov 14, 2016 at 03:44:10PM +0000, Sudeep Holla wrote:
> Though the mmc core driver will continue to support the legacy
> "enable-sdio-wakeup" property to enable SDIO as the wakeup source,
> "wakeup-source" is the new standard binding.
>
> This patch replaces the legacy "enable-sdio-wakeup" with the unified
> "wakeup-source" property in order to avoid any further copy-paste
> duplication.
>
> Cc: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> Cc: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
Applied, thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 5/5] media: platform: rcar_drif: Add DRIF support
From: Rob Herring @ 2016-11-14 19:52 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Ramesh Shanmugasundaram, mark.rutland-5wv7dgnIgG8,
mchehab-DgEjT+Ai2ygdnm+yROfE0A, hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
sakari.ailus-VuQAYsv1563Yd54FQh9/CA, crope-X3B1VOXEql0,
chris.paterson2-zM6kxYcvzFBBDgjK7y7TUQ,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
linux-media-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <2857106.83gIJRJqtY@avalon>
On Thu, Nov 10, 2016 at 11:22:20AM +0200, Laurent Pinchart wrote:
> Hi Ramesh,
>
> Thank you for the patch.
>
> On Wednesday 09 Nov 2016 15:44:44 Ramesh Shanmugasundaram wrote:
> > This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3 SoCs.
> > The driver exposes each instance of DRIF as a V4L2 SDR device. A DRIF
> > device represents a channel and each channel can have one or two
> > sub-channels respectively depending on the target board.
> >
> > DRIF supports only Rx functionality. It receives samples from a RF
> > frontend tuner chip it is interfaced with. The combination of DRIF and the
> > tuner device, which is registered as a sub-device, determines the receive
> > sample rate and format.
> >
> > In order to be compliant as a V4L2 SDR device, DRIF needs to bind with
> > the tuner device, which can be provided by a third party vendor. DRIF acts
> > as a slave device and the tuner device acts as a master transmitting the
> > samples. The driver allows asynchronous binding of a tuner device that
> > is registered as a v4l2 sub-device. The driver can learn about the tuner
> > it is interfaced with based on port endpoint properties of the device in
> > device tree. The V4L2 SDR device inherits the controls exposed by the
> > tuner device.
> >
> > The device can also be configured to use either one or both of the data
> > pins at runtime based on the master (tuner) configuration.
> >
> > Signed-off-by: Ramesh Shanmugasundaram
> > <ramesh.shanmugasundaram-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org> ---
> > .../devicetree/bindings/media/renesas,drif.txt | 136 ++
> > drivers/media/platform/Kconfig | 25 +
> > drivers/media/platform/Makefile | 1 +
> > drivers/media/platform/rcar_drif.c | 1574
> > ++++++++++++++++++++ 4 files changed, 1736 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/renesas,drif.txt
> > create mode 100644 drivers/media/platform/rcar_drif.c
> >
> > diff --git a/Documentation/devicetree/bindings/media/renesas,drif.txt
> > b/Documentation/devicetree/bindings/media/renesas,drif.txt new file mode
> > 100644
> > index 0000000..d65368a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/renesas,drif.txt
> > @@ -0,0 +1,136 @@
> > +Renesas R-Car Gen3 Digital Radio Interface controller (DRIF)
> > +------------------------------------------------------------
> > +
> > +R-Car Gen3 DRIF is a serial slave device. It interfaces with a master
> > +device as shown below
> > +
> > ++---------------------+ +---------------------+
> > +| |-----SCK------->|CLK |
> > +| Master |-----SS-------->|SYNC DRIFn (slave) |
> > +| |-----SD0------->|D0 |
> > +| |-----SD1------->|D1 |
> > ++---------------------+ +---------------------+
> > +
> > +Each DRIF channel (drifn) consists of two sub-channels (drifn0 & drifn1).
> > +The sub-channels are like two individual channels in itself that share the
> > +common CLK & SYNC. Each sub-channel has it's own dedicated resources like
> > +irq, dma channels, address space & clock.
> > +
> > +The device tree model represents the channel and each of it's sub-channel
> > +as a separate node. The parent channel ties the sub-channels together with
> > +their phandles.
> > +
> > +Required properties of a sub-channel:
> > +-------------------------------------
> > +- compatible: "renesas,r8a7795-drif" if DRIF controller is a part of
> > R8A7795 SoC.
> > + "renesas,rcar-gen3-drif" for a generic R-Car Gen3 compatible
> > device.
> > + When compatible with the generic version, nodes must list the
> > + SoC-specific version corresponding to the platform first
> > + followed by the generic version.
> > +- reg: offset and length of that sub-channel.
> > +- interrupts: associated with that sub-channel.
> > +- clocks: phandle and clock specifier of that sub-channel.
> > +- clock-names: clock input name string: "fck".
> > +- dmas: phandles to the DMA channel of that sub-channel.
> > +- dma-names: names of the DMA channel: "rx".
> > +
> > +Optional properties of a sub-channel:
> > +-------------------------------------
> > +- power-domains: phandle to the respective power domain.
> > +
> > +Required properties of a channel:
> > +---------------------------------
> > +- pinctrl-0: pin control group to be used for this channel.
> > +- pinctrl-names: must be "default".
> > +- sub-channels : phandles to the two sub-channels.
> > +
> > +Optional properties of a channel:
> > +---------------------------------
> > +- port: child port node of a channel that defines the local and remote
> > + endpoints. The remote endpoint is assumed to be a tuner subdevice
> > + endpoint.
> > +- renesas,syncmd : sync mode
> > + 0 (Frame start sync pulse mode. 1-bit width pulse
> > + indicates start of a frame)
> > + 1 (L/R sync or I2S mode) (default)
> > +- renesas,lsb-first : empty property indicates lsb bit is received
> > first.
> > + When not defined msb bit is received first (default)
>
> Shouldn't those two properties be instead queried from the tuner at runtime
> through the V4L2 subdev API ?
>
> > +- renesas,syncac-pol-high : empty property indicates sync signal polarity.
> > + When defined, active high or high->low sync signal.
> > + When not defined, active low or low->high sync signal
> > + (default)
>
> This could be queried too, except that an inverter could be present on the
> board, so it has to be specified in DT. I would however try to standardize it
> the same way that hsync-active and vsync-active are standardized in
> Documentation/devicetree/bindings/media/video-interfaces.txt.
>
> > +- renesas,dtdl : delay between sync signal and start of reception.
>
> Are this and the next property meant to account for PCB traces delays ?
>
> > + Must contain one of the following values:
> > + 0 (no bit delay)
> > + 50 (0.5-clock-cycle delay)
> > + 100 (1-clock-cycle delay) (default)
> > + 150 (1.5-clock-cycle delay)
> > + 200 (2-clock-cycle delay)
>
> How about specifying the property in half clock cycle units, from 0 to 4 ?
>
> > +- renesas,syncdl : delay between end of reception and sync signal
> > edge.
> > + Must contain one of the following values:
> > + 0 (no bit delay) (default)
> > + 50 (0.5-clock-cycle delay)
> > + 100 (1-clock-cycle delay)
> > + 150 (1.5-clock-cycle delay)
> > + 200 (2-clock-cycle delay)
> > + 300 (3-clock-cycle delay)
> > +
> > +Example
> > +--------
> > +
> > +SoC common dtsi file
> > +
> > + drif00: rif@e6f40000 {
> > + compatible = "renesas,r8a7795-drif",
> > + "renesas,rcar-gen3-drif";
> > + reg = <0 0xe6f40000 0 0x64>;
> > + interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&cpg CPG_MOD 515>;
> > + clock-names = "fck";
> > + dmas = <&dmac1 0x20>, <&dmac2 0x20>;
> > + dma-names = "rx", "rx";
rx, rx? That doesn't make sense. While we don't explicitly disallow
this, I'm thinking we should. I wonder if there's any case this is
valid. If not, then a dtc check for this could be added.
> > + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> > + status = "disabled";
> > + };
> > +
> > + drif01: rif@e6f50000 {
> > + compatible = "renesas,r8a7795-drif",
> > + "renesas,rcar-gen3-drif";
> > + reg = <0 0xe6f50000 0 0x64>;
> > + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&cpg CPG_MOD 514>;
> > + clock-names = "fck";
> > + dmas = <&dmac1 0x22>, <&dmac2 0x22>;
> > + dma-names = "rx", "rx";
> > + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> > + status = "disabled";
> > + };
> > +
> > + drif0: rif@0 {
> > + compatible = "renesas,r8a7795-drif",
> > + "renesas,rcar-gen3-drif";
> > + sub-channels = <&drif00>, <&drif01>;
> > + status = "disabled";
> > + };
>
> I'm afraid this really hurts my eyes, especially using the same compatible
> string for both the channel and sub-channel nodes.
>
> We need to decide how to model the hardware in DT. Given that the two channels
> are mostly independent having separate DT nodes makes sense to me. However, as
> they share the clock and sync signals, somehow grouping them makes sense. I
> see three ways to do so, and there might be more.
>
> 1. Adding an extra DT node for the channels group, with phandles to the two
> channels. This is what you're proposing here.
>
> 2. Adding an extra DT node for the channels group, as a parent of the two
> channels.
>
> 3. Adding phandles to the channels, pointing to each other, or possibly a
> phandle from channel 0 pointing to channel 1.
>
> Neither of these options seem perfect to me. I don't like option 1 as the
> group DT node really doesn't describe a hardware block. If we want to use a DT
> node to convey group information, option 2 seems better to me. However, it
> somehow abuses the DT parent-child model that is supposed to describe
> relationships from a control bus point of view. Option 3 has the drawback of
> not scaling properly, at least with phandles in both channels pointing to the
> other one.
>
> Rob, Geert, tell me you have a fourth idea I haven't thought of that would
> solve all those problems :-)
What's the purpose/need for grouping them?
I'm fine with Option 2, though I want to make sure it is really needed.
Rob
--
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 2/5] media: i2c: max2175: Add MAX2175 support
From: Rob Herring @ 2016-11-14 19:41 UTC (permalink / raw)
To: Ramesh Shanmugasundaram
Cc: mark.rutland-5wv7dgnIgG8, mchehab-DgEjT+Ai2ygdnm+yROfE0A,
hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
sakari.ailus-VuQAYsv1563Yd54FQh9/CA, crope-X3B1VOXEql0,
chris.paterson2-zM6kxYcvzFBBDgjK7y7TUQ,
laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
linux-media-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1478706284-59134-3-git-send-email-ramesh.shanmugasundaram-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>
On Wed, Nov 09, 2016 at 03:44:41PM +0000, Ramesh Shanmugasundaram wrote:
> This patch adds driver support for MAX2175 chip. This is Maxim
> Integrated's RF to Bits tuner front end chip designed for software-defined
> radio solutions. This driver exposes the tuner as a sub-device instance
> with standard and custom controls to configure the device.
>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>
> ---
> .../devicetree/bindings/media/i2c/max2175.txt | 61 +
It's preferred that bindings are a separate patch.
> drivers/media/i2c/Kconfig | 4 +
> drivers/media/i2c/Makefile | 2 +
> drivers/media/i2c/max2175/Kconfig | 8 +
> drivers/media/i2c/max2175/Makefile | 4 +
> drivers/media/i2c/max2175/max2175.c | 1558 ++++++++++++++++++++
> drivers/media/i2c/max2175/max2175.h | 108 ++
> 7 files changed, 1745 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/max2175.txt
> create mode 100644 drivers/media/i2c/max2175/Kconfig
> create mode 100644 drivers/media/i2c/max2175/Makefile
> create mode 100644 drivers/media/i2c/max2175/max2175.c
> create mode 100644 drivers/media/i2c/max2175/max2175.h
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/max2175.txt b/Documentation/devicetree/bindings/media/i2c/max2175.txt
> new file mode 100644
> index 0000000..69f0dad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/max2175.txt
> @@ -0,0 +1,61 @@
> +Maxim Integrated MAX2175 RF to Bits tuner
> +-----------------------------------------
> +
> +The MAX2175 IC is an advanced analog/digital hybrid-radio receiver with
> +RF to Bits® front-end designed for software-defined radio solutions.
> +
> +Required properties:
> +--------------------
> +- compatible: "maxim,max2175" for MAX2175 RF-to-bits tuner.
> +- clocks: phandle to the fixed xtal clock.
> +- clock-names: name of the fixed xtal clock.
> +- port: child port node of a tuner that defines the local and remote
> + endpoints. The remote endpoint is assumed to be an SDR device
> + that is capable of receiving the digital samples from the tuner.
> +
> +Optional properties:
> +--------------------
> +- maxim,slave : empty property indicates this is a slave of
> + another master tuner. This is used to define two
> + tuners in diversity mode (1 master, 1 slave). By
> + default each tuner is an individual master.
> +- maxim,refout-load-pF: load capacitance value (in pF) on reference
Please add 'pF' to property-units.txt.
> + output drive level. The possible load values are
> + 0pF (default - refout disabled)
> + 10pF
> + 20pF
> + 30pF
> + 40pF
> + 60pF
> + 70pF
> +- maxim,am-hiz : empty property indicates AM Hi-Z filter path is
> + selected for AM antenna input. By default this
> + filter path is not used.
> +
> +Example:
> +--------
> +
> +Board specific DTS file
> +
> +/* Fixed XTAL clock node */
> +maxim_xtal: maximextal {
clock {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <36864000>;
> +};
> +
> +/* A tuner device instance under i2c bus */
> +max2175_0: tuner@60 {
> + compatible = "maxim,max2175";
> + reg = <0x60>;
> + clocks = <&maxim_xtal>;
> + clock-names = "xtal";
> + maxim,refout-load-pF = <10>;
> +
> + port {
> + max2175_0_ep: endpoint {
> + remote-endpoint = <&slave_rx_v4l2_sdr_device>;
'v4l2' is not something that should appear in a DT.
> + };
> + };
> +
> +};
--
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 v7 08/14] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm
From: Stephen Boyd @ 2016-11-14 19:37 UTC (permalink / raw)
To: Ritesh Harjani
Cc: ulf.hansson, linux-mmc, adrian.hunter, shawn.lin, andy.gross,
devicetree, linux-clk, david.brown, linux-arm-msm, georgi.djakov,
alex.lemberg, mateusz.nowak, Yuliy.Izrailov, asutoshd, kdorfman,
david.griego, stummala, venkatg, rnayak, pramod.gurav
In-Reply-To: <1479103248-9491-9-git-send-email-riteshh@codeaurora.org>
On 11/14, Ritesh Harjani wrote:
> @@ -577,6 +578,90 @@ static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
> return SDHCI_MSM_MIN_CLOCK;
> }
>
> +/**
> + * __sdhci_msm_set_clock - sdhci_msm clock control.
> + *
> + * Description:
> + * Implement MSM version of sdhci_set_clock.
> + * This is required since MSM controller does not
> + * use internal divider and instead directly control
> + * the GCC clock as per HW recommendation.
> + **/
> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> + u16 clk;
> + unsigned long timeout;
> +
> + /*
> + * Keep actual_clock as zero -
> + * - since there is no divider used so no need of having actual_clock.
> + * - MSM controller uses SDCLK for data timeout calculation. If
> + * actual_clock is zero, host->clock is taken for calculation.
> + */
> + host->mmc->actual_clock = 0;
> +
> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> +
> + if (clock == 0)
> + return;
> +
> + /*
> + * MSM controller do not use clock divider.
> + * Thus read SDHCI_CLOCK_CONTROL and only enable
> + * clock with no divider value programmed.
> + */
> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +
> + clk |= SDHCI_CLOCK_INT_EN;
> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +
> + /* Wait max 20 ms */
> + timeout = 20;
> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
> + & SDHCI_CLOCK_INT_STABLE)) {
> + if (timeout == 0) {
> + pr_err("%s: Internal clock never stabilised\n",
> + mmc_hostname(host->mmc));
> + return;
> + }
> + timeout--;
> + mdelay(1);
> + }
> +
> + clk |= SDHCI_CLOCK_CARD_EN;
> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
This is almost a copy/paste of sdhci_set_clock(). Can we make
sdhci_set_clock() call a __sdhci_set_clock() function that takes
unsigned int clock, and also a flag indicating if we want to set
the internal clock divider or not? Then we can call
__sdhci_set_clock() from sdhci_set_clock() with (clock, true) as
arguments and (clock, false).
> +}
> +
> +/* sdhci_msm_set_clock - Called with (host->lock) spinlock held. */
> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> + int rc;
> +
> + if (!clock) {
> + msm_host->clk_rate = clock;
> + goto out;
> + }
> +
> + spin_unlock_irq(&host->lock);
> + if (clock != msm_host->clk_rate) {
Why do we need to check here? Can't we call clk_set_rate()
Unconditionally?
> + rc = clk_set_rate(msm_host->clk, clock);
> + if (rc) {
> + pr_err("%s: Failed to set clock at rate %u\n",
> + mmc_hostname(host->mmc), clock);
> + spin_lock_irq(&host->lock);
> + goto out;
Or replace the above two lines with goto err;
> + }
> + msm_host->clk_rate = clock;
> + pr_debug("%s: Setting clock at rate %lu\n",
> + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
> + }
And put an err label here.
> + spin_lock_irq(&host->lock);
> +out:
> + __sdhci_msm_set_clock(host, clock);
> +}
> +
> static const struct of_device_id sdhci_msm_dt_match[] = {
> { .compatible = "qcom,sdhci-msm-v4" },
> {},
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2 1/2] mfd: pm8xxx: add support to pm8821
From: Srinivas Kandagatla @ 2016-11-14 19:36 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Lee Jones, Rob Herring, Andy Gross, devicetree, linux-kernel,
linux-arm-msm, linux-soc, linux-arm-kernel
In-Reply-To: <20161114185621.GC27931@tuxbot>
On 14/11/16 18:56, Bjorn Andersson wrote:
> On Mon 14 Nov 09:52 PST 2016, Srinivas Kandagatla wrote:
>
>> This patch adds support to PM8821 PMIC and interrupt support.
>> PM8821 is companion device that supplements primary PMIC PM8921 IC.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>> Changes from v1:
>> - Removed unnessary locking spotted by Bjorn
>> - updated register naming to reflect PM8821
>> - lot of cleanups suggested by Bjorn
>> - rebased on top of Linus Walleij's pm8xxx namespace
>> cleanup patch.
>
> Looks good, just some minor style nits below.
Thanks, I will address all the comments in next version.
>
>>
>> .../devicetree/bindings/mfd/qcom-pm8xxx.txt | 1 +
>> drivers/mfd/qcom-pm8xxx.c | 247 ++++++++++++++++++++-
>> 2 files changed, 238 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> index 37a088f..8f1b4ec 100644
>> --- a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> +++ b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> @@ -11,6 +11,7 @@ voltages and other various functionality to Qualcomm SoCs.
>> Definition: must be one of:
>> "qcom,pm8058"
>> "qcom,pm8921"
>> + "qcom,pm8821"
>
> 8 < 9, so move it one step up please.
sure.. makes sense.
>
>>
>> - #address-cells:
>> Usage: required
>> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> [..]
>> +#define PM8821_SSBI_REG_ADDR_IRQ_BASE 0x100
>> +#define PM8821_SSBI_REG_ADDR_IRQ_MASTER0 (PM8821_SSBI_REG_ADDR_IRQ_BASE + 0x30)
>> +#define PM8821_SSBI_REG_ADDR_IRQ_MASTER1 (PM8821_SSBI_REG_ADDR_IRQ_BASE + 0xb0)
>> +#define PM8821_SSBI_REG(m, b, offset) \
>> + ((m == 0) ? \
>> + (PM8821_SSBI_REG_ADDR_IRQ_MASTER0 + b + offset) : \
>> + (PM8821_SSBI_REG_ADDR_IRQ_MASTER1 + b + offset))
>> +#define PM8821_SSBI_ADDR_IRQ_ROOT(m, b) PM8821_SSBI_REG(m, b, 0x0)
>> +#define PM8821_SSBI_ADDR_IRQ_CLEAR(m, b) PM8821_SSBI_REG(m, b, 0x01)
>> +#define PM8821_SSBI_ADDR_IRQ_MASK(m, b) PM8821_SSBI_REG(m, b, 0x08)
>> +#define PM8821_SSBI_ADDR_IRQ_RT_STATUS(m, b) PM8821_SSBI_REG(m, b, 0x0f)
>
> I like how this cleaned up the rest of the implementation.
>
> [..]
>
>> +static void pm8821_irq_block_handler(struct pm_irq_chip *chip,
>> + int master, int block)
>> +{
>> + int pmirq, irq, i, ret;
>> + unsigned int bits;
>> +
>> + ret = regmap_read(chip->regmap,
>> + PM8821_SSBI_ADDR_IRQ_ROOT(master, block), &bits);
>> + if (ret) {
>> + pr_err("Failed reading %d block ret=%d", block, ret);
>
> "Reading block %d failed ret=%d"
yep.
>
>> + return;
>> + }
>> + if (!bits) {
>> + pr_err("block bit set in master but no irqs: %d", block);
>
> This seems more like a debug thing, either showing missbehaving hardware
> or something odd in the driver. I think you should drop the print or
> make it pr_debug().
okay.
>
>> + return;
>> + }
>
> I would prefer that you just drop the entire if statement, it's just an
> corner case optimization with a info print.
>
i will have a closer look at this part once again.
>> +
>> + /* Convert block offset to global block number */
>> + block += (master * PM8821_BLOCKS_PER_MASTER) - 1;
>> +
>> + /* Check IRQ bits */
>> + for (i = 0; i < 8; i++) {
>> + if (bits & BIT(i)) {
>> + pmirq = block * 8 + i;
>> + irq = irq_find_mapping(chip->irqdomain, pmirq);
>> + generic_handle_irq(irq);
>> + }
>> + }
>> +
>
> Empty line
will fix all the empty lines in next version.
>
>> +}
>> +
>> +static inline void pm8821_irq_master_handler(struct pm_irq_chip *chip,
>> + int master, u8 master_val)
>> +{
>> + int block;
>> +
>> + for (block = 1; block < 8; block++)
>> + if (master_val & BIT(block))
>> + pm8821_irq_block_handler(chip, master, block);
>> +
>
> Empty line
>
>> +}
>> +
>> +static void pm8821_irq_handler(struct irq_desc *desc)
>> +{
>> + struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
>> + struct irq_chip *irq_chip = irq_desc_get_chip(desc);
>> + unsigned int master;
>> + int ret;
>> +
>> + chained_irq_enter(irq_chip, desc);
>> + ret = regmap_read(chip->regmap,
>> + PM8821_SSBI_REG_ADDR_IRQ_MASTER0, &master);
>> + if (ret) {
>> + pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
> ^
> |
> I see you're using vim :)
>
>> + return;
>> + }
>> +
>> + /* bits 1 through 7 marks the first 7 blocks in master 0*/
>
> Indentation
>
>> + if (master & GENMASK(7, 1))
>> + pm8821_irq_master_handler(chip, 0, master);
>> +
>> + /* bit 0 marks if master 1 contains any bits */
>
> Dito
yep.
>
>> + if (!(master & BIT(0)))
>> + goto done;
>> +
>> + ret = regmap_read(chip->regmap,
>> + PM8821_SSBI_REG_ADDR_IRQ_MASTER1, &master);
>> + if (ret) {
>> + pr_err("Failed to read master 1 ret=%d\n", ret);
>> + return;
>
> "goto done;" so that we pass chained_irq_exit()
yes,
>
>> + }
>> +
>> + pm8821_irq_master_handler(chip, 1, master);
>> +
>> +done:
>> + chained_irq_exit(irq_chip, desc);
>> +}
>> +
>
> [..]
>
>> +static void pm8821_irq_mask_ack(struct irq_data *d)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + unsigned int pmirq = irqd_to_hwirq(d);
>> + u8 block, master;
>> + int irq_bit, rc;
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>> +
>> + rc = regmap_update_bits(chip->regmap,
>> + PM8821_SSBI_ADDR_IRQ_MASK(master, block),
>> + BIT(irq_bit), BIT(irq_bit));
>> +
>
> Empty line
>
>> + if (rc) {
>> + pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);
>
> "Failed to mask IRQ %d rc=%d\n"
>
>> + return;
>> + }
>> +
>> + rc = regmap_update_bits(chip->regmap,
>> + PM8821_SSBI_ADDR_IRQ_CLEAR(master, block),
>> + BIT(irq_bit), BIT(irq_bit));
>> +
>
> Empty line
>
>> + if (rc) {
>> + pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
>> + pmirq, rc);
>
> "Failed to clear IRQ %d rc=%d\n"
>
>> + }
>> +
>
> Empty line
>
>> +}
>> +
>> +static void pm8821_irq_unmask(struct irq_data *d)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + unsigned int pmirq = irqd_to_hwirq(d);
>> + int irq_bit, rc;
>> + u8 block, master;
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>> +
>> + rc = regmap_update_bits(chip->regmap,
>> + PM8821_SSBI_ADDR_IRQ_MASK(master, block),
>> + BIT(irq_bit), ~BIT(irq_bit));
>> +
>
> Empty line
>
>> + if (rc)
>> + pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
>
> "Failed to unmask IRQ %d rc=%d\n"
will update it in next version.
>
>> +
>
> Empty line
>
>> +}
>> +
>> +static int pm8821_irq_get_irqchip_state(struct irq_data *d,
>> + enum irqchip_irq_state which,
>> + bool *state)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + int rc, pmirq = irqd_to_hwirq(d);
>> + u8 block, irq_bit, master;
>> + unsigned int bits;
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>> +
>> + rc = regmap_read(chip->regmap,
>> + PM8821_SSBI_ADDR_IRQ_RT_STATUS(master, block), &bits);
>> + if (rc) {
>> + pr_err("Failed Reading Status rc=%d\n", rc);
>
> Odd capitalization, I suggest that you match it to the other functions
> by:
>
> "Reading status of IRQ %d failed rc=%d\n"
>
Okay
>> + return rc;
>> + }
>> +
>> + *state = !!(bits & BIT(irq_bit));
>> +
>> + return rc;
>> +}
>> +
>
> [..]
>
>>
>> static int pm8xxx_probe(struct platform_device *pdev)
>> {
>> + const struct of_device_id *match;
>> + const struct pm_irq_data *data;
>> struct regmap *regmap;
>> int irq, rc;
>> unsigned int val;
>> u32 rev;
>> struct pm_irq_chip *chip;
>> - unsigned int nirqs = PM8XXX_NR_IRQS;
>> +
>> + match = of_match_node(pm8xxx_id_table, pdev->dev.of_node);
>> + if (!match) {
>> + dev_err(&pdev->dev, "No matching driver data found\n");
>> + return -EINVAL;
>> + }
>> +
>> + data = match->data;
>
> data = of_device_get_match_data(&pdev->dev); (from of_device.h)
This is good one.. I will use it in next version.
> Regards,
> Bjorn
>
^ permalink raw reply
* Re: [PATCH v5 00/23] Support qcom's HSIC USB and rewrite USB2 HS support
From: Stephen Boyd @ 2016-11-14 19:35 UTC (permalink / raw)
To: Peter Chen
Cc: Felipe Balbi, Heikki Krogerus, Arnd Bergmann, Neil Armstrong,
linux-arm-msm, linux-usb, linux-kernel, Bjorn Andersson,
devicetree, Rob Herring, Peter Chen, Greg Kroah-Hartman,
Andy Gross, Ivan T. Ivanov, Kishon Vijay Abraham I,
linux-arm-kernel
In-Reply-To: <20161111074017.GA21045@b29397-desktop>
Quoting Peter Chen (2016-11-10 23:40:17)
> On Tue, Oct 18, 2016 at 01:51:39PM -0700, Stephen Boyd wrote:
> > Quoting Peter Chen (2016-10-18 02:31:40)
> > > On Mon, Oct 17, 2016 at 06:56:13PM -0700, Stephen Boyd wrote:
> > > > I've also sent separate patches for other minor pieces to make this
> > > > all work. The full tree can be found here[2], hacks and all to get
> > > > things working. I've tested this on the db410c, apq8074 dragonboard,
> > > > and ifc6410 with configfs gadgets and otg cables.
> > > >
> > > > Patches based on v4.8-rc1
> >
> > Oops should be v4.9-rc1 here.
> >
> > > >
> > > > Changes from v4:
> > > > * Picked up Acks from Rob
> > > > * Updated HS phy init sequence DT property to restrict it to offsets
> > >
> > > I remembered that you got all my acks for chipidea patches, right? I did
> > > not check for this series.
> >
> > Sorry I've added in one more patch
> >
> > usb: chipidea: Emulate OTGSC interrupt enable path
> >
> > to fix extcon interrupt emulation even further.
> >
> > >
> > > Besides, the patch "gpu: Remove depends on RESET_CONTROLLER when not a
> > > provider" [1] still not be accepted, I need this patch to be merged
> > > first, then apply your chipidea part, otherwise, there is a building
> > > warning.
> > >
> > > [1] https://patchwork.kernel.org/patch/9322583/
> >
> > Yes, I'm going to resend that patch now. I hope that David will apply it
> > for -rc2.
>
> Stephen, just a mind. I have rebased Greg's usb-next tree (v4.9-rc3 on
> it), your GPU fix is still not there.
>
It looks like the patch is in drm-misc. I think they're going to hold
off on merging it until the next merge window though. fb80016af071 is
the commit in drm-misc tree and in linux-next. I'm not sure anything can
be done here besides a cross tree merge or ignore the warning?
^ permalink raw reply
* Re: [PATCH v6 4/4] of/fdt: mark hotpluggable memory
From: Reza Arbab @ 2016-11-14 19:34 UTC (permalink / raw)
To: Michael Ellerman
Cc: Benjamin Herrenschmidt, Paul Mackerras, Andrew Morton,
Rob Herring, Frank Rowand, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, linuxppc-dev, linux-mm, devicetree, Bharata B Rao,
Nathan Fontenot, Stewart Smith, Alistair Popple, Balbir Singh,
Aneesh Kumar K.V, linux-kernel
In-Reply-To: <87bmxii85s.fsf@concordia.ellerman.id.au>
On Mon, Nov 14, 2016 at 10:59:43PM +1100, Michael Ellerman wrote:
>So I'm not opposed to this, but it is a little vague.
>
>What does the "hotpluggable" property really mean?
>
>Is it just a hint to the operating system? (which may or may not be
>Linux).
>
>Or is it a direction, "this memory must be able to be hotunplugged"?
>
>I think you're intending the former, ie. a hint, which is probably OK.
>But it needs to be documented clearly.
Yes, you've got it right. It's just a hint, not a mandate.
I'm about to send v7 which adds a description of "hotpluggable" in the
documentation. Hopefully I've explained it well enough there.
--
Reza Arbab
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH V2 1/2] pinctrl: tegra: Add DT binding for io pads control
From: Rob Herring @ 2016-11-14 19:34 UTC (permalink / raw)
To: Laxman Dewangan
Cc: linus.walleij, mark.rutland, swarren, thierry.reding, gnurou,
yamada.masahiro, jonathanh, linux-gpio, devicetree, linux-tegra,
linux-kernel
In-Reply-To: <1478696782-11657-2-git-send-email-ldewangan@nvidia.com>
On Wed, Nov 09, 2016 at 06:36:21PM +0530, Laxman Dewangan wrote:
> NVIDIA Tegra124 and later SoCs support the multi-voltage level and
> low power state of some of its IO pads. The IO pads can work in
> the voltage of the 1.8V and 3.3V of IO voltage from IO power rail
> sources. When IO interfaces are not used then IO pads can be
> configure in low power state to reduce the power consumption from
> that IO pads.
>
> On Tegra124, the voltage level of IO power rail source is auto
> detected by hardware(SoC) and hence it is only require to configure
> in low power mode if IO pads are not used.
>
> On T210 onwards, the auto-detection of voltage level from IO power
> rail is removed from SoC and hence SW need to configure the PMC
> register explicitly to set proper voltage in IO pads based on
> IO rail power source voltage.
>
> Add DT binding document for detailing the DT properties for
> configuring IO pads voltage levels and its power state.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>
> ---
> Changes from V1:
> The DT binding document is modified to explain the regulator handle
> for different IOs and how can it be passed from the DT.
> ---
> .../bindings/pinctrl/nvidia,tegra-io-pad.txt | 126 +++++++++++++++++++++
> 1 file changed, 126 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
> new file mode 100644
> index 0000000..6ca961f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
> @@ -0,0 +1,126 @@
> +NVIDIA Tegra PMC IO pad controller
> +
> +NVIDIA Tegra124 and later SoCs support the multi-voltage level and
> +low power state of some of its IO pads. When IO interface are not
> +used then IO pads can be configure in low power state to reduce
> +the power from that IO pads. The IO pads can work in the voltage
> +of the 1.8V and 3.3V of IO voltage from power rail sources.
> +
> +On Tegra124, the voltage of IO power rail source is auto detected by
> +SoC and hence it is only require to configure in low power mode if
> +IO pads are not used.
> +
> +On T210 onwards, the HW based auto-detection for IO voltage is removed
> +and hence SW need to configure the PMC register explicitly, to set proper
> +voltage in IO pads, based on IO rail power source voltage.
> +
> +The voltage configurations and low power state of IO pads should be done
> +in boot if it is not going to change other wise dynamically based on IO
> +rail voltage on that IO pads and usage of IO pads
s/other wise/otherwise/
The end of the sentence is not clear and missing a period.
> +
> +The DT property of the io pads must be under the node of pmc i.e.
> +pmc@7000e400 for Tegra124 onwards.
s/io/IO/
> +
> +Please refer to <pinctrl-bindings.txt> in this directory for details of the
> +common pinctrl bindings used by client devices, including the meaning of the
> +phrase "pin configuration node".
> +
> +Tegra's pin configuration nodes act as a container for an arbitrary number of
> +subnodes. Each of these subnodes represents some desired configuration for an
> +IO pads, or a list of IO pads. This configuration can include the voltage and
> +power enable/disable control
> +
> +The name of each subnode is not important; all subnodes should be enumerated
> +and processed purely based on their content. Each subnode only affects those
> +parameters that are explicitly listed. Unspecified is represented as an absent
> +property,
> +
> +See the TRM to determine which properties and values apply to each IO pads.
> +
> +Required subnode-properties:
> +==========================
> +- pins : An array of strings. Each string contains the name of an IO pads. Valid
> + values for these names are listed below.
> +
> +Optional subnode-properties:
> +==========================
> +Following properties are supported from generic pin configuration explained
> +in <dt-bindings/pinctrl/pinctrl-binding.txt>.
> +low-power-enable: enable low power mode.
> +low-power-disable: disable low power mode.
> +
> +Valid values for pin for T124 are:
> + audio, bb, cam, comp, csia, csib, csie, dsi, dsib, dsic, dsid, hdmi,
> + hsic, hv, lvds, mipi-bias, nand, pex-bias, pex-clk1, pex-clk2,
> + pex-ctrl, sdmmc1, sdmmc3, sdmmc4, sys-ddc, uart, usb0, usb1, usb2,
> + usb-bias
> +
> +Valid values for pin for T210 are:
> + audio, audio-hv, cam, csia, csib, csic, csid, csie, csif,
> + dbg, debug-nonao, dmic, dp, dsi, dsib, dsic, dsid, emmc, emmc2,
> + gpio, hdmi, hsic, lvds, mipi-bias, pex-bias, pex-clk1, pex-clk2,
> + pex-ctrl, sdmmc1, sdmmc3, spi, spi-hv, uart, usb-bias, usb0,
> + usb1, usb2, usb3.
> +
> +To find out the IO rail voltage for setting the voltage of IO pad by SW,
> +the regulator supply handle must provided from the DT and it is explained
> +in the regulator DT binding document
> + <devicetree/bindings/regulator/regulator.txt>.
> +For example, for GPIO rail the supply name is vddio-gpio and regulator
> +handle is supplied from DT as
> + vddio-gpio-supply = <®ulator_xyz>;
> +
> +For T210, following IO pads support the 1.8V/3.3V and the corresponding
> +io voltage pin names are as follows:
> + audio -> vddio-audio
> + audio-hv -> vddio-audio-hv
> + cam ->vddio-cam
> + dbg -> vddio-dbg
> + dmic -> vddio-dmic
> + gpio -> vddio-gpio
> + pex-ctrl -> vddio-pex-ctrl
> + sdmmc1 -> vddio-sdmmc1
> + sdmmc3 -> vddio-sdmmc3
> + spi -> vddio-spi
> + spi-hv -> vddio-spi-hv
> + uart -> vddio-uart
> +
> +Example:
> + i2c@7000d000 {
> + pmic@3c {
> + regulators {
> + vddio_sdmmc1: ldo2 {
> + /* Regulator entries for LDO2 */
> + };
> +
> + vdd_cam: ldo3 {
> + /* Regulator entries for LDO3 */
> + };
> + };
> + };
> + };
> +
> + pmc@7000e400 {
> + vddio-cam = <&vdd_cam>;
Missing -supply.
> + vddio-sdmmc1-supply = <&vddio_sdmmc1>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&tegra_io_pad_volt_default>;
> + tegra_io_pad_volt_default: common {
> + audio-hv {
> + pins = "audio-hv";
> + low-power-disable;
> + };
> +
> + gpio {
> + pins = "gpio";
> + low-power-disable;
> + };
> +
> + audio {
> + pins = "audio", "dmic", "sdmmc3";
What's the purpose of grouping these?
> + low-power-enable;
> + };
> + };
> +
> + };
> --
> 2.1.4
>
^ permalink raw reply
* Re: [PATCH v2 1/2] mfd: pm8xxx: add support to pm8821
From: Srinivas Kandagatla @ 2016-11-14 19:33 UTC (permalink / raw)
To: Stephen Boyd, Lee Jones
Cc: bjorn.andersson, Rob Herring, Andy Gross, devicetree,
linux-kernel, linux-arm-msm, linux-soc, linux-arm-kernel
In-Reply-To: <a1116e3f-32d2-aa2d-9404-1d8147bd8500@codeaurora.org>
On 14/11/16 18:40, Stephen Boyd wrote:
> On 11/14/2016 09:52 AM, Srinivas Kandagatla wrote:
>> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
>> index 7f9620e..dc347d3 100644
>> --- a/drivers/mfd/qcom-pm8xxx.c
>> +++ b/drivers/mfd/qcom-pm8xxx.c
>> +
>> +static void pm8821_irq_handler(struct irq_desc *desc)
>> +{
>> + struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
>> + struct irq_chip *irq_chip = irq_desc_get_chip(desc);
>> + unsigned int master;
>> + int ret;
>> +
>> + chained_irq_enter(irq_chip, desc);
>> + ret = regmap_read(chip->regmap,
>> + PM8821_SSBI_REG_ADDR_IRQ_MASTER0, &master);
>> + if (ret) {
>> + pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
>
> Hm? vi?
yes.. That was good catch!! will fix it in next version...
>
^ permalink raw reply
* Re: [PATCH v2 2/4] dt-bindings: Add TI SCI PM Domains
From: Dave Gerlach @ 2016-11-14 19:20 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring
Cc: Nishanth Menon, devicetree@vger.kernel.org, Santosh Shilimkar,
linux-pm@vger.kernel.org, Kevin Hilman, Sudeep Holla,
Rafael J . Wysocki, linux-kernel@vger.kernel.org, Jon Hunter,
Tero Kristo, Russell King, Keerthy,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAPDyKFofZdFfGA8gbS9FJxAX5Ub-s8yZWDcNVOgwiEBpho4oyw@mail.gmail.com>
Hi,
On 11/11/2016 06:34 AM, Ulf Hansson wrote:
> On 10 November 2016 at 20:56, Dave Gerlach <d-gerlach@ti.com> wrote:
>> Rob, Ulf, Jon,
>>
>> On 10/27/2016 08:15 AM, Dave Gerlach wrote:
>>>
>>> +Jon
>>> On 10/26/2016 04:59 PM, Rob Herring wrote:
>>>>
>>>> On Mon, Oct 24, 2016 at 12:00 PM, Kevin Hilman <khilman@baylibre.com>
>>>> wrote:
>>>>>
>>>>> Dave Gerlach <d-gerlach@ti.com> writes:
>>>>>
>>>>>> Hi,
>>>>>> On 10/21/2016 01:48 PM, Kevin Hilman wrote:
>>>>>>>
>>>>>>> Dave Gerlach <d-gerlach@ti.com> writes:
>>>>>>>
>>>>>>>> Add a generic power domain implementation, TI SCI PM Domains, that
>>>>>>>> will hook into the genpd framework and allow the TI SCI protocol to
>>>>>>>> control device power states.
>>>>>>>>
>>>>>>>> Also, provide macros representing each device index as understood
>>>>>>>> by TI SCI to be used in the device node power-domain references.
>>>>>>>> These are identifiers for the K2G devices managed by the PMMC.
>>>>>>>>
>>>>>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>>>>>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
>>>>>>>> ---
>>>>>>>> .../devicetree/bindings/soc/ti/sci-pm-domain.txt | 54
>>>>>>>> +++++++++++++
>>>>>>>> MAINTAINERS | 2 +
>>>>>>>> include/dt-bindings/genpd/k2g.h | 90
>>>>>>>> ++++++++++++++++++++++
>>>>>>>> 3 files changed, 146 insertions(+)
>>>>>>>> create mode 100644
>>>>>>>> Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>>> create mode 100644 include/dt-bindings/genpd/k2g.h
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>>> b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..32f38a349656
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>>> @@ -0,0 +1,54 @@
>>>>>>>> +Texas Instruments TI-SCI Generic Power Domain
>>>>>>>> +---------------------------------------------
>>>>>>>> +
>>>>>>>> +Some TI SoCs contain a system controller (like the PMMC, etc...)
>>>>>>>> that is
>>>>>>>> +responsible for controlling the state of the IPs that are present.
>>>>>>>> +Communication between the host processor running an OS and the
>>>>>>>> system
>>>>>>>> +controller happens through a protocol known as TI-SCI [1]. This pm
>>>>>>>> domain
>>>>>>>> +implementation plugs into the generic pm domain framework and makes
>>>>>>>> use of
>>>>>>>> +the TI SCI protocol power on and off each device when needed.
>>>>>>>> +
>>>>>>>> +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>>>>>>>> +
>>>>>>>> +PM Domain Node
>>>>>>>> +==============
>>>>>>>> +The PM domain node represents the global PM domain managed by the
>>>>>>>> PMMC,
>>>>>>>> +which in this case is the single implementation as documented by the
>>>>>>>> generic
>>>>>>>> +PM domain bindings in
>>>>>>>> Documentation/devicetree/bindings/power/power_domain.txt.
>>>>>>>> +
>>>>>>>> +Required Properties:
>>>>>>>> +--------------------
>>>>>>>> +- compatible: should be "ti,sci-pm-domain"
>>>>>>>> +- #power-domain-cells: Must be 0.
>>>>>>>> +- ti,sci: Phandle to the TI SCI device to use for managing the
>>>>>>>> devices.
>>>>>>>>
>>>>>>>> +Example:
>>>>>>>> +--------------------
>>>>>>>> +k2g_pds: k2g_pds {
>>>>>>>
>>>>>>>
>>>>>>> should use generic name like "power-contoller", e.g. k2g_pds:
>>>>>>> power-controller
>>>>>>
>>>>>>
>>>>>> Ok, that makes more sense.
>>>>>>
>>>>>>>
>>>>>>>> + compatible = "ti,sci-pm-domain";
>>>>>>>> + #power-domain-cells = <0>;
>>>>>>>> + ti,sci = <&pmmc>;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +PM Domain Consumers
>>>>>>>> +===================
>>>>>>>> +Hardware blocks that require SCI control over their state must
>>>>>>>> provide
>>>>>>>> +a reference to the sci-pm-domain they are part of and a unique
>>>>>>>> device
>>>>>>>> +specific ID that identifies the device.
>>>>>>>> +
>>>>>>>> +Required Properties:
>>>>>>>> +--------------------
>>>>>>>> +- power-domains: phandle pointing to the corresponding PM domain
>>>>>>>> node.
>>>>>>>> +- ti,sci-id: index representing the device id to be passed oevr SCI
>>>>>>>> to
>>>>>>>> + be used for device control.
>>>>>>>
>>>>>>>
>>>>>>> This ID doesn't look right.
>>>>>>>
>>>>>>> Why not use #power-domain-cells = <1> and pass the index in the DT?
>>>>>>> ...
>>>>
>>>>
>>>> Exactly. ti,sci-id is a NAK for me.
>>>
>>>
>>> I was told not to use the onecell during v1 discussion. I agree this would
>>> be
>>> ideal but I cannot due to what the bindings represent, the phandle
>>> parameter is
>>> an index into a list of genpds, whereas we need an actual ID number we can
>>> use
>>> and I do not have the ability to get that from the phandle.
>>>
>>> @Ulf/Jon, is there any hope of bringing back custom xlate functions for
>>> genpd
>>> providers? I don't have a good background on why it was even removed. I
>>> can
>>> maintain a single genpd for all devices but I need a way to parse this ID,
>>> whether it's from a separate property or a phandle. It is locked now to
>>> indexing
>>> into a list of genpds but I need additional per device information for
>>> devices
>>> bound to a genpd and I need either a custom parameter or the ability to
>>> parse
>>> the phandle myself.
>>>
>>
>> Any comments here? The meaning of the phandle onecell is fixed in the genpd
>> framework so I'm not sure how we want to move forward with this, I need to
>> pass a power domain ID to the genpd driver, and if this shouldn't be a new
>> property I'm not sure what direction we should take.
>>
>> Regards,
>> Dave
>>
>>
>>>>
>>>>>>>
>>>>>>>> +See dt-bindings/genpd/k2g.h for the list of valid identifiers for
>>>>>>>> k2g.
>>>>>>>> +
>>>>>>>> +Example:
>>>>>>>> +--------------------
>>>>>>>> +uart0: serial@02530c00 {
>>>>>>>> + compatible = "ns16550a";
>>>>>>>> + ...
>>>>>>>> + power-domains = <&k2g_pds>;
>>>>>>>> + ti,sci-id = <K2G_DEV_UART0>;
>>>>>>>
>>>>>>>
>>>>>>> ... like this:
>>>>>>>
>>>>>>> power-domains = <&k2g_pds K2G_DEV_UART0>;
>>>>>>
>>>>>>
>>>>>> That's how I did it in version one actually. I was able to define my
>>>>>> own xlate function to parse the phandle and get that index, but Ulf
>>>>>> pointed me to this series by Jon Hunter [1] that simplified genpd
>>>>>> providers and dropped the concept of adding your own xlate. This locks
>>>>>> the onecell approach to using a fixed static array of genpds that get
>>>>>> indexed into (without passing the index to the provider, just the
>>>>>> genpd that's looked up), which doesn't fit our usecase, as we don't
>>>>>> want a 1 to 1 genpd to device mapping based on the comments provided
>>>>>> in v1. Now we just use the genpd device attach/detach hooks to parse
>>>>>> the sci-id and then use it in the genpd device start/stop hooks.
>>>>
>>>>
>>>> I have no idea what any of this means. All sounds like driver
>>>> architecture, not anything to do with bindings.
>>>
>>>
>>> This was a response to Kevin, not part of binding description.
>>>
>>>>
>>>>>
>>>>> Ah, right. I remember now. This approach allows you to use a single
>>>>> genpd as discussed earlier.
>>>>>
>>>>> Makes sense now, suggestion retracted.
>>>>
>>>>
>>>> IIRC, the bindings in Jon's case had a node for each domain and didn't
>>>> need any additional property.
>>>
>>>
>>> Yes but we only have one domain and index into it, not into a list of
>>> domains,
>
> Exactly. And this my main point as well. We are not talking about a
> domain property but a device property.
>
>>> so the additional property is solving a different problem.
>
> Yes.
>
> Perhaps you could try to elaborate about what the TI SCI ID really
> represents for the device, as to help Rob understand the bigger
> picture?
>
> To me, the TI SCI ID, is similar to a "conid" for any another "device
> resource" (like clock, pinctrl, regulator etc) which we can describe
> in DT and assign to a device node. The only difference here, is that
> we don't have common API to fetch the resource (like clk_get(),
> regulator_get()), but instead we fetches the device's resource from
> SoC specific code, via genpd's device ->attach() callback.
Thanks for the response. Yes, you've pretty much hit it on the head. It
is not an index into a list of genpds but rather identifies the device
*within* a single genpd. It is a property specific to each device that
resides in a ti-sci-genpd, not a mapping describing which genpd the
device belongs to. The generic power domain binding is concerned with
mapping the device to a specific genpd, which is does fine for us, but
we have a sub mapping for devices that exist inside a genpd which, we
must describe as well, hence the ti,sci-id.
Regards,
Dave
>
> Hope that helps.
>
> Kind regards
> Uffe
>
^ 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