[PATCH 01/10] ARM: shmobile: Add support OF for INTC of shmobile
Mark Rutland
mark.rutland at arm.com
Tue Dec 18 05:00:01 EST 2012
Hi,
I have some comments on the devicetree binding.
On Sat, Dec 15, 2012 at 09:03:35AM +0000, Simon Horman wrote:
> From: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj at renesas.com>
>
> This provides OF support of SH/INTC.
>
> The SH/INTC driver is used by SuperH and ARM/SH-MOBILE.
> At the moment, SuperH does not have the plan corresponding to DT.
> DT of SH/INTC has taken the form where the table data of the C
> is managed by DT, in order to maintain compatibility.
>
> Cc: Magnus Damm <damm at opensource.se>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj at renesas.com>
> Signed-off-by: Simon Horman <horms+renesas at verge.net.au>
> ---
> Documentation/devicetree/bindings/sh/intc.txt | 163 +++++++
> drivers/sh/intc/Makefile | 1 +
> drivers/sh/intc/of_intc.c | 647 +++++++++++++++++++++++++
> include/linux/sh_intc.h | 83 ++++
> 4 files changed, 894 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sh/intc.txt
> create mode 100644 drivers/sh/intc/of_intc.c
>
> diff --git a/Documentation/devicetree/bindings/sh/intc.txt b/Documentation/devicetree/bindings/sh/intc.txt
> new file mode 100644
> index 0000000..ebb2398
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sh/intc.txt
> @@ -0,0 +1,163 @@
> +* Renesas SuperH / SH-MOBILE Interrupt Controller
> +
> +The SH/INTC driver is used by SuperH and ARM/SH-MOBILE.
> +At the moment, SuperH does not have the plan corresponding to DT.
> +DT of SH/INTC has taken the form where the table data of the C
> +is managed by DT, in order to maintain compatibility.
> +
> +Main node required properties:
> +
> +- compatible : should be one of:
> + "renesas,sh_intca"
> + "renesas,sh_intcs"
> + "renesas,sh_intca_irq_pins"
Typically compatible strings use '-' rather than '_'.
> +
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Set already 1.
> +- #address-cells : Set already 1.
> +- #size-cells : Set already 1.
I'm not quite sure what "Set already 1" means. Perhaps these should say "Must be 1" ?
> +- ranges : Non value.
This confused me, but I see it's a standard property. It's probably worth
noting why it's required, e.g. "ranges : empty as we have a 1-1 mapping to
parent's address space".
> +- reg : Specifies base physical address(s) and size of the INTC
> + registers.
Presumably these registers are not identical, and the order is important. This
order should be specified.
> +- intsrc* : This sets up the vector for every device.
I'm not sure what this means. If this has a well-defined meaning for the
device, it may be worth having a link to some additional documentation from the
binding doc.
> +- *_registers : There are vector table, mask, priority, ack, and sense
> + register in INTC. In order to hold these data, it is
> + necessary to set up the following contents.
> +
> + -- intc_vectors: This needs to have vector_table.
> + This specifies phandle which intsrc* defined.
> +
> + -- intc_mask_registers : This specifies the contents of the mask register.
> + This node required properties:
> + * address-cells : Set already 1.
> + * size-cells : Set already 1.
Are #address-cells and #size-cells not inherited from the parent node?
> + * ranges : Non value.
> + * intc_mask* : This has regs and reginfo.
> + ** reg : This specifies the address of mask register. First specifies
> + mask register, and 2nd specifies mask clear register.
> + First cell is address, and 2nd sell is address size. 1 is 8bit.
> + 2 is 16bit, 4 is 32bit.
Saying "address size" here is very confusing, as it sounds like you're
re-inventing the #address-cells property. I assume you mean the register size? If so
just say the size must be 1, 2, or 4 bytes.
Also, s/sell/cell/
> + ** reginfo: This specifies phandle of devices.
Which devices?
> +
> + -- intc_prio_registers : This sets up the contents of the priority register.
> + This node required properties:
> + * address-cells : Set already 1.
> + * size-cells : Set already 1.
> + * ranges : Non value.
> + * intc_prio*: This has regs and reginfo.
> + ** reg : This specifies the address of priority register. First specifies
> + priority set register, and 2nd specifies priority clear register.
> + If there is not priority clear register, specifies 0x00.
> + First cell is address, and 2nd sell is address size. 1 is 8bit.
> + 2 is 16bit, 4 is 32bit.
If there is not a priority clear register, why not only describe the priority
set register?
The absence of a second set of reg cells will tell you it's not present, and
will be easier to spot when reading the dts.
> + ** field-width : Bit size is specified for every device.
> + ** reginfo: This specifies phandle of devices.
> +
> + -- intc_sense_registers : This sets up the contents of the sense register.
> + This node required properties:
> + * address-cells : Set already 1.
> + * size-cells : Set already 1.
> + * ranges : Non value.
> + * intc_sense*: This has regs and reginfo.
> + ** reg : This specifies the address of sense register.
> + First cell is address, and 2nd sell is address size. 1 is 8bit.
> + 2 is 16bit, 4 is 32bit.
> + ** field-width : Bit size is specified for every device.
> + ** reginfo: This specifies phandle of devices.
> +
> +-- intc_ack_registers : This sets up the contents of the ACK register.
> + This node required properties:
> + * address-cells : Set already 1.
> + * size-cells : Set already 1.
> + * ranges : Non value.
> + * intc_ack*: This has regs and reginfo.
> + ** reg : This specifies the address of ack register.
> + First cell is address, and 2nd sell is address size. 1 is 8bit.
> + 2 is 16bit, 4 is 32bit.
> + ** reginfo: This specifies phandle of devices.
> +
> +Optional:
> +
> +- group_size : If this INTC register has Group, set up this value.
What does this specify? The number of intc_group nodes?
If so, can this not be omitted and calculated by counting intc_group nodes?
> +- intc_group* : This needs to have group, If INTC device have group.
> + This node required properties:
> + * group : This specifies the address phandle of group.
> + For example, when TMU1 of priority regisdter is sharing with TMU1_0,
> + TMU1_1 and TMU1_2, it describes like below.
> +
> + TMU1: intc_group2 { group = <&TMU1_0 &TMU1_1 &TMU1_2>; };
> +
> + And the phandle is specified as priority regisdter.
s/regisdter/register/
> +
> + intc_prio11 {
> + reg = <0xffd50030 2>, <0x0 0>;
> + field-width = <4>;
> + reginfo = <&TMU1 0 0 0>;
These 0s at the end of the reginfo property, why are they required? They were
not described in the rest of the binding documentation.
> + };
> +
> +- intc_intevtsa : This set up the contents of INTEVTSA.
> + This node required properties:
> + * vector : This specifies the address phandle of INTCS.
> +
> +Note:
> +- "renesas,sh_intca" needs group_size, intc_group*, intc_vectors,
> + intc_mask_registers and intc_prio_registers.
> +- "renesas,sh_intcs" needs group_size, intc_group*, intc_vectors,
> + intc_mask_registers, intc_prio_registers and intc_intevtsa.
> +- "renesas,sh_intca_irq_pins" needs intc_vectors, intc_mask_registers,
> + intc_prio_registers, intc_sense_registers and intc_ack_registers.
> +
> +Example:
> +
> + intca: interrupt-controller at 0 {
> + compatible = "renesas,sh_intca";
> + interrupt-controller;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + #interrupt-cells = <1>;
> + ranges;
> +
> + reg = <0xe6940000 0x200>, <0xe6950000 0x200>;
> + group_size = <19>;
> +
> + DIRC: intsrc1 { vector = <0x0560>; };
> + ATAPI: intsrc2 { vector = <0x05E0>; };
> + ....
> +
> + DMAC1_1: intc_group0 { group = <&DMAC1_1_DEI0 &DMAC1_1_DEI1
> + &DMAC1_1_DEI2 &DMAC1_1_DEI3>; };
> + DMAC1_2: intc_group1 { group = <&DMAC1_2_DEI4 &DMAC1_2_DEI5
> + &DMAC1_2_DADERR>; };
> + ....
> + intc_vectors {
> + vector_table = <&DIRC &ATAPI &IIC1_ALI &IIC1_TACKI &IIC1_WAITI,
> + ....
> + };
> +
> + intc_mask_registers {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + intc_mask0 {
> + reg = <0xe6940080 1>, <0xe69400c0 1>;
> + reginfo = <&DMAC2_1_DEI3 &DMAC2_1_DEI2 &DMAC2_1_DEI1
> + &DMAC2_1_DEI0 0 0 &AP_ARM_COMMTX &AP_ARM_COMMRX>;
> + };
> + ....
> + };
> +
> + intc_prio_registers {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + intc_prio0 {
> + reg = <0xe6940000 2>, <0x0 0>;
> + field-width = <4>;
> + reginfo = <&DMAC3_1 &DMAC3_2 &CMT2 &ICBS0>;
> + };
> + ....
> + };
> + };
[...]
Thanks,
Mark.
More information about the linux-arm-kernel
mailing list