RE: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read API.
Hi Thomas, > Subject: RE: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read > API. > > Hi Thomas, > > > > > > Since the FTM will be in BE mode on LS1 platform, but will be in LE > mode > > > > > On LS2 platform. > > > > > > > > > > And ftm_clocksource_read_up() will adapt to this different. > > > > > > > > You are missing the point. Why do you want a conditional in a hot > > > > path? You know at init time whether the thing is BE or LE, so you can > > > > have separate functions for BE/LE or whatever and register that with > > > > clocksource_mmio_init(). i.e. > > For our LS1 and LS2+ platforms, there will be only one Kernel Image and can > work > for both of them with different dtbs. > Additional: The LS1 CPUs and some DEVs are in LE endian mode, while most DEVs are in BE mode. For LS2, CPUs and all DEVs are in LE endian mode. So here we used one dt node property to distinguish this in run time. Thanks, BRs Xiubo -- 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 2/5] Clocksource: Flextimer: Use internal clocksource read API.
Hi Thomas, > > > > Since the FTM will be in BE mode on LS1 platform, but will be in LE mode > > > > On LS2 platform. > > > > > > > > And ftm_clocksource_read_up() will adapt to this different. > > > > > > You are missing the point. Why do you want a conditional in a hot > > > path? You know at init time whether the thing is BE or LE, so you can > > > have separate functions for BE/LE or whatever and register that with > > > clocksource_mmio_init(). i.e. For our LS1 and LS2+ platforms, there will be only one Kernel Image and can work for both of them with different dtbs. Thanks, BRs Xiubo -- 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 2/5] Clocksource: Flextimer: Use internal clocksource read API.
On Thu, 4 Sep 2014, li.xi...@freescale.com wrote:
> > >
> > > Since the FTM will be in BE mode on LS1 platform, but will be in LE mode
> > > On LS2 platform.
> > >
> > > And ftm_clocksource_read_up() will adapt to this different.
> >
> > You are missing the point. Why do you want a conditional in a hot
> > path? You know at init time whether the thing is BE or LE, so you can
> > have separate functions for BE/LE or whatever and register that with
> > clocksource_mmio_init(). i.e.
> >
> > if (be)
> > clocksource_mmio_init(., cs_read_be);
> > else if (le)
> > clocksource_mmio_init(., cs_read_le);
> > else if (magic)
> > clocksource_mmio_init(., cs_read_magic);
> >
>
> There already has the following access interfaces:
>
> static inline u32 ftm_readl(void __iomem *addr)
> {
> if (priv->big_endian)
> return ioread32be(addr);
> else
> return ioread32(addr);
> }
>
> static inline void ftm_writel(u32 val, void __iomem *addr)
> {
> if (priv->big_endian)
> iowrite32be(val, addr);
> else
> iowrite32(val, addr);
> }
>
> So I added the following code:
>
> static cycle_t ftm_clocksource_read_up(struct clocksource *c)
> {
> return ftm_readl(priv->clksrc_base + FTM_CNT) & 0x;
> }
>
> clocksource_mmio_init(.,ftm_clocksource_read_up);
>
> Is this okay ?
No. Sit down, read and try to understand what I wrote, look at your
existing code and figure out WHY it is fundamentally different to what
I told you.
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 2/5] Clocksource: Flextimer: Use internal clocksource read API.
> >
> > Since the FTM will be in BE mode on LS1 platform, but will be in LE mode
> > On LS2 platform.
> >
> > And ftm_clocksource_read_up() will adapt to this different.
>
> You are missing the point. Why do you want a conditional in a hot
> path? You know at init time whether the thing is BE or LE, so you can
> have separate functions for BE/LE or whatever and register that with
> clocksource_mmio_init(). i.e.
>
> if (be)
> clocksource_mmio_init(., cs_read_be);
> else if (le)
> clocksource_mmio_init(., cs_read_le);
> else if (magic)
> clocksource_mmio_init(., cs_read_magic);
>
There already has the following access interfaces:
static inline u32 ftm_readl(void __iomem *addr)
{
if (priv->big_endian)
return ioread32be(addr);
else
return ioread32(addr);
}
static inline void ftm_writel(u32 val, void __iomem *addr)
{
if (priv->big_endian)
iowrite32be(val, addr);
else
iowrite32(val, addr);
}
So I added the following code:
static cycle_t ftm_clocksource_read_up(struct clocksource *c)
{
return ftm_readl(priv->clksrc_base + FTM_CNT) & 0x;
}
clocksource_mmio_init(.,ftm_clocksource_read_up);
Is this okay ?
Thanks,
BRs
Xiubo
--
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 2/5] Clocksource: Flextimer: Use internal clocksource read API.
On Thu, 4 Sep 2014, li.xi...@freescale.com wrote: > > Subject: Re: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource > > read > > API. > > > > On Tue, 26 Aug 2014, Xiubo Li wrote: > > > > > Since the Flextimer device will be implemented in BE mode on > > > LS1 SoC, and in LE mode on Vybrid, LS2 SoCs, so here we need > > > the endianness judgment before doing the mmio. > > > > Brilliant. So for every clocksource read you take a conditional. > > > > > @@ -238,7 +243,7 @@ static int __init ftm_clocksource_init(unsigned long > > freq) > > > sched_clock_register(ftm_read_sched_clock, 16, freq / (1 << priv->ps)); > > > err = clocksource_mmio_init(priv->clksrc_base + FTM_CNT, "fsl-ftm", > > > freq / (1 << priv->ps), 300, 16, > > > - clocksource_mmio_readl_up); > > > + ftm_clocksource_read_up); > > > > What's wrong with having endianess aware clocksource_mmio functions > > and make the decision at init time? > > > > Since the FTM will be in BE mode on LS1 platform, but will be in LE mode > On LS2 platform. > > And ftm_clocksource_read_up() will adapt to this different. You are missing the point. Why do you want a conditional in a hot path? You know at init time whether the thing is BE or LE, so you can have separate functions for BE/LE or whatever and register that with clocksource_mmio_init(). i.e. if (be) clocksource_mmio_init(., cs_read_be); else if (le) clocksource_mmio_init(., cs_read_le); else if (magic) clocksource_mmio_init(., cs_read_magic); Hmm? 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 2/5] Clocksource: Flextimer: Use internal clocksource read API.
> Subject: Re: [PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read > API. > > On Tue, 26 Aug 2014, Xiubo Li wrote: > > > Since the Flextimer device will be implemented in BE mode on > > LS1 SoC, and in LE mode on Vybrid, LS2 SoCs, so here we need > > the endianness judgment before doing the mmio. > > Brilliant. So for every clocksource read you take a conditional. > > > @@ -238,7 +243,7 @@ static int __init ftm_clocksource_init(unsigned long > freq) > > sched_clock_register(ftm_read_sched_clock, 16, freq / (1 << priv->ps)); > > err = clocksource_mmio_init(priv->clksrc_base + FTM_CNT, "fsl-ftm", > > freq / (1 << priv->ps), 300, 16, > > - clocksource_mmio_readl_up); > > + ftm_clocksource_read_up); > > What's wrong with having endianess aware clocksource_mmio functions > and make the decision at init time? > Since the FTM will be in BE mode on LS1 platform, but will be in LE mode On LS2 platform. And ftm_clocksource_read_up() will adapt to this different. Thanks, BRs Xiubo -- 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 2/5] Clocksource: Flextimer: Use internal clocksource read API.
On Tue, 26 Aug 2014, Xiubo Li wrote: > Since the Flextimer device will be implemented in BE mode on > LS1 SoC, and in LE mode on Vybrid, LS2 SoCs, so here we need > the endianness judgment before doing the mmio. Brilliant. So for every clocksource read you take a conditional. > @@ -238,7 +243,7 @@ static int __init ftm_clocksource_init(unsigned long freq) > sched_clock_register(ftm_read_sched_clock, 16, freq / (1 << priv->ps)); > err = clocksource_mmio_init(priv->clksrc_base + FTM_CNT, "fsl-ftm", > freq / (1 << priv->ps), 300, 16, > - clocksource_mmio_readl_up); > + ftm_clocksource_read_up); What's wrong with having endianess aware clocksource_mmio functions and make the decision at init time? 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/
[PATCH 2/5] Clocksource: Flextimer: Use internal clocksource read API.
Since the Flextimer device will be implemented in BE mode on
LS1 SoC, and in LE mode on Vybrid, LS2 SoCs, so here we need
the endianness judgment before doing the mmio.
Signed-off-by: Xiubo Li
Signed-off-by: Jingchang Lu
---
drivers/clocksource/fsl_ftm_timer.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/clocksource/fsl_ftm_timer.c
b/drivers/clocksource/fsl_ftm_timer.c
index 13222c1..c4d23d0f 100644
--- a/drivers/clocksource/fsl_ftm_timer.c
+++ b/drivers/clocksource/fsl_ftm_timer.c
@@ -226,6 +226,11 @@ static int __init ftm_clockevent_init(unsigned long freq,
int irq)
return 0;
}
+static cycle_t ftm_clocksource_read_up(struct clocksource *c)
+{
+ return ftm_readl(clksrc_base + FTM_CNT) & 0x;
+}
+
static int __init ftm_clocksource_init(unsigned long freq)
{
int err;
@@ -238,7 +243,7 @@ static int __init ftm_clocksource_init(unsigned long freq)
sched_clock_register(ftm_read_sched_clock, 16, freq / (1 << priv->ps));
err = clocksource_mmio_init(priv->clksrc_base + FTM_CNT, "fsl-ftm",
freq / (1 << priv->ps), 300, 16,
- clocksource_mmio_readl_up);
+ ftm_clocksource_read_up);
if (err) {
pr_err("ftm: init clock source mmio failed: %d\n", err);
return err;
--
1.8.5
--
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/
