All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: Guenter Roeck <linux@roeck-us.net>,
	mturquette@baylibre.com, linux-clk@vger.kernel.org,
	Neil Armstrong <narmstrong@baylibre.com>,
	jbrunet@baylibre.com, khilman@baylibre.com,
	linux-kernel@vger.kernel.org, linux-amlogic@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 2/3] clk: divider: Switch from .round_rate to .determine_rate by default
Date: Fri, 02 Jul 2021 13:59:10 -0700	[thread overview]
Message-ID: <162525955027.3570193.16463056788252699243@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <CAFBinCAASQUB=cg5EFsBQ4jd3TvcCJzV1=sYJci4ibR7FjRcww@mail.gmail.com>

Quoting Martin Blumenstingl (2021-07-02 02:19:37)
> Hi Stephen,
> 
> On Fri, Jul 2, 2021 at 3:02 AM Stephen Boyd <sboyd@kernel.org> wrote:
> [...]
> > My guess is that we have drivers copying the clk_ops from the
> > divider_ops structure and so they are copying over round_rate but not
> > determine_rate.
> I just learned something new - thanks for investigating this as well!
> 
> $ git grep "clk_divider_ops\.round_rate" drivers/
> drivers/clk/bcm/clk-bcm2835.c:  return clk_divider_ops.round_rate(hw,
> rate, parent_rate);
> drivers/clk/clk-stm32f4.c:      return clk_divider_ops.round_rate(hw,
> rate, prate);
> drivers/clk/clk-stm32h7.c:      return clk_divider_ops.round_rate(hw,
> rate, prate);
> drivers/clk/clk-stm32mp1.c:             req->rate =
> clk_divider_ops.round_rate(hw, req->rate, &best_parent_rate);
> drivers/clk/imx/clk-divider-gate.c:     return
> clk_divider_ops.round_rate(hw, rate, prate);
> $ git grep "clk_divider_ro_ops\.round_rate" drivers/
> $
> 
> Changing these over to use clk_divider_ops.determine_rate doesn't seem too hard.
> The part that I am not sure about is how to organize the patches.
> 1) amend the changes to all relevant drivers (from above) to this patch
> 2) multiple patches:
> - adding .determine_rate to the default divider ops (but not removing
> .round_rate)
> - a single patch for each relevant driver (from above)
> - removing .round_rate from the default divider ops
> 
> Another approach is to first create clk_divider_determine_rate() (as
> done here) and export it.
> Then I could have one individual patch for each relevant driver (from
> above) to use:
>   .determine_rate = clk_divider_determine_rate,
> Then finally I could remove clk_divider_round_rate() and switch over
> the default divider ops to .determine_rate as well.
> 
> Which way do you prefer?
> 

I'd prefer we leave round_rate assigned in clk_divider_ops and
clk_divider_ro_ops but then also assign the determine_rate function. We
have some duplication but that's OK. Then make individual patches to
migrate each driver over to the new clk op.

We could stack a final patch on top to remove the round_rate function
from clk divider.  Unfortunately, if some driver wants to use round_rate
then it will fail in interesting ways. Probably best to live with it
until we decide to drop round_rate entirely. Patches to convert all the
round_rate code over to determine_rate would be welcome in the meantime.

WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <sboyd@kernel.org>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: Guenter Roeck <linux@roeck-us.net>,
	mturquette@baylibre.com, linux-clk@vger.kernel.org,
	Neil Armstrong <narmstrong@baylibre.com>,
	jbrunet@baylibre.com, khilman@baylibre.com,
	linux-kernel@vger.kernel.org, linux-amlogic@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 2/3] clk: divider: Switch from .round_rate to .determine_rate by default
Date: Fri, 02 Jul 2021 13:59:10 -0700	[thread overview]
Message-ID: <162525955027.3570193.16463056788252699243@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <CAFBinCAASQUB=cg5EFsBQ4jd3TvcCJzV1=sYJci4ibR7FjRcww@mail.gmail.com>

