| Message ID | 20230317030501.1811905-1-anshuman.khandual@arm.com |
|---|---|
| Headers | show |
| Series | coresight: etm4x: Migrate AMBA devices to platform driver | expand |
On 17/03/2023 03:04, Anshuman Khandual wrote: > From: Suzuki Poulose <suzuki.poulose@arm.com> > > Drop ETM4X ACPI ID from the AMBA ACPI device list, and instead just move it > inside the new ACPI devices list detected and used via platform driver. It may be a good idea to add a short summary of why we are doing this ? Though it is part of the cover letter, it would benefit people looking at the commit (e.g., maintainers or even in the future). Suzuki > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Len Brown <lenb@kernel.org> > 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: Sudeep Holla <sudeep.holla@arm.com> > Cc: Lorenzo Pieralisi <lpieralisi@kernel.org> > Cc: linux-acpi@vger.kernel.org > Cc: coresight@lists.linaro.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Suzuki Poulose <suzuki.poulose@arm.com> > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > drivers/acpi/acpi_amba.c | 1 - > drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 ++++++++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/acpi_amba.c b/drivers/acpi/acpi_amba.c > index f5b443ab01c2..099966cbac5a 100644 > --- a/drivers/acpi/acpi_amba.c > +++ b/drivers/acpi/acpi_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 */ > - {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ > {"ARMHC501", 0}, /* ARM CoreSight ETR */ > {"ARMHC502", 0}, /* ARM CoreSight STM */ > {"ARMHC503", 0}, /* ARM CoreSight Debug */ > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index 60f027e33aa0..fe494c9c6bad 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -3,6 +3,7 @@ > * Copyright (c) 2014, The Linux Foundation. All rights reserved. > */ > > +#include <linux/acpi.h> > #include <linux/bitops.h> > #include <linux/kernel.h> > #include <linux/moduleparam.h> > @@ -2344,12 +2345,21 @@ static const struct of_device_id etm4_match[] = { > {} > }; > > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id etm4x_acpi_ids[] = { > + {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ > + {} > +}; > +MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids); > +#endif > + > static struct platform_driver etm4_platform_driver = { > .probe = etm4_probe_platform_dev, > .remove = etm4_remove_platform_dev, > .driver = { > .name = "coresight-etm4x", > .of_match_table = etm4_match, > + .acpi_match_table = ACPI_PTR(etm4x_acpi_ids), > .suppress_bind_attrs = true, > }, > };
On Thu, Mar 16, 2023 at 10:05 PM Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > CoreSight ETM4x devices could be accessed either via MMIO (handled via > amba_driver) or CPU system instructions (handled via platform driver). But > this has the following issues : > > - Each new CPU comes up with its own PID and thus we need to keep on > adding the "known" PIDs to get it working with AMBA driver. While > the ETM4 architecture (and CoreSight architecture) defines way to > identify a device as ETM4. Thus older kernels won't be able to > "discover" a newer CPU, unless we add the PIDs. But v8.4 discourages MMIO access, so this problem will go away on its own. Even if not, adding IDs to stable kernels is standard practice whether it is PCI VID/PID, compatible string or AMBA PID. > - With ACPI, the ETM4x devices have the same HID to identify the device > irrespective of the mode of access. This creates a problem where two > different drivers (both AMBA based driver and platform driver) would > hook into the "HID" and could conflict. e.g., if AMBA driver gets > hold of a non-MMIO device, the probe fails. If we have single driver > hooked into the given "HID", we could handle them seamlessly, > irrespective of the mode of access. Why are we changing DT for ACPI? Just always use the platform driver for ACPI and leave DT systems alone. > - CoreSight is heavily dependent on the runtime power management. With > ACPI, amba_driver doesn't get us anywhere with handling the power > and thus one need to always turn the power ON to use them. Moving to > platform driver gives us the power management for free. This sounds like an issue for any amba driver. If this is an issue, solve it for everyone, not just work around it in one driver. When someone puts another primecell device into an ACPI system, are we going to go do the same one-off change in that driver too? (We kind of already did with SBSA UART...) Rob
On 20/03/2023 14:17, Rob Herring wrote: > On Thu, Mar 16, 2023 at 10:05 PM Anshuman Khandual > <anshuman.khandual@arm.com> wrote: >> >> CoreSight ETM4x devices could be accessed either via MMIO (handled via >> amba_driver) or CPU system instructions (handled via platform driver). But >> this has the following issues : >> >> - Each new CPU comes up with its own PID and thus we need to keep on >> adding the "known" PIDs to get it working with AMBA driver. While >> the ETM4 architecture (and CoreSight architecture) defines way to >> identify a device as ETM4. Thus older kernels won't be able to >> "discover" a newer CPU, unless we add the PIDs. > > But v8.4 discourages MMIO access, so this problem will go away on its > own. Even if not, adding IDs to stable kernels is standard practice > whether it is PCI VID/PID, compatible string or AMBA PID. Yes, it would eventually go away. As for adding the PIDs, the fundamental issue is, unlike other drivers, except for the "PIDs" everything else is architected and each CPU has this PID alone different and we have plenty of CPUs implementaions out there. But all that said, since we added this as an AMBA driver in the first place (all for simply getting the apb_clk management), I am happy to choose the "Add PIDs to stable kernel approach" for this problem. > >> - With ACPI, the ETM4x devices have the same HID to identify the device >> irrespective of the mode of access. This creates a problem where two >> different drivers (both AMBA based driver and platform driver) would >> hook into the "HID" and could conflict. e.g., if AMBA driver gets >> hold of a non-MMIO device, the probe fails. If we have single driver >> hooked into the given "HID", we could handle them seamlessly, >> irrespective of the mode of access. > > Why are we changing DT for ACPI? Just always use the platform driver > for ACPI and leave DT systems alone. This was mainly due to (1), given we have a platform driver anyway for ACPI. As mentioned above, we could leave the DT alone. > >> - CoreSight is heavily dependent on the runtime power management. With >> ACPI, amba_driver doesn't get us anywhere with handling the power >> and thus one need to always turn the power ON to use them. Moving to >> platform driver gives us the power management for free. > > This sounds like an issue for any amba driver. If this is an issue, > solve it for everyone, not just work around it in one driver. This alone wouldn't be sufficient. We need a platform driver anyway to handle the two different modes in ACPI for ETMs. But this will be a an option for the other CoreSight components which are always MMIO. Thanks Suzuki > > When someone puts another primecell device into an ACPI system, are we > going to go do the same one-off change in that driver too? (We kind of > already did with SBSA UART...) > > Rob
On Mon, Mar 20, 2023 at 09:17:16AM -0500, Rob Herring wrote: > > This sounds like an issue for any amba driver. If this is an issue, > solve it for everyone, not just work around it in one driver. > Well it is an issue in general for power management. ACPI has specific methods that can be executed for entering specific states. The way AMBA was glue into ACPI bus scan IMO was a hack and PM wasn't considered at the time. It was just hack to get AMBA drivers to work with ACPI without any consideration about runtime PM or any methods that comes as part of ACPI device. There is even some dummy clock handler to deal with AMBA requesting APB clocks. AMBA device is added as companion to the ACPI device created as part of the normal bus scan in ACPI which adds its own PM callbacks and rely on clocks and power domains independent of the ACPI standard methods(_ON/_OFF). The default enumeration adds platform devices which adds no extra PM callbacks and allows normal acpi_device probe flow. > When someone puts another primecell device into an ACPI system, are we > going to go do the same one-off change in that driver too? (We kind of > already did with SBSA UART...) > I would prefer to move all the existing users of ACPI + AMBA to move away from it and just use platform device. This list is not big today, bunch of coresight, PL061/GPIO and PL330/DMA. And all these are assumed to be working or actually working if there is no need for any power management. E.g. on juno coresight needs PM to turn on before probing and AMBA fails as dummy clocks are added but no power domains attached as ACPI doesn't need deal with power domains in the OSPM if it is all well abstracted in methods like _ON/_OFF. They are dealt with explicit power domain in the DT which needs to be turned on and AMBA relies on that. One possible further hacky solution is to add dummy genpd to satisfy AMBA but not sure if we can guarantee ordering between ACPI device calling ON and its companion AMBA device probing so that the power domain is ON before AMBA uses the dummy clock and power domains in its pm callback hooks. Even the UART would fail if it needed any PM methods, we just don't happen to need that for SBSA and may be we could have made it work as amba device (can't recollect the exact reason for not doing so now). -- Regards, Sudeep
On Tue, Mar 21, 2023 at 9:34 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Mon, Mar 20, 2023 at 09:17:16AM -0500, Rob Herring wrote: > > > > This sounds like an issue for any amba driver. If this is an issue, > > solve it for everyone, not just work around it in one driver. > > > > Well it is an issue in general for power management. ACPI has specific > methods that can be executed for entering specific states. > > The way AMBA was glue into ACPI bus scan IMO was a hack and PM wasn't > considered at the time. It was just hack to get AMBA drivers to work > with ACPI without any consideration about runtime PM or any methods that > comes as part of ACPI device. There is even some dummy clock handler to > deal with AMBA requesting APB clocks. AMBA device is added as companion > to the ACPI device created as part of the normal bus scan in ACPI which > adds its own PM callbacks and rely on clocks and power domains independent > of the ACPI standard methods(_ON/_OFF). I thought only DT had hacks... ;) > The default enumeration adds platform devices which adds no extra PM > callbacks and allows normal acpi_device probe flow. > > > When someone puts another primecell device into an ACPI system, are we > > going to go do the same one-off change in that driver too? (We kind of > > already did with SBSA UART...) > > > > I would prefer to move all the existing users of ACPI + AMBA to move away > from it and just use platform device. This list is not big today, bunch > of coresight, PL061/GPIO and PL330/DMA. And all these are assumed to be > working or actually working if there is no need for any power management. > E.g. on juno coresight needs PM to turn on before probing and AMBA fails > as dummy clocks are added but no power domains attached as ACPI doesn't > need deal with power domains in the OSPM if it is all well abstracted in > methods like _ON/_OFF. They are dealt with explicit power domain in the > DT which needs to be turned on and AMBA relies on that. > > One possible further hacky solution is to add dummy genpd to satisfy AMBA > but not sure if we can guarantee ordering between ACPI device calling ON > and its companion AMBA device probing so that the power domain is ON before > AMBA uses the dummy clock and power domains in its pm callback hooks. What if we made AMBA skip its usual matching by ID and only use DT/ACPI style matching? We have specific compatibles, but they have never been used by the kernel. The only reason the bus code needs to do PM is reading the IDs which could be pushed into the drivers that need to match on specific IDs (I suspect we have some where the compatible is not specific enough (old ST stuff)). Looks like we only have 2 platforms left not using DT: arch/arm/mach-ep93xx/core.c: amba_device_register(&uart1_device, &iomem_resource); arch/arm/mach-ep93xx/core.c: amba_device_register(&uart2_device, &iomem_resource); arch/arm/mach-ep93xx/core.c: amba_device_register(&uart3_device, &iomem_resource); arch/arm/mach-s3c/pl080.c: amba_device_register(&s3c64xx_dma0_device, &iomem_resource); arch/arm/mach-s3c/pl080.c: amba_device_register(&s3c64xx_dma1_device, &iomem_resource); Get rid of these cases and we don't have to worry about non-DT or ACPI matching. > Even the UART would fail if it needed any PM methods, we just don't happen > to need that for SBSA and may be we could have made it work as amba device > (can't recollect the exact reason for not doing so now). SBSA doesn't require ID registers. SBSA UART is a "great" example of none of the existing 2 standards work, so let's create a 3rd. Rob
CoreSight ETM4x devices could be accessed either via MMIO (handled via amba_driver) or CPU system instructions (handled via platform driver). But this has the following issues : - Each new CPU comes up with its own PID and thus we need to keep on adding the "known" PIDs to get it working with AMBA driver. While the ETM4 architecture (and CoreSight architecture) defines way to identify a device as ETM4. Thus older kernels won't be able to "discover" a newer CPU, unless we add the PIDs. - With ACPI, the ETM4x devices have the same HID to identify the device irrespective of the mode of access. This creates a problem where two different drivers (both AMBA based driver and platform driver) would hook into the "HID" and could conflict. e.g., if AMBA driver gets hold of a non-MMIO device, the probe fails. If we have single driver hooked into the given "HID", we could handle them seamlessly, irrespective of the mode of access. - CoreSight is heavily dependent on the runtime power management. With ACPI, amba_driver doesn't get us anywhere with handling the power and thus one need to always turn the power ON to use them. Moving to platform driver gives us the power management for free. Due to all of the above, we are moving the MMIO based etm4x devices to be supported via platform driver. The series makes the existing platform driver generic to handle both type of the access modes. With that we can also remove the etm4x amba driver. Finally, we need a way to make sure the new driver gets control of the ETM4x device on a DT based system. CoreSight devices have always had the "arm,primecell" in the compatible list. But the way this is handled currently in OF code is a bit messy. The ETM4x devices are identified by "arm,coresight-etm4x". The platform driver can never get a chance to probe these devices, since the "arm,primecell" takes priority and is hard-coded in the OF code. We have two options here : 1) Remove the arm,primecell from all DTS. This is fine for "new" kernels with this change. But, for existing boards, using an older kernel will break. Thus, is not preferred. 2) Add a white list of "compatibles" where the "priority" of the "arm,primecell" can be ignored. The series implements (2) above and applies on 6.3-rc2. Cc: Steve Clevenger <scclevenger@os.amperecomputing.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Frank Rowand <frowand.list@gmail.com> Cc: Russell King (Oracle) <linux@armlinux.org.uk> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Len Brown <lenb@kernel.org> Cc: Sudeep Holla <sudeep.holla@arm.com> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org> 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: devicetree@vger.kernel.org Cc: linux-acpi@vger.kernel.org Cc: coresight@lists.linaro.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Anshuman Khandual (6): coresight: etm4x: Allocate and device assign 'struct etmv4_drvdata' earlier coresight: etm4x: Drop iomem 'base' argument from etm4_probe() coresight: etm4x: Drop pid argument from etm4_probe() coresight: etm4x: Change etm4_platform_driver driver for MMIO devices of/platform: Skip coresight etm4x devices from AMBA bus coresight: etm4x: Drop the AMBA driver Suzuki Poulose (1): coresight: etm4x: Add ACPI support in platform driver drivers/acpi/acpi_amba.c | 1 - .../coresight/coresight-etm4x-core.c | 171 ++++++++---------- drivers/hwtracing/coresight/coresight-etm4x.h | 3 + drivers/of/platform.c | 10 +- include/linux/coresight.h | 56 ++++++ 5 files changed, 143 insertions(+), 98 deletions(-)