Received: by 2002:ac0:98c7:0:0:0:0:0 with SMTP id g7-v6csp5076205imd; Tue, 30 Oct 2018 11:34:48 -0700 (PDT) X-Google-Smtp-Source: AJdET5fRGbcWWNj36YWpsaZniYwJEtJHuVUyyTs1CmR0a8ZkRJpCa7hCnbr54XhZVeKeNPOuB3+w X-Received: by 2002:a63:a112:: with SMTP id b18mr8381pgf.440.1540924488056; Tue, 30 Oct 2018 11:34:48 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1540924488; cv=none; d=google.com; s=arc-20160816; b=IBCUBACWQwtvEIK22EJdkqGeyI4JH4ef2NhfYpTWDN/qNKa53bvsIB6UCt5gLntLyj OLQSd2gqSAbzyzsBun6Anz0n++nAFDEJhYcvWAl9RY7TemY1z8cPd37tXpURamOPyzk7 S9iE2JjloCV7Ps2NV03mw7Wj+zNbF3ha7yP3LbYMt87YCfYLHVLiMuh8D+H/FEfQiRy7 I51P97DbeJmlxNjOfGYQUK4jLqmCRAoHmBRVuD1qlaJyz+9Rk8iFrMS7MWEEetiL9RjK v2GQTFrb+gCY5Pttk61bwQ4kaAjzSNmc4C3V57OSz0Uf96ALdCvYH9A7pDVkc8n+WHP7 H5fA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=DFsFFLjRXDGHuUWoOloyRgpJvoEH8MXDHnD+qyam8/4=; b=pdxNj0Xv0dqO1rNIO2/fcAjul2qZM7Z4Ujk////ACJMLUpfZuXgKtAaEfoeKFIu38n 2XdpP1LSua4831KC6cT8FoZA4yWHTyBfuojvpKfxHcpR2BK0/OLTFZLznHiRongkQb5B /8hFjQ/Qrcy/v9WtvWOcIicXUK2h5xCdeWoD77TPvZ6DGnWvVDgndsLbu4ulKt/ttJv0 FvRsNVMZyPCILwV2yYcrakR0ZqqfDzhDeu8ZghZcDAO1ytMx5wlm72jq14y8yexHobAF oEUwSQX7LAHZxCl1io/04qOLPjVdr9BLJvofCotJ7EE1i/8Icr5GHuc9Q1tfHv+p8jvh Ks9A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id c10-v6si23064746pla.251.2018.10.30.11.34.24; Tue, 30 Oct 2018 11:34:48 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727814AbeJaD00 (ORCPT + 99 others); Tue, 30 Oct 2018 23:26:26 -0400 Received: from mail-wm1-f68.google.com ([209.85.128.68]:33051 "EHLO mail-wm1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726522AbeJaD0X (ORCPT ); Tue, 30 Oct 2018 23:26:23 -0400 Received: by mail-wm1-f68.google.com with SMTP id y140-v6so12507944wmd.0 for ; Tue, 30 Oct 2018 11:31:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=DFsFFLjRXDGHuUWoOloyRgpJvoEH8MXDHnD+qyam8/4=; b=CkNfFP/SS8p4TG+0B3Cp2FiEmsDu0uHb+fp4yzq70gJRjS4hRdGSUiyEPVAETJLx/w LHqCZNItVeIKD6lNszfJTn5jw+tYS/NoSoZhBhJcL3zIInDAstG69YQzvSbMEjT6DxHh Uj4U35UiQ7upYei6QHfa6Jw/ppWbg/YyOHMcm5Zfw3fJP2WuZTCcdnDAftvjDJ9jGGDQ EZtub9gdV3CQ3lOb4oZKi1JC36QW0iNIVe0Hg2v4J9LvafXqXZb4cAgmHXp1GSK4AO1j sFIoeqrgSDE9/P5VRNM8q2MftqA0vvZf9cKleBjwQYLzXhKLPBaxjXeoea12Pxtk3a6F 5ijw== X-Gm-Message-State: AGRZ1gKQnx336o+9yCdyMvILKHRw1FK5kbSsWsy61LnoV4Pnx+7HUaYm F3PXWhjHCY/1azQT8M+9Ibv37g== X-Received: by 2002:a1c:9b4b:: with SMTP id d72-v6mr15816wme.72.1540924306273; Tue, 30 Oct 2018 11:31:46 -0700 (PDT) Received: from shalem.localdomain (546A5441.cm-12-3b.dynamic.ziggo.nl. [84.106.84.65]) by smtp.gmail.com with ESMTPSA id x8-v6sm61689685wrd.54.2018.10.30.11.31.45 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 30 Oct 2018 11:31:45 -0700 (PDT) Subject: Re: Regression found (Stop-marking-clocks-as-CLK_IS_CRITICAL) To: Pierre-Louis Bossart , Dean Wallace Cc: Andy Shevchenko , Stephen Boyd , Michael Turquette , linux-clk , Stable , Johannes Stezenbach , Carlo Caione , Andy Shevchenko , Linux Kernel Mailing List , Mogens Jensen References: <20181025232517.ywnw54qibemosjws@picard> <154083512089.98144.9141070901932719147@swboyd.mtv.corp.google.com> <20181029190819.2ivlx73n6y6sx4vk@picard> <20181030143836.feo7zcxiestylxoo@picard> <2d429c87-24c5-4075-683e-b0d12c3eb1c2@linux.intel.com> <783f17c2-ecc6-00fc-5629-02319b965205@redhat.com> From: Hans de Goede Message-ID: Date: Tue, 30 Oct 2018 19:31:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Pierre-Louis, On 30-10-18 17:27, Pierre-Louis Bossart wrote: > > 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. Ok let me try to clarify this by breaking this out into the 3 separate things which I believe are in play here: 1) The PMC clocks should NOT be marked as CLK_IS_CRITICAL Commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the firmware") should, in hindsight, never have been merged. CLK_IS_CRITICAL disables all management of the clk in question and is really intended for clks which are e.g. driving the CPU core itself or DRAM. The proper fix for the ethernet problem that patch was meant to address is for the ethernet driver to get and enable the clock itself. As I've done in a recent series which has as purpose to revert d31fd43c0f9a. d31fd43c0f9a marks any PMC clock which is enabled by the firmware as boot as critical, not only the codec mclk, but *any* clk. This means that unless all clks which are enabled at boot are controlled by the firmware as either ACPI pwr-resources, or by a device's _PS3 method, some clks will stay on during suspend, see e.g. : https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102 https://bugzilla.kernel.org/show_bug.cgi?id=196861 Also on BYT this means that if the firmware has enabled the coded mclk at boot it will get marked as CLK_IS_CRITICAL and we will now never disable it. If some clocks are still on during suspend then the SoC cannot reach its deepest sleep states while suspended, which obviously is bad. TL;DR: CLK_IS_CRITICAL block reaching the deepest sleep states so d31fd43c0f9a needs to be reverted. 2) Dropping the CLK_IS_CRITICAL flag means we are now also controlling the codec mclk on CHT. Just like setting the CLK_IS_CRITICAL flag on the mclk meant that we were effectively no longer controlling the mclk on BYT, reverting this change combined with your 7735bce05a9c ("ASoC: Intel: boards: use devm_clk_get() unconditionally") change means that we are now controlling the mclk on CHT platforms. As you point out the firmware also controls the mclk on these platforms, so arguably this is undesirable. As I tried to explain I do not believe that this is actually a problem. But if you think this is a problem then we will need to revert 7735bce05a9c. If you prefer to go that route then I can prepare a patch for this. 3) No longer marking PMC clocks enabled at boot as CLK_IS_CRITICAL has caused a regression on Swanky and Clapper model chromebooks You wrote: "I wonder if audio on Swanky worked before the commit 7735bce05a9c" I don't think that that commit has impacted Swanky at all since Swanky is BYT based. Instead I believe that d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the firmware") has accidentally fixed audio on Swanky laptops. This also explains why my "Stop-marking-clocks-as-CLK_IS_CRITICAL" commit has broken it again as that is effectively a revert of d31fd43c0f9. As I wrote before the as a result of d31fd43c0f9a the clk enable/disable behavior is not only ignored on CHT, as you noticed when writing 7735bce05a9c, but it also causes it to be ignored on BYT, including the initial disable of unused clocks just before the kernel starts /sbin/init. This not only fixed the ethernet on the laptop model that d31fd43c0f9a targeted but also caused us to no longer disable plt_clk_0 on Swanky (debugging done outside of this thread has shown that d31fd43c0f9a sets CLK_IS_CRITICAL on plt_clk_0). TL;DR: d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the firmware") caused plt_clk_0 to be marked as CLK_IS_CRITICAL on Swanky, accidentally fixing audio. The proper fix for this is to make the cht_bsw_max98090_ti.c code control pmc_plt_clk_0 instead of pmc_plt_clk_3 as the Swanky board is actually using pmc_plt_clk_0 and since this is a BYT board we should be controlling the mclk there. I hope this helps to clarify things, if not please keep asking questions. Regards, Hans