DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: devel@driverdev.osuosl.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org, shubhrajyoti.datta@gmail.com
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
	Chirag Parekh <chirag.parekh@xilinx.com>,
	gregkh@linuxfoundation.org, mturquette@baylibre.com,
	Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>,
	Michal Simek <michal.simek@xilinx.com>,
	robh+dt@kernel.org, soren.brinkmann@xilinx.com
Subject: Re: [PATCH v3 04/10] clk: clock-wizard: Add support for dynamic reconfiguration
Date: Sun, 05 Jan 2020 11:54:55 -0800
Message-ID: <20200105195456.7FDDF206F0@mail.kernel.org> (raw)
In-Reply-To: <7a951e702f0c25217bacfcd4444d347d8bb2621e.1574922435.git.shubhrajyoti.datta@xilinx.com>

Quoting shubhrajyoti.datta@gmail.com (2019-11-27 22:36:11)
> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> 
> The patch adds support for dynamic reconfiguration of clock output rate.
> Output clocks are registered as dividers and set rate callback function
> is used for dynamic reconfiguration.
> 
> Based on the initial work from Chirag.
> 
> Signed-off-by: Chirag Parekh <chirag.parekh@xilinx.com>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

Are these more like Co-developed-by: tags?

> diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
> index ef9125d..870e7fb 100644
> --- a/drivers/clk/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/clk-xlnx-clock-wizard.c
> @@ -29,8 +29,23 @@
>  #define WZRD_DIVCLK_DIVIDE_SHIFT       0
>  #define WZRD_DIVCLK_DIVIDE_MASK                (0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
>  #define WZRD_CLKOUT_DIVIDE_SHIFT       0
> +#define WZRD_CLKOUT_DIVIDE_WIDTH       8
>  #define WZRD_CLKOUT_DIVIDE_MASK                (0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
>  
> +#define WZRD_DR_MAX_INT_DIV_VALUE      255
> +#define WZRD_DR_NUM_RETRIES            10000
> +#define WZRD_DR_STATUS_REG_OFFSET      0x04
> +#define WZRD_DR_LOCK_BIT_MASK          0x00000001
> +#define WZRD_DR_INIT_REG_OFFSET                0x25C
> +#define WZRD_DR_DIV_TO_PHASE_OFFSET    4
> +#define WZRD_DR_BEGIN_DYNA_RECONF      0x03
> +
> +/* Get the mask from width */
> +#define div_mask(width)                        ((1 << (width)) - 1)
> +
> +/* Extract divider instance from clock hardware instance */
> +#define to_clk_wzrd_divider(_hw) container_of(_hw, struct clk_wzrd_divider, hw)
> +
>  enum clk_wzrd_int_clks {
>         wzrd_clk_mul,
>         wzrd_clk_mul_div,
> @@ -62,6 +77,29 @@ struct clk_wzrd {
>         bool suspended;
>  };
>  
> +/**
> + * struct clk_wzrd_divider - clock divider specific to clk_wzrd
> + *
> + * @hw:                handle between common and hardware-specific interfaces
> + * @base:      base address of register containing the divider
> + * @offset:    offset address of register containing the divider
> + * @shift:     shift to the divider bit field
> + * @width:     width of the divider bit field
> + * @flags:     clk_wzrd divider flags
> + * @table:     array of value/divider pairs, last entry should have div = 0
> + * @lock:      register lock
> + */
> +struct clk_wzrd_divider {
> +       struct clk_hw hw;
> +       void __iomem *base;
> +       u16 offset;
> +       u8 shift;
> +       u8 width;
> +       u8 flags;
> +       const struct clk_div_table *table;
> +       spinlock_t *lock;  /* divider lock */

Is this register lock used by anything? I'm mostly wondering if dividers
are in a shared register with some other clk so we need to have this
lock. Otherwise it adds more conditional locking code for no gain and
should be removed to simplify.

> +};
> +
>  #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
>  
>  /* maximum frequencies for input/output clocks per speed grade */
> @@ -71,6 +109,164 @@ static const unsigned long clk_wzrd_max_freq[] = {
>         1066000000UL
>  };
>  
> +/* spin lock variable for clk_wzrd */
> +static DEFINE_SPINLOCK(clkwzrd_lock);
> +
> +static unsigned long clk_wzrd_recalc_rate(struct clk_hw *hw,
> +                                         unsigned long parent_rate)
> +{
> +       struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
> +       void __iomem *div_addr =
> +                       (void __iomem *)((u64)divider->base + divider->offset);

Is this casting necessary?

> +       unsigned int val;
> +
> +       val = readl(div_addr) >> divider->shift;
> +       val &= div_mask(divider->width);
> +
> +       return divider_recalc_rate(hw, parent_rate, val, divider->table,
> +                       divider->flags, divider->width);
> +}
> +
> +static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate,
> +                                    unsigned long parent_rate)
> +{
> +       int err = 0;
> +       u16 retries;
> +       u32 value;
> +       unsigned long flags = 0;
> +       struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
> +       void __iomem *div_addr =
> +                       (void __iomem *)((u64)divider->base + divider->offset);
> +
> +       if (divider->lock)
> +               spin_lock_irqsave(divider->lock, flags);
> +       else
> +               __acquire(divider->lock);
> +
> +       value = DIV_ROUND_CLOSEST(parent_rate, rate);
> +
> +       /* Cap the value to max */
> +       if (value > WZRD_DR_MAX_INT_DIV_VALUE)
> +               value = WZRD_DR_MAX_INT_DIV_VALUE;

Please use min().

> +
> +       /* Set divisor and clear phase offset */
> +       writel(value, div_addr);
> +       writel(0x00, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET);
> +
> +       /* Check status register */
> +       retries = WZRD_DR_NUM_RETRIES;
> +       while (retries--) {
> +               if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) &
> +                                                       WZRD_DR_LOCK_BIT_MASK)
> +                       break;
> +       }

