Re: [PATCH 1/3] ALSA: hda/tegra: Skip reset on BPMP devices

[Takashi Iwai]

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