Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1396209 > unrolled thread

Re: [PATCH v3 2/4] clk: arizona: Add clock driver for the Arizona devices

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/4] clk: arizona: Add clock driver for the Arizona  devices Stephen Boyd <sboyd@codeaurora.org> - 2016-05-07 03:00 +0200
    Re: [PATCH v3 2/4] clk: arizona: Add clock driver for the Arizona  devices Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2016-05-09 12:50 +0200
      Re: [PATCH v3 2/4] clk: arizona: Add clock driver for the Arizona  devices Stephen Boyd <sboyd@codeaurora.org> - 2016-05-09 23:50 +0200
        Re: [PATCH v3 2/4] clk: arizona: Add clock driver for the Arizona  devices Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2016-05-10 12:00 +0200

#1396209 — Re: [PATCH v3 2/4] clk: arizona: Add clock driver for the Arizona devices

I've applied this to clk-next but still have a question, see
below.

On 01/08, Charles Keepax wrote:
> diff --git a/drivers/clk/clk-arizona.c b/drivers/clk/clk-arizona.c
> new file mode 100644
> index 0000000..eaf2877
> --- /dev/null
> +++ b/drivers/clk/clk-arizona.c
> +
> +static int arizona_clk_of_get_pdata(struct arizona *arizona)
> +{
> +	const char * const pins[] = { "mclk1", "mclk2" };
> +	struct clk *mclk;
> +	int i;
> +
> +	if (!of_property_read_bool(arizona->dev->of_node, "clocks"))
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(pins); ++i) {
> +		mclk = of_clk_get_by_name(arizona->dev->of_node, pins[i]);
> +		if (IS_ERR(mclk))
> +			return PTR_ERR(mclk);
> +
> +		if (clk_get_rate(mclk) == CLK32K_RATE) {
> +			arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK1 + i;
> +			arizona->pdata.clk32k_parent = __clk_get_name(mclk);
> +		}
> +
> +		clk_put(mclk);

Could this be done through assigned parents instead of this rate
checking stuff? Presumably DT could tell us how the clk tree
should be configured.

> +	}
> +
> +	return 0;
> +}
> +
> +static int arizona_clk_probe(struct platform_device *pdev)
> +{
> +	struct arizona *arizona = dev_get_drvdata(pdev->dev.parent);
> +	struct arizona_clk *clkdata;
> +	int ret;
> +
> +	struct clk_init_data clk32k_init = {
> +		.name = "arizona-32k",
> +		.ops = &arizona_32k_ops,
> +	};
> +
> +	if (IS_ENABLED(CONFIG_OF) && !dev_get_platdata(arizona->dev)) {
> +		ret = arizona_clk_of_get_pdata(arizona);
> +		if (ret) {
> +			dev_err(arizona->dev, "Failed parsing clock DT: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
> +	clkdata = devm_kzalloc(&pdev->dev, sizeof(*clkdata), GFP_KERNEL);
> +	if (!clkdata)
> +		return -ENOMEM;
> +
> +	clkdata->arizona = arizona;
> +
> +	switch (arizona->pdata.clk32k_src) {
> +	case 0:
> +		arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK2;
> +		/* Fall through */
> +	case ARIZONA_32KZ_MCLK1:
> +	case ARIZONA_32KZ_MCLK2:
> +	case ARIZONA_32KZ_NONE:
> +		regmap_update_bits(arizona->regmap, ARIZONA_CLOCK_32K_1,
> +				   ARIZONA_CLK_32K_SRC_MASK,
> +				   arizona->pdata.clk32k_src - 1);
> +		break;
> +	default:
> +		dev_err(arizona->dev, "Invalid 32kHz clock source: %d\n",
> +			arizona->pdata.clk32k_src);
> +		return -EINVAL;
> +	}
> +
> +	if (arizona->pdata.clk32k_parent) {
> +		clk32k_init.num_parents = 1;
> +		clk32k_init.parent_names = &arizona->pdata.clk32k_parent;
> +	} else {
> +		clk32k_init.flags |= CLK_IS_ROOT;

This flag is going away so I deleted it.

> +	}
> +
> +	clkdata->clk32k_hw.init = &clk32k_init;
> +	clkdata->clk32k = devm_clk_register(&pdev->dev, &clkdata->clk32k_hw);
> +	if (IS_ERR(clkdata->clk32k)) {
> +		ret = PTR_ERR(clkdata->clk32k);
> +		dev_err(arizona->dev, "Failed to register 32k clock: %d\n",
> +			ret);
> +		return ret;
> +	}

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1396929

On Fri, May 06, 2016 at 05:55:01PM -0700, Stephen Boyd wrote:
> I've applied this to clk-next but still have a question, see
> below.
> 
> On 01/08, Charles Keepax wrote:
> > diff --git a/drivers/clk/clk-arizona.c b/drivers/clk/clk-arizona.c
> > new file mode 100644
> > index 0000000..eaf2877
> > --- /dev/null
> > +++ b/drivers/clk/clk-arizona.c
> > +
> > +static int arizona_clk_of_get_pdata(struct arizona *arizona)
> > +{
> > +	const char * const pins[] = { "mclk1", "mclk2" };
> > +	struct clk *mclk;
> > +	int i;
> > +
> > +	if (!of_property_read_bool(arizona->dev->of_node, "clocks"))
> > +		return 0;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(pins); ++i) {
> > +		mclk = of_clk_get_by_name(arizona->dev->of_node, pins[i]);
> > +		if (IS_ERR(mclk))
> > +			return PTR_ERR(mclk);
> > +
> > +		if (clk_get_rate(mclk) == CLK32K_RATE) {
> > +			arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK1 + i;
> > +			arizona->pdata.clk32k_parent = __clk_get_name(mclk);
> > +		}
> > +
> > +		clk_put(mclk);
> 
> Could this be done through assigned parents instead of this rate
> checking stuff? Presumably DT could tell us how the clk tree
> should be configured.
> 

Apologies, I have been working on a v4 that includes these
improvements. It does indeed look much nicer using assigned
parents etc. I think it might be best to drop these for now until
those are ready to send.

The only problem I really have left to sort out before I can send
it are some locking issues. It is quite tricky to get interaction
between the clocking and SPI frameworks to play nicely. The SPI
framework will sometimes punt the actually processing for the
transfer to a worker thread which will often perform operations
on clocks required for the SPI. Because this is a seperate
thread it isn't handled by the re-enterant locking in the clock
framework. I had been working around this using async transfers
for the SPI, but even then I have since found you can get lockdep
warnings because of the potential mutex inversion (SPI mutex and
the clock one).

Any suggestions on this front would be greatly appreciated?

Thanks, Charles

[toc] | [prev] | [next] | [standalone]

#1397424

On 05/09, Charles Keepax wrote:
> On Fri, May 06, 2016 at 05:55:01PM -0700, Stephen Boyd wrote:
> > I've applied this to clk-next but still have a question, see
> > below.
> > 
> > On 01/08, Charles Keepax wrote:
> > > diff --git a/drivers/clk/clk-arizona.c b/drivers/clk/clk-arizona.c
> > > new file mode 100644
> > > index 0000000..eaf2877
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-arizona.c
> > > +
> > > +static int arizona_clk_of_get_pdata(struct arizona *arizona)
> > > +{
> > > +	const char * const pins[] = { "mclk1", "mclk2" };
> > > +	struct clk *mclk;
> > > +	int i;
> > > +
> > > +	if (!of_property_read_bool(arizona->dev->of_node, "clocks"))
> > > +		return 0;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(pins); ++i) {
> > > +		mclk = of_clk_get_by_name(arizona->dev->of_node, pins[i]);
> > > +		if (IS_ERR(mclk))
> > > +			return PTR_ERR(mclk);
> > > +
> > > +		if (clk_get_rate(mclk) == CLK32K_RATE) {
> > > +			arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK1 + i;
> > > +			arizona->pdata.clk32k_parent = __clk_get_name(mclk);
> > > +		}
> > > +
> > > +		clk_put(mclk);
> > 
> > Could this be done through assigned parents instead of this rate
> > checking stuff? Presumably DT could tell us how the clk tree
> > should be configured.
> > 
> 
> Apologies, I have been working on a v4 that includes these
> improvements. It does indeed look much nicer using assigned
> parents etc. I think it might be best to drop these for now until
> those are ready to send.

Ok sure. I've dropped them.

> 
> The only problem I really have left to sort out before I can send
> it are some locking issues. It is quite tricky to get interaction
> between the clocking and SPI frameworks to play nicely. The SPI
> framework will sometimes punt the actually processing for the
> transfer to a worker thread which will often perform operations
> on clocks required for the SPI. Because this is a seperate
> thread it isn't handled by the re-enterant locking in the clock
> framework. I had been working around this using async transfers
> for the SPI, but even then I have since found you can get lockdep
> warnings because of the potential mutex inversion (SPI mutex and
> the clock one).
> 
> Any suggestions on this front would be greatly appreciated?
> 

The fix is to always prepare the first clk right? That way we
avoid any deadlock scenario.

We've been slowly working our way toward an alternate solution,
which is to have one mutex per clk so that different parts of the
clk tree can be locked independently, but so far that's blocked
on drivers re-entering the clk framework with clk consumer APIs
from within the clk_ops callbacks. Hopefully coordinated clk rate
switches will allow us to get rid of those situations and then we
can go and make sure all drivers aren't relying on the big
prepare mutex to keep their drivers safe from concurrent accesses
and finally move to one mutex per clk. This is a long term goal
though so I wouldn't depend on this happening anytime soon.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1397932

On Mon, May 09, 2016 at 02:48:29PM -0700, Stephen Boyd wrote:
> On 05/09, Charles Keepax wrote:
> > On Fri, May 06, 2016 at 05:55:01PM -0700, Stephen Boyd wrote:
> > > I've applied this to clk-next but still have a question, see
> > > below.
> > > 
> > > On 01/08, Charles Keepax wrote:
> > Apologies, I have been working on a v4 that includes these
> > improvements. It does indeed look much nicer using assigned
> > parents etc. I think it might be best to drop these for now until
> > those are ready to send.
> 
> Ok sure. I've dropped them.

Cool thanks.

> 
> > 
> > The only problem I really have left to sort out before I can send
> > it are some locking issues. It is quite tricky to get interaction
> > between the clocking and SPI frameworks to play nicely. The SPI
> > framework will sometimes punt the actually processing for the
> > transfer to a worker thread which will often perform operations
> > on clocks required for the SPI. Because this is a seperate
> > thread it isn't handled by the re-enterant locking in the clock
> > framework. I had been working around this using async transfers
> > for the SPI, but even then I have since found you can get lockdep
> > warnings because of the potential mutex inversion (SPI mutex and
> > the clock one).
> > 
> > Any suggestions on this front would be greatly appreciated?
> > 
> 
> The fix is to always prepare the first clk right? That way we
> avoid any deadlock scenario.

Yeah, been looking at that, the problem is our parts get used
in a fairly wide array of places and it tends to be up to the
individual SPI driver how the clocks are controlled. So feels a
bit like SPI driver whack a mole, perhaps something could be done
through the SPI core itself through.

> 
> We've been slowly working our way toward an alternate solution,
> which is to have one mutex per clk so that different parts of the
> clk tree can be locked independently, but so far that's blocked
> on drivers re-entering the clk framework with clk consumer APIs
> from within the clk_ops callbacks. Hopefully coordinated clk rate
> switches will allow us to get rid of those situations and then we
> can go and make sure all drivers aren't relying on the big
> prepare mutex to keep their drivers safe from concurrent accesses
> and finally move to one mutex per clk. This is a long term goal
> though so I wouldn't depend on this happening anytime soon.

Thanks, really good to hear some thoughts on this.  I had been
coming to the conclusion here that individual prepare locks was
the right course of action. I had tried some changes to allow
individual clock drivers to specify prepare_lock and unlock
callbacks and then having the clock core fall back to the global
lock if the driver didn't have these. Which is a sort of half way
house to individual locks, but its still quite easy to run into
mutex inversions with all the other clocks still sharing a global
lock.

Thanks,
Charles

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web