* Re: [RFC] New Device Tree property - "bonding"
From: Rob Herring @ 2016-12-05 15:57 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Ramesh Shanmugasundaram, frowand.list@gmail.com,
mark.rutland@arm.com, pantelis.antoniou@konsulko.com,
Chris Paterson, Geert Uytterhoeven,
laurent.pinchart+renesas@ideasonboard.com,
devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org
In-Reply-To: <2054258.NRXTrTF0Rj@avalon>
On Mon, Dec 5, 2016 at 8:40 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Rob,
>
> On Monday 05 Dec 2016 08:18:34 Rob Herring wrote:
>> On Fri, Nov 25, 2016 at 10:55 AM, Ramesh Shanmugasundaram wrote:
>> > Hello DT maintainers,
>> >
>> > In one of the Renesas SoCs we have a device called DRIF (Digital Radio
>> > Interface) controller. A DRIF channel contains 4 external pins - SCK,
>> > SYNC, Data pins D0 & D1.
>> >
>> > Internally a DRIF channel is made up of two SPI slave devices (also called
>> > sub-channels here) that share common CLK & SYNC signals but have their
>> > own resource set. The DRIF channel can have either one of the sub-channel
>> > active at a time or both. When both sub-channels are active, they need to
>> > be managed together as one device as they share same CLK & SYNC. We plan
>> > to tie these two sub-channels together with a new property called
>> > "renesas,bonding".
>>
>> Is there no need to describe the master device? No GPIOs, regulators
>> or other sideband controls needed? If that's never needed (which seems
>> doubtful), then I would do something different here probably with the
>> master device as a child of one DRIF and then phandles to master from
>> the other DRIFs. Otherwise, this looks fine to me.
>
> Here's a bit of background.
>
> The DRIF is an SPI receiver. It has three input pins, a clock line, a data
> line and a sync signal. The device is designed to be connected to a variety of
> data sources, usually plain SPI (1 data line), IIS (1 data line) but also
> radio tuners that output I/Q data (http://www.ni.com/tutorial/4805/en/) over
> two data lines.
>
> In the case of IQ each data sample is split in two I and Q values (typically
> 16 to 20 bits each in this case), and the values are transmitted serially over
> one data line each. The synchronization and clock signals are common to both
> data lines. The DRIF is optimized for this use case as the DRIF instances in
> the SoC (each of them having independent clocks, interrupts and control
> registers) are grouped by two, and the two instances in a group handle a
> single data line each but share the same clock and sync input.
>
> On the software side we need to group the I and Q values, which are DMA'ed to
> memory by the two DRIF instances, and make them available to userspace. The
> V4L2 API used here in SDR (Software Defined Radio) mode supports such use
> cases and exposes a single device node to userspace that allows control of the
> two DRIF instances as a single device. To be able to implement this we need
> kernel code to be aware of DRIF groups and, while binding to the DRIF
> instances separately, expose only one V4L2 device to userspace for each group.
>
> There's no master or slave instance from a hardware point of view, but the two
> instances are not interchangeable as they carry separate information. They
> must thus be identified at the driver level.
By master, I meant the external master device that generates the IQ
data, not which of the internal DRIF blocks is a master of the other.
So back to my question, does the external master device need to be
described? I worry the answer now for a simple case is no, but then
later people are going to have cases needing to describe more. We need
to answer this question first before we can decide what this binding
should look like.
> Back to the DT bindings, the need to expose relationships between (mostly)
> independent devices is quite common nowadays. It has been solved in some cases
> by creating a separate DT node that does not correspond to any physical
> hardware and whose sole purpose is to contain phandles to devices that need to
> be grouped. Drivers then bind to the compatible string of that "virtual" DT
> node. The proposed bonding property is a different approach to solve a similar
> problem. Would it be worth it addressing the problem at a more general level
> and try to design a common solution ?
We already have somewhat standard ways of grouping, but they are per
type of device (display, sound card, etc.). I'm not sure we gain much
standardizing across these devices and to some extent that ship has
sailed. Even within display subsystems, I don't think there is one
style that fits all. Sometimes a parent subsystem node with children
makes sense for the h/w. Sometimes that doesn't make sense and we have
the virtual node with a "ports" property (like sun8i did). I've never
been too happy with that style largely just because it feels like
we're letting DRM define the binding. However, it's generally extra
data that an OS could just ignore. There have also been many display
bindings that started out with a virtual node and we got rid of it. So
it's not something I like to encourage and we need to be clear when it
is okay or not.
To state the problem more generally (at least when using OF graph), I
think the problem is simply how do we define and group multiple entry
points to an OF graph. Maybe these should be graph nodes themselves
rather than phandles to the nodes with the entry points of the graph.
Rob
^ permalink raw reply
* Re: [PATCH v3 7/7] ARM: dts: stm32: add stm32 general purpose timer driver in DT
From: Alexandre Torgue @ 2016-12-05 16:09 UTC (permalink / raw)
To: Lee Jones, Benjamin Gaignard
Cc: robh+dt, mark.rutland, devicetree, linux-kernel, thierry.reding,
linux-pwm, jic23, knaack.h, lars, pmeerw, linux-iio,
linux-arm-kernel, fabrice.gasnier, gerald.baeza, arnaud.pouliquen,
linus.walleij, linaro-kernel, Benjamin Gaignard
In-Reply-To: <20161202132251.GL2683@dell>
Hi,
On 12/02/2016 02:22 PM, Lee Jones wrote:
> On Fri, 02 Dec 2016, Benjamin Gaignard wrote:
>
>> Add general purpose timers and it sub-nodes into DT for stm32f4.
>> Define and enable pwm1 and pwm3 for stm32f469 discovery board
>>
>> 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 | 333 +++++++++++++++++++++++++++++++++-
>> arch/arm/boot/dts/stm32f469-disco.dts | 28 +++
>> 2 files changed, 360 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
>> index bca491d..8c50d03 100644
>> --- a/arch/arm/boot/dts/stm32f429.dtsi
>> +++ b/arch/arm/boot/dts/stm32f429.dtsi
>> @@ -48,7 +48,7 @@
>> #include "skeleton.dtsi"
>> #include "armv7-m.dtsi"
>> #include <dt-bindings/pinctrl/stm32f429-pinfunc.h>
>> -
>> +#include <dt-bindings/iio/timer/st,stm32-timer-triggers.h>
>> / {
>> clocks {
>> clk_hse: clk-hse {
>> @@ -355,6 +355,21 @@
>> slew-rate = <2>;
>> };
>> };
>> +
>> + pwm1_pins: pwm@1 {
>> + 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,322 @@
>> interrupts = <80>;
>> clocks = <&rcc 0 38>;
>> };
>> +
>> + gptimer1: gptimer1@40010000 {
>
> timer@xxxxxxx
>
> Node names should be generic and not numbered.
>
> I suggest that this isn't actually a timer either. Is contains a
> timer (and a PWM), but in it's completeness it is not a timer per
> say.
>
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40010000 0x400>;
>> + clocks = <&rcc 0 160>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm1@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <4>;
>> + st,breakinput;
>> + st,complementary;
>> + status = "disabled";
>> + };
>> +
>> + timer1@0 {
>> + compatible = "st,stm32-timer-trigger";
>> + interrupts = <27>;
>> + st,input-triggers-names = TIM5_TRGO,
>> + TIM2_TRGO,
>> + TIM4_TRGO,
>> + TIM3_TRGO;
>
> I'm still dubious with matching by strings.
>
> I'll take a look at the C code to see what the alternatives could be.
>
>> + st,output-triggers-names = TIM1_TRGO,
>> + TIM1_CH1,
>> + TIM1_CH2,
>> + TIM1_CH3,
>> + TIM1_CH4;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer2: gptimer2@40000000 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40000000 0x400>;
>> + clocks = <&rcc 0 128>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm2@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <4>;
>> + st,32bits-counter;
>> + status = "disabled";
>> + };
>> +
>> + timer2@0 {
>> + compatible = "st,stm32-timer-trigger";
>> + interrupts = <28>;
>> + st,input-triggers-names = TIM1_TRGO,
>> + TIM8_TRGO,
>> + TIM3_TRGO,
>> + TIM4_TRGO;
>> + st,output-triggers-names = TIM2_TRGO,
>> + TIM2_CH1,
>> + TIM2_CH2,
>> + TIM2_CH3,
>> + TIM2_CH4;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer3: gptimer3@40000400 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40000400 0x400>;
>> + clocks = <&rcc 0 129>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm3@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <4>;
>> + status = "disabled";
>> + };
>> +
>> + timer3@0 {
>> + compatible = "st,stm32-timer-trigger";
>> + interrupts = <29>;
>> + st,input-triggers-names = TIM1_TRGO,
>> + TIM8_TRGO,
>> + TIM5_TRGO,
>> + TIM4_TRGO;
>> + st,output-triggers-names = TIM3_TRGO,
>> + TIM3_CH1,
>> + TIM3_CH2,
>> + TIM3_CH3,
>> + TIM3_CH4;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer4: gptimer4@40000800 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40000800 0x400>;
>> + clocks = <&rcc 0 130>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm4@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <4>;
>> + status = "disabled";
>> + };
>> +
>> + timer4@0 {
>> + compatible = "st,stm32-timer-trigger";
>> + interrupts = <30>;
>> + st,input-triggers-names = TIM1_TRGO,
>> + TIM2_TRGO,
>> + TIM3_TRGO,
>> + TIM8_TRGO;
>> + st,output-triggers-names = TIM4_TRGO,
>> + TIM4_CH1,
>> + TIM4_CH2,
>> + TIM4_CH3,
>> + TIM4_CH4;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer5: gptimer5@40000C00 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40000C00 0x400>;
>> + clocks = <&rcc 0 131>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm5@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <4>;
>> + st,32bits-counter;
>> + status = "disabled";
>> + };
>> +
>> + timer5@0 {
>> + compatible = "st,stm32-timer-trigger";
>> + interrupts = <50>;
>> + st,input-triggers-names = TIM2_TRGO,
>> + TIM3_TRGO,
>> + TIM4_TRGO,
>> + TIM8_TRGO;
>> + st,output-triggers-names = TIM5_TRGO,
>> + TIM5_CH1,
>> + TIM5_CH2,
>> + TIM5_CH3,
>> + TIM5_CH4;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer6: gptimer6@40001000 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40001000 0x400>;
>> + clocks = <&rcc 0 132>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + timer6@0 {
>> + compatible = "st,stm32-timer-trigger";
>> + interrupts = <54>;
>> + st,output-triggers-names = TIM6_TRGO;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer7: gptimer7@40001400 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40001400 0x400>;
>> + clocks = <&rcc 0 133>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + timer7@0 {
>> + compatible = "st,stm32-timer-trigger";
>> + interrupts = <55>;
>> + st,output-triggers-names = TIM7_TRGO;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer8: gptimer8@40010400 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40010400 0x400>;
>> + clocks = <&rcc 0 161>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm8@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <4>;
>> + st,complementary;
>> + st,breakinput;
>> + status = "disabled";
>> + };
>> +
>> + timer8@0 {
>> + compatible = "st,stm32-timer-trigger";
>> + interrupts = <46>;
>> + st,input-triggers-names = TIM1_TRGO,
>> + TIM2_TRGO,
>> + TIM4_TRGO,
>> + TIM5_TRGO;
>> + st,output-triggers-names = TIM8_TRGO,
>> + TIM8_CH1,
>> + TIM8_CH2,
>> + TIM8_CH3,
>> + TIM8_CH4;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer9: gptimer9@40014000 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40014000 0x400>;
>> + clocks = <&rcc 0 176>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm9@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <2>;
>> + status = "disabled";
>> + };
>> +
>> + timer9@0 {
>> + compatible = "st,stm32-timer-trigger";
>> + interrupts = <24>;
>> + st,input-triggers-names = TIM2_TRGO,
>> + TIM3_TRGO;
>> + st,output-triggers-names = TIM9_TRGO,
>> + TIM9_CH1,
>> + TIM9_CH2;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer10: gptimer10@40014400 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40014400 0x400>;
>> + clocks = <&rcc 0 177>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm10@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <1>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer11: gptimer11@40014800 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40014800 0x400>;
>> + clocks = <&rcc 0 178>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm11@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <1>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer12: gptimer12@40001800 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40001800 0x400>;
>> + clocks = <&rcc 0 134>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm12@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <2>;
>> + status = "disabled";
>> + };
>> +
>> + timer12@0 {
>> + compatible = "st,stm32-timer-trigger";
>> + interrupts = <43>;
>> + st,input-triggers-names = TIM4_TRGO,
>> + TIM5_TRGO;
>> + st,output-triggers-names = TIM12_TRGO,
>> + TIM12_CH1,
>> + TIM12_CH2;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer13: gptimer13@40001C00 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40001C00 0x400>;
>> + clocks = <&rcc 0 135>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm13@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <1>;
>> + status = "disabled";
>> + };
>> + };
>> +
>> + gptimer14: gptimer14@40002000 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40002000 0x400>;
>> + clocks = <&rcc 0 136>;
>> + clock-names = "clk_int";
>> + status = "disabled";
>> +
>> + pwm14@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <1>;
>> + status = "disabled";
>> + };
>> + };
>> };
>> };
>>
>> diff --git a/arch/arm/boot/dts/stm32f469-disco.dts b/arch/arm/boot/dts/stm32f469-disco.dts
>> index 8a163d7..df4ca7e 100644
>> --- a/arch/arm/boot/dts/stm32f469-disco.dts
>> +++ b/arch/arm/boot/dts/stm32f469-disco.dts
>> @@ -81,3 +81,31 @@
>> &usart3 {
>> status = "okay";
>> };
>> +
>> +&gptimer1 {
>> + status = "okay";
>> +
>> + pwm1@0 {
>> + pinctrl-0 = <&pwm1_pins>;
>> + pinctrl-names = "default";
>> + status = "okay";
>> + };
>> +
>> + timer1@0 {
>> + status = "okay";
>> + };
>> +};
>
> This is a much *better* format than before.
>
> I still don't like the '&' syntax though.
Please keep "&" format to match with existing nodes.
>
>> +&gptimer3 {
>> + status = "okay";
>> +
>> + pwm3@0 {
>> + pinctrl-0 = <&pwm3_pins>;
>> + pinctrl-names = "default";
>> + status = "okay";
>> + };
>> +
>> + timer3@0 {
>> + status = "okay";
>> + };
>> +};
>
^ permalink raw reply
* Re: [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation
From: Marek Vasut @ 2016-12-05 16:11 UTC (permalink / raw)
To: Boris Brezillon
Cc: Punnaiah Choudary Kalluri, dwmw2-wEGCiKHe2LqWVfeAwA7xHQ,
computersforpeace-Re5JQEeQqe8AvxtiuMwx3w, richard-/L3Ra7n9ekc,
cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, michals-gjFFaj9aHVfQT0dZR+AlfA,
kalluripunnaiahchoudary-Re5JQEeQqe8AvxtiuMwx3w,
kpc528-Re5JQEeQqe8AvxtiuMwx3w, Punnaiah Choudary Kalluri
In-Reply-To: <20161205093630.650451d7@bbrezillon>
On 12/05/2016 09:36 AM, Boris Brezillon wrote:
> On Mon, 5 Dec 2016 05:25:54 +0100
> Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On 12/05/2016 05:11 AM, Punnaiah Choudary Kalluri wrote:
>>> This patch adds the dts binding document for arasan nand flash
>>> controller.
>>>
>>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> ---
>>> changes in v6:
>>> - Removed num-cs property
>>> - Separated nandchip from nand controller
>>> changes in v5:
>>> - None
>>> Changes in v4:
>>> - Added num-cs property
>>> - Added clock support
>>> Changes in v3:
>>> - None
>>> Changes in v2:
>>> - None
>>> ---
>>> .../devicetree/bindings/mtd/arasan_nfc.txt | 38 ++++++++++++++++++++++
>>> 1 file changed, 38 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/mtd/arasan_nfc.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/arasan_nfc.txt b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
>>> new file mode 100644
>>> index 0000000..dcbe7ad
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
>>> @@ -0,0 +1,38 @@
>>> +Arasan Nand Flash Controller with ONFI 3.1 support
>>
>> Arasan NAND Flash ...
>>
>>> +Required properties:
>>> +- compatible: Should be "arasan,nfc-v3p10"
>>
>> This v3p10 looks like version 3 patchlevel 10, but shouldn't we have
>> some fallback option which doesn't encode IP version in the compat
>> string ?
>
> Not necessarily. Usually you define a generic compatible when you have
> other reliable means to detect the IP version (a version register for
> example).
> If you can't detect that at runtime, then providing only specific
> compatible strings is a good solution to avoid breaking the DT ABI.
Can we detect it at runtime with this hardware ?
>>
>> Also, shouldn't quirks be handled by DT props instead of effectively
>> encoding them into the compatible string ?
>
> Well, from my experience, it's better to hide as much as possible
> behind the compatible. This way, if new quirks are needed for a
> specific revision, you can update the driver without having to change
> the DT.
That's a good point, yes.
--
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC] New Device Tree property - "bonding"
From: Laurent Pinchart @ 2016-12-05 16:20 UTC (permalink / raw)
To: Rob Herring
Cc: Ramesh Shanmugasundaram,
frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org,
Chris Paterson, Geert Uytterhoeven,
laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Maxime Ripard
In-Reply-To: <CAL_JsqJw-KVZswBruG42MUdkmVSEb0L8OWCXbid7b41Ft4EpPQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Rob,
On Monday 05 Dec 2016 09:57:32 Rob Herring wrote:
> On Mon, Dec 5, 2016 at 8:40 AM, Laurent Pinchart wrote:
> > On Monday 05 Dec 2016 08:18:34 Rob Herring wrote:
> >> On Fri, Nov 25, 2016 at 10:55 AM, Ramesh Shanmugasundaram wrote:
> >>> Hello DT maintainers,
> >>>
> >>> In one of the Renesas SoCs we have a device called DRIF (Digital Radio
> >>> Interface) controller. A DRIF channel contains 4 external pins - SCK,
> >>> SYNC, Data pins D0 & D1.
> >>>
> >>> Internally a DRIF channel is made up of two SPI slave devices (also
> >>> called sub-channels here) that share common CLK & SYNC signals but have
> >>> their own resource set. The DRIF channel can have either one of the
> >>> sub-channel active at a time or both. When both sub-channels are active,
> >>> they need to be managed together as one device as they share same CLK &
> >>> SYNC. We plan to tie these two sub-channels together with a new property
> >>> called "renesas,bonding".
> >>
> >> Is there no need to describe the master device? No GPIOs, regulators
> >> or other sideband controls needed? If that's never needed (which seems
> >> doubtful), then I would do something different here probably with the
> >> master device as a child of one DRIF and then phandles to master from
> >> the other DRIFs. Otherwise, this looks fine to me.
> >
> > Here's a bit of background.
> >
> > The DRIF is an SPI receiver. It has three input pins, a clock line, a data
> > line and a sync signal. The device is designed to be connected to a
> > variety of data sources, usually plain SPI (1 data line), IIS (1 data
> > line) but also radio tuners that output I/Q data
> > (http://www.ni.com/tutorial/4805/en/) over two data lines.
> >
> > In the case of IQ each data sample is split in two I and Q values
> > (typically 16 to 20 bits each in this case), and the values are
> > transmitted serially over one data line each. The synchronization and
> > clock signals are common to both data lines. The DRIF is optimized for
> > this use case as the DRIF instances in the SoC (each of them having
> > independent clocks, interrupts and control registers) are grouped by two,
> > and the two instances in a group handle a single data line each but share
> > the same clock and sync input.
> >
> > On the software side we need to group the I and Q values, which are DMA'ed
> > to memory by the two DRIF instances, and make them available to
> > userspace. The V4L2 API used here in SDR (Software Defined Radio) mode
> > supports such use cases and exposes a single device node to userspace
> > that allows control of the two DRIF instances as a single device. To be
> > able to implement this we need kernel code to be aware of DRIF groups
> > and, while binding to the DRIF instances separately, expose only one V4L2
> > device to userspace for each group.
> >
> > There's no master or slave instance from a hardware point of view, but the
> > two instances are not interchangeable as they carry separate information.
> > They must thus be identified at the driver level.
>
> By master, I meant the external master device that generates the IQ
> data, not which of the internal DRIF blocks is a master of the other.
> So back to my question, does the external master device need to be
> described? I worry the answer now for a simple case is no, but then
> later people are going to have cases needing to describe more. We need
> to answer this question first before we can decide what this binding
> should look like.
Oh yes the external device certainly needs to be described. As it is
controlled through a separate, general-purpose I2C or SPI controller, it
should be a child node of that controller. The DRIF handles the data interface
only, not the control interface of the external device.
> > Back to the DT bindings, the need to expose relationships between (mostly)
> > independent devices is quite common nowadays. It has been solved in some
> > cases by creating a separate DT node that does not correspond to any
> > physical hardware and whose sole purpose is to contain phandles to
> > devices that need to be grouped. Drivers then bind to the compatible
> > string of that "virtual" DT node. The proposed bonding property is a
> > different approach to solve a similar problem. Would it be worth it
> > addressing the problem at a more general level and try to design a common
> > solution ?
>
> We already have somewhat standard ways of grouping, but they are per
> type of device (display, sound card, etc.). I'm not sure we gain much
> standardizing across these devices and to some extent that ship has
> sailed. Even within display subsystems, I don't think there is one
> style that fits all. Sometimes a parent subsystem node with children
> makes sense for the h/w. Sometimes that doesn't make sense and we have
> the virtual node with a "ports" property (like sun8i did). I've never
> been too happy with that style largely just because it feels like
> we're letting DRM define the binding. However, it's generally extra
> data that an OS could just ignore. There have also been many display
> bindings that started out with a virtual node and we got rid of it. So
> it's not something I like to encourage and we need to be clear when it
> is okay or not.
>
> To state the problem more generally (at least when using OF graph), I
> think the problem is simply how do we define and group multiple entry
> points to an OF graph. Maybe these should be graph nodes themselves
> rather than phandles to the nodes with the entry points of the graph.
CC'ing Maxime Ripard for the sun8i side.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3] arm: dts: zynq: Add MicroZed board support
From: Jagan Teki @ 2016-12-05 16:28 UTC (permalink / raw)
To: Michal Simek
Cc: linux-arm-kernel@lists.infradead.org, devicetree,
linux-kernel@vger.kernel.org, Jagan Teki, Soren Brinkmann
In-Reply-To: <f0c33080-0811-a515-f897-e7f851b13b5e@xilinx.com>
On Mon, Sep 26, 2016 at 8:22 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 23.9.2016 11:48, Jagan Teki wrote:
>> From: Jagan Teki <jteki@openedev.com>
>>
>> Added basic dts support for MicroZed board.
>>
>> - UART
>> - SDHCI
>> - Ethernet
>>
>> Cc: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> Signed-off-by: Jagan Teki <jteki@openedev.com>
>> ---
>> Changes for v3:
>> - Add Xilinx copyright
>> Changes for v2:
>> - Add SDHCI
>> - Add Ethernet
>>
>> arch/arm/boot/dts/Makefile | 1 +
>> arch/arm/boot/dts/zynq-microzed.dts | 96 +++++++++++++++++++++++++++++++++++++
>> 2 files changed, 97 insertions(+)
>> create mode 100644 arch/arm/boot/dts/zynq-microzed.dts
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index faacd52..4d7b858 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -862,6 +862,7 @@ dtb-$(CONFIG_ARCH_VT8500) += \
>> wm8750-apc8750.dtb \
>> wm8850-w70v2.dtb
>> dtb-$(CONFIG_ARCH_ZYNQ) += \
>> + zynq-microzed.dtb \
>> zynq-parallella.dtb \
>> zynq-zc702.dtb \
>> zynq-zc706.dtb \
>> diff --git a/arch/arm/boot/dts/zynq-microzed.dts b/arch/arm/boot/dts/zynq-microzed.dts
>> new file mode 100644
>> index 0000000..b9376a4
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/zynq-microzed.dts
>> @@ -0,0 +1,96 @@
>> +/*
>> + * Copyright (C) 2011 - 2014 Xilinx
>> + * Copyright (C) 2016 Jagan Teki <jteki@openedev.com>
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +/dts-v1/;
>> +/include/ "zynq-7000.dtsi"
>> +
>> +/ {
>> + model = "Zynq MicroZED Development Board";
>> + compatible = "xlnx,zynq-microzed", "xlnx,zynq-7000";
>> +
>> + aliases {
>> + ethernet0 = &gem0;
>> + serial0 = &uart1;
>> + };
>> +
>> + memory {
>> + device_type = "memory";
>> + reg = <0x0 0x40000000>;
>> + };
>> +
>> + chosen {
>> + bootargs = "earlycon";
>> + stdout-path = "serial0:115200n8";
>> + };
>> +
>> + usb_phy0: phy0 {
>> + compatible = "usb-nop-xceiv";
>> + #phy-cells = <0>;
>> + };
>> +};
>> +
>> +&clkc {
>> + ps-clk-frequency = <33333333>;
>> +};
>> +
>> +&gem0 {
>> + status = "okay";
>> + phy-mode = "rgmii-id";
>> + phy-handle = <ðernet_phy>;
>> +
>> + ethernet_phy: ethernet-phy@0 {
>> + reg = <0>;
>> + };
>> +};
>> +
>> +&sdhci0 {
>> + status = "okay";
>> +};
>> +
>> +&uart1 {
>> + status = "okay";
>> +};
>> +
>> +&usb0 {
>> + status = "okay";
>> + dr_mode = "host";
>> + usb-phy = <&usb_phy0>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_usb0_default>;
>> +};
>> +
>> +&pinctrl0 {
>> + pinctrl_usb0_default: usb0-default {
>> + mux {
>> + groups = "usb0_0_grp";
>> + function = "usb0";
>> + };
>> +
>> + conf {
>> + groups = "usb0_0_grp";
>> + slew-rate = <0>;
>> + io-standard = <1>;
>> + };
>> +
>> + conf-rx {
>> + pins = "MIO29", "MIO31", "MIO36";
>> + bias-high-impedance;
>> + };
>> +
>> + conf-tx {
>> + pins = "MIO28", "MIO30", "MIO32", "MIO33", "MIO34",
>> + "MIO35", "MIO37", "MIO38", "MIO39";
>> + bias-disable;
>> + };
>> + };
>> +};
>>
>
> Applied.
Was it missed? I couldn't see it on the source for a while. any help?
thanks!
--
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.
^ permalink raw reply
* Re: [PATCH 3/8] rtc: add STM32 RTC driver
From: Mathieu Poirier @ 2016-12-05 16:32 UTC (permalink / raw)
To: Amelie DELAUNAY
Cc: a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org,
alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
Alexandre TORGUE, linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org,
rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Gabriel FERNANDEZ
In-Reply-To: <15bea9e9-adcc-edb0-1bd1-33d395c72eec-qxv4g6HH51o@public.gmane.org>
On Mon, Dec 05, 2016 at 10:43:14AM +0100, Amelie DELAUNAY wrote:
> Hi Mathieu,
>
> Thanks for reviewing
>
> On 12/02/2016 06:56 PM, Mathieu Poirier wrote:
> > On Fri, Dec 02, 2016 at 03:09:56PM +0100, Amelie Delaunay wrote:
> >> This patch adds support for the STM32 RTC.
> >
> > Hello Amelie,
> >
> >>
> >> Signed-off-by: Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org>
> >> ---
> >> drivers/rtc/Kconfig | 10 +
> >> drivers/rtc/Makefile | 1 +
> >> drivers/rtc/rtc-stm32.c | 777
> ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 788 insertions(+)
> >> create mode 100644 drivers/rtc/rtc-stm32.c
> >>
> >> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> >> index e859d14..dd8b218 100644
> >> --- a/drivers/rtc/Kconfig
> >> +++ b/drivers/rtc/Kconfig
> >> @@ -1706,6 +1706,16 @@ config RTC_DRV_PIC32
> >> This driver can also be built as a module. If so, the module
> >> will be called rtc-pic32
> >>
> >> +config RTC_DRV_STM32
> >> + tristate "STM32 On-Chip RTC"
> >> + depends on ARCH_STM32
> >> + help
> >> + If you say yes here you get support for the STM32 On-Chip
> >> + Real Time Clock.
> >> +
> >> + This driver can also be built as a module, if so, the module
> >> + will be called "rtc-stm32".
> >> +
> >> comment "HID Sensor RTC drivers"
> >>
> >> config RTC_DRV_HID_SENSOR_TIME
> >> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> >> index 1ac694a..87bd9cc 100644
> >> --- a/drivers/rtc/Makefile
> >> +++ b/drivers/rtc/Makefile
> >> @@ -144,6 +144,7 @@ obj-$(CONFIG_RTC_DRV_SNVS) += rtc-snvs.o
> >> obj-$(CONFIG_RTC_DRV_SPEAR) += rtc-spear.o
> >> obj-$(CONFIG_RTC_DRV_STARFIRE) += rtc-starfire.o
> >> obj-$(CONFIG_RTC_DRV_STK17TA8) += rtc-stk17ta8.o
> >> +obj-$(CONFIG_RTC_DRV_STM32) += rtc-stm32.o
> >> obj-$(CONFIG_RTC_DRV_STMP) += rtc-stmp3xxx.o
> >> obj-$(CONFIG_RTC_DRV_ST_LPC) += rtc-st-lpc.o
> >> obj-$(CONFIG_RTC_DRV_SUN4V) += rtc-sun4v.o
> >> diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
> >> new file mode 100644
> >> index 0000000..9e710ff
> >> --- /dev/null
> >> +++ b/drivers/rtc/rtc-stm32.c
> >> @@ -0,0 +1,777 @@
> >> +/*
> >> + * Copyright (C) Amelie Delaunay 2015
> >> + * Author: Amelie Delaunay <adelaunay.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> + * License terms: GNU General Public License (GPL), version 2
> >> + */
> >> +
> >> +#include <linux/bcd.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/init.h>
> >> +#include <linux/io.h>
> >> +#include <linux/iopoll.h>
> >> +#include <linux/ioport.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/mfd/syscon.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/rtc.h>
> >> +#include <linux/spinlock.h>
> >> +
> >> +#define DRIVER_NAME "stm32_rtc"
> >> +
> >> +/* STM32 RTC registers */
> >> +#define STM32_RTC_TR 0x00
> >> +#define STM32_RTC_DR 0x04
> >> +#define STM32_RTC_CR 0x08
> >> +#define STM32_RTC_ISR 0x0C
> >> +#define STM32_RTC_PRER 0x10
> >> +#define STM32_RTC_ALRMAR 0x1C
> >> +#define STM32_RTC_WPR 0x24
> >> +
> >> +/* STM32_RTC_TR bit fields */
> >> +#define STM32_RTC_TR_SEC_SHIFT 0
> >> +#define STM32_RTC_TR_SEC GENMASK(6, 0)
> >> +#define STM32_RTC_TR_MIN_SHIFT 8
> >> +#define STM32_RTC_TR_MIN GENMASK(14, 8)
> >> +#define STM32_RTC_TR_HOUR_SHIFT 16
> >> +#define STM32_RTC_TR_HOUR GENMASK(21, 16)
> >> +
> >> +/* STM32_RTC_DR bit fields */
> >> +#define STM32_RTC_DR_DATE_SHIFT 0
> >> +#define STM32_RTC_DR_DATE GENMASK(5, 0)
> >> +#define STM32_RTC_DR_MONTH_SHIFT 8
> >> +#define STM32_RTC_DR_MONTH GENMASK(11, 8)
> >> +#define STM32_RTC_DR_WDAY_SHIFT 13
> >> +#define STM32_RTC_DR_WDAY GENMASK(15, 13)
> >> +#define STM32_RTC_DR_YEAR_SHIFT 16
> >> +#define STM32_RTC_DR_YEAR GENMASK(23, 16)
> >> +
> >> +/* STM32_RTC_CR bit fields */
> >> +#define STM32_RTC_CR_FMT BIT(6)
> >> +#define STM32_RTC_CR_ALRAE BIT(8)
> >> +#define STM32_RTC_CR_ALRAIE BIT(12)
> >> +
> >> +/* STM32_RTC_ISR bit fields */
> >> +#define STM32_RTC_ISR_ALRAWF BIT(0)
> >> +#define STM32_RTC_ISR_INITS BIT(4)
> >> +#define STM32_RTC_ISR_RSF BIT(5)
> >> +#define STM32_RTC_ISR_INITF BIT(6)
> >> +#define STM32_RTC_ISR_INIT BIT(7)
> >> +#define STM32_RTC_ISR_ALRAF BIT(8)
> >> +
> >> +/* STM32_RTC_PRER bit fields */
> >> +#define STM32_RTC_PRER_PRED_S_SHIFT 0
> >> +#define STM32_RTC_PRER_PRED_S GENMASK(14, 0)
> >> +#define STM32_RTC_PRER_PRED_A_SHIFT 16
> >> +#define STM32_RTC_PRER_PRED_A GENMASK(22, 16)
> >> +
> >> +/* STM32_RTC_ALRMAR and STM32_RTC_ALRMBR bit fields */
> >> +#define STM32_RTC_ALRMXR_SEC_SHIFT 0
> >> +#define STM32_RTC_ALRMXR_SEC GENMASK(6, 0)
> >> +#define STM32_RTC_ALRMXR_SEC_MASK BIT(7)
> >> +#define STM32_RTC_ALRMXR_MIN_SHIFT 8
> >> +#define STM32_RTC_ALRMXR_MIN GENMASK(14, 8)
> >> +#define STM32_RTC_ALRMXR_MIN_MASK BIT(15)
> >> +#define STM32_RTC_ALRMXR_HOUR_SHIFT 16
> >> +#define STM32_RTC_ALRMXR_HOUR GENMASK(21, 16)
> >> +#define STM32_RTC_ALRMXR_PM BIT(22)
> >> +#define STM32_RTC_ALRMXR_HOUR_MASK BIT(23)
> >> +#define STM32_RTC_ALRMXR_DATE_SHIFT 24
> >> +#define STM32_RTC_ALRMXR_DATE GENMASK(29, 24)
> >> +#define STM32_RTC_ALRMXR_WDSEL BIT(30)
> >> +#define STM32_RTC_ALRMXR_WDAY_SHIFT 24
> >> +#define STM32_RTC_ALRMXR_WDAY GENMASK(27, 24)
> >> +#define STM32_RTC_ALRMXR_DATE_MASK BIT(31)
> >> +
> >> +/* STM32_RTC_WPR key constants */
> >> +#define RTC_WPR_1ST_KEY 0xCA
> >> +#define RTC_WPR_2ND_KEY 0x53
> >> +#define RTC_WPR_WRONG_KEY 0xFF
> >> +
> >> +/*
> >> + * RTC registers are protected agains parasitic write access.
> >> + * PWR_CR_DBP bit must be set to enable write access to RTC registers.
> >> + */
> >> +/* STM32_PWR_CR */
> >> +#define PWR_CR 0x00
> >> +/* STM32_PWR_CR bit field */
> >> +#define PWR_CR_DBP BIT(8)
> >> +
> >> +static struct regmap *dbp;
> >> +
> >> +struct stm32_rtc {
> >> + struct rtc_device *rtc_dev;
> >> + void __iomem *base;
> >> + struct clk *pclk;
> >> + struct clk *ck_rtc;
> >> + unsigned int clksrc;
> >> + spinlock_t lock; /* Protects registers accesses */
> >> + int irq_alarm;
> >> + struct regmap *pwrcr;
> >> +};
> >> +
> >> +static inline unsigned int stm32_rtc_readl(struct stm32_rtc *rtc,
> >> + unsigned int offset)
> >> +{
> >> + return readl_relaxed(rtc->base + offset);
> >> +}
> >> +
> >> +static inline void stm32_rtc_writel(struct stm32_rtc *rtc,
> >> + unsigned int offset, unsigned int value)
> >> +{
> >> + writel_relaxed(value, rtc->base + offset);
> >> +}
> >
> > I'm not sure wrapping the readl/writel_relaxed function does anything
> special
> > other than simply redirecting the reader to another section of the code.
> During development phase, it is useful to add debug traces but you're right,
> this can be remove.
> >
> >> +
> >> +static void stm32_rtc_wpr_unlock(struct stm32_rtc *rtc)
> >> +{
> >> +// if (dbp)
> >> +// regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, PWR_CR_DBP);
> >
> > Did checkpatch let you get away with this? What did you intend to do
> here?
> Hum, as surprising as it may seem, checkpatch didn't complained about these
> comments! But anyway, this has to be removed, it was a tentative to
> enable/disable backup domain write protection any time we have to write in a
> protected RTC register, but it is not functionnal. I have commented this
> just to keep it in mind and forget to remove it before sending.
> >
> >> +
> >> + stm32_rtc_writel(rtc, STM32_RTC_WPR, RTC_WPR_1ST_KEY);
> >> + stm32_rtc_writel(rtc, STM32_RTC_WPR, RTC_WPR_2ND_KEY);
> >> +}
> >> +
> >> +static void stm32_rtc_wpr_lock(struct stm32_rtc *rtc)
> >> +{
> >> + stm32_rtc_writel(rtc, STM32_RTC_WPR, RTC_WPR_WRONG_KEY);
> >> +
> >> +// if (dbp)
> >> +// regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, ~PWR_CR_DBP);
> >> +}
> >> +
> >> +static int stm32_rtc_enter_init_mode(struct stm32_rtc *rtc)
> >> +{
> >> + unsigned int isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
> >> +
> >> + if (!(isr & STM32_RTC_ISR_INITF)) {
> >> + isr |= STM32_RTC_ISR_INIT;
> >> + stm32_rtc_writel(rtc, STM32_RTC_ISR, isr);
> >> +
> >> + return readl_relaxed_poll_timeout_atomic(
> >> + rtc->base + STM32_RTC_ISR,
> >> + isr, (isr & STM32_RTC_ISR_INITF),
> >> + 10, 100000);
> >
> > When using hard coded numerics please add comments that explains the
> reason
> > behind the selected values.
> Sure. It takes around 2 RTCCLK clock cycles to enter in initialization phase
> mode. So it depends on the frequency of the ck_rtc parent clock.
> Either I keep parent clock frequency and compute the exact timeout, or I use
> the "best and worst cases": slowest RTCCLK frequency is 32kHz, so it can
> take up to 62us, highest RTCCLK frequency should be 1MHz, so it can take
> only 2us. Polling every 10us with a timeout of 100ms seemed reasonable and
> be a good compromise.
I think this is a resonnable approach - please add that explanation as a comment
in the code.
> >
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void stm32_rtc_exit_init_mode(struct stm32_rtc *rtc)
> >> +{
> >> + unsigned int isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
> >> +
> >> + isr &= ~STM32_RTC_ISR_INIT;
> >> + stm32_rtc_writel(rtc, STM32_RTC_ISR, isr);
> >> +}
> >> +
> >> +static int stm32_rtc_wait_sync(struct stm32_rtc *rtc)
> >> +{
> >> + unsigned int isr;
> >> +
> >> + isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
> >> +
> >> + isr &= ~STM32_RTC_ISR_RSF;
> >> + stm32_rtc_writel(rtc, STM32_RTC_ISR, isr);
> >> +
> >> + /* Wait the registers to be synchronised */
> >> + return readl_relaxed_poll_timeout_atomic(rtc->base + STM32_RTC_ISR,
> >> + isr,
> >> + (isr & STM32_RTC_ISR_RSF),
> >> + 10, 100000);
> >
> > Shouldn't the break condition be !((isr & STM32_RTC_ISR_RSF) ? If not
> this
> > probably deserve a better comment.
> RSF bit is set by hardware each time the calendar registers are synchronized
> (it takes up to 2 RTCCLK). So the break condition is correct: we poll until
> RSF flag is set or timeout is reached.
> >
> >> +}
> >> +
> >> +static irqreturn_t stm32_rtc_alarm_irq(int irq, void *dev_id)
> >> +{
> >> + struct stm32_rtc *rtc = (struct stm32_rtc *)dev_id;
> >> + unsigned long irqflags, events = 0;
> >> + unsigned int isr, cr;
> >> +
> >> + spin_lock_irqsave(&rtc->lock, irqflags);
> >> +
> >> + isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
> >> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
> >> +
> >> + if ((isr & STM32_RTC_ISR_ALRAF) &&
> >> + (cr & STM32_RTC_CR_ALRAIE)) {
> >> + /* Alarm A flag - Alarm interrupt */
> >> + events |= RTC_IRQF | RTC_AF;
> >> + isr &= ~STM32_RTC_ISR_ALRAF;
> >> + }
> >> +
> >> + /* Clear event irqflags, otherwise new events won't be received */
> >> + stm32_rtc_writel(rtc, STM32_RTC_ISR, isr);
> >> +
> >> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> >> +
> >> + if (events) {
> >> + dev_info(&rtc->rtc_dev->dev, "Alarm occurred\n");
> >> +
> >> + /* Pass event to the kernel */
> >> + rtc_update_irq(rtc->rtc_dev, 1, events);
> >> + return IRQ_HANDLED;
> >> + } else {
> >> + return IRQ_NONE;
> >> + }
> >> +}
> >> +
> >> +/* Convert rtc_time structure from bin to bcd format */
> >> +static void tm2bcd(struct rtc_time *tm)
> >> +{
> >> + tm->tm_sec = bin2bcd(tm->tm_sec);
> >> + tm->tm_min = bin2bcd(tm->tm_min);
> >> + tm->tm_hour = bin2bcd(tm->tm_hour);
> >> +
> >> + tm->tm_mday = bin2bcd(tm->tm_mday);
> >> + tm->tm_mon = bin2bcd(tm->tm_mon + 1);
> >> + tm->tm_year = bin2bcd(tm->tm_year - 100);
> >> + /*
> >> + * Number of days since Sunday
> >> + * - on kernel side, 0=Sunday...6=Saturday
> >> + * - on rtc side, 0=invalid,1=Monday...7=Sunday
> >> + */
> >> + tm->tm_wday = (!tm->tm_wday) ? 7 : tm->tm_wday;
> >> +}
> >> +
> >> +/* Convert rtc_time structure from bcd to bin format */
> >> +static void bcd2tm(struct rtc_time *tm)
> >> +{
> >> + tm->tm_sec = bcd2bin(tm->tm_sec);
> >> + tm->tm_min = bcd2bin(tm->tm_min);
> >> + tm->tm_hour = bcd2bin(tm->tm_hour);
> >> +
> >> + tm->tm_mday = bcd2bin(tm->tm_mday);
> >> + tm->tm_mon = bcd2bin(tm->tm_mon) - 1;
> >> + tm->tm_year = bcd2bin(tm->tm_year) + 100;
> >> + /*
> >> + * Number of days since Sunday
> >> + * - on kernel side, 0=Sunday...6=Saturday
> >> + * - on rtc side, 0=invalid,1=Monday...7=Sunday
> >> + */
> >> + tm->tm_wday %= 7;
> >> +}
> >> +
> >> +static int stm32_rtc_read_time(struct device *dev, struct rtc_time *tm)
> >> +{
> >> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> >> + unsigned int tr, dr;
> >> + unsigned long irqflags;
> >> +
> >> + spin_lock_irqsave(&rtc->lock, irqflags);
> >> +
> >> + /* Time and Date in BCD format */
> >> + tr = stm32_rtc_readl(rtc, STM32_RTC_TR);
> >> + dr = stm32_rtc_readl(rtc, STM32_RTC_DR);
> >> +
> >> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> >> +
> >> + tm->tm_sec = (tr & STM32_RTC_TR_SEC) >> STM32_RTC_TR_SEC_SHIFT;
> >> + tm->tm_min = (tr & STM32_RTC_TR_MIN) >> STM32_RTC_TR_MIN_SHIFT;
> >> + tm->tm_hour = (tr & STM32_RTC_TR_HOUR) >> STM32_RTC_TR_HOUR_SHIFT;
> >> +
> >> + tm->tm_mday = (dr & STM32_RTC_DR_DATE) >> STM32_RTC_DR_DATE_SHIFT;
> >> + tm->tm_mon = (dr & STM32_RTC_DR_MONTH) >> STM32_RTC_DR_MONTH_SHIFT;
> >> + tm->tm_year = (dr & STM32_RTC_DR_YEAR) >> STM32_RTC_DR_YEAR_SHIFT;
> >> + tm->tm_wday = (dr & STM32_RTC_DR_WDAY) >> STM32_RTC_DR_WDAY_SHIFT;
> >> +
> >> + /* We don't report tm_yday and tm_isdst */
> >> +
> >> + bcd2tm(tm);
> >> +
> >> + if (rtc_valid_tm(tm) < 0) {
> >> + dev_err(dev, "%s: rtc_time is not valid.\n", __func__);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int stm32_rtc_set_time(struct device *dev, struct rtc_time *tm)
> >> +{
> >> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> >> + unsigned int tr, dr;
> >> + unsigned long irqflags;
> >> + int ret = 0;
> >> +
> >> + if (rtc_valid_tm(tm) < 0) {
> >> + dev_err(dev, "%s: rtc_time is not valid.\n", __func__);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + tm2bcd(tm);
> >> +
> >> + /* Time in BCD format */
> >> + tr = ((tm->tm_sec << STM32_RTC_TR_SEC_SHIFT) & STM32_RTC_TR_SEC) |
> >> + ((tm->tm_min << STM32_RTC_TR_MIN_SHIFT) & STM32_RTC_TR_MIN) |
> >> + ((tm->tm_hour << STM32_RTC_TR_HOUR_SHIFT) & STM32_RTC_TR_HOUR);
> >> +
> >> + /* Date in BCD format */
> >> + dr = ((tm->tm_mday << STM32_RTC_DR_DATE_SHIFT) & STM32_RTC_DR_DATE)
> |
> >> + ((tm->tm_mon << STM32_RTC_DR_MONTH_SHIFT) & STM32_RTC_DR_MONTH)
> |
> >> + ((tm->tm_year << STM32_RTC_DR_YEAR_SHIFT) & STM32_RTC_DR_YEAR)
> |
> >> + ((tm->tm_wday << STM32_RTC_DR_WDAY_SHIFT) & STM32_RTC_DR_WDAY);
> >> +
> >> + spin_lock_irqsave(&rtc->lock, irqflags);
> >> +
> >> + stm32_rtc_wpr_unlock(rtc);
> >> +
> >> + ret = stm32_rtc_enter_init_mode(rtc);
> >> + if (ret) {
> >> + dev_err(dev, "Can't enter in init mode. Set time aborted.\n");
> >> + goto end;
> >> + }
> >> +
> >> + stm32_rtc_writel(rtc, STM32_RTC_TR, tr);
> >> + stm32_rtc_writel(rtc, STM32_RTC_DR, dr);
> >> +
> >> + stm32_rtc_exit_init_mode(rtc);
> >> +
> >> + ret = stm32_rtc_wait_sync(rtc);
> >> +end:
> >> + stm32_rtc_wpr_lock(rtc);
> >> +
> >> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int stm32_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
> *alrm)
> >> +{
> >> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> >> + struct rtc_time *tm = &alrm->time;
> >> + unsigned int alrmar, cr, isr;
> >> + unsigned long irqflags;
> >> +
> >> + spin_lock_irqsave(&rtc->lock, irqflags);
> >> +
> >> + alrmar = stm32_rtc_readl(rtc, STM32_RTC_ALRMAR);
> >> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
> >> + isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
> >> +
> >> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> >> +
> >> + if (alrmar & STM32_RTC_ALRMXR_DATE_MASK) {
> >> + /*
> >> + * Date/day don't care in Alarm comparison so alarm triggers
> >> + * every day
> >> + */
> >> + tm->tm_mday = -1;
> >> + tm->tm_wday = -1;
> >> + } else {
> >> + if (alrmar & STM32_RTC_ALRMXR_WDSEL) {
> >> + /* Alarm is set to a day of week */
> >> + tm->tm_mday = -1;
> >> + tm->tm_wday = (alrmar & STM32_RTC_ALRMXR_WDAY) >>
> >> + STM32_RTC_ALRMXR_WDAY_SHIFT;
> >> + tm->tm_wday %= 7;
> >> + } else {
> >> + /* Alarm is set to a day of month */
> >> + tm->tm_wday = -1;
> >> + tm->tm_mday = (alrmar & STM32_RTC_ALRMXR_DATE) >>
> >> + STM32_RTC_ALRMXR_DATE_SHIFT;
> >> + }
> >> + }
> >> +
> >> + if (alrmar & STM32_RTC_ALRMXR_HOUR_MASK) {
> >> + /* Hours don't care in Alarm comparison */
> >> + tm->tm_hour = -1;
> >> + } else {
> >> + tm->tm_hour = (alrmar & STM32_RTC_ALRMXR_HOUR) >>
> >> + STM32_RTC_ALRMXR_HOUR_SHIFT;
> >> + if (alrmar & STM32_RTC_ALRMXR_PM)
> >> + tm->tm_hour += 12;
> >> + }
> >> +
> >> + if (alrmar & STM32_RTC_ALRMXR_MIN_MASK) {
> >> + /* Minutes don't care in Alarm comparison */
> >> + tm->tm_min = -1;
> >> + } else {
> >> + tm->tm_min = (alrmar & STM32_RTC_ALRMXR_MIN) >>
> >> + STM32_RTC_ALRMXR_MIN_SHIFT;
> >> + }
> >> +
> >> + if (alrmar & STM32_RTC_ALRMXR_SEC_MASK) {
> >> + /* Seconds don't care in Alarm comparison */
> >> + tm->tm_sec = -1;
> >> + } else {
> >> + tm->tm_sec = (alrmar & STM32_RTC_ALRMXR_SEC) >>
> >> + STM32_RTC_ALRMXR_SEC_SHIFT;
> >> + }
> >> +
> >> + bcd2tm(tm);
> >> +
> >> + alrm->enabled = (cr & STM32_RTC_CR_ALRAE) ? 1 : 0;
> >> + alrm->pending = (isr & STM32_RTC_ISR_ALRAF) ? 1 : 0;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int stm32_rtc_alarm_irq_enable(struct device *dev, unsigned int
> enabled)
> >> +{
> >> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> >> + unsigned long irqflags;
> >> + unsigned int isr, cr;
> >> +
> >> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
> >
> > Is the STM32_RTC_CR garanteed to be valid, i.e updated atomically? If not
> this
> > should probably be below the spinlock.
> You're right.
> >
> >> +
> >> + spin_lock_irqsave(&rtc->lock, irqflags);
> >> +
> >> + stm32_rtc_wpr_unlock(rtc);
> >> +
> >> + /* We expose Alarm A to the kernel */
> >> + if (enabled)
> >> + cr |= (STM32_RTC_CR_ALRAIE | STM32_RTC_CR_ALRAE);
> >> + else
> >> + cr &= ~(STM32_RTC_CR_ALRAIE | STM32_RTC_CR_ALRAE);
> >> + stm32_rtc_writel(rtc, STM32_RTC_CR, cr);
> >> +
> >> + /* Clear event irqflags, otherwise new events won't be received */
> >> + isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
> >> + isr &= ~STM32_RTC_ISR_ALRAF;
> >> + stm32_rtc_writel(rtc, STM32_RTC_ISR, isr);
> >> +
> >> + stm32_rtc_wpr_lock(rtc);
> >> +
> >> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int stm32_rtc_set_alarm(struct device *dev, struct rtc_wkalrm
> *alrm)
> >> +{
> >> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> >> + struct rtc_time *tm = &alrm->time;
> >> + unsigned long irqflags;
> >> + unsigned int cr, isr, alrmar;
> >> + int ret = 0;
> >> +
> >> + if (rtc_valid_tm(tm)) {
> >> + dev_err(dev, "Alarm time not valid.\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + tm2bcd(tm);
> >> +
> >> + spin_lock_irqsave(&rtc->lock, irqflags);
> >> +
> >> + stm32_rtc_wpr_unlock(rtc);
> >> +
> >> + /* Disable Alarm */
> >> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
> >> + cr &= ~STM32_RTC_CR_ALRAE;
> >> + stm32_rtc_writel(rtc, STM32_RTC_CR, cr);
> >> +
> >> + /* Poll Alarm write flag to be sure that Alarm update is allowed */
> >> + ret = readl_relaxed_poll_timeout_atomic(rtc->base + STM32_RTC_ISR,
> >> + isr,
> >> + (isr & STM32_RTC_ISR_ALRAWF),
> >> + 10, 100);
> >> +
> >> + if (ret) {
> >> + dev_err(dev, "Alarm update not allowed\n");
> >> + goto end;
> >> + }
> >> +
> >> + alrmar = 0;
> >> +
> >> + if (tm->tm_mday < 0 && tm->tm_wday < 0) {
> >> + /*
> >> + * Date/day don't care in Alarm comparison so alarm triggers
> >> + * every day
> >> + */
> >> + alrmar |= STM32_RTC_ALRMXR_DATE_MASK;
> >> + } else {
> >> + if (tm->tm_mday > 0) {
> >> + /* Date is selected (ignoring wday) */
> >> + alrmar |= (tm->tm_mday << STM32_RTC_ALRMXR_DATE_SHIFT) &
> >> + STM32_RTC_ALRMXR_DATE;
> >> + } else {
> >> + /* Day of week is selected */
> >> + int wday = (tm->tm_wday == 0) ? 7 : tm->tm_wday;
> >> +
> >> + alrmar |= STM32_RTC_ALRMXR_WDSEL;
> >> + alrmar |= (wday << STM32_RTC_ALRMXR_WDAY_SHIFT) &
> >> + STM32_RTC_ALRMXR_WDAY;
> >> + }
> >> + }
> >> +
> >> + if (tm->tm_hour < 0) {
> >> + /* Hours don't care in Alarm comparison */
> >> + alrmar |= STM32_RTC_ALRMXR_HOUR_MASK;
> >> + } else {
> >> + /* 24-hour format */
> >> + alrmar &= ~STM32_RTC_ALRMXR_PM;
> >> + alrmar |= (tm->tm_hour << STM32_RTC_ALRMXR_HOUR_SHIFT) &
> >> + STM32_RTC_ALRMXR_HOUR;
> >> + }
> >> +
> >> + if (tm->tm_min < 0) {
> >> + /* Minutes don't care in Alarm comparison */
> >> + alrmar |= STM32_RTC_ALRMXR_MIN_MASK;
> >> + } else {
> >> + alrmar |= (tm->tm_min << STM32_RTC_ALRMXR_MIN_SHIFT) &
> >> + STM32_RTC_ALRMXR_MIN;
> >> + }
> >> +
> >> + if (tm->tm_sec < 0) {
> >> + /* Seconds don't care in Alarm comparison */
> >> + alrmar |= STM32_RTC_ALRMXR_SEC_MASK;
> >> + } else {
> >> + alrmar |= (tm->tm_sec << STM32_RTC_ALRMXR_SEC_SHIFT) &
> >> + STM32_RTC_ALRMXR_SEC;
> >> + }
> >> +
> >> + /* Write to Alarm register */
> >> + stm32_rtc_writel(rtc, STM32_RTC_ALRMAR, alrmar);
> >> +
> >> + if (alrm->enabled)
> >> + stm32_rtc_alarm_irq_enable(dev, 1);
> >> + else
> >> + stm32_rtc_alarm_irq_enable(dev, 0);
> >> +
> >> +end:
> >> + stm32_rtc_wpr_lock(rtc);
> >> +
> >> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static const struct rtc_class_ops stm32_rtc_ops = {
> >> + .read_time = stm32_rtc_read_time,
> >> + .set_time = stm32_rtc_set_time,
> >> + .read_alarm = stm32_rtc_read_alarm,
> >> + .set_alarm = stm32_rtc_set_alarm,
> >> + .alarm_irq_enable = stm32_rtc_alarm_irq_enable,
> >> +};
> >> +
> >> +#ifdef CONFIG_OF
> >> +static const struct of_device_id stm32_rtc_of_match[] = {
> >> + { .compatible = "st,stm32-rtc" },
> >> + {}
> >> +};
> >> +MODULE_DEVICE_TABLE(of, stm32_rtc_of_match);
> >> +#endif
> >> +
> >> +static int stm32_rtc_init(struct platform_device *pdev,
> >> + struct stm32_rtc *rtc)
> >> +{
> >> + unsigned int prer, pred_a, pred_s, pred_a_max, pred_s_max, cr;
> >> + unsigned int rate;
> >> + unsigned long irqflags;
> >> + int ret = 0;
> >> +
> >> + rate = clk_get_rate(rtc->ck_rtc);
> >> +
> >> + /* Find prediv_a and prediv_s to obtain the 1Hz calendar clock */
> >> + pred_a_max = STM32_RTC_PRER_PRED_A >> STM32_RTC_PRER_PRED_A_SHIFT;
> >> + pred_s_max = STM32_RTC_PRER_PRED_S >> STM32_RTC_PRER_PRED_S_SHIFT;
> >> +
> >> + for (pred_a = pred_a_max; pred_a >= 0; pred_a--) {
> >> + pred_s = (rate / (pred_a + 1)) - 1;
> >> +
> >> + if (((pred_s + 1) * (pred_a + 1)) == rate)
> >> + break;
> >> + }
> >> +
> >> + /*
> >> + * Can't find a 1Hz, so give priority to RTC power consumption
> >> + * by choosing the higher possible value for prediv_a
> >> + */
> >> + if ((pred_s > pred_s_max) || (pred_a > pred_a_max)) {
> >> + pred_a = pred_a_max;
> >> + pred_s = (rate / (pred_a + 1)) - 1;
> >> +
> >> + dev_warn(&pdev->dev, "ck_rtc is %s\n",
> >> + (rate - ((pred_a + 1) * (pred_s + 1)) < 0) ?
> >> + "fast" : "slow");
> >> + }
> >> +
> >> + spin_lock_irqsave(&rtc->lock, irqflags);
> >> +
> >> + stm32_rtc_wpr_unlock(rtc);
> >> +
> >> + ret = stm32_rtc_enter_init_mode(rtc);
> >> + if (ret) {
> >> + dev_err(&pdev->dev,
> >> + "Can't enter in init mode. Prescaler config failed.\n");
> >> + goto end;
> >> + }
> >> +
> >> + prer = (pred_s << STM32_RTC_PRER_PRED_S_SHIFT) &
> STM32_RTC_PRER_PRED_S;
> >> + stm32_rtc_writel(rtc, STM32_RTC_PRER, prer);
> >> + prer |= (pred_a << STM32_RTC_PRER_PRED_A_SHIFT) &
> STM32_RTC_PRER_PRED_A;
> >> + stm32_rtc_writel(rtc, STM32_RTC_PRER, prer);
> >> +
> >> + /* Force 24h time format */
> >> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
> >> + cr &= ~STM32_RTC_CR_FMT;
> >> + stm32_rtc_writel(rtc, STM32_RTC_CR, cr);
> >> +
> >> + stm32_rtc_exit_init_mode(rtc);
> >> +
> >> + ret = stm32_rtc_wait_sync(rtc);
> >> +
> >> + if (stm32_rtc_readl(rtc, STM32_RTC_ISR) & STM32_RTC_ISR_INITS)
> >> + dev_warn(&pdev->dev, "Date/Time must be initialized\n");
> >> +end:
> >> + stm32_rtc_wpr_lock(rtc);
> >> +
> >> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int stm32_rtc_probe(struct platform_device *pdev)
> >> +{
> >> + struct stm32_rtc *rtc;
> >> + struct resource *res;
> >> + int ret;
> >> +
> >> + rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> >> + if (!rtc)
> >> + return -ENOMEM;
> >> +
> >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >
> > The value of 'res' should be checked before using it.
> res is checked in devm_ioremap_resource just below :
> if (!res || resource_type(res) != IORESOURCE_MEM) {
> dev_err(dev, "invalid resource\n");
> return IOMEM_ERR_PTR(-EINVAL);
> }
> That's why it is not checked here.
> >
> >> + rtc->base = devm_ioremap_resource(&pdev->dev, res);
> >> + if (IS_ERR(rtc->base))
> >> + return PTR_ERR(rtc->base);
> >> +
> >> + dbp = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> "st,syscfg");
> >> + if (IS_ERR(dbp)) {
> >> + dev_err(&pdev->dev, "no st,syscfg\n");
> >> + return PTR_ERR(dbp);
> >> + }
> >> +
> >> + spin_lock_init(&rtc->lock);
> >> +
> >> + rtc->ck_rtc = devm_clk_get(&pdev->dev, "ck_rtc");
> >> + if (IS_ERR(rtc->ck_rtc)) {
> >> + dev_err(&pdev->dev, "no ck_rtc clock");
> >> + return PTR_ERR(rtc->ck_rtc);
> >> + }
> >> +
> >> + ret = clk_prepare_enable(rtc->ck_rtc);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + if (dbp)
> >> + regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, PWR_CR_DBP);
> >
> > The code above exits if there is a problem with the dbp, there is no point
> in
> > checking again.
> You're right.
> >
> >> +
> >> + ret = stm32_rtc_init(pdev, rtc);
> >> + if (ret)
> >> + goto err;
> >> +
> >> + rtc->irq_alarm = platform_get_irq_byname(pdev, "alarm");
> >> + if (rtc->irq_alarm <= 0) {
> >> + dev_err(&pdev->dev, "no alarm irq\n");
> >> + ret = -ENOENT;
> >> + goto err;
> >> + }
> >> +
> >> + platform_set_drvdata(pdev, rtc);
> >> +
> >> + device_init_wakeup(&pdev->dev, true);
> >
> > What happens if device_init_wakeup() returns an error?
> It means that RTC won't be able to wake up the board with RTC alarm. I can
> add a warning for the user in this case ?
Not really sure - it really depends on the kind of system will use this.
For some not being able to wake up the board might a minor problem while
for others a reason to fail the probing.
Do we need a new binging for this, i.e one that would indicate this RTC can (and
should) be able to wake up the board and fail driver probing if this can't be
done?
I'll let Alessandro and Alexander be the judge of that.
Thanks,
Mathieu
> >
> >> +
> >> + rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, pdev->name,
> >> + &stm32_rtc_ops, THIS_MODULE);
> >> + if (IS_ERR(rtc->rtc_dev)) {
> >> + ret = PTR_ERR(rtc->rtc_dev);
> >> + dev_err(&pdev->dev, "rtc device registration failed, err=%d\n",
> >> + ret);
> >> + goto err;
> >> + }
> >> +
> >> + /* Handle RTC alarm interrupts */
> >> + ret = devm_request_irq(&pdev->dev, rtc->irq_alarm,
> >> + stm32_rtc_alarm_irq, IRQF_TRIGGER_RISING,
> >> + dev_name(&rtc->rtc_dev->dev), rtc);
> >> + if (ret) {
> >> + dev_err(&pdev->dev, "IRQ%d (alarm interrupt) already claimed\n",
> >> + rtc->irq_alarm);
> >> + goto err;
> >> + }
> >> +
> >> + return 0;
> >> +err:
> >> + clk_disable_unprepare(rtc->ck_rtc);
> >> +
> >> + if (dbp)
> >> + regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, ~PWR_CR_DBP);
> >
> > Same comment as above.
> OK.
> >
> >> +
> >> + device_init_wakeup(&pdev->dev, false);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int __exit stm32_rtc_remove(struct platform_device *pdev)
> >> +{
> >> + struct stm32_rtc *rtc = platform_get_drvdata(pdev);
> >> + unsigned int cr;
> >> +
> >> + /* Disable interrupts */
> >> + stm32_rtc_wpr_unlock(rtc);
> >> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
> >> + cr &= ~STM32_RTC_CR_ALRAIE;
> >> + stm32_rtc_writel(rtc, STM32_RTC_CR, cr);
> >> + stm32_rtc_wpr_lock(rtc);
> >> +
> >> + clk_disable_unprepare(rtc->ck_rtc);
> >> +
> >> + /* Enable backup domain write protection */
> >> + if (dbp)
> >> + regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, ~PWR_CR_DBP);
> >> +
> >> + device_init_wakeup(&pdev->dev, false);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int stm32_rtc_suspend(struct device *dev)
> >> +{
> >> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> >> +
> >> + if (device_may_wakeup(dev))
> >> + return enable_irq_wake(rtc->irq_alarm);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int stm32_rtc_resume(struct device *dev)
> >> +{
> >> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> >> + int ret = 0;
> >> +
> >> + ret = stm32_rtc_wait_sync(rtc);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + if (device_may_wakeup(dev))
> >> + return disable_irq_wake(rtc->irq_alarm);
> >> +
> >> + return ret;
> >> +}
> >> +#endif
> >> +
> >> +static SIMPLE_DEV_PM_OPS(stm32_rtc_pm_ops,
> >> + stm32_rtc_suspend, stm32_rtc_resume);
> >> +
> >> +static struct platform_driver stm32_rtc_driver = {
> >> + .probe = stm32_rtc_probe,
> >> + .remove = stm32_rtc_remove,
> >> + .driver = {
> >> + .name = DRIVER_NAME,
> >> + .pm = &stm32_rtc_pm_ops,
> >> + .of_match_table = stm32_rtc_of_match,
> >> + },
> >> +};
> >> +
> >> +module_platform_driver(stm32_rtc_driver);
> >> +
> >> +MODULE_ALIAS("platform:" DRIVER_NAME);
> >> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org>");
> >> +MODULE_DESCRIPTION("STMicroelectronics STM32 Real Time Clock driver");
> >> +MODULE_LICENSE("GPL v2");
> >> --
> >> 1.9.1
> >>
>
> Best regards,
> Amelie
>
--
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 v12 0/4] dtc: Dynamic DT support
From: Pantelis Antoniou @ 2016-12-05 17:05 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
This patchset adds Dynamic DT support in the DTC compiler
as used in a number of boards like the beaglebone/rpi/chip and others.
The first patch documents the internals of overlay generation, while
the second one adds dynamic object/overlay support proper.
The third patch adds a test method that can is used by the subsequent
patch which adds a few overlay tests verifying operation.
The following 3 patches add support for the syntactic sugar version
of &foo { }; in a similar manner.
This patchset is against DTC mainline and is also available for a pull
request from https://github.com/pantoniou/dtc/tree/overlays
Regards
-- Pantelis
Changes since v11:
* Syntax and grammatical fixes to the documentation.
* Renamed options and internal variables controlling generation of symbols
and fixups.
* Rename version flags to specify that they refer to DTS version.
* Made sure that no symbols/fixup nodes are only generated if they contain
entries.
Changes since v10:
* Split out the syntactic sugar version of &foo { }; into a different
patches to make things clearer.
* Reworked a bit the arguments passed to fixup node generation method
making things simpler and utilize common methodology.
* Avoid string parsing the full path using the node walking instead for
local fixup generation.
* Added an option to suppress generation of fixup nodes on base trees.
* Added automatic generation of symbols and fixups when compiling a
plugin.
* Minor rework according to maintainer requests.
Changes since v9:
* Reversed -M switch to by default use new DTBO magic value.
* Removed global versionflags in the parser by using inherited
attributes.
* build_node instead of malloc at add_orphan_node().
* Do not use escape for path copy
* Do not generate /plugin/ when generating a dts file even when
the plugin flag is set..
Changes since v8:
* Removed extra member of boot_info in each node; passing boot_info
parameter to the check methods instead.
* Reworked yacc syntax that supports both old and new plugin syntax
* Added handling for new magic number (enabled by 'M' switch).
* Dropped dtbo/asmo formats.
* Added overlay testsuite.
* Addressed last version maintainer comments.
Changes since v7:
* Dropped xasprintf & backward compatibility patch
* Rebased against dgibson's overlay branch
* Minor doc wording fixes.
Changes since v6:
* Introduced xasprintf
* Added append_to_property and used it
* Changed some die()'s to assert
* Reordered node generation to respect sort
* Addressed remaining maintainer changes from v6
Changes since v5:
* Rebase to latest dtc version.
* Addressed all the maintainer requested changes from v5
* Added new magic value for dynamic objects and new format
Changes since v4:
* Rebase to latest dtc version.
* Completely redesigned the generation of resolution data.
Now instead of being generated as part of blob generation
they are created in the live tree.
* Consequently the patchset is much smaller.
* Added -A auto-label alias generation option.
* Addressed maintainer comments.
* Added syntactic sugar for overlays in the form of .dtsi
* Added /dts-v1/ /plugin/ preferred plugin form and deprecate
the previous form (although still works for backward compatibility)
Changes since v3:
* Rebase to latest dtc version.
Changes since v2:
* Split single patch to a patchset.
* Updated to dtc mainline.
* Changed __local_fixups__ format
* Clean up for better legibility.
Pantelis Antoniou (7):
dtc: Document the dynamic plugin internals
dtc: Plugin and fixup support
tests: Add check_path test
tests: Add overlay tests
overlay: Documentation for the overlay sugar syntax
overlay: Add syntactic sugar version of overlays
tests: Add a test for overlays syntactic sugar
Documentation/dt-object-internal.txt | 324 +++++++++++++++++++++++++++++++++++
Documentation/manual.txt | 29 +++-
checks.c | 8 +-
dtc-lexer.l | 5 +
dtc-parser.y | 49 +++++-
dtc.c | 54 +++++-
dtc.h | 22 ++-
fdtdump.c | 2 +-
flattree.c | 18 +-
fstree.c | 2 +-
libfdt/fdt.c | 2 +-
libfdt/fdt.h | 3 +-
livetree.c | 291 ++++++++++++++++++++++++++++++-
tests/.gitignore | 1 +
tests/Makefile.tests | 3 +-
tests/check_path.c | 82 +++++++++
tests/mangle-layout.c | 7 +-
tests/overlay_base_fixups.dts | 22 +++
tests/overlay_overlay_dtc.dts | 4 +-
tests/overlay_overlay_simple.dts | 12 ++
tests/run_tests.sh | 55 ++++++
21 files changed, 962 insertions(+), 33 deletions(-)
create mode 100644 Documentation/dt-object-internal.txt
create mode 100644 tests/check_path.c
create mode 100644 tests/overlay_base_fixups.dts
create mode 100644 tests/overlay_overlay_simple.dts
--
2.1.4
^ permalink raw reply
* [PATCH v12 1/7] dtc: Document the dynamic plugin internals
From: Pantelis Antoniou @ 2016-12-05 17:05 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
In-Reply-To: <1480957528-8367-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Provides the document explaining the internal mechanics of
plugins and options.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
Documentation/dt-object-internal.txt | 308 +++++++++++++++++++++++++++++++++++
1 file changed, 308 insertions(+)
create mode 100644 Documentation/dt-object-internal.txt
diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
new file mode 100644
index 0000000..7d76296
--- /dev/null
+++ b/Documentation/dt-object-internal.txt
@@ -0,0 +1,308 @@
+Device Tree Dynamic Object format internals
+-------------------------------------------
+
+The Device Tree for most platforms is a static representation of
+the hardware capabilities. This is insufficient for platforms
+that need to dynamically insert Device Tree fragments into the
+live tree.
+
+This document explains the the Device Tree object format and
+modifications made to the Device Tree compiler, which make it possible.
+
+1. Simplified Problem Definition
+--------------------------------
+
+Assume we have a platform which boots using following simplified Device Tree.
+
+---- foo.dts -----------------------------------------------------------------
+ /* FOO platform */
+ / {
+ compatible = "corp,foo";
+
+ /* shared resources */
+ res: res {
+ };
+
+ /* On chip peripherals */
+ ocp: ocp {
+ /* peripherals that are always instantiated */
+ peripheral1 { ... };
+ };
+ };
+---- foo.dts -----------------------------------------------------------------
+
+We have a number of peripherals that after probing (using some undefined method)
+should result in different Device Tree configuration.
+
+We cannot boot with this static tree because due to the configuration of the
+foo platform there exist multiple conficting peripherals DT fragments.
+
+So for the bar peripheral we would have this:
+
+---- foo+bar.dts -------------------------------------------------------------
+ /* FOO platform + bar peripheral */
+ / {
+ compatible = "corp,foo";
+
+ /* shared resources */
+ res: res {
+ };
+
+ /* On chip peripherals */
+ ocp: ocp {
+ /* peripherals that are always instantiated */
+ peripheral1 { ... };
+
+ /* bar peripheral */
+ bar {
+ compatible = "corp,bar";
+ ... /* various properties and child nodes */
+ };
+ };
+ };
+---- foo+bar.dts -------------------------------------------------------------
+
+While for the baz peripheral we would have this:
+
+---- foo+baz.dts -------------------------------------------------------------
+ /* FOO platform + baz peripheral */
+ / {
+ compatible = "corp,foo";
+
+ /* shared resources */
+ res: res {
+ /* baz resources */
+ baz_res: res_baz { ... };
+ };
+
+ /* On chip peripherals */
+ ocp: ocp {
+ /* peripherals that are always instantiated */
+ peripheral1 { ... };
+
+ /* baz peripheral */
+ baz {
+ compatible = "corp,baz";
+ /* reference to another point in the tree */
+ ref-to-res = <&baz_res>;
+ ... /* various properties and child nodes */
+ };
+ };
+ };
+---- foo+baz.dts -------------------------------------------------------------
+
+We note that the baz case is more complicated, since the baz peripheral needs to
+reference another node in the DT tree.
+
+2. Device Tree Object Format Requirements
+-----------------------------------------
+
+Since the Device Tree is used for booting a number of very different hardware
+platforms it is imperative that we tread very carefully.
+
+2.a) No changes to the Device Tree binary format for the base tree. We cannot
+modify the tree format at all and all the information we require should be
+encoded using Device Tree itself. We can add nodes that can be safely ignored
+by both bootloaders and the kernel. The plugin dtbs are optionally tagged
+with a different magic number in the header but otherwise they're simple
+blobs.
+
+2.b) Changes to the DTS source format should be absolutely minimal, and should
+only be needed for the DT fragment definitions, and not the base boot DT.
+
+2.c) An explicit option should be used to instruct DTC to generate the required
+information needed for object resolution. Platforms that don't use the
+dynamic object format can safely ignore it.
+
+2.d) Finally, DT syntax changes should be kept to a minimum. It should be
+possible to express everything using the existing DT syntax.
+
+3. Implementation
+-----------------
+
+The basic unit of addressing in Device Tree is the phandle. Turns out it's
+relatively simple to extend the way phandles are generated and referenced
+so that it's possible to dynamically convert symbolic references (labels)
+to phandle values. This is a valid assumption as long as the author uses
+reference syntax and does not assign phandle values manually (which might
+be a problem with decompiled source files).
+
+We can roughly divide the operation into two steps.
+
+3.a) Compilation of the base board DTS file using the '-@' option
+generates a valid DT blob with an added __symbols__ node at the root node,
+containing a list of all nodes that are marked with a label.
+
+Using the foo.dts file above the following node will be generated;
+
+$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
+$ fdtdump foo.dtb
+...
+/ {
+ ...
+ res {
+ ...
+ phandle = <0x00000001>;
+ ...
+ };
+ ocp {
+ ...
+ phandle = <0x00000002>;
+ ...
+ };
+ __symbols__ {
+ res="/res";
+ ocp="/ocp";
+ };
+};
+
+Notice that all the nodes that had a label have been recorded, and that
+phandles have been generated for them.
+
+This blob can be used to boot the board normally, the __symbols__ node will
+be safely ignored both by the bootloader and the kernel (the only loss will
+be a few bytes of memory and disk space).
+
+We generate a __symbols__ node to record nodes that had labels in the base
+tree (or subsequent loaded overlays) so that they can be matched up with
+references made to them in Device Tree objects.
+
+3.b) The Device Tree fragments must be compiled with the same option but they
+must also have a tag (/plugin/) that allows undefined references to nodes
+that are not present at compilation time to be recorded so that the runtime
+loader can fix them.
+
+So the bar peripheral's DTS format would be of the form:
+
+/dts-v1/ /plugin/; /* allow undefined references and record them */
+/ {
+ .... /* various properties for loader use; i.e. part id etc. */
+ fragment@0 {
+ target = <&ocp>;
+ __overlay__ {
+ /* bar peripheral */
+ bar {
+ compatible = "corp,bar";
+ ... /* various properties and child nodes */
+ }
+ };
+ };
+};
+
+Note that there's a target property that specifies the location where the
+contents of the overlay node will be placed, and it references the node
+in the foo.dts file.
+
+$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
+$ fdtdump bar.dtbo
+...
+/ {
+ ... /* properties */
+ fragment@0 {
+ target = <0xffffffff>;
+ __overlay__ {
+ bar {
+ compatible = "corp,bar";
+ ... /* various properties and child nodes */
+ }
+ };
+ };
+ __fixups__ {
+ ocp = "/fragment@0:target:0";
+ };
+};
+
+No __symbols__ node has been generated (no label in bar.dts).
+Note that the target's ocp label is undefined, so the phandle
+value is filled with the illegal value '0xffffffff', while a __fixups__
+node has been generated, which marks the location in the tree where
+the label lookup should store the runtime phandle value of the ocp node.
+
+The format of the __fixups__ node entry is
+
+ <label> = "<local-full-path>:<property-name>:<offset>"
+ [, "<local-full-path>:<property-name>:<offset>"...];
+
+ <label> Is the label we're referring
+ <local-full-path> Is the full path of the node the reference is
+ <property-name> Is the name of the property containing the
+ reference
+ <offset> The offset (in bytes) of where the property's
+ phandle value is located.
+
+Doing the same with the baz peripheral's DTS format is a little bit more
+involved, since baz contains references to local labels which require
+local fixups.
+
+/dts-v1/ /plugin/; /* allow undefined label references and record them */
+/ {
+ .... /* various properties for loader use; i.e. part id etc. */
+ fragment@0 {
+ target = <&res>;
+ __overlay__ {
+ /* baz resources */
+ baz_res: res_baz { ... };
+ };
+ };
+ fragment@1 {
+ target = <&ocp>;
+ __overlay__ {
+ /* baz peripheral */
+ baz {
+ compatible = "corp,baz";
+ /* reference to another point in the tree */
+ ref-to-res = <&baz_res>;
+ ... /* various properties and child nodes */
+ }
+ };
+ };
+};
+
+Note that &bar_res reference.
+
+$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
+$ fdtdump baz.dtbo
+...
+/ {
+ ... /* properties */
+ fragment@0 {
+ target = <0xffffffff>;
+ __overlay__ {
+ res_baz {
+ ....
+ phandle = <0x00000001>;
+ };
+ };
+ };
+ fragment@1 {
+ target = <0xffffffff>;
+ __overlay__ {
+ baz {
+ compatible = "corp,baz";
+ ... /* various properties and child nodes */
+ ref-to-res = <0x00000001>;
+ }
+ };
+ };
+ __fixups__ {
+ res = "/fragment@0:target:0";
+ ocp = "/fragment@1:target:0";
+ };
+ __local_fixups__ {
+ fragment@1 {
+ __overlay__ {
+ baz {
+ ref-to-res = <0>;
+ };
+ };
+ };
+ };
+};
+
+This is similar to the bar case, but the reference of a local label by the
+baz node generates a __local_fixups__ entry that records the place that the
+local reference is being made. No matter how phandles are allocated from dtc
+the run time loader must apply an offset to each phandle in every dynamic
+DT object loaded. The __local_fixups__ node records the offset relative to the
+start of every local reference within that property so that the loader can apply
+the offset.
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v12 2/7] dtc: Plugin and fixup support
From: Pantelis Antoniou @ 2016-12-05 17:05 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
In-Reply-To: <1480957528-8367-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
This patch enable the generation of symbols & local fixup information
for trees compiled with the -@ (--symbols) option.
Using this patch labels in the tree and their users emit information
in __symbols__ and __local_fixups__ nodes.
The __fixups__ node make possible the dynamic resolution of phandle
references which are present in the plugin tree but lie in the
tree that are applying the overlay against.
While there is a new magic number for dynamic device tree/overlays blobs
it is by default enabled. Remember to use -M to generate compatible
blobs.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
Documentation/manual.txt | 29 ++++-
checks.c | 8 +-
dtc-lexer.l | 5 +
dtc-parser.y | 29 ++++-
dtc.c | 54 +++++++++-
dtc.h | 21 +++-
fdtdump.c | 2 +-
flattree.c | 18 ++--
fstree.c | 2 +-
libfdt/fdt.c | 2 +-
libfdt/fdt.h | 3 +-
livetree.c | 269 ++++++++++++++++++++++++++++++++++++++++++++++-
tests/mangle-layout.c | 7 +-
13 files changed, 422 insertions(+), 27 deletions(-)
diff --git a/Documentation/manual.txt b/Documentation/manual.txt
index 398de32..92a4966 100644
--- a/Documentation/manual.txt
+++ b/Documentation/manual.txt
@@ -119,6 +119,28 @@ Options:
Make space for <number> reserve map entries
Relevant for dtb and asm output only.
+ -@
+ Generates a __symbols__ node at the root node of the resulting blob
+ for any node labels used, and for any local references using phandles
+ it also generates a __local_fixups__ node that tracks them.
+
+ When using the /plugin/ tag all unresolved label references to
+ be tracked in the __fixups__ node, making dynamic resolution possible.
+
+ -A
+ Generate automatically aliases for all node labels. This is similar to
+ the -@ option (the __symbols__ node contain identical information) but
+ the semantics are slightly different since no phandles are automatically
+ generated for labeled nodes.
+
+ -M
+ Generate blobs with the old FDT magic number for device tree objects.
+ By default blobs use the DTBO FDT magic number instead.
+
+ -F
+ Suppress generation of fixups when -@ is used. This is useful for generating
+ a base tree with symbols but without fixups that take some amount of space.
+
-S <bytes>
Ensure the blob at least <bytes> long, adding additional
space if needed.
@@ -146,13 +168,18 @@ Additionally, dtc performs various sanity checks on the tree.
Here is a very rough overview of the layout of a DTS source file:
- sourcefile: list_of_memreserve devicetree
+ sourcefile: versioninfo plugindecl list_of_memreserve devicetree
memreserve: label 'memreserve' ADDR ADDR ';'
| label 'memreserve' ADDR '-' ADDR ';'
devicetree: '/' nodedef
+ versioninfo: '/' 'dts-v1' '/' ';'
+
+ plugindecl: '/' 'plugin' '/' ';'
+ | /* empty */
+
nodedef: '{' list_of_property list_of_subnode '}' ';'
property: label PROPNAME '=' propdata ';'
diff --git a/checks.c b/checks.c
index 2bd27a4..56c231a 100644
--- a/checks.c
+++ b/checks.c
@@ -487,8 +487,12 @@ static void fixup_phandle_references(struct check *c, struct boot_info *bi,
refnode = get_node_by_ref(dt, m->ref);
if (! refnode) {
- FAIL(c, "Reference to non-existent node or label \"%s\"\n",
- m->ref);
+ if (!(bi->dtsversionflags & DTSVF_PLUGIN))
+ FAIL(c, "Reference to non-existent node or "
+ "label \"%s\"\n", m->ref);
+ else /* mark the entry as unresolved */
+ *((cell_t *)(prop->val.val + m->offset)) =
+ cpu_to_fdt32(0xffffffff);
continue;
}
diff --git a/dtc-lexer.l b/dtc-lexer.l
index 790fbf6..40bbc87 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
return DT_V1;
}
+<*>"/plugin/" {
+ DPRINT("Keyword: /plugin/\n");
+ return DT_PLUGIN;
+ }
+
<*>"/memreserve/" {
DPRINT("Keyword: /memreserve/\n");
BEGIN_DEFAULT();
diff --git a/dtc-parser.y b/dtc-parser.y
index 14aaf2e..aaacb97 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -19,6 +19,7 @@
*/
%{
#include <stdio.h>
+#include <inttypes.h>
#include "dtc.h"
#include "srcpos.h"
@@ -52,9 +53,11 @@ extern bool treesource_error;
struct node *nodelist;
struct reserve_info *re;
uint64_t integer;
+ unsigned int flags;
}
%token DT_V1
+%token DT_PLUGIN
%token DT_MEMRESERVE
%token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
%token DT_BITS
@@ -71,6 +74,8 @@ extern bool treesource_error;
%type <data> propdata
%type <data> propdataprefix
+%type <flags> versioninfo
+%type <flags> plugindecl
%type <re> memreserve
%type <re> memreserves
%type <array> arrayprefix
@@ -101,16 +106,34 @@ extern bool treesource_error;
%%
sourcefile:
- v1tag memreserves devicetree
+ versioninfo plugindecl memreserves devicetree
{
- the_boot_info = build_boot_info($2, $3,
- guess_boot_cpuid($3));
+ the_boot_info = build_boot_info($1 | $2, $3, $4,
+ guess_boot_cpuid($4));
+ }
+ ;
+
+versioninfo:
+ v1tag
+ {
+ $$ = DTSVF_V1;
}
;
v1tag:
DT_V1 ';'
+ | DT_V1
| DT_V1 ';' v1tag
+
+plugindecl:
+ DT_PLUGIN ';'
+ {
+ $$ = DTSVF_PLUGIN;
+ }
+ | /* empty */
+ {
+ $$ = 0;
+ }
;
memreserves:
diff --git a/dtc.c b/dtc.c
index 9dcf640..d35e0da 100644
--- a/dtc.c
+++ b/dtc.c
@@ -32,6 +32,10 @@ int minsize; /* Minimum blob size */
int padsize; /* Additional padding to blob */
int alignsize; /* Additional padding to blob accroding to the alignsize */
int phandle_format = PHANDLE_BOTH; /* Use linux,phandle or phandle properties */
+int generate_symbols; /* enable symbols & fixup support */
+int generate_fixups; /* suppress generation of fixups on symbol support */
+int auto_label_aliases; /* auto generate labels -> aliases */
+int no_dtbo_magic; /* use old FDT magic values for objects */
static int is_power_of_2(int x)
{
@@ -59,7 +63,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
#define FDT_VERSION(version) _FDT_VERSION(version)
#define _FDT_VERSION(version) #version
static const char usage_synopsis[] = "dtc [options] <input file>";
-static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:hv";
+static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@FAMhv";
static struct option const usage_long_opts[] = {
{"quiet", no_argument, NULL, 'q'},
{"in-format", a_argument, NULL, 'I'},
@@ -78,6 +82,10 @@ static struct option const usage_long_opts[] = {
{"phandle", a_argument, NULL, 'H'},
{"warning", a_argument, NULL, 'W'},
{"error", a_argument, NULL, 'E'},
+ {"symbols", no_argument, NULL, '@'},
+ {"fixups", no_argument, NULL, 'F'},
+ {"auto-alias", no_argument, NULL, 'A'},
+ {"no-dtbo-magic", no_argument, NULL, 'M'},
{"help", no_argument, NULL, 'h'},
{"version", no_argument, NULL, 'v'},
{NULL, no_argument, NULL, 0x0},
@@ -109,6 +117,10 @@ static const char * const usage_opts_help[] = {
"\t\tboth - Both \"linux,phandle\" and \"phandle\" properties",
"\n\tEnable/disable warnings (prefix with \"no-\")",
"\n\tEnable/disable errors (prefix with \"no-\")",
+ "\n\tEnable generation of symbols",
+ "\n\tEnable generation of fixups",
+ "\n\tEnable auto-alias of labels",
+ "\n\tDo not use DTBO magic value for plugin objects",
"\n\tPrint this help and exit",
"\n\tPrint version and exit",
NULL,
@@ -153,7 +165,7 @@ static const char *guess_input_format(const char *fname, const char *fallback)
fclose(f);
magic = fdt32_to_cpu(magic);
- if (magic == FDT_MAGIC)
+ if (magic == FDT_MAGIC || magic == FDT_MAGIC_DTBO)
return "dtb";
return guess_type_by_name(fname, fallback);
@@ -172,6 +184,7 @@ int main(int argc, char *argv[])
FILE *outf = NULL;
int outversion = DEFAULT_FDT_VERSION;
long long cmdline_boot_cpuid = -1;
+ fdt32_t out_magic = FDT_MAGIC;
quiet = 0;
reservenum = 0;
@@ -249,6 +262,19 @@ int main(int argc, char *argv[])
parse_checks_option(false, true, optarg);
break;
+ case '@':
+ generate_symbols = 1;
+ break;
+ case 'F':
+ generate_fixups = 1;
+ break;
+ case 'A':
+ auto_label_aliases = 1;
+ break;
+ case 'M':
+ no_dtbo_magic = 1;
+ break;
+
case 'h':
usage(NULL);
default:
@@ -306,6 +332,23 @@ int main(int argc, char *argv[])
fill_fullpaths(bi->dt, "");
process_checks(force, bi);
+ /* on a plugin, generate by default */
+ if (bi->dtsversionflags & DTSVF_PLUGIN) {
+ generate_symbols = 1;
+ generate_fixups = 1;
+ }
+
+ if (auto_label_aliases)
+ generate_label_tree(bi, "aliases", false);
+
+ if (generate_symbols)
+ generate_label_tree(bi, "__symbols__", true);
+
+ if (generate_fixups) {
+ generate_fixups_tree(bi, "__fixups__");
+ generate_local_fixups_tree(bi, "__local_fixups__");
+ }
+
if (sort)
sort_tree(bi);
@@ -318,12 +361,15 @@ int main(int argc, char *argv[])
outname, strerror(errno));
}
+ if (!no_dtbo_magic && (bi->dtsversionflags & DTSVF_PLUGIN))
+ out_magic = FDT_MAGIC_DTBO;
+
if (streq(outform, "dts")) {
dt_to_source(outf, bi);
} else if (streq(outform, "dtb")) {
- dt_to_blob(outf, bi, outversion);
+ dt_to_blob(outf, bi, out_magic, outversion);
} else if (streq(outform, "asm")) {
- dt_to_asm(outf, bi, outversion);
+ dt_to_asm(outf, bi, out_magic, outversion);
} else if (streq(outform, "null")) {
/* do nothing */
} else {
diff --git a/dtc.h b/dtc.h
index 32009bc..6aa82eb 100644
--- a/dtc.h
+++ b/dtc.h
@@ -55,6 +55,10 @@ extern int minsize; /* Minimum blob size */
extern int padsize; /* Additional padding to blob */
extern int alignsize; /* Additional padding to blob accroding to the alignsize */
extern int phandle_format; /* Use linux,phandle or phandle properties */
+extern int generate_symbols; /* generate symbols for nodes with labels */
+extern int generate_fixups; /* generate fixups */
+extern int auto_label_aliases; /* auto generate labels -> aliases */
+extern int no_dtbo_magic; /* use old FDT magic values for objects */
#define PHANDLE_LEGACY 0x1
#define PHANDLE_EPAPR 0x2
@@ -202,6 +206,8 @@ void delete_property(struct property *prop);
void add_child(struct node *parent, struct node *child);
void delete_node_by_name(struct node *parent, char *name);
void delete_node(struct node *node);
+void append_to_property(struct node *node,
+ char *name, const void *data, int len);
const char *get_unitname(struct node *node);
struct property *get_property(struct node *node, const char *propname);
@@ -237,14 +243,23 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
struct boot_info {
+ unsigned int dtsversionflags;
struct reserve_info *reservelist;
struct node *dt; /* the device tree */
uint32_t boot_cpuid_phys;
};
-struct boot_info *build_boot_info(struct reserve_info *reservelist,
+/* DTS version flags definitions */
+#define DTSVF_V1 0x0001 /* /dts-v1/ */
+#define DTSVF_PLUGIN 0x0002 /* /plugin/ */
+
+struct boot_info *build_boot_info(unsigned int dtsversionflags,
+ struct reserve_info *reservelist,
struct node *tree, uint32_t boot_cpuid_phys);
void sort_tree(struct boot_info *bi);
+void generate_label_tree(struct boot_info *bi, char *name, bool allocph);
+void generate_fixups_tree(struct boot_info *bi, char *name);
+void generate_local_fixups_tree(struct boot_info *bi, char *name);
/* Checks */
@@ -253,8 +268,8 @@ void process_checks(bool force, struct boot_info *bi);
/* Flattened trees */
-void dt_to_blob(FILE *f, struct boot_info *bi, int version);
-void dt_to_asm(FILE *f, struct boot_info *bi, int version);
+void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
+void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
struct boot_info *dt_from_blob(const char *fname);
diff --git a/fdtdump.c b/fdtdump.c
index a9a2484..dd63ac2 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -201,7 +201,7 @@ int main(int argc, char *argv[])
p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
if (!p)
break;
- if (fdt_magic(p) == FDT_MAGIC) {
+ if (fdt_magic(p) == FDT_MAGIC || fdt_magic(p) == FDT_MAGIC_DTBO) {
/* try and validate the main struct */
off_t this_len = endp - p;
fdt32_t max_version = 17;
diff --git a/flattree.c b/flattree.c
index a9d9520..5ddde45 100644
--- a/flattree.c
+++ b/flattree.c
@@ -335,6 +335,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
}
static void make_fdt_header(struct fdt_header *fdt,
+ fdt32_t magic,
struct version_info *vi,
int reservesize, int dtsize, int strsize,
int boot_cpuid_phys)
@@ -345,7 +346,7 @@ static void make_fdt_header(struct fdt_header *fdt,
memset(fdt, 0xff, sizeof(*fdt));
- fdt->magic = cpu_to_fdt32(FDT_MAGIC);
+ fdt->magic = cpu_to_fdt32(magic);
fdt->version = cpu_to_fdt32(vi->version);
fdt->last_comp_version = cpu_to_fdt32(vi->last_comp_version);
@@ -366,7 +367,7 @@ static void make_fdt_header(struct fdt_header *fdt,
fdt->size_dt_struct = cpu_to_fdt32(dtsize);
}
-void dt_to_blob(FILE *f, struct boot_info *bi, int version)
+void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
{
struct version_info *vi = NULL;
int i;
@@ -390,7 +391,7 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
reservebuf = flatten_reserve_list(bi->reservelist, vi);
/* Make header */
- make_fdt_header(&fdt, vi, reservebuf.len, dtbuf.len, strbuf.len,
+ make_fdt_header(&fdt, magic, vi, reservebuf.len, dtbuf.len, strbuf.len,
bi->boot_cpuid_phys);
/*
@@ -467,7 +468,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
}
}
-void dt_to_asm(FILE *f, struct boot_info *bi, int version)
+void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
{
struct version_info *vi = NULL;
int i;
@@ -830,6 +831,7 @@ struct boot_info *dt_from_blob(const char *fname)
struct node *tree;
uint32_t val;
int flags = 0;
+ unsigned int dtsversionflags = DTSVF_V1;
f = srcfile_relative_open(fname, NULL);
@@ -845,9 +847,12 @@ struct boot_info *dt_from_blob(const char *fname)
}
magic = fdt32_to_cpu(magic);
- if (magic != FDT_MAGIC)
+ if (magic != FDT_MAGIC && magic != FDT_MAGIC_DTBO)
die("Blob has incorrect magic number\n");
+ if (magic == FDT_MAGIC_DTBO)
+ dtsversionflags |= DTSVF_PLUGIN;
+
rc = fread(&totalsize, sizeof(totalsize), 1, f);
if (ferror(f))
die("Error reading DT blob size: %s\n", strerror(errno));
@@ -942,5 +947,6 @@ struct boot_info *dt_from_blob(const char *fname)
fclose(f);
- return build_boot_info(reservelist, tree, boot_cpuid_phys);
+ return build_boot_info(dtsversionflags, reservelist, tree,
+ boot_cpuid_phys);
}
diff --git a/fstree.c b/fstree.c
index 6d1beec..c4f1ccf 100644
--- a/fstree.c
+++ b/fstree.c
@@ -86,6 +86,6 @@ struct boot_info *dt_from_fs(const char *dirname)
tree = read_fstree(dirname);
tree = name_node(tree, "");
- return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
+ return build_boot_info(DTSVF_V1, NULL, tree, guess_boot_cpuid(tree));
}
diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 22286a1..28d422c 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -57,7 +57,7 @@
int fdt_check_header(const void *fdt)
{
- if (fdt_magic(fdt) == FDT_MAGIC) {
+ if (fdt_magic(fdt) == FDT_MAGIC || fdt_magic(fdt) == FDT_MAGIC_DTBO) {
/* Complete tree */
if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
return -FDT_ERR_BADVERSION;
diff --git a/libfdt/fdt.h b/libfdt/fdt.h
index 526aedb..493cd55 100644
--- a/libfdt/fdt.h
+++ b/libfdt/fdt.h
@@ -55,7 +55,7 @@
#ifndef __ASSEMBLY__
struct fdt_header {
- fdt32_t magic; /* magic word FDT_MAGIC */
+ fdt32_t magic; /* magic word FDT_MAGIC[|_DTBO] */
fdt32_t totalsize; /* total size of DT block */
fdt32_t off_dt_struct; /* offset to structure */
fdt32_t off_dt_strings; /* offset to strings */
@@ -93,6 +93,7 @@ struct fdt_property {
#endif /* !__ASSEMBLY */
#define FDT_MAGIC 0xd00dfeed /* 4: version, 4: total size */
+#define FDT_MAGIC_DTBO 0xd00dfdb0 /* DTBO magic */
#define FDT_TAGSIZE sizeof(fdt32_t)
#define FDT_BEGIN_NODE 0x1 /* Start node: full name */
diff --git a/livetree.c b/livetree.c
index 3dc7559..54a16ba 100644
--- a/livetree.c
+++ b/livetree.c
@@ -296,6 +296,23 @@ void delete_node(struct node *node)
delete_labels(&node->labels);
}
+void append_to_property(struct node *node,
+ char *name, const void *data, int len)
+{
+ struct data d;
+ struct property *p;
+
+ p = get_property(node, name);
+ if (p) {
+ d = data_append_data(p->val, data, len);
+ p->val = d;
+ } else {
+ d = data_append_data(empty_data, data, len);
+ p = build_property(name, d);
+ add_property(node, p);
+ }
+}
+
struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
{
struct reserve_info *new = xmalloc(sizeof(*new));
@@ -335,12 +352,14 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
return list;
}
-struct boot_info *build_boot_info(struct reserve_info *reservelist,
+struct boot_info *build_boot_info(unsigned int dtsversionflags,
+ struct reserve_info *reservelist,
struct node *tree, uint32_t boot_cpuid_phys)
{
struct boot_info *bi;
bi = xmalloc(sizeof(*bi));
+ bi->dtsversionflags = dtsversionflags;
bi->reservelist = reservelist;
bi->dt = tree;
bi->boot_cpuid_phys = boot_cpuid_phys;
@@ -709,3 +728,251 @@ void sort_tree(struct boot_info *bi)
sort_reserve_entries(bi);
sort_node(bi->dt);
}
+
+/* utility helper to avoid code duplication */
+static struct node *build_and_name_child_node(struct node *parent, char *name)
+{
+ struct node *node;
+
+ node = build_node(NULL, NULL);
+ name_node(node, xstrdup(name));
+ add_child(parent, node);
+
+ return node;
+}
+
+static struct node *build_root_node(struct node *dt, char *name)
+{
+ struct node *an;
+
+ an = get_subnode(dt, name);
+ if (!an)
+ an = build_and_name_child_node(dt, name);
+
+ if (!an)
+ die("Could not build root node /%s\n", name);
+
+ return an;
+}
+
+static bool any_label_tree(struct boot_info *bi, struct node *node)
+{
+ struct node *c;
+
+ if (node->labels)
+ return true;
+
+ for_each_child(node, c)
+ if (any_label_tree(bi, c))
+ return true;
+
+ return false;
+}
+
+static void generate_label_tree_internal(struct boot_info *bi,
+ struct node *an, struct node *node,
+ bool allocph)
+{
+ struct node *dt = bi->dt;
+ struct node *c;
+ struct property *p;
+ struct label *l;
+
+ /* if there are labels */
+ if (node->labels) {
+
+ /* now add the label in the node */
+ for_each_label(node->labels, l) {
+
+ /* check whether the label already exists */
+ p = get_property(an, l->label);
+ if (p) {
+ fprintf(stderr, "WARNING: label %s already"
+ " exists in /%s", l->label,
+ an->name);
+ continue;
+ }
+
+ /* insert it */
+ p = build_property(l->label,
+ data_copy_mem(node->fullpath,
+ strlen(node->fullpath) + 1));
+ add_property(an, p);
+ }
+
+ /* force allocation of a phandle for this node */
+ if (allocph)
+ (void)get_node_phandle(dt, node);
+ }
+
+ for_each_child(node, c)
+ generate_label_tree_internal(bi, an, c, allocph);
+}
+
+static bool any_fixup_tree(struct boot_info *bi, struct node *node)
+{
+ struct node *c;
+ struct property *prop;
+ struct marker *m;
+
+ for_each_property(node, prop) {
+ m = prop->val.markers;
+ for_each_marker_of_type(m, REF_PHANDLE) {
+ if (!get_node_by_ref(bi->dt, m->ref))
+ return true;
+ }
+ }
+
+ for_each_child(node, c) {
+ if (any_fixup_tree(bi, c))
+ return true;
+ }
+
+ return false;
+}
+
+static void add_fixup_entry(struct boot_info *bi, struct node *fn,
+ struct node *node, struct property *prop,
+ struct marker *m)
+{
+ char *entry;
+
+ /* m->ref can only be a REF_PHANDLE, but check anyway */
+ assert(m->type == REF_PHANDLE);
+
+ /* there shouldn't be any ':' in the arguments */
+ if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
+ die("arguments should not contain ':'\n");
+
+ xasprintf(&entry, "%s:%s:%u",
+ node->fullpath, prop->name, m->offset);
+ append_to_property(fn, m->ref, entry, strlen(entry) + 1);
+}
+
+static void generate_fixups_tree_internal(struct boot_info *bi,
+ struct node *fn,
+ struct node *node)
+{
+ struct node *dt = bi->dt;
+ struct node *c;
+ struct property *prop;
+ struct marker *m;
+ struct node *refnode;
+
+ for_each_property(node, prop) {
+ m = prop->val.markers;
+ for_each_marker_of_type(m, REF_PHANDLE) {
+ refnode = get_node_by_ref(dt, m->ref);
+ if (!refnode)
+ add_fixup_entry(bi, fn, node, prop, m);
+ }
+ }
+
+ for_each_child(node, c)
+ generate_fixups_tree_internal(bi, fn, c);
+}
+
+static bool any_local_fixup_tree(struct boot_info *bi, struct node *node)
+{
+ struct node *c;
+ struct property *prop;
+ struct marker *m;
+
+ for_each_property(node, prop) {
+ m = prop->val.markers;
+ for_each_marker_of_type(m, REF_PHANDLE) {
+ if (get_node_by_ref(bi->dt, m->ref))
+ return true;
+ }
+ }
+
+ for_each_child(node, c) {
+ if (any_local_fixup_tree(bi, c))
+ return true;
+ }
+
+ return false;
+}
+
+static void add_local_fixup_entry(struct boot_info *bi,
+ struct node *lfn, struct node *node,
+ struct property *prop, struct marker *m,
+ struct node *refnode)
+{
+ struct node *wn, *nwn; /* local fixup node, walk node, new */
+ uint32_t value_32;
+ char **compp;
+ int i, depth;
+
+ /* walk back retreiving depth */
+ depth = 0;
+ for (wn = node; wn; wn = wn->parent)
+ depth++;
+
+ /* allocate name array */
+ compp = xmalloc(sizeof(*compp) * depth);
+
+ /* store names in the array */
+ for (wn = node, i = depth - 1; wn; wn = wn->parent, i--)
+ compp[i] = wn->name;
+
+ /* walk the path components creating nodes if they don't exist */
+ for (wn = lfn, i = 1; i < depth; i++, wn = nwn) {
+ /* if no node exists, create it */
+ nwn = get_subnode(wn, compp[i]);
+ if (!nwn)
+ nwn = build_and_name_child_node(wn, compp[i]);
+ }
+
+ free(compp);
+
+ value_32 = cpu_to_fdt32(m->offset);
+ append_to_property(wn, prop->name, &value_32, sizeof(value_32));
+}
+
+static void generate_local_fixups_tree_internal(struct boot_info *bi,
+ struct node *lfn,
+ struct node *node)
+{
+ struct node *dt = bi->dt;
+ struct node *c;
+ struct property *prop;
+ struct marker *m;
+ struct node *refnode;
+
+ for_each_property(node, prop) {
+ m = prop->val.markers;
+ for_each_marker_of_type(m, REF_PHANDLE) {
+ refnode = get_node_by_ref(dt, m->ref);
+ if (refnode)
+ add_local_fixup_entry(bi, lfn, node, prop, m, refnode);
+ }
+ }
+
+ for_each_child(node, c)
+ generate_local_fixups_tree_internal(bi, lfn, c);
+}
+
+void generate_label_tree(struct boot_info *bi, char *name, bool allocph)
+{
+ if (!any_label_tree(bi, bi->dt))
+ return;
+ generate_label_tree_internal(bi, build_root_node(bi->dt, name),
+ bi->dt, allocph);
+}
+
+void generate_fixups_tree(struct boot_info *bi, char *name)
+{
+ if (!any_fixup_tree(bi, bi->dt))
+ return;
+ generate_fixups_tree_internal(bi, build_root_node(bi->dt, name),
+ bi->dt);
+}
+
+void generate_local_fixups_tree(struct boot_info *bi, char *name)
+{
+ if (!any_local_fixup_tree(bi, bi->dt))
+ return;
+ generate_local_fixups_tree_internal(bi, build_root_node(bi->dt, name),
+ bi->dt);
+}
diff --git a/tests/mangle-layout.c b/tests/mangle-layout.c
index a76e51e..d29ebc6 100644
--- a/tests/mangle-layout.c
+++ b/tests/mangle-layout.c
@@ -42,7 +42,8 @@ static void expand_buf(struct bufstate *buf, int newsize)
buf->size = newsize;
}
-static void new_header(struct bufstate *buf, int version, const void *fdt)
+static void new_header(struct bufstate *buf, fdt32_t magic, int version,
+ const void *fdt)
{
int hdrsize;
@@ -56,7 +57,7 @@ static void new_header(struct bufstate *buf, int version, const void *fdt)
expand_buf(buf, hdrsize);
memset(buf->buf, 0, hdrsize);
- fdt_set_magic(buf->buf, FDT_MAGIC);
+ fdt_set_magic(buf->buf, magic);
fdt_set_version(buf->buf, version);
fdt_set_last_comp_version(buf->buf, 16);
fdt_set_boot_cpuid_phys(buf->buf, fdt_boot_cpuid_phys(fdt));
@@ -145,7 +146,7 @@ int main(int argc, char *argv[])
if (fdt_version(fdt) < 17)
CONFIG("Input tree must be v17");
- new_header(&buf, version, fdt);
+ new_header(&buf, FDT_MAGIC, version, fdt);
while (*blockorder) {
add_block(&buf, version, *blockorder, fdt);
--
2.1.4
^ permalink raw reply related
* [PATCH v12 3/7] tests: Add check_path test
From: Pantelis Antoniou @ 2016-12-05 17:05 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
In-Reply-To: <1480957528-8367-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Add a test that checks for existence or not of a node.
It is useful for testing the various cases when generating
symbols and fixups for dynamic device tree objects.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
tests/.gitignore | 1 +
tests/Makefile.tests | 3 +-
tests/check_path.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
tests/run_tests.sh | 3 ++
4 files changed, 88 insertions(+), 1 deletion(-)
create mode 100644 tests/check_path.c
diff --git a/tests/.gitignore b/tests/.gitignore
index 354b565..9e209d5 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -8,6 +8,7 @@ tmp.*
/asm_tree_dump
/boot-cpuid
/char_literal
+/check_path
/del_node
/del_property
/dtbs_equal_ordered
diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index eb039c5..3d7a4f8 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -25,7 +25,8 @@ LIB_TESTS_L = get_mem_rsv \
integer-expressions \
property_iterate \
subnode_iterate \
- overlay overlay_bad_fixup
+ overlay overlay_bad_fixup \
+ check_path
LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)
LIBTREE_TESTS_L = truncated_property
diff --git a/tests/check_path.c b/tests/check_path.c
new file mode 100644
index 0000000..0d6a73b
--- /dev/null
+++ b/tests/check_path.c
@@ -0,0 +1,82 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ * Testcase for node existence
+ * Copyright (C) 2016 Konsulko Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <stdio.h>
+
+#include <libfdt.h>
+
+#include "tests.h"
+
+#define CHECK(code) \
+ { \
+ if (code) \
+ FAIL(#code ": %s", fdt_strerror(code)); \
+ }
+
+/* 4k ought to be enough for anybody */
+#define FDT_COPY_SIZE (4 * 1024)
+
+static void *open_dt(char *path)
+{
+ void *dt, *copy;
+
+ dt = load_blob(path);
+ copy = xmalloc(FDT_COPY_SIZE);
+
+ /*
+ * Resize our DTs to 4k so that we have room to operate on
+ */
+ CHECK(fdt_open_into(dt, copy, FDT_COPY_SIZE));
+
+ return copy;
+}
+
+int main(int argc, char *argv[])
+{
+ void *fdt_base;
+ int fail_config, exists, check_exists;
+
+ test_init(argc, argv);
+ fail_config = 0;
+
+ if (argc != 4)
+ fail_config = 1;
+
+ if (!fail_config) {
+ if (!strcmp(argv[2], "exists"))
+ check_exists = 1;
+ else if (!strcmp(argv[2], "not-exists"))
+ check_exists = 0;
+ else
+ fail_config = 1;
+ }
+
+ if (fail_config)
+ CONFIG("Usage: %s <base dtb> <[exists|not-exists]> <node-path>", argv[0]);
+
+ fdt_base = open_dt(argv[1]);
+
+ exists = fdt_path_offset(fdt_base, argv[3]) >= 0;
+
+ if (exists == check_exists)
+ PASS();
+ else
+ FAIL();
+}
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index e4139dd..d64cb2a 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -508,6 +508,9 @@ dtc_tests () {
run_sh_test dtc-checkfails.sh duplicate_label -- -I dts -O dtb reuse-label5.dts
run_sh_test dtc-checkfails.sh duplicate_label -- -I dts -O dtb reuse-label6.dts
+ run_test check_path test_tree1.dtb exists "/subnode@1"
+ run_test check_path test_tree1.dtb not-exists "/subnode@10"
+
# Check warning options
run_sh_test dtc-checkfails.sh address_cells_is_cell interrupt_cells_is_cell -n size_cells_is_cell -- -Wno_size_cells_is_cell -I dts -O dtb bad-ncells.dts
run_sh_test dtc-fails.sh -n test-warn-output.test.dtb -I dts -O dtb bad-ncells.dts
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v12 4/7] tests: Add overlay tests
From: Pantelis Antoniou @ 2016-12-05 17:05 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
In-Reply-To: <1480957528-8367-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Add a number of tests for dynamic objects/overlays.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
tests/overlay_base_fixups.dts | 22 +++++++++++++++++++
tests/overlay_overlay_dtc.dts | 4 ++--
tests/overlay_overlay_simple.dts | 12 +++++++++++
tests/run_tests.sh | 46 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 82 insertions(+), 2 deletions(-)
create mode 100644 tests/overlay_base_fixups.dts
create mode 100644 tests/overlay_overlay_simple.dts
diff --git a/tests/overlay_base_fixups.dts b/tests/overlay_base_fixups.dts
new file mode 100644
index 0000000..815a054
--- /dev/null
+++ b/tests/overlay_base_fixups.dts
@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2016 Konsulko Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+/dts-v1/;
+
+/ {
+ test: test-node {
+ test-int-property = <42>;
+ test-str-property = "foo";
+
+ subtest: sub-test-node {
+ sub-test-property;
+ };
+ };
+
+ ref {
+ local-ref = <&test &subtest>;
+ };
+};
diff --git a/tests/overlay_overlay_dtc.dts b/tests/overlay_overlay_dtc.dts
index 30d2362..b760b5a 100644
--- a/tests/overlay_overlay_dtc.dts
+++ b/tests/overlay_overlay_dtc.dts
@@ -1,12 +1,12 @@
/*
* Copyright (c) 2016 NextThing Co
* Copyright (c) 2016 Free Electrons
+ * Copyright (c) 2016 Konsulko Inc.
*
* SPDX-License-Identifier: GPL-2.0+
*/
-/dts-v1/;
-/plugin/;
+/dts-v1/ /plugin/;
/ {
/* Test that we can change an int by another */
diff --git a/tests/overlay_overlay_simple.dts b/tests/overlay_overlay_simple.dts
new file mode 100644
index 0000000..8657e1e
--- /dev/null
+++ b/tests/overlay_overlay_simple.dts
@@ -0,0 +1,12 @@
+/*
+ * Copyright (c) 2016 Konsulko Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+/dts-v1/;
+/plugin/;
+
+&test {
+ test-int-property = <43>;
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index d64cb2a..ee30383 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -181,6 +181,52 @@ overlay_tests () {
run_dtc_test -@ -I dts -O dtb -o overlay_base_with_symbols.test.dtb overlay_base.dts
run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols.test.dtb overlay_overlay_dtc.dts
run_test overlay overlay_base_with_symbols.test.dtb overlay_overlay_with_symbols.test.dtb
+
+ # Test symbols/fixups existence
+ run_test check_path overlay_overlay_with_symbols.test.dtb exists "/__symbols__"
+ run_test check_path overlay_overlay_with_symbols.test.dtb exists "/__fixups__"
+ run_test check_path overlay_overlay_with_symbols.test.dtb exists "/__local_fixups__"
+
+ # test new magic option
+ run_dtc_test -M@ -I dts -O dtb -o overlay_overlay_with_symbols_new_magic.test.dtb overlay_overlay_dtc.dts
+ run_test check_path overlay_overlay_with_symbols_new_magic.test.dtb exists "/__symbols__"
+ run_test check_path overlay_overlay_with_symbols_new_magic.test.dtb exists "/__fixups__"
+ run_test check_path overlay_overlay_with_symbols_new_magic.test.dtb exists "/__local_fixups__"
+
+ # test plugin source to dtb and back
+ run_dtc_test -@ -I dtb -O dts -o overlay_overlay_dtc.test.dts overlay_overlay_with_symbols.test.dtb
+ run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols.test.test.dtb overlay_overlay_dtc.test.dts
+ run_test dtbs_equal_ordered overlay_overlay_with_symbols.test.dtb overlay_overlay_with_symbols.test.test.dtb
+
+ # test plugin source to dtb and back (with new magic)
+ run_dtc_test -@ -I dtb -O dts -o overlay_overlay_dtc_new_magic.test.dts overlay_overlay_with_symbols_new_magic.test.dtb
+ run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols_new_magic.test.test.dtb overlay_overlay_dtc_new_magic.test.dts
+ run_test dtbs_equal_ordered overlay_overlay_with_symbols_new_magic.test.dtb overlay_overlay_with_symbols_new_magic.test.test.dtb
+
+ # test plugin auto-generation without using -@
+ run_dtc_test -I dts -O dtb -o overlay_overlay_with_symbols_auto.test.dtb overlay_overlay_dtc.dts
+ run_test check_path overlay_overlay_with_symbols_auto.test.dtb exists "/__symbols__"
+ run_test check_path overlay_overlay_with_symbols_auto.test.dtb exists "/__fixups__"
+ run_test check_path overlay_overlay_with_symbols_auto.test.dtb exists "/__local_fixups__"
+
+ # Test suppression of fixups
+ run_dtc_test -@ -I dts -O dtb -o overlay_base_with_symbols_no_fixups.test.dtb overlay_base_fixups.dts
+ run_test check_path overlay_base_with_symbols_no_fixups.test.dtb exists "/__symbols__"
+ run_test check_path overlay_base_with_symbols_no_fixups.test.dtb not-exists "/__fixups__"
+ run_test check_path overlay_base_with_symbols_no_fixups.test.dtb not-exists "/__local_fixups__"
+
+ # Test generation of fixups
+ run_dtc_test -F -@ -I dts -O dtb -o overlay_base_with_symbols_fixups.test.dtb overlay_base_fixups.dts
+ run_test check_path overlay_base_with_symbols_fixups.test.dtb exists "/__symbols__"
+ run_test check_path overlay_base_with_symbols_fixups.test.dtb not-exists "/__fixups__"
+ run_test check_path overlay_base_with_symbols_fixups.test.dtb exists "/__local_fixups__"
+
+ # Test generation of aliases insted of symbols
+ run_dtc_test -A -I dts -O dtb -o overlay_overlay_with_aliases.dtb overlay_overlay_dtc.dts
+ run_test check_path overlay_overlay_with_aliases.dtb exists "/aliases"
+ run_test check_path overlay_overlay_with_aliases.dtb exists "/__symbols__"
+ run_test check_path overlay_overlay_with_aliases.dtb exists "/__fixups__"
+ run_test check_path overlay_overlay_with_aliases.dtb exists "/__local_fixups__"
fi
# Bad fixup tests
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v12 5/7] overlay: Documentation for the overlay sugar syntax
From: Pantelis Antoniou @ 2016-12-05 17:05 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
In-Reply-To: <1480957528-8367-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
There exists a syntactic sugar version of overlays which
make them simpler to write for the trivial case of a single target.
Document it in the device tree object internals.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
Documentation/dt-object-internal.txt | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
index 7d76296..9219d27 100644
--- a/Documentation/dt-object-internal.txt
+++ b/Documentation/dt-object-internal.txt
@@ -306,3 +306,19 @@ the run time loader must apply an offset to each phandle in every dynamic
DT object loaded. The __local_fixups__ node records the offset relative to the
start of every local reference within that property so that the loader can apply
the offset.
+
+There is an alternative syntax to the expanded form for overlays with phandle
+targets which makes the format similar to the one using in .dtsi include files.
+
+So for the &ocp target example above one can simply write:
+
+/dts-v1/ /plugin/;
+&ocp {
+ /* bar peripheral */
+ bar {
+ compatible = "corp,bar";
+ ... /* various properties and child nodes */
+ }
+};
+
+The resulting dtb object is identical.
--
2.1.4
^ permalink raw reply related
* [PATCH v12 6/7] overlay: Add syntactic sugar version of overlays
From: Pantelis Antoniou @ 2016-12-05 17:05 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
In-Reply-To: <1480957528-8367-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
For simple overlays that use a single target there exists a
simpler syntax version.
&foo { }; generates an overlay with a single target at foo.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
dtc-parser.y | 20 +++++++++++++++++---
dtc.h | 1 +
livetree.c | 22 ++++++++++++++++++++++
3 files changed, 40 insertions(+), 3 deletions(-)
diff --git a/dtc-parser.y b/dtc-parser.y
index aaacb97..9f8f7ee 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -184,10 +184,19 @@ devicetree:
{
struct node *target = get_node_by_ref($1, $2);
- if (target)
+ if (target) {
merge_nodes(target, $3);
- else
- ERROR(&@2, "Label or path %s not found", $2);
+ } else {
+ /*
+ * We rely on the rule being always:
+ * versioninfo plugindecl memreserves devicetree
+ * so $-1 is what we want (plugindecl)
+ */
+ if ($<flags>-1 & DTSVF_PLUGIN)
+ add_orphan_node($1, $3, $2);
+ else
+ ERROR(&@2, "Label or path %s not found", $2);
+ }
$$ = $1;
}
| devicetree DT_DEL_NODE DT_REF ';'
@@ -202,6 +211,11 @@ devicetree:
$$ = $1;
}
+ | /* empty */
+ {
+ /* build empty node */
+ $$ = name_node(build_node(NULL, NULL), "");
+ }
;
nodedef:
diff --git a/dtc.h b/dtc.h
index 6aa82eb..b0c6772 100644
--- a/dtc.h
+++ b/dtc.h
@@ -199,6 +199,7 @@ struct node *build_node_delete(void);
struct node *name_node(struct node *node, char *name);
struct node *chain_node(struct node *first, struct node *list);
struct node *merge_nodes(struct node *old_node, struct node *new_node);
+void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
void add_property(struct node *node, struct property *prop);
void delete_property_by_name(struct node *node, char *name);
diff --git a/livetree.c b/livetree.c
index 54a16ba..443429f 100644
--- a/livetree.c
+++ b/livetree.c
@@ -216,6 +216,28 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
return old_node;
}
+void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
+{
+ static unsigned int next_orphan_fragment = 0;
+ struct node *node;
+ struct property *p;
+ struct data d = empty_data;
+ char *name;
+
+ d = data_add_marker(d, REF_PHANDLE, ref);
+ d = data_append_integer(d, 0xffffffff, 32);
+
+ p = build_property("target", d);
+
+ xasprintf(&name, "fragment@%u",
+ next_orphan_fragment++);
+ name_node(new_node, "__overlay__");
+ node = build_node(p, new_node);
+ name_node(node, name);
+
+ add_child(dt, node);
+}
+
struct node *chain_node(struct node *first, struct node *list)
{
assert(first->next_sibling == NULL);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v12 7/7] tests: Add a test for overlays syntactic sugar
From: Pantelis Antoniou @ 2016-12-05 17:05 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
In-Reply-To: <1480957528-8367-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Add a single test makeing sure the &foo { }; syntax works.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
tests/run_tests.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index ee30383..04ed9c8 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -227,6 +227,12 @@ overlay_tests () {
run_test check_path overlay_overlay_with_aliases.dtb exists "/__symbols__"
run_test check_path overlay_overlay_with_aliases.dtb exists "/__fixups__"
run_test check_path overlay_overlay_with_aliases.dtb exists "/__local_fixups__"
+
+ # test simplified plugin syntax
+ run_dtc_test -@ -I dts -O dtb -o overlay_overlay_simple.dtb overlay_overlay_simple.dts
+
+ # verify non-generation of local fixups
+ run_test check_path overlay_overlay_simple.dtb not-exists "/__local_fixups__"
fi
# Bad fixup tests
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file
From: Rob Herring @ 2016-12-05 17:27 UTC (permalink / raw)
To: Serge Semin
Cc: Greg Kroah-Hartman, Srinivas Kandagatla, Andrew Lunn,
Mark Rutland, Sergey.Semin-vHJ8rsvMqnUPfZBKTuL5GA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20161205152502.GA17538@mobilestation>
On Mon, Dec 5, 2016 at 9:25 AM, Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Mon, Dec 05, 2016 at 08:46:21AM -0600, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Tue, Nov 29, 2016 at 01:38:21AM +0300, Serge Semin wrote:
>> > See cover-letter for changelog
>> >
>> > Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >
>> > ---
>> > .../devicetree/bindings/misc/idt_89hpesx.txt | 41 ++++++++++++++++++++++
>>
>> There's not a better location for this? I can't tell because you don't
>> describe what the device is.
>>
>
> The device is PCIe-switch EEPROM driver with additional debug-interface to
> access the switch CSRs. EEPROM is accesses via a separate i2c-slave
> interface of the switch.
>
> There might be another place to put the binding file in. There is a special
> location for EEPROM drivers bindings - Documentation/devicetree/bindings/eeprom/ .
> But as far as I understood from the files put in there, it's intended for
> pure EEPROM drivers only. On the other hand the directory I've chosen:
> Documentation/devicetree/bindings/misc/
> mostly intended for some unusual devices. My device isn't usual, since it
> has CSRs debug-interface as well. Additionally I've found
> eeprom-93xx46.txt binding file there, which describes EEPROM bindings.
>
> Anyway if you find the file should be placed in
> Documentation/devicetree/bindings/eeprom/ instead, I'll move it, it's not
> that a big problem.
>
>> > 1 file changed, 41 insertions(+)
>> > create mode 100644 Documentation/devicetree/bindings/misc/idt_89hpesx.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/misc/idt_89hpesx.txt b/Documentation/devicetree/bindings/misc/idt_89hpesx.txt
>> > index 0000000..469cc93
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/misc/idt_89hpesx.txt
>> > @@ -0,0 +1,41 @@
>> > +EEPROM / CSR SMBus-slave interface of IDT 89HPESx devices
>> > +
>> > +Required properties:
>> > + - compatible : should be "<manufacturer>,<type>"
>> > + Basically there is only one manufacturer: idt, but some
>> > + compatible devices may be produced in future. Following devices
>> > + are supported: 89hpes8nt2, 89hpes12nt3, 89hpes24nt6ag2,
>> > + 89hpes32nt8ag2, 89hpes32nt8bg2, 89hpes12nt12g2, 89hpes16nt16g2,
>> > + 89hpes24nt24g2, 89hpes32nt24ag2, 89hpes32nt24bg2;
>> > + 89hpes12n3, 89hpes12n3a, 89hpes24n3, 89hpes24n3a;
>> > + 89hpes32h8, 89hpes32h8g2, 89hpes48h12, 89hpes48h12g2,
>> > + 89hpes48h12ag2, 89hpes16h16, 89hpes22h16, 89hpes22h16g2,
>> > + 89hpes34h16, 89hpes34h16g2, 89hpes64h16, 89hpes64h16g2,
>> > + 89hpes64h16ag2;
>> > + 89hpes12t3g2, 89hpes24t3g2, 89hpes16t4, 89hpes4t4g2,
>> > + 89hpes10t4g2, 89hpes16t4g2, 89hpes16t4ag2, 89hpes5t5,
>> > + 89hpes6t5, 89hpes8t5, 89hpes8t5a, 89hpes24t6, 89hpes6t6g2,
>> > + 89hpes24t6g2, 89hpes16t7, 89hpes32t8, 89hpes32t8g2,
>> > + 89hpes48t12, 89hpes48t12g2.
>> > + Current implementation of the driver doesn't have any device-
>>
>> Driver capabilties are irrelevant to bindings.
>>
>
> Why? I've told in the comment, that the devices actually differ by the CSRs
> map. Even though it's not reflected in the code at the moment, the CSRs
> read/write restrictions can be added by some concerned programmer in
> future. But If I left something like "compatible : idt,89hpesx" device
> only, it will be problematic to add that functionality.
Bindings describe the h/w, not what the Linux, FreeBSD, etc. driver
does. You don't want to be changing the binding doc when the driver
changes.
> Howbeit If you think it's not necessary and "compatible = idt,89hpesx" is
> ok, it's perfectly fine for me to make it this way. The property will be
> even simpler, than current approach.
NO! That's not at all what I'm suggesting. Specific compatible strings
are the right way to go for the reasons you give. You just don't need
to state why here (because it is true for all bindings).
>> > + specific functionalities. But since each of them differs
>> > + by registers mapping, CSRs read/write restrictions can be
>> > + added in future.
>> > + - reg : I2C address of the IDT 89HPES device.
>> > +
>> > +Optional properties:
>> > + - read-only : Parameterless property disables writes to the EEPROM
>> > + - idt,eesize : Size of EEPROM device connected to IDT 89HPES i2c-master bus
>> > + (default value is 4096 bytes if option isn't specified)
>> > + - idt,eeaddr : Custom address of EEPROM device
>> > + (If not specified IDT 89HPESx device will try to communicate
>> > + with EEPROM sited by default address - 0x50)
>>
>> Don't we already have standard EEPROM properties that could be used
>> here?
>>
>
> If we do, just tell me which one. There are standard options:
You can grep thru bindings as easily as I can. I can't do that for
everyone's binding.
> "compatible, reg, pagesize, read-only". There isn't any connected with
> EEPROM actual size.
> Why so? Because standard EEPROM-drivers determine the device size from the
> compatible-string name. Such approach won't work in this case, because
> PCIe-switch and it EEPROM are actually two different devices. Look at the
> chain of the usual platform board design:
> Host <--- i2c ----> i2c-slave iface |PCIe-switch| i2c-master iface <--- i2c ---> EEPROM
>
> As you cas see the Host reaches EEPROM through the set of PCIe-switch
> i2c-interfaces. In order to properly get data from it my driver needs actual
> EEPROM size and it address in the i2c-master bus of the PCIe-switch, in
> addition to the standard reg-field, which is address of PCIe-switch i2c-slave
> interface and read-only parameter if EEPROM-device has got WP pin asserted.
Ah, this needs to be much different than I thought. You need to model
(i.e. use the same binding) the EEPROM node just like it was directly
attached to the host. So this means you need the 2nd i2c bus modeled
which means you need the PCIe switch modeled. A rough outline of the
nodes would look like this:
host-i2c: i2c {
compatible ="host-i2c"
};
pcie {
pcie-switch {
i2c-bus = <&host-i2c>;
i2c-bus {
eeprom@50 {
};
};
};
};
So this models the PCIe switch as a PCIe device, it has a phandle back
to it's controller since it's not a child of the i2c controller. Then
the devices on switches i2c bus are modeled as children of the switch.
Alternatively, it could be described all as children of host-i2c node.
It's common for i2c devices to have downstream i2c buses. I2C muxes
are one example and there are bindings defined for all this. There's
also chips like mpu-6050 that have slave buses.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v5 0/2] Add support for Omnivision OV5647
From: Ramiro Oliveira @ 2016-12-05 17:36 UTC (permalink / raw)
To: mchehab-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
dheitmueller-eb9eJ82Ua7k9XoPSrs7Ehg,
slongerbeam-Re5JQEeQqe8AvxtiuMwx3w, lars-Qo5EllUWu/uELgA04lAiVw,
robert.jarzmik-GANU6spQydw, pavel-+ZI9xUNit7I,
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
sakari.ailus-VuQAYsv1563Yd54FQh9/CA, mark.rutland-5wv7dgnIgG8,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
Hello,
This patch adds support for the Omnivision OV5647 sensor.
At the moment it only supports 640x480 in Raw 8.
This is the fifth version of the OV5647 camera driver patchset.
v5:
- Refactor code
- Change comments
- Add missing error handling in some functions
v4:
- Add correct license
- Revert debugging info to generic infrastructure
- Turn defines into enums
- Correct code style issues
- Remove unused defines
- Make sure all errors where being handled
- Rename some functions to make code more readable
- Add some debugging info
v3:
- No changes. Re-submitted due to lack of responses
v2:
- Corrections in DT documentation
Ramiro Oliveira (2):
Add OV5647 device tree documentation
Add support for OV5647 sensor
.../devicetree/bindings/media/i2c/ov5647.txt | 19 +
MAINTAINERS | 7 +
drivers/media/i2c/Kconfig | 12 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/ov5647.c | 866 +++++++++++++++++++++
5 files changed, 905 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
create mode 100644 drivers/media/i2c/ov5647.c
--
2.10.2
--
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 v5 1/2] Add OV5647 device tree documentation
From: Ramiro Oliveira @ 2016-12-05 17:36 UTC (permalink / raw)
To: mchehab-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
dheitmueller-eb9eJ82Ua7k9XoPSrs7Ehg,
slongerbeam-Re5JQEeQqe8AvxtiuMwx3w, lars-Qo5EllUWu/uELgA04lAiVw,
robert.jarzmik-GANU6spQydw, pavel-+ZI9xUNit7I,
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
sakari.ailus-VuQAYsv1563Yd54FQh9/CA, mark.rutland-5wv7dgnIgG8,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <cover.1480958609.git.roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
Add device tree documentation.
Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
---
.../devicetree/bindings/media/i2c/ov5647.txt | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
new file mode 100644
index 0000000..4c91b3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
@@ -0,0 +1,19 @@
+Omnivision OV5647 raw image sensor
+---------------------------------
+
+OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
+and CCI (I2C compatible) control bus.
+
+Required properties:
+
+- compatible : "ovti,ov5647";
+- reg : I2C slave address of the sensor;
+
+The common video interfaces bindings (see video-interfaces.txt) should be
+used to specify link to the image data receiver. The OV5647 device
+node should contain one 'port' child node with an 'endpoint' subnode.
+
+Following properties are valid for the endpoint node:
+
+- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
+ video-interfaces.txt. The sensor supports only two data lanes.
--
2.10.2
--
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 v5 2/2] Add support for OV5647 sensor
From: Ramiro Oliveira @ 2016-12-05 17:36 UTC (permalink / raw)
To: mchehab-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
dheitmueller-eb9eJ82Ua7k9XoPSrs7Ehg,
slongerbeam-Re5JQEeQqe8AvxtiuMwx3w, lars-Qo5EllUWu/uELgA04lAiVw,
robert.jarzmik-GANU6spQydw, pavel-+ZI9xUNit7I,
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
sakari.ailus-VuQAYsv1563Yd54FQh9/CA, mark.rutland-5wv7dgnIgG8,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <cover.1480958609.git.roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
Add support for OV5647 sensor.
Modes supported:
- 640x480 RAW 8
Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
---
MAINTAINERS | 7 +
drivers/media/i2c/Kconfig | 12 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/ov5647.c | 866 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 886 insertions(+)
create mode 100644 drivers/media/i2c/ov5647.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 52cc077..72e828a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8923,6 +8923,13 @@ M: Harald Welte <laforge-TgoAw6mPHtdg9hUCZPvPmw@public.gmane.org>
S: Maintained
F: drivers/char/pcmcia/cm4040_cs.*
+OMNIVISION OV5647 SENSOR DRIVER
+M: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
+L: linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/i2c/ov5647.c
+
OMNIVISION OV7670 SENSOR DRIVER
M: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
L: linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index b31fa6f..c1b78e5 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -531,6 +531,18 @@ config VIDEO_OV2659
To compile this driver as a module, choose M here: the
module will be called ov2659.
+config VIDEO_OV5647
+ tristate "OmniVision OV5647 sensor support"
+ depends on OF
+ depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+ depends on MEDIA_CAMERA_SUPPORT
+ ---help---
+ This is a Video4Linux2 sensor-level driver for the OmniVision
+ OV5647 camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ov5647.
+
config VIDEO_OV7640
tristate "OmniVision OV7640 sensor support"
depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 92773b2..0d9014c 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -82,3 +82,4 @@ obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o
obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
+obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
new file mode 100644
index 0000000..2aae806
--- /dev/null
+++ b/drivers/media/i2c/ov5647.c
@@ -0,0 +1,866 @@
+/*
+ * A V4L2 driver for OmniVision OV5647 cameras.
+ *
+ * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver
+ * Copyright (C) 2011 Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
+ *
+ * Based on Omnivision OV7670 Camera Driver
+ * Copyright (C) 2006-7 Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
+ *
+ * Copyright (C) 2016, Synopsys, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed .as is. WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/videodev2.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-mediabus.h>
+#include <media/v4l2-image-sizes.h>
+#include <media/v4l2-of.h>
+#include <linux/io.h>
+
+#define SENSOR_NAME "ov5647"
+
+#define OV5647_SW_RESET 0x1003
+#define OV5647_REG_CHIPID_H 0x300A
+#define OV5647_REG_CHIPID_L 0x300B
+
+#define REG_TERM 0xfffe
+#define VAL_TERM 0xfe
+#define REG_DLY 0xffff
+
+#define OV5647_ROW_START 0x01
+#define OV5647_ROW_START_MIN 0
+#define OV5647_ROW_START_MAX 2004
+#define OV5647_ROW_START_DEF 54
+
+#define OV5647_COLUMN_START 0x02
+#define OV5647_COLUMN_START_MIN 0
+#define OV5647_COLUMN_START_MAX 2750
+#define OV5647_COLUMN_START_DEF 16
+
+#define OV5647_WINDOW_HEIGHT 0x03
+#define OV5647_WINDOW_HEIGHT_MIN 2
+#define OV5647_WINDOW_HEIGHT_MAX 2006
+#define OV5647_WINDOW_HEIGHT_DEF 1944
+
+#define OV5647_WINDOW_WIDTH 0x04
+#define OV5647_WINDOW_WIDTH_MIN 2
+#define OV5647_WINDOW_WIDTH_MAX 2752
+#define OV5647_WINDOW_WIDTH_DEF 2592
+
+struct regval_list {
+ u16 addr;
+ u8 data;
+};
+
+struct cfg_array {
+ struct regval_list *regs;
+ int size;
+};
+
+struct sensor_win_size {
+ int width;
+ int height;
+ unsigned int hoffset;
+ unsigned int voffset;
+ unsigned int hts;
+ unsigned int vts;
+ unsigned int pclk;
+ unsigned int mipi_bps;
+ unsigned int fps_fixed;
+ unsigned int bin_factor;
+ unsigned int intg_min;
+ unsigned int intg_max;
+ void *regs;
+ int regs_size;
+ int (*set_size)(struct v4l2_subdev *sd);
+};
+
+
+struct ov5647 {
+ struct device *dev;
+ struct v4l2_subdev sd;
+ struct media_pad pad;
+ struct mutex lock;
+ struct v4l2_mbus_framefmt format;
+ struct sensor_format_struct *fmt;
+ unsigned int width;
+ unsigned int height;
+ unsigned int capture_mode;
+ int hue;
+ struct v4l2_fract tpf;
+ struct sensor_win_size *current_wins;
+};
+
+static inline struct ov5647 *to_state(struct v4l2_subdev *sd)
+{
+ return container_of(sd, struct ov5647, sd);
+}
+
+static struct regval_list sensor_oe_disable_regs[] = {
+ {0x3000, 0x00},
+ {0x3001, 0x00},
+ {0x3002, 0x00},
+};
+
+static struct regval_list sensor_oe_enable_regs[] = {
+ {0x3000, 0x0f},
+ {0x3001, 0xff},
+ {0x3002, 0xe4},
+};
+
+static struct regval_list ov5647_640x480[] = {
+ {0x0100, 0x00},
+ {0x0103, 0x01},
+ {0x3034, 0x08},
+ {0x3035, 0x21},
+ {0x3036, 0x46},
+ {0x303c, 0x11},
+ {0x3106, 0xf5},
+ {0x3821, 0x07},
+ {0x3820, 0x41},
+ {0x3827, 0xec},
+ {0x370c, 0x0f},
+ {0x3612, 0x59},
+ {0x3618, 0x00},
+ {0x5000, 0x06},
+ {0x5001, 0x01},
+ {0x5002, 0x41},
+ {0x5003, 0x08},
+ {0x5a00, 0x08},
+ {0x3000, 0x00},
+ {0x3001, 0x00},
+ {0x3002, 0x00},
+ {0x3016, 0x08},
+ {0x3017, 0xe0},
+ {0x3018, 0x44},
+ {0x301c, 0xf8},
+ {0x301d, 0xf0},
+ {0x3a18, 0x00},
+ {0x3a19, 0xf8},
+ {0x3c01, 0x80},
+ {0x3b07, 0x0c},
+ {0x380c, 0x07},
+ {0x380d, 0x68},
+ {0x380e, 0x03},
+ {0x380f, 0xd8},
+ {0x3814, 0x31},
+ {0x3815, 0x31},
+ {0x3708, 0x64},
+ {0x3709, 0x52},
+ {0x3808, 0x02},
+ {0x3809, 0x80},
+ {0x380a, 0x01},
+ {0x380b, 0xE0},
+ {0x3801, 0x00},
+ {0x3802, 0x00},
+ {0x3803, 0x00},
+ {0x3804, 0x0a},
+ {0x3805, 0x3f},
+ {0x3806, 0x07},
+ {0x3807, 0xa1},
+ {0x3811, 0x08},
+ {0x3813, 0x02},
+ {0x3630, 0x2e},
+ {0x3632, 0xe2},
+ {0x3633, 0x23},
+ {0x3634, 0x44},
+ {0x3636, 0x06},
+ {0x3620, 0x64},
+ {0x3621, 0xe0},
+ {0x3600, 0x37},
+ {0x3704, 0xa0},
+ {0x3703, 0x5a},
+ {0x3715, 0x78},
+ {0x3717, 0x01},
+ {0x3731, 0x02},
+ {0x370b, 0x60},
+ {0x3705, 0x1a},
+ {0x3f05, 0x02},
+ {0x3f06, 0x10},
+ {0x3f01, 0x0a},
+ {0x3a08, 0x01},
+ {0x3a09, 0x27},
+ {0x3a0a, 0x00},
+ {0x3a0b, 0xf6},
+ {0x3a0d, 0x04},
+ {0x3a0e, 0x03},
+ {0x3a0f, 0x58},
+ {0x3a10, 0x50},
+ {0x3a1b, 0x58},
+ {0x3a1e, 0x50},
+ {0x3a11, 0x60},
+ {0x3a1f, 0x28},
+ {0x4001, 0x02},
+ {0x4004, 0x02},
+ {0x4000, 0x09},
+ {0x4837, 0x24},
+ {0x4050, 0x6e},
+ {0x4051, 0x8f},
+ {0x0100, 0x01},
+};
+
+struct sensor_format_struct;
+
+/**
+ * @short I2C Write operation
+ * @param[in] i2c_client I2C client
+ * @param[in] reg register address
+ * @param[in] val value to write
+ * @return Error code
+ */
+static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
+{
+ int ret;
+ unsigned char data[3] = { reg >> 8, reg & 0xff, val};
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+ ret = i2c_master_send(client, data, 3);
+ if (ret != 3) {
+ dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
+ __func__, reg);
+ return ret < 0 ? ret : -EIO;
+ }
+ return 0;
+}
+
+/**
+ * @short I2C Read operation
+ * @param[in] i2c_client I2C client
+ * @param[in] reg register address
+ * @param[out] val value read
+ * @return Error code
+ */
+static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
+{
+ int ret;
+ unsigned char data_w[2] = { reg >> 8, reg & 0xff };
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+
+ ret = i2c_master_send(client, data_w, 2);
+
+ if (ret < 2) {
+ dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
+ __func__, reg);
+ return ret < 0 ? ret : -EIO;
+ }
+
+
+ ret = i2c_master_recv(client, val, 1);
+
+ if (ret < 1) {
+ dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
+ __func__, reg);
+ return ret < 0 ? ret : -EIO;
+ }
+ return 0;
+}
+
+static int ov5647_write_array(struct v4l2_subdev *sd,
+ struct regval_list *regs, int array_size)
+{
+ int i = 0;
+ int ret = 0;
+
+ if (!regs)
+ return -EINVAL;
+
+ while (i < array_size) {
+ ret = ov5647_write(sd, regs->addr, regs->data);
+ if (ret < 0)
+ return ret;
+ i++;
+ regs++;
+ }
+ return 0;
+}
+
+static void ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
+{
+ u8 channel_id;
+
+ ov5647_read(sd, 0x4814, &channel_id);
+ channel_id &= ~(3 << 6);
+ ov5647_write(sd, 0x4814, channel_id | (channel << 6));
+}
+
+void ov5647_stream_on(struct v4l2_subdev *sd)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+ ov5647_write(sd, 0x4202, 0x00);
+ dev_dbg(&client->dev, "Stream on");
+ ov5647_write(sd, 0x300D, 0x00);
+}
+
+void ov5647_stream_off(struct v4l2_subdev *sd)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+ ov5647_write(sd, 0x4202, 0x0f);
+ dev_dbg(&client->dev, "Stream off");
+ ov5647_write(sd, 0x300D, 0x01);
+}
+
+/****************************************************************************/
+
+/**
+ * @short Set SW standby
+ * @param[in] sd v4l2 sd
+ * @param[in] stanby standby mode status (on or off)
+ * @return Error code
+ */
+static int set_sw_standby(struct v4l2_subdev *sd, bool standby)
+{
+ int ret;
+ unsigned char rdval;
+
+ ret = ov5647_read(sd, 0x0100, &rdval);
+ if (ret != 0)
+ return ret;
+
+ if (standby)
+ rdval &= 0xfe;
+ else
+ rdval |= 0x01;
+
+ ret = ov5647_write(sd, 0x0100, rdval);
+
+ return ret;
+}
+
+/**
+ * @short Store information about the video data format.
+ */
+static struct sensor_format_struct {
+ u8 *desc;
+ u32 mbus_code;
+ struct regval_list *regs;
+ int regs_size;
+ int bpp;
+} sensor_formats[] = {
+ {
+ .desc = "Raw RGB Bayer",
+ .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
+ .regs = ov5647_640x480,
+ .regs_size = ARRAY_SIZE(ov5647_640x480),
+ .bpp = 1
+ },
+};
+#define N_FMTS ARRAY_SIZE(sensor_formats)
+
+/* ----------------------------------------------------------------------- */
+
+/**
+ * @short Initialize sensor
+ * @param[in] sd v4l2 subdev
+ * @param[in] val not used
+ * @return Error code
+ */
+static int __sensor_init(struct v4l2_subdev *sd)
+{
+ int ret;
+ u8 resetval;
+ u8 rdval;
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+ dev_dbg(&client->dev, "sensor init\n");
+
+ ret = ov5647_read(sd, 0x0100, &rdval);
+ if (ret != 0)
+ return ret;
+
+ ov5647_write(sd, 0x4800, 0x25);
+ ov5647_stream_off(sd);
+
+ ret = ov5647_write_array(sd, ov5647_640x480,
+ ARRAY_SIZE(ov5647_640x480));
+ if (ret < 0) {
+ dev_err(&client->dev, "write sensor_default_regs error\n");
+ return ret;
+ }
+
+ ov5647_set_virtual_channel(sd, 0);
+
+ ov5647_read(sd, 0x0100, &resetval);
+ if (!(resetval & 0x01)) {
+ dev_err(&client->dev, "Device was in SW standby");
+ ov5647_write(sd, 0x0100, 0x01);
+ }
+
+ ov5647_write(sd, 0x4800, 0x04);
+ ov5647_stream_on(sd);
+
+ return 0;
+}
+
+/**
+ * @short Control sensor power state
+ * @param[in] sd v4l2 subdev
+ * @param[in] on Sensor power
+ * @return Error code
+ */
+static int sensor_power(struct v4l2_subdev *sd, int on)
+{
+ int ret;
+ struct ov5647 *ov5647 = to_state(sd);
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+ ret = 0;
+ mutex_lock(&ov5647->lock);
+
+ if (on) {
+ dev_dbg(&client->dev, "OV5647 power on\n");
+
+ ret = ov5647_write_array(sd, sensor_oe_enable_regs,
+ ARRAY_SIZE(sensor_oe_enable_regs));
+
+ ret = __sensor_init(sd);
+
+ if (ret < 0)
+ dev_err(&client->dev,
+ "Camera not available, check Power\n");
+ } else {
+ dev_dbg(&client->dev, "OV5647 power off\n");
+
+ dev_dbg(&client->dev, "disable oe\n");
+ ret = ov5647_write_array(sd, sensor_oe_disable_regs,
+ ARRAY_SIZE(sensor_oe_disable_regs));
+
+ if (ret < 0)
+ dev_dbg(&client->dev, "disable oe failed\n");
+
+ ret = set_sw_standby(sd, true);
+
+ if (ret < 0)
+ dev_dbg(&client->dev, "soft stby failed\n");
+
+ }
+
+ mutex_unlock(&ov5647->lock);
+
+ return ret;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+/**
+ * @short Get register value
+ * @param[in] sd v4l2 subdev
+ * @param[in] reg register struct
+ * @return Error code
+ */
+static int sensor_get_register(struct v4l2_subdev *sd,
+ struct v4l2_dbg_register *reg)
+{
+ unsigned char val = 0;
+ int ret;
+
+ ret = ov5647_read(sd, reg->reg & 0xff, &val);
+ if (ret != 0)
+ return ret;
+
+ reg->val = val;
+ reg->size = 1;
+
+ return ret;
+}
+
+/**
+ * @short Set register value
+ * @param[in] sd v4l2 subdev
+ * @param[in] reg register struct
+ * @return Error code
+ */
+static int sensor_set_register(struct v4l2_subdev *sd,
+ const struct v4l2_dbg_register *reg)
+{
+ return ov5647_write(sd, reg->reg & 0xff, reg->val & 0xff);
+}
+#endif
+
+/* ----------------------------------------------------------------------- */
+
+/**
+ * @short Subdev core operations registration
+ */
+static const struct v4l2_subdev_core_ops sensor_core_ops = {
+ .s_power = sensor_power,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+ .g_register = sensor_get_register,
+ .s_register = sensor_set_register,
+#endif
+};
+
+/* ----------------------------------------------------------------------- */
+
+
+
+/**
+ * @short Enumerate available image formats
+ * @param[in] sd v4l2 subdev
+ * @param[in] index index
+ * @param[in] code MBUS Pixel code
+ * @return Error code
+ */
+static int sensor_enum_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ if (code->pad || code->index >= N_FMTS)
+ return -EINVAL;
+
+ code->code = sensor_formats[code->index].mbus_code;
+ return 0;
+}
+
+/**
+ * @short Try frame format internal function
+ * @param[in] sd v4l2 subdev
+ * @param[in] fmt frame format
+ * @return Error code
+ */
+static int sensor_try_fmt_internal(struct v4l2_subdev *sd,
+ struct v4l2_mbus_framefmt *fmt, struct sensor_format_struct **ret_fmt,
+ struct sensor_win_size **ret_wsize)
+{
+ int index;
+
+ for (index = 0; index < N_FMTS; index++)
+ if (sensor_formats[index].mbus_code == fmt->code)
+ break;
+
+ if (index >= N_FMTS)
+ return -EINVAL;
+
+ if (ret_fmt != NULL)
+ *ret_fmt = sensor_formats + index;
+
+ fmt->field = V4L2_FIELD_NONE;
+
+ return 0;
+}
+
+/**
+ * @short Set frame format
+ * @param[in] sd v4l2 subdev
+ * @param[in] fmt frame format
+ * @return Error code
+ */
+static int sensor_s_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *fmt)
+{
+ int ret;
+ struct sensor_format_struct *sensor_fmt;
+ struct sensor_win_size *wsize;
+ struct ov5647 *info = to_state(sd);
+
+ ov5647_write_array(sd, sensor_oe_disable_regs,
+ ARRAY_SIZE(sensor_oe_disable_regs));
+
+ ret = sensor_try_fmt_internal(sd, &fmt->format,
+ &sensor_fmt, &wsize);
+ if (ret)
+ return ret;
+
+ ov5647_write_array(sd, sensor_fmt->regs, sensor_fmt->regs_size);
+
+ ret = 0;
+
+ if (wsize->regs)
+ ov5647_write_array(sd, wsize->regs, wsize->regs_size);
+
+ if (wsize->set_size)
+ wsize->set_size(sd);
+
+ info->fmt = sensor_fmt;
+ info->width = wsize->width;
+ info->height = wsize->height;
+
+ ov5647_write_array(sd, sensor_oe_enable_regs,
+ ARRAY_SIZE(sensor_oe_enable_regs));
+
+ return 0;
+}
+
+/**
+ * @short Set stream parameters
+ * @param[in] sd v4l2 subdev
+ * @param[in] parms stream parameters
+ * @return Error code
+ */
+static int sensor_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
+{
+ struct v4l2_captureparm *cp = &parms->parm.capture;
+ struct ov5647 *info = to_state(sd);
+
+ if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+
+ if (info->tpf.numerator == 0)
+ return -EINVAL;
+
+ info->capture_mode = cp->capturemode;
+
+ return 0;
+}
+
+/**
+ * @short Get stream parameters
+ * @param[in] sd v4l2 subdev
+ * @param[in] parms stream parameters
+ * @return Error code
+ */
+static int sensor_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
+{
+ struct v4l2_captureparm *cp = &parms->parm.capture;
+ struct ov5647 *info = to_state(sd);
+
+ if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+
+ memset(cp, 0, sizeof(struct v4l2_captureparm));
+ cp->capability = V4L2_CAP_TIMEPERFRAME;
+ cp->capturemode = info->capture_mode;
+
+ return 0;
+}
+
+/**
+ * @short Subdev video operations registration
+ *
+ */
+static const struct v4l2_subdev_video_ops sensor_video_ops = {
+ .s_parm = sensor_s_parm,
+ .g_parm = sensor_g_parm,
+};
+
+/* ----------------------------------------------------------------------- */
+
+/**
+ * @short Subdev operations registration
+ *
+ */
+static const struct v4l2_subdev_ops subdev_ops = {
+ .core = &sensor_core_ops,
+ .video = &sensor_video_ops,
+};
+
+/* -----------------------------------------------------------------------------
+ * V4L2 subdev internal operations
+ */
+
+/**
+ * @short Detect camera version and model
+ * @param[in] sd v4l2 subdev
+ * @return Error code
+ */
+int ov5647_detect(struct v4l2_subdev *sd)
+{
+ unsigned char v;
+ int ret;
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+ ret = ov5647_write(sd, OV5647_SW_RESET, 0x01);
+ if (ret < 0)
+ return ret;
+ ret = ov5647_read(sd, OV5647_REG_CHIPID_H, &v);
+ if (ret < 0)
+ return ret;
+ if (v != 0x56) {
+ dev_err(&client->dev, "Wrong model version detected");
+ return -ENODEV;
+ }
+ ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &v);
+ if (ret < 0)
+ return ret;
+ if (v != 0x47) {
+ dev_err(&client->dev, "Wrong model version detected");
+ return -ENODEV;
+ }
+
+ ret = ov5647_write(sd, OV5647_SW_RESET, 0x00);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+/**
+ * @short Detect if camera is registered
+ * @param[in] sd v4l2 subdev
+ * @return Error code
+ */
+static int ov5647_registered(struct v4l2_subdev *sd)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+ dev_info(&client->dev, "OV5647 detected at address 0x%02x\n",
+ client->addr);
+
+ return 0;
+}
+
+/**
+ * @short Open device
+ * @param[in] sd v4l2 subdev
+ * @param[in] fh v4l2 file handler
+ * @return Error code
+ */
+static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ struct v4l2_mbus_framefmt *format =
+ v4l2_subdev_get_try_format(sd, fh->pad, 0);
+ struct v4l2_rect *crop =
+ v4l2_subdev_get_try_crop(sd, fh->pad, 0);
+
+ crop->left = OV5647_COLUMN_START_DEF;
+ crop->top = OV5647_ROW_START_DEF;
+ crop->width = OV5647_WINDOW_WIDTH_DEF;
+ crop->height = OV5647_WINDOW_HEIGHT_DEF;
+
+ format->code = MEDIA_BUS_FMT_SBGGR8_1X8;
+
+ format->width = OV5647_WINDOW_WIDTH_DEF;
+ format->height = OV5647_WINDOW_HEIGHT_DEF;
+ format->field = V4L2_FIELD_NONE;
+ format->colorspace = V4L2_COLORSPACE_SRGB;
+
+ return sensor_power(sd, true);
+}
+
+/**
+ * @short Open device
+ * @param[in] sd v4l2 subdev
+ * @param[in] fh v4l2 file handler
+ * @return Error code
+ */
+static int ov5647_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ return sensor_power(sd, false);
+}
+
+/**
+ * @short Subdev internal operations registration
+ *
+ */
+static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
+ .registered = ov5647_registered,
+ .open = ov5647_open,
+ .close = ov5647_close,
+};
+
+/**
+ * @short Initialization routine - Entry point of the driver
+ * @param[in] client pointer to the i2c client structure
+ * @param[in] id pointer to the i2c device id structure
+ * @return 0 on success and a negative number on failure
+ */
+static int ov5647_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct ov5647 *sensor;
+ int ret = 0;
+ struct v4l2_subdev *sd;
+
+ dev_info(&client->dev, "Installing OmniVision OV5647 camera driver\n");
+
+ sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+ if (sensor == NULL)
+ return -ENOMEM;
+
+ mutex_init(&sensor->lock);
+ sensor->dev = dev;
+
+ sd = &sensor->sd;
+ v4l2_i2c_subdev_init(sd, client, &subdev_ops);
+ sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+ sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+ sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
+ ret = media_entity_pads_init(&sd->entity, 1, &sensor->pad);
+ if (ret < 0)
+ goto mutex_remove;
+
+ ret = ov5647_detect(sd);
+ if (ret < 0)
+ goto error;
+
+ ret = v4l2_async_register_subdev(sd);
+ if (ret < 0)
+ goto error;
+
+ return 0;
+error:
+ media_entity_cleanup(&sd->entity);
+mutex_remove:
+ mutex_destroy(&sensor->lock);
+ return ret;
+}
+
+/**
+ * @short Exit routine - Exit point of the driver
+ * @param[in] client pointer to the i2c client structure
+ * @return 0 on success and a negative number on failure
+ */
+static int ov5647_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct ov5647 *ov5647 = to_state(sd);
+
+ v4l2_async_unregister_subdev(&ov5647->sd);
+ media_entity_cleanup(&ov5647->sd.entity);
+ v4l2_device_unregister_subdev(sd);
+ mutex_destroy(&ov5647->lock);
+
+ return 0;
+}
+
+static const struct i2c_device_id ov5647_id[] = {
+ { "ov5647", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, ov5647_id);
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id ov5647_of_match[] = {
+ { .compatible = "ovti,ov5647" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ov5647_of_match);
+#endif
+
+/**
+ * @short i2c driver structure
+ */
+static struct i2c_driver ov5647_driver = {
+ .driver = {
+ .of_match_table = of_match_ptr(ov5647_of_match),
+ .owner = THIS_MODULE,
+ .name = "ov5647",
+ },
+ .probe = ov5647_probe,
+ .remove = ov5647_remove,
+ .id_table = ov5647_id,
+};
+
+module_i2c_driver(ov5647_driver);
+
+MODULE_AUTHOR("Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>");
+MODULE_DESCRIPTION("A low-level driver for OmniVision ov5647 sensors");
+MODULE_LICENSE("GPL v2");
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 1/6] net: ethernet: ti: netcp: add support of cpts
From: Grygorii Strashko @ 2016-12-05 18:25 UTC (permalink / raw)
To: Rob Herring
Cc: David S. Miller, netdev, Mugunthan V N, Richard Cochran,
Sekhar Nori, linux-kernel, linux-omap, devicetree,
Murali Karicheri, Wingman Kwok
In-Reply-To: <20161205144918.oj4jpj65aha3x5gf@rob-hp-laptop>
On 12/05/2016 08:49 AM, Rob Herring wrote:
> On Mon, Nov 28, 2016 at 05:04:23PM -0600, Grygorii Strashko wrote:
>> From: WingMan Kwok <w-kwok2@ti.com>
>>
>> This patch adds support of the cpts device found in the
>> gbe and 10gbe ethernet switches on the keystone 2 SoCs
>> (66AK2E/L/Hx, 66AK2Gx).
>>
>> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> .../devicetree/bindings/net/keystone-netcp.txt | 9 +
>> drivers/net/ethernet/ti/Kconfig | 7 +-
>> drivers/net/ethernet/ti/netcp.h | 2 +-
>> drivers/net/ethernet/ti/netcp_core.c | 18 +-
>> drivers/net/ethernet/ti/netcp_ethss.c | 437 ++++++++++++++++++++-
>> 5 files changed, 459 insertions(+), 14 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/keystone-netcp.txt b/Documentation/devicetree/bindings/net/keystone-netcp.txt
>> index 04ba1dc..c37b54e 100644
>> --- a/Documentation/devicetree/bindings/net/keystone-netcp.txt
>> +++ b/Documentation/devicetree/bindings/net/keystone-netcp.txt
>> @@ -113,6 +113,15 @@ Optional properties:
>> will only initialize these ports and attach PHY
>> driver to them if needed.
>>
>> + Properties related to cpts configurations.
>> + - cpts_clock_mult/cpts_clock_shift:
>
> Needs vendor prefix. Don't use '_'.
This module is used as part of OMAP and Keystone SoCs, so names for
this props is ABI already :(
>
>> + used for converting time counter cycles to ns as in
>> +
>> + ns = (cycles * clock_mult) >> _shift
>> +
>> + Defaults: clock_mult, clock_shift = calculated from
>> + CPTS refclk
>
> What does this mean?
>
I'll add more description here.
>> +
>> NetCP interface properties: Interface specification for NetCP sub-modules.
>> Required properties:
>> - rx-channel: the navigator packet dma channel name for rx.
--
regards,
-grygorii
^ permalink raw reply
* Re: [PATCH v3 00/13] net: ethernet: ti: cpts: update and fixes
From: Grygorii Strashko @ 2016-12-05 18:31 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, Mugunthan V N,
Sekhar Nori, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA, Murali Karicheri, Wingman Kwok,
Thomas Gleixner
In-Reply-To: <20161203092211.GA2762@netboy>
On 12/03/2016 03:22 AM, Richard Cochran wrote:
> On Fri, Dec 02, 2016 at 02:30:10PM -0600, Grygorii Strashko wrote:
>> It is preparation series intended to clean up and optimize TI CPTS driver to
>> facilitate further integration with other TI's SoCs like Keystone 2.
>>
>> Changes in v3:
>> - patches reordered: fixes and small updates moved first
>> - added comments in code about cpts->cc_mult
>> - conversation range (maxsec) limited to 10sec
>
> On net-next:
>
> $ git am ~/grygorii.strashko
> Applying: net: ethernet: ti: cpts: switch to readl/writel_relaxed()
> Applying: net: ethernet: ti: allow cpts to be built separately
> error: patch failed: drivers/net/ethernet/ti/cpsw.c:1963
> error: drivers/net/ethernet/ti/cpsw.c: patch does not apply
> Patch failed at 0002 net: ethernet: ti: allow cpts to be built separately
Sorry for that, also there build error due to patch reordering :(
>
> Also, you have the order of the SOB tags wrong. The author's SOB goes
> first.
Will fix and resend, sorry again.
--
regards,
-grygorii
--
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] of/irq: improve error report on irq discovery process failure
From: Guilherme G. Piccoli @ 2016-12-05 19:01 UTC (permalink / raw)
To: Rob Herring
Cc: linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
linuxppc-dev, Mark Rutland, Benjamin Herrenschmidt, Frank Rowand,
Marc Zyngier
In-Reply-To: <CAL_JsqKg4+SANVuhfjFQRrC4Ax7OhAjwuBpo0Xes9UFevFAdWg@mail.gmail.com>
On 12/05/2016 12:28 PM, Rob Herring wrote:
> On Mon, Dec 5, 2016 at 7:59 AM, Guilherme G. Piccoli
> <gpiccoli@linux.vnet.ibm.com> wrote:
>> On PowerPC machines some PCI slots might not have level triggered
>> interrupts capability (also know as level signaled interrupts),
>> leading of_irq_parse_pci() to complain by presenting error messages
>> on the kernel log - in this case, the properties "interrupt-map" and
>> "interrupt-map-mask" are not present on device's node in the device
>> tree.
>>
>> This patch introduces a different message for this specific case,
>> and also reduces its level from error to warning. Besides, we warn
>> (once) that possibly some PCI slots on the system have no level
>> triggered interrupts available.
>> We changed some error return codes too on function of_irq_parse_raw()
>> in order other failure's cases can be presented in a more precise way.
>>
>> Before this patch, when an adapter was plugged in a slot without level
>> interrupts capabilitiy on PowerPC, we saw a generic error message
>> like this:
>>
>> [54.239] pci 002d:70:00.0: of_irq_parse_pci() failed with rc=-22
>>
>> Now, with this applied, we see the following specific message:
>>
>> [16.154] pci 0014:60:00.1: of_irq_parse_pci: no interrupt-map found,
>> INTx interrupts not available
>>
>> Finally, we standardize the error path in of_irq_parse_raw() by always
>> taking the fail path instead of returning directly from the loop.
>>
>> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
>> ---
>>
>> v2:
>> * Changed function return code to always return negative values;
>
> Are you sure this is safe? This is tricky because of differing values
> of NO_IRQ (0 or -1).
Thanks Rob, but this is purely bad wording from myself. I'm sorry - I
meant to say that I changed only my positive return code (that was
suggested to be removed in the prior revision) to negative return code!
So, I changed only code I added myself in v1 =)
>
>> * Improved/simplified warning outputs;
>> * Changed some return codes and some error paths in of_irq_parse_raw()
>> in order to be more precise/consistent;
>
> This too could have some side effects on callers.
>
> Not saying don't do these changes, just need some assurances this has
> been considered.
Thanks for your attention. I performed a quick investigation before
changing this, all the places that use the return values are just
getting "true/false" information from that, meaning they just are
comparing to 0 basically. So change -EINVAL to -ENOENT wouldn't hurt any
user of these return values, it'll only become more informative IMHO.
Now, regarding the only error path that was changed: for some reason,
this was the only place in which we didn't goto fail label in case of
failure - it was added by a legacy commit from Ben, dated from 2006:
006b64de60 ("[POWERPC] Make OF irq map code detect more error cases").
Then it was carried by Grant Likely's commit 7dc2e1134a ("of/irq: merge
irq mapping code"), 6-year old commit.
I wasn't able to imagine a scenario in which changing this would break
something; I believe the change improve consistency, but I'd remove it
if you or somebody else thinks it worth be removed.
Cheers,
Guilherme
>
> Rob
>
^ permalink raw reply
* Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file
From: Serge Semin @ 2016-12-05 19:04 UTC (permalink / raw)
To: Rob Herring
Cc: Greg Kroah-Hartman, Srinivas Kandagatla, Andrew Lunn,
Mark Rutland, Sergey.Semin-vHJ8rsvMqnUPfZBKTuL5GA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAL_JsqLbMGXSRLtTiD2d3Bsu4qc4bE029gieRN8x+QxPs=VbcA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, Dec 05, 2016 at 11:27:07AM -0600, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Dec 5, 2016 at 9:25 AM, Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Mon, Dec 05, 2016 at 08:46:21AM -0600, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >> On Tue, Nov 29, 2016 at 01:38:21AM +0300, Serge Semin wrote:
> >> > See cover-letter for changelog
> >> >
> >> > Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> >
> >> > ---
> >> > .../devicetree/bindings/misc/idt_89hpesx.txt | 41 ++++++++++++++++++++++
> >>
> >> There's not a better location for this? I can't tell because you don't
> >> describe what the device is.
> >>
> >
> > The device is PCIe-switch EEPROM driver with additional debug-interface to
> > access the switch CSRs. EEPROM is accesses via a separate i2c-slave
> > interface of the switch.
> >
> > There might be another place to put the binding file in. There is a special
> > location for EEPROM drivers bindings - Documentation/devicetree/bindings/eeprom/ .
> > But as far as I understood from the files put in there, it's intended for
> > pure EEPROM drivers only. On the other hand the directory I've chosen:
> > Documentation/devicetree/bindings/misc/
> > mostly intended for some unusual devices. My device isn't usual, since it
> > has CSRs debug-interface as well. Additionally I've found
> > eeprom-93xx46.txt binding file there, which describes EEPROM bindings.
> >
> > Anyway if you find the file should be placed in
> > Documentation/devicetree/bindings/eeprom/ instead, I'll move it, it's not
> > that a big problem.
> >
What about this comment? Shall the file be left at the path I placed it?
> >> > 1 file changed, 41 insertions(+)
> >> > create mode 100644 Documentation/devicetree/bindings/misc/idt_89hpesx.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/misc/idt_89hpesx.txt b/Documentation/devicetree/bindings/misc/idt_89hpesx.txt
> >> > index 0000000..469cc93
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/misc/idt_89hpesx.txt
> >> > @@ -0,0 +1,41 @@
> >> > +EEPROM / CSR SMBus-slave interface of IDT 89HPESx devices
> >> > +
> >> > +Required properties:
> >> > + - compatible : should be "<manufacturer>,<type>"
> >> > + Basically there is only one manufacturer: idt, but some
> >> > + compatible devices may be produced in future. Following devices
> >> > + are supported: 89hpes8nt2, 89hpes12nt3, 89hpes24nt6ag2,
> >> > + 89hpes32nt8ag2, 89hpes32nt8bg2, 89hpes12nt12g2, 89hpes16nt16g2,
> >> > + 89hpes24nt24g2, 89hpes32nt24ag2, 89hpes32nt24bg2;
> >> > + 89hpes12n3, 89hpes12n3a, 89hpes24n3, 89hpes24n3a;
> >> > + 89hpes32h8, 89hpes32h8g2, 89hpes48h12, 89hpes48h12g2,
> >> > + 89hpes48h12ag2, 89hpes16h16, 89hpes22h16, 89hpes22h16g2,
> >> > + 89hpes34h16, 89hpes34h16g2, 89hpes64h16, 89hpes64h16g2,
> >> > + 89hpes64h16ag2;
> >> > + 89hpes12t3g2, 89hpes24t3g2, 89hpes16t4, 89hpes4t4g2,
> >> > + 89hpes10t4g2, 89hpes16t4g2, 89hpes16t4ag2, 89hpes5t5,
> >> > + 89hpes6t5, 89hpes8t5, 89hpes8t5a, 89hpes24t6, 89hpes6t6g2,
> >> > + 89hpes24t6g2, 89hpes16t7, 89hpes32t8, 89hpes32t8g2,
> >> > + 89hpes48t12, 89hpes48t12g2.
> >> > + Current implementation of the driver doesn't have any device-
> >>
> >> Driver capabilties are irrelevant to bindings.
> >>
> >
> > Why? I've told in the comment, that the devices actually differ by the CSRs
> > map. Even though it's not reflected in the code at the moment, the CSRs
> > read/write restrictions can be added by some concerned programmer in
> > future. But If I left something like "compatible : idt,89hpesx" device
> > only, it will be problematic to add that functionality.
>
> Bindings describe the h/w, not what the Linux, FreeBSD, etc. driver
> does. You don't want to be changing the binding doc when the driver
> changes.
>
> > Howbeit If you think it's not necessary and "compatible = idt,89hpesx" is
> > ok, it's perfectly fine for me to make it this way. The property will be
> > even simpler, than current approach.
>
> NO! That's not at all what I'm suggesting. Specific compatible strings
> are the right way to go for the reasons you give. You just don't need
> to state why here (because it is true for all bindings).
>
Oh, I just misunderstood what you said. I'll discard the comment.
> >> > + specific functionalities. But since each of them differs
> >> > + by registers mapping, CSRs read/write restrictions can be
> >> > + added in future.
> >> > + - reg : I2C address of the IDT 89HPES device.
> >> > +
> >> > +Optional properties:
> >> > + - read-only : Parameterless property disables writes to the EEPROM
> >> > + - idt,eesize : Size of EEPROM device connected to IDT 89HPES i2c-master bus
> >> > + (default value is 4096 bytes if option isn't specified)
> >> > + - idt,eeaddr : Custom address of EEPROM device
> >> > + (If not specified IDT 89HPESx device will try to communicate
> >> > + with EEPROM sited by default address - 0x50)
> >>
> >> Don't we already have standard EEPROM properties that could be used
> >> here?
> >>
> >
> > If we do, just tell me which one. There are standard options:
>
> You can grep thru bindings as easily as I can. I can't do that for
> everyone's binding.
>
It won't be necessary due to the next comment.
> > "compatible, reg, pagesize, read-only". There isn't any connected with
> > EEPROM actual size.
> > Why so? Because standard EEPROM-drivers determine the device size from the
> > compatible-string name. Such approach won't work in this case, because
> > PCIe-switch and it EEPROM are actually two different devices. Look at the
> > chain of the usual platform board design:
> > Host <--- i2c ----> i2c-slave iface |PCIe-switch| i2c-master iface <--- i2c ---> EEPROM
> >
> > As you cas see the Host reaches EEPROM through the set of PCIe-switch
> > i2c-interfaces. In order to properly get data from it my driver needs actual
> > EEPROM size and it address in the i2c-master bus of the PCIe-switch, in
> > addition to the standard reg-field, which is address of PCIe-switch i2c-slave
> > interface and read-only parameter if EEPROM-device has got WP pin asserted.
>
> Ah, this needs to be much different than I thought. You need to model
> (i.e. use the same binding) the EEPROM node just like it was directly
> attached to the host. So this means you need the 2nd i2c bus modeled
> which means you need the PCIe switch modeled. A rough outline of the
> nodes would look like this:
>
> host-i2c: i2c {
> compatible ="host-i2c"
> };
>
> pcie {
> pcie-switch {
> i2c-bus = <&host-i2c>;
> i2c-bus {
> eeprom@50 {
> };
> };
> };
> };
>
> So this models the PCIe switch as a PCIe device, it has a phandle back
> to it's controller since it's not a child of the i2c controller. Then
> the devices on switches i2c bus are modeled as children of the switch.
>
> Alternatively, it could be described all as children of host-i2c node.
> It's common for i2c devices to have downstream i2c buses. I2C muxes
> are one example and there are bindings defined for all this. There's
> also chips like mpu-6050 that have slave buses.
>
> Rob
I think I understand what you says. However let me just bring some details
to make things clear.
First of all the driver doesn't do any PCI-Express-related work. The device
!IDT PCI Express switch! just has two additional i2c interfaces: i2c-slave
and i2c-master. As it is obvious from the bus-names i2c-slave is the interface,
where IDT PCIe-switch device is actually slave. This interface can be reached
from the host by ordinary i2c buses. i2c-master interface is connected to an
i2c-bus, where IDT PCIe-switch is single master. This bus can have just one
EEPROM device to store some initialization data. Host can send some specific
smbus-packets to i2c-slave interface of IDT PCIe-switch in order to
preinitialize EEPROM data, connected to i2c-master interface of the device.
Additionally IDT PCIe-switch handles some special smbus packets coming to it
i2c-slave interface to read/write its internal CSR. This interface can be
used to debug the device, when there are problems with it usual PCI Express
related functioning.
So to speak, it wouldn't be good to have PCIe-switch declared in dts as a
PCI-device, since PCI-bus is actually dynamically populated by PCI-core
subsystem.
According to what you said and the device/driver design I described, the
following bindings can be suggested:
i2c0: i2c@FFFF0000 {
compatible = "vendor,i2c-adapter";
#address-cells = <1>;
#size-cells = <0>;
idt_i2c_iface: idt@60 {
compatible = "idt,89hpes32nt8ag2";
reg = <0x60>;
#address-cells = <1>;
#size-cells = <0>;
eeprom@51 {
compatible = "at,24c64";
reg = <0x51>;
read-only;
};
};
};
Suppose there is some host-i2c adapter like "vendor,i2c-adapter" and
i2c-slave interface of IDT PCIe-switch is connected to it. In this way
i2c-slave interface will be visible like ordinary i2c-device with just
one subnode. This subnode explains the actual EEPROM connected to
IDT PCIe-switch i2c-master interface.
Does it look acceptable? It seems like your last suggestion. Is it?
Thanks,
-Sergey
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/6] net: ethernet: ti: netcp: add support of cpts
From: Richard Cochran @ 2016-12-05 19:30 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Rob Herring, David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, devicetree, Murali Karicheri,
Wingman Kwok
In-Reply-To: <7a484f9c-a47d-3ccd-9611-d00b86feabdd@ti.com>
On Mon, Dec 05, 2016 at 12:25:57PM -0600, Grygorii Strashko wrote:
> >> --- a/Documentation/devicetree/bindings/net/keystone-netcp.txt
> >> +++ b/Documentation/devicetree/bindings/net/keystone-netcp.txt
> >> @@ -113,6 +113,15 @@ Optional properties:
> >> will only initialize these ports and attach PHY
> >> driver to them if needed.
> >>
> >> + Properties related to cpts configurations.
> >> + - cpts_clock_mult/cpts_clock_shift:
> >
> > Needs vendor prefix. Don't use '_'.
>
> This module is used as part of OMAP and Keystone SoCs, so names for
> this props is ABI already :(
Your automatic calculation makes these unnecessary, and so you can
drop these altogether.
Also, maybe you should mark them as deprecated in cpsw.txt?
(The underscores were my fault, sorry)
Thanks,
Richard
^ permalink raw reply
* [PATCH v4 00/13] net: ethernet: ti: cpts: update and fixes
From: Grygorii Strashko @ 2016-12-05 20:05 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, devicetree,
Murali Karicheri, Wingman Kwok, Thomas Gleixner,
Grygorii Strashko
It is preparation series intended to clean up and optimize TI CPTS driver to
facilitate further integration with other TI's SoCs like Keystone 2.
Changes in v4:
- fixed build error in patch
"net: ethernet: ti: cpts: clean up event list if event pool is empty"
- rebased on top of net-next
Changes in v3:
- patches reordered: fixes and small updates moved first
- added comments in code about cpts->cc_mult
- conversation range (maxsec) limited to 10sec
Changes in v2:
- patch "net: ethernet: ti: cpts: rework initialization/deinitialization"
was split on 4 patches
- applied comments from Richard Cochran
- dropped patch
"net: ethernet: ti: cpts: add return value to tx and rx timestamp funcitons"
- new patches added:
"net: ethernet: ti: cpts: drop excessive writes to CTRL and INT_EN regs"
and "clocksource: export the clocks_calc_mult_shift to use by timestamp code"
Links on prev versions:
v3: https://www.spinics.net/lists/devicetree/msg153474.html
v2: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1282034.html
v1: http://www.spinics.net/lists/linux-omap/msg131925.html
Grygorii Strashko (11):
net: ethernet: ti: cpts: switch to readl/writel_relaxed()
net: ethernet: ti: allow cpts to be built separately
net: ethernet: ti: cpsw: minimize direct access to struct cpts
net: ethernet: ti: cpts: fix unbalanced clk api usage in cpts_register/unregister
net: ethernet: ti: cpts: fix registration order
net: ethernet: ti: cpts: disable cpts when unregistered
net: ethernet: ti: cpts: drop excessive writes to CTRL and INT_EN regs
net: ethernet: ti: cpts: rework initialization/deinitialization
net: ethernet: ti: cpts: move dt props parsing to cpts driver
net: ethernet: ti: cpts: calc mult and shift from refclk freq
net: ethernet: ti: cpts: fix overflow check period
Murali Karicheri (1):
clocksource: export the clocks_calc_mult_shift to use by timestamp code
WingMan Kwok (1):
net: ethernet: ti: cpts: clean up event list if event pool is empty
Documentation/devicetree/bindings/net/cpsw.txt | 8 +-
drivers/net/ethernet/ti/Kconfig | 2 +-
drivers/net/ethernet/ti/Makefile | 3 +-
drivers/net/ethernet/ti/cpsw.c | 84 ++++-----
drivers/net/ethernet/ti/cpsw.h | 2 -
drivers/net/ethernet/ti/cpts.c | 239 +++++++++++++++++++------
drivers/net/ethernet/ti/cpts.h | 80 ++++++++-
kernel/time/clocksource.c | 1 +
8 files changed, 304 insertions(+), 115 deletions(-)
--
2.10.1
^ permalink raw reply
* [PATCH v4 01/13] net: ethernet: ti: cpts: switch to readl/writel_relaxed()
From: Grygorii Strashko @ 2016-12-05 20:05 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, devicetree,
Murali Karicheri, Wingman Kwok, Thomas Gleixner,
Grygorii Strashko
In-Reply-To: <20161205200525.16664-1-grygorii.strashko@ti.com>
Switch to readl/writel_relaxed() APIs, because this is recommended
API and the CPTS IP is reused on Keystone 2 SoCs
where LE/BE modes are supported.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Acked-by: Richard Cochran <richardcochran@gmail.com>
---
drivers/net/ethernet/ti/cpts.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 85a55b4..a42c449 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -33,8 +33,8 @@
#ifdef CONFIG_TI_CPTS
-#define cpts_read32(c, r) __raw_readl(&c->reg->r)
-#define cpts_write32(c, v, r) __raw_writel(v, &c->reg->r)
+#define cpts_read32(c, r) readl_relaxed(&c->reg->r)
+#define cpts_write32(c, v, r) writel_relaxed(v, &c->reg->r)
static int event_expired(struct cpts_event *event)
{
--
2.10.1
^ permalink raw reply related
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