lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <7b8cba35-c477-2b0d-69a1-4d6814869273@arm.com> Date: Mon, 20 Mar 2023 09:58:08 +0530 From: Anshuman Khandual <anshuman.khandual@....com> To: Suzuki K Poulose <suzuki.poulose@....com>, linux-arm-kernel@...ts.infradead.org, coresight@...ts.linaro.org Cc: scclevenger@...amperecomputing.com, Rob Herring <robh+dt@...nel.org>, Frank Rowand <frowand.list@...il.com>, Russell King <linux@...linux.org.uk>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>, Sudeep Holla <sudeep.holla@....com>, Lorenzo Pieralisi <lpieralisi@...nel.org>, Mathieu Poirier <mathieu.poirier@...aro.org>, Mike Leach <mike.leach@...aro.org>, Leo Yan <leo.yan@...aro.org>, devicetree@...r.kernel.org, linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH 4/7] coresight: etm4x: Change etm4_platform_driver driver for MMIO devices 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 <mathieu.poirier@...aro.org> >> Cc: Suzuki K Poulose <suzuki.poulose@....com> >> Cc: Mike Leach <mike.leach@...aro.org> >> Cc: Leo Yan <leo.yan@...aro.org> >> Cc: coresight@...ts.linaro.org >> Cc: linux-arm-kernel@...ts.infradead.org >> Cc: linux-kernel@...r.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com> >> --- >> .../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 <linux/platform_device.h> >> #include <linux/pm_runtime.h> >> #include <linux/property.h> >> +#include <linux/clk/clk-conf.h> >> #include <asm/barrier.h> >> #include <asm/sections.h> >> @@ -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 <linux/amba/bus.h> >> +#include <linux/clk.h> >> #include <linux/device.h> >> #include <linux/io.h> >> #include <linux/perf_event.h> >> @@ -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 */
Powered by blists - more mailing lists