On 16/04/2024 21:51, Pierre-Louis Bossart wrote: > > > On 4/16/24 02:23, Xingyu Wu wrote: > > On 02/04/2024 21:57, Pierre-Louis Bossart wrote: > >> > >> > >>>> > >>>>> +#define PERIODS_MIN 2 > >>>>> + > >>>>> +static unsigned int cdns_i2s_pcm_tx(struct cdns_i2s_dev *dev, > >>>>> + struct snd_pcm_runtime *runtime, > >>>>> + unsigned int tx_ptr, bool > *period_elapsed, > >>>>> + snd_pcm_format_t format) > >>>>> +{ > >>>>> + unsigned int period_pos = tx_ptr % runtime->period_size; > >>>> > >>>> not following what the modulo is for, usually it's modulo the buffer size? > >>> > >>> This is to see if the new data is divisible by period_size and to > >>> determine whether it is enough for a period_size in the later loop. > >> > >> That didn't answer to my question, the position is usually between > >> 0..buffer_size.1. > > > > Yes, this position will be used later in the cdns_i2s_pcm_pointer(). > > But this cdns_i2s_pcm_tx() is called by I2S hardware interrupt which > > would be frequently called several times each period. The period_pos > > is used to determine whether there is enough a period_size to call > > snd_pcm_period_elapsed(). > > > >> > >> Doing increments on a modulo value then comparisons as done below > >> seems rather questionable. > >> > >>>>> + > >>>>> + iowrite32(data[0], dev->base + CDNS_FIFO_MEM); > >>>>> + iowrite32(data[1], dev->base + CDNS_FIFO_MEM); > >>>>> + period_pos++; > >>>>> + if (++tx_ptr >= runtime->buffer_size) > >>>>> + tx_ptr = 0; > >>>>> + } > >>>>> + > >>>>> + *period_elapsed = period_pos >= runtime->period_size; > >>>>> + return tx_ptr; > >>>>> +} > >> > >>>>> + pm_runtime_enable(&pdev->dev); > >>>>> + if (pm_runtime_enabled(&pdev->dev)) > >>>>> + cdns_i2s_runtime_suspend(&pdev->dev); > >>>> > >>>> that sequence looks suspicious.... Why would you suspend > >>>> immediately during the probe? You're probably missing all the autosuspend > stuff? > >>> > >>> Since I have enabled clocks before, and the device is in the suspend > >>> state after pm_runtime_enable(), I need to disable clocks in > >>> cdns_i2s_runtime_suspend() to match the suspend state. > >> > >> That is very odd on two counts > >> a) if you haven't enabled the clocks, why do you need to disbale them? > >> b) if you do a pm_runtime_enable(), then the branch if > >> (pm_runtime_enabled) is always true. > >> > > > > a) It must enable clocks first to read and write registers when I2S probe. > > Then it is done to probe, the clocks are still enabled and the state > > of pm is suspend. So it need to be disabled to match the state and > > will resume and be enabled by ALSA. > > I think you are missing a pm_runtime_set_active() to reconcile the pm state with > the hardware state. The premise of pm_runtime is that on probe your device is > active and later on it will suspend. Having pm_runtime_enabled with a > suspended device without the framework involved to trigger the transition to > suspend is asking for trouble. Great, It is better to use pm_runtime_set_active(). I will modify it in next patch. > > > b) Because CONFIG_PM would be disabled and pm_runtime_enabled() return > > false , then it is no need to disable clock and I2S still can work. > > Again you are trying to make things more complicated than they need to be. > Don't try to actively manage and query states, let the framework do it for you. > > Try to probe and bring the device to an active state. Then use > pm_runtime_mark_last_busy(), use pm_runtime_enable and let autosuspend > do the work for you. If pm_runtime is not enabled the suspend will not happen. > > Also keep in mind that pm_runtime_enabled() will return false if the user mucks > with the power state in sysfs, it's not only a case of CONFIG_PM being selected > or not. Noted. Thanks. > > > >> > >>> > >>>> > >>>>> + > >>>>> + dev_dbg(&pdev->dev, "I2S supports %d stereo channels > with %s.\n", > >>>>> + i2s->max_channels, ((i2s->irq < 0) ? "dma" : "interrupt")); > >>>>> + > >>>>> + return 0; > >>>>> + > >>>>> +err: > >>>>> + return ret; > >>>>> +} > >>>>> + > >>>>> +static int cdns_i2s_remove(struct platform_device *pdev) { > >>>>> + pm_runtime_disable(&pdev->dev); > >>>>> + if (!pm_runtime_status_suspended(&pdev->dev)) > >>>>> + cdns_i2s_runtime_suspend(&pdev->dev); > >>>> > >>>> ... and this one too. Once you've disabled pm_runtime, checking the > >>>> status is irrelevant... > >>> > >>> I think the clocks need to be always enabled after probe if disable > >>> pm_runtime, and should be disabled when remove. This will do that. > >> > >> if you are disabling pm_runtime, then the pm_runtime state becames invalid. > >> When pm_runtime_disable() is added in remove operations, it's mainly > >> to prevent the device from suspending. > > > > Should I use the pm_runtime_enabled() before the pm_runtime_disable()? > > It doesn't matter, the problem is the second part where you try to check the > status of pm_runtime *after* disabling it. > Will fix. Thanks, Xingyu Wu