Is this readl_poll_timeout()?

> +
> +       if (retries == 0) {
> +               err = -ETIMEDOUT;
> +               goto err_reconfig;
> +       }
> +
> +       /* Initiate reconfiguration */
> +       writel(WZRD_DR_BEGIN_DYNA_RECONF,
> +              divider->base + WZRD_DR_INIT_REG_OFFSET);
> +
> +       /* Check status register */
> +       retries = WZRD_DR_NUM_RETRIES;
> +       while (retries--) {
> +               if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) &
> +                                                       WZRD_DR_LOCK_BIT_MASK)
> +                       break;
> +       }
> +
> +       if (retries == 0)
> +               err = -ETIMEDOUT;

readl_poll_timeout()?

> +
> +err_reconfig:
> +       if (divider->lock)
> +               spin_unlock_irqrestore(divider->lock, flags);
> +       else
> +               __release(divider->lock);
> +
> +       return err;
> +}
> +
> +static long clk_wzrd_round_rate(struct clk_hw *hw, unsigned long rate,
> +                               unsigned long *prate)
> +{
> +       u8 div;
> +
> +       /*
> +        * since we donot change parent rate we just round rate to closest

s/donot/don't/

> +        * achievable
> +        */
> +       div = DIV_ROUND_CLOSEST(*prate, rate);
> +
> +       return (*prate / div);
> +}
> +
> +static const struct clk_ops clk_wzrd_clk_divider_ops = {
> +       .round_rate = clk_wzrd_round_rate,
> +       .set_rate = clk_wzrd_dynamic_reconfig,
> +       .recalc_rate = clk_wzrd_recalc_rate,
> +};
> +
> +static struct clk *clk_wzrd_register_divider(struct device *dev,
> +                                            const char *name,
> +                                            const char *parent_name,
> +                                            unsigned long flags,
> +                                            void __iomem *base, u16 offset,
> +                                            u8 shift, u8 width,
> +                                            u8 clk_divider_flags,
> +                                            const struct clk_div_table *table,
> +                                            spinlock_t *lock)
> +{
> +       struct clk_wzrd_divider *div;
> +       struct clk_hw *hw;
> +       struct clk_init_data init;
> +       int ret;
> +
> +       if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) {

Do you use HIWORD mask things? I thought that was mostly rockchip
specific quirk. If not, please remove this code.

> +               if (width + shift > 16) {
> +                       pr_warn("divider value exceeds LOWORD field\n");
> +                       return ERR_PTR(-EINVAL);
> +               }
> +       }
> +
> +       /* allocate the divider */
> +       div = kzalloc(sizeof(*div), GFP_KERNEL);
> +       if (!div)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.name = name;
> +       if (clk_divider_flags & CLK_DIVIDER_READ_ONLY)
> +               init.ops = &clk_divider_ro_ops;
> +       else
> +               init.ops = &clk_wzrd_clk_divider_ops;
> +       init.flags = flags | CLK_IS_BASIC;

Oh yeah, don't add this flag. This code won't compile so please compile
test each patch in a series to make sure we don't have compilation
bisection holes.

> +       init.parent_names = (parent_name ? &parent_name : NULL);
> +       init.num_parents = (parent_name ? 1 : 0);

Is it sometimes the root of the tree? Seems unlikely so probably just
assume there is a parent all the time.

