Re: [RESEND PATCH v3 1/2] drivers: amba: Updates to component identification for driver matching.

From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: mike.leach@linaro.org, saiprakash.ranjan@codeaurora.org
Cc: rnayak@codeaurora.org, mathieu.poirier@linaro.org,
	coresight@lists.linaro.org, linux@armlinux.org.uk,
	linux-kernel@vger.kernel.org, sibis@codeaurora.org,
	vivek.gautam@codeaurora.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [RESEND PATCH v3 1/2] drivers: amba: Updates to component identification for driver matching.
Date: Fri, 25 Jan 2019 10:42:23 +0000	[thread overview]
Message-ID: <f4d78f6f-1b85-22ca-bfa6-abaf0d67a898@arm.com> (raw)
In-Reply-To: <CAJ9a7Vh6McpjtyRuBDPSYMWec0TuWkXeOLBOpKvv8tWUU0sV2A@mail.gmail.com>

Mike,

On 25/01/2019 10:37, Mike Leach wrote:
> Hello Sai
> On Fri, 25 Jan 2019 at 07:20, Sai Prakash Ranjan
> <saiprakash.ranjan@codeaurora.org> wrote:
>>
>> Hi Mike,
>>
>> Thanks for the patch.
>>
>> BTW somehow I can't find the latest series in my inbox, so commenting
>> on this here.
>>
>> Mathieu pointed me to this patch series.This solves CPU debug module
>> sharing same PID as ETM on MSM8996. I will be posting patch for CPU
>> debug UCI table soon.
>>
>> But please find my one comment inline.
>>
>> On 12/19/2018 3:29 AM, Mike Leach wrote:
>>> The CoreSight specification (ARM IHI 0029E), updates the ID register
>>> requirements for components on an AMBA bus, to cover both traditional
>>> ARM Primecell type devices, and newer CoreSight and other components.
>>>
>>> The Peripheral ID (PID) / Component ID (CID) pair is extended in certain
>>> cases to uniquely identify components. CoreSight components related to
>>> a single function can share Peripheral ID values, and must be further
>>> identified using a Unique Component Identifier (UCI). e.g. the ETM, CTI,
>>> PMU and Debug hardware of the A35 all share the same PID.
>>>
>>
>> [..]
>>
>>> +static const struct amba_id *
>>> +amba_lookup(const struct amba_id *table, struct amba_device *dev)
>>> +{
>>>        while (table->mask) {
>>> -             ret = (dev->periphid & table->mask) == table->id;
>>> -             if (ret)
>>> -                     break;
>>> +             if (((dev->periphid & table->mask) == table->id) &&
>>> +                     ((dev->cid != CORESIGHT_CID) ||
>>> +                      (amba_cs_uci_id_match(table, dev))))
>>
>> Shouldn't the check be (dev->cid == CORESIGHT_CID) ?
>> Without this STM fails to probe on both SDM845 and MSM8996.
>>
> I believe the test is correct
> 
> To expand the logic here:
> 
> if  (dev->periphid & table->mask) == table->id) {
>        //** match on peripheral ID at this point
>       if (CID != CORESIGHT_ID)
>                 return table; //** not coresight - match on peripheral ID only
>       //** or
>       if (amba_cs_uci_id_match() )
>                  return table;  //** is coresight - match on UCI if
> available - otherwise peripheral ID only;
> 
> However - looking at the coresight STM driver - this is using the
> private .data field in the amba_id for a name - which I had not
> spotted before.

Good point. We also use this field for Coresight SOC-600 ETR for
advertising the caps not detected from hardware.

> I will have to revisit this patchset to fix either the amba id struct
> or the method for getting the uci data into the amba_id.data which
> allows for multiple uses.

Thanks Mike !

Cheers
Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel