Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
Hi Magnus,
On Monday 17 February 2014 11:07:06 Magnus Damm wrote:
> On Mon, Feb 17, 2014 at 10:48 AM, Laurent Pinchart wrote:
> > On Monday 17 February 2014 10:41:31 Magnus Damm wrote:
> >> On Mon, Feb 17, 2014 at 3:18 AM, Laurent Pinchart wrote:
> >> > On Saturday 15 February 2014 13:46:54 Thomas Gleixner wrote:
> >> >> On Fri, 14 Feb 2014, Laurent Pinchart wrote:
> >> >> > CMT hardware devices can support multiple channels, with global
> >> >> > registers and per-channel registers. The sh_cmt driver currently
> >> >> > models the hardware with one Linux device per channel. This model
> >> >> > makes it difficult to handle global registers in a clean way.
> >> >> >
> >> >> > Add support for a new model that uses one Linux device per timer
> >> >> > with multiple channels per device. This requires changes to platform
> >> >> > data, add new channel configuration fields.
> >> >> >
> >> >> > Support for the legacy model is kept and will be removed after all
> >> >> > platforms switch to the new model.
> >> >> >
> >> >> > Signed-off-by: Laurent Pinchart
> >> >> >
> >> >> > ---
> >> >> >
> >> >> > drivers/clocksource/sh_cmt.c | 299
> >> >> > include/linux/sh_timer.h | 9 ++
> >> >> > 2 files changed, 239 insertions(+), 69 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/clocksource/sh_cmt.c
> >> >> > b/drivers/clocksource/sh_cmt.c
> >> >> > index 5280231..8390f0f 100644
> >> >> > --- a/drivers/clocksource/sh_cmt.c
> >> >> > +++ b/drivers/clocksource/sh_cmt.c
> >
> > [snip]
> >
> >> >> > @@ -85,11 +94,15 @@ struct sh_cmt_info {
> >> >> >
> >> >> > struct sh_cmt_channel {
> >> >> > struct sh_cmt_device *cmt;
> >> >> >
> >> >> > - unsigned int index;
> >> >> > - void __iomem *base;
> >> >> > + unsigned int index; /* Index in the documentation */
> >> >> > + unsigned int hwidx; /* Real hardware index */
> >> >> > +
> >> >> > + void __iomem *iostart;
> >> >> > + void __iomem *ioctrl;
> >> >> >
> >> >> > struct irqaction irqaction;
> >> >>
> >> >> While you are at it, can you please get rid of that irqaction and use
> >> >> request_irq() ?
> >> >
> >> > The driver claims it can't use request_irq() because the function is
> >> > not available for early platform devices. If the situation has changed
> >> > I'd gladly get rid of irqaction.
> >>
> >> With the risk of stating the obvious, this depends on how early the
> >> early platform device stuff is being run. The actual location may not
> >> be the same on SH and ARM for instance.
> >>
> >> On ARM we are doing all we can to initialize these devices as late as
> >> ever possible in the MULTIPLAFORM (DT reference) case. Once we manage
> >> to remove the legacy ARM case then there is one less user of early
> >> platform timers.
> >>
> >> One perhaps reasonable way forward could be to use request_irq() in
> >> case of DT and leave the legacy platform data case to rely on
> >> irqaction.
> >
> > The whole point of switching from setup_irq() to request_irq() is to
> > simplify the code. Adding request_irq() as an option would go in the
> > opposite direction.
>
> The point of switching to setup_irq() was not very clear to me. I
> think it is fine to switch to using it over time, and I'm open to any
> alternative ways forward. Usually moving in incremental steps means
> more crap in the short term, but if someone could come up with a way
> that involves little crap then I'm all for that! =)
I've had a quick look at request_irq(). The function is defined as a static
inline in include/linux/interrupt.h:
static inline int __must_check
request_irq(unsigned int irq, irq_handler_t handler, unsigned long flags,
const char *name, void *dev)
{
return request_threaded_irq(irq, handler, NULL, flags, name, dev);
}
request_threaded_irq() is implemented in kernel/irq/manage.c. I've removed the
code paths that would never be taken when called by the sh_cmt driver, due to
- handler not being NULL
- thread_fn being NULL
- CONFIG_DEBUG_SHIRQ_FIXME not being set
- IRQF_SHARED not being set
int request_threaded_irq(unsigned int irq, irq_handler_t handler,
irq_handler_t thread_fn, unsigned long irqflags,
const char *devname, void *dev_id)
{
struct irqaction *action;
struct irq_desc *desc;
int retval;
desc = irq_to_desc(irq);
if (!desc)
return -EINVAL;
if (!irq_settings_can_request(desc) ||
WARN_ON(irq_settings_is_per_cpu_devid(desc)))
return -EINVAL;
action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
if (!action)
return -ENOMEM;
action->handler = handler;
action->thread_fn = thread_fn;
action->flags = irqflags;
action->name = devname;
action->dev_id = dev_id;
chip_bus_lock(desc);
retval = __setup_irq(irq, desc, action);
Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
Hi Magnus,
On Monday 17 February 2014 11:07:06 Magnus Damm wrote:
On Mon, Feb 17, 2014 at 10:48 AM, Laurent Pinchart wrote:
On Monday 17 February 2014 10:41:31 Magnus Damm wrote:
On Mon, Feb 17, 2014 at 3:18 AM, Laurent Pinchart wrote:
On Saturday 15 February 2014 13:46:54 Thomas Gleixner wrote:
On Fri, 14 Feb 2014, Laurent Pinchart wrote:
CMT hardware devices can support multiple channels, with global
registers and per-channel registers. The sh_cmt driver currently
models the hardware with one Linux device per channel. This model
makes it difficult to handle global registers in a clean way.
Add support for a new model that uses one Linux device per timer
with multiple channels per device. This requires changes to platform
data, add new channel configuration fields.
Support for the legacy model is kept and will be removed after all
platforms switch to the new model.
Signed-off-by: Laurent Pinchart
laurent.pinchart+rene...@ideasonboard.com
---
drivers/clocksource/sh_cmt.c | 299
include/linux/sh_timer.h | 9 ++
2 files changed, 239 insertions(+), 69 deletions(-)
diff --git a/drivers/clocksource/sh_cmt.c
b/drivers/clocksource/sh_cmt.c
index 5280231..8390f0f 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
[snip]
@@ -85,11 +94,15 @@ struct sh_cmt_info {
struct sh_cmt_channel {
struct sh_cmt_device *cmt;
- unsigned int index;
- void __iomem *base;
+ unsigned int index; /* Index in the documentation */
+ unsigned int hwidx; /* Real hardware index */
+
+ void __iomem *iostart;
+ void __iomem *ioctrl;
struct irqaction irqaction;
While you are at it, can you please get rid of that irqaction and use
request_irq() ?
The driver claims it can't use request_irq() because the function is
not available for early platform devices. If the situation has changed
I'd gladly get rid of irqaction.
With the risk of stating the obvious, this depends on how early the
early platform device stuff is being run. The actual location may not
be the same on SH and ARM for instance.
On ARM we are doing all we can to initialize these devices as late as
ever possible in the MULTIPLAFORM (DT reference) case. Once we manage
to remove the legacy ARM case then there is one less user of early
platform timers.
One perhaps reasonable way forward could be to use request_irq() in
case of DT and leave the legacy platform data case to rely on
irqaction.
The whole point of switching from setup_irq() to request_irq() is to
simplify the code. Adding request_irq() as an option would go in the
opposite direction.
The point of switching to setup_irq() was not very clear to me. I
think it is fine to switch to using it over time, and I'm open to any
alternative ways forward. Usually moving in incremental steps means
more crap in the short term, but if someone could come up with a way
that involves little crap then I'm all for that! =)
I've had a quick look at request_irq(). The function is defined as a static
inline in include/linux/interrupt.h:
static inline int __must_check
request_irq(unsigned int irq, irq_handler_t handler, unsigned long flags,
const char *name, void *dev)
{
return request_threaded_irq(irq, handler, NULL, flags, name, dev);
}
request_threaded_irq() is implemented in kernel/irq/manage.c. I've removed the
code paths that would never be taken when called by the sh_cmt driver, due to
- handler not being NULL
- thread_fn being NULL
- CONFIG_DEBUG_SHIRQ_FIXME not being set
- IRQF_SHARED not being set
int request_threaded_irq(unsigned int irq, irq_handler_t handler,
irq_handler_t thread_fn, unsigned long irqflags,
const char *devname, void *dev_id)
{
struct irqaction *action;
struct irq_desc *desc;
int retval;
desc = irq_to_desc(irq);
if (!desc)
return -EINVAL;
if (!irq_settings_can_request(desc) ||
WARN_ON(irq_settings_is_per_cpu_devid(desc)))
return -EINVAL;
action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
if (!action)
return -ENOMEM;
action-handler = handler;
action-thread_fn = thread_fn;
action-flags = irqflags;
action-name = devname;
action-dev_id = dev_id;
chip_bus_lock(desc);
retval = __setup_irq(irq, desc, action);
chip_bus_sync_unlock(desc);
if (retval)
kfree(action);
return retval;
}
setup_irq() is implemented in the same file as
int setup_irq(unsigned int irq, struct irqaction *act)
{
int retval;
struct irq_desc *desc = irq_to_desc(irq);
if
Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
Hi Laurent,
On Mon, Feb 17, 2014 at 10:48 AM, Laurent Pinchart
wrote:
> Hi Magnus,
>
> On Monday 17 February 2014 10:41:31 Magnus Damm wrote:
>> On Mon, Feb 17, 2014 at 3:18 AM, Laurent Pinchart wrote:
>> > On Saturday 15 February 2014 13:46:54 Thomas Gleixner wrote:
>> >> On Fri, 14 Feb 2014, Laurent Pinchart wrote:
>> >> > CMT hardware devices can support multiple channels, with global
>> >> > registers and per-channel registers. The sh_cmt driver currently models
>> >> > the hardware with one Linux device per channel. This model makes it
>> >> > difficult to handle global registers in a clean way.
>> >> >
>> >> > Add support for a new model that uses one Linux device per timer with
>> >> > multiple channels per device. This requires changes to platform data,
>> >> > add new channel configuration fields.
>> >> >
>> >> > Support for the legacy model is kept and will be removed after all
>> >> > platforms switch to the new model.
>> >> >
>> >> > Signed-off-by: Laurent Pinchart
>> >> >
>> >> > ---
>> >> >
>> >> > drivers/clocksource/sh_cmt.c | 299 ++-
>> >> > include/linux/sh_timer.h | 9 ++
>> >> > 2 files changed, 239 insertions(+), 69 deletions(-)
>> >> >
>> >> > diff --git a/drivers/clocksource/sh_cmt.c
>> >> > b/drivers/clocksource/sh_cmt.c
>> >> > index 5280231..8390f0f 100644
>> >> > --- a/drivers/clocksource/sh_cmt.c
>> >> > +++ b/drivers/clocksource/sh_cmt.c
>
> [snip]
>
>> >> > @@ -85,11 +94,15 @@ struct sh_cmt_info {
>> >> >
>> >> > struct sh_cmt_channel {
>> >> > struct sh_cmt_device *cmt;
>> >> > - unsigned int index;
>> >> > - void __iomem *base;
>> >> > + unsigned int index; /* Index in the documentation */
>> >> > + unsigned int hwidx; /* Real hardware index */
>> >> > +
>> >> > + void __iomem *iostart;
>> >> > + void __iomem *ioctrl;
>> >> >
>> >> > struct irqaction irqaction;
>> >>
>> >> While you are at it, can you please get rid of that irqaction and use
>> >> request_irq() ?
>> >
>> > The driver claims it can't use request_irq() because the function is not
>> > available for early platform devices. If the situation has changed I'd
>> > gladly get rid of irqaction.
>>
>> With the risk of stating the obvious, this depends on how early the
>> early platform device stuff is being run. The actual location may not
>> be the same on SH and ARM for instance.
>>
>> On ARM we are doing all we can to initialize these devices as late as
>> ever possible in the MULTIPLAFORM (DT reference) case. Once we manage
>> to remove the legacy ARM case then there is one less user of early
>> platform timers.
>>
>> One perhaps reasonable way forward could be to use request_irq() in
>> case of DT and leave the legacy platform data case to rely on
>> irqaction.
>
> The whole point of switching from setup_irq() to request_irq() is to simplify
> the code. Adding request_irq() as an option would go in the opposite
> direction.
The point of switching to setup_irq() was not very clear to me. I
think it is fine to switch to using it over time, and I'm open to any
alternative ways forward. Usually moving in incremental steps means
more crap in the short term, but if someone could come up with a way
that involves little crap then I'm all for that! =)
Thanks,
/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
Hi Magnus,
On Monday 17 February 2014 10:41:31 Magnus Damm wrote:
> On Mon, Feb 17, 2014 at 3:18 AM, Laurent Pinchart wrote:
> > On Saturday 15 February 2014 13:46:54 Thomas Gleixner wrote:
> >> On Fri, 14 Feb 2014, Laurent Pinchart wrote:
> >> > CMT hardware devices can support multiple channels, with global
> >> > registers and per-channel registers. The sh_cmt driver currently models
> >> > the hardware with one Linux device per channel. This model makes it
> >> > difficult to handle global registers in a clean way.
> >> >
> >> > Add support for a new model that uses one Linux device per timer with
> >> > multiple channels per device. This requires changes to platform data,
> >> > add new channel configuration fields.
> >> >
> >> > Support for the legacy model is kept and will be removed after all
> >> > platforms switch to the new model.
> >> >
> >> > Signed-off-by: Laurent Pinchart
> >> >
> >> > ---
> >> >
> >> > drivers/clocksource/sh_cmt.c | 299 ++-
> >> > include/linux/sh_timer.h | 9 ++
> >> > 2 files changed, 239 insertions(+), 69 deletions(-)
> >> >
> >> > diff --git a/drivers/clocksource/sh_cmt.c
> >> > b/drivers/clocksource/sh_cmt.c
> >> > index 5280231..8390f0f 100644
> >> > --- a/drivers/clocksource/sh_cmt.c
> >> > +++ b/drivers/clocksource/sh_cmt.c
[snip]
> >> > @@ -85,11 +94,15 @@ struct sh_cmt_info {
> >> >
> >> > struct sh_cmt_channel {
> >> > struct sh_cmt_device *cmt;
> >> > - unsigned int index;
> >> > - void __iomem *base;
> >> > + unsigned int index; /* Index in the documentation */
> >> > + unsigned int hwidx; /* Real hardware index */
> >> > +
> >> > + void __iomem *iostart;
> >> > + void __iomem *ioctrl;
> >> >
> >> > struct irqaction irqaction;
> >>
> >> While you are at it, can you please get rid of that irqaction and use
> >> request_irq() ?
> >
> > The driver claims it can't use request_irq() because the function is not
> > available for early platform devices. If the situation has changed I'd
> > gladly get rid of irqaction.
>
> With the risk of stating the obvious, this depends on how early the
> early platform device stuff is being run. The actual location may not
> be the same on SH and ARM for instance.
>
> On ARM we are doing all we can to initialize these devices as late as
> ever possible in the MULTIPLAFORM (DT reference) case. Once we manage
> to remove the legacy ARM case then there is one less user of early
> platform timers.
>
> One perhaps reasonable way forward could be to use request_irq() in
> case of DT and leave the legacy platform data case to rely on
> irqaction.
The whole point of switching from setup_irq() to request_irq() is to simplify
the code. Adding request_irq() as an option would go in the opposite
direction.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
Hi Laurent,
On Mon, Feb 17, 2014 at 3:18 AM, Laurent Pinchart
wrote:
> Hi Thomas,
>
> On Saturday 15 February 2014 13:46:54 Thomas Gleixner wrote:
>> On Fri, 14 Feb 2014, Laurent Pinchart wrote:
>> > CMT hardware devices can support multiple channels, with global
>> > registers and per-channel registers. The sh_cmt driver currently models
>> > the hardware with one Linux device per channel. This model makes it
>> > difficult to handle global registers in a clean way.
>> >
>> > Add support for a new model that uses one Linux device per timer with
>> > multiple channels per device. This requires changes to platform data,
>> > add new channel configuration fields.
>> >
>> > Support for the legacy model is kept and will be removed after all
>> > platforms switch to the new model.
>> >
>> > Signed-off-by: Laurent Pinchart
>> >
>> > ---
>> >
>> > drivers/clocksource/sh_cmt.c | 299 +-
>> > include/linux/sh_timer.h | 9 ++
>> > 2 files changed, 239 insertions(+), 69 deletions(-)
>> >
>> > diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
>> > index 5280231..8390f0f 100644
>> > --- a/drivers/clocksource/sh_cmt.c
>> > +++ b/drivers/clocksource/sh_cmt.c
>> > @@ -53,7 +53,16 @@ struct sh_cmt_device;
>> >
>> > * channel registers block. All other versions have a shared start/stop
>> > register * located in the global space.
>> > *
>> >
>> > - * Note that CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing
>> > 32-bit + * Channels are indexed from 0 to N-1 in the documentation. The
>> > channel index + * infers the start/stop bit position in the control
>> > register and the channel + * registers block address. Some CMT instances
>> > have a subset of channels + * available, in which case the index in the
>> > documentation doesn't match the + * "real" index as implemented in
>> > hardware. This is for instance the case with + * CMT0 on r8a7740, which
>> > is a 32-bit variant with a single channel numbered 0 + * in the
>> > documentation but using start/stop bit 5 and having its registers + *
>> > block at 0x60.
>> > + *
>> > + * Similarly CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing
>> > 32-bit>
>> > * channels only, is a 48-bit gen2 CMT with the 48-bit channels
>> > unavailable.
>> > */
>> >
>> > @@ -85,11 +94,15 @@ struct sh_cmt_info {
>> >
>> > struct sh_cmt_channel {
>> >
>> > struct sh_cmt_device *cmt;
>> >
>> > - unsigned int index;
>> >
>> > - void __iomem *base;
>> > + unsigned int index; /* Index in the documentation */
>> > + unsigned int hwidx; /* Real hardware index */
>> > +
>> > + void __iomem *iostart;
>> > + void __iomem *ioctrl;
>> >
>> > struct irqaction irqaction;
>>
>> While you are at it, can you please get rid of that irqaction and use
>> request_irq() ?
>
> The driver claims it can't use request_irq() because the function is not
> available for early platform devices. If the situation has changed I'd gladly
> get rid of irqaction.
With the risk of stating the obvious, this depends on how early the
early platform device stuff is being run. The actual location may not
be the same on SH and ARM for instance.
On ARM we are doing all we can to initialize these devices as late as
ever possible in the MULTIPLAFORM (DT reference) case. Once we manage
to remove the legacy ARM case then there is one less user of early
platform timers.
One perhaps reasonable way forward could be to use request_irq() in
case of DT and leave the legacy platform data case to rely on
irqaction.
Cheers,
/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
Hi Thomas,
On Saturday 15 February 2014 13:46:54 Thomas Gleixner wrote:
> On Fri, 14 Feb 2014, Laurent Pinchart wrote:
> > CMT hardware devices can support multiple channels, with global
> > registers and per-channel registers. The sh_cmt driver currently models
> > the hardware with one Linux device per channel. This model makes it
> > difficult to handle global registers in a clean way.
> >
> > Add support for a new model that uses one Linux device per timer with
> > multiple channels per device. This requires changes to platform data,
> > add new channel configuration fields.
> >
> > Support for the legacy model is kept and will be removed after all
> > platforms switch to the new model.
> >
> > Signed-off-by: Laurent Pinchart
> >
> > ---
> >
> > drivers/clocksource/sh_cmt.c | 299 +-
> > include/linux/sh_timer.h | 9 ++
> > 2 files changed, 239 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
> > index 5280231..8390f0f 100644
> > --- a/drivers/clocksource/sh_cmt.c
> > +++ b/drivers/clocksource/sh_cmt.c
> > @@ -53,7 +53,16 @@ struct sh_cmt_device;
> >
> > * channel registers block. All other versions have a shared start/stop
> > register * located in the global space.
> > *
> >
> > - * Note that CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing
> > 32-bit + * Channels are indexed from 0 to N-1 in the documentation. The
> > channel index + * infers the start/stop bit position in the control
> > register and the channel + * registers block address. Some CMT instances
> > have a subset of channels + * available, in which case the index in the
> > documentation doesn't match the + * "real" index as implemented in
> > hardware. This is for instance the case with + * CMT0 on r8a7740, which
> > is a 32-bit variant with a single channel numbered 0 + * in the
> > documentation but using start/stop bit 5 and having its registers + *
> > block at 0x60.
> > + *
> > + * Similarly CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing
> > 32-bit>
> > * channels only, is a 48-bit gen2 CMT with the 48-bit channels
> > unavailable.
> > */
> >
> > @@ -85,11 +94,15 @@ struct sh_cmt_info {
> >
> > struct sh_cmt_channel {
> >
> > struct sh_cmt_device *cmt;
> >
> > - unsigned int index;
> >
> > - void __iomem *base;
> > + unsigned int index; /* Index in the documentation */
> > + unsigned int hwidx; /* Real hardware index */
> > +
> > + void __iomem *iostart;
> > + void __iomem *ioctrl;
> >
> > struct irqaction irqaction;
>
> While you are at it, can you please get rid of that irqaction and use
> request_irq() ?
The driver claims it can't use request_irq() because the function is not
available for early platform devices. If the situation has changed I'd gladly
get rid of irqaction.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
Hi Thomas,
On Saturday 15 February 2014 13:46:54 Thomas Gleixner wrote:
On Fri, 14 Feb 2014, Laurent Pinchart wrote:
CMT hardware devices can support multiple channels, with global
registers and per-channel registers. The sh_cmt driver currently models
the hardware with one Linux device per channel. This model makes it
difficult to handle global registers in a clean way.
Add support for a new model that uses one Linux device per timer with
multiple channels per device. This requires changes to platform data,
add new channel configuration fields.
Support for the legacy model is kept and will be removed after all
platforms switch to the new model.
Signed-off-by: Laurent Pinchart
laurent.pinchart+rene...@ideasonboard.com
---
drivers/clocksource/sh_cmt.c | 299 +-
include/linux/sh_timer.h | 9 ++
2 files changed, 239 insertions(+), 69 deletions(-)
diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index 5280231..8390f0f 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -53,7 +53,16 @@ struct sh_cmt_device;
* channel registers block. All other versions have a shared start/stop
register * located in the global space.
*
- * Note that CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing
32-bit + * Channels are indexed from 0 to N-1 in the documentation. The
channel index + * infers the start/stop bit position in the control
register and the channel + * registers block address. Some CMT instances
have a subset of channels + * available, in which case the index in the
documentation doesn't match the + * real index as implemented in
hardware. This is for instance the case with + * CMT0 on r8a7740, which
is a 32-bit variant with a single channel numbered 0 + * in the
documentation but using start/stop bit 5 and having its registers + *
block at 0x60.
+ *
+ * Similarly CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing
32-bit
* channels only, is a 48-bit gen2 CMT with the 48-bit channels
unavailable.
*/
@@ -85,11 +94,15 @@ struct sh_cmt_info {
struct sh_cmt_channel {
struct sh_cmt_device *cmt;
- unsigned int index;
- void __iomem *base;
+ unsigned int index; /* Index in the documentation */
+ unsigned int hwidx; /* Real hardware index */
+
+ void __iomem *iostart;
+ void __iomem *ioctrl;
struct irqaction irqaction;
While you are at it, can you please get rid of that irqaction and use
request_irq() ?
The driver claims it can't use request_irq() because the function is not
available for early platform devices. If the situation has changed I'd gladly
get rid of irqaction.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
Hi Laurent,
On Mon, Feb 17, 2014 at 3:18 AM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
Hi Thomas,
On Saturday 15 February 2014 13:46:54 Thomas Gleixner wrote:
On Fri, 14 Feb 2014, Laurent Pinchart wrote:
CMT hardware devices can support multiple channels, with global
registers and per-channel registers. The sh_cmt driver currently models
the hardware with one Linux device per channel. This model makes it
difficult to handle global registers in a clean way.
Add support for a new model that uses one Linux device per timer with
multiple channels per device. This requires changes to platform data,
add new channel configuration fields.
Support for the legacy model is kept and will be removed after all
platforms switch to the new model.
Signed-off-by: Laurent Pinchart
laurent.pinchart+rene...@ideasonboard.com
---
drivers/clocksource/sh_cmt.c | 299 +-
include/linux/sh_timer.h | 9 ++
2 files changed, 239 insertions(+), 69 deletions(-)
diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index 5280231..8390f0f 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -53,7 +53,16 @@ struct sh_cmt_device;
* channel registers block. All other versions have a shared start/stop
register * located in the global space.
*
- * Note that CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing
32-bit + * Channels are indexed from 0 to N-1 in the documentation. The
channel index + * infers the start/stop bit position in the control
register and the channel + * registers block address. Some CMT instances
have a subset of channels + * available, in which case the index in the
documentation doesn't match the + * real index as implemented in
hardware. This is for instance the case with + * CMT0 on r8a7740, which
is a 32-bit variant with a single channel numbered 0 + * in the
documentation but using start/stop bit 5 and having its registers + *
block at 0x60.
+ *
+ * Similarly CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing
32-bit
* channels only, is a 48-bit gen2 CMT with the 48-bit channels
unavailable.
*/
@@ -85,11 +94,15 @@ struct sh_cmt_info {
struct sh_cmt_channel {
struct sh_cmt_device *cmt;
- unsigned int index;
- void __iomem *base;
+ unsigned int index; /* Index in the documentation */
+ unsigned int hwidx; /* Real hardware index */
+
+ void __iomem *iostart;
+ void __iomem *ioctrl;
struct irqaction irqaction;
While you are at it, can you please get rid of that irqaction and use
request_irq() ?
The driver claims it can't use request_irq() because the function is not
available for early platform devices. If the situation has changed I'd gladly
get rid of irqaction.
With the risk of stating the obvious, this depends on how early the
early platform device stuff is being run. The actual location may not
be the same on SH and ARM for instance.
On ARM we are doing all we can to initialize these devices as late as
ever possible in the MULTIPLAFORM (DT reference) case. Once we manage
to remove the legacy ARM case then there is one less user of early
platform timers.
One perhaps reasonable way forward could be to use request_irq() in
case of DT and leave the legacy platform data case to rely on
irqaction.
Cheers,
/ magnus
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
Hi Magnus,
On Monday 17 February 2014 10:41:31 Magnus Damm wrote:
On Mon, Feb 17, 2014 at 3:18 AM, Laurent Pinchart wrote:
On Saturday 15 February 2014 13:46:54 Thomas Gleixner wrote:
On Fri, 14 Feb 2014, Laurent Pinchart wrote:
CMT hardware devices can support multiple channels, with global
registers and per-channel registers. The sh_cmt driver currently models
the hardware with one Linux device per channel. This model makes it
difficult to handle global registers in a clean way.
Add support for a new model that uses one Linux device per timer with
multiple channels per device. This requires changes to platform data,
add new channel configuration fields.
Support for the legacy model is kept and will be removed after all
platforms switch to the new model.
Signed-off-by: Laurent Pinchart
laurent.pinchart+rene...@ideasonboard.com
---
drivers/clocksource/sh_cmt.c | 299 ++-
include/linux/sh_timer.h | 9 ++
2 files changed, 239 insertions(+), 69 deletions(-)
diff --git a/drivers/clocksource/sh_cmt.c
b/drivers/clocksource/sh_cmt.c
index 5280231..8390f0f 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
[snip]
@@ -85,11 +94,15 @@ struct sh_cmt_info {
struct sh_cmt_channel {
struct sh_cmt_device *cmt;
- unsigned int index;
- void __iomem *base;
+ unsigned int index; /* Index in the documentation */
+ unsigned int hwidx; /* Real hardware index */
+
+ void __iomem *iostart;
+ void __iomem *ioctrl;
struct irqaction irqaction;
While you are at it, can you please get rid of that irqaction and use
request_irq() ?
The driver claims it can't use request_irq() because the function is not
available for early platform devices. If the situation has changed I'd
gladly get rid of irqaction.
With the risk of stating the obvious, this depends on how early the
early platform device stuff is being run. The actual location may not
be the same on SH and ARM for instance.
On ARM we are doing all we can to initialize these devices as late as
ever possible in the MULTIPLAFORM (DT reference) case. Once we manage
to remove the legacy ARM case then there is one less user of early
platform timers.
One perhaps reasonable way forward could be to use request_irq() in
case of DT and leave the legacy platform data case to rely on
irqaction.
The whole point of switching from setup_irq() to request_irq() is to simplify
the code. Adding request_irq() as an option would go in the opposite
direction.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
Hi Laurent,
On Mon, Feb 17, 2014 at 10:48 AM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
Hi Magnus,
On Monday 17 February 2014 10:41:31 Magnus Damm wrote:
On Mon, Feb 17, 2014 at 3:18 AM, Laurent Pinchart wrote:
On Saturday 15 February 2014 13:46:54 Thomas Gleixner wrote:
On Fri, 14 Feb 2014, Laurent Pinchart wrote:
CMT hardware devices can support multiple channels, with global
registers and per-channel registers. The sh_cmt driver currently models
the hardware with one Linux device per channel. This model makes it
difficult to handle global registers in a clean way.
Add support for a new model that uses one Linux device per timer with
multiple channels per device. This requires changes to platform data,
add new channel configuration fields.
Support for the legacy model is kept and will be removed after all
platforms switch to the new model.
Signed-off-by: Laurent Pinchart
laurent.pinchart+rene...@ideasonboard.com
---
drivers/clocksource/sh_cmt.c | 299 ++-
include/linux/sh_timer.h | 9 ++
2 files changed, 239 insertions(+), 69 deletions(-)
diff --git a/drivers/clocksource/sh_cmt.c
b/drivers/clocksource/sh_cmt.c
index 5280231..8390f0f 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
[snip]
@@ -85,11 +94,15 @@ struct sh_cmt_info {
struct sh_cmt_channel {
struct sh_cmt_device *cmt;
- unsigned int index;
- void __iomem *base;
+ unsigned int index; /* Index in the documentation */
+ unsigned int hwidx; /* Real hardware index */
+
+ void __iomem *iostart;
+ void __iomem *ioctrl;
struct irqaction irqaction;
While you are at it, can you please get rid of that irqaction and use
request_irq() ?
The driver claims it can't use request_irq() because the function is not
available for early platform devices. If the situation has changed I'd
gladly get rid of irqaction.
With the risk of stating the obvious, this depends on how early the
early platform device stuff is being run. The actual location may not
be the same on SH and ARM for instance.
On ARM we are doing all we can to initialize these devices as late as
ever possible in the MULTIPLAFORM (DT reference) case. Once we manage
to remove the legacy ARM case then there is one less user of early
platform timers.
One perhaps reasonable way forward could be to use request_irq() in
case of DT and leave the legacy platform data case to rely on
irqaction.
The whole point of switching from setup_irq() to request_irq() is to simplify
the code. Adding request_irq() as an option would go in the opposite
direction.
The point of switching to setup_irq() was not very clear to me. I
think it is fine to switch to using it over time, and I'm open to any
alternative ways forward. Usually moving in incremental steps means
more crap in the short term, but if someone could come up with a way
that involves little crap then I'm all for that! =)
Thanks,
/ magnus
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
On Fri, 14 Feb 2014, Laurent Pinchart wrote:
> CMT hardware devices can support multiple channels, with global
> registers and per-channel registers. The sh_cmt driver currently models
> the hardware with one Linux device per channel. This model makes it
> difficult to handle global registers in a clean way.
>
> Add support for a new model that uses one Linux device per timer with
> multiple channels per device. This requires changes to platform data,
> add new channel configuration fields.
>
> Support for the legacy model is kept and will be removed after all
> platforms switch to the new model.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/clocksource/sh_cmt.c | 299
> +--
> include/linux/sh_timer.h | 9 ++
> 2 files changed, 239 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
> index 5280231..8390f0f 100644
> --- a/drivers/clocksource/sh_cmt.c
> +++ b/drivers/clocksource/sh_cmt.c
> @@ -53,7 +53,16 @@ struct sh_cmt_device;
> * channel registers block. All other versions have a shared start/stop
> register
> * located in the global space.
> *
> - * Note that CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing 32-bit
> + * Channels are indexed from 0 to N-1 in the documentation. The channel index
> + * infers the start/stop bit position in the control register and the channel
> + * registers block address. Some CMT instances have a subset of channels
> + * available, in which case the index in the documentation doesn't match the
> + * "real" index as implemented in hardware. This is for instance the case
> with
> + * CMT0 on r8a7740, which is a 32-bit variant with a single channel numbered > 0
> + * in the documentation but using start/stop bit 5 and having its registers
> + * block at 0x60.
> + *
> + * Similarly CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing 32-bit
> * channels only, is a 48-bit gen2 CMT with the 48-bit channels unavailable.
> */
>
> @@ -85,11 +94,15 @@ struct sh_cmt_info {
>
> struct sh_cmt_channel {
> struct sh_cmt_device *cmt;
> - unsigned int index;
>
> - void __iomem *base;
> + unsigned int index; /* Index in the documentation */
> + unsigned int hwidx; /* Real hardware index */
> +
> + void __iomem *iostart;
> + void __iomem *ioctrl;
> struct irqaction irqaction;
While you are at it, can you please get rid of that irqaction and use
request_irq() ?
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/27] clocksource: sh_cmt: Add support for multiple channels per device
On Fri, 14 Feb 2014, Laurent Pinchart wrote:
CMT hardware devices can support multiple channels, with global
registers and per-channel registers. The sh_cmt driver currently models
the hardware with one Linux device per channel. This model makes it
difficult to handle global registers in a clean way.
Add support for a new model that uses one Linux device per timer with
multiple channels per device. This requires changes to platform data,
add new channel configuration fields.
Support for the legacy model is kept and will be removed after all
platforms switch to the new model.
Signed-off-by: Laurent Pinchart laurent.pinchart+rene...@ideasonboard.com
---
drivers/clocksource/sh_cmt.c | 299
+--
include/linux/sh_timer.h | 9 ++
2 files changed, 239 insertions(+), 69 deletions(-)
diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index 5280231..8390f0f 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -53,7 +53,16 @@ struct sh_cmt_device;
* channel registers block. All other versions have a shared start/stop
register
* located in the global space.
*
- * Note that CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing 32-bit
+ * Channels are indexed from 0 to N-1 in the documentation. The channel index
+ * infers the start/stop bit position in the control register and the channel
+ * registers block address. Some CMT instances have a subset of channels
+ * available, in which case the index in the documentation doesn't match the
+ * real index as implemented in hardware. This is for instance the case
with
+ * CMT0 on r8a7740, which is a 32-bit variant with a single channel numbered 0
+ * in the documentation but using start/stop bit 5 and having its registers
+ * block at 0x60.
+ *
+ * Similarly CMT0 on r8a73a4, r8a7790 and r8a7791, while implementing 32-bit
* channels only, is a 48-bit gen2 CMT with the 48-bit channels unavailable.
*/
@@ -85,11 +94,15 @@ struct sh_cmt_info {
struct sh_cmt_channel {
struct sh_cmt_device *cmt;
- unsigned int index;
- void __iomem *base;
+ unsigned int index; /* Index in the documentation */
+ unsigned int hwidx; /* Real hardware index */
+
+ void __iomem *iostart;
+ void __iomem *ioctrl;
struct irqaction irqaction;
While you are at it, can you please get rid of that irqaction and use
request_irq() ?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
