From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C337EC41620 for ; Tue, 19 Jan 2021 18:29:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A998522AAA for ; Tue, 19 Jan 2021 18:29:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731578AbhASRbt (ORCPT ); Tue, 19 Jan 2021 12:31:49 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35348 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2390805AbhASRbX (ORCPT ); Tue, 19 Jan 2021 12:31:23 -0500 Received: from mail-wm1-x32d.google.com (mail-wm1-x32d.google.com [IPv6:2a00:1450:4864:20::32d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 074CBC061575; Tue, 19 Jan 2021 09:30:43 -0800 (PST) Received: by mail-wm1-x32d.google.com with SMTP id m2so533721wmm.1; Tue, 19 Jan 2021 09:30:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=nDyXlmZnRyCURD1JgXC1GjN+18/ZRktMLfCEGonjNiY=; b=cB/cvOGRB1gD5rc6vL9IgpaDCXgmftNI+tHoINdC6AkY39fyHjVEKIWETwsNn9a+K9 2ptLMPA15AN0o/aquqG1B1u4a7ThvW2kQwhR+Rn9ByjrKDI1LuE+K+/x1i02hi2pRMmT e4XOZJUD18lVOOhSofGDpfYPntuY+nHbFNFE2gGK5fuxskKYrojUWzMc32eJ8gcQG3nD AgdXH+7YzVlhUFv+Ns3zs65pqvFht1Uuj4RZFm/iP/w7pyIt5RT0voiGYqH/ptx2ZymI Qg71NLESVLsIRhqxeaeoqbQN0nVKbynplj4Yuuldi3d4fn4gcglpyhL4hwVPRCyQ7Nqg InqQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=nDyXlmZnRyCURD1JgXC1GjN+18/ZRktMLfCEGonjNiY=; b=k3Gzts2BZ+S1A8mScB/m35kgiomCXLW7+akumjCNaFlGP4CTlMYNp27cFMNUspPbjp SVtLg4peBjqRdfXaVr9OwXPBqPDcTgCjsiiqpbsHX3Qy6ssr/AFy7XHbc0f2lx3BeMi7 7cYVyjIhOScBPodCpkzj0+2ug4fo8EwAVfe2RXP1ecJ06vahsMa3c104mJMGTK3LI4PR DItsRgyQqhcfg1BHq4kUnoTa2yr58oRk4DHedVmFexd97NMIPfO78SMYkAIVGbeJRXWz KTTRitKZBjJlnNmstHNRD3hn8oindE1IPCghSwz9NMm3rv7ykZbbw6zJggqQs7hBGqFC OeKQ== X-Gm-Message-State: AOAM530o0iA0hpht1KaiGdVRqXvY2ByYpoXIn/ekVNADmYmlKZQVgMzj Px43WdUjvlpX89KolXmZCYk= X-Google-Smtp-Source: ABdhPJxFo4IKI66fTtRW7gSBX2uD/raX9mtUo9KupK/7Dj8LSiAN4LhhRiY4DDdNxWwdEzmmGBaA6g== X-Received: by 2002:a1c:e486:: with SMTP id b128mr649392wmh.136.1611077441676; Tue, 19 Jan 2021 09:30:41 -0800 (PST) Received: from localhost ([62.96.65.119]) by smtp.gmail.com with ESMTPSA id 62sm5866498wmd.34.2021.01.19.09.30.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 19 Jan 2021 09:30:40 -0800 (PST) Date: Tue, 19 Jan 2021 18:30:39 +0100 From: Thierry Reding To: Dmitry Osipenko Cc: Jonathan Hunter , Sameer Pujar , Peter Geis , Nicolas Chauvet , Takashi Iwai , Jaroslav Kysela , alsa-devel@alsa-project.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 2/5] ALSA: hda/tegra: Reset hardware Message-ID: References: <20210112125834.21545-1-digetx@gmail.com> <20210112125834.21545-3-digetx@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="/klCL1aWdNUFUILq" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/2.0.4 (26f41dd1) (2020-12-30) Precedence: bulk List-ID: X-Mailing-List: linux-tegra@vger.kernel.org --/klCL1aWdNUFUILq Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 18, 2021 at 02:39:37AM +0300, Dmitry Osipenko wrote: > 15.01.2021 18:35, Thierry Reding =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > On Tue, Jan 12, 2021 at 03:58:31PM +0300, Dmitry Osipenko wrote: > >> Reset hardware in order to bring it into a predictable state. > >> > >> Tested-by: Peter Geis > >> Tested-by: Nicolas Chauvet > >> Signed-off-by: Dmitry Osipenko > >> --- > >> sound/pci/hda/hda_tegra.c | 18 ++++++++++++++++++ > >> 1 file changed, 18 insertions(+) > >> > >> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c > >> index 4c799661c2f6..e406b7980f31 100644 > >> --- a/sound/pci/hda/hda_tegra.c > >> +++ b/sound/pci/hda/hda_tegra.c > >> @@ -17,6 +17,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> #include > >> #include > >> #include > >> @@ -70,6 +71,7 @@ > >> struct hda_tegra { > >> struct azx chip; > >> struct device *dev; > >> + struct reset_control *reset; > >> struct clk_bulk_data clocks[3]; > >> unsigned int nclocks; > >> void __iomem *regs; > >> @@ -167,6 +169,12 @@ static int __maybe_unused hda_tegra_runtime_resum= e(struct device *dev) > >> struct hda_tegra *hda =3D container_of(chip, struct hda_tegra, chip); > >> int rc; > >> =20 > >> + if (!(chip && chip->running)) { > >=20 > > Isn't that check for !chip a bit redundant? If that pointer isn't valid, > > we're just going to go crash when dereferencing hda later on, so I think > > this can simply be: > >=20 > > if (!chip->running) > >=20 > > I guess you took this from the inverse check below, but I think we can > > also drop it from there, perhaps in a separate patch. > >=20 > >> + rc =3D reset_control_assert(hda->reset); > >> + if (rc) > >> + return rc; > >> + } > >> + > >> rc =3D clk_bulk_prepare_enable(hda->nclocks, hda->clocks); > >> if (rc !=3D 0) > >> return rc; > >> @@ -176,6 +184,10 @@ static int __maybe_unused hda_tegra_runtime_resum= e(struct device *dev) > >> /* disable controller wake up event*/ > >> azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) & > >> ~STATESTS_INT_MASK); > >> + } else { > >> + rc =3D reset_control_reset(hda->reset); > >=20 > > The "if (chip)" part definitely doesn't make sense after this anymore > > because now if chip =3D=3D NULL, then we end up in here and dereference= an > > invalid "hda" pointer. >=20 > Okay, I took a note for the v3. >=20 > > Also, why reset_control_reset() here? We'll reach this if we ran > > reset_control_assert() above, so this should just be > > reset_control_deassert() to undo that, right? I suppose it wouldn't hurt > > to put throw that standard usleep_range() in there as well that we use > > to wait between reset assert and deassert to make sure the clocks have > > stabilized and the reset has indeed propagated through the whole IP. >=20 > The reset_control_reset() does the delaying before the deassert, i.e. it > does assert -> udelay(1) -> deassert. >=20 > https://elixir.free-electrons.com/linux/v5.11-rc3/source/drivers/clk/tegr= a/clk.c#L133 >=20 > The reset_control_reset() usage appears to be a bit more code-tidy > variant in comparison to delaying directly. But I don't mind to use > delay + reset_control_deassert() directly since it may not be obvious to > everyone what reset_control_reset() does. > I'll change it in v3. Thanks. I know that manually having to add the delay everywhere seems a bit tedious, but I like the way we very explicitly only ever do reset assert and deassert, rather than the combined reset pulse, because the latter can give the impression that the device isn't actually in reset when we do reset_control_reset(). Thierry --/klCL1aWdNUFUILq Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmAHFz8ACgkQ3SOs138+ s6Hyxw//SGrLuWocfMwUkHmxiyxq+LvXQERK6nWL9L0e+TX2eHKm68l8j3eCsY/t gMEXlaF60LxIqyVvah+Yg3uXAhiHOGzdeFWprkstQ9XKZZKyqjOpK9fUuzLwSa5m WXMti4whOfk3g5eQsotpK1C0pdY2cdfWzYyUFGkDRnrBUK4KZZ2+iQXtHeT1tVaV LzgYRXvzMX8dfCQYJx0w8wnEs3PGbIa+K1XsVpK5LxpXZe91LV4aaYRJZw68PHie APKx9lVi+gMwpYLtkPmGtkdT+stLMicaAccPHY9TBpYHzUbGFAVSTxWOM0j92efr JXkHOkRJy8W1cs0cl3CCz+b4XM/1CKjHIJpB9Z0fwymX13iCm2zKVEKDKxJBfoLV gQAWcJc7A0bjPftSk4cyeth3YCasQ7c/JXKDtNQVrXOYuqKmHBRq73gOZPtU/5mU ndwpZT72SBKf1co8GWGiq0sVTlxHQy7WgfscxxVHphzeuKCyxZHN+SsEAzID154p KIrRsSbp/L8AD0Mck/rPxPDapbH15Zlng5D4AothCnHoulVARtZLIpuxWxY0l84z Ud84sqD8vI3HQ3HtkbkkZKEcIZ//kJ7HhfGs8qEeUcX6arb/hb4tak/VYzraturM vSAFZ9a/joyT2zQlByjJsgunHfkyf5mTJuWWd0xIHiSZ3k9z3BU= =eV6P -----END PGP SIGNATURE----- --/klCL1aWdNUFUILq--