| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5253FC90.1000808@overkiz.com>
Date: Tue, 08 Oct 2013 14:37:36 +0200
From: boris brezillon <b.brezillon@...rkiz.com>
To: Nicolas Ferre <nicolas.ferre@...el.com>,
Grant Likely <grant.likely@...aro.org>,
Rob Herring <rob.herring@...xeda.com>,
Rob Landley <rob@...dley.net>,
Andrew Victor <linux@...im.org.za>,
Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
Russell King <linux@....linux.org.uk>,
Mike Turquette <mturquette@...aro.org>,
Felipe Balbi <balbi@...com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Ludovic Desroches <ludovic.desroches@...el.com>,
Josh Wu <josh.wu@...el.com>,
Richard Genoud <richard.genoud@...il.com>
CC: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 17/19] clk: at91: add PMC clk device tree binding doc.
On 08/10/2013 11:44, Nicolas Ferre wrote:
> On 08/08/2013 09:19, Boris BREZILLON :
>> This patch adds new at91 clks dt bindings documentation.
>>
>> Signed-off-by: Boris BREZILLON <b.brezillon@...rkiz.com>
>> ---
>> .../devicetree/bindings/clock/at91-clock.txt | 312
>> ++++++++++++++++++++
>> 1 file changed, 312 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/clock/at91-clock.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/at91-clock.txt
>> b/Documentation/devicetree/bindings/clock/at91-clock.txt
>> new file mode 100644
>> index 0000000..04739da
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/at91-clock.txt
>> @@ -0,0 +1,312 @@
>> +Device Tree Clock bindings for arch-at91
>> +
>> +This binding uses the common clock binding[1].
>> +
>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +Required properties:
>> +- compatible : shall be one of the following:
>> + "atmel,at91rm9200-pmc" or
>> + "atmel,at91sam9g45-pmc" or
>> + "atmel,at91sam9n12-pmc" or
>> + "atmel,at91sam9x5-pmc" or
>> + "atmel,at91sam9g35-pmc" or
>
> Already said in previous patches: 9g35 is not different from the 9x5:
> it was a bug in the older datasheet.
I'll drop it.
>
>> + "atmel,sama5d3-pmc":
>> + at91 PMC (Power Management Controller)
>> + All at91 specific clocks (clocks defined below) must be child
>> + node of the PMC node.
>> +
>> + "atmel,at91rm9200-clk-main":
>> + at91 main oscillator
>> +
>> + "atmel,at91rm9200-clk-master" or
>> + "atmel,at91sam9x5-clk-master":
>> + at91 master clock
>> +
>> + "atmel,at91sam9x5-clk-peripheral" or
>> + "atmel,at91rm9200-clk-peripheral":
>> + at91 peripheral clocks
>> +
>> + "atmel,at91rm9200-clk-pll" or
>> + "atmel,at91sam9g45-clk-pll" or
>> + "atmel,at91sam9g20-clk-pllb" or
>> + "atmel,sama5d3-clk-pll":
>> + at91 pll clocks
>> +
>> + "atmel,at91sam9x5-clk-plldiv":
>> + at91 plla divisor
>> +
>> + "atmel,at91rm9200-clk-programmable" or
>> + "atmel,at91sam9g45-clk-programmable" or
>> + "atmel,at91sam9x5-clk-programmable":
>> + at91 programmable clocks
>> +
>> + "atmel,at91sam9x5-clk-smd":
>> + at91 SMD (Soft Modem) clock
>> +
>> + "atmel,at91rm9200-clk-system":
>> + at91 system clocks
>> +
>> + "atmel,at91rm9200-clk-usb" or
>> + "atmel,at91sam9x5-clk-usb":
>> + at91 usb clock
>> +
>> + "atmel,at91sam9x5-clk-utmi":
>> + at91 utmi clock
>> +
>> +Required properties for PMC node:
>> +- reg : defines the IO memory reserved for the PMC.
>> +- interrupts : shall be set to PMC interrupt line.
>> +- interrupt-controller : tell that the PMC is an interrupt controller
>> +- #interrupt-cells : must be set to 2. The first cell encodes the
>> interrupt id
>
> Please add more information about these values.
>
The first cell encodes the clk/interrupt id, which is represented by the
bit position in the PMC_SR register:
- MAIN clk = 0
- PLLA = 1
- ...
>
>> + the second cell encodes the interrupt type.
>
> Here also: is it always the same type that shall be given? Following
> which rule? What are the allowed values?
Yes it's always IRQ_TYPE_LEVEL_HIGH, maybe I should just define one cell
and drop the irq type cell.
>
>
>> +For example:
>> + pmc: pmc@...ffc00 {
>> + compatible = "atmel,sama5d3-pmc";
>> + interrupts = <AT91_ID_SYS IRQ_TYPE_LEVEL_HIGH 7>;
>
> It is an habit not to use macro names in device tree examples (even if
> it is true that it is more readable).
I'll use numerical values instead of macros (anyway, the AT91_ID_XX will
be dropped).
>
>> + interrupt-controller;
>> + #interrupt-cells = <2>;
>> +
>> + /* put at91 clocks here */
>> + };
>> +
>> +Required properties for main clock:
>> +- interrupt-parent : must reference the PMC node.
>> +- interrupts : shall be set to "<AT91_PMC_MOSCS IRQ_TYPE_LEVEL_HIGH>".
>
> Ditto. Here you can use the numerical value and also specify the macro
> name. But the numerical value should prevail.
Okay
>
>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +- clocks (optional if clock-frequency is provided) : shall be the
>> slow clock
>> + phandle. This clock is used to compute the main clock rate if
>> + "clock-frequency" is not provided.
>> +- clock-frequency: the main oscillator frequency.Prefer the use of
>
> Nit. one white space missing
>
>> + "clock-frequency" over automatic clock rate computation.
>
>
>> +
>> +For example:
>> + main: mainck {
>> + compatible = "atmel,at91rm9200-clk-main";
>> + interrupt-parent = <&pmc>;
>> + interrupts = <AT91_PMC_MOSCS IRQ_TYPE_LEVEL_HIGH>;
>
> Ditto
>
>> + #clock-cells = <0>;
>> + clocks = <&ck32k>;
>> + clock-frequency = <18432000>;
>> + };
>> +
>> +Required properties for master clock:
>> +- interrupt-parent : must reference the PMC node.
>> +- interrupts : shall be set to "<AT91_PMC_MCKRDY IRQ_TYPE_LEVEL_HIGH>".
>
> Ditto
>
>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +- clocks : shall be the master clock sources (see atmel datasheet)
>> phandles.
>> + e.g. "<&ck32k>, <&main>, <&plla>, <&pllb>".
>> +- atmel,clk-output-range : minimum and maximum clock frequency (two u32
>> + fields).
>> + e.g. output = <0 133000000>; <=> 0 to 133MHz.
>> +- atmel,clk-divisors : master clock divisors table (four u32 fields).
>> + 0 <=> reserved value.
>> + e.g. divisors = <1 2 4 6>;
>> +- atmel,master-clk-have-div3-pres : some SoC use the reserved value
>> 7 in the
>> + PRES field as CLOCK_DIV3 (e.g sam9x5).
>
> I will check with care the master clock driver as this one is pretty
> picky about changes that could affect it! Note that in previous clock
> implementation we did not touched the MCK configuration, we were only
> reading it...
>
> Anyway, let's keep this binding but make sure that driver is written
> with extreme care ;-)
>
>> +
>> +For example:
>> + mck: mck {
>> + compatible = "atmel,at91rm9200-clk-master";
>> + interrupt-parent = <&pmc>;
>> + interrupts = <AT91_PMC_MCKRDY IRQ_TYPE_LEVEL_HIGH>;
>> + #clock-cells = <0>;
>> + atmel,clk-output-range = <0 133000000>;
>> + atmel,clk-divisors = <1 2 4 0>;
>> + };
>> +
>> +Required properties for peripheral clocks:
>> +- #clock-cells : from common clock binding; shall be set to 1. The
>> second cell
>> + is used to encode the peripheral id. Peripheral ids are defined in
>> + atmel's SoC datasheets.
>> +- clocks : shall be the master clock phandle.
>> + e.g. clocks = <&mck>;
>> +- name: device tree node describing a specific system clock.
>> + * atmel,clk-id: peripheral id. Peripheral id macros should be used.
>
> No. Please use raw numbers. We will not switch to macros for these
> peripheral IDs.
Sure, I'll change this.
>
>> + * atmel,clk-default-divisor (optional, only available for
>> + "atmel,at91sam9x5-clk-peripheral"): sam9x5 and sama5d3 SoC
>> provides
>> + configurable peripheral clock divisor. If you define this
>> property
>> + (u32), the default divisor will be applied when enabling
>> + peripheral clock. If not provided the peripheral clock is not
>> + divided.
>> +
>> +For example:
>> + periph: periphck {
>> + compatible = "atmel,at91sam9x5-clk-peripheral";
>> + #clock-cells = <1>;
>> + clocks = <&mck>;
>> +
>> + pioA_clk {
>> + atmel,clk-id = <AT91SAM9X5_ID_PIOA>;
>
> Ditto.
>
>> + atmel,clk-default-divisor = <1>;
>> + };
>> +
>> + pioB_clk {
>> + atmel,clk-id = <AT91SAM9X5_ID_PIOB>;
>> + atmel,clk-default-divisor = <2>;
>> + };
>> + };
>> +
>> +
>> +Required properties for pll clocks:
>> +- interrupt-parent : must reference the PMC node.
>> +- interrupts : shall be set to "<AT91_PMC_LOCKA IRQ_TYPE_LEVEL_HIGH>".
>
> Ditto
>
>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +- clocks : shall be the main clock phandle.
>> +- atmel,clk-id : pll id. PLL id macros should be used.
>
> No macros here, simply raw numbers. So this mean that we must have
> some documentation of these numbers.
>
>> +- atmel,clk-input-range : minimum and maximum source clock frequency
>> (two u32
>> + fields).
>> + e.g. input = <1 32000000>; <=> 1 to 32MHz.
>> +- #atmel,pll-clk-output-range-cells : number of cells reserved for
>> pll output
>> + range description. Sould be set to 2, 3
>> + or 4.
>> + * 1st and 2nd cells represent the frequency range (min-max).
>> + * 3rd cell is optional and represents the OUT field value for
>> the given
>> + range.
>> + * 4th cell is optional and represents the ICPLL field (PLLICPR
>> + register)
>> +- atmel,pll-clk-output-ranges : pll output frequency ranges +
>> optional parameter
>> + depending on #atmel,pll-output-range-cells
>> + property value.
>> +
>> +For example:
>> + plla: pllack {
>> + compatible = "atmel,at91sam9g45-clk-pll";
>> + interrupt-parent = <&pmc>;
>> + interrupts = <AT91_PMC_LOCKA IRQ_TYPE_LEVEL_HIGH>;
>> + #clock-cells = <0>;
>> + clocks = <&main>;
>> + atmel,clk-id = <AT91_PLLA_CLK>;
>> + atmel,clk-input-range = <2000000 32000000>;
>> + #atmel,pll-clk-output-range-cells = <4>;
>> + atmel,pll-clk-output-ranges = <74500000 800000000 0 0
>> + 69500000 750000000 1 0
>> + 64500000 700000000 2 0
>> + 59500000 650000000 3 0
>> + 54500000 600000000 0 1
>> + 49500000 550000000 1 1
>> + 44500000 500000000 2 1
>> + 40000000 450000000 3 1>;
>> + };
>> +
>> +Required properties for plldiv clocks:
>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +- clocks : shall be the plla clock phandle.
>> +
>> +For example:
>> + plladiv: plladivck {
>> + compatible = "atmel,at91sam9x5-clk-plldiv";
>> + #clock-cells = <0>;
>> + clocks = <&plla>;
>> + };
>
> Maybe a little explanation about this clock. It is a fixed divided
> (how many times?) clock issued from the specified clock.
It is a fixed clock divider (by 2) and AFAIK clk parent must be plla.
I'll add more precision about this clk.
>
>> +
>> +Required properties for programmable clocks:
>> +- interrupt-parent : must reference the PMC node.
>> +- #clock-cells : from common clock binding; shall be set to 1. The
>> second cell
>> + is used to encode the programmable clock id.
>> + Peripheral ids are in atmel's SoC
>> + datasheets.
>> +- clocks : shall be the programmable clock source phandles.
>> + e.g. clocks = <&clk32k>, <&main>, <&plla>, <&pllb>;
>> +- name: device tree node describing a specific prog clock.
>> + * atmel,clk-id : programmable clock id (register offset from PCKx
>> + register).
>> + * interrupts : shall be set to
>> + "<AT91_PMC_PCKRDY(id) IRQ_TYPE_LEVEL_HIGH>".
>
> Ditto
>
>> +
>> +For example:
>> + prog: progck {
>> + compatible = "atmel,at91sam9g45-clk-programmable";
>> + interrupt-parent = <&pmc>;
>> + #clock-cells = <1>;
>> + clocks = <&clk32k>, <&main>, <&plladiv>, <&utmi>, <&mck>;
>> +
>> + prog0 {
>> + atmel,clk-id = <0>;
>> + interrupts = <AT91_PMC_PCKRDY(0) IRQ_TYPE_LEVEL_HIGH>;
>
> Ditto
>
>> + };
>> +
>> + prog1 {
>> + atmel,clk-id = <1>;
>> + interrupts = <AT91_PMC_PCKRDY(1) IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> + };
>> +
>> +
>> +Required properties for smd clock:
>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +- clocks : shall be the smd clock source phandles.
>> + e.g. clocks = <&plladiv>, <&utmi>;
>> +
>> +For example:
>> + smd: smdck {
>> + compatible = "atmel,at91sam9x5-clk-smd";
>> + #clock-cells = <0>;
>> + clocks = <&plladiv>, <&utmi>;
>> + };
>> +
>> +Required properties for system clocks:
>> +- #clock-cells : from common clock binding; shall be set to 1. The
>> second cell
>> + is used to encode the system clock id (bit used in SCER/SCDR
>> register).
>> +- name: device tree node describing a specific system clock.
>> + * id: system clock id (bit position in SCER/SCDR/SCSR registers).
>> + System clock id macros should be used.
>
> Ditto
>
>> +
>> +For example:
>> + system: systemck {
>> + compatible = "atmel,at91rm9200-clk-system";
>> + #clock-cells = <1>;
>> +
>> + ddrck {
>> + atmel,clk-id = <AT91_DDRCK_SYS_CLK>;
>> + };
>> +
>> + uhpck {
>> + atmel,clk-id = <AT91_UHP_SYS_CLK>;
>> + };
>> +
>> + udpck {
>> + atmel,clk-id = <AT91_UDP_SYS_CLK>;
>> + };
>> + };
>> +
>> +
>> +Required properties for usb clock:
>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +- clocks : shall be the smd clock source phandles.
>> + e.g. clocks = <&pllb>;
>> +- atmel,clk-divisors (only available for "atmel,at91rm9200-clk-usb"):
>> + usb clock divisor table.
>> + e.g. divisors = <1 2 4 0>;
>> +- atmel,usb-clk-src0-unused (only available for
>> "atmel,at91sam9x5-clk-usb"):
>> + Some SoC (sam9n12) use usb source 0 to disable the usb clock.
>
> I am not sure that we should use a special case for this device.
> The enable/disable is still done by system clock register set.
>
> It is true that this bit is defined differently but just because the
> only source for this clock is pllb.
Yes, but at least, I need to know that I must add one to the clock
parent id when I set the USBS field in the PMC_USB register.
Or I can drop the atmel,usb-clk-src0-unused property and just add a new
"atmel,at91sam9n12-clk-usb" compatible string,
which might be a better solution here.
Tell me what you'd prefer.
>
>> +
>> +For example:
>> + usb: usbck {
>> + compatible = "atmel,at91sam9x5-clk-usb";
>> + #clock-cells = <0>;
>> + clocks = <&plladiv>, <&utmi>;
>> + };
>> +
>> + usb: usbck {
>> + compatible = "atmel,at91rm9200-clk-usb";
>> + #clock-cells = <0>;
>> + clocks = <&pllb>;
>> + atmel,clk-divisors = <1 2 4 0>;
>> + };
>> +
>> +
>> +Required properties for utmi clock:
>> +- interrupt-parent : must reference the PMC node.
>> +- interrupts : shall be set to "<AT91_PMC_LOCKU IRQ_TYPE_LEVEL_HIGH>".
>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +- clocks : shall be the main clock source phandle.
>> +
>> +For example:
>> + utmi: utmick {
>> + compatible = "atmel,at91sam9x5-clk-utmi";
>> + interrupt-parent = <&pmc>;
>> + interrupts = <AT91_PMC_LOCKU IRQ_TYPE_LEVEL_HIGH>;
>> + #clock-cells = <0>;
>> + clocks = <&main>;
>> + };
>>
>
> Many things in this patch that will have incident in driver code. I
> will try to be coherent when I review the driver patches ;-)
>
> Anyway, all this seem good!
>
> Bye,
Thanks for the detailed review.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists