[alsa-devel] [PATCH 04/26] ASoC: SOF: Intel: hda-dsp: Add helper for setting DSP D0ix substate

[Sridharan, Ranjani]

On Tue, Oct 29, 2019 at 3:10 AM Cezary Rojewski <cezary.rojewski at intel.com>
wrote:

> On 2019-10-26 00:41, Pierre-Louis Bossart wrote:
> > From: Keyon Jie <yang.jie at linux.intel.com>
> >
> > Adding helper to implement setting dsp to d0i3 or d0i0 status, this will
> > be needed for driver D0ix support.
> >
> > Signed-off-by: Keyon Jie <yang.jie at linux.intel.com>
> > Signed-off-by: Pierre-Louis Bossart <
> pierre-louis.bossart at linux.intel.com>
>
> > +static int hda_dsp_wait_d0i3c_done(struct snd_sof_dev *sdev, int retry)
> > +{
> > +     struct hdac_bus *bus = sof_to_bus(sdev);
> > +
> > +     while (snd_hdac_chip_readb(bus, VS_D0I3C) & SOF_HDA_VS_D0I3C_CIP) {
> > +             if (!retry--)
> > +                     return -ETIMEDOUT;
> > +             usleep_range(10, 15);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +int hda_dsp_set_power_state(struct snd_sof_dev *sdev,
> > +                         enum sof_d0_substate d0_substate)
> > +{
> > +     struct hdac_bus *bus = sof_to_bus(sdev);
> > +     int retry = 50;
> > +     int ret;
> > +     u8 value;
> > +
> > +     /* Write to D0I3C after Command-In-Progress bit is cleared */
> > +     ret = hda_dsp_wait_d0i3c_done(sdev, retry);
> > +     if (ret < 0) {
> > +             dev_err(bus->dev, "CIP timeout before update D0I3C!\n");
> > +             return ret;
> > +     }
> > +
> > +     /* Update D0I3C register */
> > +     value = d0_substate == SOF_DSP_D0I3 ? SOF_HDA_VS_D0I3C_I3 : 0;
>
> Some indentation or parenthesis would make this cleaner.
>
> > +     snd_hdac_chip_updateb(bus, VS_D0I3C, SOF_HDA_VS_D0I3C_I3, value);
> > +
> > +     /* Wait for cmd in progress to be cleared before exiting the
> function */
> > +     retry = 50;
> > +     ret = hda_dsp_wait_d0i3c_done(sdev, retry);
> > +     if (ret < 0) {
> > +             dev_err(bus->dev, "CIP timeout after D0I3C updated!\n");
> > +             return ret;
> > +     }
> > +
> > +     dev_vdbg(bus->dev, "D0I3C updated, register = 0x%x\n",
> > +              snd_hdac_chip_readb(bus, VS_D0I3C));
> > +
> > +     return 0;
> > +}
> > +
>
> I believe hda_dsp_wait_d0i3c_done function could have had its argument
> list simplified. "retry" is passed externally and it is always set to
> 50. One could put the "retry" right inside _done function. This or allow
> the caller to modify the sleep period. Another option is to replace
> "retry" with "timeout period" (total timeout, that is) entirely.
>
Cezary,

This has been fixed later in the series to use
the HDA_DSP_REG_POLL_RETRY_COUNT but I agree that this can further be
simplified to just use the macro inside the hda_dsp_wait_d0i3c_done()
instead of passing the argument.


>
> In regard to maintenance, both ret checks (resulting in dev_errs) assume
> -ETIMEOUT outcome on _done failure. If said function gets updated in the
> future, these implicit checks might cause problems.
>
Possibly, but at the moment -ETIMEOUT looks like the correct error to be
reported.

Thanks,
Ranjani

>
> >   static int hda_suspend(struct snd_sof_dev *sdev, bool runtime_suspend)
> >   {
> >       struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
> > diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
> > index ea02bf40cb25..0e7c366b8f71 100644
> > --- a/sound/soc/sof/intel/hda.h
> > +++ b/sound/soc/sof/intel/hda.h
> > @@ -64,6 +64,13 @@
> >   #define SOF_HDA_PPCTL_PIE           BIT(31)
> >   #define SOF_HDA_PPCTL_GPROCEN               BIT(30)
> >
> > +/*Vendor Specific Registers*/
>
> Missing spaces.
>
> > +#define SOF_HDA_VS_D0I3C             0x104A
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>