| Message ID | 1363151142-32162-4-git-send-email-haojian.zhuang@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 2013-03-13 at 05:05 +0000, Haojian Zhuang wrote: > +++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt > @@ -0,0 +1,27 @@ > +ARM sp804 Dual Timers > +--------------------------------------- > + > +Required properties: > +- compatible: Should be "arm,sp804" & "arm,primecell" > +- interrupts: Should contain the list of Dual Timer interrupts > + interrupts = <0 0 4>, <0 1 4>; SP804 has three interrupt outputs: TIMINT1 (interrupt generated by the first timer), TIMINT2 (the second timer) and TIMINTC (combined - logical sum of the two former ones). You may want to describe this somehow to make the choice possible later (eg. VE has the combined interrupt wired while - if I understand what Rob is saying correctly - Highbank uses only the TIMINT1). > +- reg: Should contain location and length for dual timer register. > +- clocks: clock driving the dual timer hardware > + clocks = <&timclk0 &timclk1>; Again, there are three "clock" inputs (even ignoring the APBCLK): TIMCLK, TIMCLKEN1, TIMCLKEN2. The real clocking rate for the timer depends on the TIMCLK and respective TIMCLKENx - see http://infocenter.arm.com/help/topic/com.arm.doc.ddi0271d/CJABHCIG.html The driver than could make educated choice based on this information. You may choose to ignore this fact (and require only a clock representing the effective rate). I did it for the VE DTS, but it still doesn't seem completely right > +Optional properties: > +- arm,sp804-clocksource: Should contain the register offset of TIMER1 or > + TIMER2 in Dual Timer Controller. > + arm,sp804-clocksource = <0x20>; You seem to be missing the "arm,sp804-clockevent" one here. Regards Paweł
On 13 March 2013 19:05, Pawel Moll <pawel.moll@arm.com> wrote: > On Wed, 2013-03-13 at 05:05 +0000, Haojian Zhuang wrote: >> +++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt >> @@ -0,0 +1,27 @@ >> +ARM sp804 Dual Timers >> +--------------------------------------- >> + >> +Required properties: >> +- compatible: Should be "arm,sp804" & "arm,primecell" >> +- interrupts: Should contain the list of Dual Timer interrupts >> + interrupts = <0 0 4>, <0 1 4>; > > SP804 has three interrupt outputs: TIMINT1 (interrupt generated by the > first timer), TIMINT2 (the second timer) and TIMINTC (combined - logical > sum of the two former ones). You may want to describe this somehow to > make the choice possible later (eg. VE has the combined interrupt wired > while - if I understand what Rob is saying correctly - Highbank uses > only the TIMINT1). > I prefer to ignore the TIMINTC. It seems that nobody is using this interrupt. >> +- reg: Should contain location and length for dual timer register. >> +- clocks: clock driving the dual timer hardware >> + clocks = <&timclk0 &timclk1>; > > Again, there are three "clock" inputs (even ignoring the APBCLK): > TIMCLK, TIMCLKEN1, TIMCLKEN2. The real clocking rate for the timer > depends on the TIMCLK and respective TIMCLKENx - see > http://infocenter.arm.com/help/topic/com.arm.doc.ddi0271d/CJABHCIG.html > > The driver than could make educated choice based on this information. > > You may choose to ignore this fact (and require only a clock > representing the effective rate). I did it for the VE DTS, but it still > doesn't seem completely right > Sure. I can append it. >> +Optional properties: >> +- arm,sp804-clocksource: Should contain the register offset of TIMER1 or >> + TIMER2 in Dual Timer Controller. >> + arm,sp804-clocksource = <0x20>; > > You seem to be missing the "arm,sp804-clockevent" one here. > Yes, I missed. I'll update the README.
On Wed, 2013-03-13 at 11:37 +0000, Haojian Zhuang wrote: > On 13 March 2013 19:05, Pawel Moll <pawel.moll@arm.com> wrote: > > On Wed, 2013-03-13 at 05:05 +0000, Haojian Zhuang wrote: > >> +++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt > >> @@ -0,0 +1,27 @@ > >> +ARM sp804 Dual Timers > >> +--------------------------------------- > >> + > >> +Required properties: > >> +- compatible: Should be "arm,sp804" & "arm,primecell" > >> +- interrupts: Should contain the list of Dual Timer interrupts > >> + interrupts = <0 0 4>, <0 1 4>; > > > > SP804 has three interrupt outputs: TIMINT1 (interrupt generated by the > > first timer), TIMINT2 (the second timer) and TIMINTC (combined - logical > > sum of the two former ones). You may want to describe this somehow to > > make the choice possible later (eg. VE has the combined interrupt wired > > while - if I understand what Rob is saying correctly - Highbank uses > > only the TIMINT1). > > > I prefer to ignore the TIMINTC. It seems that nobody is using this interrupt. VE is the nobody then ;-) As I mentioned above - both VE motherboard SP804s have the TIMINTC wired up and TIMINT1/TIMINT2 unused. Paweł
On 03/13/2013 06:05 AM, Pawel Moll wrote: > On Wed, 2013-03-13 at 05:05 +0000, Haojian Zhuang wrote: >> +++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt >> @@ -0,0 +1,27 @@ >> +ARM sp804 Dual Timers >> +--------------------------------------- >> + >> +Required properties: >> +- compatible: Should be "arm,sp804" & "arm,primecell" >> +- interrupts: Should contain the list of Dual Timer interrupts >> + interrupts = <0 0 4>, <0 1 4>; > > SP804 has three interrupt outputs: TIMINT1 (interrupt generated by the > first timer), TIMINT2 (the second timer) and TIMINTC (combined - logical > sum of the two former ones). You may want to describe this somehow to > make the choice possible later (eg. VE has the combined interrupt wired > while - if I understand what Rob is saying correctly - Highbank uses > only the TIMINT1). How about: 1 irq - TIMINT1 2 irqs w/ same source # - TIMINTC 2 irqs w/ different source # - TIMINT1 and TIMINT2 I'm not completely sure if Linux and the irq domain code handles the same interrupt source repeated. It should because that is basically a shared irq line. If we ever see only TIMINT2 connected we can add a property for that, but I think that case is unlikely. > >> +- reg: Should contain location and length for dual timer register. >> +- clocks: clock driving the dual timer hardware >> + clocks = <&timclk0 &timclk1>; > > Again, there are three "clock" inputs (even ignoring the APBCLK): We should not ignore that clock as we may want this to be a proper amba_device someday. > TIMCLK, TIMCLKEN1, TIMCLKEN2. The real clocking rate for the timer > depends on the TIMCLK and respective TIMCLKENx - see > http://infocenter.arm.com/help/topic/com.arm.doc.ddi0271d/CJABHCIG.html > > The driver than could make educated choice based on this information. > > You may choose to ignore this fact (and require only a clock > representing the effective rate). I did it for the VE DTS, but it still > doesn't seem completely right > >> +Optional properties: >> +- arm,sp804-clocksource: Should contain the register offset of TIMER1 or >> + TIMER2 in Dual Timer Controller. >> + arm,sp804-clocksource = <0x20>; > > You seem to be missing the "arm,sp804-clockevent" one here. These should stay as aliases or go away as I suggested. Rob
On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote: > How about: > > 1 irq - TIMINT1 > 2 irqs w/ same source # - TIMINTC > 2 irqs w/ different source # - TIMINT1 and TIMINT2 > > I'm not completely sure if Linux and the irq domain code handles the > same interrupt source repeated. It should because that is basically a > shared irq line. > > If we ever see only TIMINT2 connected we can add a property for that, > but I think that case is unlikely. I was rather thinking about using the "interrupt-names" property and naming them explicitly, eg: interrupt-names = "timint1", "timint2"; interrupts = <1>, <2>; interrupt-names = "timint1"; interrupts = <1>; interrupt-names = "timint2"; interrupts = <2>; interrupt-names = "timintc"; interrupts = <3>; But now I see that of_amba_device_create() doesn't do anything about it (platform device would use them as resource name so we could use platform_get_resource_byname), so I'm not sure any more... > > > >> +- reg: Should contain location and length for dual timer register. > >> +- clocks: clock driving the dual timer hardware > >> + clocks = <&timclk0 &timclk1>; > > > > Again, there are three "clock" inputs (even ignoring the APBCLK): > > We should not ignore that clock as we may want this to be a proper > amba_device someday. Sorry, wrong wording. I meant to say "even not mentioning the APBCLK" (because it's required anyway by the primecell binding). VE SP804 node obviously has it defined: v2m_timer01: timer@110000 { compatible = "arm,sp804", "arm,primecell"; reg = <0x110000 0x1000>; interrupts = <2>; clocks = <&v2m_sysctl 0>, <&v2m_sysctl 1>, <&smbclk>; clock-names = "timclken1", "timclken2", "apb_pclk"; }; > > TIMCLK, TIMCLKEN1, TIMCLKEN2. The real clocking rate for the timer > > depends on the TIMCLK and respective TIMCLKENx - see > > http://infocenter.arm.com/help/topic/com.arm.doc.ddi0271d/CJABHCIG.html > > > > The driver than could make educated choice based on this information. > > > > You may choose to ignore this fact (and require only a clock > > representing the effective rate). I did it for the VE DTS, but it still > > doesn't seem completely right > > > >> +Optional properties: > >> +- arm,sp804-clocksource: Should contain the register offset of TIMER1 or > >> + TIMER2 in Dual Timer Controller. > >> + arm,sp804-clocksource = <0x20>; > > > > You seem to be missing the "arm,sp804-clockevent" one here. > > These should stay as aliases or go away as I suggested. I merely pointed out that it's missing from the binding description. I couldn't agree more that both should not be there in the first place. Paweł
On 03/13/2013 09:42 AM, Pawel Moll wrote: > On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote: >> How about: >> >> 1 irq - TIMINT1 >> 2 irqs w/ same source # - TIMINTC >> 2 irqs w/ different source # - TIMINT1 and TIMINT2 >> >> I'm not completely sure if Linux and the irq domain code handles the >> same interrupt source repeated. It should because that is basically a >> shared irq line. >> >> If we ever see only TIMINT2 connected we can add a property for that, >> but I think that case is unlikely. > > I was rather thinking about using the "interrupt-names" property and > naming them explicitly, eg: > > interrupt-names = "timint1", "timint2"; > interrupts = <1>, <2>; > > interrupt-names = "timint1"; > interrupts = <1>; > > interrupt-names = "timint2"; > interrupts = <2>; > > interrupt-names = "timintc"; > interrupts = <3>; > > But now I see that of_amba_device_create() doesn't do anything about it > (platform device would use them as resource name so we could use > platform_get_resource_byname), so I'm not sure any more... The interrupt-names property should not be required and without it here you cannot determine the configuration. Rob
On Wed, 2013-03-13 at 14:51 +0000, Rob Herring wrote: > On 03/13/2013 09:42 AM, Pawel Moll wrote: > > On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote: > >> How about: > >> > >> 1 irq - TIMINT1 > >> 2 irqs w/ same source # - TIMINTC > >> 2 irqs w/ different source # - TIMINT1 and TIMINT2 > >> > >> I'm not completely sure if Linux and the irq domain code handles the > >> same interrupt source repeated. It should because that is basically a > >> shared irq line. > >> > >> If we ever see only TIMINT2 connected we can add a property for that, > >> but I think that case is unlikely. > > > > I was rather thinking about using the "interrupt-names" property and > > naming them explicitly, eg: > > > > interrupt-names = "timint1", "timint2"; > > interrupts = <1>, <2>; > > > > interrupt-names = "timint1"; > > interrupts = <1>; > > > > interrupt-names = "timint2"; > > interrupts = <2>; > > > > interrupt-names = "timintc"; > > interrupts = <3>; > > > > But now I see that of_amba_device_create() doesn't do anything about it > > (platform device would use them as resource name so we could use > > platform_get_resource_byname), so I'm not sure any more... > > The interrupt-names property should not be required and without it here > you cannot determine the configuration. It is not required by the standard interrupt bindings. But it could be required by the SP804 binding, couldn't it? (like any other property) "Encoding" the information by specific organization of the field makes it "invisible to the naked eye". Not that I have any strong feelings about it. Paweł
On 13 March 2013 22:55, Pawel Moll <pawel.moll@arm.com> wrote: > On Wed, 2013-03-13 at 14:51 +0000, Rob Herring wrote: >> On 03/13/2013 09:42 AM, Pawel Moll wrote: >> > On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote: >> >> How about: >> >> >> >> 1 irq - TIMINT1 >> >> 2 irqs w/ same source # - TIMINTC >> >> 2 irqs w/ different source # - TIMINT1 and TIMINT2 >> >> >> >> I'm not completely sure if Linux and the irq domain code handles the >> >> same interrupt source repeated. It should because that is basically a >> >> shared irq line. >> >> >> >> If we ever see only TIMINT2 connected we can add a property for that, >> >> but I think that case is unlikely. >> > >> > I was rather thinking about using the "interrupt-names" property and >> > naming them explicitly, eg: >> > >> > interrupt-names = "timint1", "timint2"; >> > interrupts = <1>, <2>; >> > >> > interrupt-names = "timint1"; >> > interrupts = <1>; >> > >> > interrupt-names = "timint2"; >> > interrupts = <2>; >> > >> > interrupt-names = "timintc"; >> > interrupts = <3>; >> > >> > But now I see that of_amba_device_create() doesn't do anything about it >> > (platform device would use them as resource name so we could use >> > platform_get_resource_byname), so I'm not sure any more... >> >> The interrupt-names property should not be required and without it here >> you cannot determine the configuration. > > It is not required by the standard interrupt bindings. But it could be > required by the SP804 binding, couldn't it? (like any other property) > "Encoding" the information by specific organization of the field makes > it "invisible to the naked eye". > > Not that I have any strong feelings about it. > > Paweł > > What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are already enough on these two TIMERs. If we really needn't TIMINTC, we need to support it. Since it's over-designed. If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another case. We can consider it as replacement of TIMINT1. Regards Haojian
On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote: > How about: > > 1 irq - TIMINT1 > 2 irqs w/ same source # - TIMINTC > 2 irqs w/ different source # - TIMINT1 and TIMINT2 On Wed, 2013-03-13 at 15:11 +0000, Haojian Zhuang wrote: > What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are > already enough on these two TIMERs. If we really needn't TIMINTC, we > need to support it. Since it's over-designed. > > If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another > case. We can consider it as replacement of TIMINT1. Just a thought... How to describe a SP804 with only TIMINT2 wired up? Paweł
On 13 March 2013 23:23, Pawel Moll <pawel.moll@arm.com> wrote: > On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote: >> How about: >> >> 1 irq - TIMINT1 >> 2 irqs w/ same source # - TIMINTC >> 2 irqs w/ different source # - TIMINT1 and TIMINT2 > > On Wed, 2013-03-13 at 15:11 +0000, Haojian Zhuang wrote: >> What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are >> already enough on these two TIMERs. If we really needn't TIMINTC, we >> need to support it. Since it's over-designed. >> >> If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another >> case. We can consider it as replacement of TIMINT1. > > Just a thought... How to describe a SP804 with only TIMINT2 wired up? > > Paweł > > There's no difference on TIMINTC is only used to replace TIMINT1 or TIMINT2. We only need to tell sp804 driver which timer is using irq, and the irq number.
On Wed, 2013-03-13 at 15:25 +0000, Haojian Zhuang wrote: > On 13 March 2013 23:23, Pawel Moll <pawel.moll@arm.com> wrote: > > On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote: > >> How about: > >> > >> 1 irq - TIMINT1 > >> 2 irqs w/ same source # - TIMINTC > >> 2 irqs w/ different source # - TIMINT1 and TIMINT2 > > > > On Wed, 2013-03-13 at 15:11 +0000, Haojian Zhuang wrote: > >> What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are > >> already enough on these two TIMERs. If we really needn't TIMINTC, we > >> need to support it. Since it's over-designed. > >> > >> If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another > >> case. We can consider it as replacement of TIMINT1. > > > > Just a thought... How to describe a SP804 with only TIMINT2 wired up? > > > > Paweł > > > > > There's no difference on TIMINTC is only used to replace TIMINT1 or TIMINT2. > We only need to tell sp804 driver which timer is using irq, and the irq number. If I understand you well: * Both TIMINT1 and TIMINT2 wired up - TIMINTC doesn't matter interrupts = <1>, <2>; * TIMINT1 wired up, TIMINT2 not wired up interrupts = <1>; * Only TIMINTC wired up - treat it as TIMINT1 interrupts = <1>; So far so good. My question is, how do you describe the following: * _Only_ TIMINT2 wired up, TIMINT1 and TIMINTC _not_ wired up interrupts = ??? Paweł
On 03/13/2013 10:29 AM, Pawel Moll wrote: > On Wed, 2013-03-13 at 15:25 +0000, Haojian Zhuang wrote: >> On 13 March 2013 23:23, Pawel Moll <pawel.moll@arm.com> wrote: >>> On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote: >>>> How about: >>>> >>>> 1 irq - TIMINT1 >>>> 2 irqs w/ same source # - TIMINTC >>>> 2 irqs w/ different source # - TIMINT1 and TIMINT2 >>> >>> On Wed, 2013-03-13 at 15:11 +0000, Haojian Zhuang wrote: >>>> What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are >>>> already enough on these two TIMERs. If we really needn't TIMINTC, we >>>> need to support it. Since it's over-designed. >>>> >>>> If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another >>>> case. We can consider it as replacement of TIMINT1. >>> >>> Just a thought... How to describe a SP804 with only TIMINT2 wired up? >>> >>> Paweł >>> >>> >> There's no difference on TIMINTC is only used to replace TIMINT1 or TIMINT2. >> We only need to tell sp804 driver which timer is using irq, and the irq number. > > If I understand you well: > > * Both TIMINT1 and TIMINT2 wired up - TIMINTC doesn't matter > > interrupts = <1>, <2>; > > * TIMINT1 wired up, TIMINT2 not wired up > > interrupts = <1>; > > * Only TIMINTC wired up - treat it as TIMINT1 > > interrupts = <1>; > > So far so good. My question is, how do you describe the following: > > * _Only_ TIMINT2 wired up, TIMINT1 and TIMINTC _not_ wired up > > interrupts = ??? I previously described that to add a property in that case. That makes INT1 and INT2 only handling asymetric though. An alternative would be an optional property "arm,sp804-has-irq = <#>" in the case of only INT1 or INT2 connected. The combined irq would be a single irq without the "arm,sp804-has-irq" property and both connected would be 2 irqs. Rob
On Wed, 2013-03-13 at 15:39 +0000, Rob Herring wrote: > I previously described that to add a property in that case. That makes > INT1 and INT2 only handling asymetric though. An alternative would be an > optional property "arm,sp804-has-irq = <#>" in the case of only INT1 or > INT2 connected. The combined irq would be a single irq without the > "arm,sp804-has-irq" property and both connected would be 2 irqs. Fine with me. Paweł
On 13 March 2013 23:29, Pawel Moll <pawel.moll@arm.com> wrote: > On Wed, 2013-03-13 at 15:25 +0000, Haojian Zhuang wrote: >> On 13 March 2013 23:23, Pawel Moll <pawel.moll@arm.com> wrote: >> > On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote: >> >> How about: >> >> >> >> 1 irq - TIMINT1 >> >> 2 irqs w/ same source # - TIMINTC >> >> 2 irqs w/ different source # - TIMINT1 and TIMINT2 >> > >> > On Wed, 2013-03-13 at 15:11 +0000, Haojian Zhuang wrote: >> >> What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are >> >> already enough on these two TIMERs. If we really needn't TIMINTC, we >> >> need to support it. Since it's over-designed. >> >> >> >> If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another >> >> case. We can consider it as replacement of TIMINT1. >> > >> > Just a thought... How to describe a SP804 with only TIMINT2 wired up? >> > >> > Paweł >> > >> > >> There's no difference on TIMINTC is only used to replace TIMINT1 or TIMINT2. >> We only need to tell sp804 driver which timer is using irq, and the irq number. > > If I understand you well: > > * Both TIMINT1 and TIMINT2 wired up - TIMINTC doesn't matter > > interrupts = <1>, <2>; > > * TIMINT1 wired up, TIMINT2 not wired up > > interrupts = <1>; > > * Only TIMINTC wired up - treat it as TIMINT1 > > interrupts = <1>; > > So far so good. My question is, how do you describe the following: > > * _Only_ TIMINT2 wired up, TIMINT1 and TIMINTC _not_ wired up > > interrupts = ??? > > Paweł > > > In my current implementation, I need two things to judge whether irq is really necessary. If the timer contains "arm,sp804-clockevent = <evtoffs>" property, I'll check whether the timer irq is specified at the same time. TIMINT1 & TIMINTC are not routed. TIMINT2 is used for clock event. interrupts = <any number> <1>; if (!retevt) { if (evtoffs) { /* TIMER2 is clock event */ i = 1; evtoffs = TIMER_2_BASE; } else { /* TIMER1 is clock event */ i = 0; evtoffs = TIMER_1_BASE; } irq = irq_of_parse_and_map(np, i); ... } If the timer contains "arm,sp804-clocksource = <srcoffs>" property, I won't check the timer irq. You even needn't define it. TIMINT1 & TIMINTC are not routed. TIMER2 is used for clock source. interrupts = <any number> <any number>; or don't define interrupts if (!retsrc) { if (srcoffs) { /* TIMER2 is clock source */ i = 1; srcoffs = TIMER_2_BASE; } else { /* TIMER1 is clock source */ i = 0; srcoffs = TIMER_1_BASE; } ... }
On 13 March 2013 23:41, Pawel Moll <pawel.moll@arm.com> wrote: > On Wed, 2013-03-13 at 15:39 +0000, Rob Herring wrote: >> I previously described that to add a property in that case. That makes >> INT1 and INT2 only handling asymetric though. An alternative would be an >> optional property "arm,sp804-has-irq = <#>" in the case of only INT1 or >> INT2 connected. The combined irq would be a single irq without the >> "arm,sp804-has-irq" property and both connected would be 2 irqs. > > Fine with me. > > Paweł > > It's OK to me. I can use this property instead. Regards Haojian
On Wed, 2013-03-13 at 15:42 +0000, Haojian Zhuang wrote: > In my current implementation, I need two things to judge whether > irq is really necessary. > > If the timer contains "arm,sp804-clockevent = <evtoffs>" property, > I'll check whether the timer irq is specified at the same time. > > TIMINT1 & TIMINTC are not routed. TIMINT2 is used for clock event. Fine, so simply reverse the logic and drop the "arm,sp804-clock*" properties. Something like: if (has_timint1) { evtoffs = TIMER_1_BASE; irq = irq_of_parse_and_map(np, 1); srcoffs = TIMER_2_BASE; } else (has_timint2) { evtoffs = TIMER_2_BASE; irq = irq_of_parse_and_map(np, 2); srcoffs = TIMER_1_BASE; } else { can't be clockevent, sorry } Hope it makes clear what I'd expect from the driver. Paweł
On Wednesday 13 March 2013, Pawel Moll wrote: > Fine, so simply reverse the logic and drop the "arm,sp804-clock*" > properties. Something like: > > if (has_timint1) { > evtoffs = TIMER_1_BASE; > irq = irq_of_parse_and_map(np, 1); > srcoffs = TIMER_2_BASE; > } else (has_timint2) { > evtoffs = TIMER_2_BASE; > irq = irq_of_parse_and_map(np, 2); > srcoffs = TIMER_1_BASE; > } else { > can't be clockevent, sorry > } > > Hope it makes clear what I'd expect from the driver. Makes sense. I guess you can actually derive "has_timint1" from the presence of the irq and just try to parse it: irq = irq_of_parse_and_map(np, 0); if (irq) { evtoffs = TIMER_1_BASE; srcoffs = TIMER_2_BASE; } else { irq = irq_of_parse_and_map(np, 1); if (!irq) return -ENODEV; srcoffs = TIMER_1_BASE; evtoffs = TIMER_2_BASE; } I wonder if we can ever get into the second case though: If the first interrupt in not valid, can there be a second one in the interrupts property? I don't know of a way to put an "invalid" interrupt in there. Arnd
On Wed, 2013-03-13 at 16:35 +0000, Arnd Bergmann wrote: > Makes sense. I guess you can actually derive "has_timint1" > from the presence of the irq and just try to parse it: > > irq = irq_of_parse_and_map(np, 0); > if (irq) { > evtoffs = TIMER_1_BASE; > srcoffs = TIMER_2_BASE; > } else { > irq = irq_of_parse_and_map(np, 1); > if (!irq) > return -ENODEV; > srcoffs = TIMER_1_BASE; > evtoffs = TIMER_2_BASE; > } > > > I wonder if we can ever get into the second case though: If the > first interrupt in not valid, can there be a second one in the > interrupts property? I don't know of a way to put an "invalid" > interrupt in there. The discussion in the "[PATCH v3 03/11] clocksource: sp804: add device tree support" thread resulted in some ideas how to describe such situation. I suggested using interrupt-names property to explicitly describe the numbers, Rob wanted to simply define one or two interrupts and in the former case have a special property describing which one is it. Either way you can get this information from the tree. The main argument is about the clockevent/source aliases - whether they should be obligatory, optional or banned. Paweł
On Wed, Mar 13, 2013 at 09:17:21AM -0500, Rob Herring wrote: > How about: > > 1 irq - TIMINT1 > 2 irqs w/ same source # - TIMINTC > 2 irqs w/ different source # - TIMINT1 and TIMINT2 > > I'm not completely sure if Linux and the irq domain code handles the > same interrupt source repeated. It should because that is basically a > shared irq line. I think you're missing something here. Any particular platform will generally only wire either the common interrupt, or the individual interrupts. Very few, if any, will wire both. It's just like stuff such as MMCI and the UARTs. They have a combined interrupt out, and individual interrupt outs. However, they're all written for the combined interrupt only, because practically no one uses the individal interrupts.
diff --git a/Documentation/devicetree/bindings/timer/arm,sp804.txt b/Documentation/devicetree/bindings/timer/arm,sp804.txt new file mode 100644 index 0000000..ec1ae45 --- /dev/null +++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt @@ -0,0 +1,27 @@ +ARM sp804 Dual Timers +--------------------------------------- + +Required properties: +- compatible: Should be "arm,sp804" & "arm,primecell" +- interrupts: Should contain the list of Dual Timer interrupts + interrupts = <0 0 4>, <0 1 4>; +- reg: Should contain location and length for dual timer register. +- clocks: clock driving the dual timer hardware + clocks = <&timclk0 &timclk1>; + +Optional properties: +- arm,sp804-clocksource: Should contain the register offset of TIMER1 or + TIMER2 in Dual Timer Controller. + arm,sp804-clocksource = <0x20>; + +Example: + + timer0: timer@fc800000 { + compatible = "arm,sp804", "arm,primecell"; + reg = <0xfc800000 0x1000>; + /* timer00 & timer01 */ + interrupts = <0 0 4>, <0 1 4>; + clocks = <&timclk0 &timclk1>; + clock-names = "apb_pclk"; + status = "disabled"; + }; diff --git a/drivers/clocksource/timer-sp.c b/drivers/clocksource/timer-sp.c index a7f2510..63f757d 100644 --- a/drivers/clocksource/timer-sp.c +++ b/drivers/clocksource/timer-sp.c @@ -19,16 +19,23 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ #include <linux/clk.h> +#include <linux/clkdev.h> #include <linux/clocksource.h> #include <linux/clockchips.h> #include <linux/err.h> #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/io.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> #include <clocksource/arm_timer.h> +#include <clocksource/timer-sp.h> #include <asm/sched_clock.h> +#define SP804_CLKSRC "sp804 source" +#define SP804_CLKEVT "sp804 event" + static long __init sp804_get_clock_rate(const char *name) { struct clk *clk; @@ -189,3 +196,101 @@ void __init sp804_clockevents_init(void __iomem *base, unsigned int irq, setup_irq(irq, &sp804_timer_irq); clockevents_config_and_register(evt, rate, 0xf, 0xffffffff); } + +static struct device_node *from = NULL; + +static struct of_device_id sp804_timer_match[] __initdata = { + { .compatible = "arm,sp804", }, + {} +}; + +static struct clk __init *sp804_dt_init_clk(struct device_node *np, int i, + const char *name) +{ + struct clk_lookup *lookup = NULL; + struct clk *clk; + + clk = of_clk_get(np, i); + if (IS_ERR(clk)) + return clk; + lookup = clkdev_alloc(clk, name, "sp804"); + if (!lookup) { + clk_put(clk); + return ERR_PTR(-EINVAL); + } + clkdev_add(lookup); + return clk; +} + +static void __attribute__((__unused__)) __init sp804_dt_init(void) +{ + struct device_node *np = NULL; + struct clk *clksrc = NULL, *clkevt = NULL; + void __iomem *base; + int retsrc, retevt, i = 0, irq = 0; + u32 srcoffs = 0, evtoffs = 0; + + np = of_find_matching_node(from, sp804_timer_match); + WARN_ON(!np); + if (!np) { + pr_err("Failed to find sp804 timer\n"); + return; + } + from = np; + /* check whether timer node is available */ + if (!of_device_is_available(np)) + return; + + base = of_iomap(np, 0); + WARN_ON(!base); + if (!base) + return; + + retsrc = of_property_read_u32(np, "arm,sp804-clocksource", &srcoffs); + retevt = of_property_read_u32(np, "arm,sp804-clockevent", &evtoffs); + if (retsrc < 0 && retevt < 0) + goto err; + + if (!retsrc) { + if (srcoffs) { + /* TIMER2 is clock source */ + i = 1; + srcoffs = TIMER_2_BASE; + } else { + /* TIMER1 is clock source */ + i = 0; + srcoffs = TIMER_1_BASE; + } + clksrc = sp804_dt_init_clk(np, i, SP804_CLKSRC); + if (IS_ERR(clksrc)) + goto err; + sp804_clocksource_and_sched_clock_init(base + srcoffs, + SP804_CLKSRC); + } + if (!retevt) { + if (evtoffs) { + /* TIMER2 is clock event */ + i = 1; + evtoffs = TIMER_2_BASE; + } else { + /* TIMER1 is clock event */ + i = 0; + evtoffs = TIMER_1_BASE; + } + irq = irq_of_parse_and_map(np, i); + if (irq < 0) + goto err_evt; + clkevt = sp804_dt_init_clk(np, i, SP804_CLKEVT); + if (IS_ERR(clkevt)) + goto err_evt; + sp804_clockevents_init(base + evtoffs, irq, SP804_CLKEVT); + } + + return; +err_evt: + if (clksrc) + clk_put(clksrc); +err: + iounmap(base); +} +CLOCKSOURCE_OF_DECLARE(sp804, "arm,sp804", sp804_dt_init)
Parse clock & irq from device tree for sp804. Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> --- .../devicetree/bindings/timer/arm,sp804.txt | 27 +++++ drivers/clocksource/timer-sp.c | 105 ++++++++++++++++++++ 2 files changed, 132 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/arm,sp804.txt