* Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support
From: Tero Kristo @ 2016-12-13 9:01 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-clk-u79uwXL29TY76Z2rM5mHXA,
mturquette-rdvid1DuHRBWk0Htik3J/w,
ssantosh-DgEjT+Ai2ygdnm+yROfE0A, nm-l0cyMroinI0,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161212193800.GL5423-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
On 12/12/16 21:38, Stephen Boyd wrote:
> On 12/09, Tero Kristo wrote:
>> On 08/12/16 23:10, Stephen Boyd wrote:
>>> On 12/08, Tero Kristo wrote:
>>>> On 08/12/16 02:13, Stephen Boyd wrote:
>>>>> On 10/21, Tero Kristo wrote:
>>>>>> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
>>>>>> new file mode 100644
>>>>>> index 0000000..f6af5bd
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/clk/keystone/sci-clk.c
>>>>
>>>>>
>>>>>> +
>>>>>> + handle = devm_ti_sci_get_handle(dev);
>>>>>> + if (IS_ERR(handle))
>>>>>> + return PTR_ERR(handle);
>>>>>> +
>>>>>> + provider = devm_kzalloc(dev, sizeof(*provider), GFP_KERNEL);
>>>>>> + if (!provider)
>>>>>> + return -ENOMEM;
>>>>>> +
>>>>>> + provider->clocks = data;
>>>>>> +
>>>>>> + provider->sci = handle;
>>>>>> + provider->ops = &handle->ops.clk_ops;
>>>>>> + provider->dev = dev;
>>>>>> +
>>>>>> + ti_sci_init_clocks(provider);
>>>>>
>>>>> And if this fails?
>>>>
>>>> Yea this is kind of controversial. ti_sci_init_clocks() can fail if
>>>> any of the clocks registered will fail. I decided to have it this
>>>> way so that at least some clocks might work in failure cause, and
>>>> you might have a booting device instead of total lock-up.
>>>>
>>>> Obviously it could be done so that if any clock fails, we would
>>>> de-register all clocks at that point, but personally I think this is
>>>> a worse option.
>>>>
>>>> ti_sci_init_clocks could probably be modified to continue
>>>> registering clocks when a single clock fails though. Currently it
>>>> aborts at first failure.
>>>>
>>>
>>> That sounds like a better approach if we don't care about
>>> failures to register a clock. Returning a value from a function
>>> and not using it isn't really a great design.
>>>
>>> I worry that if we start returning errors from clk_hw_register()
>>> that something will go wrong though, so really I don't know why
>>> we want to ignore errors at all. Just for debugging a boot hang?
>>> Can't we use early console to at least see that this driver is
>>> failing to probe and debug that way?
>>
>> Early console can be used to debug that, but it is kind of annoying
>> to recompile most of the kernel when you suddenly need to use it.
>
> I thought SERIAL_EARLYCON was selected by drivers that support
> it? So there shouldn't be any rebuilding required.
Actually you can probably ignore my comment, I was just speaking out of
OMAP experience where the clocks are initialized very early, but this
doesn't apply to keystone. Sci-clock is a proper driver now with proper
probe etc. in place so my comment here is invalid.
>
>>
>> How about modifying the ti_sci_init_clocks func to print an error
>> for each failed clock?
>
> Ok that's fine too. I'd prefer the function had a return type of
> void if we're not planning on using the return value, that's all.
Ok, I'll see which way I go in v2 of the series, but seems I can pick
either your original proposal or mine.
-Tero
--
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 v6 7/8] ARM: dts: stm32: add Timers driver for stm32f429 MCU
From: Benjamin Gaignard @ 2016-12-13 9:15 UTC (permalink / raw)
To: Rob Herring
Cc: Lee Jones, Mark Rutland, alexandre.torgue-qxv4g6HH51o,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Kernel Mailing List,
Thierry Reding, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
Jonathan Cameron, knaack.h-Mmb7MZpHnFY, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Fabrice Gasnier, Gerald Baeza, Arnaud Pouliquen, Linus Walleij,
Linaro Kernel Mailman List, Benjamin Gaignard
In-Reply-To: <20161212185943.ph7njaqb2lxtgdn4@rob-hp-laptop>
2016-12-12 19:59 GMT+01:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> On Fri, Dec 09, 2016 at 03:15:18PM +0100, Benjamin Gaignard wrote:
>> Add Timers and it sub-nodes into DT for stm32f429 family.
>>
>> version 6:
>> - split patch in two: one for SoC family and one for stm32f469
>> discovery board.
>>
>> version 5:
>> - rename gptimer node to timers
>> - re-order timers node par addresses
>>
>> version 4:
>> - remove unwanted indexing in pwm@ and timer@ node name
>> - use "reg" instead of additional parameters to set timer
>> configuration
>>
>> version 3:
>> - use "st,stm32-timer-trigger" in DT
>>
>> version 2:
>> - use parameters to describe hardware capabilities
>> - do not use references for pwm and iio timer subnodes
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
>> ---
>> arch/arm/boot/dts/stm32f429.dtsi | 275 +++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 275 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
>> index bca491d..d0fb9cc 100644
>> --- a/arch/arm/boot/dts/stm32f429.dtsi
>> +++ b/arch/arm/boot/dts/stm32f429.dtsi
>> @@ -355,6 +355,21 @@
>> slew-rate = <2>;
>> };
>> };
>> +
>> + pwm1_pins: pwm@1 {
>
> No reg prop, so should not have a unit-address. Given the names in the
> define below, seems like "timer1" would be appropriate.
>
>> + pins {
>> + pinmux = <STM32F429_PA8_FUNC_TIM1_CH1>,
>> + <STM32F429_PB13_FUNC_TIM1_CH1N>,
>> + <STM32F429_PB12_FUNC_TIM1_BKIN>;
>> + };
>> + };
>> +
>> + pwm3_pins: pwm@3 {
>> + pins {
>> + pinmux = <STM32F429_PB4_FUNC_TIM3_CH1>,
>> + <STM32F429_PB5_FUNC_TIM3_CH2>;
>> + };
>> + };
>> };
>>
>> rcc: rcc@40023810 {
>> @@ -426,6 +441,266 @@
>> interrupts = <80>;
>> clocks = <&rcc 0 38>;
>> };
>> +
>> + timers2: timers@40000000 {
>
> timer@...
>
> It may be more than just a timer, there's not a better generic name.
>
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40000000 0x400>;
>> + clocks = <&rcc 0 128>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <1>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers3: timers@40000400 {
>
> ditto
>
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40000400 0x400>;
>> + clocks = <&rcc 0 129>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <2>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers4: timers@40000800 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40000800 0x400>;
>> + clocks = <&rcc 0 130>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <3>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers5: timers@40000C00 {
>
> timer@...
>
> And use lowercase hex.
>
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40000C00 0x400>;
>
> ditto
>
>> + clocks = <&rcc 0 131>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <4>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers6: timers@40001000 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40001000 0x400>;
>> + clocks = <&rcc 0 132>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <5>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers7: timers@40001400 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40001400 0x400>;
>> + clocks = <&rcc 0 133>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <6>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers12: timers@40001800 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40001800 0x400>;
>> + clocks = <&rcc 0 134>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <9>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers13: timers@40001C00 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40001C00 0x400>;
>> + clocks = <&rcc 0 135>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers14: timers@40002000 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40002000 0x400>;
>> + clocks = <&rcc 0 136>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers1: timers@40010000 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40010000 0x400>;
>> + clocks = <&rcc 0 160>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <0>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers8: timers@40010400 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40010400 0x400>;
>> + clocks = <&rcc 0 161>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <7>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers9: timers@40014000 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40014000 0x400>;
>> + clocks = <&rcc 0 176>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <8>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers10: timers@40014400 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40014400 0x400>;
>> + clocks = <&rcc 0 177>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers11: timers@40014800 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40014800 0x400>;
>> + clocks = <&rcc 0 178>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> + };
>> };
>> };
>>
>> --
>> 1.9.1
>>
--
Benjamin Gaignard
Graphic Study Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy
From: Vivek Gautam @ 2016-12-13 9:18 UTC (permalink / raw)
To: Stephen Boyd
Cc: kishon, robh+dt, Mark Rutland, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Srinivas Kandagatla, linux-arm-msm
In-Reply-To: <20161128231925.GO6095@codeaurora.org>
Hi Stephen,
On Tue, Nov 29, 2016 at 4:49 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 11/22, Vivek Gautam wrote:
>> Qualcomm chipsets have QMP phy controller that provides
>> support to a number of controller, viz. PCIe, UFS, and USB.
>> Adding dt binding information for the same.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>>
>> Changes since v1:
>> - New patch, forked out of the original driver patch:
>> "phy: qcom-qmp: new qmp phy driver for qcom-chipsets"
>> - updated bindings to include mem resource as a list of
>> offset - length pair for serdes block and for each lane.
>> - added a new binding for 'lane-offsets' that contains offsets
>> to tx, rx and pcs blocks from each lane base address.
>>
>> .../devicetree/bindings/phy/qcom-qmp-phy.txt | 74 ++++++++++++++++++++++
>> 1 file changed, 74 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> new file mode 100644
>> index 0000000..ffb173b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> @@ -0,0 +1,74 @@
>> +Qualcomm QMP PHY
>> +----------------
>> +
>> +QMP phy controller supports physical layer functionality for a number of
>> +controllers on Qualcomm chipsets, such as, PCIe, UFS, and USB.
[...]
>> +Example:
>> + pcie_phy: pciephy@34000 {
>> + compatible = "qcom,msm8996-qmp-pcie-phy";
>> + reg = <0x034000 0x48f>,
>> + <0x035000 0x5bf>,
>> + <0x036000 0x5bf>,
>> + <0x037000 0x5bf>;
>> + /* tx, rx, pcs */
>> + lane-offsets = <0x0 0x200 0x400>;
>> + #phy-cells = <1>;
>> +
>> + clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
>> + <&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
>> + <&rpmcc MSM8996_RPM_SMD_LN_BB_CLK>,
>> + <&gcc GCC_PCIE_CLKREF_CLK>,
>> + <&gcc GCC_PCIE_0_PIPE_CLK>,
>> + <&gcc GCC_PCIE_1_PIPE_CLK>,
>> + <&gcc GCC_PCIE_2_PIPE_CLK>;
>> + clock-names = "aux", "cfg_ahb",
>> + "ref_clk_src", "ref_clk",
>
> Does MSM8996_RPM_SMD_LN_BB_CLK supply the clock source for
> GCC_PCIE_CLKREF_CLK? Did we mess up the parent/child relationship
> in the GCC driver? We may want to fix that so that this node
> only references clocks that actually go into the device, instead
> of clock parents.
The clock documentations do show that the RPM_SMD_LN_BB_CLK provides
the 19.2 MHz phy clocks via pad. So, like you rightly said we may have to
fix the parents for phy reference clocks for difference phy controllers on 8996.
I will gather some more info on this to discuss it further.
Thanks
Vivek
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v6 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2016-12-13 9:20 UTC (permalink / raw)
To: M'boumba Cedric Madianga
Cc: wsa, robh+dt, mcoquelin.stm32, alexandre.torgue, linus.walleij,
patrice.chotard, linux, linux-i2c, devicetree, linux-arm-kernel,
linux-kernel
In-Reply-To: <1481559342-6106-3-git-send-email-cedric.madianga@gmail.com>
Hello,
On Mon, Dec 12, 2016 at 05:15:39PM +0100, M'boumba Cedric Madianga wrote:
> This patch adds support for the STM32F4 I2C controller.
>
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
> ---
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-stm32f4.c | 849 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 860 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-stm32f4.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 0cdc844..2719208 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -886,6 +886,16 @@ config I2C_ST
> This driver can also be built as module. If so, the module
> will be called i2c-st.
>
> +config I2C_STM32F4
> + tristate "STMicroelectronics STM32F4 I2C support"
> + depends on ARCH_STM32 || COMPILE_TEST
> + help
> + Enable this option to add support for STM32 I2C controller embedded
> + in STM32F4 SoCs.
> +
> + This driver can also be built as module. If so, the module
> + will be called i2c-stm32f4.
> +
> config I2C_STU300
> tristate "ST Microelectronics DDC I2C interface"
> depends on MACH_U300
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 1c1bac8..a2c6ff5 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_I2C_SH_MOBILE) += i2c-sh_mobile.o
> obj-$(CONFIG_I2C_SIMTEC) += i2c-simtec.o
> obj-$(CONFIG_I2C_SIRF) += i2c-sirf.o
> obj-$(CONFIG_I2C_ST) += i2c-st.o
> +obj-$(CONFIG_I2C_STM32F4) += i2c-stm32f4.o
> obj-$(CONFIG_I2C_STU300) += i2c-stu300.o
> obj-$(CONFIG_I2C_SUN6I_P2WI) += i2c-sun6i-p2wi.o
> obj-$(CONFIG_I2C_TEGRA) += i2c-tegra.o
> diff --git a/drivers/i2c/busses/i2c-stm32f4.c b/drivers/i2c/busses/i2c-stm32f4.c
> new file mode 100644
> index 0000000..89ad579
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-stm32f4.c
> @@ -0,0 +1,849 @@
> +/*
> + * Driver for STMicroelectronics STM32 I2C controller
> + *
> + * Copyright (C) M'boumba Cedric Madianga 2015
> + * Author: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
> + *
> + * This driver is based on i2c-st.c
> + *
> + * License terms: GNU General Public License (GPL), version 2
> + */
If there is a public description available for the device, a link here
would be great.
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +
> +/* STM32F4 I2C offset registers */
> +#define STM32F4_I2C_CR1 0x00
> +#define STM32F4_I2C_CR2 0x04
> +#define STM32F4_I2C_DR 0x10
> +#define STM32F4_I2C_SR1 0x14
> +#define STM32F4_I2C_SR2 0x18
> +#define STM32F4_I2C_CCR 0x1C
> +#define STM32F4_I2C_TRISE 0x20
> +#define STM32F4_I2C_FLTR 0x24
> +
> +/* STM32F4 I2C control 1*/
> +#define STM32F4_I2C_CR1_SWRST BIT(15)
> +#define STM32F4_I2C_CR1_POS BIT(11)
> +#define STM32F4_I2C_CR1_ACK BIT(10)
> +#define STM32F4_I2C_CR1_STOP BIT(9)
> +#define STM32F4_I2C_CR1_START BIT(8)
> +#define STM32F4_I2C_CR1_PE BIT(0)
> +
> +/* STM32F4 I2C control 2 */
> +#define STM32F4_I2C_CR2_FREQ_MASK GENMASK(5, 0)
> +#define STM32F4_I2C_CR2_FREQ(n) ((n & STM32F4_I2C_CR2_FREQ_MASK))
This should better be ((n) & STM32F4_I2C_CR2_FREQ_MASK). There a few
more constants that need the same fix.
> +#define STM32F4_I2C_CR2_ITBUFEN BIT(10)
> +#define STM32F4_I2C_CR2_ITEVTEN BIT(9)
> +#define STM32F4_I2C_CR2_ITERREN BIT(8)
> +#define STM32F4_I2C_CR2_IRQ_MASK (STM32F4_I2C_CR2_ITBUFEN \
> + | STM32F4_I2C_CR2_ITEVTEN \
> + | STM32F4_I2C_CR2_ITERREN)
I'd layout this like:
#define STM32F4_I2C_CR2_IRQ_MASK (STM32F4_I2C_CR2_ITBUFEN | \
STM32F4_I2C_CR2_ITEVTEN | \
STM32F4_I2C_CR2_ITERREN)
which is more usual I think.
> +/* STM32F4 I2C Status 1 */
> +#define STM32F4_I2C_SR1_AF BIT(10)
> +#define STM32F4_I2C_SR1_ARLO BIT(9)
> +#define STM32F4_I2C_SR1_BERR BIT(8)
> +#define STM32F4_I2C_SR1_TXE BIT(7)
> +#define STM32F4_I2C_SR1_RXNE BIT(6)
> +#define STM32F4_I2C_SR1_BTF BIT(2)
> +#define STM32F4_I2C_SR1_ADDR BIT(1)
> +#define STM32F4_I2C_SR1_SB BIT(0)
> +#define STM32F4_I2C_SR1_ITEVTEN_MASK (STM32F4_I2C_SR1_BTF \
> + | STM32F4_I2C_SR1_ADDR \
> + | STM32F4_I2C_SR1_SB)
> +#define STM32F4_I2C_SR1_ITBUFEN_MASK (STM32F4_I2C_SR1_TXE \
> + | STM32F4_I2C_SR1_RXNE)
> +#define STM32F4_I2C_SR1_ITERREN_MASK (STM32F4_I2C_SR1_AF \
> + | STM32F4_I2C_SR1_ARLO \
> + | STM32F4_I2C_SR1_BERR)
> +
> +/* STM32F4 I2C Status 2 */
> +#define STM32F4_I2C_SR2_BUSY BIT(1)
> +
> +/* STM32F4 I2C Control Clock */
> +#define STM32F4_I2C_CCR_CCR_MASK GENMASK(11, 0)
> +#define STM32F4_I2C_CCR_CCR(n) ((n & STM32F4_I2C_CCR_CCR_MASK))
> +#define STM32F4_I2C_CCR_FS BIT(15)
> +#define STM32F4_I2C_CCR_DUTY BIT(14)
> +
> +/* STM32F4 I2C Trise */
> +#define STM32F4_I2C_TRISE_VALUE_MASK GENMASK(5, 0)
> +#define STM32F4_I2C_TRISE_VALUE(n) ((n & STM32F4_I2C_TRISE_VALUE_MASK))
> +
> +/* STM32F4 I2C Filter */
> +#define STM32F4_I2C_FLTR_DNF_MASK GENMASK(3, 0)
> +#define STM32F4_I2C_FLTR_DNF(n) ((n & STM32F4_I2C_FLTR_DNF_MASK))
> +#define STM32F4_I2C_FLTR_ANOFF BIT(4)
> +
> +#define STM32F4_I2C_MIN_FREQ 2U
> +#define STM32F4_I2C_MAX_FREQ 42U
> +#define FAST_MODE_MAX_RISE_TIME 1000
> +#define STD_MODE_MAX_RISE_TIME 300
Are these supposed to be the values "rise time of both SDA and SCL
signals" from the i2c specification? If so, you got it wrong, fast mode
has the smaller value.
Maybe these constants could get a home in a more central place?
Also I'd add /* ns */ to the definition.
> +#define MHZ_TO_HZ 1000000
> +
> +enum stm32f4_i2c_speed {
> + STM32F4_I2C_SPEED_STANDARD, /* 100 kHz */
> + STM32F4_I2C_SPEED_FAST, /* 400 kHz */
> + STM32F4_I2C_SPEED_END,
> +};
> +
> +/**
> + * struct stm32f4_i2c_timings - per-Mode tuning parameters
> + * @duty: Fast mode duty cycle
> + * @mul_ccr: Value to be multiplied to CCR to reach 100Khz/400Khz SCL frequency
> + * @min_ccr: Minimum clock ctrl reg value to reach 100Khz/400Khz SCL frequency
> + */
> +struct stm32f4_i2c_timings {
> + u32 rate;
rate is undocumented and unused.
> + u32 duty;
> + u32 mul_ccr;
> + u32 min_ccr;
> +};
> +
> +/**
> + * struct stm32f4_i2c_msg - client specific data
> + * @addr: 8-bit slave addr, including r/w bit
> + * @count: number of bytes to be transferred
> + * @buf: data buffer
> + * @result: result of the transfer
> + * @stop: last I2C msg to be sent, i.e. STOP to be generated
> + */
> +struct stm32f4_i2c_msg {
> + u8 addr;
> + u32 count;
> + u8 *buf;
> + int result;
> + bool stop;
> +};
> +
> +/**
> + * struct stm32f4_i2c_dev - private data of the controller
> + * @adap: I2C adapter for this controller
> + * @dev: device for this controller
> + * @base: virtual memory area
> + * @complete: completion of I2C message
> + * @irq_event: interrupt event line for the controller
> + * @irq_error: interrupt error line for the controller
> + * @clk: hw i2c clock
> + * speed: I2C clock frequency of the controller. Standard or Fast only supported
> + * @msg: I2C transfer information
> + */
> +struct stm32f4_i2c_dev {
> + struct i2c_adapter adap;
> + struct device *dev;
> + void __iomem *base;
> + struct completion complete;
> + int irq_event;
> + int irq_error;
You only use irq_event in the probe function. So there is no need to
remember this one and you could use a local variable instead.
> + struct clk *clk;
> + int speed;
> + struct stm32f4_i2c_msg msg;
> +};
> +
> +static struct stm32f4_i2c_timings i2c_timings[] = {
> + [STM32F4_I2C_SPEED_STANDARD] = {
> + .mul_ccr = 1,
> + .min_ccr = 4,
> + .duty = 0,
> + },
> + [STM32F4_I2C_SPEED_FAST] = {
> + .mul_ccr = 16,
> + .min_ccr = 1,
> + .duty = 1,
> + },
Are these values from the datasheet?
> +};
> +
> +static inline void stm32f4_i2c_set_bits(void __iomem *reg, u32 mask)
> +{
> + writel_relaxed(readl_relaxed(reg) | mask, reg);
> +}
> +
> +static inline void stm32f4_i2c_clr_bits(void __iomem *reg, u32 mask)
> +{
> + writel_relaxed(readl_relaxed(reg) & ~mask, reg);
> +}
> +
> +static void stm32f4_i2c_soft_reset(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
> +
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_SWRST);
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_SWRST);
Not very critical, but you're doing an unneeded register access here
because the register is read twice.
Also I think readability would improve if you dropped
stm32f4_i2c_{set,clr}_bits and do their logic explicitly in the callers.
stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_SWRST);
stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_SWRST);
vs
val = readl_relaxed(reg);
writel_relaxed(val | STM32F4_I2C_CR1_SWRST, reg);
writel_relaxed(val, reg);
> +}
> +
> +static void stm32f4_i2c_disable_it(struct stm32f4_i2c_dev *i2c_dev)
What is "it"? If it stands for "interrupt" the more usual abbrev is
"irq".
> +{
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_IRQ_MASK);
> +}
> +
> +static void stm32f4_i2c_set_periph_clk_freq(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + u32 clk_rate, cr2, freq;
> +
> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> + cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
> + clk_rate = clk_get_rate(i2c_dev->clk);
> + freq = clk_rate / MHZ_TO_HZ;
> + freq = clamp(freq, STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ);
> + cr2 |= STM32F4_I2C_CR2_FREQ(freq);
> + writel_relaxed(cr2, i2c_dev->base + STM32F4_I2C_CR2);
Can you quote the data sheet enough in a comment here to make it obvious
that your calculation is right?
Would it be more sensible to error out if clk_rate / MHZ_TO_HZ isn't in
the interval [STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ]?
Usually I would expect that you need to use
DIV_ROUND_UP(clk_rate, MHZ_TO_HZ) instead of a plain division.
> +}
> +
> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + u32 trise, freq, cr2, val;
> +
> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> +
> + trise = readl_relaxed(i2c_dev->base + STM32F4_I2C_TRISE);
> + trise &= ~STM32F4_I2C_TRISE_VALUE_MASK;
Are you required to use rmw for STM32F4_I2C_TRISE? I'd prefer
writel_relaxed(STM32F4_I2C_TRISE_VALUE(..), i2c_dev->base + STM32F4_I2C_TRISE);
unless the datasheet requires rmw.
> + /* Maximum rise time computation */
> + if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD) {
> + trise |= STM32F4_I2C_TRISE_VALUE((freq + 1));
A single pair of parenthesis is enough when you fix
STM32F4_I2C_TRISE_VALUE as suggested above.
> + } else {
> + val = freq * FAST_MODE_MAX_RISE_TIME / STD_MODE_MAX_RISE_TIME;
> + trise |= STM32F4_I2C_TRISE_VALUE((val + 1));
val could be local to this branch.
Or make it shorter using:
freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
if (i2c_dev->speed == STM32F4_I2C_SPEED_FAST)
freq = freq * FAST_MODE_MAX_RISE_TIME / STD_MODE_MAX_RISE_TIME;
writel_relaxed(STM32F4_I2C_TRISE_VALUE(freq + 1), ...);
A quote from the data sheet about the algorithm would be good here, too.
> + }
> +
> + writel_relaxed(trise, i2c_dev->base + STM32F4_I2C_TRISE);
> +}
> +
> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_timings *t = &i2c_timings[i2c_dev->speed];
> + u32 ccr, clk_rate;
> + int val;
> +
> + ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
> + ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
> + STM32F4_I2C_CCR_CCR_MASK);
> +
> + clk_rate = clk_get_rate(i2c_dev->clk);
> + val = clk_rate / MHZ_TO_HZ * t->mul_ccr;
Is the rounding done right? Again please describe the hardware in a
comment.
> + if (val < t->min_ccr)
> + val = t->min_ccr;
> + ccr |= STM32F4_I2C_CCR_CCR(val);
> +
> + if (t->duty)
> + ccr |= STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY;
> +
> + writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
> +}
> +[...]
> +
> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + u32 status;
> + int ret;
> +
> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> + status,
> + !(status & STM32F4_I2C_SR2_BUSY),
> + 10, 1000);
> + if (ret) {
> + dev_err(i2c_dev->dev, "bus not free\n");
> + ret = -EBUSY;
I'm not sure if "bus not free" deserves an error message. Wolfram?
> + }
> +
> + return ret;
> +}
> +
> +[...]
> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + u32 rbuf;
> +
> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
> + *msg->buf++ = (u8)rbuf & 0xff;
unneeded cast (or unneeded & 0xff).
> + msg->count--;
> +}
> +
> +[...]
> +/**
> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> + switch (msg->count) {
> + case 1:
> + stm32f4_i2c_disable_it(i2c_dev);
> + stm32f4_i2c_read_msg(i2c_dev);
> + complete(&i2c_dev->complete);
> + break;
> + case 2:
> + case 3:
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> + break;
> + default:
> + stm32f4_i2c_read_msg(i2c_dev);
> + }
It looks wrong that you don't call stm32f4_i2c_read_msg if msg->count is
2 or 3. I guess that's because these cases are handled in
stm32f4_i2c_handle_rx_btf? Maybe you can simplify the logic a bit?
> +}
> +
> +/**
> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
> + * in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg;
> + u32 mask;
> + int i;
> +
> + switch (msg->count) {
I don't understand why the handling depends on the number of messages.
> + case 2:
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + /* Generate STOP or REPSTART */
I stumbled about "REPSTART" and would spell it out as "repeated Start".
> + if (msg->stop)
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> + else
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> +
> + /* Read two last data bytes */
> + for (i = 2; i > 0; i--)
> + stm32f4_i2c_read_msg(i2c_dev);
> +
> + /* Disable EVT and ERR interrupt */
> + reg = i2c_dev->base + STM32F4_I2C_CR2;
> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
> + stm32f4_i2c_clr_bits(reg, mask);
> +
> + complete(&i2c_dev->complete);
> + break;
> + case 3:
> + /* Enable ACK and read data */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> + stm32f4_i2c_read_msg(i2c_dev);
> + break;
> + default:
> + stm32f4_i2c_read_msg(i2c_dev);
> + }
> +}
> +
> +/**
> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
> + * master receiver
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg;
> +
> + switch (msg->count) {
> + case 0:
> + stm32f4_i2c_terminate_xfer(i2c_dev);
> + /* Clear ADDR flag */
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> + break;
> + case 1:
> + /*
> + * Single byte reception:
> + * Enable NACK, clear ADDR flag and generate STOP or RepSTART
> + */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> + if (msg->stop)
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> + else
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> + break;
> + case 2:
> + /*
> + * 2-byte reception:
> + * Enable NACK and PEC Position Ack and clear ADDR flag
What is PEC?
> + */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> + break;
> +
> + default:
> + /* N-byte reception: Enable ACK and clear ADDR flag */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> + break;
> + }
> +}
> +
> +/**
> + * stm32f4_i2c_isr_event() - Interrupt routine for I2C bus event
> + * @irq: interrupt number
> + * @data: Controller's private data
> + */
> +static irqreturn_t stm32f4_i2c_isr_event(int irq, void *data)
> +{
> +[...]
> + real_status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
s/real_status/status/ ?
> +
> + if (!(real_status & possible_status)) {
> + dev_dbg(i2c_dev->dev,
> + "spurious evt it (status=0x%08x, ien=0x%08x)\n",
> + real_status, ien);
s/it/irq/
> + return IRQ_NONE;
> + }
> +
> + /* Use __fls() to check error bits first */
> + flag = __fls(real_status & possible_status);
If you get several events reported you only handle a single one. Is this
effective?
> + switch (1 << flag) {
> + case STM32F4_I2C_SR1_SB:
> + stm32f4_i2c_write_byte(i2c_dev, msg->addr);
> + break;
> +
> + case STM32F4_I2C_SR1_ADDR:
> + if (msg->addr & I2C_M_RD)
> + stm32f4_i2c_handle_rx_addr(i2c_dev);
> + else
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> +
> + /* Enable ITBUF interrupts */
What is ITBUF?
> + reg = i2c_dev->base + STM32F4_I2C_CR2;
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> + break;
> +
> + case STM32F4_I2C_SR1_BTF:
> + if (msg->addr & I2C_M_RD)
> + stm32f4_i2c_handle_rx_btf(i2c_dev);
> + else
> + stm32f4_i2c_handle_write(i2c_dev);
> + break;
> +
> + case STM32F4_I2C_SR1_TXE:
> + stm32f4_i2c_handle_write(i2c_dev);
> + break;
> +
> + case STM32F4_I2C_SR1_RXNE:
> + stm32f4_i2c_handle_read(i2c_dev);
> + break;
> +
> + default:
> + dev_err(i2c_dev->dev,
> + "evt it unhandled: status=0x%08x)\n", real_status);
s/it/irq/
> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +[...]
> +static int stm32f4_i2c_xfer_msg(struct stm32f4_i2c_dev *i2c_dev,
> + struct i2c_msg *msg, bool is_first,
> + bool is_last)
> +{
> +[...]
> + /* Enable ITEVT and ITERR interrupts */
This comment isn't helpful. Mentioning their meaning would be great
instead.
> +[...]
> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
> + int num)
> +{
> + struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> + int ret, i;
> +
> + ret = clk_enable(i2c_dev->clk);
> + if (ret) {
> + dev_err(i2c_dev->dev, "Failed to enable clock\n");
> + return ret;
> + }
> +
> + stm32f4_i2c_hw_config(i2c_dev);
> +
> + for (i = 0; i < num && !ret; i++)
> + ret = stm32f4_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0,
> + i == num - 1);
> +
> + clk_disable(i2c_dev->clk);
> +
> + return (ret < 0) ? ret : i;
using num instead of i would be a bit more obvious.
> +static int stm32f4_i2c_probe(struct platform_device *pdev)
> +{
> +[...]
> + i2c_dev->speed = STM32F4_I2C_SPEED_STANDARD;
> + ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
> + if ((!ret) && (clk_rate == 400000))
> + i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
I'd use
if (!ret && clk_rate >= 400000)
i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
. That's less parenthesis and a more robust selection of the bus
frequency.
> +
> + i2c_dev->dev = &pdev->dev;
> +
> + ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_event,
> + NULL, stm32f4_i2c_isr_event,
> + IRQF_ONESHOT, pdev->name, i2c_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request irq %i\n",
> + i2c_dev->irq_error);
That's wrong. Requesting irq_event failed.
> + goto clk_free;
> + }
> +
> + ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_error,
> + NULL, stm32f4_i2c_isr_error,
> + IRQF_ONESHOT, pdev->name, i2c_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request irq %i\n",
> + i2c_dev->irq_error);
> + goto clk_free;
It would also be nice to know for which type of irq this failed. I.e.
please point out if this is the error irq or the event irq in the
message. Ditto for checking the return type of irq_of_parse_and_map.
> + }
> +
> + adap = &i2c_dev->adap;
> + i2c_set_adapdata(adap, i2c_dev);
> + snprintf(adap->name, sizeof(adap->name), "STM32 I2C(%pa)", &res->start);
> + adap->owner = THIS_MODULE;
> + adap->timeout = 2 * HZ;
> + adap->retries = 0;
> + adap->algo = &stm32f4_i2c_algo;
> + adap->dev.parent = &pdev->dev;
> + adap->dev.of_node = pdev->dev.of_node;
> +
> + init_completion(&i2c_dev->complete);
> +
> + ret = i2c_add_adapter(adap);
> + if (ret)
> + goto clk_free;
> +
> + platform_set_drvdata(pdev, i2c_dev);
> +
> + dev_info(i2c_dev->dev, "STM32F4 I2C driver initialized\n");
This is wrong. The driver is bound now to a device, not initialized.
> +static const struct of_device_id stm32f4_i2c_match[] = {
> + { .compatible = "st,stm32f4-i2c", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, stm32f4_i2c_match);
> +
> +static struct platform_driver stm32f4_i2c_driver = {
> + .driver = {
> + .name = "stm32f4-i2c",
> + .of_match_table = stm32f4_i2c_match,
Is this needed?
> + },
> + .probe = stm32f4_i2c_probe,
> + .remove = stm32f4_i2c_remove,
> +};
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v6 1/8] MFD: add bindings for STM32 Timers driver
From: Benjamin Gaignard @ 2016-12-13 9:29 UTC (permalink / raw)
To: Rob Herring
Cc: Lee Jones, Mark Rutland, alexandre.torgue-qxv4g6HH51o,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Kernel Mailing List,
Thierry Reding, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
Jonathan Cameron, knaack.h-Mmb7MZpHnFY, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Fabrice Gasnier, Gerald Baeza, Arnaud Pouliquen, Linus Walleij,
Linaro Kernel Mailman List, Benjamin Gaignard
In-Reply-To: <20161212185149.rt3xqpn3mbaavb4l@rob-hp-laptop>
2016-12-12 19:51 GMT+01:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> On Fri, Dec 09, 2016 at 03:15:12PM +0100, Benjamin Gaignard wrote:
>> Add bindings information for STM32 Timers
>>
>> version 6:
>> - rename stm32-gtimer to stm32-timers
>> - change compatible
>> - add description about the IPs
>>
>> version 2:
>> - rename stm32-mfd-timer to stm32-gptimer
>> - only keep one compatible string
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
>> ---
>> .../devicetree/bindings/mfd/stm32-timers.txt | 46 ++++++++++++++++++++++
>> 1 file changed, 46 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mfd/stm32-timers.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/stm32-timers.txt b/Documentation/devicetree/bindings/mfd/stm32-timers.txt
>> new file mode 100644
>> index 0000000..b30868e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/stm32-timers.txt
>> @@ -0,0 +1,46 @@
>> +STM32 Timers driver bindings
>> +
>> +This IP provides 3 types of timer along with PWM functionality:
>> +- advanced-control timers consist of a 16-bit auto-reload counter driven by a programmable
>> + prescaler, break input feature, PWM outputs and complementary PWM ouputs channels.
>> +- general-purpose timers consist of a 16-bit or 32-bit auto-reload counter driven by a
>> + programmable prescaler and PWM outputs.
>> +- basic timers consist of a 16-bit auto-reload counter driven by a programmable prescaler.
>> +
>> +Required parameters:
>> +- compatible: must be "st,stm32-timers"
>> +
>> +- reg: Physical base address and length of the controller's
>> + registers.
>> +- clock-names: Set to "clk_int".
>
> 'clk' is redundant. Also, you don't really need -names when there is
> only one of them.
I use devm_regmap_init_mmio_clk() which get the clock by it name so
I have to define it in DT.
>> +- clocks: Phandle to the clock used by the timer module.
>> + For Clk properties, please refer to ../clock/clock-bindings.txt
>> +
>> +Optional parameters:
>> +- resets: Phandle to the parent reset controller.
>> + See ../reset/st,stm32-rcc.txt
>> +
>> +Optional subnodes:
>> +- pwm: See ../pwm/pwm-stm32.txt
>> +- timer: See ../iio/timer/stm32-timer-trigger.txt
>> +
>> +Example:
>> + timers@40010000 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40010000 0x400>;
>> + clocks = <&rcc 0 160>;
>> + clock-names = "clk_int";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + pinctrl-0 = <&pwm1_pins>;
>> + pinctrl-names = "default";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <0>;
>
> You don't need reg here as there is only one. In turn, you don't need
> #address-cells or #size-cells.
I use "reg" to set each timer configuration.
>From hardware point of view they are all the same except for which hardware
signals they could consume and/or send.
"reg" is used as index of the two tables in driver code.
>
>> + };
>> + };
>> --
>> 1.9.1
>>
^ permalink raw reply
* Re: [PATCH v6 7/8] ARM: dts: stm32: add Timers driver for stm32f429 MCU
From: Benjamin Gaignard @ 2016-12-13 9:29 UTC (permalink / raw)
To: Rob Herring
Cc: Lee Jones, Mark Rutland, alexandre.torgue, devicetree,
Linux Kernel Mailing List, Thierry Reding, linux-pwm,
Jonathan Cameron, knaack.h, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, linux-arm-kernel,
Fabrice Gasnier, Gerald Baeza, Arnaud Pouliquen, Linus Walleij,
Linaro Kernel Mailman List, Benjamin Gaignard
In-Reply-To: <20161212185943.ph7njaqb2lxtgdn4@rob-hp-laptop>
2016-12-12 19:59 GMT+01:00 Rob Herring <robh@kernel.org>:
> On Fri, Dec 09, 2016 at 03:15:18PM +0100, Benjamin Gaignard wrote:
>> Add Timers and it sub-nodes into DT for stm32f429 family.
>>
>> version 6:
>> - split patch in two: one for SoC family and one for stm32f469
>> discovery board.
>>
>> version 5:
>> - rename gptimer node to timers
>> - re-order timers node par addresses
>>
>> version 4:
>> - remove unwanted indexing in pwm@ and timer@ node name
>> - use "reg" instead of additional parameters to set timer
>> configuration
>>
>> version 3:
>> - use "st,stm32-timer-trigger" in DT
>>
>> version 2:
>> - use parameters to describe hardware capabilities
>> - do not use references for pwm and iio timer subnodes
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>> arch/arm/boot/dts/stm32f429.dtsi | 275 +++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 275 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
>> index bca491d..d0fb9cc 100644
>> --- a/arch/arm/boot/dts/stm32f429.dtsi
>> +++ b/arch/arm/boot/dts/stm32f429.dtsi
>> @@ -355,6 +355,21 @@
>> slew-rate = <2>;
>> };
>> };
>> +
>> + pwm1_pins: pwm@1 {
>
> No reg prop, so should not have a unit-address. Given the names in the
> define below, seems like "timer1" would be appropriate.
>
Here pins muxing is only targeting PWM part of the the MFD , that why I have
labeled it with "pwm".
>> + pins {
>> + pinmux = <STM32F429_PA8_FUNC_TIM1_CH1>,
>> + <STM32F429_PB13_FUNC_TIM1_CH1N>,
>> + <STM32F429_PB12_FUNC_TIM1_BKIN>;
>> + };
>> + };
>> +
>> + pwm3_pins: pwm@3 {
>> + pins {
>> + pinmux = <STM32F429_PB4_FUNC_TIM3_CH1>,
>> + <STM32F429_PB5_FUNC_TIM3_CH2>;
>> + };
>> + };
>> };
>>
>> rcc: rcc@40023810 {
>> @@ -426,6 +441,266 @@
>> interrupts = <80>;
>> clocks = <&rcc 0 38>;
>> };
>> +
>> + timers2: timers@40000000 {
>
> timer@...
>
> It may be more than just a timer, there's not a better generic name.
"timer" is already used in DT for clocksource driver.
"timers" cover "advanced-control", "generic" and "basic" hardware timers IPs,
which share the same registers mapping (only the level of feature are different)
>
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40000000 0x400>;
>> + clocks = <&rcc 0 128>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <1>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers3: timers@40000400 {
>
> ditto
>
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40000400 0x400>;
>> + clocks = <&rcc 0 129>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <2>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers4: timers@40000800 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40000800 0x400>;
>> + clocks = <&rcc 0 130>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <3>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers5: timers@40000C00 {
>
> timer@...
>
> And use lowercase hex.
ok
>
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40000C00 0x400>;
>
> ditto
>
>> + clocks = <&rcc 0 131>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <4>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers6: timers@40001000 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40001000 0x400>;
>> + clocks = <&rcc 0 132>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <5>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers7: timers@40001400 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40001400 0x400>;
>> + clocks = <&rcc 0 133>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <6>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers12: timers@40001800 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40001800 0x400>;
>> + clocks = <&rcc 0 134>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <9>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers13: timers@40001C00 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40001C00 0x400>;
>> + clocks = <&rcc 0 135>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers14: timers@40002000 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40002000 0x400>;
>> + clocks = <&rcc 0 136>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers1: timers@40010000 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40010000 0x400>;
>> + clocks = <&rcc 0 160>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <0>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers8: timers@40010400 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40010400 0x400>;
>> + clocks = <&rcc 0 161>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <7>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers9: timers@40014000 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40014000 0x400>;
>> + clocks = <&rcc 0 176>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> +
>> + timer {
>> + compatible = "st,stm32-timer-trigger";
>> + reg = <8>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers10: timers@40014400 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40014400 0x400>;
>> + clocks = <&rcc 0 177>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> + };
>> +
>> + timers11: timers@40014800 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "st,stm32-timers";
>> + reg = <0x40014800 0x400>;
>> + clocks = <&rcc 0 178>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm {
>> + compatible = "st,stm32-pwm";
>> + status = "disabled";
>> + };
>> + };
>> };
>> };
>>
>> --
>> 1.9.1
>>
^ permalink raw reply
* Re: [PATCH v1 2/3] clk: rockchip: add clock controller for rk3328
From: Shawn Lin @ 2016-12-13 9:38 UTC (permalink / raw)
To: Elaine Zhang
Cc: heiko, mturquette, sboyd, xf, shawn.lin, mark.rutland, devicetree,
huangtao, xxx, linux-kernel, linux-rockchip, robh+dt, cl,
linux-clk, linux-arm-kernel
In-Reply-To: <1481618869-1239-3-git-send-email-zhangqing@rock-chips.com>
Hi, Elaine,
I always only keep an eye for mmc stuff here. :)
On 2016/12/13 16:47, Elaine Zhang wrote:
> Add the clock tree definition for the new rk3328 SoC.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
----8<--------------
> +
> + /* PD_MMC */
> + MMC(SCLK_SDMMC_DRV, "sdmmc_drv", "sclk_sdmmc",
> + RK3328_SDMMC_CON0, 1),
> + MMC(SCLK_SDMMC_SAMPLE, "sdmmc_sample", "sclk_sdmmc",
> + RK3328_SDMMC_CON1, 0),
> +
All of offset for these *_sample are wrong, and they should be 1
, the same as *_drv. You could refer to P565 of TRM instead the
section of CRU for these details.
> + MMC(SCLK_SDIO_DRV, "sdio_drv", "sclk_sdio",
> + RK3328_SDIO_CON0, 1),
> + MMC(SCLK_SDIO_SAMPLE, "sdio_sample", "sclk_sdio",
> + RK3328_SDIO_CON1, 0),
> +
> + MMC(SCLK_EMMC_DRV, "emmc_drv", "sclk_emmc",
> + RK3328_EMMC_CON0, 1),
> + MMC(SCLK_EMMC_SAMPLE, "emmc_sample", "sclk_emmc",
> + RK3328_EMMC_CON1, 0),
> +
> + MMC(SCLK_SDMMC_EXT_DRV, "sdmmc_ext_drv", "sclk_sdmmc_ext",
> + RK3328_SDMMC_EXT_CON0, 1),
> + MMC(SCLK_SDMMC_EXT_SAMPLE, "sdmmc_ext_sample", "sclk_sdmmc_ext",
> + RK3328_SDMMC_EXT_CON1, 0),
> +};
> +
----8<--------
> +#define RK3328_SDMMC_CON0 0x380
> +#define RK3328_SDMMC_CON1 0x384
> +#define RK3328_SDIO_CON0 0x388
> +#define RK3328_SDIO_CON1 0x38c
> +#define RK3328_EMMC_CON0 0x390
> +#define RK3328_EMMC_CON1 0x394
> +#define RK3328_SDMMC_EXT_CON0 0x398
> +#define RK3328_SDMMC_EXT_CON1 0x39C
Just wondering is it worth, but this uppercase 'C' isn't
consistent with the former lowercase
> +
> #define RK3368_PLL_CON(x) RK2928_PLL_CON(x)
> #define RK3368_CLKSEL_CON(x) ((x) * 0x4 + 0x100)
> #define RK3368_CLKGATE_CON(x) ((x) * 0x4 + 0x200)
> @@ -130,6 +152,7 @@
> enum rockchip_pll_type {
> pll_rk3036,
> pll_rk3066,
> + pll_rk3328,
> pll_rk3399,
> };
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply
* Re: [PATCH v6 3/5] ARM: dts: stm32: Add I2C1 support for STM32F429 SoC
From: Alexandre Torgue @ 2016-12-13 10:01 UTC (permalink / raw)
To: M'boumba Cedric Madianga, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A, patrice.chotard-qxv4g6HH51o,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1481559342-6106-4-git-send-email-cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Cedric,
On 12/12/2016 05:15 PM, M'boumba Cedric Madianga wrote:
> Signed-off-by: Patrice Chotard <patrice.chotard-qxv4g6HH51o@public.gmane.org>
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Please Add a commit message.
> ---
> arch/arm/boot/dts/stm32f429.dtsi | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
> index 7de52ee..cbdece7 100644
> --- a/arch/arm/boot/dts/stm32f429.dtsi
> +++ b/arch/arm/boot/dts/stm32f429.dtsi
> @@ -48,6 +48,7 @@
> #include "skeleton.dtsi"
> #include "armv7-m.dtsi"
> #include <dt-bindings/pinctrl/stm32f429-pinfunc.h>
> +#include <dt-bindings/mfd/stm32f4-rcc.h>
>
> / {
> clocks {
> @@ -337,6 +338,16 @@
> slew-rate = <2>;
> };
> };
> +
> + i2c1_pins_b: i2c1@0 {
> + pins1 {
> + pinmux = <STM32F429_PB9_FUNC_I2C1_SDA>;
> + drive-open-drain;
> + };
> + pins2 {
> + pinmux = <STM32F429_PB6_FUNC_I2C1_SCL>;
> + };
> + };
> };
>
> rcc: rcc@40023810 {
> @@ -409,6 +420,18 @@
> interrupts = <80>;
> clocks = <&rcc 0 38>;
> };
> +
> + i2c1: i2c@40005400 {
Can you check the order on device node please ? (should follow base@)
> + compatible = "st,stm32f4-i2c";
> + reg = <0x40005400 0x400>;
> + interrupts = <31>,
> + <32>;
> + resets = <&rcc STM32F4_APB1_RESET(I2C1)>;
> + clocks = <&rcc 0 STM32F4_APB1_CLOCK(I2C1)>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "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 v6 4/5] ARM: dts: stm32: Add I2C1 support for STM32429 eval board
From: Alexandre Torgue @ 2016-12-13 10:02 UTC (permalink / raw)
To: M'boumba Cedric Madianga, wsa, robh+dt, mcoquelin.stm32,
linus.walleij, patrice.chotard, linux, linux-i2c, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <1481559342-6106-5-git-send-email-cedric.madianga@gmail.com>
Hi Cedric,
On 12/12/2016 05:15 PM, M'boumba Cedric Madianga wrote:
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
Can you please add a commit message.
> ---
> arch/arm/boot/dts/stm32429i-eval.dts | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stm32429i-eval.dts b/arch/arm/boot/dts/stm32429i-eval.dts
> index afb90bc..74e0045 100644
> --- a/arch/arm/boot/dts/stm32429i-eval.dts
> +++ b/arch/arm/boot/dts/stm32429i-eval.dts
> @@ -141,3 +141,9 @@
> pinctrl-names = "default";
> status = "okay";
> };
> +
> +&i2c1 {
> + pinctrl-0 = <&i2c1_pins_b>;
> + pinctrl-names = "default";
> + status = "okay";
> +};
>
^ permalink raw reply
* Re: [PATCH v6 5/5] ARM: configs: stm32: Add I2C support for STM32 defconfig
From: Alexandre Torgue @ 2016-12-13 10:03 UTC (permalink / raw)
To: M'boumba Cedric Madianga, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A, patrice.chotard-qxv4g6HH51o,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1481559342-6106-6-git-send-email-cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Cedric,
On 12/12/2016 05:15 PM, M'boumba Cedric Madianga wrote:
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Can you please add a commit message.
Thx in advance
Alex
> ---
> arch/arm/configs/stm32_defconfig | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm/configs/stm32_defconfig b/arch/arm/configs/stm32_defconfig
> index e7b56d4..9494eaf 100644
> --- a/arch/arm/configs/stm32_defconfig
> +++ b/arch/arm/configs/stm32_defconfig
> @@ -52,6 +52,9 @@ CONFIG_SERIAL_NONSTANDARD=y
> CONFIG_SERIAL_STM32=y
> CONFIG_SERIAL_STM32_CONSOLE=y
> # CONFIG_HW_RANDOM is not set
> +CONFIG_I2C=y
> +CONFIG_I2C_CHARDEV=y
> +CONFIG_I2C_STM32F4=y
> # CONFIG_HWMON is not set
> # CONFIG_USB_SUPPORT is not set
> CONFIG_NEW_LEDS=y
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v7 0/5] ARM: dts: da850: tilcdc related DT changes
From: Bartosz Golaszewski @ 2016-12-13 10:09 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King,
Maxime Ripard
Cc: LKML, arm-soc, linux-drm, linux-devicetree, Bartosz Golaszewski
This series contains the last DT changes required for LCDC support
on da850-lcdk. The first one adds the dumb-vga-dac nodes, the second
limits the maximum pixel clock rate.
v1 -> v2:
- drop patch 3/3 (already merged)
- use max-pixelclock instead of max-bandwidth for display mode limiting
v2 -> v3:
- make the commit message in patch [2/2] more detailed
- move the max-pixelclock property to da850.dtsi as the limit
affects all da850-based boards
v3 -> v4:
- remove the input port from the display node
- move the display ports node to da850-lcdk.dts
- rename the vga_bridge node to vga-bridge
- move the LCDC pins to the LCDC node (from the vga bridge node)
v4 -> v5:
- rename the display label to lcdc
- instead of using the 'dumb-vga-dac' compatible, add bindings for
ti,ths8135 and use it as the vga-bridge node compatible
v5 -> v6:
- drop the endpoint numbers for nodes with single endpoints
- split patch 2/4 from v5 into two separate patches: one adding DT
bindings and one adding actual support for ths8135
v6 -> v7:
- simplified patch 4/5 by removing unnecessary properties from ports
and endpoints nodes
Bartosz Golaszewski (5):
ARM: dts: da850: rename the display node label
drm: bridge: add DT bindings for TI ths8135
drm: bridge: add support for TI ths8135
ARM: dts: da850-lcdk: add the vga-bridge node
ARM: dts: da850: specify the maximum pixel clock rate for tilcdc
.../bindings/display/bridge/ti,ths8135.txt | 46 +++++++++++++++++++
arch/arm/boot/dts/da850-lcdk.dts | 51 ++++++++++++++++++++++
arch/arm/boot/dts/da850.dtsi | 3 +-
drivers/gpu/drm/bridge/dumb-vga-dac.c | 1 +
4 files changed, 100 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
--
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
* [PATCH v7 1/5] ARM: dts: da850: rename the display node label
From: Bartosz Golaszewski @ 2016-12-13 10:09 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King,
Maxime Ripard
Cc: LKML, arm-soc, linux-drm, linux-devicetree, Bartosz Golaszewski
In-Reply-To: <1481623759-12786-1-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
The tilcdc node name is 'display' as per the ePAPR 1.1 recommendation.
The label is also 'display', but change it to 'lcdc' to make it clear
what the underlying hardware is.
Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
arch/arm/boot/dts/da850.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 104155d..6b0ef3d 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -458,7 +458,7 @@
dma-names = "tx", "rx";
};
- display: display@213000 {
+ lcdc: display@213000 {
compatible = "ti,da850-tilcdc";
reg = <0x213000 0x1000>;
interrupts = <52>;
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v7 2/5] drm: bridge: add DT bindings for TI ths8135
From: Bartosz Golaszewski @ 2016-12-13 10:09 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King,
Maxime Ripard
Cc: linux-devicetree, linux-drm, LKML, arm-soc, Bartosz Golaszewski
In-Reply-To: <1481623759-12786-1-git-send-email-bgolaszewski@baylibre.com>
THS8135 is a configurable video DAC. Add DT bindings for this chip.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Rob Herring <robh@kernel.org>
---
.../bindings/display/bridge/ti,ths8135.txt | 46 ++++++++++++++++++++++
1 file changed, 46 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt b/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
new file mode 100644
index 0000000..6ec1a88
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
@@ -0,0 +1,46 @@
+THS8135 Video DAC
+-----------------
+
+This is the binding for Texas Instruments THS8135 Video DAC bridge.
+
+Required properties:
+
+- compatible: Must be "ti,ths8135"
+
+Required nodes:
+
+This device has two video ports. Their connections are modelled using the OF
+graph bindings specified in Documentation/devicetree/bindings/graph.txt.
+
+- Video port 0 for RGB input
+- Video port 1 for VGA output
+
+Example
+-------
+
+vga-bridge {
+ compatible = "ti,ths8135";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ vga_bridge_in: endpoint {
+ remote-endpoint = <&lcdc_out_vga>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+
+ vga_bridge_out: endpoint {
+ remote-endpoint = <&vga_con_in>;
+ };
+ };
+ };
+};
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v7 3/5] drm: bridge: add support for TI ths8135
From: Bartosz Golaszewski @ 2016-12-13 10:09 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King,
Maxime Ripard
Cc: linux-devicetree, linux-drm, LKML, arm-soc, Bartosz Golaszewski
In-Reply-To: <1481623759-12786-1-git-send-email-bgolaszewski@baylibre.com>
THS8135 is a configurable video DAC, but no configuration is actually
necessary to make it work.
For now use the dumb-vga-dac driver to support it.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/gpu/drm/bridge/dumb-vga-dac.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index e570698..86e9f9c 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -237,6 +237,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
static const struct of_device_id dumb_vga_match[] = {
{ .compatible = "dumb-vga-dac" },
+ { .compatible = "ti,ths8135" },
{},
};
MODULE_DEVICE_TABLE(of, dumb_vga_match);
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v7 4/5] ARM: dts: da850-lcdk: add the vga-bridge node
From: Bartosz Golaszewski @ 2016-12-13 10:09 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King,
Maxime Ripard
Cc: linux-devicetree, linux-drm, LKML, arm-soc, Bartosz Golaszewski
In-Reply-To: <1481623759-12786-1-git-send-email-bgolaszewski@baylibre.com>
Add the vga-bridge node to the board DT together with corresponding
ports and vga connector. This allows to retrieve the edid info from
the display automatically.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
arch/arm/boot/dts/da850-lcdk.dts | 51 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
index afcb482..1ae2260 100644
--- a/arch/arm/boot/dts/da850-lcdk.dts
+++ b/arch/arm/boot/dts/da850-lcdk.dts
@@ -51,6 +51,45 @@
system-clock-frequency = <24576000>;
};
};
+
+ vga-bridge {
+ compatible = "ti,ths8135";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ vga_bridge_in: endpoint {
+ remote-endpoint = <&lcdc_out_vga>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+
+ vga_bridge_out: endpoint {
+ remote-endpoint = <&vga_con_in>;
+ };
+ };
+ };
+ };
+
+ vga {
+ compatible = "vga-connector";
+
+ ddc-i2c-bus = <&i2c0>;
+
+ port {
+ vga_con_in: endpoint {
+ remote-endpoint = <&vga_bridge_out>;
+ };
+ };
+ };
};
&pmx_core {
@@ -236,3 +275,15 @@
&memctrl {
status = "okay";
};
+
+&lcdc {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&lcd_pins>;
+
+ port {
+ lcdc_out_vga: endpoint {
+ remote-endpoint = <&vga_bridge_in>;
+ };
+ };
+};
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v7 5/5] ARM: dts: da850: specify the maximum pixel clock rate for tilcdc
From: Bartosz Golaszewski @ 2016-12-13 10:09 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King,
Maxime Ripard
Cc: linux-devicetree, linux-drm, LKML, arm-soc, Bartosz Golaszewski
In-Reply-To: <1481623759-12786-1-git-send-email-bgolaszewski@baylibre.com>
At maximum CPU frequency of 300 MHz the maximum pixel clock frequency
is 37.5 MHz[1]. We must filter out any mode for which the calculated
pixel clock rate would exceed this value.
Specify the max-pixelclock property for the display node for
da850-lcdk.
[1] http://processors.wiki.ti.com/index.php/OMAP-L1x/C674x/AM1x_LCD_Controller_(LCDC)_Throughput_and_Optimization_Techniques
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
arch/arm/boot/dts/da850.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 6b0ef3d..58b1566 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -462,6 +462,7 @@
compatible = "ti,da850-tilcdc";
reg = <0x213000 0x1000>;
interrupts = <52>;
+ max-pixelclock = <37500>;
status = "disabled";
};
};
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* Re: [PATCH v4 1/4] powernv:idle: Add IDLE_STATE_ENTER_SEQ_NORET macro
From: Balbir Singh @ 2016-12-13 10:13 UTC (permalink / raw)
To: Gautham R. Shenoy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Rafael J. Wysocki, Daniel Lezcano,
Michael Neuling, Vaidyanathan Srinivasan, Shreyas B. Prabhu,
Shilpasri G Bhat, Stewart Smith, Oliver O'Halloran
Cc: linuxppc-dev, linux-kernel, linux-pm, devicetree, Rob Herring,
Mark Rutland
In-Reply-To: <0947ced2f87c68a0a10ede8e1aaf39838a6f8d52.1481288905.git.ego@linux.vnet.ibm.com>
On 10/12/16 00:32, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>
> Currently all the low-power idle states are expected to wake up
> at reset vector 0x100. Which is why the macro IDLE_STATE_ENTER_SEQ
> that puts the CPU to an idle state and never returns.
>
> On ISA_300, when the ESL and EC bits in the PSSCR are zero, the
> CPU is expected to wake up at the next instruction of the idle
> instruction.
>
> This patch adds a new macro named IDLE_STATE_ENTER_SEQ_NORET for the
> no-return variant and reuses the name IDLE_STATE_ENTER_SEQ
> for a variant that allows resuming operation at the instruction next
> to the idle-instruction.
>
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/cpuidle.h | 5 ++++-
> arch/powerpc/kernel/exceptions-64s.S | 6 +++---
> arch/powerpc/kernel/idle_book3s.S | 10 +++++-----
> 3 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/cpuidle.h b/arch/powerpc/include/asm/cpuidle.h
> index 3919332..0a3255b 100644
> --- a/arch/powerpc/include/asm/cpuidle.h
> +++ b/arch/powerpc/include/asm/cpuidle.h
> @@ -21,7 +21,7 @@
>
> /* Idle state entry routines */
> #ifdef CONFIG_PPC_P7_NAP
> -#define IDLE_STATE_ENTER_SEQ(IDLE_INST) \
> +#define IDLE_STATE_ENTER_SEQ(IDLE_INST) \
> /* Magic NAP/SLEEP/WINKLE mode enter sequence */ \
> std r0,0(r1); \
> ptesync; \
> @@ -29,6 +29,9 @@
> 1: cmpd cr0,r0,r0; \
> bne 1b; \
> IDLE_INST; \
> +
Is the power saving magic sequence the same as before for power 9
as well?
Balbir Singh
^ permalink raw reply
* Re: [PATCH v6 4/5] ARM: dts: da850-lcdk: add the vga-bridge node
From: Bartosz Golaszewski @ 2016-12-13 10:18 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jyri Sarha, David Airlie, Kevin Hilman, Michael Turquette,
Sekhar Nori, Rob Herring, Frank Rowand, Mark Rutland,
Laurent Pinchart, Peter Ujfalusi, Russell King, LKML, arm-soc,
linux-drm, linux-devicetree
In-Reply-To: <9d5fa409-bb4f-f62b-2548-0a3b82228e08-l0cyMroinI0@public.gmane.org>
2016-12-13 9:46 GMT+01:00 Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>:
> Hi,
>
> On 12/12/16 15:05, Bartosz Golaszewski wrote:
>
>> +&lcdc {
>> + status = "okay";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&lcd_pins>;
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + lcdc_out: port@1 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <1>;
>> +
>> + lcdc_out_vga: endpoint {
>> + reg = <0>;
>> + remote-endpoint = <&vga_bridge_in>;
>> + };
>> + };
>> + };
>> +};
>>
>
> This is not correct. LCDC has just one output, so port@1 doesn't make
> sense. It's port@0. But with just one port, you can leave "ports" away.
> And you don't need the port's label for anything, if I'm not mistaken. So:
>
> &lcdc {
> status = "okay";
> pinctrl-names = "default";
> pinctrl-0 = <&lcd_pins>;
>
> port {
> lcdc_out_vga: endpoint {
> remote-endpoint = <&vga_bridge_in>;
> };
> };
> };
>
> Tomi
>
Right, fixed in v7.
Thanks,
Bartosz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 0/2] power: supply: add sbs-charger driver
From: Nicolas Saenz Julienne @ 2016-12-13 10:41 UTC (permalink / raw)
To: sre, mark.rutland; +Cc: linux-kernel, devicetree, linux-pm
In-Reply-To: <1479990823-25841-1-git-send-email-nicolas.saenz@prodys.net>
On 24/11/16 13:33, Nicolas Saenz Julienne wrote:
> Hi,
>
> This series adds support for all SBS compatible battery chargers, as defined
> here: http://sbs-forum.org/specs/sbc110.pdf.
>
> The first patch changes the sbs-battery device name in order to be able to
> create a proper supplier/supplied relation between the two of them.
>
> The second introduces the driver.
>
> Regards,
> Nicolas
>
> changes since v2:
> - updated driver and dt-binding with Sebatian's comments
>
> changes since v1:
> - added dt bindings
> - updated driver with Sebastian's comments
> - s/Nicola/Nicolas/ in commits
>
> Nicolas Saenz Julienne (2):
> power: supply: add sbs-charger driver
> dt-bindings: power: add bindings for sbs-charger
>
> .../bindings/power/supply/sbs_sbs-charger.txt | 24 ++
> drivers/power/supply/Kconfig | 6 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/sbs-charger.c | 275 +++++++++++++++++++++
> 4 files changed, 306 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt
> create mode 100644 drivers/power/supply/sbs-charger.c
>
Hi,
any update?
Kind Regards,
Nicolas
^ permalink raw reply
* RE: [PATCH v4 3/5] i2c: designware: Add slave definitions
From: Luis de Oliveira @ 2016-12-13 10:50 UTC (permalink / raw)
To: Rob Herring, Luis de Oliveira
Cc: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org,
andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w@public.gmane.org,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w@public.gmane.org,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w@public.gmane.org
In-Reply-To: <CAL_JsqJwjX1Y=bDD-hKcXqa1aBKjVhXjEZm6j2tMAQszyv-QhA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
The controller for i2c-designware cannot be slave/master at the same time and it has to be enabled knowing beforehand if we want it to be slave or master by something outside of the controller itself.
I as looking and I see the use of this I2C_OWN_SLAVE_ADDRESS with the "linux,slave-24c02" slave interface but I am not seeing how it will help me identify a selected i2c-designware block as a "slave" device before instantiation. I'm sorry if I'm not understanding properly.
I use the "linux,slave-24c02" to instantiate the i2c-designware as a slave with an address so I can do write/read operations, it is how I tested it.
Luis
-----Original Message-----
From: Rob Herring [mailto:robh@kernel.org]
Sent: Monday, December 12, 2016 23:16
To: Luis de Oliveira <Luis.Oliveira@synopsys.com>
Cc: wsa@the-dreams.de; mark.rutland@arm.com; jarkko.nikula@linux.intel.com; andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Ramiro.Oliveira@synopsys.com; Joao.Pinto@synopsys.com; CARLOS.PALMINHA@synopsys.com
Subject: Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
On Mon, Dec 12, 2016 at 12:35 PM, Luis de Oliveira <Luis.Oliveira@synopsys.com> wrote:
> Hi all,
Please don't top post.
>
> The slave address could be set by the I2C slave backend so I can't use it to setup the controller.
> A boolean property was my initial approach then Andy and Wolfram Sang suggested the use of compatible strings and later It was suggested to use a property to select mode but I can do it again if it's the best way.
> Can you please tell me where should it be documented?
bindings/i2c/i2c.txt.
Actually, looking at this some more, we already have a way to describe the controller being a slave device with the I2C_OWN_SLAVE_ADDRESS flag in the reg property. We should just need a helper to read reg property of each child and check for the bit set.
Rob
^ permalink raw reply
* Re: [PATCH v3 6/6] mfd: dt: Move syscon bindings to syscon subdirectory
From: Lee Jones @ 2016-12-13 11:07 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Rob Herring, Mark Rutland, Linus Walleij, Corey Minyard,
Cédric Le Goater, Joel Stanley, devicetree, linux-arm-kernel,
linux-kernel
In-Reply-To: <1481604793.3112.32.camel@aj.id.au>
On Tue, 13 Dec 2016, Andrew Jeffery wrote:
> On Mon, 2016-12-12 at 09:39 -0600, Rob Herring wrote:
> > On Tue, Dec 06, 2016 at 01:53:21PM +1100, Andrew Jeffery wrote:
> > > The use of syscons is growing, lets collate them in their own part of
> > > the bindings tree.
> > >
> > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > ---
> > > Documentation/devicetree/bindings/mfd/{ => syscon}/aspeed-scu.txt | 0
> > > Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-gpbr.txt | 0
> > > Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-matrix.txt | 0
> > > Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-smc.txt | 0
> > > Documentation/devicetree/bindings/mfd/{ => syscon}/qcom,tcsr.txt | 0
> > > Documentation/devicetree/bindings/mfd/{ => syscon}/syscon.txt | 0
> > > .../devicetree/bindings/mfd/{ => syscon}/ti-keystone-devctrl.txt | 0
> > > 7 files changed, 0 insertions(+), 0 deletions(-)
> > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/aspeed-scu.txt (100%)
> > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-gpbr.txt (100%)
> > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-matrix.txt (100%)
> > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-smc.txt (100%)
> > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/qcom,tcsr.txt (100%)
> > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/syscon.txt (100%)
> > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/ti-keystone-devctrl.txt (100%)
> >
> > I'm not so sure this is the right direction. syscon usage is pretty much
> > spread throughout the tree.
>
> This patch was created based on my interpretation of Lee's feedback
> here:
>
> https://lkml.org/lkml/2016/11/18/650
>
> Lee's next email in the chain poked Arnd for an opinion, but Arnd
> didn't reply.
>
> I don't mind. I moved these bindings separately so we could just drop
> the patch if there was push-back. If we drop the whole idea I'll need
> to apply a small fix to patch 5/6 to avoid creating the syscon
> subdirectory.
The sub-directory is a good idea for drivers who are *solely* syscon
based.
--
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 v6 3/8] PWM: add pwm-stm32 DT bindings
From: Lee Jones @ 2016-12-13 11:11 UTC (permalink / raw)
To: Rob Herring
Cc: Benjamin Gaignard, mark.rutland-5wv7dgnIgG8,
alexandre.torgue-qxv4g6HH51o, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
linux-pwm-u79uwXL29TY76Z2rM5mHXA, jic23-DgEjT+Ai2ygdnm+yROfE0A,
knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
pmeerw-jW+XmwGofnusTnJN9+BGXg, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
fabrice.gasnier-qxv4g6HH51o, gerald.baeza-qxv4g6HH51o,
arnaud.pouliquen-qxv4g6HH51o,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw, Benjamin Gaignard
In-Reply-To: <20161212190205.b5t3x6i2o7igldlo@rob-hp-laptop>
On Mon, 12 Dec 2016, Rob Herring wrote:
> On Fri, Dec 09, 2016 at 03:15:14PM +0100, Benjamin Gaignard wrote:
> > Define bindings for pwm-stm32
> >
> > version 6:
> > - change st,breakinput parameter format to make it usuable on stm32f7 too.
> >
> > version 2:
> > - use parameters instead of compatible of handle the hardware configuration
> >
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
> > ---
> > .../devicetree/bindings/pwm/pwm-stm32.txt | 33 ++++++++++++++++++++++
> > 1 file changed, 33 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > new file mode 100644
> > index 0000000..866f222
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > @@ -0,0 +1,33 @@
> > +STMicroelectronics STM32 Timers PWM bindings
> > +
> > +Must be a sub-node of an STM32 Timers device tree node.
> > +See ../mfd/stm32-timers.txt for details about the parent node.
> > +
> > +Required parameters:
> > +- compatible: Must be "st,stm32-pwm".
> > +- pinctrl-names: Set to "default".
> > +- pinctrl-0: List of phandles pointing to pin configuration nodes for PWM module.
> > + For Pinctrl properties see ../pinctrl/pinctrl-bindings.txt
> > +
> > +Optional parameters:
> > +- st,breakinput: Arrays of three u32 <index level filter> to describe break input configurations.
> > + "index" indicates on which break input the configuration should be applied.
> > + "level" gives the active level (0=low or 1=high) for this configuration.
> > + "filter" gives the filtering value to be applied.
> > +
> > +Example:
> > + timers@40010000 {
>
> timer@...
No, it should be timers.
The 's' is intentional, since this parent (MFD) device houses 3
different types of timers. The "timer" node is a child of this one.
> With that,
>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
>
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "st,stm32-timers";
> > + reg = <0x40010000 0x400>;
> > + clocks = <&rcc 0 160>;
> > + clock-names = "clk_int";
> > +
> > + pwm {
> > + compatible = "st,stm32-pwm";
> > + pinctrl-0 = <&pwm1_pins>;
> > + pinctrl-names = "default";
> > + st,breakinput = <0 1 5>;
> > + };
> > + };
--
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 v4 2/4] cpuidle:powernv: Add helper function to populate powernv idle states.
From: Balbir Singh @ 2016-12-13 11:51 UTC (permalink / raw)
To: Gautham R. Shenoy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Rafael J. Wysocki, Daniel Lezcano,
Michael Neuling, Vaidyanathan Srinivasan, Shreyas B. Prabhu,
Shilpasri G Bhat, Stewart Smith, Oliver O'Halloran
Cc: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Mark Rutland
In-Reply-To: <36f9cd2d944772d8e414a8240f9ec36eaec65ebd.1481288905.git.ego-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
On 10/12/16 00:32, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" <ego-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>
> In the current code for powernv_add_idle_states, there is a lot of code
> duplication while initializing an idle state in powernv_states table.
>
> Add an inline helper function to populate the powernv_states[] table for
> a given idle state. Invoke this for populating the "Nap", "Fastsleep"
> and the stop states in powernv_add_idle_states.
>
> Signed-off-by: Gautham R. Shenoy <ego-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
> drivers/cpuidle/cpuidle-powernv.c | 85 ++++++++++++++++++++++-----------------
> include/linux/cpuidle.h | 1 +
> 2 files changed, 50 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> index 7fe442c..db18af1 100644
> --- a/drivers/cpuidle/cpuidle-powernv.c
> +++ b/drivers/cpuidle/cpuidle-powernv.c
> @@ -167,6 +167,24 @@ static int powernv_cpuidle_driver_init(void)
> return 0;
> }
>
> +static inline void add_powernv_state(int index, const char *name,
> + unsigned int flags,
> + int (*idle_fn)(struct cpuidle_device *,
> + struct cpuidle_driver *,
> + int),
> + unsigned int target_residency,
> + unsigned int exit_latency,
> + u64 psscr_val)
> +{
> + strlcpy(powernv_states[index].name, name, CPUIDLE_NAME_LEN);
> + strlcpy(powernv_states[index].desc, name, CPUIDLE_NAME_LEN);
Do name and desc ever diverge?
> + powernv_states[index].flags = flags;
> + powernv_states[index].target_residency = target_residency;
> + powernv_states[index].exit_latency = exit_latency;
> + powernv_states[index].enter = idle_fn;
Why not call it idle_fn instead of enter?
> + stop_psscr_table[index] = psscr_val;
> +}
> +
> static int powernv_add_idle_states(void)
> {
> struct device_node *power_mgt;
> @@ -236,6 +254,7 @@ static int powernv_add_idle_states(void)
> "ibm,cpu-idle-state-residency-ns", residency_ns, dt_idle_states);
>
> for (i = 0; i < dt_idle_states; i++) {
> + unsigned int exit_latency, target_residency;
> /*
> * If an idle state has exit latency beyond
> * POWERNV_THRESHOLD_LATENCY_NS then don't use it
> @@ -243,28 +262,33 @@ static int powernv_add_idle_states(void)
> */
> if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS)
Ideally this should be called POWERNV_MAX_THRESHOLD_LATENCY_NS then
> continue;
> + /*
> + * Firmware passes residency and latency values in ns.
> + * cpuidle expects it in us.
> + */
> + exit_latency = ((unsigned int)latency_ns[i]) / 1000;
> + if (!rc)
> + target_residency = residency_ns[i] / 1000;
> + else
> + target_residency = 0;
Where do we get rc from? what does target_residency = 0 mean?
Balbir Singh
--
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 0/7] ath9k: EEPROM swapping improvements
From: Valo, Kalle @ 2016-12-13 12:03 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: ath9k-devel,
linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
ath9k-devel-xDcbHBWguxHbcTqmT+pZeQ@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
arnd-r2nGTMty4D4@public.gmane.org,
chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org,
nbd-Vt+b4OUoWG0@public.gmane.org
In-Reply-To: <CAFBinCC6JWBhZwma=66fBi3_to2SaHOMNDQS23jHNhcc+RUcYQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> writes:
> Hello Kalle,
>
> On Fri, Nov 25, 2016 at 4:06 PM, Valo, Kalle <kvalo-A+ZNKFmMK5xy9aJCnZT0Uw@public.gmane.org> wrote:
>> Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>>
>>> Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> writes:
>>>
>>>> There are two types of swapping the EEPROM data in the ath9k driver.
>>>> Before this series one type of swapping could not be used without the
>>>> other.
>>>>
>>>> The first type of swapping looks at the "magic bytes" at the start of
>>>> the EEPROM data and performs swab16 on the EEPROM contents if needed.
>>>> The second type of swapping is EEPROM format specific and swaps
>>>> specific fields within the EEPROM itself (swab16, swab32 - depends on
>>>> the EEPROM format).
>>>>
>>>> With this series the second part now looks at the EEPMISC register
>>>> inside the EEPROM, which uses a bit to indicate if the EEPROM data
>>>> is Big Endian (this is also done by the FreeBSD kernel).
>>>> This has a nice advantage: currently there are some out-of-tree hacks
>>>> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
>>>> Big Endian system (= no swab16 is performed) but the EEPROM itself
>>>> indicates that it's data is Little Endian. Until now the out-of-tree
>>>> code simply did a swab16 before passing the data to ath9k, so ath9k
>>>> first did the swab16 - this also enabled the format specific swapping.
>>>> These out-of-tree hacks are still working with the new logic, but it
>>>> is recommended to remove them. This implementation is based on a
>>>> discussion with Arnd Bergmann who raised concerns about the
>>>> robustness and portability of the swapping logic in the original OF
>>>> support patch review, see [0].
>>>>
>>>> After a second round of patches (= v1 of this series) neither Arnd
>>>> Bergmann nor I were really happy with the complexity of the EEPROM
>>>> swapping logic. Based on a discussion (see [1] and [2]) we decided
>>>> that ath9k should use a defined format (specifying the endianness
>>>> of the data - I went with __le16 and __le32) when accessing the
>>>> EEPROM fields. A benefit of this is that we enable the EEPMISC based
>>>> swapping logic by default, just like the FreeBSD driver, see [3]. On
>>>> the devices which I have tested (see below) ath9k now works without
>>>> having to specify the "endian_check" field in ath9k_platform_data (or
>>>> a similar logic which could provide this via devicetree) as ath9k now
>>>> detects the endianness automatically. Only EEPROMs which are mangled
>>>> by some out-of-tree code still need the endian_check flag (or one can
>>>> simply remove that mangling from the out-of-tree code).
>>>>
>>>> Testing:
>>>> - tested by myself on AR9287 with Big Endian EEPROM
>>>> - tested by myself on AR9227 with Little Endian EEPROM
>>>> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>>>> which did not suffer from this whole problem)
>>>> - how do we proceed with testing? maybe we could keep this in a
>>>> feature-branch and add these patches to LEDE once we have an ACK to
>>>> get more people to test this
>>>>
>>>> This series depends on my other series (v7):
>>>> "add devicetree support to ath9k" - see [4]
>>>
>>> I think this looks pretty good. If there's a bug somewhere it should be
>>> quite easy to fix so I'm not that worried and would be willing to take
>>> these as soon as I have applied the dependency series. IIRC your
>>> devicetree patches will have at least one more review round so that will
>>> take some time still. In the meantime it would be great if LEDE folks
>>> could take a look at these and comment (or test).
>>
>> So are everyone happy with this? I haven't seen any comments. If I don't
>> here anything I'm planning to take these, most likely for 4.11.
>
> the patches have been in LEDE for almost two weeks now and I did not
> see any reports of ath9k breakage (footnote below).
>
> It seems that there are a few devices out there where the whole EEPROM
> is swab16'ed which switches the position of the 1-byte fields
> opCapFlags and eepMisc.
> those still work fine with the new code, however I had a second patch
> in LEDE [0] which results in ath9k_platform_data.endian_check NOT
> being set anymore.
> that endian_check flag was used before to swab16 the whole EEPROM, to
> correct the position of the 1-byte fields again.
> Currently we are fixing this in the firmware hotplug script: [1]
> This is definitely not a blocker for this series though (if we want to
> have a devicetree replacement for "ath9k_platform_data.endian_check"
> then I'd work on that within a separate series, but I somewhat
> consider these EEPROMs as "broken" so fixing them in
> userspace/firmware hotplug script is fine for me)
Sounds good to me, thanks for the thorough followup. I'm planning to
apply these any day.
--
Kalle Valo--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 6/6] mfd: dt: Move syscon bindings to syscon subdirectory
From: Andrew Jeffery @ 2016-12-13 12:05 UTC (permalink / raw)
To: Lee Jones
Cc: Mark Rutland, Corey Minyard, devicetree, Linus Walleij,
linux-kernel, Cédric Le Goater, linux-arm-kernel,
Joel Stanley
In-Reply-To: <20161213110710.GV3625@dell.home>
[-- Attachment #1.1: Type: text/plain, Size: 2716 bytes --]
On Tue, 2016-12-13 at 11:07 +0000, Lee Jones wrote:
> On Tue, 13 Dec 2016, Andrew Jeffery wrote:
>
> > On Mon, 2016-12-12 at 09:39 -0600, Rob Herring wrote:
> > > On Tue, Dec 06, 2016 at 01:53:21PM +1100, Andrew Jeffery wrote:
> > > > The use of syscons is growing, lets collate them in their own part of
> > > > the bindings tree.
> > > >
> > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > >
> > > > ---
> > > > Documentation/devicetree/bindings/mfd/{ => syscon}/aspeed-scu.txt | 0
> > > > Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-gpbr.txt | 0
> > > > Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-matrix.txt | 0
> > > > Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-smc.txt | 0
> > > > Documentation/devicetree/bindings/mfd/{ => syscon}/qcom,tcsr.txt | 0
> > > > Documentation/devicetree/bindings/mfd/{ => syscon}/syscon.txt | 0
> > > > .../devicetree/bindings/mfd/{ => syscon}/ti-keystone-devctrl.txt | 0
> > > > 7 files changed, 0 insertions(+), 0 deletions(-)
> > > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/aspeed-scu.txt (100%)
> > > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-gpbr.txt (100%)
> > > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-matrix.txt (100%)
> > > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/atmel-smc.txt (100%)
> > > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/qcom,tcsr.txt (100%)
> > > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/syscon.txt (100%)
> > > > rename Documentation/devicetree/bindings/mfd/{ => syscon}/ti-keystone-devctrl.txt (100%)
> > >
> > > I'm not so sure this is the right direction. syscon usage is pretty much
> > > spread throughout the tree.
> >
> > This patch was created based on my interpretation of Lee's feedback
> > here:
> >
> > https://lkml.org/lkml/2016/11/18/650
> >
> > Lee's next email in the chain poked Arnd for an opinion, but Arnd
> > didn't reply.
> >
> > I don't mind. I moved these bindings separately so we could just drop
> > the patch if there was push-back. If we drop the whole idea I'll need
> > to apply a small fix to patch 5/6 to avoid creating the syscon
> > subdirectory.
>
> The sub-directory is a good idea for drivers who are *solely* syscon
> based.
>
Yes, I wasn't saying otherwise, just commenting on my motivation and
approach.
As far as I can tell all of the bindings I move here describe solely
syscon-based devices.
Cheers,
Andrew
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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