RE: [PATCH 5/5] Clocksource: Flextimer: Use Macro for clock source selection.
On Thu, 4 Sep 2014, li.xi...@freescale.com wrote: > > So you remove the FTM_SC_PS_MASK without mentioning it in the > > changelog. Either this is by mistake or it wants to be documented WHY > > it is not needed in the first place. > > > > This is just prepare for the other new features in the future. So split it into a separate patch and provide a proper changelog WHY you remove it. 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 5/5] Clocksource: Flextimer: Use Macro for clock source selection.
On Thu, 4 Sep 2014, li.xi...@freescale.com wrote: So you remove the FTM_SC_PS_MASK without mentioning it in the changelog. Either this is by mistake or it wants to be documented WHY it is not needed in the first place. This is just prepare for the other new features in the future. So split it into a separate patch and provide a proper changelog WHY you remove it. 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 5/5] Clocksource: Flextimer: Use Macro for clock source selection.
> Subject: Re: [PATCH 5/5] Clocksource: Flextimer: Use Macro for clock source > selection. > > On Tue, 26 Aug 2014, Xiubo Li wrote: > > FTM source clock is selectable: > > Source clock can be the system clock, the fixed frequency clock, > > or an external clock. > > Fixed frequency clock is an additional clock input to allow the > > selection of an on chip clock source other than the system clock. > > Selecting external clock connects FTM clock to a chip level input > > pin therefore allowing to synchronize the FTM counter with an off > > chip clock source > > > > Clock Source Selection: > > Selects one of the three FTM counter clock sources. > > > > 00 No clock selected. This in effect disables the FTM counter. > > 01 System clock > > --- > > 10 Fixed frequency clock > > 11 External clock > > > > These two will be useful for the alarm on LS1 platform in late future, > > the system clock's frequency is too high to get a long delay timer when > > go to sleep, so the fixed or external clock could be used instead. > > And how is that relevant to changing the mask from a hardcoded value > to a constant? It's still hardcoded at compile time and I don't see > how that helps chosing the clock at runtime for a particular use case. > > > #define FTM_SC 0x00 > > +#define FTM_SC_CLKS_NON0 > > +#define FTM_SC_CLKS_SYS1 > > #define FTM_SC_CLK_SHIFT 3 > > #define FTM_SC_CLK_MASK(0x3 << FTM_SC_CLK_SHIFT) > > #define FTM_SC_CLK(c) ((c) << FTM_SC_CLK_SHIFT) > > @@ -67,7 +69,7 @@ static inline void ftm_counter_enable(void __iomem *base) > > /* select and enable counter clock source */ > > val = ftm_readl(base + FTM_SC); > > val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); > > - val |= priv->ps | FTM_SC_CLK(1); > > + val |= priv->ps | FTM_SC_CLK(FTM_SC_CLKS_SYS); > > ftm_writel(val, base + FTM_SC); > > } > > > > @@ -77,7 +79,8 @@ static inline void ftm_counter_disable(void __iomem *base) > > > > /* disable counter clock source */ > > val = ftm_readl(base + FTM_SC); > > - val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); > > + val &= ~(FTM_SC_CLK_MASK); > > So you remove the FTM_SC_PS_MASK without mentioning it in the > changelog. Either this is by mistake or it wants to be documented WHY > it is not needed in the first place. > This is just prepare for the other new features in the future. And delay this changes maybe better. 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 5/5] Clocksource: Flextimer: Use Macro for clock source selection.
On Tue, 26 Aug 2014, Xiubo Li wrote: > FTM source clock is selectable: > Source clock can be the system clock, the fixed frequency clock, > or an external clock. > Fixed frequency clock is an additional clock input to allow the > selection of an on chip clock source other than the system clock. > Selecting external clock connects FTM clock to a chip level input > pin therefore allowing to synchronize the FTM counter with an off > chip clock source > > Clock Source Selection: > Selects one of the three FTM counter clock sources. > > 00 No clock selected. This in effect disables the FTM counter. > 01 System clock > --- > 10 Fixed frequency clock > 11 External clock > > These two will be useful for the alarm on LS1 platform in late future, > the system clock's frequency is too high to get a long delay timer when > go to sleep, so the fixed or external clock could be used instead. And how is that relevant to changing the mask from a hardcoded value to a constant? It's still hardcoded at compile time and I don't see how that helps chosing the clock at runtime for a particular use case. > #define FTM_SC 0x00 > +#define FTM_SC_CLKS_NON 0 > +#define FTM_SC_CLKS_SYS 1 > #define FTM_SC_CLK_SHIFT 3 > #define FTM_SC_CLK_MASK (0x3 << FTM_SC_CLK_SHIFT) > #define FTM_SC_CLK(c)((c) << FTM_SC_CLK_SHIFT) > @@ -67,7 +69,7 @@ static inline void ftm_counter_enable(void __iomem *base) > /* select and enable counter clock source */ > val = ftm_readl(base + FTM_SC); > val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); > - val |= priv->ps | FTM_SC_CLK(1); > + val |= priv->ps | FTM_SC_CLK(FTM_SC_CLKS_SYS); > ftm_writel(val, base + FTM_SC); > } > > @@ -77,7 +79,8 @@ static inline void ftm_counter_disable(void __iomem *base) > > /* disable counter clock source */ > val = ftm_readl(base + FTM_SC); > - val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); > + val &= ~(FTM_SC_CLK_MASK); So you remove the FTM_SC_PS_MASK without mentioning it in the changelog. Either this is by mistake or it wants to be documented WHY it is not needed in the first place. 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 5/5] Clocksource: Flextimer: Use Macro for clock source selection.
On Tue, 26 Aug 2014, Xiubo Li wrote: FTM source clock is selectable: Source clock can be the system clock, the fixed frequency clock, or an external clock. Fixed frequency clock is an additional clock input to allow the selection of an on chip clock source other than the system clock. Selecting external clock connects FTM clock to a chip level input pin therefore allowing to synchronize the FTM counter with an off chip clock source Clock Source Selection: Selects one of the three FTM counter clock sources. 00 No clock selected. This in effect disables the FTM counter. 01 System clock --- 10 Fixed frequency clock 11 External clock These two will be useful for the alarm on LS1 platform in late future, the system clock's frequency is too high to get a long delay timer when go to sleep, so the fixed or external clock could be used instead. And how is that relevant to changing the mask from a hardcoded value to a constant? It's still hardcoded at compile time and I don't see how that helps chosing the clock at runtime for a particular use case. #define FTM_SC 0x00 +#define FTM_SC_CLKS_NON 0 +#define FTM_SC_CLKS_SYS 1 #define FTM_SC_CLK_SHIFT 3 #define FTM_SC_CLK_MASK (0x3 FTM_SC_CLK_SHIFT) #define FTM_SC_CLK(c)((c) FTM_SC_CLK_SHIFT) @@ -67,7 +69,7 @@ static inline void ftm_counter_enable(void __iomem *base) /* select and enable counter clock source */ val = ftm_readl(base + FTM_SC); val = ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); - val |= priv-ps | FTM_SC_CLK(1); + val |= priv-ps | FTM_SC_CLK(FTM_SC_CLKS_SYS); ftm_writel(val, base + FTM_SC); } @@ -77,7 +79,8 @@ static inline void ftm_counter_disable(void __iomem *base) /* disable counter clock source */ val = ftm_readl(base + FTM_SC); - val = ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); + val = ~(FTM_SC_CLK_MASK); So you remove the FTM_SC_PS_MASK without mentioning it in the changelog. Either this is by mistake or it wants to be documented WHY it is not needed in the first place. 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 5/5] Clocksource: Flextimer: Use Macro for clock source selection.
Subject: Re: [PATCH 5/5] Clocksource: Flextimer: Use Macro for clock source selection. On Tue, 26 Aug 2014, Xiubo Li wrote: FTM source clock is selectable: Source clock can be the system clock, the fixed frequency clock, or an external clock. Fixed frequency clock is an additional clock input to allow the selection of an on chip clock source other than the system clock. Selecting external clock connects FTM clock to a chip level input pin therefore allowing to synchronize the FTM counter with an off chip clock source Clock Source Selection: Selects one of the three FTM counter clock sources. 00 No clock selected. This in effect disables the FTM counter. 01 System clock --- 10 Fixed frequency clock 11 External clock These two will be useful for the alarm on LS1 platform in late future, the system clock's frequency is too high to get a long delay timer when go to sleep, so the fixed or external clock could be used instead. And how is that relevant to changing the mask from a hardcoded value to a constant? It's still hardcoded at compile time and I don't see how that helps chosing the clock at runtime for a particular use case. #define FTM_SC 0x00 +#define FTM_SC_CLKS_NON0 +#define FTM_SC_CLKS_SYS1 #define FTM_SC_CLK_SHIFT 3 #define FTM_SC_CLK_MASK(0x3 FTM_SC_CLK_SHIFT) #define FTM_SC_CLK(c) ((c) FTM_SC_CLK_SHIFT) @@ -67,7 +69,7 @@ static inline void ftm_counter_enable(void __iomem *base) /* select and enable counter clock source */ val = ftm_readl(base + FTM_SC); val = ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); - val |= priv-ps | FTM_SC_CLK(1); + val |= priv-ps | FTM_SC_CLK(FTM_SC_CLKS_SYS); ftm_writel(val, base + FTM_SC); } @@ -77,7 +79,8 @@ static inline void ftm_counter_disable(void __iomem *base) /* disable counter clock source */ val = ftm_readl(base + FTM_SC); - val = ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); + val = ~(FTM_SC_CLK_MASK); So you remove the FTM_SC_PS_MASK without mentioning it in the changelog. Either this is by mistake or it wants to be documented WHY it is not needed in the first place. This is just prepare for the other new features in the future. And delay this changes maybe better. 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/
[PATCH 5/5] Clocksource: Flextimer: Use Macro for clock source selection.
FTM source clock is selectable: Source clock can be the system clock, the fixed frequency clock, or an external clock. Fixed frequency clock is an additional clock input to allow the selection of an on chip clock source other than the system clock. Selecting external clock connects FTM clock to a chip level input pin therefore allowing to synchronize the FTM counter with an off chip clock source Clock Source Selection: Selects one of the three FTM counter clock sources. 00 No clock selected. This in effect disables the FTM counter. 01 System clock --- 10 Fixed frequency clock 11 External clock These two will be useful for the alarm on LS1 platform in late future, the system clock's frequency is too high to get a long delay timer when go to sleep, so the fixed or external clock could be used instead. Signed-off-by: Xiubo Li Signed-off-by: Jingchang Lu Cc: Wang Dongsheng --- drivers/clocksource/fsl_ftm_timer.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/clocksource/fsl_ftm_timer.c b/drivers/clocksource/fsl_ftm_timer.c index 974890e..3935ba2 100644 --- a/drivers/clocksource/fsl_ftm_timer.c +++ b/drivers/clocksource/fsl_ftm_timer.c @@ -21,6 +21,8 @@ #include #define FTM_SC 0x00 +#define FTM_SC_CLKS_NON0 +#define FTM_SC_CLKS_SYS1 #define FTM_SC_CLK_SHIFT 3 #define FTM_SC_CLK_MASK(0x3 << FTM_SC_CLK_SHIFT) #define FTM_SC_CLK(c) ((c) << FTM_SC_CLK_SHIFT) @@ -67,7 +69,7 @@ static inline void ftm_counter_enable(void __iomem *base) /* select and enable counter clock source */ val = ftm_readl(base + FTM_SC); val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); - val |= priv->ps | FTM_SC_CLK(1); + val |= priv->ps | FTM_SC_CLK(FTM_SC_CLKS_SYS); ftm_writel(val, base + FTM_SC); } @@ -77,7 +79,8 @@ static inline void ftm_counter_disable(void __iomem *base) /* disable counter clock source */ val = ftm_readl(base + FTM_SC); - val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); + val &= ~(FTM_SC_CLK_MASK); + val |= FTM_SC_CLK(FTM_SC_CLKS_NON); ftm_writel(val, base + FTM_SC); } -- 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/
[PATCH 5/5] Clocksource: Flextimer: Use Macro for clock source selection.
FTM source clock is selectable: Source clock can be the system clock, the fixed frequency clock, or an external clock. Fixed frequency clock is an additional clock input to allow the selection of an on chip clock source other than the system clock. Selecting external clock connects FTM clock to a chip level input pin therefore allowing to synchronize the FTM counter with an off chip clock source Clock Source Selection: Selects one of the three FTM counter clock sources. 00 No clock selected. This in effect disables the FTM counter. 01 System clock --- 10 Fixed frequency clock 11 External clock These two will be useful for the alarm on LS1 platform in late future, the system clock's frequency is too high to get a long delay timer when go to sleep, so the fixed or external clock could be used instead. Signed-off-by: Xiubo Li li.xi...@freescale.com Signed-off-by: Jingchang Lu jingchang...@freescale.com Cc: Wang Dongsheng dongsheng.w...@freescale.com --- drivers/clocksource/fsl_ftm_timer.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/clocksource/fsl_ftm_timer.c b/drivers/clocksource/fsl_ftm_timer.c index 974890e..3935ba2 100644 --- a/drivers/clocksource/fsl_ftm_timer.c +++ b/drivers/clocksource/fsl_ftm_timer.c @@ -21,6 +21,8 @@ #include linux/slab.h #define FTM_SC 0x00 +#define FTM_SC_CLKS_NON0 +#define FTM_SC_CLKS_SYS1 #define FTM_SC_CLK_SHIFT 3 #define FTM_SC_CLK_MASK(0x3 FTM_SC_CLK_SHIFT) #define FTM_SC_CLK(c) ((c) FTM_SC_CLK_SHIFT) @@ -67,7 +69,7 @@ static inline void ftm_counter_enable(void __iomem *base) /* select and enable counter clock source */ val = ftm_readl(base + FTM_SC); val = ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); - val |= priv-ps | FTM_SC_CLK(1); + val |= priv-ps | FTM_SC_CLK(FTM_SC_CLKS_SYS); ftm_writel(val, base + FTM_SC); } @@ -77,7 +79,8 @@ static inline void ftm_counter_disable(void __iomem *base) /* disable counter clock source */ val = ftm_readl(base + FTM_SC); - val = ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK); + val = ~(FTM_SC_CLK_MASK); + val |= FTM_SC_CLK(FTM_SC_CLKS_NON); ftm_writel(val, base + FTM_SC); } -- 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/
