On 10/30/18 11:02 AM, Hans de Goede wrote:
Hi,
On 30-10-18 16:46, Hans de Goede wrote:
Hi,
On 30-10-18 16:04, Pierre-Louis Bossart wrote:
On 10/30/18 9:38 AM, Dean Wallace wrote:
On 30-10-18, Hans de Goede wrote:
Hi Dean,
Attached are 2 different attempts at fixing this.
When trying these patches do not forget to remove the revert of the
"Stop-marking-clocks-as-CLK_IS_CRITICAL" commit.
Please first try the
0001-ASoC-intel-cht_bsw_max98090_ti-Use-pmc_plt_clk_0-ins.patch
patch I expect that one to do the trick indicating that the Swanky
model
uses a different pmc clk then which is normally used for the codec
clock.
If that patch does not fix things, please give the other patch a try.
For Baytrail devices, the audio platform clocks are not managed by
the firmware. They are for CHT-based devices - as can be seen by
clock resources being described in the DSDT. We used to have a
if(baytrail) in the code which was replaced by this CRITICAL label,
but the point remains that there is a difference between the two SOC
versions.
As I mentioned before the CRITICAL flag was only added a year ago to
workaround
an issue with on board ethernet needing plt_clk_4 on some laptops,
this never
had anything to do with sound.
Ah I see now that you later made some changes based on the patch to
fix the ethernet:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=7735bce05a9c0bb0eb0f08c9002d65843a7c5798
Note though that that does not touch the machine driver which we are
discussing
now and the reporters machine is also BYT based.
As explained below (in my original reply) I think it is fine to always
manage the clk from within the kernel. But if you think this is a bad
idea, we could re-introduce the is_valleyview() checks in machine
drivers which are used on CHT devices.
I am having difficulties following the proposal
for audio, managing the platform clocks from the kernel is only useful
for Baytrail, and the code handling the CRITICAL flags is really
equivalent to having a is_valleyview() test in the machine drivers. I
don't quite understand the S0ix-related changes and I also don't get how
using plt_clk_0 makes things better or wonder if audio on Swanky worked
before the commit 7735bce05a9c ('ASoC: Intel: boards: use devm_clk_get()
unconditionally'.
In other words, I don't know if we are dealing with a platform that only
started working with 7735bce05a9c and does need a quirk to make use of
plt_clk_0 or a fundamental issue with clock/power management.
In addition I am not aware of any baytrail device using plt_clk_0,
so moving a common machine driver such a cht_bsw_max98090_ti to use
plt_clk0 only would break other devices (e.g. Rambi/Orco). Asking
for both clocks to be on might work though,
Ok, so we need to have a DMI based quirk for the Swanky and maybe also
the clapper to use plt_clk_0 there. Asking for 2 clks if we only need
one does not seem like a good plan.
however you still have the problem of trying to manage from the
kernel what the firmware already manages.
The firmware only manages it when going to D3 state, with ASoC most of
the codecs gets turned off (and we no longer need the clock) but we do
not put the device in D3 / execute the _PS3 method. So from a pm pov
it is better if we manage the clk ourselves.
Once we do actually put the device in D3 (on suspend) the kernel will
have
already turned off the clk and the _OFF method of the CLK3 power
resource
which directly pokes mmio, will just set the enable bit to 0 when it
already is 0, so no problem.
Regards,
Hans