Quoting Martin Blumenstingl (2021-07-02 02:19:37)
> Hi Stephen,
> 
> On Fri, Jul 2, 2021 at 3:02 AM Stephen Boyd <sboyd@kernel.org> wrote:
> [...]
> > My guess is that we have drivers copying the clk_ops from the
> > divider_ops structure and so they are copying over round_rate but not
> > determine_rate.
> I just learned something new - thanks for investigating this as well!
> 
> $ git grep "clk_divider_ops\.round_rate" drivers/
> drivers/clk/bcm/clk-bcm2835.c:  return clk_divider_ops.round_rate(hw,
> rate, parent_rate);
> drivers/clk/clk-stm32f4.c:      return clk_divider_ops.round_rate(hw,
> rate, prate);
> drivers/clk/clk-stm32h7.c:      return clk_divider_ops.round_rate(hw,
> rate, prate);
> drivers/clk/clk-stm32mp1.c:             req->rate =
> clk_divider_ops.round_rate(hw, req->rate, &best_parent_rate);
> drivers/clk/imx/clk-divider-gate.c:     return
> clk_divider_ops.round_rate(hw, rate, prate);
> $ git grep "clk_divider_ro_ops\.round_rate" drivers/
> $
> 
> Changing these over to use clk_divider_ops.determine_rate doesn't seem too hard.
> The part that I am not sure about is how to organize the patches.
> 1) amend the changes to all relevant drivers (from above) to this patch
> 2) multiple patches:
> - adding .determine_rate to the default divider ops (but not removing
> .round_rate)
> - a single patch for each relevant driver (from above)
> - removing .round_rate from the default divider ops
> 
> Another approach is to first create clk_divider_determine_rate() (as
> done here) and export it.
> Then I could have one individual patch for each relevant driver (from
> above) to use:
>   .determine_rate = clk_divider_determine_rate,
> Then finally I could remove clk_divider_round_rate() and switch over
> the default divider ops to .determine_rate as well.
> 
> Which way do you prefer?
> 

I'd prefer we leave round_rate assigned in clk_divider_ops and
clk_divider_ro_ops but then also assign the determine_rate function. We
have some duplication but that's OK. Then make individual patches to
migrate each driver over to the new clk op.

We could stack a final patch on top to remove the round_rate function
from clk divider.  Unfortunately, if some driver wants to use round_rate
then it will fail in interesting ways. Probably best to live with it
until we decide to drop round_rate entirely. Patches to convert all the
round_rate code over to determine_rate would be welcome in the meantime.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <sboyd@kernel.org>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: Guenter Roeck <linux@roeck-us.net>,
	mturquette@baylibre.com, linux-clk@vger.kernel.org,
	Neil Armstrong <narmstrong@baylibre.com>,
	jbrunet@baylibre.com, khilman@baylibre.com,
	linux-kernel@vger.kernel.org, linux-amlogic@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 2/3] clk: divider: Switch from .round_rate to .determine_rate by default
Date: Fri, 02 Jul 2021 13:59:10 -0700	[thread overview]
Message-ID: <162525955027.3570193.16463056788252699243@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <CAFBinCAASQUB=cg5EFsBQ4jd3TvcCJzV1=sYJci4ibR7FjRcww@mail.gmail.com>

Quoting Martin Blumenstingl (2021-07-02 02:19:37)
> Hi Stephen,
> 
> On Fri, Jul 2, 2021 at 3:02 AM Stephen Boyd <sboyd@kernel.org> wrote:
> [...]
> > My guess is that we have drivers copying the clk_ops from the
> > divider_ops structure and so they are copying over round_rate but not
> > determine_rate.
> I just learned something new - thanks for investigating this as well!
> 
> $ git grep "clk_divider_ops\.round_rate" drivers/
> drivers/clk/bcm/clk-bcm2835.c:  return clk_divider_ops.round_rate(hw,
> rate, parent_rate);
> drivers/clk/clk-stm32f4.c:      return clk_divider_ops.round_rate(hw,
> rate, prate);
> drivers/clk/clk-stm32h7.c:      return clk_divider_ops.round_rate(hw,
> rate, prate);
> drivers/clk/clk-stm32mp1.c:             req->rate =
> clk_divider_ops.round_rate(hw, req->rate, &best_parent_rate);
> drivers/clk/imx/clk-divider-gate.c:     return
> clk_divider_ops.round_rate(hw, rate, prate);
> $ git grep "clk_divider_ro_ops\.round_rate" drivers/
> $
> 
> Changing these over to use clk_divider_ops.determine_rate doesn't seem too hard.
> The part that I am not sure about is how to organize the patches.
> 1) amend the changes to all relevant drivers (from above) to this patch
> 2) multiple patches:
> - adding .determine_rate to the default divider ops (but not removing
> .round_rate)
> - a single patch for each relevant driver (from above)
> - removing .round_rate from the default divider ops
> 
> Another approach is to first create clk_divider_determine_rate() (as
> done here) and export it.
> Then I could have one individual patch for each relevant driver (from
> above) to use:
>   .determine_rate = clk_divider_determine_rate,
> Then finally I could remove clk_divider_round_rate() and switch over
> the default divider ops to .determine_rate as well.
> 
> Which way do you prefer?
> 

