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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C5A94C433EF for ; Tue, 19 Oct 2021 11:41:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B28056137D for ; Tue, 19 Oct 2021 11:41:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235390AbhJSLnO (ORCPT ); Tue, 19 Oct 2021 07:43:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53982 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230231AbhJSLnH (ORCPT ); Tue, 19 Oct 2021 07:43:07 -0400 Received: from mail-lf1-x132.google.com (mail-lf1-x132.google.com [IPv6:2a00:1450:4864:20::132]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F165EC061745 for ; Tue, 19 Oct 2021 04:40:54 -0700 (PDT) Received: by mail-lf1-x132.google.com with SMTP id x192so6699406lff.12 for ; Tue, 19 Oct 2021 04:40:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=p/y6DgAZycykowwrleZpy70Yak++qIhNqJ9ImugcYMs=; b=gbZb/rIEkq16uEpMdDfbn531uPXxDE2vXRTZ1At7zc8FyijjHf+TaWP9WSCNsx9CRl Ax+uAwkmd3zCdbRdDaM3dpQ6QH2wtz6LdoMLA6+gjkzDI8FsGa3Gg4WLu7V9K5SrUrby y6A+UktAxyprVLbdSpurxxaZ8mnWtv03QgVC25vrqwdz6F5CjpkyH0G67ZL5o2xSalbx z2YLwvEkGXJDZVRENSUyiANgZXnd7Hl65An187cV1TSi/tcYxCoWRVEqIP0BwzqCyQr0 hoZBxk1g0lrSFu4sw+cYxe+pvM8G9j1I2iiX+c9B9ryltLhmQtiX2zDR77yR1QolGdKK MOng== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=p/y6DgAZycykowwrleZpy70Yak++qIhNqJ9ImugcYMs=; b=7qCQTJtCxGu5JPUYOCg6VuQfF0y+qBD7ZSz/jhXIcGTBY5LzVm0zIfte5NYJYdvFan C+GbkbNvWJ1Ovm7g4OfD7LdvMvbKyYnikizrJOmArfVPTSTbbTib4Dl1OekMF0lnclhs b4NLdeTM432Z53jkoZwYRQwqSlbbpZhylmF1kEgp4lTiF8f4ulTS4KPok++wU+PUy7mL 34CLR0MatXaXH5qlczZgy0n8xNfWv0iwCoUD6oTHmYp8y9aaJGnC87hJ2Gi3eCMuhnvc dMJie7gl3JhAvu1P323h74kfutI9G1ldPjHO6RXwRyXM8vocYGHh8CTU3kKBxV4GAvyA rc1Q== X-Gm-Message-State: AOAM530kNEnY9oYGEg2ryrloPzb+gXcK86YzNPtQhPn3+cIF/tyXUwwN OGt4qbUnniw3/bgcqRGhl99KxWdPYWqSDJoweAJ7GA== X-Google-Smtp-Source: ABdhPJyiZ8hxPaWs5ZGwktR/58DXEHsnrg2iCZG7+dtLEFQZezq+DvEjCTIwbhL0fm341kLJAfk/05h60QX3kuXQ9G4= X-Received: by 2002:a05:6512:3084:: with SMTP id z4mr5314051lfd.167.1634643653278; Tue, 19 Oct 2021 04:40:53 -0700 (PDT) MIME-Version: 1.0 References: <20210926224058.1252-1-digetx@gmail.com> <20210926224058.1252-21-digetx@gmail.com> <0bcbcd3d-2154-03d2-f572-dc9032125c26@gmail.com> <073114ea-490b-89a9-e82d-852b34cb11df@gmail.com> In-Reply-To: <073114ea-490b-89a9-e82d-852b34cb11df@gmail.com> From: Ulf Hansson Date: Tue, 19 Oct 2021 13:40:16 +0200 Message-ID: Subject: Re: [PATCH v13 20/35] mtd: rawnand: tegra: Add runtime PM and OPP support To: Dmitry Osipenko Cc: Thierry Reding , Jonathan Hunter , Viresh Kumar , Stephen Boyd , Peter De Schrijver , Mikko Perttunen , Peter Chen , Lee Jones , =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= , Nishanth Menon , Adrian Hunter , Michael Turquette , Linux Kernel Mailing List , linux-tegra , Linux PM , Linux USB List , linux-staging@lists.linux.dev, linux-pwm@vger.kernel.org, linux-mmc , dri-devel , DTML , linux-clk , Mark Brown , Vignesh Raghavendra , Richard Weinberger , Miquel Raynal , Lucas Stach , Stefan Agner , Mauro Carvalho Chehab , David Heidelberg Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 17 Oct 2021 at 10:38, Dmitry Osipenko wrote: > > 01.10.2021 18:01, Ulf Hansson =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > On Fri, 1 Oct 2021 at 16:35, Dmitry Osipenko wrote: > >> > >> 01.10.2021 17:24, Ulf Hansson =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > >>> On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wro= te: > >>>> > >>>> The NAND on Tegra belongs to the core power domain and we're going t= o > >>>> enable GENPD support for the core domain. Now NAND must be resumed u= sing > >>>> runtime PM API in order to initialize the NAND power state. Add runt= ime PM > >>>> and OPP support to the NAND driver. > >>>> > >>>> Acked-by: Miquel Raynal > >>>> Signed-off-by: Dmitry Osipenko > >>>> --- > >>>> drivers/mtd/nand/raw/tegra_nand.c | 55 ++++++++++++++++++++++++++--= --- > >>>> 1 file changed, 47 insertions(+), 8 deletions(-) > >>>> > >>>> diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/ra= w/tegra_nand.c > >>>> index 32431bbe69b8..098fcc9cb9df 100644 > >>>> --- a/drivers/mtd/nand/raw/tegra_nand.c > >>>> +++ b/drivers/mtd/nand/raw/tegra_nand.c > >>>> @@ -17,8 +17,11 @@ > >>>> #include > >>>> #include > >>>> #include > >>>> +#include > >>>> #include > >>>> > >>>> +#include > >>>> + > >>>> #define COMMAND 0x00 > >>>> #define COMMAND_GO BIT(31) > >>>> #define COMMAND_CLE BIT(30) > >>>> @@ -1151,6 +1154,7 @@ static int tegra_nand_probe(struct platform_de= vice *pdev) > >>>> return -ENOMEM; > >>>> > >>>> ctrl->dev =3D &pdev->dev; > >>>> + platform_set_drvdata(pdev, ctrl); > >>>> nand_controller_init(&ctrl->controller); > >>>> ctrl->controller.ops =3D &tegra_nand_controller_ops; > >>>> > >>>> @@ -1166,14 +1170,22 @@ static int tegra_nand_probe(struct platform_= device *pdev) > >>>> if (IS_ERR(ctrl->clk)) > >>>> return PTR_ERR(ctrl->clk); > >>>> > >>>> - err =3D clk_prepare_enable(ctrl->clk); > >>>> + err =3D devm_pm_runtime_enable(&pdev->dev); > >>>> + if (err) > >>>> + return err; > >>>> + > >>>> + err =3D devm_tegra_core_dev_init_opp_table_common(&pdev->dev= ); > >>>> + if (err) > >>>> + return err; > >>>> + > >>>> + err =3D pm_runtime_resume_and_get(&pdev->dev); > >>>> if (err) > >>>> return err; > >>>> > >>>> err =3D reset_control_reset(rst); > >>>> if (err) { > >>>> dev_err(ctrl->dev, "Failed to reset HW: %d\n", err); > >>>> - goto err_disable_clk; > >>>> + goto err_put_pm; > >>>> } > >>>> > >>>> writel_relaxed(HWSTATUS_CMD_DEFAULT, ctrl->regs + HWSTATUS_C= MD); > >>>> @@ -1188,21 +1200,19 @@ static int tegra_nand_probe(struct platform_= device *pdev) > >>>> dev_name(&pdev->dev), ctrl); > >>>> if (err) { > >>>> dev_err(ctrl->dev, "Failed to get IRQ: %d\n", err); > >>>> - goto err_disable_clk; > >>>> + goto err_put_pm; > >>>> } > >>>> > >>>> writel_relaxed(DMA_MST_CTRL_IS_DONE, ctrl->regs + DMA_MST_CT= RL); > >>>> > >>>> err =3D tegra_nand_chips_init(ctrl->dev, ctrl); > >>>> if (err) > >>>> - goto err_disable_clk; > >>>> - > >>>> - platform_set_drvdata(pdev, ctrl); > >>>> + goto err_put_pm; > >>>> > >>> > >>> There is no corresponding call pm_runtime_put() here. Is it > >>> intentional to always leave the device runtime resumed after ->probe(= ) > >>> has succeeded? > >>> > >>> I noticed you included some comments about this for some other > >>> drivers, as those needed more tweaks. Is that also the case for this > >>> driver? > >> > >> Could you please clarify? There is pm_runtime_put() in both probe-erro= r > >> and remove() code paths here. > > > > I was not considering the error path of ->probe() (or ->remove()), but > > was rather thinking about when ->probe() completes successfully. Then > > you keep the device runtime resumed, because you have called > > pm_runtime_resume_and_get() for it. > > > > Shouldn't you have a corresponding pm_runtime_put() in ->probe(), > > allowing it to be runtime suspended, until the device is really needed > > later on. No? > > This driver doesn't support active power management. I don't have Tegra > hardware that uses NAND storage for testing, so it's up to somebody else > to implement dynamic power management. NAND doesn't require high > voltages, so it's fine to keep the old driver behaviour by keeping > hardware resumed since the probe time. Alright, fair enough and thanks for clarifying! Kind regards Uffe