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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EA95FC7618A for ; Mon, 20 Mar 2023 04:28:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229756AbjCTE20 (ORCPT ); Mon, 20 Mar 2023 00:28:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56738 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229460AbjCTE2V (ORCPT ); Mon, 20 Mar 2023 00:28:21 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 9E8EC144B3; Sun, 19 Mar 2023 21:28:18 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 749DEFEC; Sun, 19 Mar 2023 21:29:01 -0700 (PDT) Received: from [10.162.41.7] (a077893.blr.arm.com [10.162.41.7]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B25AB3F67D; Sun, 19 Mar 2023 21:28:11 -0700 (PDT) Message-ID: <7b8cba35-c477-2b0d-69a1-4d6814869273@arm.com> Date: Mon, 20 Mar 2023 09:58:08 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [PATCH 4/7] coresight: etm4x: Change etm4_platform_driver driver for MMIO devices Content-Language: en-US To: Suzuki K Poulose , linux-arm-kernel@lists.infradead.org, coresight@lists.linaro.org Cc: scclevenger@os.amperecomputing.com, Rob Herring , Frank Rowand , Russell King , Greg Kroah-Hartman , "Rafael J. Wysocki" , Len Brown , Sudeep Holla , Lorenzo Pieralisi , Mathieu Poirier , Mike Leach , Leo Yan , devicetree@vger.kernel.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org References: <20230317030501.1811905-1-anshuman.khandual@arm.com> <20230317030501.1811905-5-anshuman.khandual@arm.com> From: Anshuman Khandual In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 3/17/23 15:02, Suzuki K Poulose wrote: > On 17/03/2023 03:04, Anshuman Khandual wrote: >> This updates existing etm4_platform_driver to accommodate MMIO based device >> tree represented coresight etm4x devices along with current sysreg ones. It >> first looks for 'apb_clk' clock and tries to enable it via a new helper i.e >> coresight_get_enable_apb_pclk(). If 'apb_clock' is not found on the system >> as indicated by a return value 'NULL', ignore and proceed further assuming >> that platform already has got required clocks enabled. But if the clock is >> but could not be enabled, device probe fails with -ENODEV. Similarly iomem >> base address is fetched via devm_ioremap_resource() onyl when the platform >> has valid 'struct resource'. The probed device is ensured to be a coresight >> etm4x, via two new helpers in etm4_init_iomem_access(). This also registers >> runtime power management callbacks i.e for suspend and resume operations. > > This looks too much detail about the actual implementation of HOW rather > than WHAT. > > Could it be something like : > > "Add support for handling MMIO based devices via platform driver. We > need to make sure that : >   1) The APB clock, if present is enabled at probe and via runtime_pm ops. >   2) Use the ETM4x architecture/CoreSight Architecture registers to >      identify a device as CoreSight ETM4x, instead of relying a white >      list of "Peripheral IDs" > The driver doesn't get to handle the devices yet, until we wire the ACPI > and DT changes to move the devices to be handled via platform driver > than the etm4_amba driver.> " Sure, will update the commit message as above. > >> >> Cc: Mathieu Poirier >> Cc: Suzuki K Poulose >> Cc: Mike Leach >> Cc: Leo Yan >> Cc: coresight@lists.linaro.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual >> --- >>   .../coresight/coresight-etm4x-core.c          | 62 +++++++++++++++++-- >>   drivers/hwtracing/coresight/coresight-etm4x.h |  3 + >>   include/linux/coresight.h                     | 44 +++++++++++++ >>   3 files changed, 105 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> index a4c138e67920..60f027e33aa0 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> @@ -30,6 +30,7 @@ >>   #include >>   #include >>   #include >> +#include >>     #include >>   #include >> @@ -1067,12 +1068,24 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata, >>       return true; >>   } >>   +static bool is_etm4x_device(void __iomem *base) >> +{ >> +    u32 devarch = readl(base + TRCDEVARCH); >> +    u32 devtype = readl(base + TRCDEVTYPE); >> + >> +    return (((devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH) && >> +        (devtype == ETM_DEVTYPE_ETMv4x_ARCH)); >> +} >> + >>   static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata, >>                      struct csdev_access *csa) >>   { >>       u32 devarch = readl_relaxed(drvdata->base + TRCDEVARCH); >>       u32 idr1 = readl_relaxed(drvdata->base + TRCIDR1); >>   +    if (!is_coresight_device(drvdata->base) || !is_etm4x_device(drvdata->base)) >> +        return false; >> + >>       /* >>        * All ETMs must implement TRCDEVARCH to indicate that >>        * the component is an ETMv4. To support any broken > > Now that we do the is_etm4x_device(), we could the following ^^ > TRCDEVARCH check. In order to keep the change at minimum, is_etm4x_device() has been changed to assert device type but not the devarch itself, which is being checked in the existing code. --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1069,13 +1069,11 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata, return true; } -static bool is_etm4x_device(void __iomem *base) +static bool is_etm4x_devtype(void __iomem *base) { - u32 devarch = readl(base + TRCDEVARCH); u32 devtype = readl(base + TRCDEVTYPE); - return (((devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH) && - (devtype == ETM_DEVTYPE_ETMv4x_ARCH)); + return (devtype == ETM_DEVTYPE_ETMv4x_ARCH); } static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata, @@ -1084,7 +1082,7 @@ static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata, u32 devarch = readl_relaxed(drvdata->base + TRCDEVARCH); u32 idr1 = readl_relaxed(drvdata->base + TRCIDR1); - if (!is_coresight_device(drvdata->base) || !is_etm4x_device(drvdata->base)) + if (!is_coresight_device(drvdata->base) || !is_etm4x_devtype(drvdata->base)) return false; /* > >> @@ -2133,6 +2146,7 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id) >>     static int etm4_probe_platform_dev(struct platform_device *pdev) >>   { >> +    struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>       struct etmv4_drvdata *drvdata; >>       int ret; >>   @@ -2140,7 +2154,16 @@ static int etm4_probe_platform_dev(struct platform_device *pdev) >>       if (!drvdata) >>           return -ENOMEM; >>   -    drvdata->base = NULL; >> +    drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev); >> +    if (IS_ERR(drvdata->pclk)) >> +        return -ENODEV; >> + >> +    if (res) { >> +        drvdata->base = devm_ioremap_resource(&pdev->dev, res); >> +        if (IS_ERR(drvdata->base)) >> +            return PTR_ERR(drvdata->base); > > Drop the clock reference ? Right, will drop the clock here. if (res) { drvdata->base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(drvdata->base)) { clk_put(drvdata->pclk); return PTR_ERR(drvdata->base); } } > >> +    } >> + >>       dev_set_drvdata(&pdev->dev, drvdata); >>       pm_runtime_get_noresume(&pdev->dev); >>       pm_runtime_set_active(&pdev->dev); >> @@ -2186,7 +2209,7 @@ static struct amba_cs_uci_id uci_id_etm4[] = { >>           /*  ETMv4 UCI data */ >>           .devarch    = ETM_DEVARCH_ETMv4x_ARCH, >>           .devarch_mask    = ETM_DEVARCH_ID_MASK, >> -        .devtype    = 0x00000013, >> +        .devtype    = ETM_DEVTYPE_ETMv4x_ARCH, >>       } >>   }; >>   @@ -2244,6 +2267,10 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev) >>         if (drvdata) >>           ret = etm4_remove_dev(drvdata); >> + >> +    if (!IS_ERR(drvdata->pclk)) >> +        clk_put(drvdata->pclk); >> + >>       pm_runtime_disable(&pdev->dev); > > If we re-order the clk_put() after pm_runtime_disable(), we could use a label to jump here from the ioremap_resource failure. This is in etm4_remove_platform_dev() and there is no ioremap_resource() failure ? > >>       return ret; >>   } >> @@ -2284,7 +2311,34 @@ static struct amba_driver etm4x_amba_driver = { >>       .id_table    = etm4_ids, >>   }; >>   -static const struct of_device_id etm4_sysreg_match[] = { >> +#ifdef CONFIG_PM >> +static int etm4_runtime_suspend(struct device *dev) >> +{ >> +    struct etmv4_drvdata *drvdata = dev_get_drvdata(dev); >> + >> +    if (!IS_ERR(drvdata->pclk)) >> +        clk_disable_unprepare(drvdata->pclk); >> + >> +    return 0; >> +} >> + >> +static int etm4_runtime_resume(struct device *dev) >> +{ >> +    struct etmv4_drvdata *drvdata = dev_get_drvdata(dev); >> + >> +    if (!IS_ERR(drvdata->pclk)) >> +        clk_prepare_enable(drvdata->pclk); >> + >> +    return 0; >> +} >> +#endif >> + >> +static const struct dev_pm_ops etm4_dev_pm_ops = { >> +    SET_RUNTIME_PM_OPS(etm4_runtime_suspend, etm4_runtime_resume, NULL) >> +}; > > Where is this hooked in ? These pm_ops needs to tagged with the platform driver. diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 3ce2b4911a49..fe10dd91183e 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -2280,6 +2280,7 @@ static struct platform_driver etm4_platform_driver = { .of_match_table = etm4_match, .acpi_match_table = ACPI_PTR(etm4x_acpi_ids), .suppress_bind_attrs = true, + .pm = &etm4_dev_pm_ops, }, }; > >> + >> +static const struct of_device_id etm4_match[] = { >> +    { .compatible    = "arm,coresight-etm4x" }, >>       { .compatible    = "arm,coresight-etm4x-sysreg" }, >>       { .compatible    = "arm,embedded-trace-extension" }, >>       {} >> @@ -2295,7 +2349,7 @@ static struct platform_driver etm4_platform_driver = { >>       .remove        = etm4_remove_platform_dev, >>       .driver            = { >>           .name            = "coresight-etm4x", >> -        .of_match_table        = etm4_sysreg_match, >> +        .of_match_table        = etm4_match, >>           .suppress_bind_attrs    = true, >>       }, >>   }; >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h >> index 434f4e95ee17..5a37df4a02e9 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x.h >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h >> @@ -701,6 +701,8 @@ >>   #define ETM_DEVARCH_ETE_ARCH                        \ >>       (ETM_DEVARCH_ARCHITECT_ARM | ETM_DEVARCH_ARCHID_ETE | ETM_DEVARCH_PRESENT) >>   +#define ETM_DEVTYPE_ETMv4x_ARCH        0x00000013 >> + >>   #define TRCSTATR_IDLE_BIT        0 >>   #define TRCSTATR_PMSTABLE_BIT        1 >>   #define ETM_DEFAULT_ADDR_COMP        0 >> @@ -1017,6 +1019,7 @@ struct etmv4_save_state { >>    * @arch_features: Bitmap of arch features of etmv4 devices. >>    */ >>   struct etmv4_drvdata { >> +    struct clk            *pclk; > > Please document the field, above the structure. Will add the following document for the field. --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -954,6 +954,7 @@ struct etmv4_save_state { /** * struct etm4_drvdata - specifics associated to an ETM component + * @pclk APB clock for this component * @base: Memory mapped base address for this component. * @csdev: Component vitals needed by the framework. * @spinlock: Only one at a time pls. > >>       void __iomem            *base; >>       struct coresight_device        *csdev; >>       spinlock_t            spinlock; >> diff --git a/include/linux/coresight.h b/include/linux/coresight.h >> index f85b041ea475..75a7aa6d7444 100644 >> --- a/include/linux/coresight.h >> +++ b/include/linux/coresight.h >> @@ -6,6 +6,8 @@ >>   #ifndef _LINUX_CORESIGHT_H >>   #define _LINUX_CORESIGHT_H >>   +#include >> +#include >>   #include >>   #include >>   #include >> @@ -370,6 +372,48 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa, >>       return csa->read(offset, true, false); >>   } >>   +#define CORESIGHT_CIDRn(i)    (0xFF0 + ((i) * 4)) >> + >> +static inline u32 coresight_get_cid(void __iomem *base) >> +{ >> +    u32 i, cid = 0; >> + >> +    for (i = 0; i < 4; i++) >> +        cid |= readl(base + CORESIGHT_CIDRn(i)) << (i * 8); >> + >> +    return cid; >> +} >> + >> +static inline bool is_coresight_device(void __iomem *base) >> +{ >> +    u32 cid = coresight_get_cid(base); >> + >> +    return cid == CORESIGHT_CID; >> +} >> + >> +/* >> + * This function attempts to find a 'apb_pclk' clock on the system and >> + * if found, enables it. This returns NULL if 'apb_pclk' clock is not >> + * and return error pointer from clk_prepare_enable(), if it fails to >> + * enable the discovered clock. > > minor nit: Easier if it is something like: > >     Attempt to find and enable "APB clock" for the given device. > >     Returns: >         NULL    - No clock found >         clk    - Clock is found and enabled. >         ERROR    - Failed to enable the clock. Sure, will change as follows (slightly reorganized) /* * Attempt to find and enable "APB clock" for the given device * * Returns: * * clk - Clock is found and enabled * NULL - clock is not found * ERROR - Clock is found but failed to enable */