[PATCH v2] clocksource: Add BE APIs support for clocksource counter reading.

[PATCH v2] clocksource: Add BE APIs support for clocksource counter reading.

[Xiubo Li]

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---

Hi Daniel, Thomas

For now I just added  _be() support using ioread{16,32}be.
And i have a check of the clocksource drivers, didn't find anyone
using LE mode on one BE SoC, so _le() APIs is not needed.


For V2:
- Using ioread{16,32}[be]() instead of readl_relaxed().
- Add clocksource_mmio_readX_Y_be() supports only.

 drivers/clocksource/mmio.c  | 28 ++++++++++++++++++++++++----
 include/linux/clocksource.h |  4 ++++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
index 1593ade..d35a407 100644
--- a/drivers/clocksource/mmio.c
+++ b/drivers/clocksource/mmio.c
@@ -20,24 +20,44 @@ static inline struct clocksource_mmio *to_mmio_clksrc(struct clocksource *c)
 	return container_of(c, struct clocksource_mmio, clksrc);
 }
 
+cycle_t clocksource_mmio_readl_up_be(struct clocksource *c)
+{
+	return (cycle_t)ioread32be(to_mmio_clksrc(c)->reg);
+}
+
 cycle_t clocksource_mmio_readl_up(struct clocksource *c)
 {
-	return (cycle_t)readl_relaxed(to_mmio_clksrc(c)->reg);
+	return (cycle_t)ioread32(to_mmio_clksrc(c)->reg);
+}
+
+cycle_t clocksource_mmio_readl_down_be(struct clocksource *c)
+{
+	return ~(cycle_t)ioread32be(to_mmio_clksrc(c)->reg) & c->mask;
 }
 
 cycle_t clocksource_mmio_readl_down(struct clocksource *c)
 {
-	return ~(cycle_t)readl_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
+	return ~(cycle_t)ioread32(to_mmio_clksrc(c)->reg) & c->mask;
+}
+
+cycle_t clocksource_mmio_readw_up_be(struct clocksource *c)
+{
+	return (cycle_t)ioread16be(to_mmio_clksrc(c)->reg);
 }
 
 cycle_t clocksource_mmio_readw_up(struct clocksource *c)
 {
-	return (cycle_t)readw_relaxed(to_mmio_clksrc(c)->reg);
+	return (cycle_t)ioread16(to_mmio_clksrc(c)->reg);
+}
+
+cycle_t clocksource_mmio_readw_down_be(struct clocksource *c)
+{
+	return ~(cycle_t)ioread16be(to_mmio_clksrc(c)->reg) & c->mask;
 }
 
 cycle_t clocksource_mmio_readw_down(struct clocksource *c)
 {
-	return ~(cycle_t)readw_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
+	return ~(cycle_t)ioread16(to_mmio_clksrc(c)->reg) & c->mask;
 }
 
 /**
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 653f0e2..931a037 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -327,9 +327,13 @@ static inline void __clocksource_updatefreq_khz(struct clocksource *cs, u32 khz)
 
 extern int timekeeping_notify(struct clocksource *clock);
 
+extern cycle_t clocksource_mmio_readl_up_be(struct clocksource *);
 extern cycle_t clocksource_mmio_readl_up(struct clocksource *);
+extern cycle_t clocksource_mmio_readl_down_be(struct clocksource *);
 extern cycle_t clocksource_mmio_readl_down(struct clocksource *);
+extern cycle_t clocksource_mmio_readw_up_be(struct clocksource *);
 extern cycle_t clocksource_mmio_readw_up(struct clocksource *);
+extern cycle_t clocksource_mmio_readw_down_be(struct clocksource *);
 extern cycle_t clocksource_mmio_readw_down(struct clocksource *);
 
 extern int clocksource_mmio_init(void __iomem *, const char *,
-- 
2.1.0.27.g96db324

Re: [PATCH v2] clocksource: Add BE APIs support for clocksource counter reading.

[Thomas Gleixner]

On Fri, 26 Sep 2014, Xiubo Li wrote:
> For now I just added  _be() support using ioread{16,32}be.
> And i have a check of the clocksource drivers, didn't find anyone
> using LE mode on one BE SoC, so _le() APIs is not needed.

Nonsense. The existing clocksource_mmio accessor function are
providing LE access independent of the CPU endianess. So we don't need
an _le() API simply because we have it already.
 
>  cycle_t clocksource_mmio_readl_up(struct clocksource *c)
>  {
> -	return (cycle_t)readl_relaxed(to_mmio_clksrc(c)->reg);
> +	return (cycle_t)ioread32(to_mmio_clksrc(c)->reg);

And how exactly is this change related to adding BE support?

Thanks,

	tglx

RE: [PATCH v2] clocksource: Add BE APIs support for clocksource counter reading.

[Li.Xiubo]

Hi,

> Subject: Re: [PATCH v2] clocksource: Add BE APIs support for clocksource
> counter reading.
> 
> On Fri, 26 Sep 2014, Xiubo Li wrote:
> > For now I just added  _be() support using ioread{16,32}be.
> > And i have a check of the clocksource drivers, didn't find anyone
> > using LE mode on one BE SoC, so _le() APIs is not needed.
> 
> Nonsense. The existing clocksource_mmio accessor function are
> providing LE access independent of the CPU endianess. So we don't need
> an _le() API simply because we have it already.
> 
> >  cycle_t clocksource_mmio_readl_up(struct clocksource *c)
> >  {
> > -	return (cycle_t)readl_relaxed(to_mmio_clksrc(c)->reg);
> > +	return (cycle_t)ioread32(to_mmio_clksrc(c)->reg);
> 
> And how exactly is this change related to adding BE support?
> 

Actually not very much, since the _be() APIs are using ioread{16,32}be(),
so I think using ioread{16,32}() will be less odd to having two different
accessors here.

Wouldn't this be more unified somehow ?  

Thanks very much,

BRs
Xiubo

> Thanks,
> 
> 	tglx

RE: [PATCH v2] clocksource: Add BE APIs support for clocksource counter reading.

[Thomas Gleixner]

On Sun, 28 Sep 2014, Li.Xiubo@freescale.com wrote:
> > Subject: Re: [PATCH v2] clocksource: Add BE APIs support for clocksource
> > counter reading.
> > 
> > On Fri, 26 Sep 2014, Xiubo Li wrote:
> > > For now I just added  _be() support using ioread{16,32}be.
> > > And i have a check of the clocksource drivers, didn't find anyone
> > > using LE mode on one BE SoC, so _le() APIs is not needed.
> > 
> > Nonsense. The existing clocksource_mmio accessor function are
> > providing LE access independent of the CPU endianess. So we don't need
> > an _le() API simply because we have it already.
> > 
> > >  cycle_t clocksource_mmio_readl_up(struct clocksource *c)
> > >  {
> > > -	return (cycle_t)readl_relaxed(to_mmio_clksrc(c)->reg);
> > > +	return (cycle_t)ioread32(to_mmio_clksrc(c)->reg);
> > 
> > And how exactly is this change related to adding BE support?
> > 
> 
> Actually not very much, since the _be() APIs are using ioread{16,32}be(),
> so I think using ioread{16,32}() will be less odd to having two different
> accessors here.
> 
> Wouldn't this be more unified somehow ?  

Changing existing code wants to be a separate patch with a proper
changelog and a proper argument WHY it needs to be changed in the
first place.

So please provide that separate patch first with a VERY REASONABLE
explanation in the changelog WHY the existing readl_relaxed() should
be replaced by ioread32().

Thanks,

	tglx

RE: [PATCH v2] clocksource: Add BE APIs support for clocksource counter reading.

[Li.Xiubo]

Hi,

> Subject: RE: [PATCH v2] clocksource: Add BE APIs support for clocksource
> counter reading.
> 
> On Sun, 28 Sep 2014, Li.Xiubo@freescale.com wrote:
> > > Subject: Re: [PATCH v2] clocksource: Add BE APIs support for clocksource
> > > counter reading.
> > >
> > > On Fri, 26 Sep 2014, Xiubo Li wrote:
> > > > For now I just added  _be() support using ioread{16,32}be.
> > > > And i have a check of the clocksource drivers, didn't find anyone
> > > > using LE mode on one BE SoC, so _le() APIs is not needed.
> > >
> > > Nonsense. The existing clocksource_mmio accessor function are
> > > providing LE access independent of the CPU endianess. So we don't need
> > > an _le() API simply because we have it already.
> > >
> > > >  cycle_t clocksource_mmio_readl_up(struct clocksource *c)
> > > >  {
> > > > -	return (cycle_t)readl_relaxed(to_mmio_clksrc(c)->reg);
> > > > +	return (cycle_t)ioread32(to_mmio_clksrc(c)->reg);
> > >
> > > And how exactly is this change related to adding BE support?
> > >
> >
> > Actually not very much, since the _be() APIs are using ioread{16,32}be(),
> > so I think using ioread{16,32}() will be less odd to having two different
> > accessors here.
> >
> > Wouldn't this be more unified somehow ?
> 
> Changing existing code wants to be a separate patch with a proper
> changelog and a proper argument WHY it needs to be changed in the
> first place.
> 
> So please provide that separate patch first with a VERY REASONABLE
> explanation in the changelog WHY the existing readl_relaxed() should
> be replaced by ioread32().
> 

Okay, I will follow your advice.

Thanks,

BRs
Xiubo




> Thanks,
> 
> 	tglx