From: Sameer Pujar <spujar@nvidia.com>
To: Michael Walle <michael@walle.cc>, Mark Brown <broonie@kernel.org>
Cc: devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
kuninori.morimoto.gx@renesas.com, robh@kernel.org,
linux-kernel@vger.kernel.org, jonathanh@nvidia.com,
sharadg@nvidia.com, thierry.reding@gmail.com,
linux-tegra@vger.kernel.org
Subject: Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
Date: Mon, 15 Mar 2021 20:49:00 +0530 [thread overview]
Message-ID: <6af6439c-bdb8-cd0f-635d-069040ba5b65@nvidia.com> (raw)
In-Reply-To: <8cdf1cfa971945792b509a687a4de735@walle.cc>
On 3/15/2021 5:35 PM, Michael Walle wrote:
> External email: Use caution opening links or attachments
>
>
> Am 2021-03-12 14:46, schrieb Mark Brown:
>> On Fri, Mar 12, 2021 at 01:30:02PM +0100, Michael Walle wrote:
>>
>>> The card calls set_sysclk(), which eventually ends up in the codec.
>>> The codec therefore, could figure out if it needs to configure the
>>> clock or if it can use its internal FLL.
>>> Is that what you mean?
>>
>> Yes.
>>
>>> But the set_sysclk() of the codec isn't even called, because the
>>> card itself already tries to call clk_set_rate() on the Codec's MCLK,
>>> which returns with an error [0].
>>
>> OK, so I think we need to push this down a level so that the clock
>> setting is implemented by the core/CODEC rather than by simple-card,
>> with the helpers being something the CODEC can opt out of.
>
> Sameer, it looks like the proper fix should be to add the clock
> support to your codec.
I agree that complicated clock relationships should be handled within
the codec itself, however MCLK rate setting depends on "mclk-fs" factor
and this property is specified as part of simple-card/audio-graph-card
codec subnode. Right now codec, in general, does not have a way to know
this. The set_sysclk() callback takes rate argument and not the factor.
Moreover the same codec is used by other platform vendors too and unless
a new DT property is added for codec, runtime MCLK update based on the
scaling factor cannot be supported. This would mean that we will be
having two methods to specify "mclk-fs" factor, one from
simple-card/audio-graph-card and one from respective codec nodes, which
does not seem ideal.
Also it does not seem consistent with the way we handle MCLK clock based
on where it is specified.
a) If specified in simple-card/audio-graph-card, MCLK clock
rate/enable/disable updates are allowed.
b) If specified in codec device node, it is not expected to touch the
MCLK clock. This patch tried to treat it the same way as (a) does.
Advantage of this is, all codec drivers need not explicitly handle MCLK,
instead it is done at a central place. The platforms which use specific
machine drivers do the same and that is why probably the codec driver
patch was never required. It is about just setting MCLK clock as a
factor of sample rate, whenever the factor is available. I understand
that it is breaking your use case, but I am not sure if the usage of
set_sysclk() is consistent. I mean, it takes the "freq" argument. Does
it refer to MCLK rate or system-clock (sysclk) rate? MCLK and sysclk are
not really the same when codec PLL is involved. So I would like to
understand clearly about what "freq" argument means. Also when "mclk-fs"
factor is specified, is it related to MCLK or sysclk? My understanding
is that it should be strictly viewed as related to MCLK.
Does it help if a separate helper is used for audio-graph-card with
current change and reverting the simple-card to its previous state?
Morimoto-san, does it affect any other users of audio-graph-card?
>
> I've also looked at other users of "simple-audio-card" and
> it looks like they will break too. For example,
> - arch/arm64/boot/dts/rockchip/rk3399.dtsi
> If I'm not mistaken, this will try to set the first clock
> of hdmi@ff940000 there, which is "iahb".
> - arch/arm/boot/dts/sun8i-a33.dtsi
> There "&ccu CLK_BUS_CODEC" of codec@1c22e00 will be changed
>
> And it doesn't stop there, it also sets the first clock
> of the CPU endpoint, which I guess just works because .set_rate
> is a noop for the most clocks which are used there.
Yes this is a problem, unfortunately I missed checking some of the
simple-card examples. I wonder we should be specifically looking for
"mclk" clock here.
next prev parent reply other threads:[~2021-03-15 15:20 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-10 6:43 [PATCH 0/3] Use clocks property in a device node Sameer Pujar
2021-02-10 6:43 ` [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock Sameer Pujar
2021-02-11 23:44 ` Kuninori Morimoto
2021-02-14 17:56 ` Sameer Pujar
2021-02-14 23:25 ` Kuninori Morimoto
2021-03-09 14:41 ` Michael Walle
2021-03-09 16:27 ` Sameer Pujar
2021-03-09 22:30 ` Michael Walle
2021-03-10 14:50 ` Sameer Pujar
2021-03-10 18:14 ` Michael Walle
2021-03-10 19:19 ` Sameer Pujar
2021-03-11 10:27 ` Michael Walle
2021-03-11 11:05 ` Sameer Pujar
2021-03-11 11:16 ` Michael Walle
2021-03-11 14:29 ` Sameer Pujar
2021-03-11 15:43 ` Michael Walle
2021-03-11 16:41 ` Mark Brown
2021-03-11 16:15 ` Mark Brown
2021-03-11 22:11 ` Michael Walle
2021-03-12 11:35 ` Mark Brown
2021-03-12 12:01 ` Michael Walle
2021-03-12 12:04 ` Mark Brown
2021-03-12 12:30 ` Michael Walle
2021-03-12 13:46 ` Mark Brown
2021-03-15 12:05 ` Michael Walle
2021-03-15 15:19 ` Sameer Pujar [this message]
2021-03-15 15:33 ` Michael Walle
2021-03-15 15:57 ` Sameer Pujar
2021-03-15 15:39 ` Mark Brown
2021-03-15 17:10 ` Sameer Pujar
2021-03-15 17:13 ` Michael Walle
2021-03-11 16:00 ` Mark Brown
2021-03-11 21:34 ` Michael Walle
2021-02-10 6:43 ` [PATCH 2/3] Revert "ASoC: audio-graph-card: Add clocks property to endpoint node" Sameer Pujar
2021-02-11 13:00 ` Mark Brown
2021-02-10 6:43 ` [PATCH 3/3] arm64: tegra: Move clocks from RT5658 endpoint to device node Sameer Pujar
2021-02-11 15:38 ` [PATCH 0/3] Use clocks property in a " Mark Brown
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=6af6439c-bdb8-cd0f-635d-069040ba5b65@nvidia.com \
--to=spujar@nvidia.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jonathanh@nvidia.com \
--cc=kuninori.morimoto.gx@renesas.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=michael@walle.cc \
--cc=robh@kernel.org \
--cc=sharadg@nvidia.com \
--cc=thierry.reding@gmail.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 NNTP newsgroup(s).