[1/5] ASoC: madera: Add DT bindings for Cirrus Logic Madera codecs

From: Richard Fitzgerald <rf@opensource.cirrus.com>

The Cirrus Logic Madera codecs are a family of related codecs with
extensive digital and analogue I/O, digital mixing and routing,
signal processing and programmable DSPs.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 Documentation/devicetree/bindings/sound/madera.txt | 67 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 include/dt-bindings/sound/madera.h                 | 29 ++++++++++
 3 files changed, 97 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/madera.txt
 create mode 100644 include/dt-bindings/sound/madera.h

On Fri, May 24, 2019 at 11:41:55AM +0100, Charles Keepax wrote:

> +	/*
> +	 * Just read a register a few times to ensure the internal
> +	 * oscillator sends out a few clocks.
> +	 */
> +	for (i = 0; i < 4; i++) {
> +		ret = regmap_read(madera->regmap, MADERA_SOFTWARE_RESET, &val);
> +		if (ret)
> +			dev_err(madera->dev,
> +				"%s Failed to read register: %d (%d)\n",
> +				__func__, ret, i);

Why use %s to format the __func__ rather than just concatenate?

> +	}
> +
> +	udelay(300);

So we read the register a few times then add a few hundred us of delay
after?  Surely that delay is going to be negligable compared to the time
spent on I/O?

