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@xxxxxxxxxx>
>> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>> Cc: Mike Leach <mike.leach@xxxxxxxxxx>
>> Cc: Leo Yan <leo.yan@xxxxxxxxxx>
>> Cc: coresight@xxxxxxxxxxxxxxxx
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>> .../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
*/