> +
> +       /* struct clk_divider assignments */

Drop useless comment please.

> +       div->base = base;
> +       div->offset = offset;
> +       div->shift = shift;
> +       div->width = width;
> +       div->flags = clk_divider_flags;
> +       div->lock = lock;
> +       div->hw.init = &init;
> +       div->table = table;
> +
> +       /* register the clock */

Drop useless comment please.

> +       hw = &div->hw;
> +       ret = clk_hw_register(dev, hw);
> +       if (ret) {
> +               kfree(div);
> +               hw = ERR_PTR(ret);
> +       }
> +
> +       return hw->clk;
> +}
> +
>  static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
>                                  void *data)
>  {
> @@ -241,11 +437,14 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>                         ret = -EINVAL;
>                         goto err_rm_int_clks;
>                 }
> -               reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(2) + i * 12);
> -               reg &= WZRD_CLKOUT_DIVIDE_MASK;
> -               reg >>= WZRD_CLKOUT_DIVIDE_SHIFT;
> -               clk_wzrd->clkout[i] = clk_register_fixed_factor
> -                       (&pdev->dev, clkout_name, clk_name, 0, 1, reg);
> +               clk_wzrd->clkout[i] = clk_wzrd_register_divider(&pdev->dev,
> +                                                               clkout_name,
> +                               clk_name, 0,
> +                               clk_wzrd->base, (WZRD_CLK_CFG_REG(2) + i * 12),

Please remove useless parenthesis.

> +                               WZRD_CLKOUT_DIVIDE_SHIFT,
> +                               WZRD_CLKOUT_DIVIDE_WIDTH,
> +                               CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO,
> +                               NULL, &clkwzrd_lock);
>                 if (IS_ERR(clk_wzrd->clkout[i])) {
>                         int j;
>  
> -- 
> 2.1.1
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-28  6:36 [PATCH v3 00/10] clk: clk-wizard: clock-wizard: Driver updates shubhrajyoti.datta
2019-11-28  6:36 ` [PATCH v3 01/10] dt-bindings: add documentation of xilinx clocking wizard shubhrajyoti.datta
2019-12-13 19:52   ` Rob Herring
2019-11-28  6:36 ` [PATCH v3 02/10] clk: clock-wizard: Move the clockwizard to clk shubhrajyoti.datta
2019-12-13 19:49   ` Rob Herring
2019-11-28  6:36 ` [PATCH v3 03/10] clk: clock-wizard: Fix kernel-doc warning shubhrajyoti.datta
2020-01-05 19:48   ` Stephen Boyd
2019-11-28  6:36 ` [PATCH v3 04/10] clk: clock-wizard: Add support for dynamic reconfiguration shubhrajyoti.datta
2020-01-05 19:54   ` Stephen Boyd [this message]
2019-11-28  6:36 ` [PATCH v3 05/10] clk: clock-wizard: Add support for fractional support shubhrajyoti.datta
2019-11-28  6:36 ` [PATCH v3 06/10] clk: clock-wizard: Remove the hardcoding of the clock outputs shubhrajyoti.datta
2020-01-05 19:56   ` Stephen Boyd
2019-11-28  6:36 ` [PATCH v3 07/10] clk: clock-wizard: Update the fixed factor divisors shubhrajyoti.datta
2020-01-05 20:00   ` Stephen Boyd
2020-01-06  4:17     ` Shubhrajyoti Datta
2020-01-06  6:10       ` Stephen Boyd
2019-11-28  6:36 ` [PATCH v3 08/10] clk: clock-wizard: Make the output names unique shubhrajyoti.datta
2019-11-28  7:45   ` Dan Carpenter
2019-11-29 12:07     ` Shubhrajyoti Datta
2019-11-29 19:02       ` Dan Carpenter
2019-11-28  6:36 ` [PATCH v3 09/10] staging: clocking-wizard: Delete the driver from the staging shubhrajyoti.datta
2020-01-05 19:45   ` Stephen Boyd
2019-11-28  6:36 ` [PATCH v3 10/10] clk: clock-wizard: Fix the compilation failure shubhrajyoti.datta
2020-01-05 19:46   ` Stephen Boyd

Reply instructions:

You may reply publically 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=20200105195456.7FDDF206F0@mail.kernel.org \
    --to=sboyd@kernel.org \
    --cc=chirag.parekh@xilinx.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=michal.simek@xilinx.com \
    --cc=mturquette@baylibre.com \
    --cc=robh+dt@kernel.org \
    --cc=shubhrajyoti.datta@gmail.com \
    --cc=shubhrajyoti.datta@xilinx.com \
    --cc=soren.brinkmann@xilinx.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

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git