[6/7] of/platform: Skip coresight etm4x devices from AMBA bus

  Allow other drivers to claim a device, disregarding the "priority" of
"arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
(AMBA Bus) or via CPU system instructions. The CoreSight ETM4x platform
driver can now handle both types of devices. In order to make sure the
driver gets to handle the "MMIO based" devices, which always had the
"arm,primecell" compatible, we have two options :

1) Remove the "arm,primecell" from the DTS. But this may be problematic
 for an older kernel without the support.

2) The other option is to allow OF code to "ignore" the arm,primecell
priority for a selected list of compatibles. This would make sure that
both older kernels and the new kernels work fine without breaking
the functionality. The new DTS could always have the "arm,primecell"
removed.

This patch implements Option (2).

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: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Co-developed-by: Suzuki Poulose <suzuki.poulose@arm.com>
Signed-off-by: Suzuki Poulose <suzuki.poulose@arm.com>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 drivers/of/platform.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
  

On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
> Allow other drivers to claim a device, disregarding the "priority" of
> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
> (AMBA Bus) or via CPU system instructions.

The OS can pick which one, use both, or this is a system integration
time decision?

> The CoreSight ETM4x platform
> driver can now handle both types of devices. In order to make sure the
> driver gets to handle the "MMIO based" devices, which always had the
> "arm,primecell" compatible, we have two options :
>
> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
>  for an older kernel without the support.
>
> 2) The other option is to allow OF code to "ignore" the arm,primecell
> priority for a selected list of compatibles. This would make sure that
> both older kernels and the new kernels work fine without breaking
> the functionality. The new DTS could always have the "arm,primecell"
> removed.

3) Drop patches 6 and 7 and just register as both AMBA and platform
drivers. It's just some extra boilerplate. I would also do different
compatible strings for CPU system instruction version (assuming this
is an integration time decision).

>
> This patch implements Option (2).
>
> 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: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Co-developed-by: Suzuki Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Suzuki Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  drivers/of/platform.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b2bd2e783445..59ff1a38ccaa 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -325,6 +325,13 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
>         return NULL;
>  }
>
> +static const struct of_device_id of_ignore_amba_table[] = {
> +#ifdef CONFIG_CORESIGHT_SOURCE_ETM4X
> +       { .compatible = "arm,coresight-etm4x" },
> +#endif
> +       {}    /* NULL terminated */
> +};
> +
>  /**
>   * of_platform_bus_create() - Create a device for a node and its children.
>   * @bus: device node of the bus to instantiate
> @@ -373,7 +380,8 @@ static int of_platform_bus_create(struct device_node *bus,
>                 platform_data = auxdata->platform_data;
>         }
>
> -       if (of_device_is_compatible(bus, "arm,primecell")) {
> +       if (of_device_is_compatible(bus, "arm,primecell") &&
> +           unlikely(!of_match_node(of_ignore_amba_table, bus))) {

of_match_node is going to take orders of magnitude longer than any
difference unlikely() would make. Drop it.

>                 /*
>                  * Don't return an error here to keep compatibility with older
>                  * device tree files.
> --
> 2.25.1
>
  

Hi Rob

Thanks for your response.

On 17/03/2023 14:52, Rob Herring wrote:
> On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
> <anshuman.khandual@arm.com> wrote:
>>
>> Allow other drivers to claim a device, disregarding the "priority" of
>> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
>> (AMBA Bus) or via CPU system instructions.
> 
> The OS can pick which one, use both, or this is a system integration
> time decision?

Not an OS choice. Historically, this has always been MMIO accessed but
with v8.4 TraceFiltering support, CPUs are encouraged to use system
instructions and obsolete MMIO. So, yes, MMIO is still possible but
something that is discouraged and have to be decided at system
integration time.

> 
>> The CoreSight ETM4x platform
>> driver can now handle both types of devices. In order to make sure the
>> driver gets to handle the "MMIO based" devices, which always had the
>> "arm,primecell" compatible, we have two options :
>>
>> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
>>   for an older kernel without the support.
>>
>> 2) The other option is to allow OF code to "ignore" the arm,primecell
>> priority for a selected list of compatibles. This would make sure that
>> both older kernels and the new kernels work fine without breaking
>> the functionality. The new DTS could always have the "arm,primecell"
>> removed.
> 
> 3) Drop patches 6 and 7 and just register as both AMBA and platform
> drivers. It's just some extra boilerplate. I would also do different
> compatible strings for CPU system instruction version (assuming this
> is an integration time decision).

The system instruction (and the reigster layouts) are all part of the
ETMv4/ETE architecture and specific capabilities/features are
discoverable, just like the Arm CPUs. Thus we don't need special
versions within the ETMv4x or ETE minor versions. As of now, we have
one for etm4x and another for ete.

One problem with the AMBA driver in place is having to keep on adding
new PIDs for the CPUs. The other option is to have a blanket mask
for matching the PIDs with AMBA_UCI_ID checks.


> 
>>
>> This patch implements Option (2).
>>
>> 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: devicetree@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Co-developed-by: Suzuki Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Suzuki Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>   drivers/of/platform.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index b2bd2e783445..59ff1a38ccaa 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -325,6 +325,13 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
>>          return NULL;
>>   }
>>
>> +static const struct of_device_id of_ignore_amba_table[] = {
>> +#ifdef CONFIG_CORESIGHT_SOURCE_ETM4X
>> +       { .compatible = "arm,coresight-etm4x" },
>> +#endif
>> +       {}    /* NULL terminated */
>> +};
>> +
>>   /**
>>    * of_platform_bus_create() - Create a device for a node and its children.
>>    * @bus: device node of the bus to instantiate
>> @@ -373,7 +380,8 @@ static int of_platform_bus_create(struct device_node *bus,
>>                  platform_data = auxdata->platform_data;
>>          }
>>
>> -       if (of_device_is_compatible(bus, "arm,primecell")) {
>> +       if (of_device_is_compatible(bus, "arm,primecell") &&
>> +           unlikely(!of_match_node(of_ignore_amba_table, bus))) {
> 
> of_match_node is going to take orders of magnitude longer than any
> difference unlikely() would make. Drop it.

Agreed.

Suzuki

> 
>>                  /*
>>                   * Don't return an error here to keep compatibility with older
>>                   * device tree files.
>> --
>> 2.25.1
>>
  

On Fri, Mar 17, 2023 at 11:03 AM Suzuki K Poulose
<suzuki.poulose@arm.com> wrote:
>
> Hi Rob
>
> Thanks for your response.
>
> On 17/03/2023 14:52, Rob Herring wrote:
> > On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
> > <anshuman.khandual@arm.com> wrote:
> >>
> >> Allow other drivers to claim a device, disregarding the "priority" of
> >> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
> >> (AMBA Bus) or via CPU system instructions.
> >
> > The OS can pick which one, use both, or this is a system integration
> > time decision?
>
> Not an OS choice. Historically, this has always been MMIO accessed but
> with v8.4 TraceFiltering support, CPUs are encouraged to use system
> instructions and obsolete MMIO. So, yes, MMIO is still possible but
> something that is discouraged and have to be decided at system
> integration time.
>
> >
> >> The CoreSight ETM4x platform
> >> driver can now handle both types of devices. In order to make sure the
> >> driver gets to handle the "MMIO based" devices, which always had the
> >> "arm,primecell" compatible, we have two options :
> >>
> >> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
> >>   for an older kernel without the support.
> >>
> >> 2) The other option is to allow OF code to "ignore" the arm,primecell
> >> priority for a selected list of compatibles. This would make sure that
> >> both older kernels and the new kernels work fine without breaking
> >> the functionality. The new DTS could always have the "arm,primecell"
> >> removed.
> >
> > 3) Drop patches 6 and 7 and just register as both AMBA and platform
> > drivers. It's just some extra boilerplate. I would also do different
> > compatible strings for CPU system instruction version (assuming this
> > is an integration time decision).
>
> The system instruction (and the reigster layouts) are all part of the
> ETMv4/ETE architecture and specific capabilities/features are
> discoverable, just like the Arm CPUs. Thus we don't need special
> versions within the ETMv4x or ETE minor versions. As of now, we have
> one for etm4x and another for ete.

I just meant 2 new compatible strings. One each for ETMv4x and ETE,
but different from the 2 existing ones. It is different h/w presented
to the OS, so different compatible.

> One problem with the AMBA driver in place is having to keep on adding
> new PIDs for the CPUs. The other option is to have a blanket mask
> for matching the PIDs with AMBA_UCI_ID checks.

But if MMIO access is discouraged, then new h/w would use the platform
driver(s), not the amba driver, and you won't have to add PIDs.

Rob
  

On 3/17/23 21:33, Suzuki K Poulose wrote:
>>>   drivers/of/platform.c | 10 +++++++++-
>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index b2bd2e783445..59ff1a38ccaa 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -325,6 +325,13 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
>>>          return NULL;
>>>   }
>>>
>>> +static const struct of_device_id of_ignore_amba_table[] = {
>>> +#ifdef CONFIG_CORESIGHT_SOURCE_ETM4X
>>> +       { .compatible = "arm,coresight-etm4x" },
>>> +#endif
>>> +       {}    /* NULL terminated */
>>> +};
>>> +
>>>   /**
>>>    * of_platform_bus_create() - Create a device for a node and its children.
>>>    * @bus: device node of the bus to instantiate
>>> @@ -373,7 +380,8 @@ static int of_platform_bus_create(struct device_node *bus,
>>>                  platform_data = auxdata->platform_data;
>>>          }
>>>
>>> -       if (of_device_is_compatible(bus, "arm,primecell")) {
>>> +       if (of_device_is_compatible(bus, "arm,primecell") &&
>>> +           unlikely(!of_match_node(of_ignore_amba_table, bus))) {
>>
>> of_match_node is going to take orders of magnitude longer than any
>> difference unlikely() would make. Drop it.
> 
> Agreed.

Sure, will drop the unlikely() here.
  

On 17/03/2023 20:06, Rob Herring wrote:
> On Fri, Mar 17, 2023 at 11:03 AM Suzuki K Poulose
> <suzuki.poulose@arm.com> wrote:
>>
>> Hi Rob
>>
>> Thanks for your response.
>>
>> On 17/03/2023 14:52, Rob Herring wrote:
>>> On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
>>> <anshuman.khandual@arm.com> wrote:
>>>>
>>>> Allow other drivers to claim a device, disregarding the "priority" of
>>>> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
>>>> (AMBA Bus) or via CPU system instructions.
>>>
>>> The OS can pick which one, use both, or this is a system integration
>>> time decision?
>>
>> Not an OS choice. Historically, this has always been MMIO accessed but
>> with v8.4 TraceFiltering support, CPUs are encouraged to use system
>> instructions and obsolete MMIO. So, yes, MMIO is still possible but
>> something that is discouraged and have to be decided at system
>> integration time.
>>
>>>
>>>> The CoreSight ETM4x platform
>>>> driver can now handle both types of devices. In order to make sure the
>>>> driver gets to handle the "MMIO based" devices, which always had the
>>>> "arm,primecell" compatible, we have two options :
>>>>
>>>> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
>>>>    for an older kernel without the support.
>>>>
>>>> 2) The other option is to allow OF code to "ignore" the arm,primecell
>>>> priority for a selected list of compatibles. This would make sure that
>>>> both older kernels and the new kernels work fine without breaking
>>>> the functionality. The new DTS could always have the "arm,primecell"
>>>> removed.
>>>
>>> 3) Drop patches 6 and 7 and just register as both AMBA and platform
>>> drivers. It's just some extra boilerplate. I would also do different
>>> compatible strings for CPU system instruction version (assuming this
>>> is an integration time decision).
>>
>> The system instruction (and the reigster layouts) are all part of the
>> ETMv4/ETE architecture and specific capabilities/features are
>> discoverable, just like the Arm CPUs. Thus we don't need special
>> versions within the ETMv4x or ETE minor versions. As of now, we have
>> one for etm4x and another for ete.
> 
> I just meant 2 new compatible strings. One each for ETMv4x and ETE,
> but different from the 2 existing ones. It is different h/w presented
> to the OS, so different compatible.
> 

Sorry, was not very clear here.

Right now, we have :

1) arm,coresight-etm4x && arm,primecell - For AMBA based devices
2) arm,coresight-etm4x-sysreg	- For system instruction based
3) arm,embedded-trace-extension - For ETE


>> One problem with the AMBA driver in place is having to keep on adding
>> new PIDs for the CPUs. The other option is to have a blanket mask
>> for matching the PIDs with AMBA_UCI_ID checks.
> 
> But if MMIO access is discouraged, then new h/w would use the platform
> driver(s), not the amba driver, and you won't have to add PIDs.

Yes for v8.4 onwards. Alternatively, the newer DTS could skip 
arm,primecell in the entry and that would kick the platform driver
in. So, that should be fine I guess.

Kind regards
Suzuki

> 
> Rob
  

On Mon, Mar 20, 2023 at 5:37 AM Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
>
> On 17/03/2023 20:06, Rob Herring wrote:
> > On Fri, Mar 17, 2023 at 11:03 AM Suzuki K Poulose
> > <suzuki.poulose@arm.com> wrote:
> >>
> >> Hi Rob
> >>
> >> Thanks for your response.
> >>
> >> On 17/03/2023 14:52, Rob Herring wrote:
> >>> On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
> >>> <anshuman.khandual@arm.com> wrote:
> >>>>
> >>>> Allow other drivers to claim a device, disregarding the "priority" of
> >>>> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
> >>>> (AMBA Bus) or via CPU system instructions.
> >>>
> >>> The OS can pick which one, use both, or this is a system integration
> >>> time decision?
> >>
> >> Not an OS choice. Historically, this has always been MMIO accessed but
> >> with v8.4 TraceFiltering support, CPUs are encouraged to use system
> >> instructions and obsolete MMIO. So, yes, MMIO is still possible but
> >> something that is discouraged and have to be decided at system
> >> integration time.
> >>
> >>>
> >>>> The CoreSight ETM4x platform
> >>>> driver can now handle both types of devices. In order to make sure the
> >>>> driver gets to handle the "MMIO based" devices, which always had the
> >>>> "arm,primecell" compatible, we have two options :
> >>>>
> >>>> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
> >>>>    for an older kernel without the support.
> >>>>
> >>>> 2) The other option is to allow OF code to "ignore" the arm,primecell
> >>>> priority for a selected list of compatibles. This would make sure that
> >>>> both older kernels and the new kernels work fine without breaking
> >>>> the functionality. The new DTS could always have the "arm,primecell"
> >>>> removed.
> >>>
> >>> 3) Drop patches 6 and 7 and just register as both AMBA and platform
> >>> drivers. It's just some extra boilerplate. I would also do different
> >>> compatible strings for CPU system instruction version (assuming this
> >>> is an integration time decision).
> >>
> >> The system instruction (and the reigster layouts) are all part of the
> >> ETMv4/ETE architecture and specific capabilities/features are
> >> discoverable, just like the Arm CPUs. Thus we don't need special
> >> versions within the ETMv4x or ETE minor versions. As of now, we have
> >> one for etm4x and another for ete.
> >
> > I just meant 2 new compatible strings. One each for ETMv4x and ETE,
> > but different from the 2 existing ones. It is different h/w presented
> > to the OS, so different compatible.
> >
>
> Sorry, was not very clear here.
>
> Right now, we have :
>
> 1) arm,coresight-etm4x && arm,primecell - For AMBA based devices
> 2) arm,coresight-etm4x-sysreg   - For system instruction based
> 3) arm,embedded-trace-extension - For ETE

Ah, so we already have that...

>
>
> >> One problem with the AMBA driver in place is having to keep on adding
> >> new PIDs for the CPUs. The other option is to have a blanket mask
> >> for matching the PIDs with AMBA_UCI_ID checks.
> >
> > But if MMIO access is discouraged, then new h/w would use the platform
> > driver(s), not the amba driver, and you won't have to add PIDs.
>
> Yes for v8.4 onwards. Alternatively, the newer DTS could skip
> arm,primecell in the entry and that would kick the platform driver
> in. So, that should be fine I guess.

Except that the new dts would not work with older kernels.

I'm now lost as to what problem this series is trying to solve. Will
reply further on cover letter...

Rob