| Message ID | 1544690354-16409-4-git-send-email-sunny.luo@amlogic.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: meson-axg: add few enhanced features | expand |
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) > 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; > + 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; > + 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) { > + 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; > + > + 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); > + 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[] = { >
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[] = { > >
Hi Neil, On 2018/12/13 16:55, 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. Would you please show the reason? > >> >> + /* 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. > Yes, I moved it here and forgot remove it at prepare_message().
Hi Jerome, On 2018/12/13 17:28, Jerome Brunet wrote: > On Thu, 2018-12-13 at 09:55 +0100, Neil Armstrong wrote: >>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig >>> 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 > ? > I did it by the way. Maybe it's better to add it in patch 1. >>> +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 > ? > Good, It is really confusing, I will change next patch. >>> + div1 = &meson_spicc_div1; >>> + 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. This is a terrible bug for more than one instances. I must overwrite it. >>> + 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 ? Yes, all Soc with 'has_oen' must have 'has_enhance_clk_div' too. It is distinct to use two flags here, what do you think of it? >>> + 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 ... > There are two div in AXG, one is the coarse old-div and the other is enhance-div. From SoCs designer's opinion, the ehance-div aims to take place of the old-div.
On Thu, 2018-12-13 at 22:37 +0800, Sunny Luo wrote: > Hi Jerome, > > On 2018/12/13 17:28, Jerome Brunet wrote: > > On Thu, 2018-12-13 at 09:55 +0100, Neil Armstrong wrote: > > > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig > > > > 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 > > ? > > I did it by the way. Maybe it's better to add it in patch 1. > > > > +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 > > ? > > Good, It is really confusing, I will change next patch. > > > > + div1 = &meson_spicc_div1; > > > > + 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. > > This is a terrible bug for more than one instances. I must overwrite it. You must set them properly, yes ... having these static data is not necessary > > > > > > + 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 ? > > Yes, all Soc with 'has_oen' must have 'has_enhance_clk_div' too. > It is distinct to use two flags here, what do you think of it? I wonder if you should be using of_device_is_compatible() instead of adding these flags. > > > > > + 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 ... > > > There are two div in AXG, one is the coarse old-div and the other is > enhance-div. From SoCs designer's opinion, the ehance-div aims to take > place of the old-div. ... and all likelyhood, CCF will pick it BUT, unless the old path is broken, please let the framework do its job. If the old path was broken you should not have described it ... but we have been using it so far, so we know it works fine. it's a very simple fix: drop CLK_SET_RATE_NO_REPARENT and your call clk_set_parent() > > Last but not least, I did not see it in my initial review, but: I see you call clk_set_rate(spicc->clk, ...) but I don't see any call to clk_prepare_enable() and clk_disable_unprepare(). It works because the input clock and your local tree does not gate, but it is wrong. A driver should not assume that they clock is enabled by default.
Hi Sunny,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on spi/for-next]
[also build test WARNING on v4.20-rc6 next-20181214]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sunny-Luo/spi-meson-axg-support-MAX-80M-clock/20181214-175627
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=xtensa
All warnings (new ones prefixed by >>):
drivers/spi/spi-meson-spicc.c: In function 'meson_spicc_clk_init':
>> drivers/spi/spi-meson-spicc.c:583:28: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
div1->reg = spicc->base + (u64)div1->reg;
^
drivers/spi/spi-meson-spicc.c:621:28: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
div1->reg = spicc->base + (u64)div1->reg;
^
drivers/spi/spi-meson-spicc.c:638:27: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
mux->reg = spicc->base + (u64)mux->reg;
^
vim +583 drivers/spi/spi-meson-spicc.c
546
547 static int meson_spicc_clk_init(struct meson_spicc_device *spicc)
548 {
549 struct device *dev = &spicc->pdev->dev;
550 struct clk_fixed_factor *div0;
551 struct clk_divider *div1;
552 struct clk_mux *mux;
553 struct clk_init_data init;
554 struct clk *clk;
555 const char *parent_names[1];
556 const char *mux_parent_names[2];
557 char name[32];
558
559 div0 = &meson_spicc_div0;
560 snprintf(name, sizeof(name), "%s#_div0", dev_name(dev));
561 init.name = name;
562 init.ops = &clk_fixed_factor_ops;
563 init.flags = 0;
564 parent_names[0] = __clk_get_name(spicc->core);
565 init.parent_names = parent_names;
566 init.num_parents = 1;
567
568 div0->hw.init = &init;
569
570 clk = devm_clk_register(dev, &div0->hw);
571 if (WARN_ON(IS_ERR(clk)))
572 return PTR_ERR(clk);
573
574 div1 = &meson_spicc_div1;
575 snprintf(name, sizeof(name), "%s#_div1", dev_name(dev));
576 init.name = name;
577 init.ops = &clk_divider_ops;
578 init.flags = CLK_SET_RATE_PARENT;
579 parent_names[0] = __clk_get_name(clk);
580 init.parent_names = parent_names;
581 init.num_parents = 1;
582
> 583 div1->reg = spicc->base + (u64)div1->reg;
584 div1->hw.init = &init;
585
586 clk = devm_clk_register(dev, &div1->hw);
587 if (WARN_ON(IS_ERR(clk)))
588 return PTR_ERR(clk);
589
590 if (!spicc->data->has_enhance_clk_div) {
591 spicc->clk = clk;
592 return 0;
593 }
594
595 mux_parent_names[0] = __clk_get_name(clk);
596
597 div0 = &meson_spicc_div2;
598 snprintf(name, sizeof(name), "%s#_div2", dev_name(dev));
599 init.name = name;
600 init.ops = &clk_fixed_factor_ops;
601 init.flags = 0;
602 parent_names[0] = __clk_get_name(spicc->core);
603 init.parent_names = parent_names;
604 init.num_parents = 1;
605
606 div0->hw.init = &init;
607
608 clk = devm_clk_register(dev, &div0->hw);
609 if (WARN_ON(IS_ERR(clk)))
610 return PTR_ERR(clk);
611
612 div1 = &meson_spicc_div3;
613 snprintf(name, sizeof(name), "%s#_div3", dev_name(dev));
614 init.name = name;
615 init.ops = &clk_divider_ops;
616 init.flags = CLK_SET_RATE_PARENT;
617 parent_names[0] = __clk_get_name(clk);
618 init.parent_names = parent_names;
619 init.num_parents = 1;
620
621 div1->reg = spicc->base + (u64)div1->reg;
622 div1->hw.init = &init;
623
624 clk = devm_clk_register(dev, &div1->hw);
625 if (WARN_ON(IS_ERR(clk)))
626 return PTR_ERR(clk);
627
628 mux_parent_names[1] = __clk_get_name(clk);
629
630 mux = &meson_spicc_sel;
631 snprintf(name, sizeof(name), "%s#_sel", dev_name(dev));
632 init.name = name;
633 init.ops = &clk_mux_ops;
634 init.parent_names = mux_parent_names;
635 init.num_parents = 2;
636 init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT;
637
638 mux->reg = spicc->base + (u64)mux->reg;
639 mux->hw.init = &init;
640
641 spicc->clk = devm_clk_register(dev, &mux->hw);
642 if (WARN_ON(IS_ERR(spicc->clk)))
643 return PTR_ERR(spicc->clk);
644
645 clk_set_parent(spicc->clk, clk);
646 return 0;
647 }
648
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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) 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; + 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; + 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) { + 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; + + 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); + return 0; +} + 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); + /* 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[] = {