lkml.org 
[lkml]   [2021]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure
From
Date


On 12/23/2021 12:10 AM, Dmitry Osipenko wrote:
> 22.12.2021 07:35, Sameer Pujar пишет:
>> HDA regression is recently reported on Tegra194 based platforms.
>> This happens because "hda2codec_2x" reset does not really exist
>> in Tegra194 and it causes probe failure. All the HDA based audio
>> tests fail at the moment. This underlying issue is exposed by
>> commit c045ceb5a145 ("reset: tegra-bpmp: Handle errors in BPMP
>> response") which now checks return code of BPMP command response.
>> Fix this issue by skipping unavailable reset on Tegra194.
>>
>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>> Cc: stable@vger.kernel.org
>> Depends-on: 87f0e46e7559 ("ALSA: hda/tegra: Reset hardware")
> Is "Depends-on" a valid tag? I can't find it in Documentation/.

I do find the usage of the tag in many commits though there is no
reference of this in doc. I always thought it would act as a reference
when commits get pulled to other branches. If this is not true and it
does not mean anything, I will drop this.

>
>> ---
>> sound/pci/hda/hda_tegra.c | 45 ++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 36 insertions(+), 9 deletions(-)
>>
>> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
>> index ea700395..7c3df54 100644
>> --- a/sound/pci/hda/hda_tegra.c
>> +++ b/sound/pci/hda/hda_tegra.c
>> @@ -68,14 +68,20 @@
>> */
>> #define TEGRA194_NUM_SDO_LINES 4
>>
>> +struct hda_tegra_soc {
>> + bool has_hda2codec_2x_reset;
>> +};
>> +
>> struct hda_tegra {
>> struct azx chip;
>> struct device *dev;
>> - struct reset_control *reset;
>> + struct reset_control_bulk_data resets[3];
>> struct clk_bulk_data clocks[3];
>> + unsigned int nresets;
>> unsigned int nclocks;
>> void __iomem *regs;
>> struct work_struct probe_work;
>> + const struct hda_tegra_soc *data;
>> };
>>
>> #ifdef CONFIG_PM
>> @@ -170,7 +176,7 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>> int rc;
>>
>> if (!chip->running) {
>> - rc = reset_control_assert(hda->reset);
>> + rc = reset_control_bulk_assert(hda->nresets, hda->resets);
>> if (rc)
>> return rc;
>> }
>> @@ -187,7 +193,7 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>> } else {
>> usleep_range(10, 100);
>>
>> - rc = reset_control_deassert(hda->reset);
>> + rc = reset_control_bulk_deassert(hda->nresets, hda->resets);
>> if (rc)
>> return rc;
>> }
>> @@ -427,9 +433,17 @@ static int hda_tegra_create(struct snd_card *card,
>> return 0;
>> }
>>
>> +static const struct hda_tegra_soc tegra30_data = {
>> + .has_hda2codec_2x_reset = true,
>> +};
>> +
>> +static const struct hda_tegra_soc tegra194_data = {
>> + .has_hda2codec_2x_reset = false,
>> +};
>> +
>> static const struct of_device_id hda_tegra_match[] = {
>> - { .compatible = "nvidia,tegra30-hda" },
>> - { .compatible = "nvidia,tegra194-hda" },
>> + { .compatible = "nvidia,tegra30-hda", .data = &tegra30_data },
>> + { .compatible = "nvidia,tegra194-hda", .data = &tegra194_data },
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, hda_tegra_match);
>> @@ -449,6 +463,10 @@ static int hda_tegra_probe(struct platform_device *pdev)
>> hda->dev = &pdev->dev;
>> chip = &hda->chip;
>>
>> + hda->data = of_device_get_match_data(&pdev->dev);
>> + if (!hda->data)
>> + return -EINVAL;
> hda->data can't ever be NULL because all hda_tegra_match[] compatibles
> above have .data assigned. Technically this check is redundant.

Will remove.

>
> Thierry suggested previously to name it "hda->soc", like we usually do
> it in other drivers.

Previously I renamed strcture, but didn't update the member name. Will
update.

>> err = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,
>> THIS_MODULE, 0, &card);
>> if (err < 0) {
>> @@ -456,11 +474,20 @@ static int hda_tegra_probe(struct platform_device *pdev)
>> return err;
>> }
>>
>> - hda->reset = devm_reset_control_array_get_exclusive(&pdev->dev);
>> - if (IS_ERR(hda->reset)) {
>> - err = PTR_ERR(hda->reset);
>> + hda->resets[hda->nresets++].id = "hda";
>> + hda->resets[hda->nresets++].id = "hda2hdmi";
>> + /*
>> + * "hda2codec_2x" reset is not present on Tegra194. Though DT would
>> + * be updated to reflect this, but to have backward compatibility
>> + * below is necessary.
>> + */
>> + if (hda->data->has_hda2codec_2x_reset)
>> + hda->resets[hda->nresets++].id = "hda2codec_2x";
>> +
>> + err = devm_reset_control_bulk_get_exclusive(&pdev->dev, hda->nresets,
>> + hda->resets);
>> + if (err)
>> goto out_free;
>> - }
>>
>> hda->clocks[hda->nclocks++].id = "hda";
>> hda->clocks[hda->nclocks++].id = "hda2hdmi";
>>
> Not sure whether the above nits worth making v4. I'll leave it up to you
> and other reviewers to decide.
>
> Overall this patch looks good to me, thank you.
>
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

\
 
 \ /
  Last update: 2021-12-23 05:35    [W:0.341 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site