| Message ID | 20230317030501.1811905-5-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | coresight: etm4x: Migrate AMBA devices to platform driver | expand |
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. " > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Mike Leach <mike.leach@linaro.org> > Cc: Leo Yan <leo.yan@linaro.org> > Cc: coresight@lists.linaro.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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. > @@ -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 ? > + } > + > 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. > 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 ? > + > +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. > 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. Suzuki
Hi Anshuman,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on soc/for-next rafael-pm/linux-next linus/master v6.3-rc2 next-20230317]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230317030501.1811905-5-anshuman.khandual%40arm.com
patch subject: [PATCH 4/7] coresight: etm4x: Change etm4_platform_driver driver for MMIO devices
config: arm64-buildonly-randconfig-r002-20230312 (https://download.01.org/0day-ci/archive/20230318/202303181800.KxbuwjRT-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/f02ad9e7f97ab4fc1f90c7e6399004e9ec89ef26
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755
git checkout f02ad9e7f97ab4fc1f90c7e6399004e9ec89ef26
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/hwtracing/coresight/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303181800.KxbuwjRT-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/hwtracing/coresight/coresight-etm4x-core.c:2336:32: warning: unused variable 'etm4_dev_pm_ops' [-Wunused-const-variable]
static const struct dev_pm_ops etm4_dev_pm_ops = {
^
1 warning generated.
vim +/etm4_dev_pm_ops +2336 drivers/hwtracing/coresight/coresight-etm4x-core.c
2335
> 2336 static const struct dev_pm_ops etm4_dev_pm_ops = {
2337 SET_RUNTIME_PM_OPS(etm4_runtime_suspend, etm4_runtime_resume, NULL)
2338 };
2339
On 3/18/23 15:54, kernel test robot wrote: > Hi Anshuman, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on robh/for-next] > [also build test WARNING on soc/for-next rafael-pm/linux-next linus/master v6.3-rc2 next-20230317] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755 > base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next > patch link: https://lore.kernel.org/r/20230317030501.1811905-5-anshuman.khandual%40arm.com > patch subject: [PATCH 4/7] coresight: etm4x: Change etm4_platform_driver driver for MMIO devices > config: arm64-buildonly-randconfig-r002-20230312 (https://download.01.org/0day-ci/archive/20230318/202303181800.KxbuwjRT-lkp@intel.com/config) > compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # install arm64 cross compiling tool for clang build > # apt-get install binutils-aarch64-linux-gnu > # https://github.com/intel-lab-lkp/linux/commit/f02ad9e7f97ab4fc1f90c7e6399004e9ec89ef26 > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755 > git checkout f02ad9e7f97ab4fc1f90c7e6399004e9ec89ef26 > # save the config file > mkdir build_dir && cp config build_dir/.config > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/hwtracing/coresight/ > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > | Link: https://lore.kernel.org/oe-kbuild-all/202303181800.KxbuwjRT-lkp@intel.com/ > > All warnings (new ones prefixed by >>): > >>> drivers/hwtracing/coresight/coresight-etm4x-core.c:2336:32: warning: unused variable 'etm4_dev_pm_ops' [-Wunused-const-variable] > static const struct dev_pm_ops etm4_dev_pm_ops = { These pm_ops needs to tagged along 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, }, };
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@linaro.org> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Cc: Mike Leach <mike.leach@linaro.org> >> Cc: Leo Yan <leo.yan@linaro.org> >> Cc: coresight@lists.linaro.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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 */
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 @@ -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); + } + 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); 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) +}; + +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; 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. + */ +static inline struct clk *coresight_get_enable_apb_pclk(struct device *dev) +{ + struct clk *pclk; + int ret; + + pclk = clk_get(dev, "apb_pclk"); + if (IS_ERR(pclk)) + return NULL; + + ret = clk_prepare_enable(pclk); + if (ret) { + clk_put(pclk); + return ERR_PTR(ret); + } + return pclk; +} + #define CORESIGHT_PIDRn(i) (0xFE0 + ((i) * 4)) static inline u32 coresight_get_pid(struct csdev_access *csa)
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. Cc: Mathieu Poirier <mathieu.poirier@linaro.org> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Mike Leach <mike.leach@linaro.org> Cc: Leo Yan <leo.yan@linaro.org> Cc: coresight@lists.linaro.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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(-)