On Tue, 07 Dec 2021 09:16:43 +0100,
Thierry Reding wrote:
>
> On Tue, Dec 07, 2021 at 12:02:48PM +0530, Sameer Pujar wrote:
> > 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.
> >
> > The failure can be fixed by avoiding above reset in the driver,
> > but the explicit reset is not necessary for Tegra devices which
> > depend on BPMP. On such devices, BPMP ensures reset application
> > during unpowergate calls. Hence skip reset on these devices
> > which is applicable for Tegra186 and later.
> >
> > Signed-off-by: Sameer Pujar <spujar@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Depends-on: 87f0e46e7559 ("ALSA: hda/tegra: Reset hardware")
> > ---
> > sound/pci/hda/hda_tegra.c | 24 +++++++++++++++++++-----
> > 1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> > index ea700395..862141e 100644
> > --- a/sound/pci/hda/hda_tegra.c
> > +++ b/sound/pci/hda/hda_tegra.c
> > @@ -68,6 +68,10 @@
> > */
> > #define TEGRA194_NUM_SDO_LINES 4
> >
> > +struct hda_data {
> > + unsigned int do_reset:1;
> > +};
>
> I suppose this could also be a bool. Not sure if we need to care about
> packing optimizations at this point.
>
> It may also be useful to rename this to something less generic to avoid
> potential clashes with other data structures in the future. We've often
> used the _soc suffix in other drivers to mark this kind of SoC-specific
> data. In this case it would be struct hda_tegra_soc.
>
> If Takashi is fine with this as-is, I don't have any strong objections,
> though.
Indeed, a bit more prefix would be better for avoiding the possible
conflict in future, but the struct name is local, so I don't mind to
use the simple name for now. We can change it later once when needed,
too.
> > +
> > struct hda_tegra {
> > struct azx chip;
> > struct device *dev;
> > @@ -76,6 +80,7 @@ struct hda_tegra {
> > unsigned int nclocks;
> > void __iomem *regs;
> > struct work_struct probe_work;
> > + const struct hda_data *data;
> > };
> >
> > #ifdef CONFIG_PM
> > @@ -427,8 +432,13 @@ static int hda_tegra_create(struct snd_card *card,
> > return 0;
> > }
> >
> > +static const struct hda_data tegra30_data = {
> > + .do_reset = 1,
> > +};
> > +
> > static const struct of_device_id hda_tegra_match[] = {
> > - { .compatible = "nvidia,tegra30-hda" },
> > + { .compatible = "nvidia,tegra30-hda", .data = &tegra30_data },
> > + { .compatible = "nvidia,tegra186-hda" },
> > { .compatible = "nvidia,tegra194-hda" },
> > {},
> > };
>
> One other thing we've done in the past is to explicitly pass these
> structures for each compatible string. That simplifies things a bit
> because we don't have to keep checking for non-NULL pointers and instead
> rely on the fact that there's always a valid pointer.
>
> To do so, you'd basically add:
>
> static const struct hda_data tegra186_data = {
> .do_reset = 0,
> };
>
> And reference that for both the Tegra186 and Tegra194 entries. Again,
> not strictly necessary and since we have only one occurrence where we
> need to check this, it seems fine as-is, so:
>
> Acked-by: Thierry Reding <treding@xxxxxxxxxx>
That's true, too. OTOH, completely without a NULL check would be also
unsafe, so some sanity check would be still required.
That said, the current patch is good enough for taking as a regression
fix, but I'm fine to wait for a while for v2 to address those, too :)
thanks,
Takashi
