* Re: [PATCH] ARM: dts: sun8i: fix the pinmux for UART1
From: Maxime Ripard @ 2016-10-25 10:51 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Rob Herring, Chen-Yu Tsai, Mark Rutland, Russell King,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20161024170831.38819-1-icenowy-ymACFijhrKM@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 675 bytes --]
On Tue, Oct 25, 2016 at 01:08:31AM +0800, Icenowy Zheng wrote:
> When the patch is applied, the allwinner,driver and allwinner,pull
> properties are removed.
>
> Although they're described to be optional in the devicetree binding,
> without them, the pinmux cannot be initialized, and the uart cannot
> be used.
>
> Add them back to fix the problem, and makes the bluetooth on iNet D978
> Rev2 board work.
>
> Fixes: 82eec384249f (ARM: dts: sun8i: add pinmux for UART1 at PG)
>
> Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
Applied, thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH/RFT v2 12/17] USB: ochi-da8xx: Use a regulator for vbus/overcurrent
From: Sekhar Nori @ 2016-10-25 10:43 UTC (permalink / raw)
To: ahaslam-rdvid1DuHRBWk0Htik3J/w,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
johan-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
khilman-rdvid1DuHRBWk0Htik3J/w, sshtylyov-hkdhdckH98+B+jHODAdFcQ,
david-nq/r/kbU++upp/zk7JDF2g,
manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A,
broonie-DgEjT+Ai2ygdnm+yROfE0A, abailon-rdvid1DuHRBWk0Htik3J/w
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20161024164634.4330-13-ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>
> Currently, the da8xx ohci driver uses a set of gpios and callbacks in
> board files to handle vbus and overcurrent irqs form the power supply.
> However, this does not play nice when moving to a DT based boot were
> we wont have board files.
>
> Instead of requesting and handling the gpio, use the regulator framework
> to take care of enabling and disabling vbus power. This has the benefit
> that we dont need to pass any more platform data to the driver:
>
> These will be handled by the regulator framework:
> set_power -> regulator_enable/regulator_disable
> get_power -> regulator_is_enabled
> get_oci -> regulator_get_mode
> ocic_notify -> regulator notification
>
> We can keep the default potpgt and use the regulator start delay instead:
> potpgt -> regulator startup delay time
>
> The hawk board does not have a GPIO/OVERCURRENT gpio to control vbus,
> (they should not have been decleared/reserved) so, just remove those
> definitions from the hwk board file.
>
> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
> arch/arm/mach-davinci/board-da830-evm.c | 97 ++++++++----------------
> arch/arm/mach-davinci/board-omapl138-hawk.c | 96 +-----------------------
> arch/arm/mach-davinci/include/mach/da8xx.h | 2 +-
> arch/arm/mach-davinci/usb-da8xx.c | 3 +-
> drivers/usb/host/ohci-da8xx.c | 111 ++++++++++++++++++----------
> include/linux/platform_data/usb-davinci.h | 19 -----
> 6 files changed, 105 insertions(+), 223 deletions(-)
Can you separate out the driver enhancement from the platform
(mach-davinci) changes? They need to go through different trees.
Thanks,
Sekhar
--
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: [RFC V4 PATCH 1/6] arm64: dts: msm8992 SoC and LG Bullhead (Nexus 5X) support
From: Jeremy McNicoll @ 2016-10-25 10:41 UTC (permalink / raw)
To: Stephen Boyd
Cc: Jeremy McNicoll, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, robh-DgEjT+Ai2ygdnm+yROfE0A,
andy.gross-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
michael.scott-QSEj5FYQhm4dnm+yROfE0A
In-Reply-To: <20161021192147.GM26139-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
On Fri, Oct 21, 2016 at 12:21:47PM -0700, Stephen Boyd wrote:
> On 10/21, Jeremy McNicoll wrote:
> > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> > index 5dd05de..abb366e 100644
> > --- a/arch/arm64/boot/dts/qcom/Makefile
> > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > @@ -1,6 +1,6 @@
> > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb msm8916-mtp.dtb
> > -dtb-$(CONFIG_ARCH_QCOM) += msm8996-mtp.dtb
> > -dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb
> > +dtb-$(CONFIG_ARCH_QCOM) += msm8996-mtp.dtb apq8096-db820c.dtb
> > +dtb-$(CONFIG_ARCH_QCOM) += msm8992-bullhead-rev-101.dtb
>
> One line per dtb please.
>
> >
> > always := $(dtb-y)
> > subdir-y := $(dts-dirs)
> > diff --git a/arch/arm64/boot/dts/qcom/msm8992-bullhead-rev-101.dts b/arch/arm64/boot/dts/qcom/msm8992-bullhead-rev-101.dts
> > new file mode 100644
> > index 0000000..63fa3b0
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/msm8992-bullhead-rev-101.dts
> > @@ -0,0 +1,42 @@
> > +/* Copyright (c) 2015, LGE Inc. All rights reserved.
> > + * Copyright (c) 2016, The Linux Foundation. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 and
> > + * only version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "msm8992.dtsi"
> > +
> > +/ {
> > + model = "LGE MSM8992 BULLHEAD rev-1.01";
> > + compatible = "qcom,msm8992";
> > + /* required for bootloader to select correct board */
> > + qcom,board-id = <0xb64 0>;
> > +};
>
> Why do we end
> > +
> > +/ {
>
> and then restart the root node? Just keep the same node all the
> time please.
>
> > + aliases {
> > + serial0 = &blsp1_uart2;
> > + };
> > +
> > + chosen {
> > + stdout-path = "serial0:115200n8";
> > + };
> > +
> > + soc {
> > + serial@f991e000 {
> > + status = "okay";
> > + pinctrl-names = "default", "sleep";
> > + pinctrl-0 = <&blsp1_uart2_default>;
> > + pinctrl-1 = <&blsp1_uart2_sleep>;
> > + };
> > + };
> > +};
> > diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
> > new file mode 100644
> > index 0000000..5d69a6b
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
> > @@ -0,0 +1,214 @@
> > +/* Copyright (c) 2013-2016, The Linux Foundation. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 and
> > + * only version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/clock/qcom,gcc-msm8994.h>
> > +
> > +/ {
> > + model = "Qualcomm Technologies, Inc. MSM 8992";
> > + compatible = "qcom,msm8992";
> > + // msm-id and pmic-id are needed by bootloader for
> > + // selecting correct blob
> > + qcom,msm-id = <251 0>, <252 0>;
> > + qcom,pmic-id = <0x10009 0x1000A 0x0 0x0>;
>
> Can't these just be in the final dts file and not in the dtsi
> file? Technically the pmic pairings are a board choice and not an
> SoC choice so they shouldn't be here. The msm-id properties are
> a little easier to justify, but then I thought we decided to only
> put them in the boards that need them?
>
Board-id and pmic-id are currently in the board file,
(msm8992-bullhead-rev-101.dts) and msm-id is staying in msm8992.dtsi
as its a _REASONABLE_ compromise. Same thing for 8994.
[...snip...]
--
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 4/9] pinctrl: meson: allow gpio to request irq
From: Thomas Gleixner @ 2016-10-25 10:39 UTC (permalink / raw)
To: Linus Walleij
Cc: Jerome Brunet, Carlo Caione, Kevin Hilman,
open list:ARM/Amlogic Meson...,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jason Cooper,
Marc Zyngier, Rob Herring, Catalin Marinas, Will Deacon,
Russell King
In-Reply-To: <CACRpkdYUVXny57AC9rKX3VRAyooEk_2XLvqe9jOuT3rOaE75rg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, 25 Oct 2016, Linus Walleij wrote:
> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
>
> >> Isn't this usecase (also as described in the cover letter) a textbook
> >> example of when you should be using hierarchical irqdomain?
> >>
> >> Please check with Marc et al on hierarchical irqdomains.
> >
> > Linus,
> > Do you mean I should create a new hierarchical irqdomains in each of
> > the two pinctrl instances we have in these SoC, these domains being
> > stacked on the one I just added for controller in irqchip ?
> >
> > I did not understand this is what you meant when I asked you the
> > question at ELCE.
>
> Honestly, I do not understand when and where to properly use
> hierarchical irqdomain, even after Marc's talk at ELC-E.
Hierarchical irqdomains are used when you have several levels of interrupt
hardware to deliver an interrupt.
For example on x86 we have:
device --- [IOAPIC] -- [VECTOR]
and we can have this expanded to
device --- [IOAPIC] -- [IRQ Remapping] -- [VECTOR]
and we have more things hanging off the VECTOR domain
device --- [IOAPIC] ---
|--- [VECTOR]
device --- [PCIMSI] ---
So with irq remapping this might look like this:
device --- [IOAPIC] ---
|-----------------------
device --- [PCIMSI] --- |
|---[VECTOR]
device --- [IOAPIC] --- |
|--[IRQ Remapping]------
device --- [PCIMSI] ---
The important part is that this hierarchy deals with a single Linux virq
and all parts of the hierarchy are required for setup and possibly for
mask/ack/eoi.
This is different from a demultiplex interrupt
device --- [DEMUX] --- [GIC]
where the demultiplex interrupt is a different virq than the device
virq. The demux interrupt chip can have a parent relation ship, which can
be required to propagate information, e.g. wake on a device behind the
demux must keep the gic as a wake irq as well. But it's not hierarchical in
the sense of our hierarchical irq domains.
Hope that helps.
Thanks,
tglx
--
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 4/9] pinctrl: meson: allow gpio to request irq
From: Marc Zyngier @ 2016-10-25 10:38 UTC (permalink / raw)
To: Linus Walleij, Jerome Brunet
Cc: Carlo Caione, Kevin Hilman, open list:ARM/Amlogic Meson...,
linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
Thomas Gleixner, Jason Cooper, Rob Herring, Catalin Marinas,
Will Deacon, Russell King
In-Reply-To: <CACRpkdYUVXny57AC9rKX3VRAyooEk_2XLvqe9jOuT3rOaE75rg@mail.gmail.com>
On 25/10/16 10:14, Linus Walleij wrote:
> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>
>>> Isn't this usecase (also as described in the cover letter) a textbook
>>> example of when you should be using hierarchical irqdomain?
>>>
>>> Please check with Marc et al on hierarchical irqdomains.
>>
>> Linus,
>> Do you mean I should create a new hierarchical irqdomains in each of
>> the two pinctrl instances we have in these SoC, these domains being
>> stacked on the one I just added for controller in irqchip ?
>>
>> I did not understand this is what you meant when I asked you the
>> question at ELCE.
>
> Honestly, I do not understand when and where to properly use
> hierarchical irqdomain, even after Marc's talk at ELC-E.
I probably didn't do that good a job explaining it then. Let's try
again. You want to use hierarchical domains when you want to describe an
interrupt whose path traverses multiple controllers without ever being
multiplexed with other signals. As long as you have this 1:1
relationship between controllers, you can use them.
> Which is problematic since quite a few GPIO drivers now
> need to use them.
>
> I will review his slides, in the meantime I would say: whatever
> Marc ACKs is fine with me. I trust this guy 100%. So I guess I
> could ask that he ACK the entire chain of patches
> from GIC->specialchip->GPIO.
Man, I don't even trust myself... ;-)
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH/RFT v2 08/17] ARM: davinci: hawk: add full constraints for ohci plat boot
From: Axel Haslam @ 2016-10-25 10:31 UTC (permalink / raw)
To: Sekhar Nori
Cc: Greg KH, Johan Hovold, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, Alan Stern,
Kevin Hilman, Sergei Shtylyov, David Lechner,
manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A, Mark Brown,
Alexandre Bailon, linux-usb-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <522eeb9a-6fb1-88eb-5c58-7ee209a50fc3-l0cyMroinI0@public.gmane.org>
On Tue, Oct 25, 2016 at 12:28 PM, Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
> On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>
>> The phy framework requests an optional "phy" regulator. If it does
>> not find one, it returns -EPROBE_DEFER. In the case of non-DT based boot
>> for the omap138-lcdk board, this would prevent the usb11 phy to probe
>> correctly and ohci would not enumerate.
>>
>> By calling "regulator_has_full_constraints", An error would be returned
>
> nit: prefer regulator_has_full_constraints()
>
>> instead of DEFER for the "optional" regulator, and the probe of
>> the phy driver can continue normally without a regulator.
>>
>> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>
> Looks good to me. Just drop the "hawk: from subject line since you also
> touch da830 evm. I am not sure what "ohci plat boot" means. How about
> the following:
>
> "ARM: davinci: da8xx: fix OHCI PHY probe for non-DT boot"
>
Will do.
Thanks
Axel.
> Thanks,
> Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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/RFT v2 08/17] ARM: davinci: hawk: add full constraints for ohci plat boot
From: Sekhar Nori @ 2016-10-25 10:28 UTC (permalink / raw)
To: ahaslam-rdvid1DuHRBWk0Htik3J/w,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
johan-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
khilman-rdvid1DuHRBWk0Htik3J/w, sshtylyov-hkdhdckH98+B+jHODAdFcQ,
david-nq/r/kbU++upp/zk7JDF2g,
manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A,
broonie-DgEjT+Ai2ygdnm+yROfE0A, abailon-rdvid1DuHRBWk0Htik3J/w
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20161024164634.4330-9-ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>
> The phy framework requests an optional "phy" regulator. If it does
> not find one, it returns -EPROBE_DEFER. In the case of non-DT based boot
> for the omap138-lcdk board, this would prevent the usb11 phy to probe
> correctly and ohci would not enumerate.
>
> By calling "regulator_has_full_constraints", An error would be returned
nit: prefer regulator_has_full_constraints()
> instead of DEFER for the "optional" regulator, and the probe of
> the phy driver can continue normally without a regulator.
>
> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Looks good to me. Just drop the "hawk: from subject line since you also
touch da830 evm. I am not sure what "ohci plat boot" means. How about
the following:
"ARM: davinci: da8xx: fix OHCI PHY probe for non-DT boot"
Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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: [RFC] ARM: memory: da8xx-ddrctl: new driver
From: Bartosz Golaszewski @ 2016-10-25 10:17 UTC (permalink / raw)
To: Mark Rutland
Cc: Kevin Hilman, Michael Turquette, Sekhar Nori, Rob Herring,
Frank Rowand, Peter Ujfalusi, Russell King, LKML, arm-soc,
linux-drm, linux-devicetree, Jyri Sarha, Tomi Valkeinen,
David Airlie, Laurent Pinchart
In-Reply-To: <20161024170035.GO15620@leverpostej>
2016-10-24 19:00 GMT+02:00 Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>:
> On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>> +
>> + dev = &pdev->dev;
>> + node = dev->of_node;
>> +
>> + /* Find the board name. */
>> + for (parent = node;
>> + !of_node_is_root(parent);
>> + parent = of_get_parent(parent));
>> +
>> + ret = of_property_read_string(parent, "compatible", &board);
>> + if (ret) {
>> + dev_err(dev, "unable to read the soc model\n");
>> + return ret;
>> + }
>
> I can see that you want to expose sysfs knobs for this, but is it really
> necessary to match boards like this? It's very fragile, and commits us
> to maintaining a database of board data (i.e. a board file).
>
> I am very much not keen on that.
>
I Mark,
I don't want to expose any new sysfs interface.
The initial idea was to follow the way the ti-aemif driver is
implemented and expose DT bindings for the memory controller knobs
(initially only the Peripheral Bus Burst Priority Register, tweaking
which is required to make LCDC work correctly on da850 based boards as
mentioned by Kevin). This was rejected as it's not hardware
description but configuration, so it should not be controlled by DT
properties.
The correct approach for this kind of performance knobs doesn't exist
yet. There was a BoF during this year's ELCE in Berlin during which
several ideas were discussed, but no code has been written so far.
Using sysfs would have exactly the same disadvantage - committing to a
stable interface that would have to be maintained indefinitely.
In the end it was decided that a fairly good, temporary solution would
be to create a driver for the da850 DDR controller which would
hardcode the required tweaks for several boards (as the LCDC issue is
known to affect more TI SoCs and boards). Once a framework for
performance knobs is implemented and merged, it would be easy to port
the driver to it as we would not have implemented any stable
interface.
The same solution would be used for the SYSCFG registers on the da8xx SoCs.
Hope that clarifies the need for this patch a bit. I will address all
other issues in v2.
Best regards,
Bartosz Golaszewski
--
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/RFT v2 02/17] ARM: davinci: da8xx: Add CFGCHIP syscon platform declaration.
From: Sekhar Nori @ 2016-10-25 10:17 UTC (permalink / raw)
To: Axel Haslam
Cc: Greg KH, Johan Hovold, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, Alan Stern,
Kevin Hilman, Sergei Shtylyov, David Lechner,
manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A, Mark Brown,
Alexandre Bailon, linux-usb-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <CAKXjFTOVuWRL+yuk6sARa2PAd6j-V4Ej5AmPGz7F7JXKidvJrg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tuesday 25 October 2016 03:07 PM, Axel Haslam wrote:
> Hi Sekar,
>
> On Tue, Oct 25, 2016 at 10:10 AM, Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
>> On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>> From: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>>>
>>> The CFGCHIP registers are used by a number of devices, so using a syscon
>>> device to share them. The first consumer of this will by the phy-da8xx-usb
>>> driver.
>>>
>>> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>>> [Axel: minor fix: change id to -1]
>>
>> Can you please clarify this change? There could be other syscon devices
>> on the chip for other common registers. Why use the singular device-id?
>>
>
> in the case of non DT boot, the phy driver is looking for "syscon" :
>
> d_phy->regmap = syscon_regmap_lookup_by_pdevname("syscon");
>
> if we register the syscon driver with id = 0, the actual name of the syscon
> device will be "syscon.0" and the phy driver will fail to probe, because
> the strncmp match in the syscon driver (syscon_match_pdevname)
> will fail.
>
> should i change the phy driver instead?
Yes, please. Forcing only one syscon region for the whole chip will be
too restrictive, I am pretty sure.
Thanks,
Sekhar
--
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: Device tree hardware auto-configuration
From: Hans de Goede @ 2016-10-25 10:15 UTC (permalink / raw)
To: Palacios, Hector,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
pawel.moll-5wv7dgnIgG8@public.gmane.org,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org,
galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, Pantelis Antoniou
In-Reply-To: <52F917CEA1B9C64C94833D53889D478C014C6CD7-tnWY20uW8+Bm0VuiGEAYKDbuus4N2nEH@public.gmane.org>
Hi,
On 25-10-16 12:07, Palacios, Hector wrote:
> Hello,
>
> Last ELCE 2016 I attended the conference "Device Tree hardware
> auto-configuration" by Hans de Goede. Although the room was not very big, it was fully crowded, sign of the big interest in the embedded sector to find a convenient solution to manage hardware changes based on self-detection of the platform.
>
> Hans also started a thread recently going over this [1].
>
> Examples of what we try to solve are:
> - The same hardware platform has some hardware differences, for example because there are different revisions of the hardware.
> - A platform has several slightly different models but a 95% of the device tree
> is the same (a different GPIO is used to power a regulator, a different model of touch screen controller is used...) and we want to avoid multiple yet very similar DTs.
>
> There is no agreement on where these things should be solved.
> Neither U-Boot nor the kernel are straightforward options.
>
> I would like to share an idea. It is just a sketch and I do apologize if it
> looks naive, but maybe it can be used as foundation for a better solution.
> In the DT, we have the 'compatible' property to match nodes to drivers.
> In a similar way, I was wondering if we could have a new property 'hardware-compatible' that matches nodes to variants of the hardware.
>
> A simple example:
> - Board model A with an I2C touchscreen controller
> - Board model B with a different I2C touchscreen controller (same address)
>
> &i2c1 {
> /* Touchscreen for model A */
> touch_modelA@28 {
> compatible = "aaa,touch";
> hardware-compatible = "model" "A"
> reg = <0x28>;
> };
>
> /* Touchscreen for model B */
> touch_modelB@28 {
> compatible = "bbb,touch";
> hardware-compatible = "model" "B"
> reg = <0x28>;
> };
> };
>
> Then, it would be a question of fixing up the DT at run-time with the hardware identified properties (not with the touch nodes themselves). Let's say we identify that we're running on model A, revision 3 (in e-fuses, eeprom, or by heuristics), so we fixup the DT with root-level properties about the hardware variant:
>
> / {
> hardware,model = "A";
> hardware,revision = "3";
> };
>
> The names of the properties (after the 'hardware,' prefix) could be anything.
> The match would need to be made between the remaining property name and its value. The DT fixup might be done by the boot loader or by a misc kernel driver as in Hans' patch. Then the driver's probe would only be called if the hardware-compatible property values are satisfied.
>
> The idea behind is to not heavily fixup the DT with devices, but instead limit the fixup to a reduced set of properties. Or in other words, you can still tell what hardware you are supposed to have by looking at the DT, and the fixup (whether done the boot loader or the kernel) simply fills in the variables that determine it.
>
> As I said, it's just a sketch-idea that I had floating around since ELCE, and that I wanted to share.
This idea has been discussed before, the more or less common name for it now is devicetree quirks, see:
http://www.spinics.net/lists/devicetree/msg69490.html
http://www.spinics.net/lists/devicetree/msg69494.html
For an initial attempt at implementing this.
Regards,
Hans
--
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/RFT v2 07/17] ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable
From: Sekhar Nori @ 2016-10-25 10:12 UTC (permalink / raw)
To: ahaslam, gregkh, johan, robh+dt, stern, khilman, sshtylyov, david,
manjunath.goudar, broonie, abailon
Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20161024164634.4330-8-ahaslam@baylibre.com>
On Monday 24 October 2016 10:16 PM, ahaslam@baylibre.com wrote:
> diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
> index 9e41a7f..982e105 100644
> --- a/arch/arm/mach-davinci/usb-da8xx.c
> +++ b/arch/arm/mach-davinci/usb-da8xx.c
> @@ -53,11 +53,19 @@ int __init da8xx_register_usb_refclkin(int rate)
>
> static void usb20_phy_clk_enable(struct clk *clk)
> {
> + struct clk *usb20_clk;
> u32 val;
> u32 timeout = 500000; /* 500 msec */
>
> val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
>
> + usb20_clk = clk_get(NULL, "usb20");
We should not be using a NULL device pointer here. Can you pass the musb
device pointer available in the same file? Also, da850_clks[] in da850.c
needs to be fixed to add the matching device name.
> + if (IS_ERR(usb20_clk)) {
> + pr_err("could not get usb20 clk\n");
> + return;
> + }
Thanks,
Sekhar
^ permalink raw reply
* Device tree hardware auto-configuration
From: Palacios, Hector @ 2016-10-25 10:07 UTC (permalink / raw)
To: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
pawel.moll-5wv7dgnIgG8@public.gmane.org,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org,
galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org
Hello,
Last ELCE 2016 I attended the conference "Device Tree hardware
auto-configuration" by Hans de Goede. Although the room was not very big, it was fully crowded, sign of the big interest in the embedded sector to find a convenient solution to manage hardware changes based on self-detection of the platform.
Hans also started a thread recently going over this [1].
Examples of what we try to solve are:
- The same hardware platform has some hardware differences, for example because there are different revisions of the hardware.
- A platform has several slightly different models but a 95% of the device tree
is the same (a different GPIO is used to power a regulator, a different model of touch screen controller is used...) and we want to avoid multiple yet very similar DTs.
There is no agreement on where these things should be solved.
Neither U-Boot nor the kernel are straightforward options.
I would like to share an idea. It is just a sketch and I do apologize if it
looks naive, but maybe it can be used as foundation for a better solution.
In the DT, we have the 'compatible' property to match nodes to drivers.
In a similar way, I was wondering if we could have a new property 'hardware-compatible' that matches nodes to variants of the hardware.
A simple example:
- Board model A with an I2C touchscreen controller
- Board model B with a different I2C touchscreen controller (same address)
&i2c1 {
/* Touchscreen for model A */
touch_modelA@28 {
compatible = "aaa,touch";
hardware-compatible = "model" "A"
reg = <0x28>;
};
/* Touchscreen for model B */
touch_modelB@28 {
compatible = "bbb,touch";
hardware-compatible = "model" "B"
reg = <0x28>;
};
};
Then, it would be a question of fixing up the DT at run-time with the hardware identified properties (not with the touch nodes themselves). Let's say we identify that we're running on model A, revision 3 (in e-fuses, eeprom, or by heuristics), so we fixup the DT with root-level properties about the hardware variant:
/ {
hardware,model = "A";
hardware,revision = "3";
};
The names of the properties (after the 'hardware,' prefix) could be anything.
The match would need to be made between the remaining property name and its value. The DT fixup might be done by the boot loader or by a misc kernel driver as in Hans' patch. Then the driver's probe would only be called if the hardware-compatible property values are satisfied.
The idea behind is to not heavily fixup the DT with devices, but instead limit the fixup to a reduced set of properties. Or in other words, you can still tell what hardware you are supposed to have by looking at the DT, and the fixup (whether done the boot loader or the kernel) simply fills in the variables that determine it.
As I said, it's just a sketch-idea that I had floating around since ELCE, and that I wanted to share.
Thanks for reading,
--
Héctor Palacios
[1] http://www.spinics.net/lists/devicetree/msg145985.html
--
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/RFT v2 06/17] ARM: davinci: da8xx: Fix some redefined symbol warnings
From: Sekhar Nori @ 2016-10-25 10:03 UTC (permalink / raw)
To: ahaslam, gregkh, johan, robh+dt, stern, khilman, sshtylyov, david,
manjunath.goudar, broonie, abailon
Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20161024164634.4330-7-ahaslam@baylibre.com>
On Monday 24 October 2016 10:16 PM, ahaslam@baylibre.com wrote:
> From: Alexandre Bailon <abailon@baylibre.com>
>
> Some macro for DA8xx CFGCHIP are defined in usb-davinci.h,
> but da8xx-cfgchip.h intend to replace them.
> The usb-da8xx.c is using both headers, causing redefined symbol warnings.
> Remove the old macros.
>
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
This is a v4.9-rc bug fix. Can you please post it as a separate patch
for Greg to pick up?
You can add:
Acked-by: Sekhar Nori <nsekhar@ti.com>
Thanks,
Sekhar
^ permalink raw reply
* Re: [PATCH/RFT v2 07/17] ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable
From: Axel Haslam @ 2016-10-25 10:01 UTC (permalink / raw)
To: David Lechner
Cc: Greg KH, Johan Hovold, robh+dt, Sekhar Nori, Alan Stern,
Kevin Hilman, Sergei Shtylyov, manjunath.goudar, Mark Brown,
Alexandre Bailon, linux-usb, devicetree, linux-kernel,
linux-arm-kernel
In-Reply-To: <c2d31f5a-32c6-5e93-2d3d-d6ef75b697fc@lechnology.com>
On Tue, Oct 25, 2016 at 4:53 AM, David Lechner <david@lechnology.com> wrote:
> On 10/24/2016 11:46 AM, ahaslam@baylibre.com wrote:
>>
>> From: Axel Haslam <ahaslam@baylibre.com>
>>
>> While probing ochi phy with usb20 phy as a parent clock for usb11_phy,
>> the usb20_phy clock enable would time out. This is because the usb20
>> module clock needs to enabled while trying to lock the usb20_phy PLL.
>>
>> Call clk enable and get for the usb20 peripheral before trying to
>> enable the phy PLL.
>>
>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>> ---
>
>
>
> This patch can be combined with "ARM: davinci: da8xx: add usb phy clocks"
> since that patch has not been merged yet.
yes, agree, these should be merged.
>
> If you like, I can resubmit my patches from this series along with the
> changes from this patch.
Ok, if you can resubmit those patches with this change included
i can reference them and start making the series shorter.
i will also submit in separate patches the regulator changes, as requested
by Mark.
Regards
Axel.
>
^ permalink raw reply
* Re: [PATCH] ARM: sti: stih410-clocks: Add PROC_STFE as a critical clock
From: Lee Jones @ 2016-10-25 10:01 UTC (permalink / raw)
To: Peter Griffin
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
kernel-F5mvAk5X5gdBDgjK7y7TUQ, patrice.chotard-qxv4g6HH51o,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161025093926.GA3206@griffinp-ThinkPad-X1-Carbon-2nd>
On Tue, 25 Oct 2016, Peter Griffin wrote:
> Hi Lee,
>
> On Tue, 25 Oct 2016, Lee Jones wrote:
>
> > On Mon, 24 Oct 2016, Peter Griffin wrote:
> >
> > > Hi Lee,
> > >
> > > On Mon, 24 Oct 2016, Lee Jones wrote:
> > > > On Tue, 18 Oct 2016, Peter Griffin wrote:
> > > >
> > > > > Once the ST frontend demux HW IP has been enabled, the clock can't
> > > > > be disabled otherwise the system will hang and the board will
> > > > > be unserviceable.
> > > > >
> > > > > To allow balanced clock enable/disable calls in the driver we use
> > > > > the critical clock infrastructure to take an extra reference on the
> > > > > clock so the clock will never actually be disabled.
> > > >
> > > > This is an abuse of the critical-clocks framework, and is exactly the
> > > > type of hack I promised the clk guys I'd try to prevent.
> > >
> > > I expect the best way to do this would be to write some documentation on the
> > > clock-critical DT binding and/or CRITICAL_CLK flag. The only documentation I can
> > > find currently is with the initial patch series [1] and the comment in
> > > clk-provider.h of
> > >
> > > #define CLK_IS_CRITICAL BIT(11) /* do not gate, ever */
> > >
> > > Or the patch decription
> > >
> > > "Critical clocks are those which must not be gated, else undefined
> > > or catastrophic failure would occur. Here we have chosen to
> > > ensure the prepare/enable counts are correctly incremented, so as
> > > not to confuse users with enabled clocks with no visible users."
> > >
> > > Which is the functionality I want for this clock.
> >
> > No, that's not the functionality you want.
>
> Yes it is :)
No, it's the functionally that is most convenient.
> > You want for the clock not
> > to be RE-gated (big difference). Currently, the STFE clock will never
> > be gated, even when a) it's not used and b) can actually be disabled.
> > You're needlessly wasting power here.
>
> IMO it is *never* safe for Linux to gate this clock, as you have no idea
> on the state of the hardware from the primary and secondary bootloaders.
You can say that with any of the clocks on this platform.
> If the clock is enabled when Linux boots, the Linux clock framework *needs*
> to assume the hardware may have been used in previous boot stages, and it should
> not attempt to disable the clock.
None of the boot loaders we use do this.
I have never seen the STFE clock crash a platform.
> > Also, in your use-case there is a visible user, and the prepare/enable
> > counts will be correct.
>
> Your correct there is a visible user, but this is the same as CLK_EXT2F_A9 where
> there are multiple users of the clock (SPI, I2C, UART etc).
Right, but their clocks can not be turned off, ever. So all the boxes
are ticked for criticalness. Not the case with your clock.
> > > > If this, or
> > > > any other IP has some quirks (i.e. once enabled, if this clock is
> > > > subsequently disabled it will have a catastrophic effect on the
> > > > platform), then they should be worked around in the driver.
> > > >
> > > > The correct thing to do here is craft a clk-keep-on flag and ensure it
> > > > is set to true for the effected platform(s)' platform data.
> > >
> > > I'm always wary of creating a driver specific flag, especially when its
> > > purpose is to do the same thing as an existing mechanism provided by the
> > > subsystem of not gating the clock.
> >
> > Using existing sub-system supplied mechanisms in the way they were not
> > intended is sub-optimal (read "hacky").
>
> I think the scope of this flag has been defined in a very narrow way, which is
> making you want to suggest lots of hacks in the client driver.
The scope was narrowed intentionally, buy the maintainers. I am
merely reflecting their views. If you really wish to contend these,
you should take it up with them.
> > > I can see a couple of problems with what you propose:
> > >
> > > 1) You have to put the clk-keep-on flag in every driver which consumes the
> > > clock. IMO it is much better to have this knowledge in the SoC's
> > > clock driver so every consumer of the clock automatically benefits.
> >
> > That would also be fine(ish). The issue is that this problem is
> > board specific, so the platform clock driver would have to contain
> > board level knowledge.
>
> It is not board specific. It is a SoC HW bug, so by definition present on all
> boards which have this SoC.
Okay SoC specific. Is there a SoC specific clock driver?
> > Also, if you were to implement this, it would
> > too mess up reference counting in the core.
This still stands.
> > > 2) You don't benefit from the CLK_IS_CRITICAL reference counting logic in
> > > clk.c. So then each driver has to also work around that to get sensible reference
> > > counts. e.g.
> > >
> > > if (!__clk_is_enabled(clk) && pdata->clk-keep-on)
> > > clk_enable(clk)
> > >
> > > Which seems to me to be fighting against the subsystem. Given that the only use of
> > > _clk_is_enabled() outside drivers/clk is in an old arch/arm/mach-omap2/pm24xx.c
> > > driver I suspect its use is frowned upon, and it shouldn't really be an EXPORTED_SYMBOL.
> >
> > In this instance, since the STFE clock is only used by this IP, I
> > would choose to handle it in the driver.
>
> There are other IPs within this IP block for which upstream drivers don't yet exist.
If they are many and various, we can discuss alternative solutions.
What are they?
> > This can be done using a
> > single flag stored in pdata which should be fetched using
> > of_match_device(). This way there is no need for any more API abuse;
> > either by incorrectly identifying the STFE clock as critical OR
> > invoking any internal __clk_*() calls.
> >
> > Enable the clock once in .probe(), which you already do.
>
> But these drivers are by default built as modules, so when you rmmod, and insmod the
> driver you now have an ever increasing clock reference count. This is the
> problem I was describing in the previous email, and why what you propose is a
> bad idea.
Then the variable would have to be exported.
> > Then, whenever you do any power saving do:
> >
> > suspend()
> > {
> > if (!ddata->enable_clk_once)
> > clk_disable(clk);
> > }
> >
> > resume()
> > {
> > if (!ddata->enable_clk_once)
> > clk_enable(clk);
> > }
> >
> > However, looking at your driver, I think this point might even be
> > moot, since you don't have any power saving. The only time you
> > disable the clock is in the error path. Just replace that with a
> > comment about the platform's unfortunate errata.
>
> This is exactly what I want to avoid doing. The driver already has these
> hacks as it was waiting for the critical clock patches to land so I could remove
> them and fix the problem properly.
>
> Much like you did with the I2C and SPI drivers, where you removed similar hacks
> and added the clock to the critical clock list.
Right, see above.
> > > [1] https://lkml.org/lkml/2016/1/18/272
> >
> > I'm glad you mentioned this. Let's take a look:
> >
> > > Some platforms contain clocks which if gated, will cause undefined or
> > > catastrophic behaviours. As such they are not to be turned off, ever.
> >
> > Not the case here.
> >
> > This clock *can* be gated and can be turned off *sometimes*.
>
> See above, if the clock is on when Linux boots it can never be assumed that it
> is safe to disable it.
See above.
> > > Many of these such clocks do not have devices, thus device drivers
> > > where clocks may be enabled and references taken to ensure they stay
> > > enabled do not exist. Therefore, we must handle these such cases in
> > > the core.
> >
> > This clock *does* have a driver and correct references *can* be
> > taken.
>
> As above this is the same for CLK_EXT2F_A9, which has multiple users in the
> kernel.
>
> The point is we don't wish to have the knowledge in the individual drivers that
> this clock is critical and can destroy the system if it is disabled.
>
> >
> > [...]
> >
> > All I'm saying is, and it's the same thing I've said many times; these
> > types of issues do not exhibit the same set of symptoms as a critical
> > clock by definition. Critical clocks are those which references can
> > not be taken by any other means.
>
> That is not how you are using it currently. See CLK_EXT2F_A9 and
> CLK_TX_ICN_DMU.
>
> > Think of the critical clock
> > framework as a mechanism to circumvent the requirement of writing a
> > special driver which would *only* handle clocks i.e. an interconnect
> > driver in the ST case.
> >
>
> You didn't need a new flag for that. CLK_IGNORE_UNUSED already allowed you to not
> write a specical driver which only handled for example an interconnect clock and
> would ensure it wouldn't be gated by the disable_unused logic.
>
> IMO the point of the critical clock stuff, and what CLK_IGNORE_UNUSED *did not* allow
> you to do is for situations like CLK_EXT2F_A9, CLK_TX_ICN_DMU and PROC_STFE where users
> *do* exist in the kernel, but the SoC clock driver has *special* knowledge of the
> clock tree on the SoC and knows that despite what the drivers are doing with
> their enable/disable calls the clock should never be disabled.
>
> In fact out of what we currently mark as clock-critical on STi platform most
> of the clocks could have been handled perfectly fine with the CLK_IGNORE_UNUSED
> flag (apart from the fact that there is no way to set the flag from DT). It is
> only CLK_EXT2F_A9 and CLK_TX_ICN_DMU which *do* have kernel users where the
> extra functionality of critical clocks is required (we need the additional
> reference taken by the clk framework to avoid the kernel drivers from disabling
> the clock).
Probably best to take your special use-case up with the Clock
Maintainers. As the author of the critical-clocks patch-set, I would
say that your use-case does not tick all the boxes for use of the
critical-clock mechanism, but at the end of the day, it really isn't
my train-set.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/RFT v2 13/17] USB: da8xx: use ohci priv data instead of globals
From: Axel Haslam @ 2016-10-25 9:56 UTC (permalink / raw)
To: David Lechner
Cc: Greg KH, Johan Hovold, robh+dt, Sekhar Nori, Alan Stern,
Kevin Hilman, Sergei Shtylyov, Mark Brown, Alexandre Bailon,
linux-usb, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <0cf6f211-90ba-2f7b-978f-f1cfa1b11554@lechnology.com>
On Tue, Oct 25, 2016 at 3:12 AM, David Lechner <david@lechnology.com> wrote:
> On 10/24/2016 11:46 AM, ahaslam@baylibre.com wrote:
>>
>> From: Axel Haslam <ahaslam@baylibre.com>
>> > static const struct ohci_driver_overrides da8xx_overrides __initconst =
>> > {
>> - .reset = ohci_da8xx_reset
>> + .reset = ohci_da8xx_reset,
>> + .extra_priv_size = sizeof(struct da8xx_ohci_hcd),
>
>
> nit: since you are changing both lines anyway, you might as well make the
> ='s line up.
ok, will do.
>
>> };
>>
>> /*
>>
>
^ permalink raw reply
* Re: [PATCH/RFT v2 15/17] usb: host: ohci-da8xx: Add devicetree bindings documentation
From: Axel Haslam @ 2016-10-25 9:56 UTC (permalink / raw)
To: David Lechner
Cc: Greg KH, Johan Hovold, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
Sekhar Nori, Alan Stern, Kevin Hilman, Sergei Shtylyov,
Mark Brown, Alexandre Bailon, linux-usb-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <523928a1-a321-abb6-504f-a1053c88ce53-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
On Tue, Oct 25, 2016 at 3:02 AM, David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org> wrote:
> On 10/24/2016 11:46 AM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>>
>> From: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>
>> This patch documents the device tree bindings required for
>> the ohci controller found in TI da8xx family of SoC's
>>
>> Signed-off-by: Axel Haslam <ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> ---
>> .../devicetree/bindings/usb/ohci-da8xx.txt | 39
>> ++++++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/usb/ohci-da8xx.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/ohci-da8xx.txt
>> b/Documentation/devicetree/bindings/usb/ohci-da8xx.txt
>> new file mode 100644
>> index 0000000..4251c84
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/ohci-da8xx.txt
>> @@ -0,0 +1,39 @@
>> +DA8XX USB OHCI controller
>> +
>> +Required properties:
>> +
>> + - compatible: Should be "ti,da830-ohci"
>> + - reg: Should contain one register range i.e. start and length
>> + - interrupts: Description of the interrupt line
>> + - phys: Phandle for the PHY device
>> + - phy-names: Should be "usb-phy"
>> +
>> +Optional properties:
>> + - vbus-supply: Regulator that controls vbus power
>
>
>
> Isn't vbus-supply property required?
>
> If it is really supposed to be optional, the ohci driver needs to use
> devm_regulator_get_optional() and handle the case when there is no
> regulator.
>
> I don't see a problem with making it required though since one can just use
> a dummy supply if there is not a real one.
The regulator framework will use a dummy regulator if none is provided.
>
>> +
>> +Example for omap138-lck:
>> +
>> +vbus_fixed: fixed-regulator-vbus {
>> + compatible = "regulator-fixed";
>> + gpio = <&gpio 109 0>;
>> + oc-gpio = <&gpio 36 0>;
>> + regulator-boot-on;
>> + enable-active-high;
>> + regulator-name = "vbus";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <5000000>;
>> +};
>> +
>> +usb_phy: usb-phy {
>> + compatible = "ti,da830-usb-phy";
>> + #phy-cells = <1>;
>> + status = "disabled";
>
>
> why disabled?
>
yes, i copied from the device tree im using, but
i should enable these for the example,
i will fix.
>> +};
>> +usb: usb@0225000 {
>> + compatible = "ti,da830-ohci";
>> + reg = <0x225000 0x1000>;
>> + interrupts = <59>;
>> + phys = <&usb_phy 1>;
>> + phy-names = "usb-phy";
>
>
> missing vbus-supply property
>
>> + status = "disabled";
>
>
> why disabled?
>
>> +};
>>
>
--
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] soc: qcom: Add SoC info driver
From: Imran Khan @ 2016-10-25 9:53 UTC (permalink / raw)
To: Arnd Bergmann
Cc: andy.gross, David Brown, Rob Herring, Mark Rutland,
open list:ARM/QUALCOMM SUPPORT, open list:ARM/QUALCOMM SUPPORT,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
In-Reply-To: <2349770.MVMXr64OoI@wuerfel>
On 10/21/2016 4:03 PM, Arnd Bergmann wrote:
> On Thursday, October 20, 2016 7:36:22 PM CEST Imran Khan wrote:
>> +#include <linux/soc/qcom/socinfo.h>
>> +#include <linux/soc/qcom/smem.h>
>> +
>> +#include <asm/system_misc.h>
>
> I don't see anything here that needs asm/system_misc.h
>
Okay. I will not use this header file here.
>> +const char *hw_platform[] = {
>> + [HW_PLATFORM_UNKNOWN] = "Unknown",
>> + [HW_PLATFORM_SURF] = "Surf",
>> + [HW_PLATFORM_FFA] = "FFA",
>> + [HW_PLATFORM_FLUID] = "Fluid",
>> +};
>> +
>> +const char *qrd_hw_platform_subtype[] = {
>> + [PLATFORM_SUBTYPE_QRD] = "QRD",
>> + [PLATFORM_SUBTYPE_SKUAA] = "SKUAA",
>> +};
>> +
>> +const char *hw_platform_subtype[] = {
>> + [PLATFORM_SUBTYPE_UNKNOWN] = "Unknown",
>> +};
>
>
> Make these all static
>
Okay. Will do this.
>> +
>> +/* Used to parse shared memory. Must match the modem. */
>> +struct socinfo_v0_1 {
>> + uint32_t format;
>> + uint32_t id;
>> + uint32_t version;
>
> s/uint32_t/u32/
>
Okay. Will do this.
>> +
>> +uint32_t socinfo_get_id(void)
>> +{
>> + return (socinfo) ? socinfo->v0_1.id : 0;
>> +}
>> +EXPORT_SYMBOL_GPL(socinfo_get_id);
>> +
>> +uint32_t socinfo_get_version(void)
>> +{
>> + return (socinfo) ? socinfo->v0_1.version : 0;
>> +}
>> +EXPORT_SYMBOL_GPL(socinfo_get_version);
>> +
>> +char *socinfo_get_build_id(void)
>> +{
>> + return (socinfo) ? socinfo->v0_1.build_id : NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(socinfo_get_build_id);
>
> Please remove all the exports and mark the functions static,
> we don't want any drivers poking vendor-specific interfaces
> for this.
>
Yes. I will avoid exporting vendor specific interface.
>> +/* socinfo: sysfs functions */
>
> This seems overly verbose, having both raw and human-readable
> IDs is generally not necessary, pick one of the two. If you
> need any fields that we don't already support in soc_device,
> let's talk about adding them to the generic structure.
>
>
Okay. I will go for human readable IDs. Can we add 2 more fields
in the generic structure.
These 2 fields would be:
vendor: A string for vendor name
serial_number: A string containing serial number for the platform
>> +static ssize_t
>> +qcom_get_image_version(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + char *string_address;
>> +
>> + string_address = socinfo_get_image_version_base_address(dev);
>> + if (IS_ERR_OR_NULL(string_address)) {
>
> IS_ERR_OR_NULL() indicates that your interface is not well-designed.
> Please return either NULL on all failures, or return an error pointer
> on all failures, not both.
>
>> +/* Platform Subtype String is being deprecated. Use Platform
>> + * Subtype ID instead.
>> + */
>
> If it's deprecated, don't add it to the kernel!
>
>
Okay.
>> diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
>> new file mode 100644
>> index 0000000..17ca50a
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/socinfo.h
>
> Merge the header file into the driver.
>
okay.
> Arnd
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH v2 3/4] soc: ti: Add ti_sci_pm_domains driver
From: Ulf Hansson @ 2016-10-25 9:48 UTC (permalink / raw)
To: Dave Gerlach
Cc: Rafael J . Wysocki, Kevin Hilman,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Nishanth Menon, Keerthy, Russell King, Tero Kristo, Sudeep Holla,
Santosh Shilimkar
In-Reply-To: <20161019203347.17893-4-d-gerlach-l0cyMroinI0@public.gmane.org>
On 19 October 2016 at 22:33, Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org> wrote:
> Introduce a ti_sci_pm_domains driver to act as a generic pm domain provider
> to allow each device to attach and associate it's ti-sci-id so that it can
> be controlled through the TI SCI protocol.
>
> This driver implements a simple genpd where each device node has
> a phandle to the power domain node and also must provide an index which
> represents the ID to be passed with TI SCI representing the device using a
> ti,sci-id property. Through this interface the genpd dev_ops start and
> stop hooks will use TI SCI to turn on and off each device as determined
> by pm_runtime usage.
>
> Signed-off-by: Keerthy <j-keerthy-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
Looks good!
Acked-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Kind regards
Uffe
> ---
> MAINTAINERS | 1 +
> arch/arm/mach-keystone/Kconfig | 1 +
> drivers/soc/ti/Kconfig | 12 +++
> drivers/soc/ti/Makefile | 1 +
> drivers/soc/ti/ti_sci_pm_domains.c | 198 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 213 insertions(+)
> create mode 100644 drivers/soc/ti/ti_sci_pm_domains.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d894873c2bff..3eaac5ede847 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11894,6 +11894,7 @@ F: drivers/firmware/ti_sci*
> F: include/linux/soc/ti/ti_sci_protocol.h
> F: Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
> F: include/dt-bindings/genpd/k2g.h
> +F: drivers/soc/ti/ti_sci_pm_domains.c
>
> THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
> M: Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org>
> diff --git a/arch/arm/mach-keystone/Kconfig b/arch/arm/mach-keystone/Kconfig
> index 24bd64dabdfc..18d49465cafb 100644
> --- a/arch/arm/mach-keystone/Kconfig
> +++ b/arch/arm/mach-keystone/Kconfig
> @@ -9,6 +9,7 @@ config ARCH_KEYSTONE
> select ARCH_SUPPORTS_BIG_ENDIAN
> select ZONE_DMA if ARM_LPAE
> select PINCTRL
> + select PM_GENERIC_DOMAINS if PM
> help
> Support for boards based on the Texas Instruments Keystone family of
> SoCs.
> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
> index 3557c5e32a93..39e152abe6b9 100644
> --- a/drivers/soc/ti/Kconfig
> +++ b/drivers/soc/ti/Kconfig
> @@ -38,4 +38,16 @@ config WKUP_M3_IPC
> to communicate and use the Wakeup M3 for PM features like suspend
> resume and boots it using wkup_m3_rproc driver.
>
> +config TI_SCI_PM_DOMAINS
> + tristate "TI SCI PM Domains Driver"
> + depends on TI_SCI_PROTOCOL
> + depends on PM_GENERIC_DOMAINS
> + help
> + Generic power domain implementation for TI device implementing
> + the TI SCI protocol.
> +
> + To compile this as a module, choose M here. The module will be
> + called ti_sci_pm_domains. Note this is needed early in boot before
> + rootfs may be available.
> +
> endif # SOC_TI
> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
> index 48ff3a79634f..7d572736c86e 100644
> --- a/drivers/soc/ti/Makefile
> +++ b/drivers/soc/ti/Makefile
> @@ -5,3 +5,4 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS) += knav_qmss.o
> knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
> obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA) += knav_dma.o
> obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o
> +obj-$(CONFIG_TI_SCI_PM_DOMAINS) += ti_sci_pm_domains.o
> diff --git a/drivers/soc/ti/ti_sci_pm_domains.c b/drivers/soc/ti/ti_sci_pm_domains.c
> new file mode 100644
> index 000000000000..ec76215d64c7
> --- /dev/null
> +++ b/drivers/soc/ti/ti_sci_pm_domains.c
> @@ -0,0 +1,198 @@
> +/*
> + * TI SCI Generic Power Domain Driver
> + *
> + * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/
> + * J Keerthy <j-keerthy-l0cyMroinI0@public.gmane.org>
> + * Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/slab.h>
> +#include <linux/soc/ti/ti_sci_protocol.h>
> +
> +/**
> + * struct ti_sci_genpd_dev_data: holds data needed for every device attached
> + * to this genpd
> + * @idx: index of the device that identifies it with the system
> + * control processor.
> + */
> +struct ti_sci_genpd_dev_data {
> + int idx;
> +};
> +
> +/**
> + * struct ti_sci_pm_domain: TI specific data needed for power domain
> + * @ti_sci: handle to TI SCI protocol driver that provides ops to
> + * communicate with system control processor.
> + * @dev: pointer to dev for the driver for devm allocs
> + * @pd: generic_pm_domain for use with the genpd framework
> + */
> +struct ti_sci_pm_domain {
> + const struct ti_sci_handle *ti_sci;
> + struct device *dev;
> + struct generic_pm_domain pd;
> +};
> +
> +#define genpd_to_ti_sci_pd(gpd) container_of(gpd, struct ti_sci_pm_domain, pd)
> +
> +/**
> + * ti_sci_dev_id(): get prepopulated ti_sci id from struct dev
> + * @dev: pointer to device associated with this genpd
> + *
> + * Returns device_id stored from ti,sci_id property
> + */
> +static int ti_sci_dev_id(struct device *dev)
> +{
> + struct generic_pm_domain_data *genpd_data = dev_gpd_data(dev);
> + struct ti_sci_genpd_dev_data *sci_dev_data = genpd_data->data;
> +
> + return sci_dev_data->idx;
> +}
> +
> +/**
> + * ti_sci_dev_to_sci_handle(): get pointer to ti_sci_handle
> + * @dev: pointer to device associated with this genpd
> + *
> + * Returns ti_sci_handle to be used to communicate with system
> + * control processor.
> + */
> +static const struct ti_sci_handle *ti_sci_dev_to_sci_handle(struct device *dev)
> +{
> + struct generic_pm_domain *pd = pd_to_genpd(dev->pm_domain);
> + struct ti_sci_pm_domain *ti_sci_genpd = genpd_to_ti_sci_pd(pd);
> +
> + return ti_sci_genpd->ti_sci;
> +}
> +
> +/**
> + * ti_sci_dev_start(): genpd device start hook called to turn device on
> + * @dev: pointer to device associated with this genpd to be powered on
> + */
> +static int ti_sci_dev_start(struct device *dev)
> +{
> + const struct ti_sci_handle *ti_sci = ti_sci_dev_to_sci_handle(dev);
> + int idx = ti_sci_dev_id(dev);
> +
> + return ti_sci->ops.dev_ops.get_device(ti_sci, idx);
> +}
> +
> +/**
> + * ti_sci_dev_stop(): genpd device stop hook called to turn device off
> + * @dev: pointer to device associated with this genpd to be powered off
> + */
> +static int ti_sci_dev_stop(struct device *dev)
> +{
> + const struct ti_sci_handle *ti_sci = ti_sci_dev_to_sci_handle(dev);
> + int idx = ti_sci_dev_id(dev);
> +
> + return ti_sci->ops.dev_ops.put_device(ti_sci, idx);
> +}
> +
> +static int ti_sci_pd_attach_dev(struct generic_pm_domain *domain,
> + struct device *dev)
> +{
> + struct device_node *np = dev->of_node;
> + struct ti_sci_pm_domain *ti_sci_genpd = genpd_to_ti_sci_pd(domain);
> + const struct ti_sci_handle *ti_sci = ti_sci_genpd->ti_sci;
> + struct ti_sci_genpd_dev_data *sci_dev_data;
> + struct generic_pm_domain_data *genpd_data;
> + int idx, ret = 0;
> +
> + ret = of_property_read_u32(np, "ti,sci-id", &idx);
> + if (ret) {
> + dev_err(ti_sci_genpd->dev, "Cannot find ti,sci-id for %s\n",
> + dev_name(dev));
> + return -ENODEV;
> + }
> +
> + /*
> + * Check the validity of the requested idx, if the index is not valid
> + * the PMMC will return a NAK here and we will not allocate it.
> + */
> + ret = ti_sci->ops.dev_ops.is_valid(ti_sci, idx);
> + if (ret)
> + return -EINVAL;
> +
> + sci_dev_data = kzalloc(sizeof(*sci_dev_data), GFP_KERNEL);
> + if (!sci_dev_data)
> + return -ENOMEM;
> +
> + sci_dev_data->idx = idx;
> +
> + genpd_data = dev_gpd_data(dev);
> + genpd_data->data = sci_dev_data;
> +
> + return 0;
> +}
> +
> +static void ti_sci_pd_detach_dev(struct generic_pm_domain *domain,
> + struct device *dev)
> +{
> + struct generic_pm_domain_data *genpd_data = dev_gpd_data(dev);
> + struct ti_sci_genpd_dev_data *sci_dev_data = genpd_data->data;
> +
> + kfree(sci_dev_data);
> + genpd_data->data = NULL;
> +}
> +
> +static const struct of_device_id ti_sci_pm_domain_matches[] = {
> + { .compatible = "ti,sci-pm-domain", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, ti_sci_pm_domain_matches);
> +
> +static int ti_sci_pm_domain_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct ti_sci_pm_domain *ti_sci_pd;
> + int ret;
> +
> + ti_sci_pd = devm_kzalloc(dev, sizeof(*ti_sci_pd), GFP_KERNEL);
> + if (!ti_sci_pd)
> + return -ENOMEM;
> +
> + ti_sci_pd->ti_sci = devm_ti_sci_get_handle(dev);
> + if (IS_ERR(ti_sci_pd->ti_sci))
> + return PTR_ERR(ti_sci_pd->ti_sci);
> +
> + ti_sci_pd->dev = dev;
> +
> + ti_sci_pd->pd.attach_dev = ti_sci_pd_attach_dev;
> + ti_sci_pd->pd.detach_dev = ti_sci_pd_detach_dev;
> +
> + ti_sci_pd->pd.dev_ops.start = ti_sci_dev_start;
> + ti_sci_pd->pd.dev_ops.stop = ti_sci_dev_stop;
> +
> + pm_genpd_init(&ti_sci_pd->pd, NULL, true);
> +
> + ret = of_genpd_add_provider_simple(np, &ti_sci_pd->pd);
> +
> + return ret;
> +}
> +
> +static struct platform_driver ti_sci_pm_domains_driver = {
> + .probe = ti_sci_pm_domain_probe,
> + .driver = {
> + .name = "ti_sci_pm_domains",
> + .of_match_table = ti_sci_pm_domain_matches,
> + },
> +};
> +module_platform_driver(ti_sci_pm_domains_driver);
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("TI System Control Interface (SCI) Power Domain driver");
> +MODULE_AUTHOR("Dave Gerlach");
> --
> 2.9.3
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 1/4] PM / Domains: Add generic data pointer to genpd data struct
From: Ulf Hansson @ 2016-10-25 9:48 UTC (permalink / raw)
To: Dave Gerlach
Cc: Rafael J . Wysocki, Kevin Hilman,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Nishanth Menon, Keerthy, Russell King, Tero Kristo, Sudeep Holla,
Santosh Shilimkar
In-Reply-To: <20161019203347.17893-2-d-gerlach-l0cyMroinI0@public.gmane.org>
On 19 October 2016 at 22:33, Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org> wrote:
> Add a void *data pointer to struct generic_pm_domain_data. Because this
> exists for each device associated with a genpd it will allow us to
> assign per-device data if needed on a platform for control of that
> specific device.
>
> Signed-off-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
Acked-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Kind regards
Uffe
> ---
> include/linux/pm_domain.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index a09fe5c009c8..9c0a897fe605 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -105,6 +105,7 @@ struct generic_pm_domain_data {
> struct pm_domain_data base;
> struct gpd_timing_data td;
> struct notifier_block nb;
> + void *data;
> };
>
> #ifdef CONFIG_PM_GENERIC_DOMAINS
> --
> 2.9.3
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC V4 PATCH 1/6] arm64: dts: msm8992 SoC and LG Bullhead (Nexus 5X) support
From: Jeremy McNicoll @ 2016-10-25 9:40 UTC (permalink / raw)
To: Stephen Boyd
Cc: Jeremy McNicoll, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, robh-DgEjT+Ai2ygdnm+yROfE0A,
andy.gross-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
michael.scott-QSEj5FYQhm4dnm+yROfE0A
In-Reply-To: <20161021192147.GM26139-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
On Fri, Oct 21, 2016 at 12:21:47PM -0700, Stephen Boyd wrote:
> On 10/21, Jeremy McNicoll wrote:
> > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> > index 5dd05de..abb366e 100644
> > --- a/arch/arm64/boot/dts/qcom/Makefile
> > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > @@ -1,6 +1,6 @@
> > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb msm8916-mtp.dtb
> > -dtb-$(CONFIG_ARCH_QCOM) += msm8996-mtp.dtb
> > -dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb
> > +dtb-$(CONFIG_ARCH_QCOM) += msm8996-mtp.dtb apq8096-db820c.dtb
> > +dtb-$(CONFIG_ARCH_QCOM) += msm8992-bullhead-rev-101.dtb
>
> One line per dtb please.
Bjorn mentioned this as well, its now one entry per line.
>
> >
> > always := $(dtb-y)
> > subdir-y := $(dts-dirs)
> > diff --git a/arch/arm64/boot/dts/qcom/msm8992-bullhead-rev-101.dts b/arch/arm64/boot/dts/qcom/msm8992-bullhead-rev-101.dts
> > new file mode 100644
> > index 0000000..63fa3b0
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/msm8992-bullhead-rev-101.dts
> > @@ -0,0 +1,42 @@
> > +/* Copyright (c) 2015, LGE Inc. All rights reserved.
> > + * Copyright (c) 2016, The Linux Foundation. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 and
> > + * only version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "msm8992.dtsi"
> > +
> > +/ {
> > + model = "LGE MSM8992 BULLHEAD rev-1.01";
> > + compatible = "qcom,msm8992";
> > + /* required for bootloader to select correct board */
> > + qcom,board-id = <0xb64 0>;
> > +};
>
> Why do we end
> > +
> > +/ {
>
> and then restart the root node? Just keep the same node all the
> time please.
>
removed the extra { } 's
> > + aliases {
> > + serial0 = &blsp1_uart2;
> > + };
> > +
> > + chosen {
> > + stdout-path = "serial0:115200n8";
> > + };
> > +
> > + soc {
> > + serial@f991e000 {
> > + status = "okay";
> > + pinctrl-names = "default", "sleep";
> > + pinctrl-0 = <&blsp1_uart2_default>;
> > + pinctrl-1 = <&blsp1_uart2_sleep>;
> > + };
> > + };
> > +};
> > diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
> > new file mode 100644
> > index 0000000..5d69a6b
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
> > @@ -0,0 +1,214 @@
> > +/* Copyright (c) 2013-2016, The Linux Foundation. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 and
> > + * only version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/clock/qcom,gcc-msm8994.h>
> > +
> > +/ {
> > + model = "Qualcomm Technologies, Inc. MSM 8992";
> > + compatible = "qcom,msm8992";
> > + // msm-id and pmic-id are needed by bootloader for
> > + // selecting correct blob
> > + qcom,msm-id = <251 0>, <252 0>;
> > + qcom,pmic-id = <0x10009 0x1000A 0x0 0x0>;
>
> Can't these just be in the final dts file and not in the dtsi
> file? Technically the pmic pairings are a board choice and not an
> SoC choice so they shouldn't be here. The msm-id properties are
> a little easier to justify, but then I thought we decided to only
> put them in the boards that need them?
>
> > + interrupt-parent = <&intc>;
> > +
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > +
> > + chosen { };
> > +
> > + cpus {
> > + #address-cells = <2>;
> > + #size-cells = <0>;
> > + cpu-map {
> > + cluster0 {
> > + core0 {
> > + cpu = <&CPU0>;
> > + };
> > + };
> > + };
> > +
> > + CPU0: cpu@0 {
> > + device_type = "cpu";
> > + compatible = "arm,cortex-a53", "arm,armv8";
> > + reg = <0x0 0x0>;
> > + next-level-cache = <&L2_0>;
> > + // The currents(uA) correspond to the frequencies in the
> > + // frequency table.
> > + current = < 18250 //384000 kHZ
> > + 24330 //460800 kHZ
> > + 26920 //600000 kHZ
> > + 34600 //672000 kHz
> > + 38150 //787200 kHZ
> > + 46880 //864000 kHZ
> > + 55940 //960000 kHZ
> > + 81740 //1248000 kHZ
> > + 105870>; //1440000 kHZ
>
> I imagine these aren't used. Please remove them.
>
They are not currently being used. What was their purpose downstream?
> > + L2_0: l2-cache {
> > + compatible = "cache";
> > + cache-level = <2>;
> > + };
> > + };
> > + };
> > +
> > + soc: soc { };
>
> Just put the soc stuff here instead of having an empty node we
> fill later please. That matches other qcom dtsi files.
>
agreed, we were just following what was down in 3.10 downstream.
> > +
> > + memory {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > +
> > + device_type = "memory";
> > + reg = <0 0 0 0>; // bootloader will update
> > + };
> > +
> > + clocks {
> > + xo_board: xo_board {
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + clock-frequency = <19200000>;
> > + };
> > +
> > + sleep_clk: sleep_clk {
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + clock-frequency = <32768>;
> > + };
> > + };
> > +};
> > +
> > +&soc {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges = <0 0 0 0xffffffff>;
> > + compatible = "simple-bus";
> > +
> > + intc: interrupt-controller@f9000000 {
> > + compatible = "qcom,msm-qgic2";
> > + interrupt-controller;
> > + #interrupt-cells = <3>;
> > + reg = <0xf9000000 0x1000>,
> > + <0xf9002000 0x1000>;
> > + };
> > +
> > + timer {
> > + compatible = "arm,armv8-timer";
> > + interrupts = <1 2 0xf08>,
> > + <1 3 0xf08>,
> > + <1 4 0xf08>,
> > + <1 1 0xf08>;
> > + clock-frequency = <19200000>;
>
> Please remove this property.
>
gone
> > + };
>
> This whole node should be outside of soc node.
>
> > +
> > + timer@f9020000 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges;
> > + compatible = "arm,armv7-timer-mem";
> > + reg = <0xf9020000 0x1000>;
> > + clock-frequency = <19200000>;
>
> Please remove this property.
>
Followed the format of msm8996.dtsi
> > +
> > + frame@f9021000 {
> > + frame-number = <0>;
> > + interrupts = <0 9 0x4>,
> > + <0 8 0x4>;
> > + reg = <0xf9021000 0x1000>,
> > + <0xf9022000 0x1000>;
> > + };
> > +
> > + frame@f9023000 {
> > + frame-number = <1>;
> > + interrupts = <0 10 0x4>;
> > + reg = <0xf9023000 0x1000>;
> > + status = "disabled";
> > + };
> > +
> > + frame@f9024000 {
> > + frame-number = <2>;
> > + interrupts = <0 11 0x4>;
> > + reg = <0xf9024000 0x1000>;
> > + status = "disabled";
> > + };
> > +
> > + frame@f9025000 {
> > + frame-number = <3>;
> > + interrupts = <0 12 0x4>;
> > + reg = <0xf9025000 0x1000>;
> > + status = "disabled";
> > + };
> > +
> > + frame@f9026000 {
> > + frame-number = <4>;
> > + interrupts = <0 13 0x4>;
> > + reg = <0xf9026000 0x1000>;
> > + status = "disabled";
> > + };
> > +
> > + frame@f9027000 {
> > + frame-number = <5>;
> > + interrupts = <0 14 0x4>;
> > + reg = <0xf9027000 0x1000>;
> > + status = "disabled";
> > + };
> > +
> > + frame@f9028000 {
> > + frame-number = <6>;
> > + interrupts = <0 15 0x4>;
> > + reg = <0xf9028000 0x1000>;
> > + status = "disabled";
> > + };
> > + };
> > +
> > + restart@fc4ab000 {
> > + compatible = "qcom,pshold";
> > + reg = <0xfc4ab000 0x4>;
> > + };
> > +
> > + msmgpio: pinctrl@fd510000 {
> > + compatible = "qcom,msm8994-pinctrl", "qcom,msm8974-pinctrl";
> > + reg = <0xfd510000 0x4000>;
> > + interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + interrupt-controller;
> > + #interrupt-cells = <2>;
> > + };
> > +
> > + blsp1_uart2: serial@f991e000 {
> > + compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
> > + reg = <0xf991e000 0x1000>;
> > + interrupts = <0 108 0>;
>
> Please add a trigger type and use the GIC_SPI/IRQ_TYPE_* macros.
>
updated both 8992, 8994
> > + status = "disabled";
> > + clock-names = "core", "iface";
> > + clocks = <&clock_gcc GCC_BLSP1_UART2_APPS_CLK>,
> > + <&clock_gcc GCC_BLSP1_AHB_CLK>;
> > + };
> > +
> > + clock_gcc: qcom,gcc@fc400000 {
> > + compatible = "qcom,gcc-8994";
> > + #clock-cells = <1>;
> > + #reset-cells = <1>;
> > + #power-domain-cells = <1>;
> > + reg = <0xfc400000 0x2000>;
> > + clock-names = "xo", "xo_a_clk";
>
> What is this for? Please remove.
It came from here:
https://android.googlesource.com/kernel/msm.git/+/android-msm-bullhead-3.10-marshmallow-dr1.6/arch/arm/boot/dts/qcom/msm8992.dtsi#745
I take it we are not using them anymore? Removed.
>
> > + };
> > +
> > + clock_rpm: qcom,rpmcc@fc401880 {
> > + compatible = "qcom,rpmcc";
>
> What is this node? Please remove.
Again, something from 3.10. Doesnt look like we need it now.
-jeremy
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
--
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 v4 3/5] powerpc/mm: allow memory hotplug into a memoryless node
From: Michael Ellerman @ 2016-10-25 9:39 UTC (permalink / raw)
To: Balbir Singh, Reza Arbab, Benjamin Herrenschmidt, Paul Mackerras,
Rob Herring, Frank Rowand, Andrew Morton
Cc: Bharata B Rao, Nathan Fontenot, Stewart Smith, Alistair Popple,
Aneesh Kumar K.V, Tang Chen, linux-kernel, linuxppc-dev,
devicetree, linux-mm
In-Reply-To: <872f253d-8a55-246c-2be0-636a588e2dd0@gmail.com>
Balbir Singh <bsingharora@gmail.com> writes:
> FYI, these checks were temporary to begin with
>
> I found this in git history
>
> b226e462124522f2f23153daff31c311729dfa2f (powerpc: don't add memory to empty node/zone)
Nice thanks for digging it up.
commit b226e462124522f2f23153daff31c311729dfa2f
Author: Mike Kravetz <kravetz@us.ibm.com>
AuthorDate: Fri Dec 16 14:30:35 2005 -0800
^^^^
That is why maintainers don't like to merge "temporary" patches :)
cheers
--
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] ARM: sti: stih410-clocks: Add PROC_STFE as a critical clock
From: Peter Griffin @ 2016-10-25 9:39 UTC (permalink / raw)
To: Lee Jones
Cc: linux-arm-kernel, linux-kernel, kernel, patrice.chotard,
devicetree
In-Reply-To: <20161025074257.GF8574@dell>
Hi Lee,
On Tue, 25 Oct 2016, Lee Jones wrote:
> On Mon, 24 Oct 2016, Peter Griffin wrote:
>
> > Hi Lee,
> >
> > On Mon, 24 Oct 2016, Lee Jones wrote:
> > > On Tue, 18 Oct 2016, Peter Griffin wrote:
> > >
> > > > Once the ST frontend demux HW IP has been enabled, the clock can't
> > > > be disabled otherwise the system will hang and the board will
> > > > be unserviceable.
> > > >
> > > > To allow balanced clock enable/disable calls in the driver we use
> > > > the critical clock infrastructure to take an extra reference on the
> > > > clock so the clock will never actually be disabled.
> > >
> > > This is an abuse of the critical-clocks framework, and is exactly the
> > > type of hack I promised the clk guys I'd try to prevent.
> >
> > I expect the best way to do this would be to write some documentation on the
> > clock-critical DT binding and/or CRITICAL_CLK flag. The only documentation I can
> > find currently is with the initial patch series [1] and the comment in
> > clk-provider.h of
> >
> > #define CLK_IS_CRITICAL BIT(11) /* do not gate, ever */
> >
> > Or the patch decription
> >
> > "Critical clocks are those which must not be gated, else undefined
> > or catastrophic failure would occur. Here we have chosen to
> > ensure the prepare/enable counts are correctly incremented, so as
> > not to confuse users with enabled clocks with no visible users."
> >
> > Which is the functionality I want for this clock.
>
> No, that's not the functionality you want.
Yes it is :)
> You want for the clock not
> to be RE-gated (big difference). Currently, the STFE clock will never
> be gated, even when a) it's not used and b) can actually be disabled.
> You're needlessly wasting power here.
IMO it is *never* safe for Linux to gate this clock, as you have no idea
on the state of the hardware from the primary and secondary bootloaders.
If the clock is enabled when Linux boots, the Linux clock framework *needs*
to assume the hardware may have been used in previous boot stages, and it should
not attempt to disable the clock.
>
> Also, in your use-case there is a visible user, and the prepare/enable
> counts will be correct.
Your correct there is a visible user, but this is the same as CLK_EXT2F_A9 where
there are multiple users of the clock (SPI, I2C, UART etc).
>
> > > If this, or
> > > any other IP has some quirks (i.e. once enabled, if this clock is
> > > subsequently disabled it will have a catastrophic effect on the
> > > platform), then they should be worked around in the driver.
> > >
> > > The correct thing to do here is craft a clk-keep-on flag and ensure it
> > > is set to true for the effected platform(s)' platform data.
> >
> > I'm always wary of creating a driver specific flag, especially when its
> > purpose is to do the same thing as an existing mechanism provided by the
> > subsystem of not gating the clock.
>
> Using existing sub-system supplied mechanisms in the way they were not
> intended is sub-optimal (read "hacky").
I think the scope of this flag has been defined in a very narrow way, which is
making you want to suggest lots of hacks in the client driver.
>
> > I can see a couple of problems with what you propose:
> >
> > 1) You have to put the clk-keep-on flag in every driver which consumes the
> > clock. IMO it is much better to have this knowledge in the SoC's
> > clock driver so every consumer of the clock automatically benefits.
>
> That would also be fine(ish). The issue is that this problem is
> board specific, so the platform clock driver would have to contain
> board level knowledge.
It is not board specific. It is a SoC HW bug, so by definition present on all
boards which have this SoC.
> Also, if you were to implement this, it would
> too mess up reference counting in the core.
>
> > 2) You don't benefit from the CLK_IS_CRITICAL reference counting logic in
> > clk.c. So then each driver has to also work around that to get sensible reference
> > counts. e.g.
> >
> > if (!__clk_is_enabled(clk) && pdata->clk-keep-on)
> > clk_enable(clk)
> >
> > Which seems to me to be fighting against the subsystem. Given that the only use of
> > _clk_is_enabled() outside drivers/clk is in an old arch/arm/mach-omap2/pm24xx.c
> > driver I suspect its use is frowned upon, and it shouldn't really be an EXPORTED_SYMBOL.
>
> In this instance, since the STFE clock is only used by this IP, I
> would choose to handle it in the driver.
There are other IPs within this IP block for which upstream drivers don't yet exist.
> This can be done using a
> single flag stored in pdata which should be fetched using
> of_match_device(). This way there is no need for any more API abuse;
> either by incorrectly identifying the STFE clock as critical OR
> invoking any internal __clk_*() calls.
>
> Enable the clock once in .probe(), which you already do.
But these drivers are by default built as modules, so when you rmmod, and insmod the
driver you now have an ever increasing clock reference count. This is the
problem I was describing in the previous email, and why what you propose is a
bad idea.
>
> Then, whenever you do any power saving do:
>
> suspend()
> {
> if (!ddata->enable_clk_once)
> clk_disable(clk);
> }
>
> resume()
> {
> if (!ddata->enable_clk_once)
> clk_enable(clk);
> }
>
> However, looking at your driver, I think this point might even be
> moot, since you don't have any power saving. The only time you
> disable the clock is in the error path. Just replace that with a
> comment about the platform's unfortunate errata.
This is exactly what I want to avoid doing. The driver already has these
hacks as it was waiting for the critical clock patches to land so I could remove
them and fix the problem properly.
Much like you did with the I2C and SPI drivers, where you removed similar hacks
and added the clock to the critical clock list.
>
> > [1] https://lkml.org/lkml/2016/1/18/272
>
> I'm glad you mentioned this. Let's take a look:
>
> > Some platforms contain clocks which if gated, will cause undefined or
> > catastrophic behaviours. As such they are not to be turned off, ever.
>
> Not the case here.
>
> This clock *can* be gated and can be turned off *sometimes*.
See above, if the clock is on when Linux boots it can never be assumed that it
is safe to disable it.
>
> > Many of these such clocks do not have devices, thus device drivers
> > where clocks may be enabled and references taken to ensure they stay
> > enabled do not exist. Therefore, we must handle these such cases in
> > the core.
>
> This clock *does* have a driver and correct references *can* be
> taken.
As above this is the same for CLK_EXT2F_A9, which has multiple users in the
kernel.
The point is we don't wish to have the knowledge in the individual drivers that
this clock is critical and can destroy the system if it is disabled.
>
> [...]
>
> All I'm saying is, and it's the same thing I've said many times; these
> types of issues do not exhibit the same set of symptoms as a critical
> clock by definition. Critical clocks are those which references can
> not be taken by any other means.
That is not how you are using it currently. See CLK_EXT2F_A9 and
CLK_TX_ICN_DMU.
> Think of the critical clock
> framework as a mechanism to circumvent the requirement of writing a
> special driver which would *only* handle clocks i.e. an interconnect
> driver in the ST case.
>
You didn't need a new flag for that. CLK_IGNORE_UNUSED already allowed you to not
write a specical driver which only handled for example an interconnect clock and
would ensure it wouldn't be gated by the disable_unused logic.
IMO the point of the critical clock stuff, and what CLK_IGNORE_UNUSED *did not* allow
you to do is for situations like CLK_EXT2F_A9, CLK_TX_ICN_DMU and PROC_STFE where users
*do* exist in the kernel, but the SoC clock driver has *special* knowledge of the
clock tree on the SoC and knows that despite what the drivers are doing with
their enable/disable calls the clock should never be disabled.
In fact out of what we currently mark as clock-critical on STi platform most
of the clocks could have been handled perfectly fine with the CLK_IGNORE_UNUSED
flag (apart from the fact that there is no way to set the flag from DT). It is
only CLK_EXT2F_A9 and CLK_TX_ICN_DMU which *do* have kernel users where the
extra functionality of critical clocks is required (we need the additional
reference taken by the clk framework to avoid the kernel drivers from disabling
the clock).
regards,
Peter.
^ permalink raw reply
* Re: [PATCH] soc: qcom: Add SoC info driver
From: Imran Khan @ 2016-10-25 9:38 UTC (permalink / raw)
To: Andy Gross
Cc: David Brown, Rob Herring, Mark Rutland,
open list:ARM/QUALCOMM SUPPORT, open list:ARM/QUALCOMM SUPPORT,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
In-Reply-To: <20161020152017.GE3145@hector.attlocal.net>
On 10/20/2016 8:50 PM, Andy Gross wrote:
> On Thu, Oct 20, 2016 at 07:36:22PM +0530, Imran Khan wrote:
>> The SoC info driver provides information such as Chip ID,
>> Chip family, serial number and other such details about
>> Qualcomm SoCs.
>>
>> Signed-off-by: Imran Khan <kimran@codeaurora.org>
>> ---
>> .../devicetree/bindings/soc/qcom/qcom,socinfo.txt | 18 +
>> drivers/soc/qcom/socinfo.c | 1173 ++++++++++++++++++++
>
> 1173 lines!!!!!! To latch smem entry and read/process those contents?
>
The number of code lines will be reduced once we remove the function to setup dummy socinfo.
Also I will try to remove redundant sysfs attributes.
>> include/linux/soc/qcom/socinfo.h | 198 ++++
>> 3 files changed, 1389 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
>> create mode 100644 drivers/soc/qcom/socinfo.c
>> create mode 100644 include/linux/soc/qcom/socinfo.h
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
>> new file mode 100644
>> index 0000000..1f26299
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
>> @@ -0,0 +1,18 @@
>> +Qualcomm SoC Information (socinfo) Driver binding
>> +
>> +This binding describes the Qualcomm SoC Information Driver, which provides
>> +information such as chip id, chip family, serial number and other such
>> +details about Qualcomm SoCs.
>> +
>> +- compatible:
>> + Usage: required
>> + Value type: <stringlist>
>> + Definition: must be "qcom,socinfo"
>> +
>> += EXAMPLE
>> +
>> +The following example represents a socinfo node.
>> +
>> + socinfo {
>> + compatible = "qcom,socinfo";
>> + };
>
> Let's drop adding a new binding. Let's roll this into the smem driver itself.
>
Okay. I will move the socinfo setup part to smem and will keep only socinfo sysfs related
stuff in this file.
>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>> new file mode 100644
>> index 0000000..dc26028
>> --- /dev/null
>> +++ b/drivers/soc/qcom/socinfo.c
>> @@ -0,0 +1,1173 @@
>> +/*
>> + * Copyright (c) 2009-2016, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +/*
>> + * SOC Info Routines
>> + *
>> + */
>> +
>> +#define pr_fmt(fmt) "%s: " fmt, __func__
>> +
>> +#include <linux/export.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/sys_soc.h>
>> +#include <linux/slab.h>
>> +#include <linux/stat.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +#include <linux/soc/qcom/socinfo.h>
>> +#include <linux/soc/qcom/smem.h>
>> +
>> +#include <asm/system_misc.h>
>> +
>> +
>> +#define BUILD_ID_LENGTH 32
>
> This needs to be more specific. SMEM_SOCINFO_XXX
>
>> +#define SMEM_IMAGE_VERSION_BLOCKS_COUNT 32
>> +#define SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE 128
>> +#define SMEM_IMAGE_VERSION_SIZE 4096
>> +#define SMEM_IMAGE_VERSION_NAME_SIZE 75
>> +#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20
>> +#define SMEM_IMAGE_VERSION_VARIANT_OFFSET 75
>> +#define SMEM_IMAGE_VERSION_OEM_SIZE 32
>> +#define SMEM_IMAGE_VERSION_OEM_OFFSET 96
>> +#define SMEM_IMAGE_VERSION_PARTITION_APPS 10
>> +#define SMEM_ITEM_SIZE_ALIGN 8
>> +/*
>> + * Shared memory identifiers, used to acquire handles to respective memory
>> + * region.
>> + */
>> +#define SMEM_IMAGE_VERSION_TABLE 469
>> +#define SMEM_HW_SW_BUILD_ID 137
>> +
>
> <snip>
>
>> +static enum qcom_cpu cur_cpu;
>> +static int current_image;
>> +static uint32_t socinfo_format;
>> +
>> +static struct socinfo_v0_1 dummy_socinfo = {
>> + .format = SOCINFO_VERSION(0, 1),
>> + .version = 1,
>> +};
>
> Instead of having a dummy, can we just fail if we don't find a match? And by
> fail, I mean having an 'unknown' cpu?
>
>> +
>> +static char *socinfo_get_id_string(void)
>> +{
>> + return (socinfo) ? cpu_of_id[socinfo->v0_1.id].soc_id_string : NULL;
>> +}
>> +
>
> <snip>
>
>> +uint32_t socinfo_get_id(void)
>> +{
>> + return (socinfo) ? socinfo->v0_1.id : 0;
>> +}
>> +EXPORT_SYMBOL_GPL(socinfo_get_id);
>
> Why do we have EXPORTS at all? Drivers should be using compatibles to figure
> out what they are.
>
> <snip>
>
Okay.
>> +/* socinfo: sysfs functions */
>> +
>> +static ssize_t
>> +qcom_get_vendor(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + return snprintf(buf, PAGE_SIZE, "Qualcomm\n");
>> +}
>> +
>> +static ssize_t
>> +qcom_get_raw_id(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + return snprintf(buf, PAGE_SIZE, "%u\n",
>> + socinfo_get_raw_id());
>> +}
>> +
>
> <snip>
>
>> +static void * __init setup_dummy_socinfo(void)
>> +{
>> + if (early_machine_is_apq8064()) {
>> + dummy_socinfo.id = APQ_8064_ID;
>> + strlcpy(dummy_socinfo.build_id, "apq8064",
>> + sizeof(dummy_socinfo.build_id));
>> + } else if (early_machine_is_apq8084()) {
>> + dummy_socinfo.id = APQ_8084_ID;
>> + strlcpy(dummy_socinfo.build_id, "apq8084",
>> + sizeof(dummy_socinfo.build_id));
>> + } else if (early_machine_is_msm8916()) {
>> + dummy_socinfo.id = MSM_8916_ID;
>> + strlcpy(dummy_socinfo.build_id, "msm8916",
>> + sizeof(dummy_socinfo.build_id));
>> + } else if (early_machine_is_msm8660()) {
>> + dummy_socinfo.id = MSM_8660A_ID;
>> + strlcpy(dummy_socinfo.build_id, "msm8660",
>> + sizeof(dummy_socinfo.build_id));
>> + } else if (early_machine_is_msm8960()) {
>> + dummy_socinfo.id = MSM_8960_ID;
>> + strlcpy(dummy_socinfo.build_id, "msm8960",
>> + sizeof(dummy_socinfo.build_id));
>> + } else if (early_machine_is_msm8974()) {
>> + dummy_socinfo.id = MSM_8974_ID;
>> + strlcpy(dummy_socinfo.build_id, "msm8974",
>> + sizeof(dummy_socinfo.build_id));
>> + } else if (early_machine_is_msm8996()) {
>> + dummy_socinfo.id = MSM_8996_ID;
>> + strlcpy(dummy_socinfo.build_id, "msm8996",
>> + sizeof(dummy_socinfo.build_id));
>> + }
>> +
>> + strlcat(dummy_socinfo.build_id, "Dummy socinfo",
>> + sizeof(dummy_socinfo.build_id));
>> + return (void *) &dummy_socinfo;
>> +}
>
> Can we just remove this and just return whatever unknown values for the sysfs
> entries?
>
Will shorten this function. In case of error I will use 0 as socid and "unknown CPU"
as soc-id.
>> +
>> +static void socinfo_populate_sysfs_files(struct device *qcom_soc_device)
>> +{
>> + device_create_file(qcom_soc_device, &qcom_soc_attr_vendor);
>> + device_create_file(qcom_soc_device, &image_version);
>> + device_create_file(qcom_soc_device, &image_variant);
>> + device_create_file(qcom_soc_device, &image_crm_version);
>> + device_create_file(qcom_soc_device, &select_image);
>> + device_create_file(qcom_soc_device, &images);
>> +
>> + switch (socinfo_format) {
>> + case SOCINFO_VERSION(0, 12):
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_chip_family);
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_raw_device_family);
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_raw_device_number);
>> + case SOCINFO_VERSION(0, 11):
>> + case SOCINFO_VERSION(0, 10):
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_serial_number);
>> + case SOCINFO_VERSION(0, 9):
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_foundry_id);
>> + case SOCINFO_VERSION(0, 8):
>> + case SOCINFO_VERSION(0, 7):
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_pmic_model);
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_pmic_die_revision);
>> + case SOCINFO_VERSION(0, 6):
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_platform_subtype);
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_platform_subtype_id);
>> + case SOCINFO_VERSION(0, 5):
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_accessory_chip);
>> + case SOCINFO_VERSION(0, 4):
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_platform_version);
>> + case SOCINFO_VERSION(0, 3):
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_hw_platform);
>> + case SOCINFO_VERSION(0, 2):
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_raw_id);
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_raw_version);
>> + case SOCINFO_VERSION(0, 1):
>> + device_create_file(qcom_soc_device,
>> + &qcom_soc_attr_build_id);
>> + break;
>> + default:
>> + pr_err("Unknown socinfo format: v%u.%u\n",
>> + SOCINFO_VERSION_MAJOR(socinfo_format),
>> + SOCINFO_VERSION_MINOR(socinfo_format));
>> + break;
>> + }
>> +}
>> +
>
> <snip>
>
>> +static int qcom_socinfo_probe(struct platform_device *pdev)
>> +{
>> + size_t size;
>> +
>> + socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
>> + &size);
>> + if (IS_ERR_OR_NULL(socinfo)) {
>> + if (PTR_ERR(socinfo) == -EPROBE_DEFER)
>> + return PTR_ERR(socinfo);
>> + else {
>> + dev_warn(&pdev->dev,
>> + "Can't find SMEM_HW_SW_BUILD_ID; falling back on dummy values.\n");
>> + socinfo = setup_dummy_socinfo();
>> + }
>> + }
>> +
>> + socinfo_select_format();
>> +
>> + WARN(!socinfo_get_id(), "Unknown SOC ID!\n");
>> +
>> + if (socinfo_get_id() >= ARRAY_SIZE(cpu_of_id))
>> + BUG_ON("New IDs added! ID => CPU mapping needs an update.\n");
>> + else
>> + cur_cpu = cpu_of_id[socinfo->v0_1.id].generic_soc_type;
>> +
>> + socinfo_print();
>> +
>> + socinfo_init_sysfs();
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_socinfo_of_match[] = {
>> + { .compatible = "qcom,socinfo" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_socinfo_of_match);
>> +
>> +static struct platform_driver qcom_socinfo_driver = {
>> + .probe = qcom_socinfo_probe,
>> + .driver = {
>> + .name = "qcom-socinfo",
>> + .of_match_table = qcom_socinfo_of_match,
>> + },
>> +};
>
> As mentioned above:
> Perhaps we can forgo the separate platform driver and just roll this into the
> smem driver that already exists. You can create a device there and get the
> right attributes for the system and setup your sysfs entries.
>
> Maybe splitting off the sysfs into a separate file would also be good. That
> keeps it nice and clean and away from the main smem code.
>
>
Okay. I will move the socinfo setup part to smem and will keep only socinfo sysfs related
stuff in this file.
>> +module_platform_driver(qcom_socinfo_driver);
>> +
>> diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
>> new file mode 100644
>> index 0000000..17ca50a
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/socinfo.h
>> @@ -0,0 +1,198 @@
>> +/*
>> + * Copyright (c) 2009-2016, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#ifndef _ARCH_ARM_MACH_MSM_SOCINFO_H_
>> +#define _ARCH_ARM_MACH_MSM_SOCINFO_H_
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/errno.h>
>> +#include <linux/of_fdt.h>
>> +#include <linux/of.h>
>> +
>> +#include <asm/cputype.h>
>> +/*
>> + * SOC version type with major number in the upper 16 bits and minor
>> + * number in the lower 16 bits. For example:
>> + * 1.0 -> 0x00010000
>> + * 2.3 -> 0x00020003
>> + */
>> +#define SOCINFO_VERSION_MAJOR(ver) (((ver) & 0xffff0000) >> 16)
>> +#define SOCINFO_VERSION_MINOR(ver) ((ver) & 0x0000ffff)
>> +#define SOCINFO_VERSION(maj, min) ((((maj) & 0xffff) << 16)|((min) & 0xffff))
>> +
>> +#ifdef CONFIG_OF
>> +#define early_machine_is_apq8064() \
>> + of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,apq8064")
>> +#define early_machine_is_apq8084() \
>> + of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,apq8084")
>> +#define early_machine_is_msm8916() \
>> + of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8916")
>> +#define early_machine_is_msm8660() \
>> + of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8660")
>> +#define early_machine_is_msm8960() \
>> + of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8960")
>> +#define early_machine_is_msm8974() \
>> + of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8974")
>> +#define early_machine_is_msm8996() \
>> + of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8996")
>> +#else
>> +#define early_machine_is_apq8064() 0
>> +#define early_machine_is_apq8084() 0
>> +#define early_machine_is_msm8916() 0
>> +#define early_machine_is_msm8660() 0
>> +#define early_machine_is_msm8960() 0
>> +#define early_machine_is_msm8974() 0
>> +#define early_machine_is_msm8996() 0
>> +#endif
>
> You don't need this mess. You could just build a compat match and match against
> the root compatible.
>
Okay. As we don't intend to use dummy soc information, this part will become
redundant and hence it will be removed.
>> +
>> +#define PLATFORM_SUBTYPE_MDM 1
>> +#define PLATFORM_SUBTYPE_INTERPOSERV3 2
>> +#define PLATFORM_SUBTYPE_SGLTE 6
>> +
>
> <snip>
>
>> +enum msm_cpu socinfo_get_msm_cpu(void);
>> +uint32_t socinfo_get_id(void);
>> +uint32_t socinfo_get_version(void);
>> +uint32_t socinfo_get_raw_id(void);
>> +char *socinfo_get_build_id(void);
>> +uint32_t socinfo_get_platform_type(void);
>> +uint32_t socinfo_get_platform_subtype(void);
>> +uint32_t socinfo_get_platform_version(void);
>> +uint32_t socinfo_get_serial_number(void);
>> +enum qcom_pmic_model socinfo_get_pmic_model(void);
>> +uint32_t socinfo_get_pmic_die_revision(void);
>> +int __init socinfo_init(void) __must_check;
>
> We shouldn't have APIs for this. This was supposed to be userspace access only,
> correct?
>
Yes.I will remove these. Although if some driver needs some data pertaining to soc
information, I will have to provide an alternate way for that but that can be done
in a generic way rather than using vendor specific interface
>
> Andy
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH/RFT v2 03/17] ARM: davinci: da8xx: Add USB PHY platform declaration
From: Axel Haslam @ 2016-10-25 9:37 UTC (permalink / raw)
To: Sekhar Nori
Cc: Greg KH, Johan Hovold, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, Alan Stern,
Kevin Hilman, Sergei Shtylyov, David Lechner, Mark Brown,
Alexandre Bailon, linux-usb-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <4129f1be-42f6-f182-c8a2-84befe01d298-l0cyMroinI0@public.gmane.org>
On Tue, Oct 25, 2016 at 11:18 AM, Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
> On Monday 24 October 2016 10:16 PM, ahaslam-rdvid1DuHRBWk0Htik3J/w@public.gmane.org wrote:
>> +static struct platform_device da8xx_usb_phy = {
>> + .name = "da8xx-usb-phy",
>> + .id = 0,
>
> There is a single phy control in the system for both 1.1 and 2.0 PHYs.
> so this can be a singular device (id -1).
>
Ok.
> Thanks,
> Sekhar
>
--
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
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