Messages in this thread | | | Subject | Re: [PATCH v3 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure | From | Sameer Pujar <> | Date | Thu, 23 Dec 2021 10:04:19 +0530 |
| |
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>
| |