LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Michel Pollet <michel.pollet@bp.renesas.com>
Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	Simon Horman <horms@verge.net.au>,
	Phil Edworthy <phil.edworthy@renesas.com>,
	Michel Pollet <buserror+upstream@gmail.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	linux-clk <linux-clk@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v7 5/5] clk: renesas: Renesas R9A06G032 clock driver
Date: Wed, 30 May 2018 21:49:25 +0200	[thread overview]
Message-ID: <CAMuHMdW9fSKZv2qA2oz9NCL+MdASAMnxf1UZomJv3_Zts44ygw@mail.gmail.com> (raw)
In-Reply-To: <1527154169-32380-6-git-send-email-michel.pollet@bp.renesas.com>

Hi Michel,

On Thu, May 24, 2018 at 11:28 AM, Michel Pollet
<michel.pollet@bp.renesas.com> wrote:
> This provides a clock driver for the Renesas R09A06G032.
> This uses a structure derived from both the RCAR gen2 driver as well as
> the renesas-cpg-mssr driver.
>
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/clk/renesas/r9a06g032-clocks.c

> +/* register/bit pairs are encoded as an uint16_t */
> +static void clk_rdesc_set(
> +       struct r9a06g032_priv *clocks,
> +       uint16_t one, unsigned int on)
> +{
> +       u32 __iomem *reg = ((u32 __iomem *)clocks->reg) + (one >> 5);

Do you need the cast? Gcc does support void pointer arithmetic, and treats
that like byte pointers.

> +       u32 val = clk_readl(reg);
> +
> +       val = (val & ~(1U << (one & 0x1f))) | ((!!on) << (one & 0x1f));
> +       clk_writel(val, reg);

Hence

        clk_writel(val, clocks->reg + 4 * (one >> 5));

Actually clk_{read,write}l() are deprecated, please use {read,write}l() instead.

> +static void r9a06g032_clk_gate_disable(struct clk_hw *hw)
> +{
> +       struct r9a06g032_clk_gate *g = to_r9a06g032_gate(hw);
> +
> +       if (!g->read_only)
> +               r9a06g032_clk_gate_set(g->clocks, &g->gate, 0);
> +       else
> +               pr_debug("%s %s: disallowed\n", __func__,
> +                       __clk_get_name(hw->clk));

You can print the name of a clock using %pC:

    pr_debug("%s %pC: disallowed\n", __func__, hw->clk);

But I don't think you need the check, cfr. below.

> +static struct clk *r9a06g032_register_gate(
> +       struct r9a06g032_priv *clocks,
> +       const char *parent_name,
> +       const struct r9a06g032_clkdesc *desc)
> +{
> +       struct clk *clk;
> +       struct r9a06g032_clk_gate *g;
> +       struct clk_init_data init;
> +
> +       g = kzalloc(sizeof(struct r9a06g032_clk_gate), GFP_KERNEL);
> +       if (!g)
> +               return NULL;
> +
> +       init.name = desc->name;
> +       init.ops = &r9a06g032_clk_gate_ops;
> +       init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
> +       init.parent_names = parent_name ? &parent_name : NULL;
> +       init.num_parents = parent_name ? 1 : 0;
> +
> +       g->clocks = clocks;
> +       g->index = desc->index;
> +       g->gate = desc->gate;
> +       g->hw.init = &init;
> +       g->read_only = 0;
> +
> +       clk = clk_register(NULL, &g->hw);
> +       if (IS_ERR(clk)) {
> +               kfree(g);
> +               return NULL;
> +       }
> +       /*
> +        * important here, some clocks are already in use by the CM3, we
> +        * have to assume they are not Linux's to play with and try to disable
> +        * at the end of the boot!
> +        * Therefore we increase the clock usage count by arbitrarily enabling
> +        * the clock, allowing it to stay untouched at the end of the boot.
> +        */
> +       g->read_only = r9a06g032_clk_gate_is_enabled(&g->hw);

Is checking if the clock is enabled the recommended way to find out if a
clock is used by the Cortex M3? No need for a table?

> +       if (g->read_only)
> +               pr_debug("%s was enabled, making read-only\n", desc->name);

You can set init.flags |= CLK_IS_CRITICAL instead of using your own flag.

> +static unsigned long r9a06g032_divider_recalc_rate(
> +       struct clk_hw *hw,
> +       unsigned long parent_rate)
> +{
> +       struct r9a06g032_clk_div *clk = to_r9a06g032_divider(hw);
> +       u32 *reg = ((u32 *)clk->clocks->reg) + clk->reg;

Fishy operations on __iomem pointers

> +       long div = clk_readl(reg);

u32 div?

> +
> +       if (div < clk->min)
> +               div = clk->min;
> +       else if (div > clk->max)
> +               div = clk->max;
> +       return DIV_ROUND_UP(parent_rate, div);
> +}
> +
> +/*
> + * Attempts to find a value that is in range of min,max,
> + * and if a table of set dividers was specified for this
> + * register, try to find the fixed divider that is the closest
> + * to the target frequency
> + */
> +static long r9a06g032_divider_clamp_div(
> +       struct r9a06g032_clk_div *clk,
> +       unsigned long rate, unsigned long prate)
> +{
> +       /* + 1 to cope with rates that have the remainder dropped */
> +       long div = DIV_ROUND_UP(prate, rate + 1);
> +       int i;

unsigned int i

> +
> +       if (div <= clk->min)
> +               return clk->min;
> +       if (div >= clk->max)
> +               return clk->max;
> +
> +       for (i = 0; clk->table_size && i < clk->table_size - 1; i++) {

> +static long r9a06g032_divider_round_rate(
> +       struct clk_hw *hw, unsigned long rate,
> +       unsigned long *prate)
> +{
> +       struct r9a06g032_clk_div *clk = to_r9a06g032_divider(hw);
> +       long div = DIV_ROUND_UP(*prate, rate);
> +
> +       pr_devel("%s %pC %ld (prate %ld) (wanted div %ld)\n", __func__,

Ah, you do know about %pC ;-)

> +               hw->clk, rate, *prate, div);
> +       pr_devel("   min %d (%ld) max %d (%ld)\n",
> +               clk->min, DIV_ROUND_UP(*prate, clk->min),
> +               clk->max, DIV_ROUND_UP(*prate, clk->max));
> +
> +       div = r9a06g032_divider_clamp_div(clk, rate, *prate);
> +       /*
> +        * this is a hack. Currently the serial driver asks for a clock rate
> +        * that is 16 times the baud rate -- and that is wildly outside the
> +        * range of the UART divider, somehow there is no provision for that
> +        * case of 'let the divider as is if outside range'.
> +        * The serial driver *shouldn't* play with these clocks anyway, there's
> +        * several uarts attached to this divider, and changing this impacts
> +        * everyone.

Huh?

> +        */
> +       if (clk->index == R9A06G032_DIV_UART) {
> +               pr_devel("%s div uart hack!\n", __func__);
> +               return clk_get_rate(hw->clk);
> +       }
> +       pr_devel("%s %pC %ld / %ld = %ld\n", __func__, hw->clk,
> +               *prate, div, DIV_ROUND_UP(*prate, div));
> +       return DIV_ROUND_UP(*prate, div);
> +}
> +
> +static int r9a06g032_divider_set_rate(
> +       struct clk_hw *hw, unsigned long rate,
> +       unsigned long parent_rate)
> +{
> +       struct r9a06g032_clk_div *clk = to_r9a06g032_divider(hw);
> +       /* + 1 to cope with rates that have the remainder dropped */
> +       u32 div = DIV_ROUND_UP(parent_rate, rate + 1);
> +       u32 *reg = ((u32 *)clk->clocks->reg) + clk->reg;
> +
> +       pr_devel("%s %pC rate %ld parent %ld div %d\n", __func__, hw->clk,
> +               rate, parent_rate, div);
> +
> +       /*
> +        * Need to write the bit 31 with the divider value to
> +        * latch it. Technically we should wait until it has been
> +        * cleared too.
> +        * TODO: Find whether this callback is sleepable, in case
> +        * the hardware /does/ require some sort of spinloop here.
> +        */
> +       clk_writel(div | (1U << 31), reg);

BIT(31)?

> +
> +       return 0;
> +}

> +static struct clk *r9a06g032_register_divider(
> +       struct r9a06g032_priv *clocks,
> +       const char *parent_name,
> +       const struct r9a06g032_clkdesc *desc)
> +{
> +       struct r9a06g032_clk_div *div;
> +       struct clk *clk;
> +       struct clk_init_data init;
> +       int i;

unsigned int i;


> +static void __init r9a06g032_clocks_init(struct device_node *np)
> +{
> +       struct r9a06g032_priv *clocks;
> +       struct clk **clks;
> +       unsigned int i;
> +       uint16_t uart_group_sel[2];
> +
> +       clocks = kzalloc(sizeof(*clocks), GFP_KERNEL);
> +       clks = kzalloc(R9A06G032_CLOCK_COUNT * sizeof(struct clk *),
> +                       GFP_KERNEL);

kcalloc()?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

      parent reply	other threads:[~2018-05-30 19:49 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-24  9:28 [PATCH v7 0/5] arm: Base support for Renesas RZN1D-DB Board Michel Pollet
2018-05-24  9:28 ` [PATCH v7 1/5] dt-bindings: Add the r9a06g032-sysctrl.h file Michel Pollet
2018-05-25 10:31   ` Geert Uytterhoeven
2018-05-31  9:11     ` M P
2018-05-31  9:32       ` Geert Uytterhoeven
2018-05-31 10:01         ` M P
2018-06-01  8:18           ` Geert Uytterhoeven
2018-05-24  9:28 ` [PATCH v7 2/5] dt-bindings: clock: renesas,r9a06g032-sysctrl: documentation Michel Pollet
2018-05-25  9:23   ` Geert Uytterhoeven
2018-05-31 10:16     ` M P
2018-06-01  8:22       ` Geert Uytterhoeven
2018-05-24  9:28 ` [PATCH v7 3/5] ARM: dts: Renesas R9A06G032 base device tree file Michel Pollet
2018-05-25  9:27   ` Geert Uytterhoeven
2018-05-28  9:15     ` Simon Horman
2018-05-24  9:28 ` [PATCH v7 4/5] ARM: dts: Renesas RZN1D-DB Board base file Michel Pollet
2018-05-25  9:28   ` Geert Uytterhoeven
2018-05-24  9:28 ` [PATCH v7 5/5] clk: renesas: Renesas R9A06G032 clock driver Michel Pollet
2018-05-26 13:45   ` kbuild test robot
2018-05-30 19:49   ` Geert Uytterhoeven [this message]

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=CAMuHMdW9fSKZv2qA2oz9NCL+MdASAMnxf1UZomJv3_Zts44ygw@mail.gmail.com \
    --to=geert@linux-m68k.org \
    --cc=buserror+upstream@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=geert+renesas@glider.be \
    --cc=horms@verge.net.au \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=michel.pollet@bp.renesas.com \
    --cc=mturquette@baylibre.com \
    --cc=phil.edworthy@renesas.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --subject='Re: [PATCH v7 5/5] clk: renesas: Renesas R9A06G032 clock driver' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).