I'd prefer we leave round_rate assigned in clk_divider_ops and
clk_divider_ro_ops but then also assign the determine_rate function. We
have some duplication but that's OK. Then make individual patches to
migrate each driver over to the new clk op.

We could stack a final patch on top to remove the round_rate function
from clk divider.  Unfortunately, if some driver wants to use round_rate
then it will fail in interesting ways. Probably best to live with it
until we decide to drop round_rate entirely. Patches to convert all the
round_rate code over to determine_rate would be welcome in the meantime.

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

  parent reply	other threads:[~2021-07-02 20:59 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-27 22:39 [PATCH v3 0/3] clk: meson: rounding for fast clocks on 32-bit SoCs Martin Blumenstingl
2021-06-27 22:39 ` Martin Blumenstingl
2021-06-27 22:39 ` Martin Blumenstingl
2021-06-27 22:39 ` [PATCH v3 1/3] clk: divider: Add re-usable determine_rate implementations Martin Blumenstingl
2021-06-27 22:39   ` Martin Blumenstingl
2021-06-27 22:39   ` Martin Blumenstingl
2021-06-30 18:39   ` Stephen Boyd
2021-06-30 18:39     ` Stephen Boyd
2021-06-30 18:39     ` Stephen Boyd
2021-06-27 22:39 ` [PATCH v3 2/3] clk: divider: Switch from .round_rate to .determine_rate by default Martin Blumenstingl
2021-06-27 22:39   ` Martin Blumenstingl
2021-06-27 22:39   ` Martin Blumenstingl
2021-06-30 18:39   ` Stephen Boyd
2021-06-30 18:39     ` Stephen Boyd
2021-06-30 18:39     ` Stephen Boyd
2021-07-01 20:25   ` Guenter Roeck
2021-07-01 20:25     ` Guenter Roeck
2021-07-01 20:25     ` Guenter Roeck
2021-07-01 20:57     ` Martin Blumenstingl
2021-07-01 20:57       ` Martin Blumenstingl
2021-07-01 20:57       ` Martin Blumenstingl
2021-07-01 21:43       ` Guenter Roeck
2021-07-01 21:43         ` Guenter Roeck
2021-07-01 21:43         ` Guenter Roeck
2021-07-02  0:53         ` Stephen Boyd
2021-07-02  0:53           ` Stephen Boyd
2021-07-02  0:53           ` Stephen Boyd
2021-07-02  1:02       ` Stephen Boyd
2021-07-02  1:02         ` Stephen Boyd
2021-07-02  1:02         ` Stephen Boyd
2021-07-02  9:19         ` Martin Blumenstingl
2021-07-02  9:19           ` Martin Blumenstingl
2021-07-02  9:19           ` Martin Blumenstingl
     [not found]           ` <CGME20210702124612eucas1p1762911deb37e4fb03adc9239bb715135@eucas1p1.samsung.com>
2021-07-02 12:46             ` Marek Szyprowski
2021-07-02 12:46               ` Marek Szyprowski
2021-07-02 12:46               ` Marek Szyprowski
2021-07-02 21:00               ` Stephen Boyd
2021-07-02 21:00                 ` Stephen Boyd
2021-07-02 21:00                 ` Stephen Boyd
2021-07-02 20:59           ` Stephen Boyd [this message]
2021-07-02 20:59             ` Stephen Boyd
2021-07-02 20:59             ` Stephen Boyd
2021-07-02 22:57             ` Martin Blumenstingl
2021-07-02 22:57               ` Martin Blumenstingl
2021-07-02 22:57               ` Martin Blumenstingl
2021-07-02  1:04   ` Stephen Boyd
2021-07-02  1:04     ` Stephen Boyd
2021-07-02  1:04     ` Stephen Boyd
2021-06-27 22:39 ` [PATCH v3 3/3] clk: meson: regmap: switch to determine_rate for the dividers Martin Blumenstingl
2021-06-27 22:39   ` Martin Blumenstingl
2021-06-27 22:39   ` Martin Blumenstingl
2021-06-30 18:39   ` Stephen Boyd
2021-06-30 18:39     ` Stephen Boyd
2021-06-30 18:39     ` Stephen Boyd

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=162525955027.3570193.16463056788252699243@swboyd.mtv.corp.google.com \
    --to=sboyd@kernel.org \
    --cc=jbrunet@baylibre.com \
    --cc=khilman@baylibre.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=martin.blumenstingl@googlemail.com \
    --cc=mturquette@baylibre.com \
    --cc=narmstrong@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.