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=-10.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 34CE0C5519F for ; Sat, 14 Nov 2020 16:43:09 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A339820719 for ; Sat, 14 Nov 2020 16:43:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="v+b9Wlvn" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A339820719 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=YEHhvClPBqXK+VLYSbUyE/toY1DvhZ0Qegipz5nzq2Y=; b=v+b9Wlvn5cqBfhO8dgQqsTntM ZEUQyjwniuplMrg+kc3O/dSf1G7qvJbpcKPzuTbQsrmw0qJQS3sbE8enkFmjkQCVM9UZG0mHXTDID HtgBJz8Brs03qTK7YF2OuKNg+JXE+hyFZ0cbbTwcQEC4/61bWnfKSeG1ZRQ4jBQku16gg10LaT153 Rfxz/65caAmU8GuaTiGSSXaly9gC27lNr48RL66nkV+18TdvDSsCTbQpQt1ytiME3pFzL4wleWD7y V+DEaoATbIXhV1u6oO08SFUjExri8zIMS6lBjbQnxmckHDgVWE1s2Vl4aN6DNdLOX18u3qU/LNyG9 H+KGkio/g==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdycJ-0000Vo-Ny; Sat, 14 Nov 2020 16:41:35 +0000 Received: from mail-wr1-f67.google.com ([209.85.221.67]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdycG-0000VO-CV for linux-arm-kernel@lists.infradead.org; Sat, 14 Nov 2020 16:41:33 +0000 Received: by mail-wr1-f67.google.com with SMTP id p8so13758023wrx.5 for ; Sat, 14 Nov 2020 08:41:31 -0800 (PST) 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; bh=SpRx3Z/j6jHk5nd0jo6lnZDJH72gLG9Ify1E26wOmjY=; b=QFqAh5r2TxS+mlmBMSrhZWTjA5he4Y0xTrGJdFQrN62scFYKhlJXI3ICfDos0kBmsQ eT+uuFvx2Z526ZSv3mwFOTg9zz5Hj65UdFn/x27Ttqh41biOmTcNyBkXdjZyMYnmSRdg rd0m2PivsTABspF7PczO8fDEalj27YgxXiIm1fRwgUU/sdTCupO3Mp9XSo1CPzaz96xo sptzR0C8r5p3PUZMimo9tv91eIEjJrTzAiWUOrowlomFBnCzXb8V6UNV4kPsHdUV1ELP pjiKL9uQW/qq7OXfBok6OxP8/Pcex52aa5nE8lJ1gFoQr0HIdT1KeXop8ST482Qs5ZHO CUDw== X-Gm-Message-State: AOAM533sWdPWCvhM3U53JD+16OJ2+YlRYbzXIi30lR29uENhntDQdlmE U7c3SKM/FmYIttvZ4VOM4GU= X-Google-Smtp-Source: ABdhPJyL14R2ZiuceH6wNj3z0v7yMwHY3AYKtn5uC4XwjXoTIiJeYjfR4KWRbyrAuMK4eFNIZCqWag== X-Received: by 2002:adf:b358:: with SMTP id k24mr9375362wrd.388.1605372090946; Sat, 14 Nov 2020 08:41:30 -0800 (PST) Received: from kozik-lap (adsl-84-226-167-205.adslplus.ch. [84.226.167.205]) by smtp.googlemail.com with ESMTPSA id c6sm16655400wrh.74.2020.11.14.08.41.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 14 Nov 2020 08:41:29 -0800 (PST) Date: Sat, 14 Nov 2020 17:41:28 +0100 From: Krzysztof Kozlowski To: Alice Guo Subject: Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver Message-ID: <20201114164128.GD14989@kozik-lap> References: <20201113110409.13546-1-alice.guo@nxp.com> <20201113110409.13546-4-alice.guo@nxp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201113110409.13546-4-alice.guo@nxp.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201114_114132_457958_23B27BA0 X-CRM114-Status: GOOD ( 36.89 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, peng.fan@nxp.com, s.hauer@pengutronix.de, linux-kernel@vger.kernel.org, robh+dt@kernel.org, linux-imx@nxp.com, shawnguo@kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Nov 13, 2020 at 07:04:09PM +0800, Alice Guo wrote: > Directly reading ocotp register depends on that bootloader enables ocotp > clk, which is not always effective, so change to use nvmem API. Using > nvmem API requires to support driver defer probe and thus change > soc-imx8m.c to use platform driver. > > The other reason is that directly reading ocotp register causes kexec > kernel hang because the 1st kernel running will disable unused clks > after kernel boots up, and then ocotp clk will be disabled even if > bootloader enables it. When kexec kernel, ocotp clk needs to be enabled > before reading ocotp registers, and nvmem API with platform driver > supported can accomplish this. > > Old .dts files can also work. > > Signed-off-by: Alice Guo > --- > drivers/soc/imx/soc-imx8m.c | 89 ++++++++++++++++++++++++++++++++----- > 1 file changed, 79 insertions(+), 10 deletions(-) > > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c > index cc57a384d74d..af2c0dbe8291 100644 > --- a/drivers/soc/imx/soc-imx8m.c > +++ b/drivers/soc/imx/soc-imx8m.c > @@ -5,6 +5,8 @@ > > #include > #include > +#include > +#include > #include > #include > #include > @@ -29,7 +31,7 @@ > > struct imx8_soc_data { > char *name; > - u32 (*soc_revision)(void); > + u32 (*soc_revision)(struct device *dev, int flag); > }; > > static u64 soc_uid; > @@ -50,7 +52,7 @@ static u32 imx8mq_soc_revision_from_atf(void) > static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; }; > #endif > > -static u32 __init imx8mq_soc_revision(void) > +static u32 __init imx8mq_soc_revision(struct device *dev, int flag) > { > struct device_node *np; > void __iomem *ocotp_base; > @@ -75,9 +77,17 @@ static u32 __init imx8mq_soc_revision(void) > rev = REV_B1; > } > > - soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH); > - soc_uid <<= 32; > - soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW); > + if (flag) { > + int ret = 0; > + > + ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid); > + if (ret) > + return ret; > + } else { > + soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH); > + soc_uid <<= 32; > + soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW); > + } > > iounmap(ocotp_base); > of_node_put(np); > @@ -107,7 +117,7 @@ static void __init imx8mm_soc_uid(void) > of_node_put(np); > } > > -static u32 __init imx8mm_soc_revision(void) > +static u32 __init imx8mm_soc_revision(struct device *dev, int flag) > { > struct device_node *np; > void __iomem *anatop_base; > @@ -125,7 +135,15 @@ static u32 __init imx8mm_soc_revision(void) > iounmap(anatop_base); > of_node_put(np); > > - imx8mm_soc_uid(); > + if (flag) { > + int ret = 0; > + > + ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid); > + if (ret) > + return ret; > + } else { > + imx8mm_soc_uid(); > + } > > return rev; > } > @@ -158,12 +176,21 @@ static __maybe_unused const struct of_device_id imx8_soc_match[] = { > { } > }; > > +static __maybe_unused const struct of_device_id imx8m_soc_match[] = { Could this really be unused? > + { .compatible = "fsl,imx8mq-soc", .data = &imx8mq_soc_data, }, > + { .compatible = "fsl,imx8mm-soc", .data = &imx8mm_soc_data, }, > + { .compatible = "fsl,imx8mn-soc", .data = &imx8mn_soc_data, }, > + { .compatible = "fsl,imx8mp-soc", .data = &imx8mp_soc_data, }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, imx8m_soc_match); You already have "imx8_soc_match" which covers imx8m and now you add "imx8m_soc_match" which also covers imx8m. Such naming is a pure confusion. > + > #define imx8_revision(soc_rev) \ > soc_rev ? \ > kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf, soc_rev & 0xf) : \ > "unknown" > > -static int __init imx8_soc_init(void) > +static int imx8_soc_init_flag(struct platform_device *pdev, int flag) > { > struct soc_device_attribute *soc_dev_attr; > struct soc_device *soc_dev; > @@ -182,7 +209,10 @@ static int __init imx8_soc_init(void) > if (ret) > goto free_soc; > > - id = of_match_node(imx8_soc_match, of_root); > + if (flag) > + id = of_match_node(imx8m_soc_match, pdev->dev.of_node); > + else > + id = of_match_node(imx8_soc_match, of_root); > if (!id) { > ret = -ENODEV; > goto free_soc; > @@ -192,7 +222,13 @@ static int __init imx8_soc_init(void) > if (data) { > soc_dev_attr->soc_id = data->name; > if (data->soc_revision) > - soc_rev = data->soc_revision(); > + soc_rev = data->soc_revision(&pdev->dev, flag); > + > + if (flag) { > + ret = soc_rev; > + if (ret < 0) > + goto free_soc; > + } > } > > soc_dev_attr->revision = imx8_revision(soc_rev); > @@ -230,4 +266,37 @@ static int __init imx8_soc_init(void) > kfree(soc_dev_attr); > return ret; > } > + > +static int __init imx8_soc_init(void) > +{ > + int ret = 0, flag = 0; > + > + if (of_find_compatible_node(NULL, NULL, "fsl,imx8mm-soc") || > + of_find_compatible_node(NULL, NULL, "fsl,imx8mn-soc") || > + of_find_compatible_node(NULL, NULL, "fsl,imx8mp-soc") || > + of_find_compatible_node(NULL, NULL, "fsl,imx8mq-soc")) Missing puts. Don't duplicate the compatibles, iterate over existing structure... or see comments below. Maybe you could simplify it with something like of_find_matching_node_and_match()... but check comments below. > + return 0; > + > + ret = imx8_soc_init_flag(NULL, flag); > + return ret; > +} > device_initcall(imx8_soc_init); Where is the changelog? This was removed previously, now it stays... After more thoughs, it looks you have kept it for the purpose of supporting existing DTB, but it is not explained. Neither in the source code (which after applying this patch looks confusing) nor in commit message. In case of old DTB without fsl,imx8mm-soc-like compatibles, it would be better to still register a platform driver and create a device (of_platform_device_create())). However still this won't solve the problem of actually missing device node... so maybe this double entry point is acceptable, if properly explained. > + > +static int imx8_soc_init_probe(struct platform_device *pdev) > +{ > + int ret = 0, flag = 1; > + > + ret = imx8_soc_init_flag(pdev, flag); Never name unspecified booleans like "flag". The same as string variables should be named "string", integers should not be named "number". > + return ret; > +} > + > +static struct platform_driver imx8_soc_init_driver = { > + .probe = imx8_soc_init_probe, > + .driver = { > + .name = "imx8_soc_init", > + .of_match_table = of_match_ptr(imx8m_soc_match), Can it bind without OF? Why it's a of_match_ptr()? > + }, > +}; > + > +module_platform_driver(imx8_soc_init_driver); Here and in all other places (including driver name) this is not a SoC initialization (init) driver. You cannot initialize a SoC. This looks like a SoC ID driver, so one name could be "imx8_soc_id". Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel