| Message ID | 20231201062053.1268492-7-anshuman.khandual@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | coresight: Move remaining AMBA ACPI devices into platform driver | expand |
On 01/12/2023 06:20, Anshuman Khandual wrote: > Add support for the stm devices in the platform driver, which can then be > used on ACPI based platforms. This change would now allow runtime power > management for ACPI based systems. The driver would try to enable the APB > clock if available. > > Cc: Lorenzo Pieralisi <lpieralisi@kernel.org> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Mike Leach <mike.leach@linaro.org> > Cc: James Clark <james.clark@arm.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > Cc: linux-acpi@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Cc: coresight@lists.linaro.org > Cc: linux-stm32@st-md-mailman.stormreply.com > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- [...] > > -module_amba_driver(stm_driver); > +static int stm_platform_probe(struct platform_device *pdev) > +{ > + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + int ret = 0; > + > + pm_runtime_get_noresume(&pdev->dev); > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + > + ret = __stm_probe(&pdev->dev, res, NULL); Very minor nit, but this used to print this: coresight stm0: STM500 initialized And now it prints this: coresight stm0: (null) initialized (null) kind of makes it look a little bit like something has gone wrong. Maybe we could just put "initialised" if you don't have a string from ACPI?
On 12/4/23 15:53, James Clark wrote: > > > On 01/12/2023 06:20, Anshuman Khandual wrote: >> Add support for the stm devices in the platform driver, which can then be >> used on ACPI based platforms. This change would now allow runtime power >> management for ACPI based systems. The driver would try to enable the APB >> clock if available. >> >> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org> >> Cc: Sudeep Holla <sudeep.holla@arm.com> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Cc: Mike Leach <mike.leach@linaro.org> >> Cc: James Clark <james.clark@arm.com> >> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> >> Cc: linux-acpi@vger.kernel.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Cc: coresight@lists.linaro.org >> Cc: linux-stm32@st-md-mailman.stormreply.com >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- > [...] >> >> -module_amba_driver(stm_driver); >> +static int stm_platform_probe(struct platform_device *pdev) >> +{ >> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + int ret = 0; >> + >> + pm_runtime_get_noresume(&pdev->dev); >> + pm_runtime_set_active(&pdev->dev); >> + pm_runtime_enable(&pdev->dev); >> + >> + ret = __stm_probe(&pdev->dev, res, NULL); > > Very minor nit, but this used to print this: > > coresight stm0: STM500 initialized > > And now it prints this: > > coresight stm0: (null) initialized > > (null) kind of makes it look a little bit like something has gone wrong. > Maybe we could just put "initialised" if you don't have a string from ACPI? __stm_probe() gets called from both AMBA and platform driver paths. Even though a NULL check inside dev_info(..."%s initialized\n",...) could be added, but how to differentiate it from a scenario when coresight_get_uci_data() returns NULL ?
On Mon, Dec 04, 2023 at 10:23:49AM +0000, James Clark wrote: > > On 01/12/2023 06:20, Anshuman Khandual wrote: > > Add support for the stm devices in the platform driver, which can then be > > used on ACPI based platforms. This change would now allow runtime power > > management for ACPI based systems. The driver would try to enable the APB > > clock if available. > > > > Cc: Lorenzo Pieralisi <lpieralisi@kernel.org> > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > > Cc: Mike Leach <mike.leach@linaro.org> > > Cc: James Clark <james.clark@arm.com> > > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > > Cc: linux-acpi@vger.kernel.org > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: linux-kernel@vger.kernel.org > > Cc: coresight@lists.linaro.org > > Cc: linux-stm32@st-md-mailman.stormreply.com > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > > --- > [...] > > > > -module_amba_driver(stm_driver); > > +static int stm_platform_probe(struct platform_device *pdev) > > +{ > > + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + int ret = 0; > > + > > + pm_runtime_get_noresume(&pdev->dev); > > + pm_runtime_set_active(&pdev->dev); > > + pm_runtime_enable(&pdev->dev); > > + > > + ret = __stm_probe(&pdev->dev, res, NULL); > > Very minor nit, but this used to print this: > > coresight stm0: STM500 initialized > > And now it prints this: > > coresight stm0: (null) initialized > > (null) kind of makes it look a little bit like something has gone wrong. > Maybe we could just put "initialised" if you don't have a string from ACPI? Ah right, I too noticed this and forgot to mention. Just add a generic "STM" string for ACPI if we don't have a way to identify exact IP ? -- Regards, Sudeep
On 04/12/2023 11:50, Anshuman Khandual wrote: > > > On 12/4/23 15:53, James Clark wrote: >> >> >> On 01/12/2023 06:20, Anshuman Khandual wrote: >>> Add support for the stm devices in the platform driver, which can then be >>> used on ACPI based platforms. This change would now allow runtime power >>> management for ACPI based systems. The driver would try to enable the APB >>> clock if available. >>> >>> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org> >>> Cc: Sudeep Holla <sudeep.holla@arm.com> >>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >>> Cc: Mike Leach <mike.leach@linaro.org> >>> Cc: James Clark <james.clark@arm.com> >>> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> >>> Cc: linux-acpi@vger.kernel.org >>> Cc: linux-arm-kernel@lists.infradead.org >>> Cc: linux-kernel@vger.kernel.org >>> Cc: coresight@lists.linaro.org >>> Cc: linux-stm32@st-md-mailman.stormreply.com >>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>> --- >> [...] >>> >>> -module_amba_driver(stm_driver); >>> +static int stm_platform_probe(struct platform_device *pdev) >>> +{ >>> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + int ret = 0; >>> + >>> + pm_runtime_get_noresume(&pdev->dev); >>> + pm_runtime_set_active(&pdev->dev); >>> + pm_runtime_enable(&pdev->dev); >>> + >>> + ret = __stm_probe(&pdev->dev, res, NULL); >> >> Very minor nit, but this used to print this: >> >> coresight stm0: STM500 initialized >> >> And now it prints this: >> >> coresight stm0: (null) initialized >> >> (null) kind of makes it look a little bit like something has gone wrong. >> Maybe we could just put "initialised" if you don't have a string from ACPI? > > __stm_probe() gets called from both AMBA and platform driver paths. Even though > a NULL check inside dev_info(..."%s initialized\n",...) could be added, but how > to differentiate it from a scenario when coresight_get_uci_data() returns NULL ? Sudeep's suggestion seems ok, just add a hard coded string instead of the NULL. And keep the coresight_get_uci_data() the same.
On 12/4/23 18:47, James Clark wrote: > > > On 04/12/2023 11:50, Anshuman Khandual wrote: >> >> >> On 12/4/23 15:53, James Clark wrote: >>> >>> >>> On 01/12/2023 06:20, Anshuman Khandual wrote: >>>> Add support for the stm devices in the platform driver, which can then be >>>> used on ACPI based platforms. This change would now allow runtime power >>>> management for ACPI based systems. The driver would try to enable the APB >>>> clock if available. >>>> >>>> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org> >>>> Cc: Sudeep Holla <sudeep.holla@arm.com> >>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >>>> Cc: Mike Leach <mike.leach@linaro.org> >>>> Cc: James Clark <james.clark@arm.com> >>>> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >>>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> >>>> Cc: linux-acpi@vger.kernel.org >>>> Cc: linux-arm-kernel@lists.infradead.org >>>> Cc: linux-kernel@vger.kernel.org >>>> Cc: coresight@lists.linaro.org >>>> Cc: linux-stm32@st-md-mailman.stormreply.com >>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>>> --- >>> [...] >>>> >>>> -module_amba_driver(stm_driver); >>>> +static int stm_platform_probe(struct platform_device *pdev) >>>> +{ >>>> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> + int ret = 0; >>>> + >>>> + pm_runtime_get_noresume(&pdev->dev); >>>> + pm_runtime_set_active(&pdev->dev); >>>> + pm_runtime_enable(&pdev->dev); >>>> + >>>> + ret = __stm_probe(&pdev->dev, res, NULL); >>> >>> Very minor nit, but this used to print this: >>> >>> coresight stm0: STM500 initialized >>> >>> And now it prints this: >>> >>> coresight stm0: (null) initialized >>> >>> (null) kind of makes it look a little bit like something has gone wrong. >>> Maybe we could just put "initialised" if you don't have a string from ACPI? >> >> __stm_probe() gets called from both AMBA and platform driver paths. Even though >> a NULL check inside dev_info(..."%s initialized\n",...) could be added, but how >> to differentiate it from a scenario when coresight_get_uci_data() returns NULL ? > > Sudeep's suggestion seems ok, just add a hard coded string instead of > the NULL. And keep the coresight_get_uci_data() the same. Something like this works ? diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c index 3387ebc9d8ab..2b6834c4cac6 100644 --- a/drivers/hwtracing/coresight/coresight-stm.c +++ b/drivers/hwtracing/coresight/coresight-stm.c @@ -906,7 +906,7 @@ static int __stm_probe(struct device *dev, struct resource *res, void *dev_caps) pm_runtime_put(dev); dev_info(&drvdata->csdev->dev, "%s initialized\n", - (char *)dev_caps); + dev_caps ? (char *)dev_caps: "STM"); return 0; cs_unregister:
On Tue, Dec 05, 2023 at 10:50:19AM +0530, Anshuman Khandual wrote: > Something like this works ? > > diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c > index 3387ebc9d8ab..2b6834c4cac6 100644 > --- a/drivers/hwtracing/coresight/coresight-stm.c > +++ b/drivers/hwtracing/coresight/coresight-stm.c > @@ -906,7 +906,7 @@ static int __stm_probe(struct device *dev, struct resource *res, void *dev_caps) > pm_runtime_put(dev); > > dev_info(&drvdata->csdev->dev, "%s initialized\n", > - (char *)dev_caps); > + dev_caps ? (char *)dev_caps: "STM"); > return 0; > > cs_unregister: Yes, looks good to me.
On 05/12/2023 05:20, Anshuman Khandual wrote: > > > On 12/4/23 18:47, James Clark wrote: >> >> >> On 04/12/2023 11:50, Anshuman Khandual wrote: >>> >>> >>> On 12/4/23 15:53, James Clark wrote: >>>> >>>> >>>> On 01/12/2023 06:20, Anshuman Khandual wrote: >>>>> Add support for the stm devices in the platform driver, which can then be >>>>> used on ACPI based platforms. This change would now allow runtime power >>>>> management for ACPI based systems. The driver would try to enable the APB >>>>> clock if available. >>>>> >>>>> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org> >>>>> Cc: Sudeep Holla <sudeep.holla@arm.com> >>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >>>>> Cc: Mike Leach <mike.leach@linaro.org> >>>>> Cc: James Clark <james.clark@arm.com> >>>>> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >>>>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> >>>>> Cc: linux-acpi@vger.kernel.org >>>>> Cc: linux-arm-kernel@lists.infradead.org >>>>> Cc: linux-kernel@vger.kernel.org >>>>> Cc: coresight@lists.linaro.org >>>>> Cc: linux-stm32@st-md-mailman.stormreply.com >>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>>>> --- >>>> [...] >>>>> >>>>> -module_amba_driver(stm_driver); >>>>> +static int stm_platform_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> + int ret = 0; >>>>> + >>>>> + pm_runtime_get_noresume(&pdev->dev); >>>>> + pm_runtime_set_active(&pdev->dev); >>>>> + pm_runtime_enable(&pdev->dev); >>>>> + >>>>> + ret = __stm_probe(&pdev->dev, res, NULL); >>>> >>>> Very minor nit, but this used to print this: >>>> >>>> coresight stm0: STM500 initialized >>>> >>>> And now it prints this: >>>> >>>> coresight stm0: (null) initialized >>>> >>>> (null) kind of makes it look a little bit like something has gone wrong. >>>> Maybe we could just put "initialised" if you don't have a string from ACPI? >>> >>> __stm_probe() gets called from both AMBA and platform driver paths. Even though >>> a NULL check inside dev_info(..."%s initialized\n",...) could be added, but how >>> to differentiate it from a scenario when coresight_get_uci_data() returns NULL ? >> >> Sudeep's suggestion seems ok, just add a hard coded string instead of >> the NULL. And keep the coresight_get_uci_data() the same. > > Something like this works ? > > diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c > index 3387ebc9d8ab..2b6834c4cac6 100644 > --- a/drivers/hwtracing/coresight/coresight-stm.c > +++ b/drivers/hwtracing/coresight/coresight-stm.c > @@ -906,7 +906,7 @@ static int __stm_probe(struct device *dev, struct resource *res, void *dev_caps) > pm_runtime_put(dev); > > dev_info(&drvdata->csdev->dev, "%s initialized\n", > - (char *)dev_caps); > + dev_caps ? (char *)dev_caps: "STM"); > return 0; > Please could stop abusing the private data for storing the name ? Instead, we could read the PID of the device and use a table to map it to the name and use that instead ? Suzuki > cs_unregister: >
diff --git a/drivers/acpi/arm64/amba.c b/drivers/acpi/arm64/amba.c index d3c1defa7bc8..bec0976541da 100644 --- a/drivers/acpi/arm64/amba.c +++ b/drivers/acpi/arm64/amba.c @@ -22,7 +22,6 @@ static const struct acpi_device_id amba_id_list[] = { {"ARMH0061", 0}, /* PL061 GPIO Device */ {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */ - {"ARMHC502", 0}, /* ARM CoreSight STM */ {"ARMHC503", 0}, /* ARM CoreSight Debug */ {"", 0}, }; diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c index a1c27c901ad1..564aa5f1eb8a 100644 --- a/drivers/hwtracing/coresight/coresight-stm.c +++ b/drivers/hwtracing/coresight/coresight-stm.c @@ -29,6 +29,8 @@ #include <linux/perf_event.h> #include <linux/pm_runtime.h> #include <linux/stm.h> +#include <linux/platform_device.h> +#include <linux/acpi.h> #include "coresight-priv.h" #include "coresight-trace-id.h" @@ -132,6 +134,7 @@ DEFINE_CORESIGHT_DEVLIST(stm_devs, "stm"); struct stm_drvdata { void __iomem *base; struct clk *atclk; + struct clk *pclk; struct coresight_device *csdev; spinlock_t spinlock; struct channel_space chs; @@ -804,14 +807,12 @@ static void stm_init_generic_data(struct stm_drvdata *drvdata, drvdata->stm.set_options = stm_generic_set_options; } -static int stm_probe(struct amba_device *adev, const struct amba_id *id) +static int __stm_probe(struct device *dev, struct resource *res, void *dev_caps) { int ret, trace_id; void __iomem *base; - struct device *dev = &adev->dev; struct coresight_platform_data *pdata = NULL; struct stm_drvdata *drvdata; - struct resource *res = &adev->res; struct resource ch_res; struct coresight_desc desc = { 0 }; @@ -823,12 +824,16 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id) if (!drvdata) return -ENOMEM; - drvdata->atclk = devm_clk_get(&adev->dev, "atclk"); /* optional */ + drvdata->atclk = devm_clk_get(dev, "atclk"); /* optional */ if (!IS_ERR(drvdata->atclk)) { ret = clk_prepare_enable(drvdata->atclk); if (ret) return ret; } + + drvdata->pclk = coresight_get_enable_apb_pclk(dev); + if (IS_ERR(drvdata->pclk)) + return -ENODEV; dev_set_drvdata(dev, drvdata); base = devm_ioremap_resource(dev, res); @@ -876,7 +881,7 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id) ret = PTR_ERR(pdata); goto stm_unregister; } - adev->dev.platform_data = pdata; + dev->platform_data = pdata; desc.type = CORESIGHT_DEV_TYPE_SOURCE; desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE; @@ -897,10 +902,10 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id) } drvdata->traceid = (u8)trace_id; - pm_runtime_put(&adev->dev); + pm_runtime_put(dev); dev_info(&drvdata->csdev->dev, "%s initialized\n", - (char *)coresight_get_uci_data(id)); + (char *)dev_caps); return 0; cs_unregister: @@ -911,9 +916,14 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id) return ret; } -static void stm_remove(struct amba_device *adev) +static int stm_probe(struct amba_device *adev, const struct amba_id *id) { - struct stm_drvdata *drvdata = dev_get_drvdata(&adev->dev); + return __stm_probe(&adev->dev, &adev->res, coresight_get_uci_data(id)); +} + +static void __stm_remove(struct device *dev) +{ + struct stm_drvdata *drvdata = dev_get_drvdata(dev); coresight_trace_id_put_system_id(drvdata->traceid); coresight_unregister(drvdata->csdev); @@ -921,6 +931,11 @@ static void stm_remove(struct amba_device *adev) stm_unregister_device(&drvdata->stm); } +static void stm_remove(struct amba_device *adev) +{ + __stm_remove(&adev->dev); +} + #ifdef CONFIG_PM static int stm_runtime_suspend(struct device *dev) { @@ -929,6 +944,8 @@ static int stm_runtime_suspend(struct device *dev) if (drvdata && !IS_ERR(drvdata->atclk)) clk_disable_unprepare(drvdata->atclk); + if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) + clk_disable_unprepare(drvdata->pclk); return 0; } @@ -939,6 +956,8 @@ static int stm_runtime_resume(struct device *dev) if (drvdata && !IS_ERR(drvdata->atclk)) clk_prepare_enable(drvdata->atclk); + if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) + clk_prepare_enable(drvdata->pclk); return 0; } #endif @@ -967,7 +986,59 @@ static struct amba_driver stm_driver = { .id_table = stm_ids, }; -module_amba_driver(stm_driver); +static int stm_platform_probe(struct platform_device *pdev) +{ + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + int ret = 0; + + pm_runtime_get_noresume(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + + ret = __stm_probe(&pdev->dev, res, NULL); + if (ret) { + pm_runtime_put_noidle(&pdev->dev); + pm_runtime_disable(&pdev->dev); + } + return ret; +} + +static int stm_platform_remove(struct platform_device *pdev) +{ + __stm_remove(&pdev->dev); + return 0; +} + +#ifdef CONFIG_ACPI +static const struct acpi_device_id stm_acpi_ids[] = { + {"ARMHC502", 0}, /* ARM CoreSight STM */ + {}, +}; +MODULE_DEVICE_TABLE(acpi, stm_acpi_ids); +#endif + +static struct platform_driver stm_platform_driver = { + .probe = stm_platform_probe, + .remove = stm_platform_remove, + .driver = { + .name = "coresight-stm-platform", + .acpi_match_table = ACPI_PTR(stm_acpi_ids), + .suppress_bind_attrs = true, + .pm = &stm_dev_pm_ops, + }, +}; + +static int __init stm_init(void) +{ + return coresight_init_driver("stm", &stm_driver, &stm_platform_driver); +} + +static void __exit stm_exit(void) +{ + coresight_remove_driver(&stm_driver, &stm_platform_driver); +} +module_init(stm_init); +module_exit(stm_exit); MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>"); MODULE_DESCRIPTION("Arm CoreSight System Trace Macrocell driver");
Add support for the stm devices in the platform driver, which can then be used on ACPI based platforms. This change would now allow runtime power management for ACPI based systems. The driver would try to enable the APB clock if available. Cc: Lorenzo Pieralisi <lpieralisi@kernel.org> Cc: Sudeep Holla <sudeep.holla@arm.com> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Mike Leach <mike.leach@linaro.org> Cc: James Clark <james.clark@arm.com> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> Cc: linux-acpi@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Cc: coresight@lists.linaro.org Cc: linux-stm32@st-md-mailman.stormreply.com Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- drivers/acpi/arm64/amba.c | 1 - drivers/hwtracing/coresight/coresight-stm.c | 91 ++++++++++++++++++--- 2 files changed, 81 insertions(+), 11 deletions(-)