From: Jerome Brunet <jbrunet@baylibre.com>
To: Neil Armstrong <narmstrong@baylibre.com>,
Sunny Luo <sunny.luo@amlogic.com>,
Mark Brown <broonie@kernel.org>
Cc: Yixun Lan <yixun.lan@amlogic.com>,
Kevin Hilman <khilman@baylibre.com>,
Carlo Caione <carlo@caione.org>,
Jianxin Pan <jianxin.pan@amlogic.com>,
Xingyu Chen <xingyu.chen@amlogic.com>,
linux-spi@vger.kernel.org, linux-amlogic@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/3] spi: meson-axg: add a linear clock divider support
Date: Thu, 13 Dec 2018 10:28:30 +0100 [thread overview]
Message-ID: <e6d5220e45a54d5251bec0f8fd3c147307f8b071.camel@baylibre.com> (raw)
In-Reply-To: <3cc699dc-4021-b993-2b47-b00b40f380f8@baylibre.com>
On Thu, 2018-12-13 at 09:55 +0100, Neil Armstrong wrote:
> Hi Sunny,
>
> On 13/12/2018 09:39, Sunny Luo wrote:
> > The SPICC controller in Meson-AXG SoC is capable of using
> > a linear clock divider to reach a much fine tuned range of clocks,
> > while the old controller only use a power of two clock divider,
> > result at a more coarse clock range.
>
> This patch should definitely go before patch 1.
>
> > Also convert the clock registration into Common Clock Framework.
> >
> > Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
> > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> > ---
> > drivers/spi/Kconfig | 2 +-
> > drivers/spi/spi-meson-spicc.c | 209 ++++++++++++++++++++++++++++++++++---
> > -----
> > 2 files changed, 170 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index 9f89cb1..75f7362 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -384,7 +384,7 @@ config SPI_FSL_ESPI
> >
> > config SPI_MESON_SPICC
> > tristate "Amlogic Meson SPICC controller"
> > - depends on ARCH_MESON || COMPILE_TEST
> > + depends on OF && COMMON_CLK && (ARCH_MESON || COMPILE_TEST)
The purpose of this patch is clock, right ? Why does it add a dependency on OF
?
> > help
> > This enables master mode support for the SPICC (SPI communication
> > controller) available in Amlogic Meson SoCs.
> > diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> > index 0384c28..59af18e 100644
> > --- a/drivers/spi/spi-meson-spicc.c
> > +++ b/drivers/spi/spi-meson-spicc.c
> > @@ -116,6 +116,9 @@
> > #define SPICC_DWADDR 0x24 /* Write Address of DMA */
> >
> > #define SPICC_ENH_CTL0 0x38 /* Enhanced Feature */
> > +#define SPICC_ENH_CLK_CS_DELAY_MASK GENMASK(15, 0)
> > +#define SPICC_ENH_DATARATE_MASK GENMASK(23, 16)
> > +#define SPICC_ENH_DATARATE_EN BIT(24)
> > #define SPICC_ENH_MOSI_OEN BIT(25)
> > #define SPICC_ENH_CLK_OEN BIT(26)
> > #define SPICC_ENH_CS_OEN BIT(27)
> > @@ -131,6 +134,7 @@
> > struct meson_spicc_data {
> > unsigned int max_speed_hz;
> > bool has_oen;
> > + bool has_enhance_clk_div;
> > };
> >
> > struct meson_spicc_device {
> > @@ -138,6 +142,7 @@ struct meson_spicc_device {
> > struct platform_device *pdev;
> > void __iomem *base;
> > struct clk *core;
> > + struct clk *clk;
> > struct spi_message *message;
> > struct spi_transfer *xfer;
> > const struct meson_spicc_data *data;
> > @@ -325,40 +330,6 @@ static irqreturn_t meson_spicc_irq(int irq, void
> > *data)
> > return IRQ_HANDLED;
> > }
> >
> > -static u32 meson_spicc_setup_speed(struct meson_spicc_device *spicc, u32
> > conf,
> > - u32 speed)
> > -{
> > - unsigned long parent, value;
> > - unsigned int i, div;
> > -
> > - parent = clk_get_rate(spicc->core);
> > -
> > - /* Find closest inferior/equal possible speed */
> > - for (i = 0 ; i < 7 ; ++i) {
> > - /* 2^(data_rate+2) */
> > - value = parent >> (i + 2);
> > -
> > - if (value <= speed)
> > - break;
> > - }
> > -
> > - /* If provided speed it lower than max divider, use max divider */
> > - if (i > 7) {
> > - div = 7;
> > - dev_warn_once(&spicc->pdev->dev, "unable to get close to speed
> > %u\n",
> > - speed);
> > - } else
> > - div = i;
> > -
> > - dev_dbg(&spicc->pdev->dev, "parent %lu, speed %u -> %lu (%u)\n",
> > - parent, speed, value, div);
> > -
> > - conf &= ~SPICC_DATARATE_MASK;
> > - conf |= FIELD_PREP(SPICC_DATARATE_MASK, div);
> > -
> > - return conf;
> > -}
> > -
> > static void meson_spicc_setup_xfer(struct meson_spicc_device *spicc,
> > struct spi_transfer *xfer)
> > {
> > @@ -367,9 +338,6 @@ static void meson_spicc_setup_xfer(struct
> > meson_spicc_device *spicc,
> > /* Read original configuration */
> > conf = conf_orig = readl_relaxed(spicc->base + SPICC_CONREG);
> >
> > - /* Select closest divider */
> > - conf = meson_spicc_setup_speed(spicc, conf, xfer->speed_hz);
> > -
> > /* Setup word width */
> > conf &= ~SPICC_BITLENGTH_MASK;
> > conf |= FIELD_PREP(SPICC_BITLENGTH_MASK,
> > @@ -378,6 +346,8 @@ static void meson_spicc_setup_xfer(struct
> > meson_spicc_device *spicc,
> > /* Ignore if unchanged */
> > if (conf != conf_orig)
> > writel_relaxed(conf, spicc->base + SPICC_CONREG);
> > +
> > + clk_set_rate(spicc->clk, xfer->speed_hz);
> > }
> >
> > static int meson_spicc_transfer_one(struct spi_master *master,
> > @@ -486,9 +456,6 @@ static int meson_spicc_unprepare_transfer(struct
> > spi_master *master)
> > /* Disable all IRQs */
> > writel(0, spicc->base + SPICC_INTREG);
> >
> > - /* Disable controller */
> > - writel_bits_relaxed(SPICC_ENABLE, 0, spicc->base + SPICC_CONREG);
> > -
> > device_reset_optional(&spicc->pdev->dev);
> >
> > return 0;
> > @@ -528,6 +495,157 @@ static void meson_spicc_cleanup(struct spi_device
> > *spi)
> > spi->controller_state = NULL;
> > }
> >
> > +/*
> > + * The Clock Mux
> > + * x-----------------x x------------x x------\
> > + * |---| 0) fixed factor |---| 1) old div |----| |
> > + * | x-----------------x x------------x | |
> > + * src ---| |5) mux|-- out
> > + * | x-----------------x x------------x | |
> > + * |---| 2) fixed factor |---| 3) new div |0---| |
> > + * x-----------------x x------------x x------/
> > + *
> > + * Clk path for GX series:
> > + * src -> 0 -> 1 -> out
> > + *
> > + * Clk path for AXG series:
> > + * src -> 0 -> 1 -> 5 -> out
> > + * src -> 2 -> 3 -> 5 -> out
> > + */
> > +
> > +/* algorithm for div0 + div1: rate = freq / 4 / (2 ^ N) */
> > +static struct clk_fixed_factor meson_spicc_div0 = {
> > + .mult = 1,
> > + .div = 4,
> > +};
> > +
> > +static struct clk_divider meson_spicc_div1 = {
> > + .reg = (void *)SPICC_CONREG,
> > + .shift = 16,
> > + .width = 3,
> > + .flags = CLK_DIVIDER_POWER_OF_TWO,
> > +};
> > +
> > +/* algorithm for div2 + div3: rate = freq / 2 / (N + 1) */
> > +static struct clk_fixed_factor meson_spicc_div2 = {
> > + .mult = 1,
> > + .div = 2,
> > +};
> > +
> > +static struct clk_divider meson_spicc_div3 = {
> > + .reg = (void *)SPICC_ENH_CTL0,
> > + .shift = 16,
> > + .width = 8,
> > +};
> > +
> > +static struct clk_mux meson_spicc_sel = {
> > + .reg = (void *)SPICC_ENH_CTL0,
> > + .mask = 0x1,
> > + .shift = 24,
> > +};
> > +
> > +static int meson_spicc_clk_init(struct meson_spicc_device *spicc)
> > +{
> > + struct device *dev = &spicc->pdev->dev;
> > + struct clk_fixed_factor *div0;
> > + struct clk_divider *div1;
Could you come up with something better than div0 and div1 ? it is confusing,
especially with the comment above about div3 and 4 ... fixed_factor, div maybe
?
> > + struct clk_mux *mux;
> > + struct clk_init_data init;
> > + struct clk *clk;
> > + const char *parent_names[1];
> > + const char *mux_parent_names[2];
> > + char name[32];
> > +
> > + div0 = &meson_spicc_div0;
> > + snprintf(name, sizeof(name), "%s#_div0", dev_name(dev));
> > + init.name = name;
> > + init.ops = &clk_fixed_factor_ops;
> > + init.flags = 0;
> > + parent_names[0] = __clk_get_name(spicc->core);
> > + init.parent_names = parent_names;
> > + init.num_parents = 1;
> > +
> > + div0->hw.init = &init;
> > +
> > + clk = devm_clk_register(dev, &div0->hw);
> > + if (WARN_ON(IS_ERR(clk)))
> > + return PTR_ERR(clk);
> > +
> > + div1 = &meson_spicc_div1;
> > + snprintf(name, sizeof(name), "%s#_div1", dev_name(dev));
> > + init.name = name;
> > + init.ops = &clk_divider_ops;
> > + init.flags = CLK_SET_RATE_PARENT;
> > + parent_names[0] = __clk_get_name(clk);
> > + init.parent_names = parent_names;
> > + init.num_parents = 1;
> > +
> > + div1->reg = spicc->base + (u64)div1->reg;
So you have static data which you override here. This works only if there is a
single instance ... and does not really improve readability in your case.
IMO, you'd be better off without the static data above.
> > + div1->hw.init = &init;
> > +
> > + clk = devm_clk_register(dev, &div1->hw);
> > + if (WARN_ON(IS_ERR(clk)))
> > + return PTR_ERR(clk);
> > +
> > + if (!spicc->data->has_enhance_clk_div) {
Do all SoC with 'has_oen' have 'has_enhance_clk_div' too ?
DO you really need two flags ?
> > + spicc->clk = clk;
> > + return 0;
> > + }
> > +
> > + mux_parent_names[0] = __clk_get_name(clk);
> > +
> > + div0 = &meson_spicc_div2;
> > + snprintf(name, sizeof(name), "%s#_div2", dev_name(dev));
> > + init.name = name;
> > + init.ops = &clk_fixed_factor_ops;
> > + init.flags = 0;
> > + parent_names[0] = __clk_get_name(spicc->core);
> > + init.parent_names = parent_names;
> > + init.num_parents = 1;
> > +
> > + div0->hw.init = &init;
> > +
> > + clk = devm_clk_register(dev, &div0->hw);
> > + if (WARN_ON(IS_ERR(clk)))
> > + return PTR_ERR(clk);
> > +
> > + div1 = &meson_spicc_div3;
> > + snprintf(name, sizeof(name), "%s#_div3", dev_name(dev));
> > + init.name = name;
> > + init.ops = &clk_divider_ops;
> > + init.flags = CLK_SET_RATE_PARENT;
> > + parent_names[0] = __clk_get_name(clk);
> > + init.parent_names = parent_names;
> > + init.num_parents = 1;
> > +
> > + div1->reg = spicc->base + (u64)div1->reg;
> > + div1->hw.init = &init;
> > +
> > + clk = devm_clk_register(dev, &div1->hw);
> > + if (WARN_ON(IS_ERR(clk)))
> > + return PTR_ERR(clk);
> > +
> > + mux_parent_names[1] = __clk_get_name(clk);
> > +
> > + mux = &meson_spicc_sel;
> > + snprintf(name, sizeof(name), "%s#_sel", dev_name(dev));
> > + init.name = name;
> > + init.ops = &clk_mux_ops;
> > + init.parent_names = mux_parent_names;
> > + init.num_parents = 2;
> > + init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT;
Why CLK_SET_RATE_NO_REPARENT ? CCF should pick the parent that give the best
results, best to let it do its task ...
> > +
> > + mux->reg = spicc->base + (u64)mux->reg;
> > + mux->hw.init = &init;
> > +
> > + spicc->clk = devm_clk_register(dev, &mux->hw);
> > + if (WARN_ON(IS_ERR(spicc->clk)))
> > + return PTR_ERR(spicc->clk);
> > +
> > + clk_set_parent(spicc->clk, clk);
... then you can drop this.
> > + return 0;
> > +}
>
> I'll let Jerome review this !
>
> > +
> > static int meson_spicc_probe(struct platform_device *pdev)
> > {
> > struct spi_master *master;
> > @@ -557,6 +675,10 @@ static int meson_spicc_probe(struct platform_device
> > *pdev)
> > goto out_master;
> > }
> >
> > + /* Set master mode and enable controller */
> > + writel_relaxed(SPICC_ENABLE | SPICC_MODE_MASTER,
> > + spicc->base + SPICC_CONREG);
>
> Please remove it from meson_spicc_prepare_message() now.
>
> > +
> > /* Disable all IRQs */
> > writel_relaxed(0, spicc->base + SPICC_INTREG);
> >
> > @@ -603,6 +725,12 @@ static int meson_spicc_probe(struct platform_device
> > *pdev)
> > master->max_speed_hz = min_t(unsigned int, rate >> 1,
> > spicc->data->max_speed_hz);
> >
> > + ret = meson_spicc_clk_init(spicc);
> > + if (ret) {
> > + dev_err(&pdev->dev, "clock registration failed\n");
> > + goto out_master;
> > + }
> > +
> > ret = devm_spi_register_master(&pdev->dev, master);
> > if (ret) {
> > dev_err(&pdev->dev, "spi master registration failed\n");
> > @@ -639,6 +767,7 @@ static const struct meson_spicc_data
> > meson_spicc_gx_data = {
> > static const struct meson_spicc_data meson_spicc_axg_data = {
> > .max_speed_hz = 80000000,
> > .has_oen = true,
> > + .has_enhance_clk_div = true,
> > };
> >
> > static const struct of_device_id meson_spicc_of_match[] = {
> >
next prev parent reply other threads:[~2018-12-13 9:28 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-13 8:39 [PATCH v2 0/3] spi: meson-axg: add few enhanced features Sunny Luo
2018-12-13 8:39 ` [PATCH v2 1/3] spi: meson-axg: support MAX 80M clock Sunny Luo
2018-12-13 8:49 ` Neil Armstrong
2018-12-13 11:55 ` Sunny Luo
2018-12-13 8:39 ` [PATCH v2 2/3] spi: meson-axg: enhance output enable feature Sunny Luo
2018-12-13 8:53 ` Neil Armstrong
2018-12-13 13:12 ` Sunny Luo
2018-12-13 9:04 ` Jerome Brunet
2018-12-13 11:53 ` Mark Brown
2018-12-13 12:50 ` Sunny Luo
2018-12-13 13:31 ` Sunny Luo
2018-12-13 8:39 ` [PATCH v2 3/3] spi: meson-axg: add a linear clock divider support Sunny Luo
2018-12-13 8:55 ` Neil Armstrong
2018-12-13 9:28 ` Jerome Brunet [this message]
2018-12-13 14:37 ` Sunny Luo
2018-12-13 15:01 ` Jerome Brunet
2018-12-13 13:25 ` Sunny Luo
2018-12-15 18:31 ` kbuild test robot
[not found] ` <1544690354-16409-1-git-send-email-sunny.luo-LpR1jeaWuhtBDgjK7y7TUQ@public.gmane.org>
2020-02-19 8:17 ` [PATCH v2 0/3] spi: meson-axg: add few enhanced features Neil Armstrong
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=e6d5220e45a54d5251bec0f8fd3c147307f8b071.camel@baylibre.com \
--to=jbrunet@baylibre.com \
--cc=broonie@kernel.org \
--cc=carlo@caione.org \
--cc=jianxin.pan@amlogic.com \
--cc=khilman@baylibre.com \
--cc=linux-amlogic@lists.infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=narmstrong@baylibre.com \
--cc=sunny.luo@amlogic.com \
--cc=xingyu.chen@amlogic.com \
--cc=yixun.lan@amlogic.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 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).