* Re: [alsa-devel] [PATCH] ASoC: pcm512x: Add PCM512x driver
From: Takashi Iwai @ 2014-02-06 13:12 UTC (permalink / raw)
To: Mark Brown
Cc: Florian Meier, Liam Girdwood, devicetree-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw
In-Reply-To: <20140206130735.GP32298-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
At Thu, 6 Feb 2014 13:07:35 +0000,
Mark Brown wrote:
>
> On Thu, Feb 06, 2014 at 01:51:33PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
>
> > > +static const char *pcm512x_dsp_program_texts[] = {
>
> > The numbers of items in pcm512x_dsp_program_texts[] and _values[]
> > don't match.
>
> Yeah, fixed.
I'm thinking whether we can check this in the macro.
I thought of using BUILD_BUG_ON(), but it's unsure whether it aligns
there well.
> > > +static const struct soc_enum pcm512x_clk_missing =
> > > + SOC_ENUM_SINGLE(PCM512x_CLKDET, 0, 7, pcm512x_clk_missing_text);
>
> > Isn't it 8?
>
> Hrm, it is but this points out an error in the control helpers which has
> been there since forever - they call that parameter max but it's not a
> maximum, it's the number of elements in the enumeration. I bet we have
> a bunch of other enumerations which miss the last element as a result.
Yeah, the argument name is really confusing. I had to double-check
the code when I reviewed your patch, too :)
Also it'd be better to have a practice to use either ARRAY_SIZE() or a
constant there, too.
Takashi
--
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: [alsa-devel] [PATCH] ASoC: pcm512x: Add PCM512x driver
From: Lars-Peter Clausen @ 2014-02-06 13:16 UTC (permalink / raw)
To: Takashi Iwai
Cc: Mark Brown, devicetree-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw, Florian Meier,
Liam Girdwood
In-Reply-To: <s5hob2kv0z3.wl%tiwai-l3A5Bk7waGM@public.gmane.org>
On 02/06/2014 02:12 PM, Takashi Iwai wrote:
> At Thu, 6 Feb 2014 13:07:35 +0000,
> Mark Brown wrote:
[...]
>> Hrm, it is but this points out an error in the control helpers which has
>> been there since forever - they call that parameter max but it's not a
>> maximum, it's the number of elements in the enumeration. I bet we have
>> a bunch of other enumerations which miss the last element as a result.
>
> Yeah, the argument name is really confusing. I had to double-check
> the code when I reviewed your patch, too :)
>
> Also it'd be better to have a practice to use either ARRAY_SIZE() or a
> constant there, too.
There is also SOC_ENUM_SINGLE_DECL(...) which takes care of the common case
where you want as many items as your array contains.
- Lars
--
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: [alsa-devel] [PATCH] ASoC: pcm512x: Add PCM512x driver
From: Takashi Iwai @ 2014-02-06 13:19 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Mark Brown, devicetree-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw, Florian Meier,
Liam Girdwood
In-Reply-To: <52F38B27.1040502-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
At Thu, 06 Feb 2014 14:16:23 +0100,
Lars-Peter Clausen wrote:
>
> On 02/06/2014 02:12 PM, Takashi Iwai wrote:
> > At Thu, 6 Feb 2014 13:07:35 +0000,
> > Mark Brown wrote:
> [...]
> >> Hrm, it is but this points out an error in the control helpers which has
> >> been there since forever - they call that parameter max but it's not a
> >> maximum, it's the number of elements in the enumeration. I bet we have
> >> a bunch of other enumerations which miss the last element as a result.
> >
> > Yeah, the argument name is really confusing. I had to double-check
> > the code when I reviewed your patch, too :)
> >
> > Also it'd be better to have a practice to use either ARRAY_SIZE() or a
> > constant there, too.
>
> There is also SOC_ENUM_SINGLE_DECL(...) which takes care of the common case
> where you want as many items as your array contains.
Ah, right, that makes life easier.
Takashi
--
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] gpio: davinci: reuse for keystone soc
From: Linus Walleij @ 2014-02-06 13:20 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Santosh Shilimkar, Sekhar Nori, Rob Herring, Alexandre Courbot,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
In-Reply-To: <1391615244-12014-1-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org>
On Wed, Feb 5, 2014 at 4:47 PM, Grygorii Strashko
<grygorii.strashko-l0cyMroinI0@public.gmane.org> wrote:
> The similar GPIO HW block is used by keystone SoCs as
> in Davinci SoCs.
> Hence, reuse Davinci GPIO driver for Keystone taking into
> account that Keystone contains ARM GIC IRQ controller which
> is implemented using IRQ Chip.
>
> Documentation:
> http://www.ti.com/lit/ug/sprugv1/sprugv1.pdf
>
> Acked-by: Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>
> Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
> ---
> Changes in v4:
> - rebased on top of v3.14 +
> [patch] gpio: davinci: signedness bug in davinci_gpio_irq_setup()
Are you taking this through ARM SoC or is this something
I should be merging?
Yours,
Linus Walleij
--
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 2/3] ARM: dts: add dts files for exynos5260 SoC
From: Tomasz Figa @ 2014-02-06 13:21 UTC (permalink / raw)
To: Rahul Sharma, linux-samsung-soc
Cc: kgene.kim, tomasz.figa, joshi, r.sh.open,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Rob Herring, Mark Rutland, Grant Likely, Ian Campbell, Pawel Moll,
Kumar Gala
In-Reply-To: <1391577375-17625-3-git-send-email-rahul.sharma@samsung.com>
Hi Rahul, Pankaj, Arun,
[adding linux-arm-kernel, devicetree MLs and DT people on Cc]
I think it's good time to stop accepting DTS files like this and force
new ones to use the proper structure with soc node, labels for every
node and node references.
In case of previous Exynos 5 SoCs I hadn't complained, because they
shared a lot of data with already existing exynos5.dtsi, but since
Exynos5260 is completely different, I'd say it should be converted to
the new layout.
As an example you can look at arch/arm/boot/dts/s3c64xx.dtsi and files
that include it or, for more complete structures, DTS of other
platforms, such as imx6*.
Btw. Please remember that linux-samsung-soc mailing list is just a
convenient utility for reviewers of Samsung-specific patches to have all
of them in one place. Sending patches to it alone is not enough - a
general kernel ML list needs to be CCed as well, in this case
linux-arm-kernel.
Also, please see my comments inline, for review comments unrelated to
the issue described above.
On 05.02.2014 06:16, Rahul Sharma wrote:
> The patch adds the dts files for exynos5260.
>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> ---
> arch/arm/boot/dts/exynos5260-pinctrl.dtsi | 572 +++++++++++++++++++++++++++++
> arch/arm/boot/dts/exynos5260.dtsi | 317 ++++++++++++++++
> 2 files changed, 889 insertions(+)
> create mode 100644 arch/arm/boot/dts/exynos5260-pinctrl.dtsi
> create mode 100644 arch/arm/boot/dts/exynos5260.dtsi
>
> diff --git a/arch/arm/boot/dts/exynos5260-pinctrl.dtsi b/arch/arm/boot/dts/exynos5260-pinctrl.dtsi
> new file mode 100644
> index 0000000..3f2c5c4
> --- /dev/null
> +++ b/arch/arm/boot/dts/exynos5260-pinctrl.dtsi
> @@ -0,0 +1,572 @@
> +/*
> + * Samsung's Exynos5260 SoC pin-mux and pin-config device tree source
> + *
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * Samsung's Exynos5260 SoC pin-mux and pin-config options are listed as device
> + * tree nodes are listed in this file.
> + *
> + * 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.
> +*/
> +
> +/ {
> + pinctrl@11600000 {
[snip]
> + spi0_bus: spi0-bus {
> + samsung,pins = "gpa2-0", "gpa2-1", "gpa2-2", "gpa2-3";
What is the reason for SPI0 to have 4 pins, while SPI1 has just 3?
> + samsung,pin-function = <2>;
> + samsung,pin-pud = <3>;
> + samsung,pin-drv = <0>;
> + };
[snip]
> diff --git a/arch/arm/boot/dts/exynos5260.dtsi b/arch/arm/boot/dts/exynos5260.dtsi
> new file mode 100644
> index 0000000..32a95c7
> --- /dev/null
> +++ b/arch/arm/boot/dts/exynos5260.dtsi
> @@ -0,0 +1,317 @@
> +/*
> + * SAMSUNG EXYNOS5260 SoC device tree source
> + *
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * 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.
> +*/
> +
> +#include "skeleton.dtsi"
> +#include "exynos5260-pinctrl.dtsi"
> +
> +#include <dt-bindings/clk/exynos5260-clk.h>
> +
> +/ {
> + compatible = "samsung,exynos5260";
> + interrupt-parent = <&gic>;
> +
> + aliases {
> + pinctrl0 = &pinctrl_0;
> + pinctrl1 = &pinctrl_1;
> + pinctrl2 = &pinctrl_2;
> + };
> +
> + chipid@10000000 {
> + compatible = "samsung,exynos4210-chipid";
> + reg = <0x10000000 0x100>;
> + };
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a15";
> + reg = <0>;
nit: Please make this consistent with CPUs 10x below, by using hex here
as well.
> + cci-control-port = <&cci_control1>;
> + };
nit: Please keep 1 blank line of spacing between nodes.
> + cpu@1 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a15";
> + reg = <1>;
> + cci-control-port = <&cci_control1>;
> + };
> + cpu@100 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a7";
> + reg = <0x100>;
> + cci-control-port = <&cci_control0>;
> + };
> + cpu@101 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a7";
> + reg = <0x101>;
> + cci-control-port = <&cci_control0>;
> + };
> + cpu@102 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a7";
> + reg = <0x102>;
> + cci-control-port = <&cci_control0>;
> + };
> + cpu@103 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a7";
> + reg = <0x103>;
> + cci-control-port = <&cci_control0>;
> + };
> + };
> +
> + cmus {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
I don't think there is a need to group these nodes under a parent node
that doesn't give any additional information, especially when the CMUs
are scattered trough the whole address space, while we'd like to keep
the nodes ordered by their addresses, as most platforms do.
> + cmu_top: clock-controller@10010000 {
> + compatible = "exynos5260-cmu-top", "samsung,exynos5260-clock";
I don't think that having the "samsung,exynos5260-clock" compatible
string for every CMU is appropriate here, because there is no way to
automatically recognize which CMU it is. Since every CMU instance is
different, they need to have different compatible strings.
> + reg = <0x10010000 0x10000>;
> + #clock-cells = <1>;
> + };
[snip]
> + mct@100B0000 {
> + compatible = "samsung,exynos4210-mct";
> + reg = <0x100B0000 0x1000>;
> + interrupt-controller;
> + #interrups-cells = <1>;
MCT is not an interrupt controller, so the 2 properties above are incorrect.
> + interrupt-parent = <&mct_map>;
> + interrupts = <0>, <1>, <2>, <3>,
> + <4>, <5>, <6>, <7>,
> + <8>, <9>, <10>, <11>;
> + clocks = <&cmu_top FIN_PLL>, <&cmu_peri PERI_CLK_MCT>;
> + clock-names = "fin_pll", "mct";
> +
> + mct_map: mct-map {
> + #interrupt-cells = <1>;
> + #address-cells = <0>;
> + #size-cells = <0>;
> + interrupt-map = <0 &gic 0 104 0>,
> + <1 &gic 0 105 0>,
> + <2 &gic 0 106 0>,
> + <3 &gic 0 107 0>,
> + <4 &gic 0 122 0>,
> + <5 &gic 0 123 0>,
> + <6 &gic 0 124 0>,
> + <7 &gic 0 125 0>,
> + <8 &gic 0 126 0>,
> + <9 &gic 0 127 0>,
> + <10 &gic 0 128 0>,
> + <11 &gic 0 129 0>;
> + };
There is no need for interrupt-map here, because all the interrupts are
from GIC.
> + };
[snip]
> + mmc_0: mmc0@12140000 {
> + compatible = "samsung,exynos5250-dw-mshc";
> + reg = <0x12140000 0x2000>;
> + interrupts = <0 156 0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clocks = <&cmu_fsys FSYS_CLK_MMC0>, <&cmu_top TOP_SCLK_MMC0>;
> + clock-names = "biu", "ciu";
> + fifo-depth = <0x40>;
nit: It might be more readable to use decimal 64 here.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 0/8] da9055: Driver initialisation fixes, add DT support
From: Guenter Roeck @ 2014-02-06 13:22 UTC (permalink / raw)
To: Opensource [Adam Thomson]
Cc: Mark Brown, Lee Jones,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
Linus Walleij, Dmitry Torokhov, Alessandro Zummo
In-Reply-To: <2E89032DDAA8B9408CB92943514A03376B7E368A-68WUHU125fLzLL1Oxlh9IgLouzNaz+3S@public.gmane.org>
On 02/06/2014 03:46 AM, Opensource [Adam Thomson] wrote:
> On Thu, Feb 06, 2014 at 00:54:11AM +0000, Guenter Roeck wrote:
>
>> Adam,
>>
>> You don't really explain what the problem actually is. Can you elaborate ?
>
> Sorry, yes. For the conflicting device Ids, both the PMIC and the CODEC used
> the same I2C Id string, which meant if you tried to intiate both together on the
> same bus, then the second would fail.
>
> For the removal of platform_get_irq_byname(), the reason for this was that it
> was conflicting with regmap_irq_get_virq() when the IRQ value returned from
> platform_get_irq_byname() was being passed to regmap_irq_get_virq(). The result
> for the code was that it would try to request a threaded IRQ using an invalid
> IRQ number (have also described this further in patch 0004 mail thread,
> https://lkml.org/lkml/2014/2/6/126).
>
>>
>> Also, I have been using platform_get_irq() to get the interrupt resource
>> in mfd client drivers and similar situations. Wouldn't this work here as well
>> if you don't want to use platform_get_irq_byname() ?
>
> What I could've done is use platform_get_irq_byname() and avoided using
> regmap_irq_get_virq() as I would already have the correct VIRQ to pass to
> request_threaded_irq(), but I figured that using regmap_irq_get_virq() made
> more sense at the time, and was unable to use both.
>
I may be missing something, but I think the problem may be that you are
doing two mappings instead of just one. I don't think you need to call
regmap_irq_get_virq() at all.
Guenter
--
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: [alsa-devel] [PATCH] ASoC: pcm512x: Add PCM512x driver
From: Lars-Peter Clausen @ 2014-02-06 13:24 UTC (permalink / raw)
To: Mark Brown
Cc: Florian Meier, Liam Girdwood, devicetree-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw, Mark Brown
In-Reply-To: <1391689575-4039-1-git-send-email-broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On 02/06/2014 01:26 PM, Mark Brown wrote:
[...]
> +static const char *pcm512x_dsp_program_texts[] = {
Nitpick: should be "static const char * const ...", a immutable array of
pointers pointing to immutable strings. Same for all the enum texts in this
driver.
[...]
> +static int __init pcm512x_modinit(void)
> +{
> + int ret = 0;
> +
> +#if IS_ENABLED(CONFIG_I2C)
> + ret = i2c_add_driver(&pcm512x_i2c_driver);
> + if (ret) {
> + printk(KERN_ERR "Failed to register pcm512x I2C driver: %d\n",
> + ret);
> + }
> +#endif
> +#if defined(CONFIG_SPI_MASTER)
> + ret = spi_register_driver(&pcm512x_spi_driver);
> + if (ret != 0) {
> + printk(KERN_ERR "Failed to register pcm512x SPI driver: %d\n",
> + ret);
> + }
> +#endif
Another reason why I think it is better to separate the I2C and SPI bits
into different modules. If the registration of the SPI driver fails you'll
return an error and the module will not be loaded. At the same time the i2c
driver is already registered. I know that this is rather theoretical, but if
we don't care if our error handling is correct, because we assume that the
error will never happen, we do not need error handling at all.
> + return ret;
> +}
--
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 4/5] ARM: init: add support for reserved memory defined by device tree
From: Marek Szyprowski @ 2014-02-06 13:26 UTC (permalink / raw)
To: Grant Likely, linux-kernel, linux-arm-kernel, linaro-mm-sig,
devicetree, linux-doc
Cc: Kyungmin Park, Benjamin Herrenschmidt, Arnd Bergmann,
Michal Nazarewicz, Tomasz Figa, Sascha Hauer, Laura Abbott,
Rob Herring, Olof Johansson, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Tomasz Figa, Kumar Gala,
Nishanth Peethambaran, Marc, Josh Cartwright
In-Reply-To: <20140205101510.59A95C40A89@trevor.secretlab.ca>
Hello,
On 2014-02-05 11:15, Grant Likely wrote:
> On Tue, 04 Feb 2014 13:09:32 +0100, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > Enable reserved memory initialization from device tree.
> >
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Laura Abbott <lauraa@codeaurora.org>
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> > arch/arm/mm/init.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> > index 804d61566a53..ebafdb479410 100644
> > --- a/arch/arm/mm/init.c
> > +++ b/arch/arm/mm/init.c
> > @@ -17,6 +17,7 @@
> > #include <linux/nodemask.h>
> > #include <linux/initrd.h>
> > #include <linux/of_fdt.h>
> > +#include <linux/of_reserved_mem.h>
> > #include <linux/highmem.h>
> > #include <linux/gfp.h>
> > #include <linux/memblock.h>
> > @@ -323,6 +324,8 @@ void __init arm_memblock_init(struct meminfo *mi,
> > if (mdesc->reserve)
> > mdesc->reserve();
> >
> > + early_init_dt_scan_reserved_mem();
> > +
>
> The new binding is being made fundamental. If the reserved-memory node
> is present, then it needs to be honored, even if the kernel doesn't know
> how to use the regions. Therefore, This needs to be unconditional for
> all architectures. The hook should be called in early_init_dt_scan()
> (drivers/of/fdt.c) immediately after the early_init_dt_scan_memory()
> hook.
In theory this will be the best solution, but it practice there is a
problem. early_init_dt_scan() is called as the first function from kernel
booting code. That time there is no memory yet added to the system, so it
would be really hard to reserve anything. Memory nodes are being added
later either with memblock_add() or by some other arch specific way.
Finally, once all memory has been added to the system we can parse and
reserve all regions defined in the device tree. This really requires
creating another function which will be called by arch specific code.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* RE: [PATCH 0/8] da9055: Driver initialisation fixes, add DT support
From: Opensource [Adam Thomson] @ 2014-02-06 13:31 UTC (permalink / raw)
To: Guenter Roeck, Opensource [Adam Thomson]
Cc: Mark Brown, Lee Jones, alsa-devel@alsa-project.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
Rob Herring, Linus Walleij, Dmitry Torokhov, Alessandro Zummo
In-Reply-To: <52F38CA1.8020806@roeck-us.net>
On Thu, Feb 06, 2014 at 13:23:47PM +0000, Guenter Roeck wrote:
> > What I could've done is use platform_get_irq_byname() and avoided using
> > regmap_irq_get_virq() as I would already have the correct VIRQ to pass to
> > request_threaded_irq(), but I figured that using regmap_irq_get_virq() made
> > more sense at the time, and was unable to use both.
> >
>
> I may be missing something, but I think the problem may be that you are
> doing two mappings instead of just one. I don't think you need to call
> regmap_irq_get_virq() at all.
Yes, you're correct. The issue was already there in the code and I was
attempting to fix it. When I made the change I figured using only
regmap_irq_get_virq() was the way to go, but seems like I chose the wrong
option. Will make the changes (remove regmap_irq_get_virq()), test on both DT
and non-DT platforms, and then will re-submit the patches.
^ permalink raw reply
* Re: [PATCH v2] dma: Add Xilinx AXI Video Direct Memory Access Engine driver support
From: Srikanth Thokala @ 2014-02-06 13:34 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Srikanth Thokala, Vinod Koul,
dan.j.williams-ral2JQCrhuEAvxtiuMwx3w,
michal.simek-gjFFaj9aHVfQT0dZR+AlfA, Grant Likely,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmaengine-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <52F26716.3010203-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
On Wed, Feb 5, 2014 at 10:00 PM, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
> On 02/05/2014 05:25 PM, Srikanth Thokala wrote:
>>
>> On Fri, Jan 31, 2014 at 12:21 PM, Srikanth Thokala <sthokal-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>> wrote:
>>>
>>> Hi Vinod,
>>>
>>> On Tue, Jan 28, 2014 at 8:43 AM, Vinod Koul <vinod.koul-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>>>>
>>>> On Mon, Jan 27, 2014 at 06:42:36PM +0530, Srikanth Thokala wrote:
>>>>>
>>>>> Hi Lars/Vinod,
>>>>>>>
>>>>>>> The question here i think would be waht this device supports? Is the
>>>>>>> hardware
>>>>>>> capable of doing interleaved transfers, then would make sense.
>>>>>>
>>>>>>
>>>>>> The hardware does 2D transfers. The parameters for a transfer are
>>>>>> height,
>>>>>> width and stride. That's only a subset of what interleaved transfers
>>>>>> can be
>>>>>> (xt->num_frames must be one for 2d transfers). But if I remember
>>>>>> correctly
>>>>>> there has been some discussion on this in the past and the result of
>>>>>> that
>>>>>> discussion was that using interleaved transfers for 2D transfers is
>>>>>> preferred over adding a custom API for 2D transfers.
>>>>>
>>>>>
>>>>> I went through the prep_interleaved_dma API and I see only one
>>>>> descriptor
>>>>> is prepared per API call (i.e. per frame). As our IP supports upto 16
>>>>> frame
>>>>> buffers (can be more in future), isn't it less efficient compared to
>>>>> the
>>>>> prep_slave_sg where we get a single sg list and can prepare all the
>>>>> descriptors
>>>>> (of non-contiguous buffers) in one go? Correct me, if am wrong and let
>>>>> me
>>>>> know your opinions.
>>>>
>>>> Well the descriptor maybe one, but that can represent multiple frames,
>>>> for
>>>> example 16 as in your case. Can you read up the documentation of how
>>>> multiple
>>>> frames are passed. Pls see include/linux/dmaengine.h
>>>>
>>>> /**
>>>> * Interleaved Transfer Request
>>>> * ----------------------------
>>>> * A chunk is collection of contiguous bytes to be transfered.
>>>> * The gap(in bytes) between two chunks is called inter-chunk-gap(ICG).
>>>> * ICGs may or maynot change between chunks.
>>>> * A FRAME is the smallest series of contiguous {chunk,icg} pairs,
>>>> * that when repeated an integral number of times, specifies the
>>>> transfer.
>>>> * A transfer template is specification of a Frame, the number of times
>>>> * it is to be repeated and other per-transfer attributes.
>>>> *
>>>> * Practically, a client driver would have ready a template for each
>>>> * type of transfer it is going to need during its lifetime and
>>>> * set only 'src_start' and 'dst_start' before submitting the
>>>> requests.
>>>> *
>>>> *
>>>> * | Frame-1 | Frame-2 | ~ |
>>>> Frame-'numf' |
>>>> * |====....==.===...=...|====....==.===...=...| ~
>>>> |====....==.===...=...|
>>>> *
>>>> * == Chunk size
>>>> * ... ICG
>>>> */
>>>
>>>
>>> Yes, it can handle multiple frames specified by 'numf' each of size
>>> 'frame_size * sgl[0].size'.
>>> But, I see it only works if all the frames' memory is contiguous and
>>> in this case we
>>> can just increment 'src_start' by the total frame size 'numf' number
>>> of times to fill in
>>> for each HW descriptor (each frame is one HW descriptor). So, there
>>> is no issue when the
>>> memory is contiguous. If the frames are non contiguous, we have to
>>> call this API for each
>>> frame (hence for each descriptor), as the src_start for each frame is
>>> different. Is it correct?
>>>
>>> FYI: This hardware has an inbuilt Scatter-Gather engine.
>>>
>>
>> Ping?
>
>
> If you want to submit multiple frames at once I think you should look at how
> the current dmaengine API can be extended to allow that. And also provide an
> explanation on how this is superior over submitting them one by one.
Sure. I would start with explaning the current implementation of this driver.
Using prep_slave_sg(), we can define multiple segments in a
async_tx_descriptor where each frame is defined by a segment (a sg
list entry). So, the slave device could DMA the data (of multiple
frames) with a descriptor by calling tx_submit in a transaction i.e.,
prep_slave_sg(16) -> tx_submit(1) -> interrupt (16 frames)
Using interleaved_dma(), we could not divide into segments when we
have scattered memory (for the reasons mentioned in above thread).
This implies we are restricting the slave device to process frame by
frame i.e.,
interleaved_dma(1) -> tx_submit(1) -> interrupt -> interleaved_dma(2)
-> tx_submit (2) -> interrupt -> ........ tx_submit(16) -> interrupt
This implementation makes the hardware to wait until the next frame is
submitted.
To overcome this, I feel it would be a good option if we could extend
interleaved_dma template to modify src_start/dest_start to be a
pointer to an array of addresses. Here, number of addresses will be
defined by numf. The other option would be to include scatterlist in
the interleaved template. This way we can handle scattered memory
using this API.
Srikanth
>
> - Lars
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
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: [alsa-devel] [PATCH] ASoC: pcm512x: Add PCM512x driver
From: Mark Brown @ 2014-02-06 13:50 UTC (permalink / raw)
To: Takashi Iwai
Cc: Florian Meier, Liam Girdwood, devicetree-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw
In-Reply-To: <s5hob2kv0z3.wl%tiwai-l3A5Bk7waGM@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]
On Thu, Feb 06, 2014 at 02:12:48PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:
> > > The numbers of items in pcm512x_dsp_program_texts[] and _values[]
> > > don't match.
> > Yeah, fixed.
> I'm thinking whether we can check this in the macro.
> I thought of using BUILD_BUG_ON(), but it's unsure whether it aligns
> there well.
Yes, or changing the way we pass things in so it's an array of
key/value. Usability isn't great for enums in general and particularly
poor for sparse ones.
> > Hrm, it is but this points out an error in the control helpers which has
> > been there since forever - they call that parameter max but it's not a
> > maximum, it's the number of elements in the enumeration. I bet we have
> > a bunch of other enumerations which miss the last element as a result.
> Yeah, the argument name is really confusing. I had to double-check
> the code when I reviewed your patch, too :)
> Also it'd be better to have a practice to use either ARRAY_SIZE() or a
> constant there, too.
Indeed - even better would be to just remove the parameter entirely and
use ARRAY_SIZE() on the array of strings we get passed in so there's no
way it could be messed up.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2 v3] i2c: exynos5: add support for HSI2C on Exynos5260 SoC
From: Tomasz Figa @ 2014-02-06 13:50 UTC (permalink / raw)
To: Naveen Krishna Chatradhi, linux-samsung-soc, linux-i2c
Cc: linux-arm-kernel, devicetree@vger.kernel.org, naveenkrishna.ch,
kgene.kim, grant.likely, Wolfram Sang, linux-kernel, taeggyun.ko,
balbi, cpgs
In-Reply-To: <52F38EA4.10805@samsung.com>
Also, please use correct addresses of DT ML and Wolfram's e-mail (fixed
in this message).
Best regards,
Tomasz
On 06.02.2014 14:31, Tomasz Figa wrote:
> Hi Naveen,
>
> On 06.02.2014 13:06, Naveen Krishna Chatradhi wrote:
>> This patch implements a variant struct to handle the differences
>> (like fifo_depths) in the HSI2C modules across SoCs.
>>
>> Adds a new compatible to support HSI2C module on Exynos5260.
>> Also resets the module as an init sequence (Needed by Exynos5260).
>>
>> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
>> ---
>> Changes since v2:
>> 1. Used variant struct as suggested by Tomasz Figa.
>> 2. Change compatible strings from samsung,exynos5-hsi2c to
>> samsung,exynos5250-hsi2c based on the first SoC to see the feature.
>> 3. Using reset as init sequences.
>> 4. Merged the 2 patches into one.
>>
>> .../devicetree/bindings/i2c/i2c-exynos5.txt | 8 ++-
>> drivers/i2c/busses/i2c-exynos5.c | 64
>> ++++++++++++++++----
>> 2 files changed, 58 insertions(+), 14 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
>> b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
>> index 056732c..5bc4998 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
>> @@ -5,7 +5,11 @@ at various speeds ranging from 100khz to 3.4Mhz.
>>
>> Required properties:
>> - compatible: value should be.
>> - -> "samsung,exynos5-hsi2c", for i2c compatible with exynos5 hsi2c.
>
> Device tree bindings need to be backwards compatible, so you need to
> keep this compatible string supported, just marked as (DEPRECATED).
>
> Driver-wise, it will use the same driver data / variant struct as
> "samsung,exynos5250-hsi2c", just one more entry in OF match table is
> needed.
>
>> + -> "samsung,exynos5250-hsi2c", for i2c compatible with HSI2C
>> available
>> + on Exynos5250 and Exynos5420 SoCs.
>> + -> "samsung,exynos5260-hsi2c", for i2c compatible with HSI2C
>> available
>> + on Exynos5260 SoCs.
>> +
>> - reg: physical base address of the controller and length of
>> memory mapped
>> region.
>> - interrupts: interrupt number to the cpu.
>> @@ -26,7 +30,7 @@ Optional properties:
>> Example:
>>
>> hsi2c@12ca0000 {
>> - compatible = "samsung,exynos5-hsi2c";
>> + compatible = "samsung,exynos5250-hsi2c";
>> reg = <0x12ca0000 0x100>;
>> interrupts = <56>;
>> clock-frequency = <100000>;
>
> [snip]
>
>> @@ -483,6 +514,7 @@ static void exynos5_i2c_message_start(struct
>> exynos5_i2c *i2c, int stop)
>> u32 i2c_auto_conf = 0;
>> u32 fifo_ctl;
>> unsigned long flags;
>> + unsigned short trig_lvl;
>>
>> i2c_ctl = readl(i2c->regs + HSI2C_CTL);
>> i2c_ctl &= ~(HSI2C_TXCHON | HSI2C_RXCHON);
>> @@ -493,13 +525,19 @@ static void exynos5_i2c_message_start(struct
>> exynos5_i2c *i2c, int stop)
>>
>> i2c_auto_conf = HSI2C_READ_WRITE;
>>
>> - fifo_ctl |= HSI2C_RXFIFO_TRIGGER_LEVEL(HSI2C_DEF_TXFIFO_LVL);
>> + trig_lvl = (i2c->msg->len > i2c->variant->fifo_depth) ?
>> + (i2c->variant->fifo_depth * 3/4) : i2c->msg->len;
>> + fifo_ctl |= HSI2C_RXFIFO_TRIGGER_LEVEL(trig_lvl);
>> +
>
> This is a rather serious semantic change, that doesn't look to belong to
> this patch. If this is needed, it should be done in a separate patch.
>
>> int_en |= (HSI2C_INT_RX_ALMOSTFULL_EN |
>> HSI2C_INT_TRAILING_EN);
>> } else {
>> i2c_ctl |= HSI2C_TXCHON;
>>
>> - fifo_ctl |= HSI2C_TXFIFO_TRIGGER_LEVEL(HSI2C_DEF_RXFIFO_LVL);
>> + trig_lvl = (i2c->msg->len > i2c->variant->fifo_depth) ?
>> + (i2c->variant->fifo_depth * 1/4) : i2c->msg->len;
>> + fifo_ctl |= HSI2C_TXFIFO_TRIGGER_LEVEL(trig_lvl);
>> +
>
> Ditto.
>
> Best regards,
> Tomasz
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH] DT: net: document Ethernet bindings in one place
From: Sergei Shtylyov @ 2014-02-06 14:06 UTC (permalink / raw)
To: Grant Likely, Florian Fainelli, Rob Herring
Cc: netdev, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, devicetree@vger.kernel.org, Rob Landley,
linux-doc@vger.kernel.org
In-Reply-To: <20140206094301.03572C40B6A@trevor.secretlab.ca>
Hello.
On 06-02-2014 13:43, Grant Likely wrote:
>>>>>>>>>>> I'm afraid that's too late, it has spread very far, so that
>>>>>>>>>>> of_get_phy_mode() handles that property, not "phy-connection-type".
>>>>>>>>>> Uggg, I guess this is a case of a defacto standard then if the kernel
>>>>>>>>>> doesn't even support it.
>>>>>>>>> Maybe I forgot to CC you on patch sent to Grant only, I sent a patch a
>>>>>>>>> while ago for of_get_phy_mode() to look for both "phy-mode" and
>>>>>>>>> "phy-connection-type" since the former has been a Linux invention, but
>>>>>>>>> the latter is ePAPR specified.
>>>>>>>> Here is a link to the actual patch in question, not sure which tree
>>>>>>>> Grant applied it to though:
>>>>>>>> http://lkml.indiana.edu/hypermail/linux/kernel/1311.2/00048.html
>>>>>>> It's not the patch mail, it's Grant's "applied" reply, patch is mangled in
>>>>>>> this reply, and I couldn't follow the thread. Here's the actual patch mail:
>>>>>>> http://marc.info/?l=devicetree&m=138449662807254
>>>>>> Florian, I didn't find this patch in Grant's official tree, so maybe you
>>>>>> should ask him where is the patch already?
>>>>> Sorry, I accidentally dropped it. It will be in the next merge window.
>>>> Already saw it, thanks. Would that it was in 3.14 instead of course, so
>>>> that I could use "phy-connection-type" in my binding...
>>> Is 3.14 broken because of missing the patch? If so I'll get it merged as
>> > a bug fix.
>> No, it's not. I could have used "phy-connection-type" in my binding
>> destined for 3.15 and document it as a preferred property as well.
> You still can. We just need to make sure that your patch is applied on
Patches.
> top of the phy-connection-type patch.
I'm not sure this trick is possible if the patches are merged via the
different trees...
> g.
WBR, Sergei
^ permalink raw reply
* Re: [PATCH v3] phy: Add new Exynos5 USB 3.0 PHY driver
From: Tomasz Figa @ 2014-02-06 14:07 UTC (permalink / raw)
To: Vivek Gautam, linux-usb, linux-samsung-soc
Cc: linux-kernel, devicetree, linux-doc, gregkh, kgene.kim, balbi,
kishon, k.debski, s.nawrocki, jwerner, jg1.han
In-Reply-To: <1390225363-24210-1-git-send-email-gautam.vivek@samsung.com>
Hi Vivek,
This patch is just adding the PHY driver. I would also like to look at
some users of it, to see how this works when put together.
For now, please see my comments inline.
On 20.01.2014 14:42, Vivek Gautam wrote:
> Add a new driver for the USB 3.0 PHY on Exynos5 series of SoCs.
> The new driver uses the generic PHY framework and will interact
> with DWC3 controller present on Exynos5 series of SoCs.
> Thereby, removing old phy-samsung-usb3 driver and related code
> used untill now which was based on usb/phy framework.
>
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> ---
>
> Changes from v2:
> 1) Added support for multiple PHYs (UTMI+ and PIPE3) and
> related changes in the driver structuring.
I'm a bit skeptical about this separation. Can the PHY operate with just
the UTMI+ or PIPE3 part enabled alone without the other? Can any PHY
consumer operate this way?
Introducing separation of something that can't exist alone doesn't add
any value, but instead makes things more difficult to work with. Of
course, it's fine if the answer to my questions above if yes, but better
safe than sorry.
> 2) Added a xlate function to get the required phy out of
> number of PHYs in mutiple PHY scenerio.
> 3) Changed the names of few structures and variables to
> have a clearer meaning.
> 4) Added 'usb3phy_config' structure to take care of mutiple
> phys for a SoC having 'exynos5_usb3phy_drv_data' driver data.
> 5) Not deleting support for old driver 'phy-samsung-usb3' until
> required support for generic phy is added to DWC3.
[snip]
> +
> +- aliases: For SoCs like Exynos5420 having multiple USB PHY controllers,
> + 'usb3_phy' nodes should have numbered alias in the aliases node,
> + in the form of usb3phyN, N = 0, 1... (depending on number of
> + controllers).
> +Example:
> + aliases {
> + usb3phy0 = &usb3_phy0;
> + usb3phy1 = &usb3_phy1;
> + };
What is the reason to have these aliases?
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 330ef2d..32f9f38 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -51,4 +51,12 @@ config PHY_EXYNOS_DP_VIDEO
> help
> Support for Display Port PHY found on Samsung EXYNOS SoCs.
>
> +config PHY_EXYNOS5_USB3
> + tristate "Exynos5 SoC series USB 3.0 PHY driver"
> + depends on ARCH_EXYNOS5
> + select GENERIC_PHY
> + select MFD_SYSCON
> + help
> + Enable USB 3.0 PHY support for Exynos 5 SoC series
> +
> endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index d0caae9..9c06a61 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO) += phy-exynos-dp-video.o
> obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-video.o
> obj-$(CONFIG_OMAP_USB2) += phy-omap-usb2.o
> obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o
> +obj-$(CONFIG_PHY_EXYNOS5_USB3) += phy-exynos5-usb3.o
> diff --git a/drivers/phy/phy-exynos5-usb3.c b/drivers/phy/phy-exynos5-usb3.c
> new file mode 100644
> index 0000000..24efed0
> --- /dev/null
> +++ b/drivers/phy/phy-exynos5-usb3.c
> @@ -0,0 +1,621 @@
> +/*
> + * Samsung EXYNOS5 SoC series USB 3.0 PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Vivek Gautam <gautam.vivek@samsung.com>
> + *
> + * 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/mutex.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +
> +/* Exynos USB PHY registers */
> +#define EXYNOS5_FSEL_9MHZ6 0x0
> +#define EXYNOS5_FSEL_10MHZ 0x1
> +#define EXYNOS5_FSEL_12MHZ 0x2
> +#define EXYNOS5_FSEL_19MHZ2 0x3
> +#define EXYNOS5_FSEL_20MHZ 0x4
> +#define EXYNOS5_FSEL_24MHZ 0x5
> +#define EXYNOS5_FSEL_50MHZ 0x7
> +
> +/* EXYNOS5: USB 3.0 DRD PHY registers */
> +#define EXYNOS5_DRD_LINKSYSTEM (0x04)
nit: No need for parentheses around simple literal. (+ more occurrences
below)
> +
> +#define LINKSYSTEM_FLADJ_MASK (0x3f << 1)
> +#define LINKSYSTEM_FLADJ(_x) ((_x) << 1)
> +#define LINKSYSTEM_XHCI_VERSION_CONTROL (0x1 << 27)
nit: BIT() macro could be used for single bits. (+ more occurrences below)
> +
> +#define EXYNOS5_DRD_PHYUTMI (0x08)
> +
> +#define PHYUTMI_OTGDISABLE (0x1 << 6)
> +#define PHYUTMI_FORCESUSPEND (0x1 << 1)
> +#define PHYUTMI_FORCESLEEP (0x1 << 0)
> +
> +#define EXYNOS5_DRD_PHYPIPE (0x0c)
> +
> +#define EXYNOS5_DRD_PHYCLKRST (0x10)
> +
> +#define PHYCLKRST_EN_UTMISUSPEND (0x1 << 31)
> +
> +#define PHYCLKRST_SSC_REFCLKSEL_MASK (0xff << 23)
> +#define PHYCLKRST_SSC_REFCLKSEL(_x) ((_x) << 23)
> +
> +#define PHYCLKRST_SSC_RANGE_MASK (0x03 << 21)
> +#define PHYCLKRST_SSC_RANGE(_x) ((_x) << 21)
> +
> +#define PHYCLKRST_SSC_EN (0x1 << 20)
> +#define PHYCLKRST_REF_SSP_EN (0x1 << 19)
> +#define PHYCLKRST_REF_CLKDIV2 (0x1 << 18)
> +
> +#define PHYCLKRST_MPLL_MULTIPLIER_MASK (0x7f << 11)
> +#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF (0x19 << 11)
> +#define PHYCLKRST_MPLL_MULTIPLIER_50M_REF (0x32 << 11)
> +#define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF (0x68 << 11)
> +#define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF (0x7d << 11)
> +#define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF (0x02 << 11)
> +
> +#define PHYCLKRST_FSEL_MASK (0x3f << 5)
> +#define PHYCLKRST_FSEL(_x) ((_x) << 5)
> +#define PHYCLKRST_FSEL_PAD_100MHZ (0x27 << 5)
> +#define PHYCLKRST_FSEL_PAD_24MHZ (0x2a << 5)
> +#define PHYCLKRST_FSEL_PAD_20MHZ (0x31 << 5)
> +#define PHYCLKRST_FSEL_PAD_19_2MHZ (0x38 << 5)
> +
> +#define PHYCLKRST_RETENABLEN (0x1 << 4)
> +
> +#define PHYCLKRST_REFCLKSEL_MASK (0x03 << 2)
> +#define PHYCLKRST_REFCLKSEL_PAD_REFCLK (0x2 << 2)
> +#define PHYCLKRST_REFCLKSEL_EXT_REFCLK (0x3 << 2)
> +
> +#define PHYCLKRST_PORTRESET (0x1 << 1)
> +#define PHYCLKRST_COMMONONN (0x1 << 0)
> +
> +#define EXYNOS5_DRD_PHYREG0 (0x14)
> +#define EXYNOS5_DRD_PHYREG1 (0x18)
> +
> +#define EXYNOS5_DRD_PHYPARAM0 (0x1c)
> +
> +#define PHYPARAM0_REF_USE_PAD (0x1 << 31)
> +#define PHYPARAM0_REF_LOSLEVEL_MASK (0x1f << 26)
> +#define PHYPARAM0_REF_LOSLEVEL (0x9 << 26)
> +
> +#define EXYNOS5_DRD_PHYPARAM1 (0x20)
> +
> +#define PHYPARAM1_PCS_TXDEEMPH_MASK (0x1f << 0)
> +#define PHYPARAM1_PCS_TXDEEMPH (0x1c)
> +
> +#define EXYNOS5_DRD_PHYTERM (0x24)
> +
> +#define EXYNOS5_DRD_PHYTEST (0x28)
> +
> +#define PHYTEST_POWERDOWN_SSP (0x1 << 3)
> +#define PHYTEST_POWERDOWN_HSP (0x1 << 2)
> +
> +#define EXYNOS5_DRD_PHYADP (0x2c)
> +
> +#define EXYNOS5_DRD_PHYBATCHG (0x30)
> +
> +#define PHYBATCHG_UTMI_CLKSEL (0x1 << 2)
> +
> +#define EXYNOS5_DRD_PHYRESUME (0x34)
> +#define EXYNOS5_DRD_LINKPORT (0x44)
> +
> +/* Power isolation defined in power management unit */
> +#define EXYNOS5_USB3DRD_PHY_PMU_REG_OFFSET (0x704)
> +#define EXYNOS5_USB3DRD_PMU_ISOL (1 << 0)
> +
> +#define KHZ 1000
> +#define MHZ (KHZ * KHZ)
> +
> +enum exynos5_usb3phy_id {
> + EXYNOS5_USB3PHY_UTMI,
> + EXYNOS5_USB3PHY_PIPE3,
> + EXYNOS5_USB3PHYS_NUM,
> +};
> +
> +struct usb3phy_config {
> + u32 id;
> + u32 reg_pmu_offset;
> + void (*phy_isol)(struct phy *phy, u32 on);
> +};
> +
> +struct exynos5_usb3phy_drv_data {
> + bool has_usb30_sclk;
> + bool has_multi_controller;
> + const struct usb3phy_config *phy_cfg;
> +};
> +
> +/**
> + * struct exynos5_usb3phy_driver - driver data for USB 3.0 PHY
> + * @dev: pointer to device instance of this platform device
> + * @reg_phy: usb phy controller register memory base
> + * @clk: phy clock for register access
> + * @usb30_sclk: additional special clock for phy operations
> + * @drv_data: pointer to SoC level driver data structure
> + * @phys[]: array for 'EXYNOS5_USB3PHYS_NUM' number of PHY
> + * instances each with its 'phy' and 'phy_cfg'.
> + * @extrefclk: frequency select settings when using 'separate
> + * reference clocks' for SS and HS operations
> + * @rate: rate of reference clock to PHY block
> + * @channel: number of PHY channels present in SoC
> + */
> +struct exynos5_usb3phy_driver {
> + struct device *dev;
> + void __iomem *reg_phy;
> + struct clk *clk;
> + struct clk *usb30_sclk;
> + const struct exynos5_usb3phy_drv_data *drv_data;
> + struct phy_usb_instance {
> + struct phy *phy;
> + u32 index;
> + struct regmap *reg_isol;
> + const struct usb3phy_config *phy_cfg;
> + } phys[EXYNOS5_USB3PHYS_NUM];
> + u32 extrefclk;
> + unsigned long rate;
> + u32 channel;
> +};
> +
> +#define to_usb3phy_driver(inst) \
> + container_of((inst), struct exynos5_usb3phy_driver, \
> + phys[(inst)->index]);
> +
> +/*
> + * exynos5_rate_to_clk() converts the supplied clock rate to the value that
> + * can be written to the phy register.
> + */
> +static u32 exynos5_rate_to_clk(unsigned long rate)
> +{
> + unsigned int clksel;
> +
> + /* EXYNOS5_FSEL_MASK */
> +
> + switch (rate) {
> + case 9600 * KHZ:
> + clksel = EXYNOS5_FSEL_9MHZ6;
> + break;
> + case 10 * MHZ:
> + clksel = EXYNOS5_FSEL_10MHZ;
> + break;
> + case 12 * MHZ:
> + clksel = EXYNOS5_FSEL_12MHZ;
> + break;
> + case 19200 * KHZ:
> + clksel = EXYNOS5_FSEL_19MHZ2;
> + break;
> + case 20 * MHZ:
> + clksel = EXYNOS5_FSEL_20MHZ;
> + break;
> + case 24 * MHZ:
> + clksel = EXYNOS5_FSEL_24MHZ;
> + break;
> + case 50 * MHZ:
> + clksel = EXYNOS5_FSEL_50MHZ;
> + break;
> + default:
> + clksel = -EINVAL;
> + }
> +
> + return clksel;
> +}
> +
> +static void exynos5_usb3phy_isol(struct phy *phy, unsigned int on)
> +{
> + u32 pmu_offset;
> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> + struct exynos5_usb3phy_driver *drv = to_usb3phy_driver(inst);
> +
> + pmu_offset = inst->phy_cfg->reg_pmu_offset;
> + if (!inst->reg_isol)
> + return;
> +
> + switch (drv->channel) {
> + case 1:
> + /* Channel 1 is at 0x708 offset */
> + pmu_offset += sizeof(&pmu_offset);
> + break;
> + case 0:
> + default:
> + /* Channel 0 is at 0x704 offset */
> + break;
> + }
> +
> + regmap_update_bits(inst->reg_isol, pmu_offset,
> + EXYNOS5_USB3DRD_PMU_ISOL, ~on);
> +}
> +
> +/*
> + * Sets the phy clk as EXTREFCLK (XXTI) which is internal clock from clock core.
> + */
> +static u32 exynos5_usb3phy_set_refclk(struct exynos5_usb3phy_driver *drv)
> +{
> + u32 reg;
> +
> + reg = PHYCLKRST_REFCLKSEL_EXT_REFCLK |
> + PHYCLKRST_FSEL(drv->extrefclk);
> +
> + switch (drv->extrefclk) {
> + case EXYNOS5_FSEL_50MHZ:
> + reg |= (PHYCLKRST_MPLL_MULTIPLIER_50M_REF |
> + PHYCLKRST_SSC_REFCLKSEL(0x00));
> + break;
> + case EXYNOS5_FSEL_24MHZ:
> + reg |= (PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF |
> + PHYCLKRST_SSC_REFCLKSEL(0x88));
> + break;
> + case EXYNOS5_FSEL_20MHZ:
> + reg |= (PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF |
> + PHYCLKRST_SSC_REFCLKSEL(0x00));
> + break;
> + case EXYNOS5_FSEL_19MHZ2:
> + reg |= (PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF |
> + PHYCLKRST_SSC_REFCLKSEL(0x88));
> + break;
> + default:
> + dev_dbg(drv->dev, "unsupported ref clk\n");
> + break;
> + }
> +
> + return reg;
> +}
> +
> +static int exynos5_usb3phy_init(struct phy *phy)
> +{
> + int ret;
> + u32 phyparam0;
> + u32 phyparam1;
> + u32 linksystem;
> + u32 phybatchg;
> + u32 phytest;
> + u32 phyclkrst;
> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> + struct exynos5_usb3phy_driver *drv = to_usb3phy_driver(inst);
> +
> + ret = clk_prepare_enable(drv->clk);
> + if (ret)
> + return ret;
> +
> + drv->extrefclk = exynos5_rate_to_clk(drv->rate);
> + if (drv->extrefclk == -EINVAL) {
> + dev_err(drv->dev, "Clock rate (%ld) not supported\n",
> + drv->rate);
> + return -EINVAL;
> + }
> +
> + /* Reset USB 3.0 PHY */
> + writel(0x0, drv->reg_phy + EXYNOS5_DRD_PHYREG0);
> +
> + phyparam0 = readl(drv->reg_phy + EXYNOS5_DRD_PHYPARAM0);
> + /* Select PHY CLK source */
> + phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
> + /* Set Loss-of-Signal Detector sensitivity */
> + phyparam0 &= ~PHYPARAM0_REF_LOSLEVEL_MASK;
> + phyparam0 |= PHYPARAM0_REF_LOSLEVEL;
> + writel(phyparam0, drv->reg_phy + EXYNOS5_DRD_PHYPARAM0);
> +
> + writel(0x0, drv->reg_phy + EXYNOS5_DRD_PHYRESUME);
> +
> + /*
> + * Setting the Frame length Adj value[6:1] to default 0x20
> + * See xHCI 1.0 spec, 5.2.4
> + */
> + linksystem = LINKSYSTEM_XHCI_VERSION_CONTROL |
> + LINKSYSTEM_FLADJ(0x20);
> + writel(linksystem, drv->reg_phy + EXYNOS5_DRD_LINKSYSTEM);
> +
> + phyparam1 = readl(drv->reg_phy + EXYNOS5_DRD_PHYPARAM1);
> + /* Set Tx De-Emphasis level */
> + phyparam1 &= ~PHYPARAM1_PCS_TXDEEMPH_MASK;
> + phyparam1 |= PHYPARAM1_PCS_TXDEEMPH;
> + writel(phyparam1, drv->reg_phy + EXYNOS5_DRD_PHYPARAM1);
> +
> + phybatchg = readl(drv->reg_phy + EXYNOS5_DRD_PHYBATCHG);
> + phybatchg |= PHYBATCHG_UTMI_CLKSEL;
> + writel(phybatchg, drv->reg_phy + EXYNOS5_DRD_PHYBATCHG);
> +
> + /* PHYTEST POWERDOWN Control */
> + phytest = readl(drv->reg_phy + EXYNOS5_DRD_PHYTEST);
> + phytest &= ~(PHYTEST_POWERDOWN_SSP |
> + PHYTEST_POWERDOWN_HSP);
> + writel(phytest, drv->reg_phy + EXYNOS5_DRD_PHYTEST);
> +
> + /* UTMI Power Control */
> + writel(PHYUTMI_OTGDISABLE, drv->reg_phy + EXYNOS5_DRD_PHYUTMI);
> +
> + phyclkrst = exynos5_usb3phy_set_refclk(drv);
> +
> + phyclkrst |= PHYCLKRST_PORTRESET |
> + /* Digital power supply in normal operating mode */
> + PHYCLKRST_RETENABLEN |
> + /* Enable ref clock for SS function */
> + PHYCLKRST_REF_SSP_EN |
> + /* Enable spread spectrum */
> + PHYCLKRST_SSC_EN |
> + /* Power down HS Bias and PLL blocks in suspend mode */
> + PHYCLKRST_COMMONONN;
> +
> + writel(phyclkrst, drv->reg_phy + EXYNOS5_DRD_PHYCLKRST);
> +
> + udelay(10);
> +
> + phyclkrst &= ~PHYCLKRST_PORTRESET;
> + writel(phyclkrst, drv->reg_phy + EXYNOS5_DRD_PHYCLKRST);
> +
> + clk_disable_unprepare(drv->clk);
I'm still not convinced that this is the right place for this setup.
This way you force consumer driver to always call phy_power_on() and
phy_init() together, otherwise the PHY won't work.
I believe the right thing to do here is to do all the initialization in
.power_on() and let the driver simply call phy_power_on() when it needs
the PHY and phy_power_off() otherwise.
Analogically, .exit() should be merged into .power_off().
> +
> + return 0;
> +}
[snip]
> + /*
> + * Exynos5420 SoC has multiple channels for USB 3.0 PHY, with
> + * each having separate power control registers.
> + * 'drv->channel' facilitates to set such registers.
> + */
> + if (drv->drv_data->has_multi_controller) {
> + drv->channel = of_alias_get_id(node, "usb3phy");
> + if (drv->channel < 0) {
> + dev_err(dev, "Invalid usb3drd phy node\n");
> + return -EINVAL;
> + }
> + }
Aha, so this is why you need aliases. Maybe a "samsung,pmu-offset"
property, simply specifying the offset to PMU regs would be better here?
> +
> + drv->clk = devm_clk_get(dev, "phy");
> + if (IS_ERR(drv->clk)) {
> + dev_err(dev, "Failed to get clock of phy controller\n");
> + return PTR_ERR(drv->clk);
> + }
> +
> + /*
> + * Exysno5420 SoC has an additional special clock, used for
> + * for USB 3.0 PHY operation, this clock goes to the PHY block
> + * as a reference clock to clock generation block of the controller,
> + * named as 'USB30_SCLK_100M'.
> + */
> + if (drv_data->has_usb30_sclk) {
> + drv->usb30_sclk = devm_clk_get(dev, "usb30_sclk_100m");
> + if (IS_ERR(drv->usb30_sclk)) {
> + dev_err(dev, "Failed to get phy reference clock\n");
> + return PTR_ERR(drv->usb30_sclk);
> + }
> + }
> +
> + clk = clk_get(dev, "usb3phy_refclk");
> + if (IS_ERR(clk)) {
> + dev_err(dev, "Failed to get reference clock of usb3drd phy\n");
> + return PTR_ERR(clk);
> + }
> + drv->rate = clk_get_rate(clk);
> + clk_put(clk);
To comply with clock API semantics, you should keep the reference on
this clock until the driver is removed. Moreover, I believe you should
call clk_prepare_enable() on it as well, to make sure that the clock is
enabled, even if current implementation of clock drivers for Exynos 5
can't disable this clock.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v5 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone
From: Ivan Khoronzhuk @ 2014-02-06 14:08 UTC (permalink / raw)
To: Thomas Gleixner
Cc: santosh.shilimkar-l0cyMroinI0, rob-VoJi6FS/r0vR7s880joybQ,
linux-lFZ/pmaqli7XmaaqVzeoHQ, galak-sgV2jX0FEOL9JmXXK+q4OQ,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
grygorii.strashko-l0cyMroinI0
In-Reply-To: <alpine.DEB.2.02.1402052123560.24986-3cz04HxQygjZikZi3RtOZ1XZhhPuCNm+@public.gmane.org>
On 02/05/2014 10:27 PM, Thomas Gleixner wrote:
> On Wed, 5 Feb 2014, Ivan Khoronzhuk wrote:
>> + /* here we have to be sure the timer has been disabled */
> Sigh. This is not a proper explanation for a barrier, really. You want
> to explain what it serializes against what. i.e. you want to explain
> why you are using the relaxed functions and avoid a separate non
> relaxed variant in favour of an explicit barrier.
>
>> + __iowmb();
> The proper thing is to have an inline function key_stone_barrier() and
> a full explanation of the issue in exactly that place instead of
> handwaving comments here and there.
>
> Thanks,
>
> tglx
I can add new inline function like:
/**
* keystone_timer_barrier: write memory barrier
* use explicit barrier to avoid using readl/writel non relaxed function
* variants, because in our case relaxed variants hide the true places
* where barrier is needed.
*/
static inline void keystone_timer_barrier(void)
{
__iowmb();
}
and use it where it is needed.
Are you OK with it?
And I propose to leave comments under the barriers in order to be
able to understand why they are used.
--
Regards,
Ivan Khoronzhuk
--
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 v5 2/3] clocksource: keystone: add bindings for keystone timer
From: Ivan Khoronzhuk @ 2014-02-06 14:09 UTC (permalink / raw)
To: Rob Herring
Cc: Santosh Shilimkar, Rob Landley, Russell King - ARM Linux,
Kumar Gala, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Daniel Lezcano, Thomas Gleixner, devicetree@vger.kernel.org,
linux-doc@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Grygorii Strashko
In-Reply-To: <CAL_JsqLSNrTvH2SFfA5KB8RsDqcj9zks1QaVmvYya6WQ--W7jg@mail.gmail.com>
On 02/06/2014 01:36 AM, Rob Herring wrote:
> On Wed, Feb 5, 2014 at 12:52 PM, Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> wrote:
>> On 02/05/2014 07:41 PM, Rob Herring wrote:
>>> On Wed, Feb 5, 2014 at 10:18 AM, Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> wrote:
>>>> On 02/05/2014 04:39 PM, Rob Herring wrote:
>>>>> On Wed, Feb 5, 2014 at 7:47 AM, Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>>> wrote:
>>>>>> This patch provides bindings for the 64-bit timer in the KeyStone
>>>>>> architecture devices. The timer can be configured as a general-purpose
>>>>>> 64-bit
>>>>>> timer, dual general-purpose 32-bit timers. When configured as dual
>>>>>> 32-bit
>>>>>> timers, each half can operate in conjunction (chain mode) or
>>>>>> independently
>>>>>> (unchained mode) of each other.
>>>>> This is software configurable or h/w design time configurations?
>>>>>
>>>>> Rob
>>>>>
>>>> This is h/w design time configurations
>>> Then it seems like the binding should provide for describing those
>>> differences either with a property or different compatible strings.
>>>
>>> Rob
>> Oh..sorry, seems I didn't catch, this is configurable by software.
>> These configurations are like modes in which timer can work
>> and they are not different hardware IPs. It depends on driver in
>> which mode it should work.
> In that case,
>
> Acked-by: Rob Herring <robh@kernel.org>
Thanks
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH v5 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone
From: Ivan Khoronzhuk @ 2014-02-06 14:09 UTC (permalink / raw)
To: Josh Cartwright
Cc: santosh.shilimkar, rob, linux, galak, mark.rutland, devicetree,
grygorii.strashko, pawel.moll, ijc+devicetree, daniel.lezcano,
linux-doc, linux-kernel, robh+dt, tglx, linux-arm-kernel
In-Reply-To: <20140206003526.GQ20228@joshc.qualcomm.com>
On 02/06/2014 02:35 AM, Josh Cartwright wrote:
> Hey Ivan-
>
> On Wed, Feb 05, 2014 at 03:47:38PM +0200, Ivan Khoronzhuk wrote:
>> Add broadcast clock-event device for the Keystone arch.
>>
>> The timer can be configured as a general-purpose 64-bit timer,
>> dual general-purpose 32-bit timers. When configured as dual 32-bit
>> timers, each half can operate in conjunction (chain mode) or
>> independently (unchained mode) of each other.
>>
>> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
>> Acked-by: Santosh shilimkar <santosh.shilimkar@ti.com>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>> drivers/clocksource/Makefile | 1 +
>> drivers/clocksource/timer-keystone.c | 233 +++++++++++++++++++++++++++++++++++
>> 2 files changed, 234 insertions(+)
>> create mode 100644 drivers/clocksource/timer-keystone.c
>>
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index c7ca50a..4abe5aa 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -37,3 +37,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o
>> obj-$(CONFIG_ARM_GLOBAL_TIMER) += arm_global_timer.o
>> obj-$(CONFIG_CLKSRC_METAG_GENERIC) += metag_generic.o
>> obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST) += dummy_timer.o
>> +obj-$(CONFIG_ARCH_KEYSTONE) += timer-keystone.o
>> diff --git a/drivers/clocksource/timer-keystone.c b/drivers/clocksource/timer-keystone.c
>> new file mode 100644
>> index 0000000..2299666
>> --- /dev/null
>> +++ b/drivers/clocksource/timer-keystone.c
>> +static void __init keystone_timer_init(struct device_node *np)
>> +{
>> + struct clock_event_device *event_dev = &timer.event_dev;
>> + unsigned long rate;
>> + struct clk *clk;
>> + int irq, error;
>> + u32 tgcr;
>> +
>> + irq = irq_of_parse_and_map(np, 0);
>> + if (irq == NO_IRQ) {
>> + pr_err("%s: failed to map interrupts\n", __func__);
>> + return;
>> + }
>> +
>> + timer.base = of_iomap(np, 0);
>> + if (!timer.base) {
>> + pr_err("%s: failed to map registers\n", __func__);
>> + return;
>> + }
>> +
>> + clk = of_clk_get(np, 0);
>> + if (!clk) {
> This condition should be IS_ERR(clk).
Thanks Josh,
I'll fix it.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH v5 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone
From: Thomas Gleixner @ 2014-02-06 14:21 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: santosh.shilimkar, rob, linux, galak, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, daniel.lezcano, devicetree,
linux-doc, linux-arm-kernel, linux-kernel, grygorii.strashko
In-Reply-To: <52F3975B.2080806@ti.com>
On Thu, 6 Feb 2014, Ivan Khoronzhuk wrote:
> On 02/05/2014 10:27 PM, Thomas Gleixner wrote:
> > On Wed, 5 Feb 2014, Ivan Khoronzhuk wrote:
> > > + /* here we have to be sure the timer has been disabled */
> > Sigh. This is not a proper explanation for a barrier, really. You want
> > to explain what it serializes against what. i.e. you want to explain
> > why you are using the relaxed functions and avoid a separate non
> > relaxed variant in favour of an explicit barrier.
> >
> > > + __iowmb();
> > The proper thing is to have an inline function key_stone_barrier() and
> > a full explanation of the issue in exactly that place instead of
> > handwaving comments here and there.
> >
> > Thanks,
> >
> > tglx
>
> I can add new inline function like:
>
> /**
> * keystone_timer_barrier: write memory barrier
> * use explicit barrier to avoid using readl/writel non relaxed function
> * variants, because in our case relaxed variants hide the true places
> * where barrier is needed.
> */
> static inline void keystone_timer_barrier(void)
> {
> __iowmb();
> }
>
> and use it where it is needed.
> Are you OK with it?
>
> And I propose to leave comments under the barriers in order to be
> able to understand why they are used.
Sounds good.
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: pcm512x: Add PCM512x driver
From: Mark Brown @ 2014-02-06 14:22 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Florian Meier, Liam Girdwood, devicetree-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw
In-Reply-To: <52F38CF7.7060306-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]
On Thu, Feb 06, 2014 at 02:24:07PM +0100, Lars-Peter Clausen wrote:
> On 02/06/2014 01:26 PM, Mark Brown wrote:
> >+static const char *pcm512x_dsp_program_texts[] = {
> Nitpick: should be "static const char * const ...", a immutable
> array of pointers pointing to immutable strings. Same for all the
> enum texts in this driver.
Oh, good - someone fixed the core code for that without me noticing.
Last time I looked if you made the strings const there were annoying
warnings since they got passed in to things that weren't expecting const
strings and it seemed to annoying to check that it was actually safe in
all cases.
> Another reason why I think it is better to separate the I2C and SPI
> bits into different modules. If the registration of the SPI driver
> fails you'll return an error and the module will not be loaded. At
> the same time the i2c driver is already registered. I know that this
> is rather theoretical, but if we don't care if our error handling is
> correct, because we assume that the error will never happen, we do
> not need error handling at all.
Like I say I'm perfectly aware of the issues, I just don't have much
enthusiasm myself since they pretty much only affect unrealistic and
apparently quite rare randconfigs - the time I might spend on that is
more likely to get spent on things like the format negotiation stuff.
If someone (you? :P) wants to do the work that's fine but nobody had
shown any sign of that until your driver the other day.
I do also want to see some explicit conversion happening so it's clearer
that this is a deliberate change that's being rolled out.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 4/8] regulator: da9055: Add DT support
From: Mark Brown @ 2014-02-06 14:30 UTC (permalink / raw)
To: Opensource [Adam Thomson]
Cc: Lee Jones, alsa-devel@alsa-project.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
Rob Herring, Linus Walleij, Dmitry Torokhov, Alessandro Zummo,
Guenter Roeck
In-Reply-To: <2E89032DDAA8B9408CB92943514A03376B7E36B9@SW-EX-MBX02.diasemi.com>
[-- Attachment #1: Type: text/plain, Size: 488 bytes --]
On Thu, Feb 06, 2014 at 12:22:13PM +0000, Opensource [Adam Thomson] wrote:
> Is what I summised but I obviously chose the wrong direction for my fix.
> Will use platform_get_irq_byname() solely, and will make sure all works on
> both DT and non-DT setups. If not I'll dig further.
Yeah. In general I'd say that if you've got two options for something
like this the option that makes the driver smaller or which replaces
more code with data is normally going to be the right way to go.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 06/15] dt: binding: add binding for ImgTec IR block
From: Rob Herring @ 2014-02-06 14:33 UTC (permalink / raw)
To: James Hogan
Cc: Mauro Carvalho Chehab, linux-media, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala,
devicetree@vger.kernel.org, Rob Landley,
linux-doc@vger.kernel.org, Tomasz Figa
In-Reply-To: <1389967140-20704-7-git-send-email-james.hogan@imgtec.com>
On Fri, Jan 17, 2014 at 7:58 AM, James Hogan <james.hogan@imgtec.com> wrote:
> Add device tree binding for ImgTec Consumer Infrared block, specifically
> major revision 1 of the hardware.
>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> Cc: linux-media@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: devicetree@vger.kernel.org
> Cc: Rob Landley <rob@landley.net>
> Cc: linux-doc@vger.kernel.org
> Cc: Tomasz Figa <tomasz.figa@gmail.com>
> ---
> v2:
> - Future proof compatible string from "img,ir" to "img,ir1", where the 1
> corresponds to the major revision number of the hardware (Tomasz
> Figa).
> - Added clock-names property and three specific clock names described in
> the manual, only one of which is used by the current driver (Tomasz
> Figa).
> ---
> .../devicetree/bindings/media/img-ir1.txt | 30 ++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/img-ir1.txt
>
> diff --git a/Documentation/devicetree/bindings/media/img-ir1.txt b/Documentation/devicetree/bindings/media/img-ir1.txt
> new file mode 100644
> index 0000000..ace5fd9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/img-ir1.txt
> @@ -0,0 +1,30 @@
> +* ImgTec Infrared (IR) decoder version 1
> +
> +This binding is for Imagination Technologies' Infrared decoder block,
> +specifically major revision 1.
> +
> +Required properties:
> +- compatible: Should be "img,ir1"
Kind of short for a name. I don't have anything much better, but how
about img,ir-rev1.
> +- reg: Physical base address of the controller and length of
> + memory mapped region.
> +- interrupts: The interrupt specifier to the cpu.
> +
> +Optional properties:
> +- clocks: List of clock specifiers as described in standard
> + clock bindings.
> +- clock-names: List of clock names corresponding to the clocks
> + specified in the clocks property.
> + Accepted clock names are:
> + "core": Core clock (defaults to 32.768KHz if omitted).
> + "sys": System side (fast) clock.
> + "mod": Power modulation clock.
You need to define the order of clocks including how they are
interpreted with different number of clocks (not relying on the name).
Although, if the h/w block really has different number of clock
inputs, then it is a different h/w block and should have a different
compatible string.
Rob
^ permalink raw reply
* Re: [PATCH 2/9 v5] crypto:s5p-sss: Add device tree support
From: Tomasz Figa @ 2014-02-06 14:36 UTC (permalink / raw)
To: Naveen Krishna Chatradhi, linux-crypto, linux-samsung-soc
Cc: linux-kernel, vzapolskiy, herbert, naveenkrishna.ch, cpgs,
devicetree, David S. Miller
In-Reply-To: <1390987259-18581-1-git-send-email-ch.naveen@samsung.com>
Hi Naveen,
On 29.01.2014 10:20, Naveen Krishna Chatradhi wrote:
> This patch adds device tree support to the s5p-sss.c crypto driver.
>
> Also, Documentation under devicetree/bindings added.
>
> Signed-off-by: Naveen Krishna Ch <ch.naveen@samsung.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: David S. Miller <davem@davemloft.net>
> CC: Vladimir Zapolskiy <vzapolskiy@gmail.com>
> TO: <linux-crypto@vger.kernel.org>
> CC: <linux-samsung-soc@vger.kernel.org>
> ---
> Changes since v4:
> Modified Documentation to give clock names and example for interrupts
>
> Changes since v3:
> None
> .../devicetree/bindings/crypto/samsung-sss.txt | 24 ++++++++++++++++++++
> drivers/crypto/s5p-sss.c | 8 +++++++
> 2 files changed, 32 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/crypto/samsung-sss.txt
>
> diff --git a/Documentation/devicetree/bindings/crypto/samsung-sss.txt b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
> new file mode 100644
> index 0000000..d193084
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
> @@ -0,0 +1,24 @@
> +Samsung SoC SSS (Security SubSystem) module
> +
> +The SSS module in S5PV210 SoC supports the following:
> +-- Feeder (FeedCtrl)
> +-- Advanced Encryption Standard (AES)
> +-- Data Encryption Standard (DES)/3DES
> +-- Public Key Accelerator (PKA)
> +-- SHA-1/SHA-256/MD5/HMAC (SHA-1/SHA-256/MD5)/PRNG
> +-- PRNG: Pseudo Random Number Generator
> +
> +Required properties:
> +
> +- compatible : Should contain entries for this and backward compatible
> + SSS versions:
> + - "samsung,s5pv210-secss" for S5PV210 SoC.
> +- reg : Offset and length of the register set for the module
> +- interrupts : the interrupt-specifier for the SSS module.
> + Two interrupts "feed control and hash" in case of S5PV210
> + Eg : interrupts = <0 feed-control 0> <0 hash 0>;
Please rewrite the description of interrupts property sa follows:
- interrupts : interrupt specifiers of SSS module interrupts, should
contain two entries:
- first : feed control interrupt,
- second : hash interrupt.
Then in later patch adding support for Exynos, it shoudl be rewritten to:
- interrupts : interrupt specifiers of SSS module interrupts, should
contain following entries:
- first : feed control interrupt (required for all variants),
- second : hash interrupt (required only for samsung,s5pv210-secss).
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v2 2/4] MFD: TPS65218: Add driver for the TPS65218 PMIC
From: Lee Jones @ 2014-02-06 14:37 UTC (permalink / raw)
To: Keerthy
Cc: rob.herring, pawel.moll, mark.rutland, swarren, ijc+devicetree,
rob, sameo, grant.likely, lgirdwood, broonie, devicetree,
linux-doc, linux-kernel, linux-omap
In-Reply-To: <1391665814-18814-3-git-send-email-j-keerthy@ti.com>
> The TPS65218 chip is a power management IC for Portable Navigation Systems
> and Tablet Computing devices. It contains the following components:
>
> - Regulators.
> - Over Temperature warning and Shut down.
>
> This patch adds support for tps65218 mfd device. At this time only
> the regulator functionality is made available.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
> drivers/mfd/Kconfig | 15 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/tps65218.c | 281 +++++++++++++++++++++++++++++++++++++++++
How much different is the TPS65218 to all the other TPS drivers?
drivers/mfd/tps6105x.c
drivers/mfd/tps65010.c
drivers/mfd/tps6507x.c
drivers/mfd/tps65090.c
drivers/mfd/tps65217.c
drivers/mfd/tps6586x.c
drivers/mfd/tps65910.c
drivers/mfd/tps65911-comparator.c
drivers/mfd/tps65912-core.c
drivers/mfd/tps65912-i2c.c
drivers/mfd/tps65912-irq.c
drivers/mfd/tps65912-spi.c
drivers/mfd/tps80031.c
Perhaps some consolidating might be in order?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 3/9 v5] crypto:s5p-sss: Add support for SSS module on Exynos
From: Tomasz Figa @ 2014-02-06 14:39 UTC (permalink / raw)
To: Naveen Krishna Chatradhi, linux-crypto, linux-samsung-soc
Cc: linux-kernel, vzapolskiy, herbert, naveenkrishna.ch, cpgs,
devicetree, David S. Miller
In-Reply-To: <1390987294-18628-1-git-send-email-ch.naveen@samsung.com>
Hi Naveen,
On 29.01.2014 10:21, Naveen Krishna Chatradhi wrote:
> This patch adds new compatible and variant struct to support the SSS
> module on Exynos4 (Exynos4210), Exynos5 (Exynos5420 and Exynos5250)
> for which
> 1. AES register are at an offset of 0x200 and
> 2. hash interrupt is not available
>
> Signed-off-by: Naveen Krishna Ch <ch.naveen@samsung.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: David S. Miller <davem@davemloft.net>
> CC: Vladimir Zapolskiy <vzapolskiy@gmail.com>
> TO: <linux-crypto@vger.kernel.org>
> CC: <linux-samsung-soc@vger.kernel.org>
> ---
> Changes since v4:
> Fix rebase error because of the patch 2/9
>
> .../devicetree/bindings/crypto/samsung-sss.txt | 21 +++-
> drivers/crypto/s5p-sss.c | 107 +++++++++++++++-----
> 2 files changed, 103 insertions(+), 25 deletions(-)
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v2 06/15] dt: binding: add binding for ImgTec IR block
From: James Hogan @ 2014-02-06 14:41 UTC (permalink / raw)
To: Rob Herring
Cc: Mauro Carvalho Chehab, linux-media, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala,
devicetree@vger.kernel.org, Rob Landley,
linux-doc@vger.kernel.org, Tomasz Figa
In-Reply-To: <CAL_Jsq+wk6_9Da5Xj3Ys-MZYPTpu6V3pAEpGFv44148BodmmrQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]
Hi Rob,
On 06/02/14 14:33, Rob Herring wrote:
> On Fri, Jan 17, 2014 at 7:58 AM, James Hogan <james.hogan@imgtec.com> wrote:
>> +Required properties:
>> +- compatible: Should be "img,ir1"
>
> Kind of short for a name. I don't have anything much better, but how
> about img,ir-rev1.
Okay, that sounds reasonable.
>> +Optional properties:
>> +- clocks: List of clock specifiers as described in standard
>> + clock bindings.
>> +- clock-names: List of clock names corresponding to the clocks
>> + specified in the clocks property.
>> + Accepted clock names are:
>> + "core": Core clock (defaults to 32.768KHz if omitted).
>> + "sys": System side (fast) clock.
>> + "mod": Power modulation clock.
>
> You need to define the order of clocks including how they are
> interpreted with different number of clocks (not relying on the name).
Would it be sufficient to specify that "clock-names" is required if
"clocks" is provided (i.e. unnamed clocks aren't used), or is there some
other reason that clock-names shouldn't be relied upon?
Thanks for reviewing,
Cheers
James
> Although, if the h/w block really has different number of clock
> inputs, then it is a different h/w block and should have a different
> compatible string.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ 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