From: Xingyu Wu <xingyu.wu@starfivetech.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>,
Claudiu Beznea <Claudiu.Beznea@microchip.com>,
Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor.dooley@microchip.com>
Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
"linux-sound@vger.kernel.org" <linux-sound@vger.kernel.org>
Subject: RE: 回复: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller
Date: Thu, 18 Apr 2024 06:54:43 +0000 [thread overview]
Message-ID: <NTZPR01MB095663E132E51B10CED1EBEA9F0EA@NTZPR01MB0956.CHNPR01.prod.partner.outlook.cn> (raw)
In-Reply-To: <35d9f59e-3cc1-41a7-bb1d-f482c004d323@linux.intel.com>
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
prev parent reply other threads:[~2024-04-18 8:29 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-20 9:02 [PATCH v2 0/2] Add Cadence I2S-MC controller driver Xingyu Wu
2024-03-20 9:02 ` [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller Xingyu Wu
2024-03-21 8:24 ` Krzysztof Kozlowski
2024-03-26 6:29 ` 回复: " Xingyu Wu
2024-03-26 6:46 ` Krzysztof Kozlowski
2024-03-26 13:43 ` Xingyu Wu
2024-03-27 5:12 ` Krzysztof Kozlowski
2024-03-29 3:56 ` Xingyu Wu
2024-03-29 11:42 ` Krzysztof Kozlowski
2024-03-29 13:36 ` Mark Brown
2024-03-29 16:01 ` Krzysztof Kozlowski
2024-04-01 6:32 ` Xingyu Wu
2024-03-20 9:02 ` [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller Xingyu Wu
2024-03-20 15:00 ` Pierre-Louis Bossart
2024-03-20 15:21 ` Mark Brown
2024-03-26 2:04 ` 回复: " Xingyu Wu
2024-03-26 2:02 ` Xingyu Wu
2024-04-02 13:57 ` Pierre-Louis Bossart
2024-04-16 7:23 ` Xingyu Wu
2024-04-16 13:51 ` Pierre-Louis Bossart
2024-04-18 6:54 ` Xingyu Wu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=NTZPR01MB095663E132E51B10CED1EBEA9F0EA@NTZPR01MB0956.CHNPR01.prod.partner.outlook.cn \
--to=xingyu.wu@starfivetech.com \
--cc=Claudiu.Beznea@microchip.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=conor.dooley@microchip.com \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sound@vger.kernel.org \
--cc=perex@perex.cz \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=robh+dt@kernel.org \
--cc=tiwai@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).