> +int madera_sysclk_ev(struct snd_soc_dapm_widget *w,
> +		     struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +	struct madera_priv *priv = snd_soc_component_get_drvdata(component);
> +
> +	madera_spin_sysclk(priv);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(madera_sysclk_ev);

This will delay both before and after every power up and power down.
Are you sure that makes sense?

> +
> +	ret = madera_check_speaker_overheat(madera, &warn, &shutdown);
> +	if (ret)
> +		shutdown = true; /* for safety attempt to shutdown on error */
> +
> +	if (shutdown) {
> +		dev_crit(madera->dev, "Thermal shutdown\n");
> +		ret = regmap_update_bits(madera->regmap,
> +					 MADERA_OUTPUT_ENABLES_1,
> +					 MADERA_OUT4L_ENA |
> +					 MADERA_OUT4R_ENA, 0);
> +		if (ret != 0)
> +			dev_crit(madera->dev,
> +				 "Failed to disable speaker outputs: %d\n",
> +				 ret);
> +	} else if (warn) {
> +		dev_crit(madera->dev, "Thermal warning\n");
> +	}
> +
> +	return IRQ_HANDLED;

We will flag the interrupt as handled if there was neither a warning nor
a critical overheat?  I'd expect some warning about a spurious interrupt
at least.

> +static int madera_get_variable_u32_array(struct madera_priv *priv,
> +					 const char *propname,
> +					 u32 *dest,
> +					 int n_max,
> +					 int multiple)
> +{
> +	struct madera *madera = priv->madera;
> +	int n, ret;
> +
> +	n = device_property_read_u32_array(madera->dev, propname, NULL, 0);
> +	if (n == -EINVAL) {
> +		return 0;	/* missing, ignore */
> +	} else if (n < 0) {
> +		dev_warn(madera->dev, "%s malformed (%d)\n",
> +			 propname, n);
> +		return n;
> +	} else if ((n % multiple) != 0) {
> +		dev_warn(madera->dev, "%s not a multiple of %d entries\n",
> +			 propname, multiple);
> +		return -EINVAL;
> +	}
> +
> +	if (n > n_max)
> +		n = n_max;
> +
> +	ret = device_property_read_u32_array(madera->dev, propname, dest, n);
> +
> +	if (ret < 0)
> +		return ret;
> +	else
> +		return n;
> +}

This feels like it should perhaps be a generic OF helper function -
there's nothing driver specific I'm seeing here and arrays that need to
be a multiple of N entries aren't that uncommon I think.

> +	mutex_lock(&priv->rate_lock);
> +	cached_rate = priv->adsp_rate_cache[adsp_num];
> +	mutex_unlock(&priv->rate_lock);

What's this lock protecting?  The value can we read can change as soon
as the lock is released and we're just reading a single word here rather
than traversing a data structure that might change under us or
something.

> +void madera_destroy_bus_error_irq(struct madera_priv *priv, int dsp_num)
> +{
> +	struct madera *madera = priv->madera;
> +
> +	madera_free_irq(madera,
> +			madera_dsp_bus_error_irqs[dsp_num],
> +			&priv->adsp[dsp_num]);
> +}
> +EXPORT_SYMBOL_GPL(madera_destroy_bus_error_irq);

We use free rather than destroy normally?

> +static const char * const madera_dfc_width_text[MADERA_DFC_WIDTH_ENUM_SIZE] = {
> +	"8bit", "16bit", "20bit", "24bit", "32bit",
> +};

Spaces might make these more readable.

> +static void madera_sleep(unsigned int delay)
> +{
> +	if (delay < 20) {
> +		delay *= 1000;
> +		usleep_range(delay, delay + 500);
> +	} else {
> +		msleep(delay);
> +	}
> +}

This feels like it might make sense as a helper function as well - I
could've sworn there was one already but I can't immediately find it.

On 24/05/19 15:56, Mark Brown wrote:
> On Fri, May 24, 2019 at 11:41:55AM +0100, Charles Keepax wrote:
> 
>> +	/*
>> +	 * Just read a register a few times to ensure the internal
>> +	 * oscillator sends out a few clocks.
>> +	 */
>> +	for (i = 0; i < 4; i++) {
>> +		ret = regmap_read(madera->regmap, MADERA_SOFTWARE_RESET, &val);
>> +		if (ret)
>> +			dev_err(madera->dev,
>> +				"%s Failed to read register: %d (%d)\n",
>> +				__func__, ret, i);
> 
> Why use %s to format the __func__ rather than just concatenate?

GCC docs say that it's a magic variable so cannot be concatenated with string literals. Though I
never tried concatenation to see if it works.

> 
>> +	}
>> +
>> +	udelay(300);
> 
> So we read the register a few times then add a few hundred us of delay
> after?  Surely that delay is going to be negligable compared to the time
> spent on I/O?

The register reads are to create clock cycles in the silicon, not to generate delay.

> 
>> +int madera_sysclk_ev(struct snd_soc_dapm_widget *w,
>> +		     struct snd_kcontrol *kcontrol, int event)
>> +{
>> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
>> +	struct madera_priv *priv = snd_soc_component_get_drvdata(component);
>> +
>> +	madera_spin_sysclk(priv);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(madera_sysclk_ev);
> 
> This will delay both before and after every power up and power down.
> Are you sure that makes sense?
>

I think that's correct but we can re-check with hardware people. It's not just a delay,
it needs to ensure there are always a certain number of SYSCLK cycles in the hardware to
avoid leaving certain state machines in limbo.

>> +
>> +	ret = madera_check_speaker_overheat(madera, &warn, &shutdown);
>> +	if (ret)
>> +		shutdown = true; /* for safety attempt to shutdown on error */
>> +
>> +	if (shutdown) {
>> +		dev_crit(madera->dev, "Thermal shutdown\n");
>> +		ret = regmap_update_bits(madera->regmap,
>> +					 MADERA_OUTPUT_ENABLES_1,
>> +					 MADERA_OUT4L_ENA |
>> +					 MADERA_OUT4R_ENA, 0);
>> +		if (ret != 0)
>> +			dev_crit(madera->dev,
>> +				 "Failed to disable speaker outputs: %d\n",
>> +				 ret);
>> +	} else if (warn) {
>> +		dev_crit(madera->dev, "Thermal warning\n");
>> +	}
>> +
>> +	return IRQ_HANDLED;
> 
> We will flag the interrupt as handled if there was neither a warning nor
> a critical overheat?  I'd expect some warning about a spurious interrupt
> at least.
> 
>> +static int madera_get_variable_u32_array(struct madera_priv *priv,
>> +					 const char *propname,
>> +					 u32 *dest,
>> +					 int n_max,
>> +					 int multiple)
>> +{
>> +	struct madera *madera = priv->madera;
>> +	int n, ret;
>> +
>> +	n = device_property_read_u32_array(madera->dev, propname, NULL, 0);
>> +	if (n == -EINVAL) {
>> +		return 0;	/* missing, ignore */
>> +	} else if (n < 0) {
>> +		dev_warn(madera->dev, "%s malformed (%d)\n",
>> +			 propname, n);
>> +		return n;
>> +	} else if ((n % multiple) != 0) {
>> +		dev_warn(madera->dev, "%s not a multiple of %d entries\n",
>> +			 propname, multiple);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (n > n_max)
>> +		n = n_max;
>> +
>> +	ret = device_property_read_u32_array(madera->dev, propname, dest, n);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +	else
>> +		return n;
>> +}
> 
> This feels like it should perhaps be a generic OF helper function -
> there's nothing driver specific I'm seeing here and arrays that need to
> be a multiple of N entries aren't that uncommon I think.
> 
>> +	mutex_lock(&priv->rate_lock);
>> +	cached_rate = priv->adsp_rate_cache[adsp_num];
>> +	mutex_unlock(&priv->rate_lock);
> 
> What's this lock protecting?  The value can we read can change as soon
> as the lock is released and we're just reading a single word here rather
> than traversing a data structure that might change under us or
> something.
> 
>> +void madera_destroy_bus_error_irq(struct madera_priv *priv, int dsp_num)
>> +{
>> +	struct madera *madera = priv->madera;
>> +
>> +	madera_free_irq(madera,
>> +			madera_dsp_bus_error_irqs[dsp_num],
>> +			&priv->adsp[dsp_num]);
>> +}
>> +EXPORT_SYMBOL_GPL(madera_destroy_bus_error_irq);
> 
> We use free rather than destroy normally?
> 
>> +static const char * const madera_dfc_width_text[MADERA_DFC_WIDTH_ENUM_SIZE] = {
>> +	"8bit", "16bit", "20bit", "24bit", "32bit",
>> +};
> 
> Spaces might make these more readable.
> 
>> +static void madera_sleep(unsigned int delay)
>> +{
>> +	if (delay < 20) {
>> +		delay *= 1000;
>> +		usleep_range(delay, delay + 500);
>> +	} else {
>> +		msleep(delay);
>> +	}
>> +}
> 
> This feels like it might make sense as a helper function as well - I
> could've sworn there was one already but I can't immediately find it.
>

On 24/05/19 16:21, Richard Fitzgerald wrote:
> On 24/05/19 15:56, Mark Brown wrote:
>> On Fri, May 24, 2019 at 11:41:55AM +0100, Charles Keepax wrote:
>>
>>> +    /*
>>> +     * Just read a register a few times to ensure the internal
>>> +     * oscillator sends out a few clocks.
>>> +     */
>>> +    for (i = 0; i < 4; i++) {
>>> +        ret = regmap_read(madera->regmap, MADERA_SOFTWARE_RESET, &val);
>>> +        if (ret)
>>> +            dev_err(madera->dev,
>>> +                "%s Failed to read register: %d (%d)\n",
>>> +                __func__, ret, i);
>>
>> Why use %s to format the __func__ rather than just concatenate?
> 
> GCC docs say that it's a magic variable so cannot be concatenated with string literals. Though I
> never tried concatenation to see if it works.
> 
>>
>>> +    }
>>> +
>>> +    udelay(300);
>>
>> So we read the register a few times then add a few hundred us of delay
>> after?  Surely that delay is going to be negligable compared to the time
>> spent on I/O?
> 
> The register reads are to create clock cycles in the silicon, not to generate delay.
> 

Sorry, just re-read your comment and realized I'd misread it. It's a hardware requirement
that after generating the internal clocks there must be a delay. I.e. we require a combination
of a guaranteed number of SYSCLKs followed by a guaranteed minimum delay.

>>
>>> +int madera_sysclk_ev(struct snd_soc_dapm_widget *w,
>>> +             struct snd_kcontrol *kcontrol, int event)
>>> +{
>>> +    struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
>>> +    struct madera_priv *priv = snd_soc_component_get_drvdata(component);
>>> +
>>> +    madera_spin_sysclk(priv);
>>> +
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(madera_sysclk_ev);
>>
>> This will delay both before and after every power up and power down.
>> Are you sure that makes sense?
>>
> 
> I think that's correct but we can re-check with hardware people. It's not just a delay,
> it needs to ensure there are always a certain number of SYSCLK cycles in the hardware to
> avoid leaving certain state machines in limbo.
> 
>>> +
>>> +    ret = madera_check_speaker_overheat(madera, &warn, &shutdown);
>>> +    if (ret)
>>> +        shutdown = true; /* for safety attempt to shutdown on error */
>>> +
>>> +    if (shutdown) {
>>> +        dev_crit(madera->dev, "Thermal shutdown\n");
>>> +        ret = regmap_update_bits(madera->regmap,
>>> +                     MADERA_OUTPUT_ENABLES_1,
>>> +                     MADERA_OUT4L_ENA |
>>> +                     MADERA_OUT4R_ENA, 0);
>>> +        if (ret != 0)
>>> +            dev_crit(madera->dev,
>>> +                 "Failed to disable speaker outputs: %d\n",
>>> +                 ret);
>>> +    } else if (warn) {
>>> +        dev_crit(madera->dev, "Thermal warning\n");
>>> +    }
>>> +
>>> +    return IRQ_HANDLED;
>>
>> We will flag the interrupt as handled if there was neither a warning nor
>> a critical overheat?  I'd expect some warning about a spurious interrupt
>> at least.
>>
>>> +static int madera_get_variable_u32_array(struct madera_priv *priv,
>>> +                     const char *propname,
>>> +                     u32 *dest,
>>> +                     int n_max,
>>> +                     int multiple)
>>> +{
>>> +    struct madera *madera = priv->madera;
>>> +    int n, ret;
>>> +
>>> +    n = device_property_read_u32_array(madera->dev, propname, NULL, 0);
>>> +    if (n == -EINVAL) {
>>> +        return 0;    /* missing, ignore */
>>> +    } else if (n < 0) {
>>> +        dev_warn(madera->dev, "%s malformed (%d)\n",
>>> +             propname, n);
>>> +        return n;
>>> +    } else if ((n % multiple) != 0) {
>>> +        dev_warn(madera->dev, "%s not a multiple of %d entries\n",
>>> +             propname, multiple);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (n > n_max)
>>> +        n = n_max;
>>> +
>>> +    ret = device_property_read_u32_array(madera->dev, propname, dest, n);
>>> +
>>> +    if (ret < 0)
>>> +        return ret;
>>> +    else
>>> +        return n;
>>> +}
>>
>> This feels like it should perhaps be a generic OF helper function -
>> there's nothing driver specific I'm seeing here and arrays that need to
>> be a multiple of N entries aren't that uncommon I think.
>>
>>> +    mutex_lock(&priv->rate_lock);
>>> +    cached_rate = priv->adsp_rate_cache[adsp_num];
>>> +    mutex_unlock(&priv->rate_lock);
>>
>> What's this lock protecting?  The value can we read can change as soon
>> as the lock is released and we're just reading a single word here rather
>> than traversing a data structure that might change under us or
>> something.
>>
>>> +void madera_destroy_bus_error_irq(struct madera_priv *priv, int dsp_num)
>>> +{
>>> +    struct madera *madera = priv->madera;
>>> +
>>> +    madera_free_irq(madera,
>>> +            madera_dsp_bus_error_irqs[dsp_num],
>>> +            &priv->adsp[dsp_num]);
>>> +}
>>> +EXPORT_SYMBOL_GPL(madera_destroy_bus_error_irq);
>>
>> We use free rather than destroy normally?
>>
>>> +static const char * const madera_dfc_width_text[MADERA_DFC_WIDTH_ENUM_SIZE] = {
>>> +    "8bit", "16bit", "20bit", "24bit", "32bit",
>>> +};
>>
>> Spaces might make these more readable.
>>
>>> +static void madera_sleep(unsigned int delay)
>>> +{
>>> +    if (delay < 20) {
>>> +        delay *= 1000;
>>> +        usleep_range(delay, delay + 500);
>>> +    } else {
>>> +        msleep(delay);
>>> +    }
>>> +}
>>
>> This feels like it might make sense as a helper function as well - I
>> could've sworn there was one already but I can't immediately find it.
>>
>

On Fri, May 24, 2019 at 03:56:03PM +0100, Mark Brown wrote:
> On Fri, May 24, 2019 at 11:41:55AM +0100, Charles Keepax wrote:
> 
> > +	/*
> > +	 * Just read a register a few times to ensure the internal
> > +	 * oscillator sends out a few clocks.
> > +	 */
> > +	for (i = 0; i < 4; i++) {
> > +		ret = regmap_read(madera->regmap, MADERA_SOFTWARE_RESET, &val);
> > +		if (ret)
> > +			dev_err(madera->dev,
> > +				"%s Failed to read register: %d (%d)\n",
> > +				__func__, ret, i);
> 
> Why use %s to format the __func__ rather than just concatenate?
> 

Really the error message is quite out of character in general,
we don't normally print the func and it doesn't really say what
failed. I will refactor it.

> > +	}
> > +
> > +	udelay(300);
> 
> So we read the register a few times then add a few hundred us of delay
> after?  Surely that delay is going to be negligable compared to the time
> spent on I/O?
> 

This is one of those technically situations. We have to do
the reads and we have to have at least 300uS after those.
Normally there will be enough slack in the system but better to
guarantee it. Maybe a comment?

> > +int madera_sysclk_ev(struct snd_soc_dapm_widget *w,
> > +		     struct snd_kcontrol *kcontrol, int event)
> > +{
> > +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> > +	struct madera_priv *priv = snd_soc_component_get_drvdata(component);
> > +
> > +	madera_spin_sysclk(priv);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(madera_sysclk_ev);
> 
> This will delay both before and after every power up and power down.
> Are you sure that makes sense?
> 

Regrettably yes, or well at least I am sure that despite a lot of
complaining this is what the hardware guys insisted needed done.

> > +
> > +	ret = madera_check_speaker_overheat(madera, &warn, &shutdown);
> > +	if (ret)
> > +		shutdown = true; /* for safety attempt to shutdown on error */
> > +
> > +	if (shutdown) {
> > +		dev_crit(madera->dev, "Thermal shutdown\n");
> > +		ret = regmap_update_bits(madera->regmap,
> > +					 MADERA_OUTPUT_ENABLES_1,
> > +					 MADERA_OUT4L_ENA |
> > +					 MADERA_OUT4R_ENA, 0);
> > +		if (ret != 0)
> > +			dev_crit(madera->dev,
> > +				 "Failed to disable speaker outputs: %d\n",
> > +				 ret);
> > +	} else if (warn) {
> > +		dev_crit(madera->dev, "Thermal warning\n");
> > +	}
> > +
> > +	return IRQ_HANDLED;
> 
> We will flag the interrupt as handled if there was neither a warning nor
> a critical overheat?  I'd expect some warning about a spurious interrupt
> at least.
> 

Yeah I will update that one.

> > +static int madera_get_variable_u32_array(struct madera_priv *priv,
> > +					 const char *propname,
> > +					 u32 *dest,
> > +					 int n_max,
> > +					 int multiple)
> > +{
> > +	struct madera *madera = priv->madera;
> > +	int n, ret;
> > +
> > +	n = device_property_read_u32_array(madera->dev, propname, NULL, 0);
> > +	if (n == -EINVAL) {
> > +		return 0;	/* missing, ignore */
> > +	} else if (n < 0) {
> > +		dev_warn(madera->dev, "%s malformed (%d)\n",
> > +			 propname, n);
> > +		return n;
> > +	} else if ((n % multiple) != 0) {
> > +		dev_warn(madera->dev, "%s not a multiple of %d entries\n",
> > +			 propname, multiple);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (n > n_max)
> > +		n = n_max;
> > +
> > +	ret = device_property_read_u32_array(madera->dev, propname, dest, n);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +	else
> > +		return n;
> > +}
> 
> This feels like it should perhaps be a generic OF helper function -
> there's nothing driver specific I'm seeing here and arrays that need to
> be a multiple of N entries aren't that uncommon I think.
> 

I will have a look and see if we might do a generic helper
function for this one.

> > +	mutex_lock(&priv->rate_lock);
> > +	cached_rate = priv->adsp_rate_cache[adsp_num];
> > +	mutex_unlock(&priv->rate_lock);
> 
> What's this lock protecting?  The value can we read can change as soon
> as the lock is released and we're just reading a single word here rather
> than traversing a data structure that might change under us or
> something.
> 

Overall the lock protects anything that changes a rate on the
chip, since we have to do that weird spin_sysclk thing
before/after anything that does that.

It might normally be safe without the lock here I guess, since
you are only really protecting against the reads of adsp_rate_cache.
But suppose that might depend on the architecture if the read/writes
arn't effectively atomic (although that is probably unlikely on most
modern architectures).

> > +void madera_destroy_bus_error_irq(struct madera_priv *priv, int dsp_num)
> > +{
> > +	struct madera *madera = priv->madera;
> > +
> > +	madera_free_irq(madera,
> > +			madera_dsp_bus_error_irqs[dsp_num],
> > +			&priv->adsp[dsp_num]);
> > +}
> > +EXPORT_SYMBOL_GPL(madera_destroy_bus_error_irq);
> 
> We use free rather than destroy normally?
> 

Yup will fix that.

> > +static const char * const madera_dfc_width_text[MADERA_DFC_WIDTH_ENUM_SIZE] = {
> > +	"8bit", "16bit", "20bit", "24bit", "32bit",
> > +};
> 
> Spaces might make these more readable.
> 

No problem to fix that too.

> > +static void madera_sleep(unsigned int delay)
> > +{
> > +	if (delay < 20) {
> > +		delay *= 1000;
> > +		usleep_range(delay, delay + 500);
> > +	} else {
> > +		msleep(delay);
> > +	}
> > +}
> 
> This feels like it might make sense as a helper function as well - I
> could've sworn there was one already but I can't immediately find it.

I do vaguely remember some discussion that it was preferred
that people had to think about which delay function to call,
rather than having a generic helper. TBF I could probably
just switch it all to use msleep, for the most part this is
a quite small optimisation of path bring up (although it was
asked for by one of our customers).

Thanks,
Charles

On Fri, May 24, 2019 at 04:24:10PM +0100, Richard Fitzgerald wrote:
> On 24/05/19 16:21, Richard Fitzgerald wrote:
> > On 24/05/19 15:56, Mark Brown wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.  You've also got some replies inserted at
the wrong quote level.

> > > So we read the register a few times then add a few hundred us of delay
> > > after?  Surely that delay is going to be negligable compared to the time
> > > spent on I/O?

> > The register reads are to create clock cycles in the silicon, not to generate delay.

> Sorry, just re-read your comment and realized I'd misread it. It's a hardware requirement
> that after generating the internal clocks there must be a delay. I.e. we require a combination
> of a guaranteed number of SYSCLKs followed by a guaranteed minimum delay.

OK, the comment could use a bit of clarification then to say that reads
are explicitly required as opposed to being purely to generate a delay.

> > > This will delay both before and after every power up and power down.
> > > Are you sure that makes sense?

> > I think that's correct but we can re-check with hardware people. It's not just a delay,
> > it needs to ensure there are always a certain number of SYSCLK cycles in the hardware to
> > avoid leaving certain state machines in limbo.

That sounds like you might want both _POST_PMU and _POST_PMD but do you
really need the _PREs as well?

On Sun, May 26, 2019 at 01:18:46PM +0100, Mark Brown wrote:
> On Fri, May 24, 2019 at 04:24:10PM +0100, Richard Fitzgerald wrote:
> > On 24/05/19 16:21, Richard Fitzgerald wrote:
> > > On 24/05/19 15:56, Mark Brown wrote:
> > > > This will delay both before and after every power up and power down.
> > > > Are you sure that makes sense?
> 
> > > I think that's correct but we can re-check with hardware people.
> > > It's not just a delay, it needs to ensure there are always a
> > > certain number of SYSCLK cycles in the hardware to avoid leaving
> > > certain state machines in limbo.
> 
> That sounds like you might want both _POST_PMU and _POST_PMD but do you
> really need the _PREs as well?

Yeah the requirement from the hardware guys was that we needed a
guard band between changing the SYSCLK state and other things
happening. So it has to happen both before and after the state of
SYSCLK changes. It is certainly far from ideal but it is what it
is.

Thanks,
Charles