| Message ID | 1355562224-29448-1-git-send-email-horms+renesas@verge.net.au |
|---|---|
| State | New |
| Headers | show |
Hi Iwamatsu-san, Thank your for the patch. On Saturday 15 December 2012 18:03:36 Simon Horman wrote: > From: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > Add information of device node to struct intc_desc. > > Cc: Magnus Damm <damm@opensource.se> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > --- > > v7 > * Delete "renesas,sh_intcs" and "renesas,sh_intca_irq_pins" as compatible. > Update their documentation. > * Remove of_sh_intc_get_meminfo() and of_sh_intc_get_pint and > of_sh_intc_get_intc(). They are not used. > > v2 - v6 > * No change > --- > Documentation/devicetree/bindings/sh/intc.txt | 15 +---- > drivers/sh/intc/core.c | 2 +- > drivers/sh/intc/internals.h | 3 +- > drivers/sh/intc/irqdomain.c | 6 +- > drivers/sh/intc/of_intc.c | 76 ---------------------- > include/linux/sh_intc.h | 29 +--------- > 6 files changed, 9 insertions(+), 122 deletions(-) Shouldn't this be squashed into patch 01/10 ? 01/10 adds DT bindings support, and you already modify them in this patch.
Hi Simon On Sat, 15 Dec 2012, Simon Horman wrote: > From: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > Cc: Magnus Damm <damm@opensource.se> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> What happened to intcs support? Why has it been dropped? Without it many important devices cannot be set up from DT, e.g. I2C #0. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
Hi Simon On Sat, 15 Dec 2012, Simon Horman wrote: > This is in preparation for initialising INTC using DT. > The proposed INTC configuration is not complete and does > not allow the TMU to be initialised, to exclude it when using DT. Not sure why you decide to not include incs, as I said, it is quite important on sh7372. If we do find a way to include it back, then this patch becomes unnecessary too. Thanks Guennadi > Cc: Magnus Damm <damm@opensource.se> > Cc: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > --- > arch/arm/mach-shmobile/setup-sh7372.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/mach-shmobile/setup-sh7372.c b/arch/arm/mach-shmobile/setup-sh7372.c > index a07954f..90af2e9 100644 > --- a/arch/arm/mach-shmobile/setup-sh7372.c > +++ b/arch/arm/mach-shmobile/setup-sh7372.c > @@ -968,7 +968,7 @@ static struct platform_device spu1_device = { > .num_resources = ARRAY_SIZE(spu1_resources), > }; > > -static struct platform_device *sh7372_early_devices[] __initdata = { > +static struct platform_device *sh7372_early_devices_dt[] __initdata = { > &scif0_device, > &scif1_device, > &scif2_device, > @@ -977,6 +977,9 @@ static struct platform_device *sh7372_early_devices[] __initdata = { > &scif5_device, > &scif6_device, > &cmt2_device, > +}; > + > +static struct platform_device *sh7372_early_devices[] __initdata = { > &tmu00_device, > &tmu01_device, > }; > @@ -1030,6 +1033,8 @@ void __init sh7372_add_standard_devices(void) > > sh7372_init_pm_domains(); > > + platform_add_devices(sh7372_early_devices_dt, > + ARRAY_SIZE(sh7372_early_devices_dt)); > platform_add_devices(sh7372_early_devices, > ARRAY_SIZE(sh7372_early_devices)); > > @@ -1048,6 +1053,8 @@ static void __init sh7372_earlytimer_init(void) > > void __init sh7372_add_early_devices(void) > { > + early_platform_add_devices(sh7372_early_devices_dt, > + ARRAY_SIZE(sh7372_early_devices_dt)); > early_platform_add_devices(sh7372_early_devices, > ARRAY_SIZE(sh7372_early_devices)); > > @@ -1064,8 +1071,8 @@ void __init sh7372_add_early_devices_dt(void) > { > shmobile_setup_delay(800, 1, 3); /* Cortex-A8 @ 800MHz */ > > - early_platform_add_devices(sh7372_early_devices, > - ARRAY_SIZE(sh7372_early_devices)); > + early_platform_add_devices(sh7372_early_devices_dt, > + ARRAY_SIZE(sh7372_early_devices_dt)); > > /* setup early console here as well */ > shmobile_setup_console(); > @@ -1080,8 +1087,8 @@ void __init sh7372_add_standard_devices_dt(void) > /* clocks are setup late during boot in the case of DT */ > sh7372_clock_init(); > > - platform_add_devices(sh7372_early_devices, > - ARRAY_SIZE(sh7372_early_devices)); > + platform_add_devices(sh7372_early_devices_dt, > + ARRAY_SIZE(sh7372_early_devices_dt)); > > of_platform_populate(NULL, of_default_bus_match_table, > sh7372_auxdata_lookup, NULL); > -- > 1.7.10.4 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
On Mon, Dec 17, 2012 at 09:29:44AM +0100, Laurent Pinchart wrote: > Hi Iwamatsu-san, > > Thank your for the patch. > > On Saturday 15 December 2012 18:03:36 Simon Horman wrote: > > From: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > > > Add information of device node to struct intc_desc. > > > > Cc: Magnus Damm <damm@opensource.se> > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > > > --- > > > > v7 > > * Delete "renesas,sh_intcs" and "renesas,sh_intca_irq_pins" as compatible. > > Update their documentation. > > * Remove of_sh_intc_get_meminfo() and of_sh_intc_get_pint and > > of_sh_intc_get_intc(). They are not used. > > > > v2 - v6 > > * No change > > --- > > Documentation/devicetree/bindings/sh/intc.txt | 15 +---- > > drivers/sh/intc/core.c | 2 +- > > drivers/sh/intc/internals.h | 3 +- > > drivers/sh/intc/irqdomain.c | 6 +- > > drivers/sh/intc/of_intc.c | 76 ---------------------- > > include/linux/sh_intc.h | 29 +--------- > > 6 files changed, 9 insertions(+), 122 deletions(-) > > Shouldn't this be squashed into patch 01/10 ? 01/10 adds DT bindings support, > and you already modify them in this patch. Good point. Yes, I think so. I'll probably be the one doing the squashing and if so I'll do so for the next spin.
On Mon, Dec 17, 2012 at 09:44:10AM +0100, Guennadi Liakhovetski wrote: > Hi Simon > > On Sat, 15 Dec 2012, Simon Horman wrote: > > > From: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > > > Cc: Magnus Damm <damm@opensource.se> > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > What happened to intcs support? Why has it been dropped? Without it many > important devices cannot be set up from DT, e.g. I2C #0. INTCS support is still being worked on and doesn't seem to be quite ready yet. I am aware that this limits the usefulness of this series. But it is a step in the right direction.
On Mon, Dec 17, 2012 at 10:58:39AM +0100, Guennadi Liakhovetski wrote: > Hi Simon > > On Sat, 15 Dec 2012, Simon Horman wrote: > > > This is in preparation for initialising INTC using DT. > > The proposed INTC configuration is not complete and does > > not allow the TMU to be initialised, to exclude it when using DT. > > Not sure why you decide to not include incs, as I said, it is quite > important on sh7372. If we do find a way to include it back, then this > patch becomes unnecessary too. Yes, I agree this should not be necessary once INTC has more complete DT support. But in the meantime it seems to be needed. Its the baby-steps approach to changing everything.
Hi Simon On Tue, 18 Dec 2012, Simon Horman wrote: > On Mon, Dec 17, 2012 at 09:44:10AM +0100, Guennadi Liakhovetski wrote: > > Hi Simon > > > > On Sat, 15 Dec 2012, Simon Horman wrote: > > > > > From: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > > > > > Cc: Magnus Damm <damm@opensource.se> > > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > > > What happened to intcs support? Why has it been dropped? Without it many > > important devices cannot be set up from DT, e.g. I2C #0. > > INTCS support is still being worked on and doesn't seem to be > quite ready yet. I am aware that this limits the usefulness of > this series. But it is a step in the right direction. What are the problems with it? My patch series uses INTCS support from the previous version of this patch-set, so, now, without INTCS, I'll have to remove from .dts I2C0 and all devices on it. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
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@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@opensource.se> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > Signed-off-by: Simon Horman <horms+renesas@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@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.
On Tue, Dec 18, 2012 at 10:00:01AM +0000, Mark Rutland wrote: > 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@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@opensource.se> > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > > Signed-off-by: Simon Horman <horms+renesas@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 '_'. Thanks, I will fix that. > > + > > +- 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" ? Thanks, I will up date it to "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". Thanks, I will update it as you suggest. > > +- 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. I believe that this is to allow discontinuous register ranges and the order is not important. > > +- 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. The way I understand this is that intsrc* is used to register the vectors associated with an interrupt source and in doing so allow a phandle to be associated with the interrupt source. So perhaps the following would be better? - intsrc* : Interrupt source. Associate an interrupt source with its vector. > > +- *_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. - *_registers : These describe the vector table, mask, priority, ack, and sense registers. It must contain the following: > > + -- intc_vectors: This requires needs to have vector_table. > > + This specifies phandle which intsrc* defined. -- intc_vectors: This requires the vector_table property which is a list of interrupt sources. > > + -- 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. -- intc_mask_registers : This specifies the contents of the mask register. This node requires the following properties: * address-cells : Must be 1. * size-cells : Must be 1. > Are #address-cells and #size-cells not inherited from the parent node? This seems to result in much unhappiness. For example: # dtc -v Version: DTC 1.3.0 # make dtbs ... DTC arch/arm/boot/dts/sh7372-mackerel-reference.dtb Warning (reg_format): "reg" property in /soc/interrupt-controller@0/intc_mask_registers/intc_mask0 has invalid length (16 bytes) (#address-cells == 2, #size-cells == 1) ... > > + * ranges : Non value. > > + * intc_mask* : This has regs and reginfo. * intc_mask* : This must contain 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/ How about this? ** reg : This specifies the address of mask registers. The first entry specifies the mask register and the second entry specifies the mask clear register. The first cell is the register's address, and the second cell is the register's size which must be 1, 2 or 4 bytes. > > + ** reginfo: This specifies phandle of devices. > > Which devices? How about this? ** reginfo : This specifies the interrupt sources controlled by the mask. The list entries correspond to bits of the mask from most to least significant. A value of 0 may be used for unused bits in the mask. Trailing list entries may be omitted in which case they will be treated as 0. > > + > > + -- 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. Thanks, actually, I believe that the code already treats any absent entries as zero. Is this documentation along the lines of what you had in mind? ** reg : This specifies the address of the priority register. The first entry specifies the priority set register and the second entry specifies priority clear register. The first cell is the register's address, and the second cell is the register's size which must be 1, 2 or 4 bytes. If there is not priority clear register then they entry may be omitted or 0 used as the register's address. > > + ** field-width : Bit size is specified for every device. ** field-width : Width of each group in the register in bits. A group contains the priority for a single interrupt vector. Thus a 16 bit register with a field-width of 4 may control the priority for 4 (16 / 4) interrupt sources. > > + ** reginfo: This specifies phandle of devices. ** reginfo : This specifies the interrupt sources controlled by the priority register. The list entries correspond to the groups of the priority register from least to most significant. A value of 0 may be used for unused groups. Trailing list entries may be omitted in which case they will be treated as 0. > > + > > + -- 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. ** reg : This specifies the address of the sense register. The first cell is the register's address, and the second cell is the register's size which must be 1, 2 or 4 bytes. > > + ** field-width : Bit size is specified for every device. ** field-width : Width of each group in the register in bits. A group contains the priority for a single interrupt vector. Thus a 16 bit register with a field-width of 4 may control the priority for 4 (16 / 4) interrupt sources. > > + ** reginfo: This specifies phandle of devices. ** reginfo : This specifies the interrupt sources controlled by the sense register. The list entries correspond to the groups of the priority register from least to most significant. A value of 0 may be used for unused groups. Trailing list entries may be omitted in which case they will be treated as 0. > > +-- 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. ** reg : This specifies the address of the ACK register. The first cell is the register's address, and the second cell is the register's size which must be 1, 2 or 4 bytes. > > + ** reginfo: This specifies phandle of devices. ** reginfo : This specifies the interrupt sources controlled by the ACK register. The list entries correspond to bits of the ACK register from most to least significant. A value of 0 may be used for unused bits in the mask. Trailing list entries may be omitted in which case they will be treated as 0. > > + > > +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? Yes, I believe so. > > +- 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. They aren't required. I have removed the example entirely as there is another example below. - intc_group* : Interrupt Source Group. Interrupt sources may be grouped with a group sharing the same bits of an interrupt priority register. This node requires the following properties: * group : This specifies the interrupt sources that belong to the group. > > > + }; > > + > > +- intc_intevtsa : This set up the contents of INTEVTSA. > > + This node required properties: > > + * vector : This specifies the address phandle of INTCS. - intc_intevtsa : This sets up the contents of INTEVTSA. This node requires the following properties: * vector : This specifies the interrupt source > > +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@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.
The aims of this series are: * Add DT bindings to to INTC * Allowing sh7372 and r8a7740 ARM shmobile SoCs to use DT to initialise INTCA Key Changes since v5 (previous public revision of this series) * Drop support for sh73a0 SoC. To be useful this requires a fuller initialisation than jsut INCA. In particular, cascade IRQs that are shared between INTC and GIC need to be handled. * Drop board support This will be revisited on a per-board basis, ideally with filler INTC initialisation via DT in place * Drop GIC initialisation This will also be revisited on a per-board basis. * Remove unused INTC DT helper functions Status: I am reasonably happy with this series and plan to merge the shmobile portions. Paul, I forget if I asked you this before or not, but could you let me know if you would like to handle the intc patches, the first two patches in the series, or if you are comfortable with them going through my tree. Git and diffstat information provided for review. ---------------------------------------------------------------------- The following changes since commit ddffeb8c4d0331609ef2581d84de4d763607bd37: Linux 3.7-rc1 (2012-10-14 14:41:04 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git devel/intc-of for you to fetch changes up to 91cb8c20ebbaaf35482cc256b9ab97279bbe753c: ARM: shmobile: sh7372: Use DT initialisation of INTC (2012-12-15 17:50:28 +0900) ---------------------------------------------------------------- Nobuhiro Iwamatsu (9): ARM: shmobile: Add support OF for INTC of shmobile SH: intc: Add support OF of IRQ ARM: shmobile: Add support OF of INTC for r8a7740 ARM: shmobile: Add support OF of INTC for sh7372 ARM: shmobile: Add DT table of INTC for sh7372 ARM: shmobile: Add DT table of INTC for r8a7740 ARM: shmobile: Include DTSI of r8a7740 to armadillo800eva ARM: shmobile: r8a7740: Use DT initialisation of INTC ARM: shmobile: sh7372: Use DT initialisation of INTC Simon Horman (1): ARM: shmobile: sh7372: Do not initialise TMU when using DT Documentation/devicetree/bindings/sh/intc.txt | 152 +++++ arch/arm/boot/dts/r8a7740-armadillo800eva.dts | 2 +- arch/arm/boot/dts/r8a7740.dtsi | 743 ++++++++++++++++++++++- arch/arm/boot/dts/sh7372.dtsi | 784 +++++++++++++++++++++++++ arch/arm/mach-shmobile/include/mach/common.h | 2 + arch/arm/mach-shmobile/intc-r8a7740.c | 70 ++- arch/arm/mach-shmobile/intc-sh7372.c | 113 +++- arch/arm/mach-shmobile/setup-r8a7740.c | 2 +- arch/arm/mach-shmobile/setup-sh7372.c | 19 +- drivers/sh/intc/Makefile | 1 + drivers/sh/intc/core.c | 2 +- drivers/sh/intc/internals.h | 3 +- drivers/sh/intc/irqdomain.c | 6 +- drivers/sh/intc/of_intc.c | 571 ++++++++++++++++++ include/linux/sh_intc.h | 56 ++ 15 files changed, 2481 insertions(+), 45 deletions(-) create mode 100644 Documentation/devicetree/bindings/sh/intc.txt create mode 100644 drivers/sh/intc/of_intc.c