On Mon, May 18, 2015 at 11:11:34AM +0200, Jens Kuske wrote:
> Hi,
>
> On 05/17/15 14:50, Maxime Ripard wrote:
> > Hi Jens,
> >
> > On Fri, May 15, 2015 at 06:38:52PM +0200, Jens Kuske wrote:
> >> Some newer sunxi SoCs (A83T, H3) don't have individual registers for
> >> AHB1, APB1 and APB2 gates anymore, but one big bus gates area where each
> >> gate can have a different parent.
> >>
> >> The current clock driver sets the same parent for all gates in a group.
> >> This commit adds a new parents field to the gates_data structure, which
> >> allows us to specify an array of parent indices for every single gate.
> >>
> >> Signed-off-by: Jens Kuske <[email protected]>
> >> ---
> >> drivers/clk/sunxi/clk-sunxi.c | 12 +++++++++++-
> >> 1 file changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> >> index 9a82f17..17cba4d 100644
> >> --- a/drivers/clk/sunxi/clk-sunxi.c
> >> +++ b/drivers/clk/sunxi/clk-sunxi.c
> >> @@ -898,6 +898,8 @@ static void __init sunxi_divider_clk_setup(struct device_node *node,
> >>
> >> struct gates_data {
> >> DECLARE_BITMAP(mask, SUNXI_GATES_MAX_SIZE);
> >> + /* If used, ARRAY_SIZE(parents) has to be >= bitmap_weight(mask) */
> >> + const u8 *parents;
> >> };
> >>
> >> static const struct gates_data sun4i_axi_gates_data __initconst = {
> >> @@ -1000,16 +1002,21 @@ static void __init sunxi_gates_clk_setup(struct device_node *node,
> >> struct gates_data *data)
> >> {
> >> struct clk_onecell_data *clk_data;
> >> + const char *parents[SUNXI_MAX_PARENTS];
> >> const char *clk_parent;
> >> const char *clk_name;
> >> void __iomem *reg;
> >> + int npar = 0;
> >> int qty;
> >> int i = 0;
> >> int j = 0;
> >>
> >> reg = of_iomap(node, 0);
> >>
> >> - clk_parent = of_clk_get_parent_name(node, 0);
> >> + while (npar < SUNXI_MAX_PARENTS &&
> >> + (parents[npar] = of_clk_get_parent_name(node, npar)) != NULL)
> >> + npar++;
> >> + clk_parent = parents[0];
> >>
> >> /* Worst-case size approximation and memory allocation */
> >> qty = find_last_bit(data->mask, SUNXI_GATES_MAX_SIZE);
> >> @@ -1026,6 +1033,9 @@ static void __init sunxi_gates_clk_setup(struct device_node *node,
> >> of_property_read_string_index(node, "clock-output-names",
> >> j, &clk_name);
> >>
> >> + if (data->parents && !WARN_ON(data->parents[j] >= npar))
> >> + clk_parent = parents[data->parents[j]];
> >> +
> >
> > I'm currently removing that code, so I was more expecting a new
> > standalone driver for that clock.
>
> How do you want to replace that code? To me this looks like a good way
> to set up all the different gates sunxi has.

By using clock-indices and moving all this gate stuff out of clk-sunxi
and into a new driver.

It's done already, I just need to make sure everything works at it
used to on all the SoCs.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (2.94 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-05-19 08:00:06

On Tue, May 19, 2015 at 04:02:39PM +0800, Chen-Yu Tsai wrote:
> On Tue, May 19, 2015 at 3:55 PM, Maxime Ripard
> <[email protected]> wrote:
> > On Mon, May 18, 2015 at 11:32:31AM +0200, Jens Kuske wrote:
> >> Hi,
> >>
> >> On 05/17/15 16:19, Maxime Ripard wrote:
> >> > On Fri, May 15, 2015 at 06:38:54PM +0200, Jens Kuske wrote:
> >> >> Add a remove function and export the init and remove function
> >> >> to allow us to build the SoC specific drivers as modules.
> >> >>
> >> >> Signed-off-by: Jens Kuske <[email protected]>
> >> >> ---
> >> >> drivers/pinctrl/sunxi/pinctrl-sunxi.c | 25 +++++++++++++++++++------
> >> >> drivers/pinctrl/sunxi/pinctrl-sunxi.h | 2 ++
> >> >> 2 files changed, 21 insertions(+), 6 deletions(-)
> >> >>
> >> >> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> >> >> index f8e171b..4ef6b3d 100644
> >> >> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> >> >> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> >> >> @@ -856,7 +856,6 @@ int sunxi_pinctrl_init(struct platform_device *pdev,
> >> >> struct sunxi_pinctrl *pctl;
> >> >> struct resource *res;
> >> >> int i, ret, last_pin;
> >> >> - struct clk *clk;
> >> >>
> >> >> pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
> >> >> if (!pctl)
> >> >> @@ -954,13 +953,13 @@ int sunxi_pinctrl_init(struct platform_device *pdev,
> >> >> goto gpiochip_error;
> >> >> }
> >> >>
> >> >> - clk = devm_clk_get(&pdev->dev, NULL);
> >> >> - if (IS_ERR(clk)) {
> >> >> - ret = PTR_ERR(clk);
> >> >> + pctl->clk = devm_clk_get(&pdev->dev, NULL);
> >> >> + if (IS_ERR(pctl->clk)) {
> >> >> + ret = PTR_ERR(pctl->clk);
> >> >> goto gpiochip_error;
> >> >> }
> >> >>
> >> >> - ret = clk_prepare_enable(clk);
> >> >> + ret = clk_prepare_enable(pctl->clk);
> >> >> if (ret)
> >> >> goto gpiochip_error;
> >> >>
> >> >> @@ -1015,10 +1014,24 @@ int sunxi_pinctrl_init(struct platform_device *pdev,
> >> >> return 0;
> >> >>
> >> >> clk_error:
> >> >> - clk_disable_unprepare(clk);
> >> >> + clk_disable_unprepare(pctl->clk);
> >> >> gpiochip_error:
> >> >> gpiochip_remove(pctl->chip);
> >> >> pinctrl_error:
> >> >> pinctrl_unregister(pctl->pctl_dev);
> >> >> return ret;
> >> >> }
> >> >> +EXPORT_SYMBOL(sunxi_pinctrl_init);
> >> >> +
> >> >> +int sunxi_pinctrl_remove(struct platform_device *pdev)
> >> >> +{
> >> >> + struct sunxi_pinctrl *pctl = platform_get_drvdata(pdev);
> >> >> +
> >> >> + gpiochip_remove(pctl->chip);
> >> >> + pinctrl_unregister(pctl->pctl_dev);
> >> >> +
> >> >> + clk_disable_unprepare(pctl->clk);
> >> >
> >> > We should also remove the domain and the interrupt mapping here.
> >>
> >> Ouch, I missed that. Only looked at the *_error: labels.
> >>
> >> Apart from that, currently the kernel panics some seconds after removing
> >> the pinctrl module because mmc wants to access a gpio. Can this be
> >> prevented somehow? I think pinctrl must not be removed once other
> >> devices use any pin-related things.
> >
> > pinctrl_unregister doesn't look like it cares about whether or not
> > there's users left in the system.
> >
> > Maybe the easiest path would be to just make this builtin like Paul
> > suggested then ... :/
>
> Is there a way to mark modules as not removable? At least we can keep
> the multi-platform kernel image small.

If there's no module_exit, the module will only be removable through a
rmmod -f, which seems like an acceptable solution.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (3.57 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-05-19 08:30:09

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v2 03/10] clk: sunxi: Let divs clocks read the base factor clock name from devicetree

On Mon, May 18, 2015 at 11:22:32AM +0200, Jens Kuske wrote:
> On 05/17/15 15:06, Maxime Ripard wrote:
> > On Fri, May 15, 2015 at 06:38:53PM +0200, Jens Kuske wrote:
> >> Currently, the sunxi clock driver gets the name for the base factor clock
> >> of divs clocks from the name field in factors_data. This prevents reusing
> >> of the factor clock for clocks with same properties, but different name.
> >>
> >> This commit makes the divs setup function try to get a name from
> >> clock-output-names in the devicetree. It also removes the name field where
> >> possible and merges the sun4i PLL5 and PLL6 clocks.
> >>
> >> The sun4i PLL5 clock doesn't have a output for the base factor clock,
> >> so we still have to use the name field there.
> >>
> >> Signed-off-by: Jens Kuske <[email protected]>
> >> ---
> >> drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++----------
> >> 1 file changed, 12 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> >> index 17cba4d..afe560c 100644
> >> --- a/drivers/clk/sunxi/clk-sunxi.c
> >> +++ b/drivers/clk/sunxi/clk-sunxi.c
> >> @@ -708,18 +708,10 @@ static const struct factors_data sun4i_pll5_data __initconst = {
> >> .name = "pll5",
> >> };
> >>
> >> -static const struct factors_data sun4i_pll6_data __initconst = {
> >> - .enable = 31,
> >> - .table = &sun4i_pll5_config,
> >> - .getter = sun4i_get_pll5_factors,
> >> - .name = "pll6",
> >> -};
> >> -
> >> static const struct factors_data sun6i_a31_pll6_data __initconst = {
> >> .enable = 31,
> >> .table = &sun6i_a31_pll6_config,
> >> .getter = sun6i_a31_get_pll6_factors,
> >> - .name = "pll6x2",
> >> };
> >>
> >> static const struct factors_data sun5i_a13_ahb_data __initconst = {
> >> @@ -1099,7 +1091,7 @@ static const struct divs_data pll5_divs_data __initconst = {
> >> };
> >>
> >> static const struct divs_data pll6_divs_data __initconst = {
> >> - .factors = &sun4i_pll6_data,
> >> + .factors = &sun4i_pll5_data,
> >> .ndivs = 4,
> >> .div = {
> >> { .shift = 0, .table = pll6_sata_tbl, .gate = 14 }, /* M, SATA */
> >> @@ -1141,6 +1133,7 @@ static void __init sunxi_divs_clk_setup(struct device_node *node,
> >> struct clk_gate *gate = NULL;
> >> struct clk_fixed_factor *fix_factor;
> >> struct clk_divider *divider;
> >> + struct factors_data factors = *data->factors;
> >> void __iomem *reg;
> >> int ndivs = SUNXI_DIVS_MAX_QTY, i = 0;
> >> int flags, clkflags;
> >> @@ -1149,8 +1142,17 @@ static void __init sunxi_divs_clk_setup(struct device_node *node,
> >> if (data->ndivs)
> >> ndivs = data->ndivs;
> >>
> >> + /* Try to find a name for base factor clock */
> >> + for (i = 0; i < ndivs; i++) {
> >> + if (data->div[i].self) {
> >
> > I'm not sure we should expect the factor clock to have a self factor.
>
> Maybe not, but in that case it would fall back to the name provided in
> the factors_data struct, as it is the case for sun4i pll5.

Which doesn't really solve the underlying issue, just make it work in
your (pll6/pll8) case.

> > what about taking the first output and taking the substring up to the
> > first "_" ?
>
> That only works for the sun[457]i pll5 & pll6, for sun[68]i the base
> clock name would have to be "pll6x2".

Why?

It's called pll6 in the datasheet, it should really be called
pll6. The fact that there is some fixed factors or dividers on some
children only impacts those children, and not the system as a whole.

And the fact that the "base" clock is pll6x2 and not pll6 itself is
very debatable. How do you know which clock is the base one? Most
clocks are actually using pll6 as a reference.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (3.69 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-05-19 08:55:14

On Tue, May 19, 2015 at 04:04:58PM +0200, Linus Walleij wrote:
> On Fri, May 15, 2015 at 6:38 PM, Jens Kuske <[email protected]> wrote:
>
> > The H3 uses the same pin controller as previous SoC's from Allwinner.
> > Add support for the pins controlled by the main PIO controller.
> >
> > Signed-off-by: Jens Kuske <[email protected]>
>
> Waiting for Maxime's ACKs on these patches before merging.

There's still the modules issue being debated on another patch of this
serie, so there will probably be a v3 of these patches.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (647.00 B)
signature.asc (819.00 B)
Digital signature Download all attachments