From: Nicolas Ferre <nicolas.ferre@atmel.com>
To: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: <linux-arm-kernel@lists.infradead.org>,
Alexandre Belloni <alexandre.belloni@free-electrons.com>,
Ludovic Desroches <ludovic.desroches@atmel.com>,
Josh Wu <josh.wu@atmel.com>, <linux-kernel@vger.kernel.org>,
<mturquette@baylibre.com>
Subject: Re: [PATCH] clk: at91: add generated clock driver
Date: Mon, 22 Jun 2015 18:50:49 +0200 [thread overview]
Message-ID: <55883CE9.2060904@atmel.com> (raw)
In-Reply-To: <20150618172534.57b09c0b@bbrezillon>
Le 18/06/2015 17:25, Boris Brezillon a écrit :
> Hi Nicolas,
Thanks for your review Boris. I'll address your remarks by sending
another revision of this series in several days...
Bye,
> I haven't run checkpatch on your patch, but there seems to be a few
> things to fix ;-).
>
> On Wed, 17 Jun 2015 15:23:29 +0200
> Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>
>
>
>
>> +
>> +static long clk_generated_determine_rate(struct clk_hw *hw,
>> + unsigned long rate,
>> + unsigned long *best_parent_rate,
>> + struct clk_hw **best_parent_hw)
>
> The ->determine_rate() prototype has changed (see [1]).
>
>
>> +/* No modification of hardware as we have the flag CLK_SET_RATE_GATE set */
>> +static int clk_generated_set_rate(struct clk_hw *hw,
>> + unsigned long rate,
>> + unsigned long parent_rate)
>> +{
>> + struct clk_generated *gck = to_clk_generated(hw);
>> + u32 div;
>> +
>> + if (!rate)
>> + return -EINVAL;
>> +
>> + if (gck->id < PERIPHERAL_ID_MIN || !gck->range.max) {
>> + if (parent_rate == rate) {
>> + gck->gckdiv = 0;
>> + return 0;
>> + } else {
>> + return -EINVAL;
>> + }
>> + }
>
> Do you really need the above check ?
> AFAICT, periph ids inferior to 2 are invalid ids, so they should be
> detected at probe time.
> The !gck->range.max will be caught by the rate > gck->range.max test.
> And finally, the case parent_rate == rate will just give you a div of 1,
> which in turns give a gckdiv of 0.
>
>> +
>> + if (rate > gck->range.max)
>> + return -EINVAL;
>> +
>> + div = DIV_ROUND_CLOSEST(parent_rate, rate);
>> + if (div > GENERATED_MAX_DIV + 1 || !div)
>> + return -EINVAL;
>> +
>> + gck->gckdiv = div - 1;
>> + return 0;
>> +}
>> +
>> +static const struct clk_ops generated_ops = {
>> + .enable = clk_generated_enable,
>> + .disable = clk_generated_disable,
>> + .is_enabled = clk_generated_is_enabled,
>> + .recalc_rate = clk_generated_recalc_rate,
>> + .determine_rate = clk_generated_determine_rate,
>> + .get_parent = clk_generated_get_parent,
>> + .set_parent = clk_generated_set_parent,
>> + .set_rate = clk_generated_set_rate,
>> +};
>> +
>> +/**
>> + * clk_generated_startup - Initialize a given clock to its default parent and
>> + * divisor parameter.
>> + *
>> + * @gck: Generated clock to set the startup parameters for.
>> + *
>> + * Take parameters from the hardware and update local clock configuration
>> + * accordingly.
>> + */
>> +static void clk_generated_startup(struct clk_generated *gck)
>> +{
>> + struct at91_pmc *pmc = gck->pmc;
>> + u32 tmp;
>> +
>> + pmc_lock(pmc);
>> + pmc_write(pmc, AT91_PMC_PCR, (gck->id & AT91_PMC_PCR_PID_MASK));
>> + tmp = pmc_read(pmc, AT91_PMC_PCR);
>> + pmc_unlock(pmc);
>> +
>> + gck->parent_id = (tmp & AT91_PMC_PCR_GCKCSS_MASK) >> AT91_PMC_PCR_GCKCSS_OFFSET;
>> + gck->gckdiv = (tmp & AT91_PMC_PCR_GCKDIV_MASK) >> AT91_PMC_PCR_GCKDIV_OFFSET;
>> +}
>> +
>> +static struct clk * __init
>> +at91_clk_register_generated(struct at91_pmc *pmc, const char *name,
>> + const char **parent_names, u8 num_parents,
>> + u8 id, const struct clk_range *range)
>> +{
>> + struct clk_generated *gck;
>> + struct clk *clk = NULL;
>> + struct clk_init_data init;
>> +
>> + gck = kzalloc(sizeof(*gck), GFP_KERNEL);
>> + if (!gck)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + init.name = name;
>> + init.ops = &generated_ops;
>> + init.parent_names = parent_names;
>> + init.num_parents = num_parents;
>> + init.flags = CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE;
>> +
>> + gck->id = id;
>> + gck->hw.init = &init;
>> + gck->pmc = pmc;
>> + gck->range = *range;
>> +
>> + clk = clk_register(NULL, &gck->hw);
>> + if (IS_ERR(clk))
>> + kfree(gck);
>> + else
>> + clk_generated_startup(gck);
>> +
>> + return clk;
>> +}
>> +
>> +void __init of_sama5d2_clk_generated_setup(struct device_node *np,
>> + struct at91_pmc *pmc)
>> +{
>> + int i;
>> + int num;
>> + u32 id;
>> + const char *name;
>> + struct clk *clk;
>> + int num_parents;
>> + const char *parent_names[GENERATED_SOURCE_MAX];
>> + struct device_node *gcknp;
>> + struct clk_range range = CLK_RANGE(0, 0);
>> +
>> + num_parents = of_count_phandle_with_args(np, "clocks", "#clock-cells");
>
> Use the of_clk_get_parent_count() [2].
>
> That's all I got for now.
>
> Thanks,
>
> Boris
>
>
> [1]http://lxr.free-electrons.com/source/include/linux/clk-provider.h#L178
> [2]http://lxr.free-electrons.com/source/include/linux/clk-provider.h#L626
>
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at http://www.tux.org/lkml/
WARNING: multiple messages have this Message-ID (diff)
From: nicolas.ferre@atmel.com (Nicolas Ferre)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] clk: at91: add generated clock driver
Date: Mon, 22 Jun 2015 18:50:49 +0200 [thread overview]
Message-ID: <55883CE9.2060904@atmel.com> (raw)
In-Reply-To: <20150618172534.57b09c0b@bbrezillon>
Le 18/06/2015 17:25, Boris Brezillon a ?crit :
> Hi Nicolas,
Thanks for your review Boris. I'll address your remarks by sending
another revision of this series in several days...
Bye,
> I haven't run checkpatch on your patch, but there seems to be a few
> things to fix ;-).
>
> On Wed, 17 Jun 2015 15:23:29 +0200
> Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>
>
>
>
>> +
>> +static long clk_generated_determine_rate(struct clk_hw *hw,
>> + unsigned long rate,
>> + unsigned long *best_parent_rate,
>> + struct clk_hw **best_parent_hw)
>
> The ->determine_rate() prototype has changed (see [1]).
>
>
>> +/* No modification of hardware as we have the flag CLK_SET_RATE_GATE set */
>> +static int clk_generated_set_rate(struct clk_hw *hw,
>> + unsigned long rate,
>> + unsigned long parent_rate)
>> +{
>> + struct clk_generated *gck = to_clk_generated(hw);
>> + u32 div;
>> +
>> + if (!rate)
>> + return -EINVAL;
>> +
>> + if (gck->id < PERIPHERAL_ID_MIN || !gck->range.max) {
>> + if (parent_rate == rate) {
>> + gck->gckdiv = 0;
>> + return 0;
>> + } else {
>> + return -EINVAL;
>> + }
>> + }
>
> Do you really need the above check ?
> AFAICT, periph ids inferior to 2 are invalid ids, so they should be
> detected at probe time.
> The !gck->range.max will be caught by the rate > gck->range.max test.
> And finally, the case parent_rate == rate will just give you a div of 1,
> which in turns give a gckdiv of 0.
>
>> +
>> + if (rate > gck->range.max)
>> + return -EINVAL;
>> +
>> + div = DIV_ROUND_CLOSEST(parent_rate, rate);
>> + if (div > GENERATED_MAX_DIV + 1 || !div)
>> + return -EINVAL;
>> +
>> + gck->gckdiv = div - 1;
>> + return 0;
>> +}
>> +
>> +static const struct clk_ops generated_ops = {
>> + .enable = clk_generated_enable,
>> + .disable = clk_generated_disable,
>> + .is_enabled = clk_generated_is_enabled,
>> + .recalc_rate = clk_generated_recalc_rate,
>> + .determine_rate = clk_generated_determine_rate,
>> + .get_parent = clk_generated_get_parent,
>> + .set_parent = clk_generated_set_parent,
>> + .set_rate = clk_generated_set_rate,
>> +};
>> +
>> +/**
>> + * clk_generated_startup - Initialize a given clock to its default parent and
>> + * divisor parameter.
>> + *
>> + * @gck: Generated clock to set the startup parameters for.
>> + *
>> + * Take parameters from the hardware and update local clock configuration
>> + * accordingly.
>> + */
>> +static void clk_generated_startup(struct clk_generated *gck)
>> +{
>> + struct at91_pmc *pmc = gck->pmc;
>> + u32 tmp;
>> +
>> + pmc_lock(pmc);
>> + pmc_write(pmc, AT91_PMC_PCR, (gck->id & AT91_PMC_PCR_PID_MASK));
>> + tmp = pmc_read(pmc, AT91_PMC_PCR);
>> + pmc_unlock(pmc);
>> +
>> + gck->parent_id = (tmp & AT91_PMC_PCR_GCKCSS_MASK) >> AT91_PMC_PCR_GCKCSS_OFFSET;
>> + gck->gckdiv = (tmp & AT91_PMC_PCR_GCKDIV_MASK) >> AT91_PMC_PCR_GCKDIV_OFFSET;
>> +}
>> +
>> +static struct clk * __init
>> +at91_clk_register_generated(struct at91_pmc *pmc, const char *name,
>> + const char **parent_names, u8 num_parents,
>> + u8 id, const struct clk_range *range)
>> +{
>> + struct clk_generated *gck;
>> + struct clk *clk = NULL;
>> + struct clk_init_data init;
>> +
>> + gck = kzalloc(sizeof(*gck), GFP_KERNEL);
>> + if (!gck)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + init.name = name;
>> + init.ops = &generated_ops;
>> + init.parent_names = parent_names;
>> + init.num_parents = num_parents;
>> + init.flags = CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE;
>> +
>> + gck->id = id;
>> + gck->hw.init = &init;
>> + gck->pmc = pmc;
>> + gck->range = *range;
>> +
>> + clk = clk_register(NULL, &gck->hw);
>> + if (IS_ERR(clk))
>> + kfree(gck);
>> + else
>> + clk_generated_startup(gck);
>> +
>> + return clk;
>> +}
>> +
>> +void __init of_sama5d2_clk_generated_setup(struct device_node *np,
>> + struct at91_pmc *pmc)
>> +{
>> + int i;
>> + int num;
>> + u32 id;
>> + const char *name;
>> + struct clk *clk;
>> + int num_parents;
>> + const char *parent_names[GENERATED_SOURCE_MAX];
>> + struct device_node *gcknp;
>> + struct clk_range range = CLK_RANGE(0, 0);
>> +
>> + num_parents = of_count_phandle_with_args(np, "clocks", "#clock-cells");
>
> Use the of_clk_get_parent_count() [2].
>
> That's all I got for now.
>
> Thanks,
>
> Boris
>
>
> [1]http://lxr.free-electrons.com/source/include/linux/clk-provider.h#L178
> [2]http://lxr.free-electrons.com/source/include/linux/clk-provider.h#L626
>
--
Nicolas Ferre
next prev parent reply other threads:[~2015-06-22 16:52 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-17 13:23 [PATCH] clk: at91: add generated clock driver Nicolas Ferre
2015-06-17 13:23 ` Nicolas Ferre
2015-06-18 7:12 ` Paul Bolle
2015-06-18 7:12 ` Paul Bolle
2015-06-18 7:33 ` Boris Brezillon
2015-06-18 7:33 ` Boris Brezillon
2015-06-18 7:40 ` Nicolas Ferre
2015-06-18 7:40 ` Nicolas Ferre
2015-06-18 7:54 ` Paul Bolle
2015-06-18 7:54 ` Paul Bolle
2015-06-18 12:40 ` Nicolas Ferre
2015-06-18 12:40 ` Nicolas Ferre
2015-06-18 14:46 ` Paul Bolle
2015-06-18 14:46 ` Paul Bolle
2015-06-18 12:59 ` Alexandre Belloni
2015-06-18 12:59 ` Alexandre Belloni
2015-06-18 13:28 ` Nicolas Ferre
2015-06-18 13:28 ` Nicolas Ferre
2015-06-18 15:11 ` Paul Bolle
2015-06-18 15:11 ` Paul Bolle
2015-06-18 7:44 ` Paul Bolle
2015-06-18 7:44 ` Paul Bolle
2015-06-18 15:25 ` Boris Brezillon
2015-06-18 15:25 ` Boris Brezillon
2015-06-22 16:50 ` Nicolas Ferre [this message]
2015-06-22 16:50 ` Nicolas Ferre
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55883CE9.2060904@atmel.com \
--to=nicolas.ferre@atmel.com \
--cc=alexandre.belloni@free-electrons.com \
--cc=boris.brezillon@free-electrons.com \
--cc=josh.wu@atmel.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ludovic.desroches@atmel.com \
--cc=mturquette@baylibre.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.