| Message ID | 1457519060-6038-3-git-send-email-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 09, 2016 at 11:24:04AM +0100, Neil Armstrong wrote: > Add timer-width optional property to specify a different vendor > specific timer counter bit-width. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > Documentation/devicetree/bindings/timer/arm,sp804.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/timer/arm,sp804.txt b/Documentation/devicetree/bindings/timer/arm,sp804.txt > index 5cd8eee7..141e143 100644 > --- a/Documentation/devicetree/bindings/timer/arm,sp804.txt > +++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt > @@ -17,6 +17,8 @@ Optional properties: > - arm,sp804-has-irq = <#>: In the case of only 1 timer irq line connected, this > specifies if the irq connection is for timer 1 or timer 2. A value of 1 > or 2 should be used. > +- arm,timer-width: Should contain the width in number of bits of the counter, > + is considered by default 32 but can be changed for vendor variants. That would not be an SP804 nor would the vendor be ARM in that case. So add a new compatible string for the vendor that decided to hack up ARM's IP block. Rob
Hi Rob, On 17/03/16 17:09, Rob Herring wrote: > On Wed, Mar 09, 2016 at 11:24:04AM +0100, Neil Armstrong wrote: >> Add timer-width optional property to specify a different vendor >> specific timer counter bit-width. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> Documentation/devicetree/bindings/timer/arm,sp804.txt | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/timer/arm,sp804.txt b/Documentation/devicetree/bindings/timer/arm,sp804.txt >> index 5cd8eee7..141e143 100644 >> --- a/Documentation/devicetree/bindings/timer/arm,sp804.txt >> +++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt >> @@ -17,6 +17,8 @@ Optional properties: >> - arm,sp804-has-irq = <#>: In the case of only 1 timer irq line connected, this >> specifies if the irq connection is for timer 1 or timer 2. A value of 1 >> or 2 should be used. >> +- arm,timer-width: Should contain the width in number of bits of the counter, >> + is considered by default 32 but can be changed for vendor variants. > > That would not be an SP804 nor would the vendor be ARM in that case. So > add a new compatible string for the vendor that decided to hack up ARM's > IP block. By all accounts this is some ancient reference design[1] which later evolved _into_ the SP804, so that vendor would probably still be ARM ;) A separate compatible string would indeed make more sense, though. Both semantically and in terms of letting the driver account for the differences automatically. Robin. [1]:http://infocenter.arm.com/help/topic/com.arm.doc.ddi0170a/I350250.html > > Rob > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, Mar 17, 2016 at 1:06 PM, Robin Murphy <robin.murphy@arm.com> wrote: > Hi Rob, > > On 17/03/16 17:09, Rob Herring wrote: >> >> On Wed, Mar 09, 2016 at 11:24:04AM +0100, Neil Armstrong wrote: >>> >>> Add timer-width optional property to specify a different vendor >>> specific timer counter bit-width. >>> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>> --- >>> Documentation/devicetree/bindings/timer/arm,sp804.txt | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/timer/arm,sp804.txt >>> b/Documentation/devicetree/bindings/timer/arm,sp804.txt >>> index 5cd8eee7..141e143 100644 >>> --- a/Documentation/devicetree/bindings/timer/arm,sp804.txt >>> +++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt >>> @@ -17,6 +17,8 @@ Optional properties: >>> - arm,sp804-has-irq = <#>: In the case of only 1 timer irq line >>> connected, this >>> specifies if the irq connection is for timer 1 or timer 2. A >>> value of 1 >>> or 2 should be used. >>> +- arm,timer-width: Should contain the width in number of bits of the >>> counter, >>> + is considered by default 32 but can be changed for vendor >>> variants. >> >> >> That would not be an SP804 nor would the vendor be ARM in that case. So >> add a new compatible string for the vendor that decided to hack up ARM's >> IP block. > > > By all accounts this is some ancient reference design[1] which later evolved > _into_ the SP804, so that vendor would probably still be ARM ;) Right. > A separate compatible string would indeed make more sense, though. Both > semantically and in terms of letting the driver account for the differences > automatically. > > Robin. > > [1]:http://infocenter.arm.com/help/topic/com.arm.doc.ddi0170a/I350250.html Humm, same as integrator timers perhaps? Rob > >> >> Rob >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >
On 17/03/16 19:00, Rob Herring wrote: > On Thu, Mar 17, 2016 at 1:06 PM, Robin Murphy <robin.murphy@arm.com> wrote: >> Hi Rob, >> >> On 17/03/16 17:09, Rob Herring wrote: >>> >>> On Wed, Mar 09, 2016 at 11:24:04AM +0100, Neil Armstrong wrote: >>>> >>>> Add timer-width optional property to specify a different vendor >>>> specific timer counter bit-width. >>>> >>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>>> --- >>>> Documentation/devicetree/bindings/timer/arm,sp804.txt | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/timer/arm,sp804.txt >>>> b/Documentation/devicetree/bindings/timer/arm,sp804.txt >>>> index 5cd8eee7..141e143 100644 >>>> --- a/Documentation/devicetree/bindings/timer/arm,sp804.txt >>>> +++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt >>>> @@ -17,6 +17,8 @@ Optional properties: >>>> - arm,sp804-has-irq = <#>: In the case of only 1 timer irq line >>>> connected, this >>>> specifies if the irq connection is for timer 1 or timer 2. A >>>> value of 1 >>>> or 2 should be used. >>>> +- arm,timer-width: Should contain the width in number of bits of the >>>> counter, >>>> + is considered by default 32 but can be changed for vendor >>>> variants. >>> >>> >>> That would not be an SP804 nor would the vendor be ARM in that case. So >>> add a new compatible string for the vendor that decided to hack up ARM's >>> IP block. >> >> >> By all accounts this is some ancient reference design[1] which later evolved >> _into_ the SP804, so that vendor would probably still be ARM ;) > > Right. > >> A separate compatible string would indeed make more sense, though. Both >> semantically and in terms of letting the driver account for the differences >> automatically. >> >> Robin. >> >> [1]:http://infocenter.arm.com/help/topic/com.arm.doc.ddi0170a/I350250.html > > Humm, same as integrator timers perhaps? Having had a quick look, what the Integrator/AP manual describes certainly smells like the same basic block as the "AMBA Timer" - 16 bit counters and the same control register layout - albeit in a mutant triple-timer version with a bigger offset between each register set. Integrator/CP, on the other hand, looks much more SP804-like. Robin.
On 03/17/2016 08:21 PM, Robin Murphy wrote: > On 17/03/16 19:00, Rob Herring wrote: >> On Thu, Mar 17, 2016 at 1:06 PM, Robin Murphy <robin.murphy@arm.com> wrote: >>> Hi Rob, >>> >>> On 17/03/16 17:09, Rob Herring wrote: >>>> That would not be an SP804 nor would the vendor be ARM in that case. So >>>> add a new compatible string for the vendor that decided to hack up ARM's >>>> IP block. >>> >>> >>> By all accounts this is some ancient reference design[1] which later evolved >>> _into_ the SP804, so that vendor would probably still be ARM ;) >> >> Right. >> >>> A separate compatible string would indeed make more sense, though. Both >>> semantically and in terms of letting the driver account for the differences >>> automatically. >>> >>> Robin. >>> >>> [1]:http://infocenter.arm.com/help/topic/com.arm.doc.ddi0170a/I350250.html >> >> Humm, same as integrator timers perhaps? > > Having had a quick look, what the Integrator/AP manual describes certainly smells like the same basic block as the "AMBA Timer" - 16 bit counters and the same control register layout - albeit in a mutant triple-timer version with a bigger offset between each register set. Integrator/CP, on the other hand, looks much more SP804-like. > > Robin. > Hi, I will switch to oxsemi,ox810se-rps-timer since it need a specific register width that will be handled by the driver. Thanks, Neil
Hi Neil, On 22/03/16 09:21, Neil Armstrong wrote: > On 03/17/2016 08:21 PM, Robin Murphy wrote: >> On 17/03/16 19:00, Rob Herring wrote: >>> On Thu, Mar 17, 2016 at 1:06 PM, Robin Murphy <robin.murphy@arm.com> wrote: >>>> Hi Rob, >>>> >>>> On 17/03/16 17:09, Rob Herring wrote: >>>>> That would not be an SP804 nor would the vendor be ARM in that case. So >>>>> add a new compatible string for the vendor that decided to hack up ARM's >>>>> IP block. >>>> >>>> >>>> By all accounts this is some ancient reference design[1] which later evolved >>>> _into_ the SP804, so that vendor would probably still be ARM ;) >>> >>> Right. >>> >>>> A separate compatible string would indeed make more sense, though. Both >>>> semantically and in terms of letting the driver account for the differences >>>> automatically. >>>> >>>> Robin. >>>> >>>> [1]:http://infocenter.arm.com/help/topic/com.arm.doc.ddi0170a/I350250.html >>> >>> Humm, same as integrator timers perhaps? >> >> Having had a quick look, what the Integrator/AP manual describes certainly smells like the same basic block as the "AMBA Timer" - 16 bit counters and the same control register layout - albeit in a mutant triple-timer version with a bigger offset between each register set. Integrator/CP, on the other hand, looks much more SP804-like. >> >> Robin. >> > > Hi, > > I will switch to oxsemi,ox810se-rps-timer since it need a specific register width that will be handled by the driver. By "needs a specific register width" do you mean the OxSemi implementation will give a bus error on a 32-bit access and requires 16-bit accessors? If so, I'd expect to see patch 1 changing readl()s to readw()s at least somewhere. Otherwise, if it's merely that the clocksource API needs to know the upper 16 bits of a word it reads are undefined, then since that's the standard behaviour I'd be inclined to add it to the driver as a canonical "arm,amba-timer" implementation, then have your implementation-specific compatible on top of that just in case. Robin. > > Thanks, > Neil > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 03/22/2016 01:02 PM, Robin Murphy wrote: > Hi Neil, >>>> >>>> Humm, same as integrator timers perhaps? >>> >>> Having had a quick look, what the Integrator/AP manual describes certainly smells like the same basic block as the "AMBA Timer" - 16 bit counters and the same control register layout - albeit in a mutant triple-timer version with a bigger offset between each register set. Integrator/CP, on the other hand, looks much more SP804-like. >>> >>> Robin. >>> >> >> Hi, >> >> I will switch to oxsemi,ox810se-rps-timer since it need a specific register width that will be handled by the driver. > > By "needs a specific register width" do you mean the OxSemi implementation will give a bus error on a 32-bit access and requires 16-bit accessors? If so, I'd expect to see patch 1 changing readl()s to readw()s at least somewhere. Otherwise, if it's merely that the clocksource API needs to know the upper 16 bits of a word it reads are undefined, then since that's the standard behaviour I'd be inclined to add it to the driver as a canonical "arm,amba-timer" implementation, then have your implementation-specific compatible on top of that just in case. No actually the bus access is 32bit but the counter is 24bit wide instead of 16bit, so the clocksource won't work and the system time will furiously drift. It's not the case of the clockevent since the delay fits in 24 bits. It also seems is ignores the 32BIT config bit, so it seems based on the initial 16bit only reference design. How do you think I should implement this ? Neil > Robin. > >> >> Thanks, >> Neil >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >
diff --git a/Documentation/devicetree/bindings/timer/arm,sp804.txt b/Documentation/devicetree/bindings/timer/arm,sp804.txt index 5cd8eee7..141e143 100644 --- a/Documentation/devicetree/bindings/timer/arm,sp804.txt +++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt @@ -17,6 +17,8 @@ Optional properties: - arm,sp804-has-irq = <#>: In the case of only 1 timer irq line connected, this specifies if the irq connection is for timer 1 or timer 2. A value of 1 or 2 should be used. +- arm,timer-width: Should contain the width in number of bits of the counter, + is considered by default 32 but can be changed for vendor variants. Example:
Add timer-width optional property to specify a different vendor specific timer counter bit-width. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- Documentation/devicetree/bindings/timer/arm,sp804.txt | 2 ++ 1 file changed, 2 insertions(+)