Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1360050 > unrolled thread

Re: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width property

Started byRob Herring <robh@kernel.org>
First post2016-03-17 18:10 +0100
Last post2016-03-22 15:40 +0100
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width  property Rob Herring <robh@kernel.org> - 2016-03-17 18:10 +0100
    Re: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width  property Robin Murphy <robin.murphy@arm.com> - 2016-03-17 19:10 +0100
      Re: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width property Rob Herring <robh@kernel.org> - 2016-03-17 20:10 +0100
        Re: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width  property Robin Murphy <robin.murphy@arm.com> - 2016-03-17 20:30 +0100
          Re: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width  property Neil Armstrong <narmstrong@baylibre.com> - 2016-03-22 10:30 +0100
            Re: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width  property Robin Murphy <robin.murphy@arm.com> - 2016-03-22 13:10 +0100
              Re: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width  property Neil Armstrong <narmstrong@baylibre.com> - 2016-03-22 15:40 +0100

#1360050 — Re: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width property

FromRob Herring <robh@kernel.org>
Date2016-03-17 18:10 +0100
SubjectRe: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width property
Message-ID<rdJAS-1Mf-7@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1360116

FromRobin Murphy <robin.murphy@arm.com>
Date2016-03-17 19:10 +0100
Message-ID<rdKwY-2ml-45@gated-at.bofh.it>
In reply to#1360050
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
>

[toc] | [prev] | [next] | [standalone]

#1360140 — Re: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width property

FromRob Herring <robh@kernel.org>
Date2016-03-17 20:10 +0100
SubjectRe: [PATCH v2 02/18] dt-bindings: timer: sp804: add timer-width property
Message-ID<rdLt0-2VH-17@gated-at.bofh.it>
In reply to#1360116
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
>>
>

[toc] | [prev] | [next] | [standalone]

#1360142

FromRobin Murphy <robin.murphy@arm.com>
Date2016-03-17 20:30 +0100
Message-ID<rdLMm-32d-3@gated-at.bofh.it>
In reply to#1360140
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.

[toc] | [prev] | [next] | [standalone]

#1362432

FromNeil Armstrong <narmstrong@baylibre.com>
Date2016-03-22 10:30 +0100
Message-ID<rfqNs-2nj-17@gated-at.bofh.it>
In reply to#1360142
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

[toc] | [prev] | [next] | [standalone]

#1362675

FromRobin Murphy <robin.murphy@arm.com>
Date2016-03-22 13:10 +0100
Message-ID<rftih-49b-5@gated-at.bofh.it>
In reply to#1362432
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
>

[toc] | [prev] | [next] | [standalone]

#1362776

FromNeil Armstrong <narmstrong@baylibre.com>
Date2016-03-22 15:40 +0100
Message-ID<rfvDs-5NV-19@gated-at.bofh.it>
In reply to#1362675
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
>>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web