Re: [PATCH 1/3] ASoC: AMD: Make ACP->SYSMEM DMA non circular
On 8/2/2018 3:26 PM, Mark Brown wrote: > On Thu, Aug 02, 2018 at 12:11:54PM +0530, Akshu Agrawal wrote: >> In capture case we don't want ACP to SYSMEM dma >> to be circular. This is because if an in place DSP >> filter is applied to captured output then circular DMA >> can overwrite the filter value with stale data. > > Isn't that just a problem with userspace not giving itself enough buffer > to get the in place processing done before we loop round again. This > will simply change the type of error, it won't actually fix anything > AFAICT but perhaps I'm missing something. > Sorry for the delayed reply. Will try and explain more on whats happening: We have 2 DMAs, one being I2S->ACP(Audio Co-Processor memory) and the second being ACP->SYSMEM. When ACP->SYSMEM is circular then this DMA is faster and keeps updating from ACP memory. This leaves no time for user space to do any kind of in place post processing. This design works fine if the user reads the data from SYSMEM at capture rate (that is the rate at which I2S->ACP transfer happens). But, when an in place post processing happens on SYSMEM then the data gets overwritten. Sending and updated series with changes in 2/3 and 3/3 patch. Thanks, Akshu
Re: [v2, 2/2] ASoC: AMD: Configure channel 1 or channel 0 for capture
On 6/20/2018 7:32 PM, Mark Brown wrote: > On Thu, Jun 07, 2018 at 02:48:44PM +0800, Akshu Agrawal wrote: > >> This patch is dependent on ASoC: AMD: Change codec to channel link as per >> hardware redesign >> https://patchwork.kernel.org/patch/10388099/ > > Can you please send a patch series with all the pending changes for this > driver? It's getting really difficult to track which of the patches > you've sent fit together and what's currently pending as there's often > quite a few different AMD patch serieses in flight with interdependencies. > Sure, Mark. There are just 2 patches remaining. Will send them as a series. Thanks, Akshu
Re: [PATCH] clk: x86: Set default parent to 48Mhz
On 8/29/2018 3:59 AM, Stephen Boyd wrote: > Quoting Akshu Agrawal (2018-08-20 23:51:57) >> System clk provided in ST soc can be set to: >> 48Mhz, non-spread >> 25Mhz, spread >> To get accurate rate, we need it to set it at non-spread >> option which is 48Mhz. >> >> Signed-off-by: Akshu Agrawal > > Does this need to go to 4.18 stable trees? I don't see a fixes tag so > I'm trying to understand merge priority of this patch. > Yes, and its a fix, as it fixes the in accuracy in the bclk which is derived from this clk. Thanks, Akshu
Re: linux-next: build warning after merge of the sound-asoc tree
On 7/26/2018 7:49 AM, Stephen Rothwell wrote:
> Hi all,
>
> After merging the sound-asoc tree, today's linux-next build (x86_64
> allmodconfig) produced this warning:
>
> sound/soc/amd/acp-da7219-max98357a.c: In function 'cz_probe':
> sound/soc/amd/acp-da7219-max98357a.c:367:3: warning: 'ret' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
>dev_err(&pdev->dev, "Failed to register regulator: %d\n",
>^
> ret);
>
>
> Introduced by commit
>
> 7b5317aa809f ("ASoC: AMD: Add a fix voltage regulator for DA7219 and
> ADAU7002")
>
> This is a real bug.
>
Posted the fix for the above warning message:
https://patchwork.kernel.org/patch/10545235/
Thanks,
Akshu
Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function
On 7/27/2018 8:39 PM, Pierre-Louis Bossart wrote:
> On 7/27/18 5:13 AM, Akshu Agrawal wrote:
>> There are cases where a pointer function populates
>> runtime->delay, such as:
>> ./sound/pci/hda/hda_controller.c
>> ./sound/soc/intel/atom/sst-mfld-platform-pcm.c
>>
>> Also, in some cases cpu dai used is generic and the pcm
>> driver needs to set delay.
>>
>> This delay was getting lost and was overwritten by delays
>> from codec or cpu dai delay function if exposed.
>
> Humm, yes the runtime->delay set in the .pointer function would be lost
> without this change, but the delay would still be provided in the
> followup call to .delay.
> With your change, the same delay will be accounted for twice?
>
It will not be accounted twice because no driver which is setting
runtime->delay is defining .delay op for cpu_dai. Vice versa is also
true, the drivers which define .delay for cpu_dai don't set
runtime->delay. And I think this is expected from drivers else it would
be a bug from their side.
.delay for codec_dai anyway is different and has to be accounted for.
Thanks,
Akshu
>>
>> Signed-off-by: Akshu Agrawal
>> ---
>> sound/soc/soc-pcm.c | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
>> index 98be04b..b1a2bc2 100644
>> --- a/sound/soc/soc-pcm.c
>> +++ b/sound/soc/soc-pcm.c
>> @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct
>> snd_pcm_substream *substream)
>> snd_pcm_sframes_t codec_delay = 0;
>> int i;
>>
>> +/* clearing the previous delay */
>> +runtime->delay = 0;
>> +
>> for_each_rtdcom(rtd, rtdcom) {
>> component = rtdcom->component;
>>
>> @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct
>> snd_pcm_substream *substream)
>> }
>> delay += codec_delay;
>>
>> -runtime->delay = delay;
>> +runtime->delay += delay;
>>
>> return offset;
>> }
>>
>
Re: [PATCH] ASoC: AMD: Add a fix voltage regulator for DA7219 and ADAU7002
On 7/20/2018 5:48 PM, Mark Brown wrote:
> On Fri, Jul 20, 2018 at 02:38:11PM +0800, Akshu Agrawal wrote:
>
>> static int cz_probe(struct platform_device *pdev)
>> {
>> int ret;
>> struct snd_soc_card *card;
>> struct acp_platform_info *machine;
>> +static bool regulators_registered;
>> +
>> +if (!regulators_registered) {
>> +ret = platform_device_register(&acp_da7219_regulator);
>> +if (ret) {
>> +dev_err(&pdev->dev, "Failed to register regulator:
>> %d\n",
>> +ret);
>> +return ret;
>> +}
>> +regulators_registered = true;
>> +}
>
> You should be unregistering the regulator in your remove function, not
> doing this hack here. I'd also expect to see the card made the parent
> of the device that gets registered.
>
This approach shows inconsistencies and in some boot cycles da7219 fails
to get regulator. Form the logs (below) it shows time gap between the
time we call “platform_device_register(&acp_da7219_regulator);” and when
the regulator actually gets registered.
[ 12.594237] regulator registered
**print after calling “platform_device_register(&acp_da7219_regulator);”
...
[ 13.583689] da7219 i2c-DLGS7219:00: i2c-DLGS7219:00 supply VDD not
found, using dummy regulator
[ 13.593818] da7219 i2c-DLGS7219:00: i2c-DLGS7219:00 supply VDDMIC not
found, using dummy regulator
[ 13.603242] da7219 i2c-DLGS7219:00: i2c-DLGS7219:00 supply VDDIO not
found, using dummy regulator
[ 13.612626] da7219 i2c-DLGS7219:00: Invalid VDDIO voltage
**Above DA7219 gets probed and does not find the regulator**
...
[ 13.750894] reg_fixed_voltage_probe: Supply -> reg-fixed-1.8V
[ 13.766746] reg-fixed-1.8V supplying 180uV
**Regulator actually gets registered**
Alternate and consistent approach to this is pushed by Daniel here:
https://patchwork.kernel.org/patch/10539485/
Thanks,
Akshu
Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function
On 7/30/2018 9:20 PM, Mark Brown wrote: > On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: > >> That said, if delay callback of CPU dai provides the additional delay, >> the patch does correct thing. OTOH, if CPU dai provides the base >> delay instead, we need to clarify that it's rather a must; the delay >> calculation in pointer callback becomes bogus in this scenario. > > Part of the theory here is that every component might have a delay > independently of the rest and we need to add them all together to figure > out what the system as a whole will see. Personally I'd rather just > have everything use a callack consistently to avoid confusion. > For consistency we can add a delay callback in snd_pcm_ops and modify the drivers which directly assigning runtime->delay to use the callback. Apart from the 2 drivers mentioned in commit message I also found sound/usb to be doing the same and its delay getting lost. Thanks, Akshu
Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function
On 7/31/2018 11:00 AM, Takashi Iwai wrote: > On Tue, 31 Jul 2018 03:25:06 +0200, > Agrawal, Akshu wrote: >> >> >> >> On 7/30/2018 9:20 PM, Mark Brown wrote: >>> On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: >>> >>>> That said, if delay callback of CPU dai provides the additional delay, >>>> the patch does correct thing. OTOH, if CPU dai provides the base >>>> delay instead, we need to clarify that it's rather a must; the delay >>>> calculation in pointer callback becomes bogus in this scenario. >>> >>> Part of the theory here is that every component might have a delay >>> independently of the rest and we need to add them all together to figure >>> out what the system as a whole will see. Personally I'd rather just >>> have everything use a callack consistently to avoid confusion. >>> >> >> For consistency we can add a delay callback in snd_pcm_ops and modify >> the drivers which directly assigning runtime->delay to use the callback. > > No, ALSA PCM ops definition is fine. The delay calculation is > basically tied with the position, hence it has to be set together, and > that's the pointer callback. > > Judging from the call pattern, the current design of ASoC delay > callback implies that the return value is more or less constant, which > can be accumulated on top of the base value. So your patch is natural > from that POV. > > OTOH, if the CPU dai can really provide a dynamic value that is > strictly tied with pointer, CPU dai itself should provide the pointer > callback that covers both the pointer and the base delay, and it > should be used instead of component pointer callback. > Not sure if all cpu dai can provide the base delay and thus require component pointer callback for it. For example, in case of AMD, it uses designware cpu dai which is a common code. >> Apart from the 2 drivers mentioned in commit message I also found >> sound/usb to be doing the same and its delay getting lost. > > The USB driver hasn't been used in ASoC, no? > Don't know, was looking who all might have been impacted by this. Thanks, Akshu
Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function
On 7/31/2018 8:10 PM, Mark Brown wrote: > On Tue, Jul 31, 2018 at 03:56:52PM +0200, Takashi Iwai wrote: >> Mark Brown wrote: > >>> Yes. I'm saying that if the CPU DAI thinks it can figure out the base >>> delay something is confused. > >> Then basically Akshu's patch does the correct thing, I suppose. > > I think so. Could perhaps do with a little clarification though. > Ok Mark, I will submit a v2 which makes it more clear on the intent. Thanks, Akshu
Re: [PATCH] ASoC: AMD: Add a fix voltage regulator for DA7219 and ADAU7002
On 7/24/2018 10:44 PM, Mark Brown wrote: > On Mon, Jul 23, 2018 at 10:56:44AM +0530, Agrawal, Akshu wrote: > >> This approach shows inconsistencies and in some boot cycles da7219 fails >> to get regulator. Form the logs (below) it shows time gap between the >> time we call “platform_device_register(&acp_da7219_regulator);” and when >> the regulator actually gets registered. > > This hack isn't going to help with that AFAICT, you're still registering > a platform device and relying on it appearing in time? It just > registers less often. Yes, I agree and by "this approach" I also meant registering platform device and waiting for its probe to happen. Instead of adding a platform device to the reg-fixed-voltage, we can register a regulator and thus not wait for the probe to occur. Pushing a v2 with this change which has consistent behavior. Thanks, Akshu
[PATCH 1/2] ASoC: Fix function return type
Function returns -1 on error and the return type
should be int.
Signed-off-by: Akshu Agrawal
---
include/sound/soc.h | 2 +-
sound/soc/soc-io.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 8ec1de8..591d743 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1341,7 +1341,7 @@ static inline int snd_soc_component_cache_sync(
/* component IO */
int snd_soc_component_read(struct snd_soc_component *component,
unsigned int reg, unsigned int *val);
-unsigned int snd_soc_component_read32(struct snd_soc_component *component,
+int snd_soc_component_read32(struct snd_soc_component *component,
unsigned int reg);
int snd_soc_component_write(struct snd_soc_component *component,
unsigned int reg, unsigned int val);
diff --git a/sound/soc/soc-io.c b/sound/soc/soc-io.c
index 1ff9175..d08f5b1 100644
--- a/sound/soc/soc-io.c
+++ b/sound/soc/soc-io.c
@@ -38,7 +38,7 @@ int snd_soc_component_read(struct snd_soc_component
*component,
}
EXPORT_SYMBOL_GPL(snd_soc_component_read);
-unsigned int snd_soc_component_read32(struct snd_soc_component *component,
+int snd_soc_component_read32(struct snd_soc_component *component,
unsigned int reg)
{
unsigned int val;
--
1.9.1
[PATCH 2/2] ASoC: DA7219: Implement error check on reg read and write
Failed i2c transaction can lead to failure in reg read or write.
Need to have error check for each read write operation.
Signed-off-by: Akshu Agrawal
---
sound/soc/codecs/da7219.c | 323 +++---
sound/soc/codecs/da7219.h | 2 +-
2 files changed, 247 insertions(+), 78 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index e46e9f4..6757129 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -312,69 +312,103 @@ static int da7219_enum_locked_put(struct snd_kcontrol
*kcontrol,
}
/* ALC */
-static void da7219_alc_calib(struct snd_soc_component *component)
+static int da7219_alc_calib(struct snd_soc_component *component)
{
- u8 mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
+ int mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
+ int ret;
/* Save current state of mic control register */
mic_ctrl = snd_soc_component_read32(component, DA7219_MIC_1_CTRL);
+ if (mic_ctrl < 0)
+ return mic_ctrl;
/* Save current state of input mixer control register */
mixin_ctrl = snd_soc_component_read32(component, DA7219_MIXIN_L_CTRL);
+ if (mixin_ctrl < 0)
+ return mixin_ctrl;
/* Save current state of input ADC control register */
adc_ctrl = snd_soc_component_read32(component, DA7219_ADC_L_CTRL);
+ if (adc_ctrl < 0)
+ return adc_ctrl;
/* Enable then Mute MIC PGAs */
- snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
DA7219_MIC_1_AMP_EN_MASK,
+ ret = snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
+ DA7219_MIC_1_AMP_EN_MASK,
DA7219_MIC_1_AMP_EN_MASK);
- snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
+ if (ret < 0)
+ return ret;
+
+ ret = snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
DA7219_MIC_1_AMP_MUTE_EN_MASK,
DA7219_MIC_1_AMP_MUTE_EN_MASK);
+ if (ret < 0)
+ return ret;
/* Enable input mixers unmuted */
- snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
+ ret = snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
DA7219_MIXIN_L_AMP_EN_MASK |
DA7219_MIXIN_L_AMP_MUTE_EN_MASK,
DA7219_MIXIN_L_AMP_EN_MASK);
+ if (ret < 0)
+ return ret;
/* Enable input filters unmuted */
- snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
+ ret = snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
DA7219_ADC_L_MUTE_EN_MASK | DA7219_ADC_L_EN_MASK,
DA7219_ADC_L_EN_MASK);
+ if (ret < 0)
+ return ret;
/* Perform auto calibration */
- snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
+ ret = snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
DA7219_ALC_AUTO_CALIB_EN_MASK,
DA7219_ALC_AUTO_CALIB_EN_MASK);
+ if (ret < 0)
+ return ret;
do {
calib_ctrl = snd_soc_component_read32(component,
DA7219_ALC_CTRL1);
+ if (calib_ctrl < 0)
+ return calib_ctrl;
} while (calib_ctrl & DA7219_ALC_AUTO_CALIB_EN_MASK);
/* If auto calibration fails, disable DC offset, hybrid ALC */
if (calib_ctrl & DA7219_ALC_CALIB_OVERFLOW_MASK) {
dev_warn(component->dev,
"ALC auto calibration failed with overflow\n");
- snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
+ ret = snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
DA7219_ALC_OFFSET_EN_MASK |
DA7219_ALC_SYNC_MODE_MASK, 0);
+ if (ret < 0)
+ return ret;
} else {
/* Enable DC offset cancellation, hybrid mode */
- snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
+ ret = snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
DA7219_ALC_OFFSET_EN_MASK |
DA7219_ALC_SYNC_MODE_MASK,
DA7219_ALC_OFFSET_EN_MASK |
DA7219_ALC_SYNC_MODE_MASK);
+ if (ret < 0)
+ return ret;
}
/* Restore input filter control register to original state */
- snd_soc_component_write(component, DA7219_ADC_L_CTRL, adc_ctrl);
+ ret = snd_soc_component_write(component, DA7219_ADC_L_CTRL, adc_ctrl);
+ if (ret < 0)
+ return ret;
/* Restore input mixer control registers to o
Re: [PATCH 2/2] ASoC: DA7219: Implement error check on reg read and write
On 12/5/2018 2:46 AM, Adam Thomson wrote:
> On 04 December 2018 18:36, Akshu Agrawal wrote:
>
>> Failed i2c transaction can lead to failure in reg read or write.
>> Need to have error check for each read write operation.
>>
>
> I'm not really clear what this gains you. If I2C is broken this won't solve
> anything, and you have far deeper problems. Seems to me like a lot of code
> which
> adds almost nothing.
>
I agree Adam that if I2C is broken, system is in bad state. But so is
the case with most of the error situations. Error handling is anyway needed.
We have seen case where read32 returned -1, unsigned int cast and the
write back of the error value ends up in updating the register with 0xff.
Need to avoid scenarios like this.
Thanks,
Akshu
>> Signed-off-by: Akshu Agrawal
>> ---
>> sound/soc/codecs/da7219.c | 323 +++-
>> --
>> sound/soc/codecs/da7219.h | 2 +-
>> 2 files changed, 247 insertions(+), 78 deletions(-)
>>
>> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index
>> e46e9f4..6757129 100644
>> --- a/sound/soc/codecs/da7219.c
>> +++ b/sound/soc/codecs/da7219.c
>> @@ -312,69 +312,103 @@ static int da7219_enum_locked_put(struct
>> snd_kcontrol *kcontrol, }
>>
>> /* ALC */
>> -static void da7219_alc_calib(struct snd_soc_component *component)
>> +static int da7219_alc_calib(struct snd_soc_component *component)
>> {
>> -u8 mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
>> +int mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
>> +int ret;
>>
>> /* Save current state of mic control register */
>> mic_ctrl = snd_soc_component_read32(component,
>> DA7219_MIC_1_CTRL);
>> +if (mic_ctrl < 0)
>> +return mic_ctrl;
>>
>> /* Save current state of input mixer control register */
>> mixin_ctrl = snd_soc_component_read32(component,
>> DA7219_MIXIN_L_CTRL);
>> +if (mixin_ctrl < 0)
>> +return mixin_ctrl;
>>
>> /* Save current state of input ADC control register */
>> adc_ctrl = snd_soc_component_read32(component,
>> DA7219_ADC_L_CTRL);
>> +if (adc_ctrl < 0)
>> +return adc_ctrl;
>>
>> /* Enable then Mute MIC PGAs */
>> -snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
>> DA7219_MIC_1_AMP_EN_MASK,
>> +ret = snd_soc_component_update_bits(component,
>> DA7219_MIC_1_CTRL,
>> +DA7219_MIC_1_AMP_EN_MASK,
>> DA7219_MIC_1_AMP_EN_MASK);
>> -snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
>> +if (ret < 0)
>> +return ret;
>> +
>> +ret = snd_soc_component_update_bits(component,
>> DA7219_MIC_1_CTRL,
>> DA7219_MIC_1_AMP_MUTE_EN_MASK,
>> DA7219_MIC_1_AMP_MUTE_EN_MASK);
>> +if (ret < 0)
>> +return ret;
>>
>> /* Enable input mixers unmuted */
>> -snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
>> +ret = snd_soc_component_update_bits(component,
>> DA7219_MIXIN_L_CTRL,
>> DA7219_MIXIN_L_AMP_EN_MASK |
>> DA7219_MIXIN_L_AMP_MUTE_EN_MASK,
>> DA7219_MIXIN_L_AMP_EN_MASK);
>> +if (ret < 0)
>> +return ret;
>>
>> /* Enable input filters unmuted */
>> -snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
>> +ret = snd_soc_component_update_bits(component,
>> DA7219_ADC_L_CTRL,
>> DA7219_ADC_L_MUTE_EN_MASK |
>> DA7219_ADC_L_EN_MASK,
>> DA7219_ADC_L_EN_MASK);
>> +if (ret < 0)
>> +return ret;
>>
>> /* Perform auto calibration */
>> -snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
>> +ret = snd_soc_component_update_bits(component,
>> DA7219_ALC_CTRL1,
>> DA7219_ALC_AUTO_CALIB_EN_MASK,
>> DA7219_ALC_AUTO_CALIB_EN_MASK);
>> +if (ret < 0)
>> +return ret;
>> do {
>> calib_ctrl = snd_soc_component_read32(component,
>> DA7219_ALC_CTRL1);
>> +if (calib_ctrl < 0)
>> +return calib_ctrl;
>> } while (calib_ctrl & DA7219_ALC_AUTO_CALIB_EN_MASK);
>>
>> /* If auto calibration fails, disable DC offset, hybrid ALC */
>> if (calib_ctrl & DA7219_ALC_CALIB_OVERFLOW_MASK) {
>> dev_warn(component->dev,
>> "ALC auto calibration failed with overflow\n");
>> -snd_soc_component_update_bits(component,
>> DA7219_ALC_CTRL1,
>> +ret = snd_soc_component_update_bits(component,
>> DA7219_ALC_CTRL1,
>> DA7219_ALC_OFFSET_EN_MASK |
>> DA7219_ALC_SYNC_MODE_MASK, 0);
>> +if (ret < 0)
>> +return ret;
>> } else {
>> /* Enable DC offset cancellation, hybrid mode */
>> -snd_soc_component_update_bits(component,
>> DA7219_ALC_CTR
[PATCH v2] ASoC: DA7219: Implement error check on reg read and write
Failed i2c transaction can lead to failure in reg read or write.
Need to have error check for each read write operation.
Signed-off-by: Akshu Agrawal
---
v2: replaced snd_soc_component_read32 by snd_soc_component_read
Since, snd_soc_component_read32 implementation has error, in coming patches
we can remove snd_soc_component_read32 from soc-io and replace all codecs by
snd_soc_component_read
sound/soc/codecs/da7219.c | 349 +++---
sound/soc/codecs/da7219.h | 2 +-
2 files changed, 265 insertions(+), 86 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index e46e9f4..c65e734 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -312,69 +312,105 @@ static int da7219_enum_locked_put(struct snd_kcontrol
*kcontrol,
}
/* ALC */
-static void da7219_alc_calib(struct snd_soc_component *component)
+static int da7219_alc_calib(struct snd_soc_component *component)
{
- u8 mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
+ unsigned int mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
+ int ret;
/* Save current state of mic control register */
- mic_ctrl = snd_soc_component_read32(component, DA7219_MIC_1_CTRL);
+ ret = snd_soc_component_read(component, DA7219_MIC_1_CTRL, &mic_ctrl);
+ if (ret)
+ return ret;
/* Save current state of input mixer control register */
- mixin_ctrl = snd_soc_component_read32(component, DA7219_MIXIN_L_CTRL);
+ ret = snd_soc_component_read(component, DA7219_MIXIN_L_CTRL,
+ &mixin_ctrl);
+ if (ret)
+ return ret;
/* Save current state of input ADC control register */
- adc_ctrl = snd_soc_component_read32(component, DA7219_ADC_L_CTRL);
+ ret = snd_soc_component_read(component, DA7219_ADC_L_CTRL, &adc_ctrl);
+ if (ret)
+ return ret;
/* Enable then Mute MIC PGAs */
- snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
DA7219_MIC_1_AMP_EN_MASK,
+ ret = snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
+ DA7219_MIC_1_AMP_EN_MASK,
DA7219_MIC_1_AMP_EN_MASK);
- snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
+ if (ret < 0)
+ return ret;
+
+ ret = snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
DA7219_MIC_1_AMP_MUTE_EN_MASK,
DA7219_MIC_1_AMP_MUTE_EN_MASK);
+ if (ret < 0)
+ return ret;
/* Enable input mixers unmuted */
- snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
+ ret = snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
DA7219_MIXIN_L_AMP_EN_MASK |
DA7219_MIXIN_L_AMP_MUTE_EN_MASK,
DA7219_MIXIN_L_AMP_EN_MASK);
+ if (ret < 0)
+ return ret;
/* Enable input filters unmuted */
- snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
+ ret = snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
DA7219_ADC_L_MUTE_EN_MASK | DA7219_ADC_L_EN_MASK,
DA7219_ADC_L_EN_MASK);
+ if (ret < 0)
+ return ret;
/* Perform auto calibration */
- snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
+ ret = snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
DA7219_ALC_AUTO_CALIB_EN_MASK,
DA7219_ALC_AUTO_CALIB_EN_MASK);
+ if (ret < 0)
+ return ret;
do {
- calib_ctrl = snd_soc_component_read32(component,
DA7219_ALC_CTRL1);
+ ret = snd_soc_component_read(component, DA7219_ALC_CTRL1,
+ &calib_ctrl);
+ if (ret)
+ return ret;
} while (calib_ctrl & DA7219_ALC_AUTO_CALIB_EN_MASK);
/* If auto calibration fails, disable DC offset, hybrid ALC */
if (calib_ctrl & DA7219_ALC_CALIB_OVERFLOW_MASK) {
dev_warn(component->dev,
"ALC auto calibration failed with overflow\n");
- snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
+ ret = snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
DA7219_ALC_OFFSET_EN_MASK |
DA7219_ALC_SYNC_MODE_MASK, 0);
+ if (ret < 0)
+ return ret;
} else {
/* Enable DC offset cancellation, hybrid mode */
- snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
+ ret = snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
DA7219_ALC_OFFSET_EN_MASK |
Re: [PATCH] drm/amdgpu/acp: Fix slab-out-of-bounds in mfd_add_device in acp_hw_init
Was this patch ever picked up? I can't find it in agd5f/linux.
>>>
>>>
>>> It wasn't applied. I don't see 51f7415039d4 ("drm/amd/amdgpu:
>>> creating two I2S instances for stoney/cz") upstream yet either.
>>> Daniel, Vijendar, which ones do you want applied? Can you send me the
>>> patches?
>>>
>>> Alex
>>
>>
>> Hi Alex,
>>
>> "drm/amd/amdgpu: creating two I2S instances for stoney/cz" patch exists in
>> drm-next branch. Please pick the patch .
>
> So just that one? I seem to recall there being later revisions of
> that patch that you reworked after applying the original version.
> Also that patch was originally part of a larger series. Are those
> changes required too?
>
> Alex
>
Hi Alex,
In agd5f/linux, branch "amd-staging-drm-next",
506f7d1 drm/amd/amdgpu: creating two I2S instances for stoney/cz
patch is present.
This patch is the correct version and there aren't any other changes
required with it.
Only Dan's, this mail's patch is currently missing form the tree.
Thanks,
Akshu
Re: [PATCH v3] ASoC: AMD: Fix race condition between register access
On 11/5/2018 4:45 PM, Mark Brown wrote: > On Wed, Oct 31, 2018 at 09:24:10PM +0000, Agrawal, Akshu wrote: > >> +/* Lock to protect access to registers */ >> +static DEFINE_SPINLOCK(lock); >> + > > Why is this a global variable and not a part of the driver structure? > Is there some interaction between multiple instances? > Yes, this lock is used to protect registers which are common to multiple instances and can cause issue in cases such as simultaneous playback and capture. Thanks, Akshu
Re: [PATCH] ASoC: AMD: Add delay before starting to capture
On 1/4/2019 5:57 PM, Mark Brown wrote: > On Thu, Jan 03, 2019 at 10:18:13AM +0000, Agrawal, Akshu wrote: >> On capture through dmic we observe a glitch at the start of record. >> This is because we start capturing even before dmic is ready to send >> out data. The glitch seen last for ~20msec. >> > >> +/* >> + * On some platforms, it takes ~20 msec for PDM DMICs and ADAU7002 >> + * to settle and start producing proper audio data. >> + */ >> +msleep(ADAU7002_DELAY_MS); > > If the delay is required for external components to start up the delay > should be going in the drivers for those components rather than in the > driver for the CPU side, that way other systems using those components > get the benefit and non-affected boards don't pay the cost. There's > already some support for this in the DMIC driver at least. > Agreed, we can use the similar to wakeup_delay in dmic driver in our codec driver. Will post the patch on same lines. Thanks, Akshu
[PATCH] ASoC: ADAU7002: Add optional delay before start of capture
On capture through some of dmic we observe a glitch at the
start of record. This is because we start capturing even before
dmic is ready to send out data.
The optional delay will be applied after enabling the mic.
Signed-off-by: Akshu Agrawal
---
sound/soc/codecs/adau7002.c | 45 +
1 file changed, 45 insertions(+)
diff --git a/sound/soc/codecs/adau7002.c b/sound/soc/codecs/adau7002.c
index fdff868..a8deb37 100644
--- a/sound/soc/codecs/adau7002.c
+++ b/sound/soc/codecs/adau7002.c
@@ -8,6 +8,7 @@
*/
#include
+#include
#include
#include
#include
@@ -15,12 +16,55 @@
#include
+struct adau7002_priv {
+ int wakeup_delay;
+};
+
+static int adau7002_aif_event(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 adau7002_priv *adau7002 =
+ snd_soc_component_get_drvdata(component);
+
+ switch (event) {
+ case SND_SOC_DAPM_POST_PMU:
+ if (adau7002->wakeup_delay)
+ msleep(adau7002->wakeup_delay);
+ break;
+ }
+
+ return 0;
+}
+
+static int adau7002_component_probe(struct snd_soc_component *component)
+{
+ struct adau7002_priv *adau7002;
+
+ adau7002 = devm_kzalloc(component->dev, sizeof(*adau7002),
+ GFP_KERNEL);
+ if (!adau7002)
+ return -ENOMEM;
+
+ device_property_read_u32(component->dev, "wakeup-delay-ms",
+&adau7002->wakeup_delay);
+
+ snd_soc_component_set_drvdata(component, adau7002);
+
+ return 0;
+}
+
static const struct snd_soc_dapm_widget adau7002_widgets[] = {
+ SND_SOC_DAPM_AIF_OUT_E("ADAU AIF", "Capture", 0,
+ SND_SOC_NOPM, 0, 0, adau7002_aif_event,
+ SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
SND_SOC_DAPM_INPUT("PDM_DAT"),
SND_SOC_DAPM_REGULATOR_SUPPLY("IOVDD", 0, 0),
};
static const struct snd_soc_dapm_route adau7002_routes[] = {
+ { "ADAU AIF", NULL, "PDM_DAT"},
{ "Capture", NULL, "PDM_DAT" },
{ "Capture", NULL, "IOVDD" },
};
@@ -40,6 +84,7 @@
};
static const struct snd_soc_component_driver adau7002_component_driver = {
+ .probe = adau7002_component_probe,
.dapm_widgets = adau7002_widgets,
.num_dapm_widgets = ARRAY_SIZE(adau7002_widgets),
.dapm_routes= adau7002_routes,
--
1.9.1
[PATCH] ASoC: AMD: Add delay before starting to capture
On capture through dmic we observe a glitch at the start of record. This is because we start capturing even before dmic is ready to send out data. The glitch seen last for ~20msec. Signed-off-by: Akshu Agrawal Signed-off-by: Daniel Kurtz --- sound/soc/amd/acp-da7219-max98357a.c | 28 ++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/sound/soc/amd/acp-da7219-max98357a.c b/sound/soc/amd/acp-da7219-max98357a.c index a5daad9..72b1cf4 100644 --- a/sound/soc/amd/acp-da7219-max98357a.c +++ b/sound/soc/amd/acp-da7219-max98357a.c @@ -44,6 +44,8 @@ #define CZ_PLAT_CLK 4800 #define DUAL_CHANNEL 2 +#define ADAU7002_DELAY_MS 20 + static struct snd_soc_jack cz_jack; static struct clk *da7219_dai_clk; @@ -213,6 +215,7 @@ static int cz_dmic0_startup(struct snd_pcm_substream *substream) struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_card *card = rtd->card; struct acp_platform_info *machine = snd_soc_card_get_drvdata(card); + int ret; /* * On this platform for PCM device we support stereo @@ -225,7 +228,17 @@ static int cz_dmic0_startup(struct snd_pcm_substream *substream) &constraints_rates); machine->cap_i2s_instance = I2S_BT_INSTANCE; - return da7219_clk_enable(substream); + ret = da7219_clk_enable(substream); + if (ret) + return ret; + /* +* On some platforms, it takes ~20 msec for PDM DMICs and ADAU7002 +* to settle and start producing proper audio data. +*/ + msleep(ADAU7002_DELAY_MS); + /* Delay in frames for 48Khz, 16bit, 2 channel */ + runtime->delay = ADAU7002_DELAY_MS * 48; + return 0; } static int cz_dmic1_startup(struct snd_pcm_substream *substream) @@ -234,6 +247,7 @@ static int cz_dmic1_startup(struct snd_pcm_substream *substream) struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_card *card = rtd->card; struct acp_platform_info *machine = snd_soc_card_get_drvdata(card); + int ret; /* * On this platform for PCM device we support stereo @@ -247,7 +261,17 @@ static int cz_dmic1_startup(struct snd_pcm_substream *substream) machine->cap_i2s_instance = I2S_SP_INSTANCE; machine->capture_channel = CAP_CHANNEL0; - return da7219_clk_enable(substream); + ret = da7219_clk_enable(substream); + if (ret) + return ret; + /* +* On some platforms, it takes ~20 msec for PDM DMICs and ADAU7002 +* to settle and start producing proper audio data. +*/ + msleep(ADAU7002_DELAY_MS); + /* Delay in frames for 48Khz, 16bit, 2 channel */ + runtime->delay = ADAU7002_DELAY_MS * 48; + return 0; } static void cz_dmic_shutdown(struct snd_pcm_substream *substream) -- 1.9.1
[PATCH] ASoC: AMD: Fix race condition between register access
During simultaneous running of playback and capture, we
got hit by incorrect value write on common register. This was due
to race condition between 2 streams.
Fixing this by locking the common register access.
Signed-off-by: Akshu Agrawal
---
sound/soc/amd/acp-pcm-dma.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 0ac4b5b..993a7db 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -121,6 +121,9 @@
.periods_max = CAPTURE_MAX_NUM_PERIODS,
};
+/* Lock to protect access to registers */
+static DEFINE_SPINLOCK(lock);
+
static u32 acp_reg_read(void __iomem *acp_mmio, u32 reg)
{
return readl(acp_mmio + (reg * 4));
@@ -168,9 +171,12 @@ static void config_dma_descriptor_in_sram(void __iomem
*acp_mmio,
acp_dma_dscr_transfer_t *descr_info)
{
u32 sram_offset;
+ unsigned long flags;
sram_offset = (descr_idx * sizeof(acp_dma_dscr_transfer_t));
+ spin_lock_irqsave(&lock, flags);
+
/* program the source base address. */
acp_reg_write(sram_offset, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
acp_reg_write(descr_info->src, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
@@ -181,6 +187,8 @@ static void config_dma_descriptor_in_sram(void __iomem
*acp_mmio,
/* program the number of bytes to be transferred for this descriptor. */
acp_reg_write(sram_offset + 8, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
acp_reg_write(descr_info->xfer_val, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
+
+ spin_unlock_irqrestore(&lock, flags);
}
static void pre_config_reset(void __iomem *acp_mmio, u16 ch_num)
@@ -309,8 +317,12 @@ static void acp_pte_config(void __iomem *acp_mmio, struct
page *pg,
u32 low;
u32 high;
u32 offset;
+ unsigned long flags;
offset = ACP_DAGB_GRP_SRBM_SRAM_BASE_OFFSET + (pte_offset * 8);
+
+ spin_lock_irqsave(&lock, flags);
+
for (page_idx = 0; page_idx < (num_of_pages); page_idx++) {
/* Load the low address of page int ACP SRAM through SRBM */
acp_reg_write((offset + (page_idx * 8)),
@@ -333,6 +345,8 @@ static void acp_pte_config(void __iomem *acp_mmio, struct
page *pg,
/* Move to next physically contiguos page */
pg++;
}
+
+ spin_unlock_irqrestore(&lock, flags);
}
static void config_acp_dma(void __iomem *acp_mmio,
@@ -367,6 +381,7 @@ static void acp_dma_cap_channel_enable(void __iomem
*acp_mmio,
u16 cap_channel)
{
u32 val, ch_reg, imr_reg, res_reg;
+ unsigned long flags;
switch (cap_channel) {
case CAP_CHANNEL1:
@@ -381,6 +396,8 @@ static void acp_dma_cap_channel_enable(void __iomem
*acp_mmio,
imr_reg = mmACP_I2SMICSP_IMR0;
break;
}
+ spin_lock_irqsave(&lock, flags);
+
val = acp_reg_read(acp_mmio,
mmACP_I2S_16BIT_RESOLUTION_EN);
if (val & ACP_I2S_MIC_16BIT_RESOLUTION_EN) {
@@ -393,12 +410,15 @@ static void acp_dma_cap_channel_enable(void __iomem
*acp_mmio,
val &= ~ACP_I2SMICSP_IMR1__I2SMICSP_RXFOM_MASK;
acp_reg_write(val, acp_mmio, imr_reg);
acp_reg_write(0x1, acp_mmio, ch_reg);
+
+ spin_unlock_irqrestore(&lock, flags);
}
static void acp_dma_cap_channel_disable(void __iomem *acp_mmio,
u16 cap_channel)
{
u32 val, ch_reg, imr_reg;
+ unsigned long flags;
switch (cap_channel) {
case CAP_CHANNEL1:
@@ -411,11 +431,15 @@ static void acp_dma_cap_channel_disable(void __iomem
*acp_mmio,
ch_reg = mmACP_I2SMICSP_RER0;
break;
}
+ spin_lock_irqsave(&lock, flags);
+
val = acp_reg_read(acp_mmio, imr_reg);
val |= ACP_I2SMICSP_IMR1__I2SMICSP_RXDAM_MASK;
val |= ACP_I2SMICSP_IMR1__I2SMICSP_RXFOM_MASK;
acp_reg_write(val, acp_mmio, imr_reg);
acp_reg_write(0x0, acp_mmio, ch_reg);
+
+ spin_unlock_irqrestore(&lock, flags);
}
/* Start a given DMA channel transfer */
@@ -847,6 +871,7 @@ static int acp_dma_hw_params(struct snd_pcm_substream
*substream,
int status;
uint64_t size;
u32 val = 0;
+ unsigned long flags;
struct page *pg;
struct snd_pcm_runtime *runtime;
struct audio_substream_data *rtd;
@@ -870,6 +895,8 @@ static int acp_dma_hw_params(struct snd_pcm_substream
*substream,
}
}
if (adata->asic_type == CHIP_STONEY) {
+ spin_lock_irqsave(&lock, flags);
+
val = acp_reg_read(adata->acp_mmio,
mmACP_I2S_16BIT_RESOLUTION_EN);
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
@@ -893,6 +920,8 @@ static int acp_dma_hw_params(struct
Re: [PATCH] ASoC: AMD: Fix race condition between register access
On 10/30/2018 7:02 AM, Daniel Kurtz wrote:
> Hi Akshu,
>
>
> On Mon, Oct 29, 2018 at 1:39 AM Agrawal, Akshu wrote:
>>
>> During simultaneous running of playback and capture, we
>> got hit by incorrect value write on common register. This was due
>> to race condition between 2 streams.
>> Fixing this by locking the common register access.
>
> Nice catch! It looks looks like one of the operations you are trying
> to make atomic is that two step Addr + Data register update.
> If so, then I recommend refactoring a bit, and just doing that locked
> 2-step-access in its own helper function, like this:
>
> static void acp_reg_write_srbm_targ(void __iomem *acp_mmio, u32
> addr, u32 data)
> {
> unsigned long flags;
>
> spin_lock_irqsave(&lock, flags);
> acp_reg_write(addr, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
> acp_reg_write(data, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
> spin_unlock_irqrestore(&lock, flags);
> }
>
> And similarly, you can add 2 more locking helpers, one for modifying
> the imr/ch and another for mmACP_I2S_16BIT_RESOLUTION_EN.
>
Accepted, will add the helper functions and push another patch.
Also, I noticed that locking is not needed in imr and ch reg as these
are separate registers for each channel, so will remove that.
Thanks,
Akshu
>>
>> Signed-off-by: Akshu Agrawal
>> ---
>> sound/soc/amd/acp-pcm-dma.c | 29 +
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
>> index 0ac4b5b..993a7db 100644
>> --- a/sound/soc/amd/acp-pcm-dma.c
>> +++ b/sound/soc/amd/acp-pcm-dma.c
>> @@ -121,6 +121,9 @@
>> .periods_max = CAPTURE_MAX_NUM_PERIODS,
>> };
>>
>> +/* Lock to protect access to registers */
>> +static DEFINE_SPINLOCK(lock);
>> +
>> static u32 acp_reg_read(void __iomem *acp_mmio, u32 reg)
>> {
>> return readl(acp_mmio + (reg * 4));
>> @@ -168,9 +171,12 @@ static void config_dma_descriptor_in_sram(void __iomem
>> *acp_mmio,
>> acp_dma_dscr_transfer_t
>> *descr_info)
>> {
>> u32 sram_offset;
>> + unsigned long flags;
>>
>> sram_offset = (descr_idx * sizeof(acp_dma_dscr_transfer_t));
>>
>> + spin_lock_irqsave(&lock, flags);
>> +
>> /* program the source base address. */
>> acp_reg_write(sram_offset, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
>> acp_reg_write(descr_info->src, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
>> @@ -181,6 +187,8 @@ static void config_dma_descriptor_in_sram(void __iomem
>> *acp_mmio,
>> /* program the number of bytes to be transferred for this
>> descriptor. */
>> acp_reg_write(sram_offset + 8, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
>> acp_reg_write(descr_info->xfer_val, acp_mmio,
>> mmACP_SRBM_Targ_Idx_Data);
>> +
>> + spin_unlock_irqrestore(&lock, flags);
>> }
>>
>> static void pre_config_reset(void __iomem *acp_mmio, u16 ch_num)
>> @@ -309,8 +317,12 @@ static void acp_pte_config(void __iomem *acp_mmio,
>> struct page *pg,
>> u32 low;
>> u32 high;
>> u32 offset;
>> + unsigned long flags;
>>
>> offset = ACP_DAGB_GRP_SRBM_SRAM_BASE_OFFSET + (pte_offset * 8);
>> +
>> + spin_lock_irqsave(&lock, flags);
>> +
>> for (page_idx = 0; page_idx < (num_of_pages); page_idx++) {
>> /* Load the low address of page int ACP SRAM through SRBM */
>> acp_reg_write((offset + (page_idx * 8)),
>> @@ -333,6 +345,8 @@ static void acp_pte_config(void __iomem *acp_mmio,
>> struct page *pg,
>> /* Move to next physically contiguos page */
>> pg++;
>> }
>> +
>> + spin_unlock_irqrestore(&lock, flags);
>> }
>>
>> static void config_acp_dma(void __iomem *acp_mmio,
>> @@ -367,6 +381,7 @@ static void acp_dma_cap_channel_enable(void __iomem
>> *acp_mmio,
>>u16 cap_channel)
>> {
>> u32 val, ch_reg, imr_reg, res_reg;
>> + unsigned long flags;
>>
>> switch (cap_channel) {
>> case CAP_CHANNEL1:
>> @@ -381,6 +396,8 @@ static void acp_dma_cap_channel_enable(void __iomem
>> *acp_mmio,
>> imr_reg = mmACP_I2SMICSP_IMR0;
>> break;
>> }
>> + spin_lock_irqsave(&lock
[PATCH v2] ASoC: AMD: Fix race condition between register access
During simultaneous running of playback and capture, we
got hit by incorrect value write on common register. This was due
to race condition between 2 streams.
Fixing this by locking the common register access.
Signed-off-by: Akshu Agrawal
---
v2: Added 2 helper functions, removed locking in ch enable/disable as they
are separate registers.
sound/soc/amd/acp-pcm-dma.c | 63 ++---
1 file changed, 48 insertions(+), 15 deletions(-)
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 0ac4b5b..5be9c2d 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -121,6 +121,9 @@
.periods_max = CAPTURE_MAX_NUM_PERIODS,
};
+/* Lock to protect access to registers */
+static DEFINE_SPINLOCK(lock);
+
static u32 acp_reg_read(void __iomem *acp_mmio, u32 reg)
{
return readl(acp_mmio + (reg * 4));
@@ -131,6 +134,33 @@ static void acp_reg_write(u32 val, void __iomem *acp_mmio,
u32 reg)
writel(val, acp_mmio + (reg * 4));
}
+static void acp_reg_write_srbm_targ(void __iomem *acp_mmio,
+ u32 addr, u32 data)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&lock, flags);
+ acp_reg_write(addr, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
+ acp_reg_write(data, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
+ spin_unlock_irqrestore(&lock, flags);
+}
+
+static void acp_reg_read_mod_write(void __iomem *acp_mmio, u32 addr,
+ u32 mask, bool set)
+{
+ u32 val;
+ unsigned long flags;
+
+ spin_lock_irqsave(&lock, flags);
+ val = acp_reg_read(acp_mmio, addr);
+ if (set)
+ val |= mask;
+ else
+ val &= ~mask;
+ acp_reg_write(val, acp_mmio, addr);
+ spin_unlock_irqrestore(&lock, flags);
+}
+
/*
* Configure a given dma channel parameters - enable/disable,
* number of descriptors, priority
@@ -172,15 +202,12 @@ static void config_dma_descriptor_in_sram(void __iomem
*acp_mmio,
sram_offset = (descr_idx * sizeof(acp_dma_dscr_transfer_t));
/* program the source base address. */
- acp_reg_write(sram_offset, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
- acp_reg_write(descr_info->src, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
+ acp_reg_write_srbm_targ(acp_mmio, sram_offset, descr_info->src);
/* program the destination base address. */
- acp_reg_write(sram_offset + 4, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
- acp_reg_write(descr_info->dest, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
-
+ acp_reg_write_srbm_targ(acp_mmio, sram_offset + 4, descr_info->dest);
/* program the number of bytes to be transferred for this descriptor. */
- acp_reg_write(sram_offset + 8, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
- acp_reg_write(descr_info->xfer_val, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
+ acp_reg_write_srbm_targ(acp_mmio, sram_offset + 8,
+ descr_info->xfer_val);
}
static void pre_config_reset(void __iomem *acp_mmio, u16 ch_num)
@@ -309,8 +336,12 @@ static void acp_pte_config(void __iomem *acp_mmio, struct
page *pg,
u32 low;
u32 high;
u32 offset;
+ unsigned long flags;
offset = ACP_DAGB_GRP_SRBM_SRAM_BASE_OFFSET + (pte_offset * 8);
+
+ spin_lock_irqsave(&lock, flags);
+
for (page_idx = 0; page_idx < (num_of_pages); page_idx++) {
/* Load the low address of page int ACP SRAM through SRBM */
acp_reg_write((offset + (page_idx * 8)),
@@ -333,6 +364,8 @@ static void acp_pte_config(void __iomem *acp_mmio, struct
page *pg,
/* Move to next physically contiguos page */
pg++;
}
+
+ spin_unlock_irqrestore(&lock, flags);
}
static void config_acp_dma(void __iomem *acp_mmio,
@@ -870,29 +903,29 @@ static int acp_dma_hw_params(struct snd_pcm_substream
*substream,
}
}
if (adata->asic_type == CHIP_STONEY) {
- val = acp_reg_read(adata->acp_mmio,
- mmACP_I2S_16BIT_RESOLUTION_EN);
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
switch (rtd->i2s_instance) {
case I2S_BT_INSTANCE:
- val |= ACP_I2S_BT_16BIT_RESOLUTION_EN;
+ val = ACP_I2S_BT_16BIT_RESOLUTION_EN;
break;
case I2S_SP_INSTANCE:
default:
- val |= ACP_I2S_SP_16BIT_RESOLUTION_EN;
+ val = ACP_I2S_SP_16BIT_RESOLUTION_EN;
}
} else {
switch (rtd->i2s_instance) {
case I2S_BT_INSTANCE:
- val |= ACP_I2S_BT_16BIT_RESOLUTION_EN;
+ val = ACP_I2S
[PATCH v3] ASoC: AMD: Fix race condition between register access
During simultaneous running of playback and capture, we
got hit by incorrect value write on common register. This was due
to race condition between 2 streams.
Fixing this by locking the common register access.
Signed-off-by: Akshu Agrawal
---
v2: Added 2 helper functions, removed locking in ch enable/disable as they
are separate registers.
v3: Modified helper acp_reg_read_mod_write, moved pte locks to helper
sound/soc/amd/acp-pcm-dma.c | 66 ++---
1 file changed, 44 insertions(+), 22 deletions(-)
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 0ac4b5b..0d4c2eb 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -121,6 +121,9 @@
.periods_max = CAPTURE_MAX_NUM_PERIODS,
};
+/* Lock to protect access to registers */
+static DEFINE_SPINLOCK(lock);
+
static u32 acp_reg_read(void __iomem *acp_mmio, u32 reg)
{
return readl(acp_mmio + (reg * 4));
@@ -131,6 +134,31 @@ static void acp_reg_write(u32 val, void __iomem *acp_mmio,
u32 reg)
writel(val, acp_mmio + (reg * 4));
}
+static void acp_reg_write_srbm_targ(void __iomem *acp_mmio,
+ u32 addr, u32 data)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&lock, flags);
+ acp_reg_write(addr, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
+ acp_reg_write(data, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
+ spin_unlock_irqrestore(&lock, flags);
+}
+
+static void acp_reg_read_mod_write(void __iomem *acp_mmio, u32 addr,
+ u32 mask, u32 value)
+{
+ u32 val;
+ unsigned long flags;
+
+ spin_lock_irqsave(&lock, flags);
+ val = acp_reg_read(acp_mmio, addr);
+ val &= ~mask;
+ val |= (mask & value);
+ acp_reg_write(val, acp_mmio, addr);
+ spin_unlock_irqrestore(&lock, flags);
+}
+
/*
* Configure a given dma channel parameters - enable/disable,
* number of descriptors, priority
@@ -172,15 +200,12 @@ static void config_dma_descriptor_in_sram(void __iomem
*acp_mmio,
sram_offset = (descr_idx * sizeof(acp_dma_dscr_transfer_t));
/* program the source base address. */
- acp_reg_write(sram_offset, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
- acp_reg_write(descr_info->src, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
+ acp_reg_write_srbm_targ(acp_mmio, sram_offset, descr_info->src);
/* program the destination base address. */
- acp_reg_write(sram_offset + 4, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
- acp_reg_write(descr_info->dest, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
-
+ acp_reg_write_srbm_targ(acp_mmio, sram_offset + 4, descr_info->dest);
/* program the number of bytes to be transferred for this descriptor. */
- acp_reg_write(sram_offset + 8, acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
- acp_reg_write(descr_info->xfer_val, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
+ acp_reg_write_srbm_targ(acp_mmio, sram_offset + 8,
+ descr_info->xfer_val);
}
static void pre_config_reset(void __iomem *acp_mmio, u16 ch_num)
@@ -311,24 +336,22 @@ static void acp_pte_config(void __iomem *acp_mmio, struct
page *pg,
u32 offset;
offset = ACP_DAGB_GRP_SRBM_SRAM_BASE_OFFSET + (pte_offset * 8);
+
for (page_idx = 0; page_idx < (num_of_pages); page_idx++) {
/* Load the low address of page int ACP SRAM through SRBM */
- acp_reg_write((offset + (page_idx * 8)),
- acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
addr = page_to_phys(pg);
low = lower_32_bits(addr);
high = upper_32_bits(addr);
- acp_reg_write(low, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
+ acp_reg_write_srbm_targ(acp_mmio, (offset + (page_idx * 8)),
+ low);
/* Load the High address of page int ACP SRAM through SRBM */
- acp_reg_write((offset + (page_idx * 8) + 4),
- acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
-
/* page enable in ACP */
high |= BIT(31);
- acp_reg_write(high, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
+ acp_reg_write_srbm_targ(acp_mmio, (offset + (page_idx * 8) + 4),
+ high);
/* Move to next physically contiguos page */
pg++;
@@ -870,29 +893,28 @@ static int acp_dma_hw_params(struct snd_pcm_substream
*substream,
}
}
if (adata->asic_type == CHIP_STONEY) {
- val = acp_reg_read(adata->acp_mmio,
- mmACP_I2S_16BIT_RESOLUTION_EN);
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
switch (rtd->i2s_instance) {
case I2S_BT_INSTANCE:
- val |= ACP_I2S_BT_16BIT_RESO
Re: [PATCH] ACPI: APD: Add AMD misc clock handler support
On 4/30/2018 2:23 PM, kbuild test robot wrote: Hi Akshu, Thank you for the patch! Yet something to improve: [auto build test ERROR on pm/linux-next] [also build test ERROR on v4.17-rc3 next-20180426] [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/Akshu-Agrawal/ACPI-APD-Add-AMD-misc-clock-handler-support/20180430-152810 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: x86_64-acpi-redef (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers//acpi/acpi_apd.c:14:10: fatal error: linux/platform_data/clk-st.h: No such file or directory #include ^~ compilation terminated. This patch is dependent on https://patchwork.kernel.org/patch/10370955/ The above patch adds the required include file. Thanks, Akshu vim +14 drivers//acpi/acpi_apd.c > 14 #include 15 #include 16 #include 17 #include 18 #include 19 #include 20 #include 21 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [alsa-devel] [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems
On 4/30/2018 9:54 PM, Pierre-Louis Bossart wrote:
On 4/30/18 4:23 AM, Akshu Agrawal wrote:
Non-dts based systems can use ACPI DSDT to pass on the mclk
to da7219.
This enables da7219 mclk to be linked to system clock.
Enable/Disable of the mclk is already handled in the codec so
platform drivers don't have to explicitly do handling of mclk.
Signed-off-by: Akshu Agrawal
---
v2: Fixed kbuild error
include/sound/da7219.h | 2 ++
sound/soc/codecs/da7219.c | 7 ++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/include/sound/da7219.h b/include/sound/da7219.h
index 1bfcb16..df7ddf4 100644
--- a/include/sound/da7219.h
+++ b/include/sound/da7219.h
@@ -38,6 +38,8 @@ struct da7219_pdata {
const char *dai_clks_name;
+ const char *mclk_name;
+
/* Mic */
enum da7219_micbias_voltage micbias_lvl;
enum da7219_mic_amp_in_sel mic_amp_in_sel;
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 980a6a8..aed68a4 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1624,6 +1624,8 @@ static struct da7219_pdata
*da7219_fw_to_pdata(struct snd_soc_component *compone
dev_warn(dev, "Using default clk name: %s\n",
pdata->dai_clks_name);
+ device_property_read_string(dev, "dlg,mclk-name",
&pdata->mclk_name);
+
if (device_property_read_u32(dev, "dlg,micbias-lvl", &of_val32)
>= 0)
pdata->micbias_lvl = da7219_fw_micbias_lvl(dev, of_val32);
else
@@ -1905,7 +1907,10 @@ static int da7219_probe(struct
snd_soc_component *component)
da7219_handle_pdata(component);
/* Check if MCLK provided */
- da7219->mclk = devm_clk_get(component->dev, "mclk");
+ if (da7219->pdata->mclk_name)
+ da7219->mclk = clk_get(NULL, da7219->pdata->mclk_name);
+ if (!da7219->mclk)
+ da7219->mclk = devm_clk_get(component->dev, "mclk");
this looks weird, why are you using different clk functions depending on
the existence of a _DSD property? Why not just change the name and keep
the same flow, e.g something like
if(!da7219->pdata->mclk_name)
da7219->pdata->mclk_name = "mclk";
da7219->mclk = devm_clk_get(component->dev, da7219->pdata->mclk_name);
We can't use devm_clk_get as the value of dev argument has to be NULL,
which can not be used with devm_clk_get.
System clock which are linked to mclk are registered by a separate ACPI
device. And this exposing of DSD property is for all those platforms
which are non-dts based.
if (IS_ERR(da7219->mclk)) {
if (PTR_ERR(da7219->mclk) != -ENOENT) {
ret = PTR_ERR(da7219->mclk);
Re: [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems
On 5/1/2018 12:35 AM, Adam Thomson wrote:
On 30 April 2018 10:23, Akshu Agrawal wrote:
Non-dts based systems can use ACPI DSDT to pass on the mclk
to da7219.
This enables da7219 mclk to be linked to system clock.
Enable/Disable of the mclk is already handled in the codec so
platform drivers don't have to explicitly do handling of mclk.
There is already a means via DT to specify the MCLK for a device using the
generic clock DT bindings, and this driver already uses that. Should ACPI not
have something similar to that which is generic, rather than adding device
specific bindings/properties to achieve the same? There will be other drivers
that will want to do the same.
IMO for all non-dts based ACPI systems, DSDT would be the best and
simplest way to link system clock to mclk. Currently, machine audio
drivers handles the clock and this can be avoided by having this property.
Signed-off-by: Akshu Agrawal
---
v2: Fixed kbuild error
include/sound/da7219.h| 2 ++
sound/soc/codecs/da7219.c | 7 ++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/include/sound/da7219.h b/include/sound/da7219.h
index 1bfcb16..df7ddf4 100644
--- a/include/sound/da7219.h
+++ b/include/sound/da7219.h
@@ -38,6 +38,8 @@ struct da7219_pdata {
const char *dai_clks_name;
+ const char *mclk_name;
+
/* Mic */
enum da7219_micbias_voltage micbias_lvl;
enum da7219_mic_amp_in_sel mic_amp_in_sel;
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 980a6a8..aed68a4 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1624,6 +1624,8 @@ static struct da7219_pdata *da7219_fw_to_pdata(struct
snd_soc_component *compone
dev_warn(dev, "Using default clk name: %s\n",
pdata->dai_clks_name);
+ device_property_read_string(dev, "dlg,mclk-name", &pdata->mclk_name);
+
if (device_property_read_u32(dev, "dlg,micbias-lvl", &of_val32) >= 0)
pdata->micbias_lvl = da7219_fw_micbias_lvl(dev, of_val32);
else
@@ -1905,7 +1907,10 @@ static int da7219_probe(struct snd_soc_component
*component)
da7219_handle_pdata(component);
/* Check if MCLK provided */
- da7219->mclk = devm_clk_get(component->dev, "mclk");
+ if (da7219->pdata->mclk_name)
+ da7219->mclk = clk_get(NULL, da7219->pdata->mclk_name);
By doing this you would need an associated 'clk_put()' whereas the devm call
avoids this.
Agreed, would add a corresponding clk_put call.
Thanks,
Akshu
+ if (!da7219->mclk)
+ da7219->mclk = devm_clk_get(component->dev, "mclk");
if (IS_ERR(da7219->mclk)) {
if (PTR_ERR(da7219->mclk) != -ENOENT) {
ret = PTR_ERR(da7219->mclk);
--
1.9.1
Re: [PATCH] clk: x86: Add ST oscout platform clock
On 5/2/2018 3:28 AM, Stephen Boyd wrote:
Quoting Akshu Agrawal (2018-04-30 00:06:53)
diff --git a/drivers/clk/x86/Makefile b/drivers/clk/x86/Makefile
index 1367afb..f7ebae1 100644
--- a/drivers/clk/x86/Makefile
+++ b/drivers/clk/x86/Makefile
@@ -1,3 +1,4 @@
clk-x86-lpss-objs := clk-lpt.o
obj-$(CONFIG_X86_INTEL_LPSS) += clk-x86-lpss.o
obj-$(CONFIG_PMC_ATOM) += clk-pmc-atom.o
+obj-$(CONFIG_X86) += clk-st.o
No desire to make a Kconfig for this driver so it isn't included all the
time on x86 devices that don't have this hardware?
Accepted, will use AMD specific config.
diff --git a/drivers/clk/x86/clk-st.c b/drivers/clk/x86/clk-st.c
new file mode 100644
index 000..6ac0dc5
--- /dev/null
+++ b/drivers/clk/x86/clk-st.c
@@ -0,0 +1,80 @@
+/*
+ * clock framework for AMD Stoney based clocks
+ *
+ * Copyright 2018 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
Can you use the SPDX tags here?
Accepted. Will add in v2.
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+/* Clock Driving Strength 2 register */
+#define CLKDRVSTR2 0x28
+/* Clock Control 1 register */
+#define MISCCLKCNTL1 0x40
+/* Auxiliary clock1 enable bit */
+#define OSCCLKENB 2
+/* 25Mhz auxiliary output clock freq bit */
+#define OSCOUT1CLK25MHZ16
+
+static const char * const clk_oscout1_parents[] = { "clk48MHz", "clk25MHz" };
+
+static int st_clk_probe(struct platform_device *pdev)
+{
+ struct st_clk_data *st_data;
+ struct clk *clk_48m;
+ struct clk *clk_25m;
+ struct clk *clk_oscout1_mux;
+ struct clk *clk_oscout1;
+
+ st_data = dev_get_platdata(&pdev->dev);
+ if (!st_data || !st_data->base)
+ return -EINVAL;
+
+ clk_48m = clk_register_fixed_rate(NULL, "clk48MHz", NULL, 0,
+ 4800);
+ clk_25m = clk_register_fixed_rate(NULL, "clk25MHz", NULL, 0,
+ 2500);
+
+ clk_oscout1_mux = clk_register_mux(NULL, "oscout1_mux",
+ clk_oscout1_parents, ARRAY_SIZE(clk_oscout1_parents),
+ 0, st_data->base + CLKDRVSTR2, OSCOUT1CLK25MHZ, 3, 0, NULL);
+
+ clk_set_parent(clk_oscout1_mux, clk_25m);
+
+ clk_oscout1 = clk_register_gate(NULL, "oscout1", "oscout1_mux",
+ 0, st_data->base + MISCCLKCNTL1, OSCCLKENB,
+ CLK_GATE_SET_TO_DISABLE, NULL);
+
+ clk_register_clkdev(clk_oscout1, "oscout1", NULL);
Can you use the clk_hw registration and clkdev APIs? It would mean that
clk_set_parent() call up above would still be needed but I guess that's
OK. We don't currently have a way for non-DT based drivers to configure
the parents of a clk when it's registered.
Accepted, Changing in v2 to use clk_hw_register_.
+
+ return 0;
+}
+
diff --git a/include/linux/platform_data/clk-st.h
b/include/linux/platform_data/clk-st.h
new file mode 100644
index 000..5ede980
--- /dev/null
+++ b/include/linux/platform_data/clk-st.h
@@ -0,0 +1,32 @@
+/*
+ * clock framework for AMD Stoney based clock
+ *
+ * Copyright 2018 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * F
Re: [PATCH v5 1/2] clk: x86: Add ST oscout platform clock
On 5/15/2018 3:02 PM, Rafael J. Wysocki wrote: > On Wednesday, May 9, 2018 11:59:00 AM CEST Akshu Agrawal wrote: >> Stoney SoC provides oscout clock. This clock can support 25Mhz and >> 48Mhz of frequency. >> The clock is available for general system use. >> >> Signed-off-by: Akshu Agrawal > > I'm not sure if the Stephen Boyd's comments on one of the previous > versions of this patch have been addressed. Have they? > > In any case, if I'm expected to take this, I need an ACK from Stephen on it. > > Thanks, > Rafael > All comments of Stephen Boyd and Daniel Kurtz have been addressed. Stephen, if you are Ok with this can you please ACK it. Thanks, Akshu
Re: [PATCH v3] ASoC: da7219: read fmw property to get mclk for non-dts systems
On 5/7/2018 12:09 PM, Daniel Kurtz wrote:
> On Sun, May 6, 2018 at 10:50 PM Agrawal, Akshu
> wrote:
>
>
>
>> On 5/4/2018 2:45 PM, Adam Thomson wrote:
>>> On 03 May 2018 08:59, Akshu Agrawal wrote:
>>>
>>>> Non-dts based systems can use ACPI DSDT to pass on the mclk
>>>> to da7219.
>>>> This enables da7219 mclk to be linked to system clock.
>>>> Enable/Disable of the mclk is already handled in the codec so
>>>> platform drivers don't have to explicitly do handling of mclk.
>>>>
>>>> Signed-off-by: Akshu Agrawal
>>>> ---
>>>> v2: Fixed kbuild error
>>>> v3: Add corresponding clk_put for clk_get
>>>> include/sound/da7219.h| 2 ++
>>>> sound/soc/codecs/da7219.c | 10 +-
>>>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/sound/da7219.h b/include/sound/da7219.h
>>>> index 1bfcb16..df7ddf4 100644
>>>> --- a/include/sound/da7219.h
>>>> +++ b/include/sound/da7219.h
>>>> @@ -38,6 +38,8 @@ struct da7219_pdata {
>>>>
>>>> const char *dai_clks_name;
>>>>
>>>> +const char *mclk_name;
>>>> +
>>>> /* Mic */
>>>> enum da7219_micbias_voltage micbias_lvl;
>>>> enum da7219_mic_amp_in_sel mic_amp_in_sel;
>>>> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
>>>> index 980a6a8..ecd46fc 100644
>>>> --- a/sound/soc/codecs/da7219.c
>>>> +++ b/sound/soc/codecs/da7219.c
>>>> @@ -1624,6 +1624,8 @@ static struct da7219_pdata
> *da7219_fw_to_pdata(struct
>>>> snd_soc_component *compone
>>>> dev_warn(dev, "Using default clk name: %s\n",
>>>> pdata->dai_clks_name);
>>>>
>>>> +device_property_read_string(dev, "dlg,mclk-name",
> &pdata->mclk_name);
>>>> +
>>>
>>> Personally am still not keen on this. To me the use of a
> device_property_*
>>> function suggests the same property resides in both DT and ACPI, but
> here we're
>>> only using this for the ACPI case. DT has no want or need for this. I
> still feel
>>> we should look at something more generic in the clock framework,
> although I do
>>> agree with Mark that this should be properly specced.
>>>
>
>> I am not an expert in field of ACPI, IMO forming a Spec and changing
>> ACPI to have DT like clock framework is good to have but a bigger change
>> which should be taken up later.
>
>> The current code of handling of mclk in the driver is usable only by DT.
>> The device_property (though ACPI specific) makes this code, a common
>> code for DT and ACPI based devices.
>
>> https://www.kernel.org/doc/Documentation/acpi/DSD-properties-rules.txt
>> "Still, for the sake of code re-use, it may make sense to provide as
>> much of the configuration data as possible in the form of device
>> properties and complement that with an ACPI-specific mechanism suitable
>> for the use case at hand.."
>
> This sounds like a pretty reasonable justification for addressing the issue
> using DSD to me.
> For what its worth, you can add:
>
> Reviewed-by: Daniel Kurtz
>
>
Hi Mark,
If you are Ok with this patch can you please take this.
Thanks,
Akshu
Re: [PATCH 2/3] ASoC: AMD: Move clk enable from hw_params/free to startup/shutdown
On 4/24/2018 10:06 PM, Daniel Kurtz wrote:
On Mon, Apr 23, 2018 at 9:03 PM Vijendar Mukunda
wrote:
From: Akshu Agrawal
hw_param can be called multiple times and thus we can have
more clk enable. The clk may not get diabled due to refcounting.
startup/shutdown ensures single clk enable/disable call.
Signed-off-by: Akshu Agrawal
Signed-off-by: Vijendar Mukunda
---
sound/soc/amd/acp-da7219-max98357a.c | 54
1 file changed, 37 insertions(+), 17 deletions(-)
diff --git a/sound/soc/amd/acp-da7219-max98357a.c
b/sound/soc/amd/acp-da7219-max98357a.c
index b205c78..0f16f6d 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -38,8 +38,7 @@
#include "../codecs/da7219.h"
#include "../codecs/da7219-aad.h"
-#define CZ_PLAT_CLK 2400
-#define MCLK_RATE 24576000
+#define CZ_PLAT_CLK 2500
#define DUAL_CHANNEL 2
static struct snd_soc_jack cz_jack;
@@ -62,7 +61,7 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime
*rtd)
}
ret = snd_soc_dai_set_pll(codec_dai, 0, DA7219_SYSCLK_PLL,
- CZ_PLAT_CLK, MCLK_RATE);
+ CZ_PLAT_CLK, DA7219_PLL_FREQ_OUT_98304);
These are unrelated fixes that should be in their own patch.
Accepted. Will split it.
if (ret < 0) {
dev_err(rtd->dev, "can't set codec pll: %d\n", ret);
return ret;
@@ -85,8 +84,7 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime
*rtd)
return 0;
}
-static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
-struct snd_pcm_hw_params *params)
+static int da7219_clk_enable(struct snd_pcm_substream *substream)
{
int ret = 0;
struct snd_soc_pcm_runtime *rtd = substream->private_data;
@@ -100,11 +98,9 @@ static int cz_da7219_hw_params(struct
snd_pcm_substream *substream,
return ret;
}
-static int cz_da7219_hw_free(struct snd_pcm_substream *substream)
+static void da7219_clk_disable(void)
{
clk_disable_unprepare(da7219_dai_clk);
-
- return 0;
}
static const unsigned int channels[] = {
@@ -127,7 +123,7 @@ static const struct snd_pcm_hw_constraint_list
constraints_channels = {
.mask = 0,
};
-static int cz_fe_startup(struct snd_pcm_substream *substream)
+static int cz_da7219_startup(struct snd_pcm_substream *substream)
{
struct snd_pcm_runtime *runtime = substream->runtime;
@@ -141,23 +137,47 @@ static int cz_fe_startup(struct snd_pcm_substream
*substream)
snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
&constraints_rates);
- return 0;
+ return da7219_clk_enable(substream);
+}
+
+static void cz_da7219_shutdown(struct snd_pcm_substream *substream)
+{
+ da7219_clk_disable();
+}
+
+static int cz_max_startup(struct snd_pcm_substream *substream)
+{
+ return da7219_clk_enable(substream);
+}
+
+static void cz_max_shutdown(struct snd_pcm_substream *substream)
+{
+ da7219_clk_disable();
+}
+
+static int cz_dmic_startup(struct snd_pcm_substream *substream)
+{
+ return da7219_clk_enable(substream);
+}
+
+static void cz_dmic_shutdown(struct snd_pcm_substream *substream)
+{
+ da7219_clk_disable();
}
This is ok, or you could combine the common cz_max_* & cz_dmic_*.
Need to be separate as they have different cpu dai.
static struct snd_soc_ops cz_da7219_cap_ops = {
I think these should all be "static const struct snd_soc_ops" (please fix
in a separate patch).
Accepted. Will post another patch for same.
- .hw_params = cz_da7219_hw_params,
- .hw_free = cz_da7219_hw_free,
- .startup = cz_fe_startup,
+ .startup = cz_da7219_startup,
+ .shutdown = cz_da7219_shutdown,
};
static struct snd_soc_ops cz_max_play_ops = {
- .hw_params = cz_da7219_hw_params,
- .hw_free = cz_da7219_hw_free,
+ .startup = cz_max_startup,
+ .shutdown = cz_max_shutdown,
};
static struct snd_soc_ops cz_dmic_cap_ops = {
- .hw_params = cz_da7219_hw_params,
- .hw_free = cz_da7219_hw_free,
+ .startup = cz_dmic_startup,
+ .shutdown = cz_dmic_shutdown,
};
static struct snd_soc_dai_link cz_dai_7219_98357[] = {
--
2.7.4
Thanks,
Akshu
[v3] ASoC: AMD: Configure wclk and bclk of master codec
With CCF support in da7219, we can now set the correct rate of
wclk and bclk.
Signed-off-by: Akshu Agrawal
---
v2: Removed clk set rate and enable/disable from da7219 ops as
da7219 codec takes care of them internally. Clock configuration
kept for those codecs where da7219 acts as master of clock.
As suggested by Adam Thomson.
v3: As per spec, adau7002 bclk to be minimum of 64x of wclk. Hence,
limiting bclk for all codecs. Also, because of this changes done in v2
of this patch have to be brought back.
sound/soc/amd/acp-da7219-max98357a.c | 66 ++--
1 file changed, 42 insertions(+), 24 deletions(-)
diff --git a/sound/soc/amd/acp-da7219-max98357a.c
b/sound/soc/amd/acp-da7219-max98357a.c
index 8a619a75b3a9..16b0ea3a3d72 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -46,7 +46,8 @@
#define DUAL_CHANNEL 2
static struct snd_soc_jack cz_jack;
-static struct clk *da7219_dai_clk;
+static struct clk *da7219_dai_wclk;
+static struct clk *da7219_dai_bclk;
extern bool bt_uart_enable;
static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
@@ -72,7 +73,8 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
return ret;
}
- da7219_dai_clk = clk_get(component->dev, "da7219-dai-bclk");
+ da7219_dai_wclk = clk_get(component->dev, "da7219-dai-wclk");
+ da7219_dai_bclk = clk_get(component->dev, "da7219-dai-bclk");
ret = snd_soc_card_jack_new(card, "Headset Jack",
SND_JACK_HEADSET | SND_JACK_LINEOUT |
@@ -94,12 +96,15 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
return 0;
}
-static int da7219_clk_enable(struct snd_pcm_substream *substream)
+static int da7219_clk_enable(struct snd_pcm_substream *substream,
+int wclk_rate, int bclk_rate)
{
int ret = 0;
struct snd_soc_pcm_runtime *rtd = substream->private_data;
- ret = clk_prepare_enable(da7219_dai_clk);
+ clk_set_rate(da7219_dai_wclk, wclk_rate);
+ clk_set_rate(da7219_dai_bclk, bclk_rate);
+ ret = clk_prepare_enable(da7219_dai_bclk);
if (ret < 0) {
dev_err(rtd->dev, "can't enable master clock %d\n", ret);
return ret;
@@ -110,7 +115,7 @@ static int da7219_clk_enable(struct snd_pcm_substream
*substream)
static void da7219_clk_disable(void)
{
- clk_disable_unprepare(da7219_dai_clk);
+ clk_disable_unprepare(da7219_dai_bclk);
}
static const unsigned int channels[] = {
@@ -151,7 +156,7 @@ static int cz_da7219_play_startup(struct snd_pcm_substream
*substream)
&constraints_rates);
machine->play_i2s_instance = I2S_SP_INSTANCE;
- return da7219_clk_enable(substream);
+ return 0;
}
static int cz_da7219_cap_startup(struct snd_pcm_substream *substream)
@@ -173,12 +178,7 @@ static int cz_da7219_cap_startup(struct snd_pcm_substream
*substream)
machine->cap_i2s_instance = I2S_SP_INSTANCE;
machine->capture_channel = CAP_CHANNEL1;
- return da7219_clk_enable(substream);
-}
-
-static void cz_da7219_shutdown(struct snd_pcm_substream *substream)
-{
- da7219_clk_disable();
+ return 0;
}
static int cz_max_startup(struct snd_pcm_substream *substream)
@@ -199,12 +199,7 @@ static int cz_max_startup(struct snd_pcm_substream
*substream)
&constraints_rates);
machine->play_i2s_instance = I2S_BT_INSTANCE;
- return da7219_clk_enable(substream);
-}
-
-static void cz_max_shutdown(struct snd_pcm_substream *substream)
-{
- da7219_clk_disable();
+ return 0;
}
static int cz_dmic0_startup(struct snd_pcm_substream *substream)
@@ -225,7 +220,7 @@ static int cz_dmic0_startup(struct snd_pcm_substream
*substream)
&constraints_rates);
machine->cap_i2s_instance = I2S_BT_INSTANCE;
- return da7219_clk_enable(substream);
+ return 0;
}
static int cz_dmic1_startup(struct snd_pcm_substream *substream)
@@ -247,10 +242,28 @@ static int cz_dmic1_startup(struct snd_pcm_substream
*substream)
machine->cap_i2s_instance = I2S_SP_INSTANCE;
machine->capture_channel = CAP_CHANNEL0;
- return da7219_clk_enable(substream);
+ return 0;
}
-static void cz_dmic_shutdown(struct snd_pcm_substream *substream)
+static int cz_da7219_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params)
+{
+ int wclk, bclk;
+
+ wclk = params_rate(params);
+ bclk = wclk * params_channels(params) *
+ snd_pcm_format_width(params_format(params));
+ /* ADAU7002 spec: "The ADAU7002 requires a BCLK rate
+* that is minimum of 64x the LRCLK sample rate."
+* DA7219 is the only clk source so for all codecs
+* we have to limit bclk to 64X lrclk.
+
Re: [v2 1/4] ACPI: APD: Change name from ST to FCH
On 7/29/2020 6:56 AM, Stephen Boyd wrote: Quoting Akshu Agrawal (2020-07-28 01:28:53) AMD SoC general pupose clk is present in new platforms with same MMIO mappings. We can reuse the same clk handler support for other platforms. Hence, changing name from ST(SoC) to FCH(IP) Signed-off-by: Akshu Agrawal --- Acked-by: Stephen Boyd Hi Rafael, I see the status of these patches as Not Applicable in patchwork, is there any pending action for me? Thanks, Akshu
Re: [v2 1/4] ACPI: APD: Change name from ST to FCH
On 7/31/2020 4:44 PM, Rafael J. Wysocki wrote: On Fri, Jul 31, 2020 at 2:44 AM Agrawal, Akshu wrote: On 7/29/2020 6:56 AM, Stephen Boyd wrote: Quoting Akshu Agrawal (2020-07-28 01:28:53) AMD SoC general pupose clk is present in new platforms with same MMIO mappings. We can reuse the same clk handler support for other platforms. Hence, changing name from ST(SoC) to FCH(IP) Signed-off-by: Akshu Agrawal --- Acked-by: Stephen Boyd Hi Rafael, I see the status of these patches as Not Applicable in patchwork, is there any pending action for me? Yes, there is. You need to let me know if you want me to apply them (and I mean the whole series). :-) Yes, please apply the whole series. Besides, I only can see 3 out of 4 patches, so if you want me to apply them, can you please resend the whole series with CCs to linux-acpi? Sending them again with cc to linux-acpi. Thanks, Akshu
[PATCH] ASoC: AMD: Configure master codec on all playback/capture cases
In the system design da7219 is the master codec and clocks are
generated by it.
Bclk is to be generated at the required rate for other codecs used when
da7219 is acting only as clock master. For this call hw_params of da7219
during playback/capture on non da7219 codecs.
Being able to set bclk at lower rate also fixes noise issue observed on
some dmics.
Signed-off-by: Akshu Agrawal
---
sound/soc/amd/acp-da7219-max98357a.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/sound/soc/amd/acp-da7219-max98357a.c
b/sound/soc/amd/acp-da7219-max98357a.c
index a5daad973ce5..ad327415290a 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -45,6 +45,7 @@
#define CZ_PLAT_CLK 4800
#define DUAL_CHANNEL 2
+static struct snd_soc_dai *codec_dai;
static struct snd_soc_jack cz_jack;
static struct clk *da7219_dai_clk;
extern int bt_uart_enable;
@@ -53,8 +54,10 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
{
int ret;
struct snd_soc_card *card = rtd->card;
- struct snd_soc_dai *codec_dai = rtd->codec_dai;
- struct snd_soc_component *component = codec_dai->component;
+ struct snd_soc_component *component;
+
+ codec_dai = rtd->codec_dai;
+ component = codec_dai->component;
dev_info(rtd->dev, "codec dai name = %s\n", codec_dai->name);
@@ -255,6 +258,16 @@ static void cz_dmic_shutdown(struct snd_pcm_substream
*substream)
da7219_clk_disable();
}
+static static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params)
+{
+ /* da7219 Codec is clock master so setup as per the needs */
+ if (codec_dai->driver->ops->hw_params)
+ return codec_dai->driver->ops->hw_params(substream, params,
+codec_dai);
+ return 0;
+}
+
static const struct snd_soc_ops cz_da7219_play_ops = {
.startup = cz_da7219_play_startup,
.shutdown = cz_da7219_shutdown,
@@ -268,16 +281,19 @@ static const struct snd_soc_ops cz_da7219_cap_ops = {
static const struct snd_soc_ops cz_max_play_ops = {
.startup = cz_max_startup,
.shutdown = cz_max_shutdown,
+ .hw_params = cz_da7219_hw_params,
};
static const struct snd_soc_ops cz_dmic0_cap_ops = {
.startup = cz_dmic0_startup,
.shutdown = cz_dmic_shutdown,
+ .hw_params = cz_da7219_hw_params,
};
static const struct snd_soc_ops cz_dmic1_cap_ops = {
.startup = cz_dmic1_startup,
.shutdown = cz_dmic_shutdown,
+ .hw_params = cz_da7219_hw_params,
};
static struct snd_soc_dai_link cz_dai_7219_98357[] = {
--
2.17.1
[PATCH] ASoC: DA7219: Fix failure in hw_params by not letting set_rate error out
We need to set minimum bclk 64x of wclk as this is hw constraint in one
of the component used.
Since, clk_set_rate and clk is enabled in machine driver the
clk_set_rate in hw_params of da7219 fails and errors out and when it
tries to override the value.
In cases like these not only clk_set_rate of da7219 codec should fail
and not override the value but also should not error out.
Signed-off-by: Akshu Agrawal
---
sound/soc/codecs/da7219.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 5f5fa3416af3..a041dbf442af 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1621,13 +1621,7 @@ static int da7219_hw_params(struct snd_pcm_substream
*substream,
if (bclk) {
bclk_rate = frame_size * sr;
- ret = clk_set_rate(bclk, bclk_rate);
- if (ret) {
- dev_err(component->dev,
- "Failed to set BCLK rate %lu: %d\n",
- bclk_rate, ret);
- return ret;
- }
+ clk_set_rate(bclk, bclk_rate);
} else {
ret = da7219_set_bclks_per_wclk(component, frame_size);
if (ret) {
--
2.17.1
Re: [alsa-devel] [PATCH] ASoC: DA7219: Fix failure in hw_params by not letting set_rate error out
On 4/22/2019 1:09 PM, Cheng-yi Chiang wrote:
> Hi Akshu,
>
>
> On Mon, Apr 22, 2019 at 2:15 PM Agrawal, Akshu wrote:
>>
>> We need to set minimum bclk 64x of wclk as this is hw constraint in one
>> of the component used.
>> Since, clk_set_rate and clk is enabled in machine driver the
>> clk_set_rate in hw_params of da7219 fails and errors out and when it
>> tries to override the value.
>>
>> In cases like these not only clk_set_rate of da7219 codec should fail
>> and not override the value but also should not error out.
>>
>> Signed-off-by: Akshu Agrawal
>> ---
>> sound/soc/codecs/da7219.c | 8 +---
>> 1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
>> index 5f5fa3416af3..a041dbf442af 100644
>> --- a/sound/soc/codecs/da7219.c
>> +++ b/sound/soc/codecs/da7219.c
>> @@ -1621,13 +1621,7 @@ static int da7219_hw_params(struct snd_pcm_substream
>> *substream,
>>
>> if (bclk) {
>> bclk_rate = frame_size * sr;
>> - ret = clk_set_rate(bclk, bclk_rate);
>> - if (ret) {
>> - dev_err(component->dev,
>> - "Failed to set BCLK rate %lu: %d\n",
>> - bclk_rate, ret);
>> - return ret;
>> - }
>> + clk_set_rate(bclk, bclk_rate);
>
> I think this might mask the real issue when setting rate fails.
Not doing an error check for clk_set_rate is not rare because
clk_set_rate fails if the clk which you are trying to set is already
enabled. Now we can add a is_prepared check before calling clk_set_rate
but that would be redundant as clk_set_rate does the same check.
> A more fundamental problem might be what are the acceptable ratios
> between bclk and wclk.
>
acceptable ratios for da7219 from spec are:
00 = 32 BCLKS per WCLK
01 = 64 BCLKS per WCLK
10 = 128 BCLK per WCLK
11 = 256 BCLK per WCLK
da7219 derives this ration from SR for a wclk. The same we do in machine
driver.
Conflict comes only in case of an exception where there is constraint
from a hw component. Else when the audio is played on da7219 codec we
could have let da7219 only set rate and enable clks.
Thanks,
Akshu
> From existing code of da7219 I see there are only two choices of
> frame_size: 32 or 64.
>
> In da7219_hw_params:
> ...
> if (da7219->master && !da7219->tdm_en) {
> if ((word_len * DA7219_DAI_CH_NUM_MAX) <= 32)
> frame_size = 32;
> else
> frame_size = 64;
>
> And in https://lkml.org/lkml/2019/4/17/322
> the choice from machine driver is 64 or higher.
>
> In cz_da7219_params:
> ...
> if (bclk < (wclk * 64))
> bclk = wclk * 64;
> return da7219_clk_enable(substream, wclk, bclk);
>
> I don't know what is the best solution to avoid this conflict.
> Maybe just set a fixed ratio from device property that can best work
> for the combination of codecs on the machine?
>
>
>
>
>
>> } else {
>> ret = da7219_set_bclks_per_wclk(component,
>> frame_size);
>> if (ret) {
>> --
>> 2.17.1
>>
>> ___
>> Alsa-devel mailing list
>> alsa-de...@alsa-project.org
>> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
[PATCH] ASoC: AMD: Configure wclk and bclk of master codec
With CCF support in da7219, we can now set the correct rate of
wclk and bclk.
Setting bclk at lower rate at 1.53Mhz from its earlier default
rate of 3Mhz, also fixes noise issue observed on some dmics.
Signed-off-by: Akshu Agrawal
---
sound/soc/amd/acp-da7219-max98357a.c | 40 +---
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/sound/soc/amd/acp-da7219-max98357a.c
b/sound/soc/amd/acp-da7219-max98357a.c
index f37a588ba345..1bb5b78b6e10 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -46,7 +46,8 @@
#define DUAL_CHANNEL 2
static struct snd_soc_jack cz_jack;
-static struct clk *da7219_dai_clk;
+static struct clk *da7219_dai_wclk;
+static struct clk *da7219_dai_bclk;
extern int bt_uart_enable;
static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
@@ -72,7 +73,8 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
return ret;
}
- da7219_dai_clk = clk_get(component->dev, "da7219-dai-bclk");
+ da7219_dai_wclk = clk_get(component->dev, "da7219-dai-wclk");
+ da7219_dai_bclk = clk_get(component->dev, "da7219-dai-bclk");
ret = snd_soc_card_jack_new(card, "Headset Jack",
SND_JACK_HEADSET | SND_JACK_LINEOUT |
@@ -94,12 +96,15 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
return 0;
}
-static int da7219_clk_enable(struct snd_pcm_substream *substream)
+static int da7219_clk_enable(struct snd_pcm_substream *substream,
+int wclk_rate, int bclk_rate)
{
int ret = 0;
struct snd_soc_pcm_runtime *rtd = substream->private_data;
- ret = clk_prepare_enable(da7219_dai_clk);
+ clk_set_rate(da7219_dai_wclk, wclk_rate);
+ clk_set_rate(da7219_dai_bclk, bclk_rate);
+ ret = clk_prepare_enable(da7219_dai_bclk);
if (ret < 0) {
dev_err(rtd->dev, "can't enable master clock %d\n", ret);
return ret;
@@ -110,7 +115,7 @@ static int da7219_clk_enable(struct snd_pcm_substream
*substream)
static void da7219_clk_disable(void)
{
- clk_disable_unprepare(da7219_dai_clk);
+ clk_disable_unprepare(da7219_dai_bclk);
}
static const unsigned int channels[] = {
@@ -151,7 +156,7 @@ static int cz_da7219_play_startup(struct snd_pcm_substream
*substream)
&constraints_rates);
machine->play_i2s_instance = I2S_SP_INSTANCE;
- return da7219_clk_enable(substream);
+ return 0;
}
static int cz_da7219_cap_startup(struct snd_pcm_substream *substream)
@@ -173,7 +178,7 @@ static int cz_da7219_cap_startup(struct snd_pcm_substream
*substream)
machine->cap_i2s_instance = I2S_SP_INSTANCE;
machine->capture_channel = CAP_CHANNEL1;
- return da7219_clk_enable(substream);
+ return 0;
}
static void cz_da7219_shutdown(struct snd_pcm_substream *substream)
@@ -199,7 +204,7 @@ static int cz_max_startup(struct snd_pcm_substream
*substream)
&constraints_rates);
machine->play_i2s_instance = I2S_BT_INSTANCE;
- return da7219_clk_enable(substream);
+ return 0;
}
static void cz_max_shutdown(struct snd_pcm_substream *substream)
@@ -225,7 +230,7 @@ static int cz_dmic0_startup(struct snd_pcm_substream
*substream)
&constraints_rates);
machine->cap_i2s_instance = I2S_BT_INSTANCE;
- return da7219_clk_enable(substream);
+ return 0;
}
static int cz_dmic1_startup(struct snd_pcm_substream *substream)
@@ -247,7 +252,7 @@ static int cz_dmic1_startup(struct snd_pcm_substream
*substream)
machine->cap_i2s_instance = I2S_SP_INSTANCE;
machine->capture_channel = CAP_CHANNEL0;
- return da7219_clk_enable(substream);
+ return 0;
}
static void cz_dmic_shutdown(struct snd_pcm_substream *substream)
@@ -255,29 +260,44 @@ static void cz_dmic_shutdown(struct snd_pcm_substream
*substream)
da7219_clk_disable();
}
+static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params)
+{
+ int wclk, bclk;
+
+ wclk = params_rate(params);
+ bclk = wclk * params_channels(params) *
+ snd_pcm_format_width(params_format(params));
+ return da7219_clk_enable(substream, wclk, bclk);
+}
static const struct snd_soc_ops cz_da7219_play_ops = {
.startup = cz_da7219_play_startup,
.shutdown = cz_da7219_shutdown,
+ .hw_params = cz_da7219_hw_params,
};
static const struct snd_soc_ops cz_da7219_cap_ops = {
.startup = cz_da7219_cap_startup,
.shutdown = cz_da7219_shutdown,
+ .hw_params = cz_da7219_hw_params,
};
static const struct snd_soc_ops cz_max_play_ops = {
.startup = cz_max_startup,
.shutdown = cz_max_shutdown,
+ .hw_params = cz_da7219_h
[v2] ASoC: AMD: Configure wclk and bclk of master codec
With CCF support in da7219, we can now set the correct rate of
wclk and bclk.
Setting bclk at lower rate at 1.53Mhz from its earlier default
rate of 3Mhz, also fixes noise issue observed on some dmics.
v2: Removed clk set rate and enable/disable from da7219 ops as
da7219 codec takes care of them internally. Clock configuration
kept for those codecs where da7219 acts as master of clock.
As suggested by Adam Thomson.
Signed-off-by: Akshu Agrawal
---
sound/soc/amd/acp-da7219-max98357a.c | 45 +---
1 file changed, 28 insertions(+), 17 deletions(-)
diff --git a/sound/soc/amd/acp-da7219-max98357a.c
b/sound/soc/amd/acp-da7219-max98357a.c
index 8e2ce369a295..7737c6dc026a 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -46,7 +46,8 @@
#define DUAL_CHANNEL 2
static struct snd_soc_jack cz_jack;
-static struct clk *da7219_dai_clk;
+static struct clk *da7219_dai_wclk;
+static struct clk *da7219_dai_bclk;
extern int bt_uart_enable;
static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
@@ -72,7 +73,8 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
return ret;
}
- da7219_dai_clk = clk_get(component->dev, "da7219-dai-bclk");
+ da7219_dai_wclk = clk_get(component->dev, "da7219-dai-wclk");
+ da7219_dai_bclk = clk_get(component->dev, "da7219-dai-bclk");
ret = snd_soc_card_jack_new(card, "Headset Jack",
SND_JACK_HEADSET | SND_JACK_LINEOUT |
@@ -94,12 +96,15 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
return 0;
}
-static int da7219_clk_enable(struct snd_pcm_substream *substream)
+static int da7219_clk_enable(struct snd_pcm_substream *substream,
+int wclk_rate, int bclk_rate)
{
int ret = 0;
struct snd_soc_pcm_runtime *rtd = substream->private_data;
- ret = clk_prepare_enable(da7219_dai_clk);
+ clk_set_rate(da7219_dai_wclk, wclk_rate);
+ clk_set_rate(da7219_dai_bclk, bclk_rate);
+ ret = clk_prepare_enable(da7219_dai_bclk);
if (ret < 0) {
dev_err(rtd->dev, "can't enable master clock %d\n", ret);
return ret;
@@ -110,7 +115,7 @@ static int da7219_clk_enable(struct snd_pcm_substream
*substream)
static void da7219_clk_disable(void)
{
- clk_disable_unprepare(da7219_dai_clk);
+ clk_disable_unprepare(da7219_dai_bclk);
}
static const unsigned int channels[] = {
@@ -151,7 +156,7 @@ static int cz_da7219_play_startup(struct snd_pcm_substream
*substream)
&constraints_rates);
machine->play_i2s_instance = I2S_SP_INSTANCE;
- return da7219_clk_enable(substream);
+ return 0;
}
static int cz_da7219_cap_startup(struct snd_pcm_substream *substream)
@@ -173,12 +178,7 @@ static int cz_da7219_cap_startup(struct snd_pcm_substream
*substream)
machine->cap_i2s_instance = I2S_SP_INSTANCE;
machine->capture_channel = CAP_CHANNEL1;
- return da7219_clk_enable(substream);
-}
-
-static void cz_da7219_shutdown(struct snd_pcm_substream *substream)
-{
- da7219_clk_disable();
+ return 0;
}
static int cz_max_startup(struct snd_pcm_substream *substream)
@@ -199,7 +199,7 @@ static int cz_max_startup(struct snd_pcm_substream
*substream)
&constraints_rates);
machine->play_i2s_instance = I2S_BT_INSTANCE;
- return da7219_clk_enable(substream);
+ return 0;
}
static void cz_max_shutdown(struct snd_pcm_substream *substream)
@@ -225,7 +225,7 @@ static int cz_dmic0_startup(struct snd_pcm_substream
*substream)
&constraints_rates);
machine->cap_i2s_instance = I2S_BT_INSTANCE;
- return da7219_clk_enable(substream);
+ return 0;
}
static int cz_dmic1_startup(struct snd_pcm_substream *substream)
@@ -247,7 +247,7 @@ static int cz_dmic1_startup(struct snd_pcm_substream
*substream)
machine->cap_i2s_instance = I2S_SP_INSTANCE;
machine->capture_channel = CAP_CHANNEL0;
- return da7219_clk_enable(substream);
+ return 0;
}
static void cz_dmic_shutdown(struct snd_pcm_substream *substream)
@@ -255,29 +255,40 @@ static void cz_dmic_shutdown(struct snd_pcm_substream
*substream)
da7219_clk_disable();
}
+static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params)
+{
+ int wclk, bclk;
+
+ wclk = params_rate(params);
+ bclk = wclk * params_channels(params) *
+ snd_pcm_format_width(params_format(params));
+ return da7219_clk_enable(substream, wclk, bclk);
+}
static const struct snd_soc_ops cz_da7219_play_ops = {
.startup = cz_da7219_play_startup,
- .shutdown = cz_da7219_shutdown,
};
static const struct snd_soc_ops cz_da7219_cap_ops = {
Re: [PATCH AUTOSEL 4.18 043/131] ASoC: soc-pcm: Use delay set in component pointer function
On 9/7/2018 5:53 AM, Sasha Levin wrote: > On Mon, Sep 03, 2018 at 12:16:26PM +0100, Mark Brown wrote: >> On Sun, Sep 02, 2018 at 01:03:55PM +, Sasha Levin wrote: >>> From: Akshu Agrawal >>> >>> [ Upstream commit 9fb4c2bf130b922c77c16a8368732699799c40de ] >>> >>> Take into account the base delay set in pointer callback. >>> >>> There are cases where a pointer function populates >>> runtime->delay, such as: >>> ./sound/pci/hda/hda_controller.c >>> ./sound/soc/intel/atom/sst-mfld-platform-pcm.c >> >> I'm worried that if anyone notices this at all they will have already >> compensated for the delays in userspace and therefore this will cause >> them to see problems as they get double compenstation for delays. > > But what happens when they update to a newer Stable? They're going to > hit that issue anyways. > Drivers which had exposed this delay in pointer function but have compensated for the issue in userspace are likely see the problem of double delay when the update happens. I Don't know what is the best way to communicate that issue is fixed in kernel and usersapce compensation isn't required. But more likely I think the delay was just getting left out and there wouldn't have been a compensation in userspace. Thanks, Akshu
Re: [PATCH 1/2] ASoC: AMD: Fix simultaneous playback and capture on different channel
On 9/10/2018 5:08 PM, Mark Brown wrote: > On Mon, Sep 10, 2018 at 01:36:29PM +0530, Akshu Agrawal wrote: >> If capture and playback are started on different channel (I2S/BT) >> there is a possibilty that channel information passed from machine driver >> is overwritten before the configuration is done in dma driver. >> Example: >> 113.597588: cz_max_startup: ---playback sets BT channel >> 113.597694: cz_dmic1_startup: ---capture sets I2S channel >> 113.597979: acp_dma_hw_params: ---configures capture for I2S channel >> 113.598114: acp_dma_hw_params: ---configures playback for I2S channel >> >> This is fixed by having lock between startup and prepare. This ensures >> no other codec startup gets called between a codec's startup(where channel >> info is set) and hw_params(where channel info is read). > > This isn't viable - the driver will deadlock if the application hits an > error and never gets to startup, or if the application tries to > simultaneously configure two channels (ie, do all the prepares and then > all the parameter configuration and then startup). We can avoid deadlock by having another mutex_unlock in the shutdown call of each of codec's ops. Wouldn't in all possible termination scenarios, it will cleanup and exit via shutdown callback? Having said that I think there is a better approach to this, is by having 2 separate instance variable for playback and capture for passing instance info from machine driver to dma driver. Respective codec in machine driver will set the capture/playback instance. dma driver on the basis of substream->stream can read the correct one. No fear of deadlock in this. Thanks, Akshu
Re: [PATCH V3 10/10] ASoC: amd: dma driver changes for bt i2s instance
On 5/3/2018 10:10 PM, Daniel Kurtz wrote:
On Thu, May 3, 2018 at 1:33 AM Mukunda,Vijendar
wrote:
On Thursday 03 May 2018 11:13 AM, Daniel Kurtz wrote:
Some checkpatch nits below...
On Tue, May 1, 2018 at 2:53 PM Vijendar Mukunda <
vijendar.muku...@amd.com>
wrote:
With in ACP, There are three I2S controllers can be
configured/enabled ( I2S SP, I2S MICSP, I2S BT).
Default enabled I2S controller instance is I2S SP.
This patch provides required changes to support I2S BT
controller Instance.
Signed-off-by: Vijendar Mukunda
---
v1->v2: defined i2s instance macros in acp header file
v2->v3: sqaushed previous patch series and spilt changes
into multiple patches (acp dma driver code cleanup
patches and bt i2s instance specific changes)
sound/soc/amd/acp-da7219-max98357a.c | 23
sound/soc/amd/acp-pcm-dma.c | 256
+++
sound/soc/amd/acp.h | 40 ++
3 files changed, 262 insertions(+), 57 deletions(-)
diff --git a/sound/soc/amd/acp-da7219-max98357a.c
b/sound/soc/amd/acp-da7219-max98357a.c
index 133139d..b3184ab 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -36,6 +36,7 @@
#include
#include
+#include "acp.h"
#include "../codecs/da7219.h"
#include "../codecs/da7219-aad.h"
@@ -44,6 +45,7 @@
static struct snd_soc_jack cz_jack;
static struct clk *da7219_dai_clk;
+extern int bt_pad_enable;
WARNING: externs should be avoided in .c files
We don't have .h file for machine driver and It can be ignored for
one variable.
static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
{
@@ -132,6 +134,9 @@ static const struct snd_pcm_hw_constraint_list
constraints_channels = {
static int cz_da7219_startup(struct snd_pcm_substream *substream)
{
struct snd_pcm_runtime *runtime = substream->runtime;
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ struct snd_soc_card *card = rtd->card;
+ struct acp_platform_info *machine =
snd_soc_card_get_drvdata(card);
/*
* On this platform for PCM device we support stereo
@@ -143,6 +148,7 @@ static int cz_da7219_startup(struct
snd_pcm_substream
*substream)
snd_pcm_hw_constraint_list(runtime, 0,
SNDRV_PCM_HW_PARAM_RATE,
&constraints_rates);
+ machine->i2s_instance = I2S_BT_INSTANCE;
I'm not a big fan of this approach, but I don't know any other way to
tell
a single "platform" driver (acp-pcm-dma) which of two channels (ST/BT)
to
use via the pcm_open() callback.
Mark, can you recommend any other way of doing this?
Hi Dan,
There have been couple of approaches worked upon this earlier.
1) To compare cpu dai name to get the I2S instance value in
acp_dma_open() call.
But, Mark suggested not to implement this approach as we are comparing
dynamically generated cpu dai names.
2) We added i2s_instance parameter as platform data to dwc driver.
By querying dwc driver platform data in acp dma driver, current i2s
instance was programmed in acp_dma_open ().
But Mark's latest comment was to implement platform specific changes in
machine driver. Machine driver and Dma driver should exchange the data
regarding this. We accepted this and current approach is based on the
same comment.
Below is the reference.
https://lkml.org/lkml/2018/4/18/597
Yes, I saw Mark's previous comment, but what we are trying to implement
here is the SoC specific binding between i2s channel and acp-dma channel.
This is a feature of the SoC, not of the i2s controller, but also not a
feature of the audio configuration on the board.
The binding of channel and dma is soc specific but codec to channel is
board specific.
These linkages can change from one board to another. Machine driver is
specific to a board (grunt here) and dma driver being common for various
boards (but specific to a platform like ST/CZ here). Tomorrow there can
be some other board with x codec linked to BT and y to SP.
Hence, machine driver should send this board specific link information
to dma driver for it to dma on the correct channel as per the board.
For these SoCs, the link
between DMA registers & I2S-channel is hard-coded. The machine driver is
already specifying which i2s channel to use when it configures, for
example, '.cpu_dai_name = "designware-i2s.2.auto"'. The i2s channel
selection already implies a particular DMA configuration. It seems
redundant to create this separate out-of-band infrastructure to make the
machine driver also tell its platform driver '.platform_name =
"acp_audio_dma.0.auto"' which i2s channel it is using.
We could have decided on the basis of "cpu_dai_name" but the decision
would have been based on dynamically generated name. Though maybe
redundant in nature, but machine driver is just sending info to dma
driver that this
Re: [PATCH v2 2/2] ACPI: APD: Add AMD misc clock handler support
On 5/4/2018 3:36 PM, Rafael J. Wysocki wrote: > On Friday, May 4, 2018 10:34:44 AM CEST Akshu Agrawal wrote: >> AMD SoC exposes clock for general purpose use. The clock registration >> is done in clk-st driver. The MMIO mapping are passed on to the >> clock driver for accessing the registers. >> The misc clock handler will create MMIO mappings to access the >> clock registers and enable the clock driver to expose the clock >> for use of drivers which will connect to it. >> >> Signed-off-by: Akshu Agrawal >> --- >> v2: Submitted with dependent patch, removed unneeded kfree for devm_kzalloc > > Well, where's patch [1/2]? > It's here: https://patchwork.kernel.org/patch/10380207/ Regards, Akshu > Thanks, > Rafael >
Re: [PATCH v2 2/2] ACPI: APD: Add AMD misc clock handler support
On 5/4/2018 3:45 PM, Rafael J. Wysocki wrote: > On Fri, May 4, 2018 at 12:09 PM, Agrawal, Akshu wrote: >> >> >> On 5/4/2018 3:36 PM, Rafael J. Wysocki wrote: >>> On Friday, May 4, 2018 10:34:44 AM CEST Akshu Agrawal wrote: >>>> AMD SoC exposes clock for general purpose use. The clock registration >>>> is done in clk-st driver. The MMIO mapping are passed on to the >>>> clock driver for accessing the registers. >>>> The misc clock handler will create MMIO mappings to access the >>>> clock registers and enable the clock driver to expose the clock >>>> for use of drivers which will connect to it. >>>> >>>> Signed-off-by: Akshu Agrawal >>>> --- >>>> v2: Submitted with dependent patch, removed unneeded kfree for devm_kzalloc >>> >>> Well, where's patch [1/2]? >>> >> >> It's here: >> https://patchwork.kernel.org/patch/10380207/ > > So can you please send them both as a series with the same CC list, > add all of the relevant maintainers to that CC list and indicate whom > you expect to take care of these patches? > Ok sure, I will send both the patches to combined cc list from ./scripts/get_maintainer.pl for each patch. Regards, Akshu > I think that they should go in together as they are clearly related to > each other. > > Thanks, > Rafael >
Re: [PATCH v2 2/2] ACPI: APD: Add AMD misc clock handler support
On 5/4/2018 4:37 PM, Rafael J. Wysocki wrote: > On Fri, May 4, 2018 at 12:23 PM, Agrawal, Akshu wrote: >> >> >> On 5/4/2018 3:45 PM, Rafael J. Wysocki wrote: >>> On Fri, May 4, 2018 at 12:09 PM, Agrawal, Akshu >>> wrote: >>>> >>>> >>>> On 5/4/2018 3:36 PM, Rafael J. Wysocki wrote: >>>>> On Friday, May 4, 2018 10:34:44 AM CEST Akshu Agrawal wrote: >>>>>> AMD SoC exposes clock for general purpose use. The clock registration >>>>>> is done in clk-st driver. The MMIO mapping are passed on to the >>>>>> clock driver for accessing the registers. >>>>>> The misc clock handler will create MMIO mappings to access the >>>>>> clock registers and enable the clock driver to expose the clock >>>>>> for use of drivers which will connect to it. >>>>>> >>>>>> Signed-off-by: Akshu Agrawal >>>>>> --- >>>>>> v2: Submitted with dependent patch, removed unneeded kfree for >>>>>> devm_kzalloc >>>>> >>>>> Well, where's patch [1/2]? >>>>> >>>> >>>> It's here: >>>> https://patchwork.kernel.org/patch/10380207/ >>> >>> So can you please send them both as a series with the same CC list, >>> add all of the relevant maintainers to that CC list and indicate whom >>> you expect to take care of these patches? >>> >> >> Ok sure, I will send both the patches to combined cc list from >> ./scripts/get_maintainer.pl for each patch. > > It also would be good to add a cover letter with an outline of all of > the changes together. > Done. I have sent the patches to combined mailing list, with a supporting cover letter. Regards, Akshu
Re: [PATCH] drm/amdgpu/acp: Fix slab-out-of-bounds in mfd_add_device in acp_hw_init
On 4/13/2018 9:45 PM, Daniel Kurtz wrote:
Commit 51f7415039d4 ("drm/amd/amdgpu: creating two I2S instances for
stoney/cz") added support for the "BT_I2S" ACP i2s channel. As part of
this change, one additional acp resource was added, but the "num_resource"
count was accidentally incremented by 2.
This incorrect count eventually causes mfd_add_device() to try to access
an invalid memory address (the location of non-existent resource 5.
This fault was detected by running a KASAN enabled kernel, which produced
the following splat at boot:
[6.612987]
==
[6.613509] BUG: KASAN: slab-out-of-bounds in mfd_add_device+0x4bc/0x7a7
[6.613509] Read of size 8 at addr 880107d4dc58 by task swapper/0/1
[6.613509]
[6.613509] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.14.33 #349
[6.613509] Hardware name: Google Grunt/Grunt, BIOS
Google_Grunt.10543.0.2018_04_03_1812 04/02/2018
[6.613509] Call Trace:
[6.613509] dump_stack+0x4d/0x63
[6.613509] print_address_description+0x80/0x2d6
[6.613509] ? mfd_add_device+0x4bc/0x7a7
[6.613509] kasan_report+0x255/0x295
[6.613509] mfd_add_device+0x4bc/0x7a7
[6.613509] ? kasan_kmalloc+0x99/0xa8
[6.613509] ? mfd_add_devices+0x58/0xe4
[6.613509] ? __kmalloc+0x154/0x178
[6.613509] mfd_add_devices+0xa5/0xe4
[6.613509] acp_hw_init+0x92e/0xc4a
[6.613509] amdgpu_device_init+0x1dfb/0x22a2
[6.613509] ? kmalloc_order+0x53/0x5d
[6.613509] ? kmalloc_order_trace+0x23/0xb3
[6.613509] amdgpu_driver_load_kms+0xce/0x267
[6.613509] drm_dev_register+0x169/0x2fb
[6.613509] amdgpu_pci_probe+0x217/0x242
[6.613509] pci_device_probe+0x101/0x18e
[6.613509] driver_probe_device+0x1dd/0x419
[6.613509] ? ___might_sleep+0x80/0x1b6
[6.613509] __driver_attach+0x9f/0xc9
[6.613509] ? driver_probe_device+0x419/0x419
[6.613509] bus_for_each_dev+0xbc/0xe1
[6.613509] bus_add_driver+0x189/0x2c0
[6.613509] driver_register+0x108/0x156
[6.613509] ? ttm_init+0x67/0x67
[6.613509] do_one_initcall+0xb2/0x161
[6.613509] kernel_init_freeable+0x25a/0x308
[6.613509] ? rest_init+0xcc/0xcc
[6.613509] kernel_init+0x11/0x10d
[6.613509] ? rest_init+0xcc/0xcc
[6.613509] ret_from_fork+0x22/0x40
[6.613509]
[6.613509] Allocated by task 1:
[6.613509] save_stack+0x46/0xce
[6.613509] kasan_kmalloc+0x99/0xa8
[6.613509] kmem_cache_alloc_trace+0x11a/0x13e
[6.613509] acp_hw_init+0x210/0xc4a
[6.613509] amdgpu_device_init+0x1dfb/0x22a2
[6.613509] amdgpu_driver_load_kms+0xce/0x267
[6.613509] drm_dev_register+0x169/0x2fb
[6.613509] amdgpu_pci_probe+0x217/0x242
[6.613509] pci_device_probe+0x101/0x18e
[6.613509] driver_probe_device+0x1dd/0x419
[6.613509] __driver_attach+0x9f/0xc9
[6.613509] bus_for_each_dev+0xbc/0xe1
[6.613509] bus_add_driver+0x189/0x2c0
[6.613509] driver_register+0x108/0x156
[6.613509] do_one_initcall+0xb2/0x161
[6.613509] kernel_init_freeable+0x25a/0x308
[6.613509] kernel_init+0x11/0x10d
[6.613509] ret_from_fork+0x22/0x40
[6.613509]
[6.613509] Freed by task 0:
[6.613509] (stack is not available)
[6.613509]
[6.613509] The buggy address belongs to the object at 880107d4db08
[6.613509] which belongs to the cache kmalloc-512 of size 512
[6.613509] The buggy address is located 336 bytes inside of
[6.613509] 512-byte region [880107d4db08, 880107d4dd08)
[6.613509] The buggy address belongs to the page:
[6.613509] page:ea00041f5300 count:1 mapcount:0 mapping:
(null) index:0x0 compound_mapcount: 0
[6.613509] flags: 0x80008100(slab|head)
[6.613509] raw: 80008100
000100120012
[6.613509] raw: ea0004208520 88010b001680 88010b002cc0
[6.613509] page dumped because: kasan: bad access detected
[6.613509]
[6.613509] Memory state around the buggy address:
[6.613509] 880107d4db00: fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00
[6.613509] 880107d4db80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00
[6.613509] >880107d4dc00: 00 00 00 00 00 00 00 00 00 fc fc fc fc fc fc
fc
[6.613509] ^
[6.613509] 880107d4dc80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
fc
[6.613509] 880107d4dd00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
fc
[6.613509]
==
Fixes: 51f7415039d4 ("drm/amd/amdgpu: creating two I2S instances for stoney/cz")
Signed-off-by: Daniel Kurtz
Acked-by: Akshu Agrawal
---
drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ac
Re: [PATCH 4/4] ASoC: amd: enabling bt i2s config after acp reset
On 4/17/2018 10:29 AM, Vijendar Mukunda wrote:
On ST/CZ based platforms, for specific platform bt uart
mux to be defined for bt i2s.
By default, these pins will be used for uart.
After acp reset , it requires to reprogram bt i2s config
mux pins to enable bt i2s instance.
added bt i2s enablement sequence during acp init.
Signed-off-by: Vijendar Mukunda
Signed-off-by: Akshu Agrawal
---
sound/soc/amd/acp-da7219-max98357a.c | 2 ++
sound/soc/amd/acp-pcm-dma.c | 9 +
sound/soc/amd/acp.h | 1 +
3 files changed, 12 insertions(+)
diff --git a/sound/soc/amd/acp-da7219-max98357a.c
b/sound/soc/amd/acp-da7219-max98357a.c
index b205c78..6dad0cb 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -44,6 +44,7 @@
static struct snd_soc_jack cz_jack;
struct clk *da7219_dai_clk;
+extern int bt_pad_enable;
static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
{
@@ -81,6 +82,7 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
}
da7219_aad_jack_det(component, &cz_jack);
+ bt_pad_enable = device_property_read_bool(&pdev->dev, "bt-pad-enable");
This is to be done in probe.
return 0;
}
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 7c392fe..b52c660 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -46,6 +46,8 @@
#define DRV_NAME "acp_audio_dma"
+bool bt_pad_enable = false;
+EXPORT_SYMBOL(bt_pad_enable);
static const struct snd_pcm_hardware acp_pcm_hardware_playback = {
.info = SNDRV_PCM_INFO_INTERLEAVED |
@@ -525,6 +527,13 @@ static int acp_init(void __iomem *acp_mmio, u32 asic_type)
val &= ~ACP_SOFT_RESET__SoftResetAud_MASK;
acp_reg_write(val, acp_mmio, mmACP_SOFT_RESET);
+ /* For BT instance change pins from UART to BT */
+ if (bt_pad_enable) {
+ val = acp_reg_read(acp_mmio, mmACP_BT_UART_PAD_SEL);
+ val |= ACP_BT_UART_PAD_SELECT_MASK;
+ acp_reg_write(val, acp_mmio, mmACP_BT_UART_PAD_SEL);
+ }
+
/* initiailize Onion control DAGB register */
acp_reg_write(ACP_ONION_CNTL_DEFAULT, acp_mmio,
mmACP_AXI2DAGB_ONION_CNTL);
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 460365c..6b43144 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -107,6 +107,7 @@
#define ACP_I2S_MIC_16BIT_RESOLUTION_EN 0x01
#define ACP_I2S_SP_16BIT_RESOLUTION_EN0x02
#define ACP_I2S_BT_16BIT_RESOLUTION_EN0x04
+#define ACP_BT_UART_PAD_SELECT_MASK0x1
enum acp_dma_priority_level {
/* 0x0 Specifies the DMA channel is given normal priority */
Re: [PATCH v4 1/2] clk: x86: Add ST oscout platform clock
On 5/8/2018 9:08 PM, Deucher, Alexander wrote: >> -Original Message- >> From: Agrawal, Akshu >> Sent: Tuesday, May 8, 2018 12:04 AM >> To: Deucher, Alexander >> Cc: djku...@chromium.org; mturque...@baylibre.com; sb...@kernel.org; >> Koenig, Christian ; airl...@redhat.com; Liu, >> Shaoyun ; linux-kernel@vger.kernel.org; linux- >> c...@vger.kernel.org; r...@rjwysocki.net; l...@kernel.org; linux- >> a...@vger.kernel.org >> Subject: Re: [PATCH v4 1/2] clk: x86: Add ST oscout platform clock >> >> >> >> On 5/8/2018 3:14 AM, Deucher, Alexander wrote: >>>> -Original Message- >>>> From: Agrawal, Akshu >>>> Sent: Monday, May 7, 2018 6:14 AM >>>> Cc: djku...@chromium.org; Agrawal, Akshu ; >>>> Deucher, Alexander ; >>>> mturque...@baylibre.com; sb...@kernel.org; Koenig, Christian >>>> ; airl...@redhat.com; Liu, Shaoyun >>>> ; linux-kernel@vger.kernel.org; linux- >>>> c...@vger.kernel.org; r...@rjwysocki.net; l...@kernel.org; linux- >>>> a...@vger.kernel.org >>>> Subject: [PATCH v4 1/2] clk: x86: Add ST oscout platform clock >>>> >>>> Stoney SoC provides oscout clock. This clock can support 25Mhz and >>>> 48Mhz of frequency. >>>> The clock is available for general system use. >>>> >>>> Signed-off-by: Akshu Agrawal >>>> --- >>>> v2: config change, added SPDX tag and used clk_hw_register_. >>>> v3: Fix kbuild warning for checking of NULL pointer >>>> v4: unregister clk_hw in driver remove, add .suppress_bind_attrs >>>> drivers/clk/x86/Makefile | 3 +- >>>> drivers/clk/x86/clk-st.c | 85 >>>> >>>> include/linux/platform_data/clk-st.h | 17 >>>> 3 files changed, 104 insertions(+), 1 deletion(-) create mode >>>> 100644 drivers/clk/x86/clk-st.c create mode 100644 >>>> include/linux/platform_data/clk-st.h >>>> >>>> diff --git a/drivers/clk/x86/Makefile b/drivers/clk/x86/Makefile >>>> index 1367afb..00303bc 100644 >>>> --- a/drivers/clk/x86/Makefile >>>> +++ b/drivers/clk/x86/Makefile >>>> @@ -1,3 +1,4 @@ >>>> +obj-$(CONFIG_PMC_ATOM)+= clk-pmc-atom.o >>>> +obj-$(CONFIG_X86_AMD_PLATFORM_DEVICE) += clk-st.o >>>> clk-x86-lpss-objs := clk-lpt.o >>>> obj-$(CONFIG_X86_INTEL_LPSS) += clk-x86-lpss.o >>>> -obj-$(CONFIG_PMC_ATOM)+= clk-pmc-atom.o >>>> diff --git a/drivers/clk/x86/clk-st.c b/drivers/clk/x86/clk-st.c new >>>> file mode >>>> 100644 index 000..8a7795c >>>> --- /dev/null >>>> +++ b/drivers/clk/x86/clk-st.c >>>> @@ -0,0 +1,85 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>> >>> Should this be MIT? The original license was MIT. >>> >>> Alex >>> >> >> We are adding SPDX tag, while license remains same GPL-2.0 >> >> What I have read is this is "to provide license identifiers inside the source >> code that could be easily parsed by machines and would allow checking for >> license compliance of an open source project easier." > > My point as just that the original license on the file that you first sent > out was MIT so the SPDX tag should be MIT rather than GPL. E.g., > SPDX-License-Identifier: MIT > Oh right it should be MIT, will change it. Thanks, Akshu
Re: [PATCH v3] ASoC: da7219: read fmw property to get mclk for non-dts systems
On 5/4/2018 2:45 PM, Adam Thomson wrote:
> On 03 May 2018 08:59, Akshu Agrawal wrote:
>
>> Non-dts based systems can use ACPI DSDT to pass on the mclk
>> to da7219.
>> This enables da7219 mclk to be linked to system clock.
>> Enable/Disable of the mclk is already handled in the codec so
>> platform drivers don't have to explicitly do handling of mclk.
>>
>> Signed-off-by: Akshu Agrawal
>> ---
>> v2: Fixed kbuild error
>> v3: Add corresponding clk_put for clk_get
>> include/sound/da7219.h| 2 ++
>> sound/soc/codecs/da7219.c | 10 +-
>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/sound/da7219.h b/include/sound/da7219.h
>> index 1bfcb16..df7ddf4 100644
>> --- a/include/sound/da7219.h
>> +++ b/include/sound/da7219.h
>> @@ -38,6 +38,8 @@ struct da7219_pdata {
>>
>> const char *dai_clks_name;
>>
>> +const char *mclk_name;
>> +
>> /* Mic */
>> enum da7219_micbias_voltage micbias_lvl;
>> enum da7219_mic_amp_in_sel mic_amp_in_sel;
>> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
>> index 980a6a8..ecd46fc 100644
>> --- a/sound/soc/codecs/da7219.c
>> +++ b/sound/soc/codecs/da7219.c
>> @@ -1624,6 +1624,8 @@ static struct da7219_pdata *da7219_fw_to_pdata(struct
>> snd_soc_component *compone
>> dev_warn(dev, "Using default clk name: %s\n",
>> pdata->dai_clks_name);
>>
>> +device_property_read_string(dev, "dlg,mclk-name", &pdata->mclk_name);
>> +
>
> Personally am still not keen on this. To me the use of a device_property_*
> function suggests the same property resides in both DT and ACPI, but here
> we're
> only using this for the ACPI case. DT has no want or need for this. I still
> feel
> we should look at something more generic in the clock framework, although I do
> agree with Mark that this should be properly specced.
>
I am not an expert in field of ACPI, IMO forming a Spec and changing
ACPI to have DT like clock framework is good to have but a bigger change
which should be taken up later.
The current code of handling of mclk in the driver is usable only by DT.
The device_property (though ACPI specific) makes this code, a common
code for DT and ACPI based devices.
https://www.kernel.org/doc/Documentation/acpi/DSD-properties-rules.txt
"Still, for the sake of code re-use, it may make sense to provide as
much of the configuration data as possible in the form of device
properties and complement that with an ACPI-specific mechanism suitable
for the use case at hand.."
Thanks,
Akshu
Re: [PATCH v2] clk: x86: Add ST oscout platform clock
On 5/5/2018 7:56 AM, Stephen Boyd wrote:
> Quoting Akshu Agrawal (2018-05-03 01:30:26)
>> diff --git a/drivers/clk/x86/Makefile b/drivers/clk/x86/Makefile
>> index 1367afb..2aee002 100644
>> --- a/drivers/clk/x86/Makefile
>> +++ b/drivers/clk/x86/Makefile
>> @@ -1,3 +1,4 @@
>> clk-x86-lpss-objs := clk-lpt.o
>> obj-$(CONFIG_X86_INTEL_LPSS) += clk-x86-lpss.o
>> obj-$(CONFIG_PMC_ATOM) += clk-pmc-atom.o
>> +obj-$(CONFIG_X86_AMD_PLATFORM_DEVICE) += clk-st.o
>
> Ok. Can you sort this by kconfig? Or by file name?
>
Accepted.
>> diff --git a/drivers/clk/x86/clk-st.c b/drivers/clk/x86/clk-st.c
>> new file mode 100644
>> index 000..d8c283c
>> --- /dev/null
>> +++ b/drivers/clk/x86/clk-st.c
>> @@ -0,0 +1,86 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * clock framework for AMD Stoney based clocks
>> + *
>> + * Copyright 2018 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the
>> "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>
> One point of SPDX is to avoid this boiler plate multi-line license
> comments. Can you remove this and just leave the AMD copyright part?
>
Accepted.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +/* Clock Driving Strength 2 register */
>> +#define CLKDRVSTR2 0x28
>> +/* Clock Control 1 register */
>> +#define MISCCLKCNTL1 0x40
>> +/* Auxiliary clock1 enable bit */
>> +#define OSCCLKENB 2
>> +/* 25Mhz auxiliary output clock freq bit */
>> +#define OSCOUT1CLK25MHZ16
>> +
>> +#define ST_CLK_48M 0
>> +#define ST_CLK_25M 1
>> +#define ST_CLK_MUX 2
>> +#define ST_CLK_GATE3
>> +#define ST_MAX_CLKS4
>> +
>> +static const char * const clk_oscout1_parents[] = { "clk48MHz", "clk25MHz"
>> };
>> +
>> +static int st_clk_probe(struct platform_device *pdev)
>> +{
>> + struct st_clk_data *st_data;
>> + struct clk_hw **hws;
>> +
>> + st_data = dev_get_platdata(&pdev->dev);
>> + if (!st_data || !st_data->base)
>> + return -EINVAL;
>> +
>> + hws = kzalloc(sizeof(*hws) * ST_MAX_CLKS, GFP_KERNEL);
>
> Fix the kbuild robot errors please.
>
Done in v3.
>> +
>> + hws[ST_CLK_48M] = clk_hw_register_fixed_rate(NULL, "clk48MHz", NULL,
>> 0,
>> +4800);
>> + hws[ST_CLK_25M] = clk_hw_register_fixed_rate(NULL, "clk25MHz", NULL,
>> 0,
>> +2500);
>
> I'm not sure why we even keep these pointers around though. The driver
> doesn't expose them as clks that clk_get() can find so they could just
> be local variables and no heap allocation is needed.
>
Respining the patch with change to unregister clk_hw in the remove callback.
Will pass these pointers as drvdata to remove and use them to unregister.
>> +
>> + hws[ST_CLK_MUX] = clk_hw_register_mux(NULL, "oscout1_mux",
>> + clk_oscout1_parents, ARRAY_SIZE(clk_oscout1_parents),
>> + 0, st_data->base + CLKDRVSTR2, OSCOUT1CLK25MHZ, 3, 0, NULL);
>> +
>> + clk_set_parent(hws[ST_CLK_MUX]->clk, hws[ST_CLK_25M]->clk);
>> +
>> + hws[ST_CLK_GATE] = clk_hw_register_gate(NULL, "oscout1",
>> "oscout1_mux",
>> + 0, st_data->base + MISCCLKCNTL1, OSCCLKENB,
>> + CLK_GATE_SET_TO_DISABLE, NULL);
>> +
>> + clk_hw_register_clkdev(hws[ST_CLK_GATE], "oscout1", NULL);
>
> Could use devm_*() here in case you want to drop this stuff on driver
> removal?
>
To achieve the same what you have suggested, I would be using remove as
explained in above comment.
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver st_clk_driver = {
>> + .driver = {
>> + .name = "clk-st",
>> + },
>> + .probe = st_clk_probe,
>
> suppress attributes here to prevent unbinding from sysfs?
>
Accepted.
Thanks,
Akshu
Re: [PATCH v4 1/2] clk: x86: Add ST oscout platform clock
On 5/8/2018 3:14 AM, Deucher, Alexander wrote: >> -Original Message- >> From: Agrawal, Akshu >> Sent: Monday, May 7, 2018 6:14 AM >> Cc: djku...@chromium.org; Agrawal, Akshu ; >> Deucher, Alexander ; >> mturque...@baylibre.com; sb...@kernel.org; Koenig, Christian >> ; airl...@redhat.com; Liu, Shaoyun >> ; linux-kernel@vger.kernel.org; linux- >> c...@vger.kernel.org; r...@rjwysocki.net; l...@kernel.org; linux- >> a...@vger.kernel.org >> Subject: [PATCH v4 1/2] clk: x86: Add ST oscout platform clock >> >> Stoney SoC provides oscout clock. This clock can support 25Mhz and 48Mhz >> of frequency. >> The clock is available for general system use. >> >> Signed-off-by: Akshu Agrawal >> --- >> v2: config change, added SPDX tag and used clk_hw_register_. >> v3: Fix kbuild warning for checking of NULL pointer >> v4: unregister clk_hw in driver remove, add .suppress_bind_attrs >> drivers/clk/x86/Makefile | 3 +- >> drivers/clk/x86/clk-st.c | 85 >> >> include/linux/platform_data/clk-st.h | 17 >> 3 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 >> drivers/clk/x86/clk-st.c create mode 100644 >> include/linux/platform_data/clk-st.h >> >> diff --git a/drivers/clk/x86/Makefile b/drivers/clk/x86/Makefile index >> 1367afb..00303bc 100644 >> --- a/drivers/clk/x86/Makefile >> +++ b/drivers/clk/x86/Makefile >> @@ -1,3 +1,4 @@ >> +obj-$(CONFIG_PMC_ATOM) += clk-pmc-atom.o >> +obj-$(CONFIG_X86_AMD_PLATFORM_DEVICE) += clk-st.o >> clk-x86-lpss-objs := clk-lpt.o >> obj-$(CONFIG_X86_INTEL_LPSS)+= clk-x86-lpss.o >> -obj-$(CONFIG_PMC_ATOM) += clk-pmc-atom.o >> diff --git a/drivers/clk/x86/clk-st.c b/drivers/clk/x86/clk-st.c new file >> mode >> 100644 index 000..8a7795c >> --- /dev/null >> +++ b/drivers/clk/x86/clk-st.c >> @@ -0,0 +1,85 @@ >> +// SPDX-License-Identifier: GPL-2.0 > > Should this be MIT? The original license was MIT. > > Alex > We are adding SPDX tag, while license remains same GPL-2.0 What I have read is this is "to provide license identifiers inside the source code that could be easily parsed by machines and would allow checking for license compliance of an open source project easier." Thanks, Akshu
Re: [PATCH] ASoC: AMD: Use mixer control to switch between DMICs
On 5/27/2020 4:57 PM, Mark Brown wrote:
On Wed, May 27, 2020 at 07:10:16AM +0530, Akshu Agrawal wrote:
+ SOC_SINGLE_BOOL_EXT("Front Mic", 0, front_mic_get, front_mic_set),
This should probably be a mux with two labelled options, or if it's a
boolean control it should end in Switch. A mux definitely seems like a
better option though.
Actually it's a dmic switch, so will change it to boolean control named
"DMIC switch". Front or rear mic might change with variants.
Thanks,
Akshu
Re: [PATCH] ASoC: rt5682: Register clocks even when mclk is NULL
On 6/12/2020 10:04 PM, Mark Brown wrote: On Fri, Jun 12, 2020 at 10:01:11PM +0530, Akshu Agrawal wrote: Fixes kernel crash on platforms which do not have mclk exposed in CCF framework. For these platforms have mclk as NULL and continue to register clocks. Derek already submitted this: https://lore.kernel.org/alsa-devel/1591938925-1070-5-git-send-email-derek.f...@realtek.com/T/#u Ok great. Thanks.
Re: [PATCH] ASoC: AMD: Clean kernel log from deferred probe error messages
On 8/26/2020 4:54 PM, Mark Brown wrote: On Wed, Aug 26, 2020 at 04:48:05PM +0530, Akshu Agrawal wrote: While the driver waits for DAIs to be probed and retries probing, avoid printing error messages. This means that if there is a problem with deferred probe no diagnostics will be available, there should be something at least at debug level. Sure will add a message at debug level for deferred probe. Thanks, Akshu
Re: [PATCH 2/5] clk: x86: Change name from ST to FCH
On 7/16/2020 6:12 AM, Stephen Boyd wrote: Quoting Akshu Agrawal (2020-07-12 17:59:50) diff --git a/drivers/clk/x86/clk-st.c b/drivers/clk/x86/clk-fch.c similarity index 73% rename from drivers/clk/x86/clk-st.c rename to drivers/clk/x86/clk-fch.c index 25d4b97aff9b..b252f0cf0628 100644 --- a/drivers/clk/x86/clk-st.c +++ b/drivers/clk/x86/clk-fch.c @@ -8,7 +8,7 @@ #include #include #include -#include +#include Is this file moved somewhere? Renaming it from ST to FCH in previous patch of the series. Thanks, Akshu
Re: [PATCH 4/5] clk: x86: Support RV architecture
On 7/16/2020 6:33 AM, Stephen Boyd wrote:
Quoting Akshu Agrawal (2020-07-12 17:59:52)
There is minor difference between previous family of SoC and
the current one. Which is the there is only 48Mh fixed clk.
There is no mux and no option to select another freq as there in previous.
Signed-off-by: Akshu Agrawal
---
I only see four out of five patches and there isn't a cover letter. I
have no idea if I can apply this change or if you're expecting me to ack
it. Please help make my life a little easier!
Numbering went wrong due to another unrelated patch.
Will send another with cover letter explaining the series.
drivers/clk/x86/clk-fch.c | 55 ---
1 file changed, 40 insertions(+), 15 deletions(-)
diff --git a/drivers/clk/x86/clk-fch.c b/drivers/clk/x86/clk-fch.c
index b252f0cf0628..a8aac71a3b65 100644
--- a/drivers/clk/x86/clk-fch.c
+++ b/drivers/clk/x86/clk-fch.c
@@ -61,9 +78,17 @@ static int fch_clk_probe(struct platform_device *pdev)
static int fch_clk_remove(struct platform_device *pdev)
{
int i;
+ struct fch_clk_data *fch_data;
+
+ fch_data = dev_get_platdata(&pdev->dev);
- for (i = 0; i < ST_MAX_CLKS; i++)
- clk_hw_unregister(hws[i]);
+ if (!fch_data->is_rv) {
+ for (i = 0; i < ST_MAX_CLKS; i++)
+ clk_hw_unregister(hws[i]);
+ } else {
+ for (i = 0; i < RV_MAX_CLKS; i++)
+ clk_hw_unregister(hws[i]);
Can ST_MAX_CLKS or RV_MAX_CLKS be a local variable and then the loop
consolidated.
Yes, making the change in next series.
Thanks,
Akshu
Re: [PATCH 1/4] ACPI: APD: Change name from ST to FCH
On 7/27/2020 6:58 PM, Rafael J. Wysocki wrote:
On Mon, Jul 20, 2020 at 7:06 AM Akshu Agrawal wrote:
AMD SoC general pupose clk is present in new platforms with
same MMIO mappings. We can reuse the same clk handler support
for other platforms. Hence, changing name from ST(SoC) to FCH(IP)
Signed-off-by: Akshu Agrawal
This patch and the [3/4] appear to be part of a larger series which
isn't visible to me as a whole.
Link to other patches:
https://patchwork.kernel.org/patch/11672857/
https://patchwork.kernel.org/patch/11672861/
Do you want me to apply them nevertheless?
This patch on its own will cause compilation error as we need the second
patch.
Since, there is dependency we need them to be merged together. Can you
or Stephen please suggest a way forward.
Thanks,
Akshu
---
drivers/acpi/acpi_apd.c| 14 +++---
.../linux/platform_data/{clk-st.h => clk-fch.h}| 10 +-
2 files changed, 12 insertions(+), 12 deletions(-)
rename include/linux/platform_data/{clk-st.h => clk-fch.h} (53%)
diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
index ba2612e9a0eb..2d99e46add1a 100644
--- a/drivers/acpi/acpi_apd.c
+++ b/drivers/acpi/acpi_apd.c
@@ -8,7 +8,7 @@
*/
#include
-#include
+#include
#include
#include
#include
@@ -79,11 +79,11 @@ static int misc_check_res(struct acpi_resource *ares, void
*data)
return !acpi_dev_resource_memory(ares, &res);
}
-static int st_misc_setup(struct apd_private_data *pdata)
+static int fch_misc_setup(struct apd_private_data *pdata)
{
struct acpi_device *adev = pdata->adev;
struct platform_device *clkdev;
- struct st_clk_data *clk_data;
+ struct fch_clk_data *clk_data;
struct resource_entry *rentry;
struct list_head resource_list;
int ret;
@@ -106,7 +106,7 @@ static int st_misc_setup(struct apd_private_data *pdata)
acpi_dev_free_resource_list(&resource_list);
- clkdev = platform_device_register_data(&adev->dev, "clk-st",
+ clkdev = platform_device_register_data(&adev->dev, "clk-fch",
PLATFORM_DEVID_NONE, clk_data,
sizeof(*clk_data));
return PTR_ERR_OR_ZERO(clkdev);
@@ -135,8 +135,8 @@ static const struct apd_device_desc cz_uart_desc = {
.properties = uart_properties,
};
-static const struct apd_device_desc st_misc_desc = {
- .setup = st_misc_setup,
+static const struct apd_device_desc fch_misc_desc = {
+ .setup = fch_misc_setup,
};
#endif
@@ -239,7 +239,7 @@ static const struct acpi_device_id acpi_apd_device_ids[] = {
{ "AMD0020", APD_ADDR(cz_uart_desc) },
{ "AMDI0020", APD_ADDR(cz_uart_desc) },
{ "AMD0030", },
- { "AMD0040", APD_ADDR(st_misc_desc)},
+ { "AMD0040", APD_ADDR(fch_misc_desc)},
#endif
#ifdef CONFIG_ARM64
{ "APMC0D0F", APD_ADDR(xgene_i2c_desc) },
diff --git a/include/linux/platform_data/clk-st.h
b/include/linux/platform_data/clk-fch.h
similarity index 53%
rename from include/linux/platform_data/clk-st.h
rename to include/linux/platform_data/clk-fch.h
index 7cdb6a402b35..850ca776156d 100644
--- a/include/linux/platform_data/clk-st.h
+++ b/include/linux/platform_data/clk-fch.h
@@ -1,17 +1,17 @@
/* SPDX-License-Identifier: MIT */
/*
- * clock framework for AMD Stoney based clock
+ * clock framework for AMD misc clocks
*
* Copyright 2018 Advanced Micro Devices, Inc.
*/
-#ifndef __CLK_ST_H
-#define __CLK_ST_H
+#ifndef __CLK_FCH_H
+#define __CLK_FCH_H
#include
-struct st_clk_data {
+struct fch_clk_data {
void __iomem *base;
};
-#endif /* __CLK_ST_H */
+#endif /* __CLK_FCH_H */
--
2.20.1
Re: [v2 4/4] clk: x86: Support RV architecture
On 7/21/2020 1:35 PM, Stephen Boyd wrote:
Quoting Akshu Agrawal (2020-07-19 22:04:59)
diff --git a/drivers/clk/x86/clk-fch.c b/drivers/clk/x86/clk-fch.c
index b252f0cf0628..d68bca7b213f 100644
--- a/drivers/clk/x86/clk-fch.c
+++ b/drivers/clk/x86/clk-fch.c
[...]
static int fch_clk_remove(struct platform_device *pdev)
{
- int i;
+ int i, clks;
+ struct fch_clk_data *fch_data;
- for (i = 0; i < ST_MAX_CLKS; i++)
+ fch_data = dev_get_platdata(&pdev->dev);
+
+ clks = (!fch_data->is_rv) ? ST_MAX_CLKS : RV_MAX_CLKS;
Useless parenthesis and negation, just write:
num = fch_data->is_rv ? RV_MAX_CLKS : ST_MAX_CLKS;
Sure, including in the next patch set.
Thanks,
Akshu
+
+ for (i = 0; i < clks; i++)
clk_hw_unregister(hws[i]);
+
return 0;
}
Re: [PATCH 2/4] clk: x86: Change name from ST to FCH
On 7/21/2020 1:36 PM, Stephen Boyd wrote: Quoting Akshu Agrawal (2020-07-19 22:04:57) diff --git a/drivers/clk/x86/clk-st.c b/drivers/clk/x86/clk-fch.c similarity index 73% rename from drivers/clk/x86/clk-st.c rename to drivers/clk/x86/clk-fch.c index 25d4b97aff9b..b252f0cf0628 100644 --- a/drivers/clk/x86/clk-st.c +++ b/drivers/clk/x86/clk-fch.c @@ -8,7 +8,7 @@ #include #include #include -#include +#include As stated before, this should go with the patch that moves the file. +Rafael Sorry for late reply, somehow I missed these mails. Will move it to acpi: apd change Thanks, Akshu
Re: [PATCH] ASoC: rt5682: Add fmw property to get name of mclk
On 7/7/2020 4:00 PM, Mark Brown wrote: On Tue, Jul 07, 2020 at 03:38:25PM +0530, Akshu Agrawal wrote: Non-dts based systems can use ACPI DSDT to pass on the mclk. Thus add fmw property mclk-name to get the name of the system clk and link it to rt5682 mclk. ACPI doesn't support clocks at all, you need to add a clock binding to ACPI first. The idiomatic way to do this would be to have board specific quirks. clk binding is present for AMD ST platform and using the same. With recent submitted patches I am making them generic for all AMD platforms. Please refer patches: https://patchwork.kernel.org/patch/11658505/ https://patchwork.kernel.org/patch/11658507/ Thanks, Akshu + device_property_read_string(dev, "realtek,mclk-name", &rt5682->pdata.mclk_name); + No, this is not at all OK - you're adding this via a device property which means that this will show up in the DT bindings too.
Re: linux-next: build failure after merge of the sound-asoc tree
On 2/19/2018 5:02 AM, Stephen Rothwell wrote:
Hi all,
After merging the sound-asoc tree, today's linux-next build (x86_64
allmodconfig) failed like this:
sound/soc/amd/acp-da7219-max98357a.c: In function 'cz_da7219_init':
sound/soc/amd/acp-da7219-max98357a.c:79:22: error: passing argument 1 of
'da7219_aad_jack_det' from incompatible pointer type
[-Werror=incompatible-pointer-types]
da7219_aad_jack_det(codec, &cz_jack);
^
In file included from sound/soc/amd/acp-da7219-max98357a.c:38:0:
sound/soc/amd/../codecs/da7219-aad.h:209:6: note: expected 'struct
snd_soc_component *' but argument is of type 'struct snd_soc_codec *'
void da7219_aad_jack_det(struct snd_soc_component *component, struct
snd_soc_jack *jack);
^~~
cc1: some warnings being treated as errors
sound/soc/intel/boards/kbl_da7219_max98357a.c: In function
'kabylake_da7219_codec_init':
sound/soc/intel/boards/kbl_da7219_max98357a.c:194:22: error: passing argument 1
of 'da7219_aad_jack_det' from incompatible pointer type
[-Werror=incompatible-pointer-types]
da7219_aad_jack_det(codec, &ctx->kabylake_headset);
^
In file included from sound/soc/intel/boards/kbl_da7219_max98357a.c:23:0:
sound/soc/intel/boards/../../codecs/da7219-aad.h:209:6: note: expected 'struct
snd_soc_component *' but argument is of type 'struct snd_soc_codec *'
void da7219_aad_jack_det(struct snd_soc_component *component, struct
snd_soc_jack *jack);
^~~
sound/soc/intel/boards/kbl_da7219_max98357a.c: In function
'kabylake_card_late_probe':
sound/soc/intel/boards/kbl_da7219_max98357a.c:552:34: error: passing argument 1
of 'hdac_hdmi_jack_port_init' from incompatible pointer type
[-Werror=incompatible-pointer-types]
return hdac_hdmi_jack_port_init(codec, &card->dapm);
^
In file included from sound/soc/intel/boards/kbl_da7219_max98357a.c:21:0:
sound/soc/intel/boards/../../codecs/hdac_hdmi.h:8:5: note: expected 'struct
snd_soc_component *' but argument is of type 'struct snd_soc_codec *'
int hdac_hdmi_jack_port_init(struct snd_soc_component *component,
^~~~
Caused by commits
608a300fc1f0 ("ASoC: AMD: Add machine driver for ST DA7219 MAX98357")
b3ea70ee64ea ("ASoC: Intel: Add Kabylake-y Dialog Maxim machine driver")
interacting with commit
451011221711 ("ASoC: hdac_hdmi/nau8825/rt286/rt298/rt5663/da7219: replace codec
to component")
I have used the sound-asoc tree from next-20180216 for today.
Thanks Kuninori for updating the patches with fixes for the above failure.
https://patchwork.kernel.org/patch/10227101/
https://patchwork.kernel.org/patch/10227099/
Re: [PATCH] ASoC: amd: Add error checking to probe function
On 11/21/2017 10:17 AM, Deucher, Alexander wrote:
-Original Message-
From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter
Roeck
Sent: Monday, November 20, 2017 11:28 PM
To: Liam Girdwood
Cc: Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-de...@alsa-project.org;
linux-kernel@vger.kernel.org; Guenter Roeck; Deucher, Alexander; Dominik
Behr; Daniel Kurtz
Subject: [PATCH] ASoC: amd: Add error checking to probe function
The acp_audio_dma does not perform sufficient error checking in its probe
function. This can result in crashes if a critical error path is
encountered.
Fixes: 7c31335a03b6a ("ASoC: AMD: add AMD ASoC ACP 2.x DMA driver")
Cc: Alex Deucher
Cc: Dominik Behr
Cc: Daniel Kurtz
Signed-off-by: Guenter Roeck
---
I didn't add an error check to acp_init() since I was not sure if
its return value is ignored on purpose.
Vijendar, Akshu can you comment?
This is also the case of missing error check.
acp_init will return error if either sw reset did not happen or clock
did not get enabled. In both cases we should error out in probe.
The patch looks good to me.
Reviewed-by: Alex Deucher
sound/soc/amd/acp-pcm-dma.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-
dma.c
index 9f521a55d610..b5e41df6bb3a 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -1051,6 +1051,11 @@ static int acp_audio_probe(struct platform_device
*pdev)
struct resource *res;
const u32 *pdata = pdev->dev.platform_data;
+ if (!pdata) {
+ dev_err(&pdev->dev, "Missing platform data\n");
+ return -ENODEV;
+ }
+
audio_drv_data = devm_kzalloc(&pdev->dev, sizeof(struct
audio_drv_data),
GFP_KERNEL);
if (audio_drv_data == NULL)
@@ -1058,6 +1063,8 @@ static int acp_audio_probe(struct platform_device
*pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
audio_drv_data->acp_mmio = devm_ioremap_resource(&pdev-
dev, res);
+ if (IS_ERR(audio_drv_data->acp_mmio))
+ return PTR_ERR(audio_drv_data->acp_mmio);
/* The following members gets populated in device 'open'
* function. Till then interrupts are disabled in 'acp_init'
--
2.7.4
