* [PATCH] clk: at91: modify PMC peripheral clock to deal with newer register layout
@ 2015-06-10 13:42 Nicolas Ferre
2015-06-10 13:55 ` Boris Brezillon
0 siblings, 1 reply; 5+ messages in thread
From: Nicolas Ferre @ 2015-06-10 13:42 UTC (permalink / raw)
To: linux-arm-kernel
As some more information is added to the PCR register, we'd better use
a copy of its content and modify just the peripheral-related bits.
Implement a read-modify-write for the enable() and disable() callbacks.
Header file is also modified to have the PCR_DIV mask.
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
drivers/clk/at91/clk-peripheral.c | 19 +++++++++++++------
include/linux/clk/at91_pmc.h | 3 ++-
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
index 597fed423d7d..37e2fea14890 100644
--- a/drivers/clk/at91/clk-peripheral.c
+++ b/drivers/clk/at91/clk-peripheral.c
@@ -161,14 +161,17 @@ static int clk_sam9x5_peripheral_enable(struct clk_hw *hw)
{
struct clk_sam9x5_peripheral *periph = to_clk_sam9x5_peripheral(hw);
struct at91_pmc *pmc = periph->pmc;
+ u32 tmp;
if (periph->id < PERIPHERAL_ID_MIN)
return 0;
- pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID) |
- AT91_PMC_PCR_CMD |
- AT91_PMC_PCR_DIV(periph->div) |
- AT91_PMC_PCR_EN);
+ pmc_lock(pmc);
+ pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID));
+ tmp = pmc_read(pmc, AT91_PMC_PCR) & ~AT91_PMC_PCR_P_DIV;
+ pmc_write(pmc, AT91_PMC_PCR, tmp | AT91_PMC_PCR_PDIV(periph->div)
+ | AT91_PMC_PCR_EN);
+ pmc_unlock(pmc);
return 0;
}
@@ -176,12 +179,16 @@ static void clk_sam9x5_peripheral_disable(struct clk_hw *hw)
{
struct clk_sam9x5_peripheral *periph = to_clk_sam9x5_peripheral(hw);
struct at91_pmc *pmc = periph->pmc;
+ u32 tmp;
if (periph->id < PERIPHERAL_ID_MIN)
return;
- pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID) |
- AT91_PMC_PCR_CMD);
+ pmc_lock(pmc);
+ pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID));
+ tmp = pmc_read(pmc, AT91_PMC_PCR) & ~AT91_PMC_PCR_EN;
+ pmc_write(pmc, AT91_PMC_PCR, tmp);
+ pmc_unlock(pmc);
}
static int clk_sam9x5_peripheral_is_enabled(struct clk_hw *hw)
diff --git a/include/linux/clk/at91_pmc.h b/include/linux/clk/at91_pmc.h
index 7669f7618f39..4685c3d62f94 100644
--- a/include/linux/clk/at91_pmc.h
+++ b/include/linux/clk/at91_pmc.h
@@ -184,7 +184,8 @@ extern void __iomem *at91_pmc_base;
#define AT91_PMC_PCR 0x10c /* Peripheral Control Register [some SAM9 and SAMA5] */
#define AT91_PMC_PCR_PID (0x3f << 0) /* Peripheral ID */
#define AT91_PMC_PCR_CMD (0x1 << 12) /* Command (read=0, write=1) */
-#define AT91_PMC_PCR_DIV(n) ((n) << 16) /* Divisor Value */
+#define AT91_PMC_PCR_P_DIV (0x3 << 16) /* Divisor mask */
+#define AT91_PMC_PCR_PDIV(n) ((n) << 16) /* Divisor Value */
#define AT91_PMC_PCR_DIV0 0x0 /* Peripheral clock is MCK */
#define AT91_PMC_PCR_DIV2 0x1 /* Peripheral clock is MCK/2 */
#define AT91_PMC_PCR_DIV4 0x2 /* Peripheral clock is MCK/4 */
--
2.1.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] clk: at91: modify PMC peripheral clock to deal with newer register layout
2015-06-10 13:42 [PATCH] clk: at91: modify PMC peripheral clock to deal with newer register layout Nicolas Ferre
@ 2015-06-10 13:55 ` Boris Brezillon
2015-06-10 14:39 ` Nicolas Ferre
0 siblings, 1 reply; 5+ messages in thread
From: Boris Brezillon @ 2015-06-10 13:55 UTC (permalink / raw)
To: linux-arm-kernel
Hi Nicolas,
On Wed, 10 Jun 2015 15:42:44 +0200
Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> As some more information is added to the PCR register, we'd better use
> a copy of its content and modify just the peripheral-related bits.
> Implement a read-modify-write for the enable() and disable() callbacks.
>
> Header file is also modified to have the PCR_DIV mask.
>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Apart from the below comment you can add my:
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/clk/at91/clk-peripheral.c | 19 +++++++++++++------
> include/linux/clk/at91_pmc.h | 3 ++-
> 2 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
> index 597fed423d7d..37e2fea14890 100644
> --- a/drivers/clk/at91/clk-peripheral.c
> +++ b/drivers/clk/at91/clk-peripheral.c
> @@ -161,14 +161,17 @@ static int clk_sam9x5_peripheral_enable(struct clk_hw *hw)
> {
> struct clk_sam9x5_peripheral *periph = to_clk_sam9x5_peripheral(hw);
> struct at91_pmc *pmc = periph->pmc;
> + u32 tmp;
>
> if (periph->id < PERIPHERAL_ID_MIN)
> return 0;
>
> - pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID) |
> - AT91_PMC_PCR_CMD |
> - AT91_PMC_PCR_DIV(periph->div) |
> - AT91_PMC_PCR_EN);
> + pmc_lock(pmc);
> + pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID));
> + tmp = pmc_read(pmc, AT91_PMC_PCR) & ~AT91_PMC_PCR_P_DIV;
> + pmc_write(pmc, AT91_PMC_PCR, tmp | AT91_PMC_PCR_PDIV(periph->div)
> + | AT91_PMC_PCR_EN);
> + pmc_unlock(pmc);
> return 0;
> }
>
> @@ -176,12 +179,16 @@ static void clk_sam9x5_peripheral_disable(struct clk_hw *hw)
> {
> struct clk_sam9x5_peripheral *periph = to_clk_sam9x5_peripheral(hw);
> struct at91_pmc *pmc = periph->pmc;
> + u32 tmp;
>
> if (periph->id < PERIPHERAL_ID_MIN)
> return;
>
> - pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID) |
> - AT91_PMC_PCR_CMD);
> + pmc_lock(pmc);
> + pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID));
> + tmp = pmc_read(pmc, AT91_PMC_PCR) & ~AT91_PMC_PCR_EN;
> + pmc_write(pmc, AT91_PMC_PCR, tmp);
> + pmc_unlock(pmc);
> }
>
> static int clk_sam9x5_peripheral_is_enabled(struct clk_hw *hw)
> diff --git a/include/linux/clk/at91_pmc.h b/include/linux/clk/at91_pmc.h
> index 7669f7618f39..4685c3d62f94 100644
> --- a/include/linux/clk/at91_pmc.h
> +++ b/include/linux/clk/at91_pmc.h
> @@ -184,7 +184,8 @@ extern void __iomem *at91_pmc_base;
> #define AT91_PMC_PCR 0x10c /* Peripheral Control Register [some SAM9 and SAMA5] */
> #define AT91_PMC_PCR_PID (0x3f << 0) /* Peripheral ID */
> #define AT91_PMC_PCR_CMD (0x1 << 12) /* Command (read=0, write=1) */
> -#define AT91_PMC_PCR_DIV(n) ((n) << 16) /* Divisor Value */
> +#define AT91_PMC_PCR_P_DIV (0x3 << 16) /* Divisor mask */
How about renaming this macro into AT91_PMC_PCR_PDIV_MSK ?
I know the macro names in this file are not consistent, but maybe it's
time to choose appropriate names for new AT91_PMC macros.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] clk: at91: modify PMC peripheral clock to deal with newer register layout
2015-06-10 13:55 ` Boris Brezillon
@ 2015-06-10 14:39 ` Nicolas Ferre
2015-06-10 14:55 ` Boris Brezillon
0 siblings, 1 reply; 5+ messages in thread
From: Nicolas Ferre @ 2015-06-10 14:39 UTC (permalink / raw)
To: linux-arm-kernel
Le 10/06/2015 15:55, Boris Brezillon a ?crit :
> Hi Nicolas,
>
> On Wed, 10 Jun 2015 15:42:44 +0200
> Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>
>> As some more information is added to the PCR register, we'd better use
>> a copy of its content and modify just the peripheral-related bits.
>> Implement a read-modify-write for the enable() and disable() callbacks.
>>
>> Header file is also modified to have the PCR_DIV mask.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>
> Apart from the below comment you can add my:
>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>
>> ---
>> drivers/clk/at91/clk-peripheral.c | 19 +++++++++++++------
>> include/linux/clk/at91_pmc.h | 3 ++-
>> 2 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
>> index 597fed423d7d..37e2fea14890 100644
>> --- a/drivers/clk/at91/clk-peripheral.c
>> +++ b/drivers/clk/at91/clk-peripheral.c
>> @@ -161,14 +161,17 @@ static int clk_sam9x5_peripheral_enable(struct clk_hw *hw)
>> {
>> struct clk_sam9x5_peripheral *periph = to_clk_sam9x5_peripheral(hw);
>> struct at91_pmc *pmc = periph->pmc;
>> + u32 tmp;
>>
>> if (periph->id < PERIPHERAL_ID_MIN)
>> return 0;
>>
>> - pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID) |
>> - AT91_PMC_PCR_CMD |
>> - AT91_PMC_PCR_DIV(periph->div) |
>> - AT91_PMC_PCR_EN);
>> + pmc_lock(pmc);
>> + pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID));
>> + tmp = pmc_read(pmc, AT91_PMC_PCR) & ~AT91_PMC_PCR_P_DIV;
>> + pmc_write(pmc, AT91_PMC_PCR, tmp | AT91_PMC_PCR_PDIV(periph->div)
>> + | AT91_PMC_PCR_EN);
>> + pmc_unlock(pmc);
>> return 0;
>> }
>>
>> @@ -176,12 +179,16 @@ static void clk_sam9x5_peripheral_disable(struct clk_hw *hw)
>> {
>> struct clk_sam9x5_peripheral *periph = to_clk_sam9x5_peripheral(hw);
>> struct at91_pmc *pmc = periph->pmc;
>> + u32 tmp;
>>
>> if (periph->id < PERIPHERAL_ID_MIN)
>> return;
>>
>> - pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID) |
>> - AT91_PMC_PCR_CMD);
>> + pmc_lock(pmc);
>> + pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID));
>> + tmp = pmc_read(pmc, AT91_PMC_PCR) & ~AT91_PMC_PCR_EN;
>> + pmc_write(pmc, AT91_PMC_PCR, tmp);
>> + pmc_unlock(pmc);
>> }
>>
>> static int clk_sam9x5_peripheral_is_enabled(struct clk_hw *hw)
>> diff --git a/include/linux/clk/at91_pmc.h b/include/linux/clk/at91_pmc.h
>> index 7669f7618f39..4685c3d62f94 100644
>> --- a/include/linux/clk/at91_pmc.h
>> +++ b/include/linux/clk/at91_pmc.h
>> @@ -184,7 +184,8 @@ extern void __iomem *at91_pmc_base;
>> #define AT91_PMC_PCR 0x10c /* Peripheral Control Register [some SAM9 and SAMA5] */
>> #define AT91_PMC_PCR_PID (0x3f << 0) /* Peripheral ID */
>> #define AT91_PMC_PCR_CMD (0x1 << 12) /* Command (read=0, write=1) */
>> -#define AT91_PMC_PCR_DIV(n) ((n) << 16) /* Divisor Value */
>> +#define AT91_PMC_PCR_P_DIV (0x3 << 16) /* Divisor mask */
>
> How about renaming this macro into AT91_PMC_PCR_PDIV_MSK ?
> I know the macro names in this file are not consistent, but maybe it's
> time to choose appropriate names for new AT91_PMC macros.
Well, this is what I tried to find: consistency ;-)
It seems that other macros are like I did for this one: the pure name of
the field for the mask and some kind of other form of the name for a
value macro or a (usually useless) list of macro-per-value things.
For this one I added a "P" for peripheral which is not in the real name
of the register field. This is to differentiate it from the upcoming
GCK_DIV field...
Bye,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] clk: at91: modify PMC peripheral clock to deal with newer register layout
2015-06-10 14:39 ` Nicolas Ferre
@ 2015-06-10 14:55 ` Boris Brezillon
2015-06-10 15:33 ` Nicolas Ferre
0 siblings, 1 reply; 5+ messages in thread
From: Boris Brezillon @ 2015-06-10 14:55 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 10 Jun 2015 16:39:40 +0200
Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> Le 10/06/2015 15:55, Boris Brezillon a ?crit :
> > Hi Nicolas,
> >
> > On Wed, 10 Jun 2015 15:42:44 +0200
> > Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> >
> >> As some more information is added to the PCR register, we'd better use
> >> a copy of its content and modify just the peripheral-related bits.
> >> Implement a read-modify-write for the enable() and disable() callbacks.
> >>
> >> Header file is also modified to have the PCR_DIV mask.
> >>
> >> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> >
> > Apart from the below comment you can add my:
> >
> > Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> >
> >> ---
> >> drivers/clk/at91/clk-peripheral.c | 19 +++++++++++++------
> >> include/linux/clk/at91_pmc.h | 3 ++-
> >> 2 files changed, 15 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
> >> index 597fed423d7d..37e2fea14890 100644
> >> --- a/drivers/clk/at91/clk-peripheral.c
> >> +++ b/drivers/clk/at91/clk-peripheral.c
> >> @@ -161,14 +161,17 @@ static int clk_sam9x5_peripheral_enable(struct clk_hw *hw)
> >> {
> >> struct clk_sam9x5_peripheral *periph = to_clk_sam9x5_peripheral(hw);
> >> struct at91_pmc *pmc = periph->pmc;
> >> + u32 tmp;
> >>
> >> if (periph->id < PERIPHERAL_ID_MIN)
> >> return 0;
> >>
> >> - pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID) |
> >> - AT91_PMC_PCR_CMD |
> >> - AT91_PMC_PCR_DIV(periph->div) |
> >> - AT91_PMC_PCR_EN);
> >> + pmc_lock(pmc);
> >> + pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID));
> >> + tmp = pmc_read(pmc, AT91_PMC_PCR) & ~AT91_PMC_PCR_P_DIV;
> >> + pmc_write(pmc, AT91_PMC_PCR, tmp | AT91_PMC_PCR_PDIV(periph->div)
> >> + | AT91_PMC_PCR_EN);
> >> + pmc_unlock(pmc);
> >> return 0;
> >> }
> >>
> >> @@ -176,12 +179,16 @@ static void clk_sam9x5_peripheral_disable(struct clk_hw *hw)
> >> {
> >> struct clk_sam9x5_peripheral *periph = to_clk_sam9x5_peripheral(hw);
> >> struct at91_pmc *pmc = periph->pmc;
> >> + u32 tmp;
> >>
> >> if (periph->id < PERIPHERAL_ID_MIN)
> >> return;
> >>
> >> - pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID) |
> >> - AT91_PMC_PCR_CMD);
> >> + pmc_lock(pmc);
> >> + pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID));
> >> + tmp = pmc_read(pmc, AT91_PMC_PCR) & ~AT91_PMC_PCR_EN;
> >> + pmc_write(pmc, AT91_PMC_PCR, tmp);
> >> + pmc_unlock(pmc);
> >> }
> >>
> >> static int clk_sam9x5_peripheral_is_enabled(struct clk_hw *hw)
> >> diff --git a/include/linux/clk/at91_pmc.h b/include/linux/clk/at91_pmc.h
> >> index 7669f7618f39..4685c3d62f94 100644
> >> --- a/include/linux/clk/at91_pmc.h
> >> +++ b/include/linux/clk/at91_pmc.h
> >> @@ -184,7 +184,8 @@ extern void __iomem *at91_pmc_base;
> >> #define AT91_PMC_PCR 0x10c /* Peripheral Control Register [some SAM9 and SAMA5] */
> >> #define AT91_PMC_PCR_PID (0x3f << 0) /* Peripheral ID */
> >> #define AT91_PMC_PCR_CMD (0x1 << 12) /* Command (read=0, write=1) */
> >> -#define AT91_PMC_PCR_DIV(n) ((n) << 16) /* Divisor Value */
> >> +#define AT91_PMC_PCR_P_DIV (0x3 << 16) /* Divisor mask */
> >
> > How about renaming this macro into AT91_PMC_PCR_PDIV_MSK ?
> > I know the macro names in this file are not consistent, but maybe it's
> > time to choose appropriate names for new AT91_PMC macros.
>
> Well, this is what I tried to find: consistency ;-)
> It seems that other macros are like I did for this one: the pure name of
> the field for the mask and some kind of other form of the name for a
> value macro or a (usually useless) list of macro-per-value things.
>
> For this one I added a "P" for peripheral which is not in the real name
> of the register field. This is to differentiate it from the upcoming
> GCK_DIV field...
Yes, but that doesn't help in describing what the macros are really
representing. My point is that, yes moving to _MSK doesn't add any
consistency, but there already is no consistency in the existing names,
so, IMHO, we should at least choose representative names.
I'm not arguing against the addition of a P before the DIV word, but
why is P_DIV chosen to reprensent the mask and PDIV used to define the
macro building the value to be stored in the PCR register ?
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] clk: at91: modify PMC peripheral clock to deal with newer register layout
2015-06-10 14:55 ` Boris Brezillon
@ 2015-06-10 15:33 ` Nicolas Ferre
0 siblings, 0 replies; 5+ messages in thread
From: Nicolas Ferre @ 2015-06-10 15:33 UTC (permalink / raw)
To: linux-arm-kernel
Le 10/06/2015 16:55, Boris Brezillon a ?crit :
> On Wed, 10 Jun 2015 16:39:40 +0200
> Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>
>> Le 10/06/2015 15:55, Boris Brezillon a ?crit :
>>> Hi Nicolas,
>>>
>>> On Wed, 10 Jun 2015 15:42:44 +0200
>>> Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>>>
>>>> As some more information is added to the PCR register, we'd better use
>>>> a copy of its content and modify just the peripheral-related bits.
>>>> Implement a read-modify-write for the enable() and disable() callbacks.
>>>>
>>>> Header file is also modified to have the PCR_DIV mask.
>>>>
>>>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>>
>>> Apart from the below comment you can add my:
>>>
>>> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>>>
>>>> ---
>>>> drivers/clk/at91/clk-peripheral.c | 19 +++++++++++++------
>>>> include/linux/clk/at91_pmc.h | 3 ++-
>>>> 2 files changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
>>>> index 597fed423d7d..37e2fea14890 100644
>>>> --- a/drivers/clk/at91/clk-peripheral.c
>>>> +++ b/drivers/clk/at91/clk-peripheral.c
>>>> @@ -161,14 +161,17 @@ static int clk_sam9x5_peripheral_enable(struct clk_hw *hw)
>>>> {
>>>> struct clk_sam9x5_peripheral *periph = to_clk_sam9x5_peripheral(hw);
>>>> struct at91_pmc *pmc = periph->pmc;
>>>> + u32 tmp;
>>>>
>>>> if (periph->id < PERIPHERAL_ID_MIN)
>>>> return 0;
>>>>
>>>> - pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID) |
>>>> - AT91_PMC_PCR_CMD |
>>>> - AT91_PMC_PCR_DIV(periph->div) |
>>>> - AT91_PMC_PCR_EN);
>>>> + pmc_lock(pmc);
>>>> + pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID));
>>>> + tmp = pmc_read(pmc, AT91_PMC_PCR) & ~AT91_PMC_PCR_P_DIV;
>>>> + pmc_write(pmc, AT91_PMC_PCR, tmp | AT91_PMC_PCR_PDIV(periph->div)
>>>> + | AT91_PMC_PCR_EN);
>>>> + pmc_unlock(pmc);
>>>> return 0;
>>>> }
>>>>
>>>> @@ -176,12 +179,16 @@ static void clk_sam9x5_peripheral_disable(struct clk_hw *hw)
>>>> {
>>>> struct clk_sam9x5_peripheral *periph = to_clk_sam9x5_peripheral(hw);
>>>> struct at91_pmc *pmc = periph->pmc;
>>>> + u32 tmp;
>>>>
>>>> if (periph->id < PERIPHERAL_ID_MIN)
>>>> return;
>>>>
>>>> - pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID) |
>>>> - AT91_PMC_PCR_CMD);
>>>> + pmc_lock(pmc);
>>>> + pmc_write(pmc, AT91_PMC_PCR, (periph->id & AT91_PMC_PCR_PID));
>>>> + tmp = pmc_read(pmc, AT91_PMC_PCR) & ~AT91_PMC_PCR_EN;
>>>> + pmc_write(pmc, AT91_PMC_PCR, tmp);
>>>> + pmc_unlock(pmc);
>>>> }
>>>>
>>>> static int clk_sam9x5_peripheral_is_enabled(struct clk_hw *hw)
>>>> diff --git a/include/linux/clk/at91_pmc.h b/include/linux/clk/at91_pmc.h
>>>> index 7669f7618f39..4685c3d62f94 100644
>>>> --- a/include/linux/clk/at91_pmc.h
>>>> +++ b/include/linux/clk/at91_pmc.h
>>>> @@ -184,7 +184,8 @@ extern void __iomem *at91_pmc_base;
>>>> #define AT91_PMC_PCR 0x10c /* Peripheral Control Register [some SAM9 and SAMA5] */
>>>> #define AT91_PMC_PCR_PID (0x3f << 0) /* Peripheral ID */
>>>> #define AT91_PMC_PCR_CMD (0x1 << 12) /* Command (read=0, write=1) */
>>>> -#define AT91_PMC_PCR_DIV(n) ((n) << 16) /* Divisor Value */
>>>> +#define AT91_PMC_PCR_P_DIV (0x3 << 16) /* Divisor mask */
>>>
>>> How about renaming this macro into AT91_PMC_PCR_PDIV_MSK ?
>>> I know the macro names in this file are not consistent, but maybe it's
>>> time to choose appropriate names for new AT91_PMC macros.
>>
>> Well, this is what I tried to find: consistency ;-)
>> It seems that other macros are like I did for this one: the pure name of
>> the field for the mask and some kind of other form of the name for a
>> value macro or a (usually useless) list of macro-per-value things.
>>
>> For this one I added a "P" for peripheral which is not in the real name
>> of the register field. This is to differentiate it from the upcoming
>> GCK_DIV field...
>
> Yes, but that doesn't help in describing what the macros are really
> representing. My point is that, yes moving to _MSK doesn't add any
> consistency, but there already is no consistency in the existing names,
> so, IMHO, we should at least choose representative names.
> I'm not arguing against the addition of a P before the DIV word, but
> why is P_DIV chosen to reprensent the mask and PDIV used to define the
> macro building the value to be stored in the PCR register ?
Ok, I'll try another attempt with cleaner field descriptions for this
register.
Bye,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-06-10 15:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-10 13:42 [PATCH] clk: at91: modify PMC peripheral clock to deal with newer register layout Nicolas Ferre
2015-06-10 13:55 ` Boris Brezillon
2015-06-10 14:39 ` Nicolas Ferre
2015-06-10 14:55 ` Boris Brezillon
2015-06-10 15:33 ` Nicolas Ferre
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox