lkml.org 
[lkml]   [2021]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/3] ALSA: hda/tegra: Skip reset on BPMP devices
    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@nvidia.com>
    > > Cc: stable@vger.kernel.org
    > > 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@nvidia.com>

    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

    \
     
     \ /
      Last update: 2021-12-07 09:37    [W:3.538 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site