diff mbox series

[v3,3/8] coresight-tpdm: Add CMB dataset support

Message ID 1700533494-19276-4-git-send-email-quic_taozha@quicinc.com
State New
Headers
Series [v3,1/8] dt-bindings: arm: Add support for CMB element size |

Commit Message

Tao Zhang Nov. 21, 2023, 2:24 a.m. UTC 
  CMB (continuous multi-bit) is one of TPDM's dataset type. CMB subunit
can be enabled for data collection by writing 1 to the first bit of
CMB_CR register. This change is to add enable/disable function for
CMB dataset by writing CMB_CR register.

Reviewed-by: James Clark <james.clark@arm.com>
Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-tpdm.c | 31 ++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpdm.h |  8 +++++
 2 files changed, 39 insertions(+)

Comments

Suzuki K Poulose Dec. 18, 2023, 10:34 a.m. UTC | #1 
On 21/11/2023 02:24, Tao Zhang wrote:
> CMB (continuous multi-bit) is one of TPDM's dataset type. CMB subunit
> can be enabled for data collection by writing 1 to the first bit of
> CMB_CR register. This change is to add enable/disable function for
> CMB dataset by writing CMB_CR register.
> 
> Reviewed-by: James Clark <james.clark@arm.com>
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
> ---
>   drivers/hwtracing/coresight/coresight-tpdm.c | 31 ++++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-tpdm.h |  8 +++++
>   2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index 97654aa4b772..c8bb38822e08 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -131,6 +131,11 @@ static bool tpdm_has_dsb_dataset(struct tpdm_drvdata *drvdata)
>   	return (drvdata->datasets & TPDM_PIDR0_DS_DSB);
>   }
>   
> +static bool tpdm_has_cmb_dataset(struct tpdm_drvdata *drvdata)
> +{
> +	return (drvdata->datasets & TPDM_PIDR0_DS_CMB);
> +}
> +
>   static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
>   				   struct attribute *attr, int n)
>   {
> @@ -267,6 +272,17 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>   	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>   }
>   
> +static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
> +{
> +	u32 val;
> +
> +	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
> +	val |= TPDM_CMB_CR_ENA;
> +
> +	/* Set the enable bit of CMB control register to 1 */
> +	writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
> +}
> +
>   /*
>    * TPDM enable operations
>    * The TPDM or Monitor serves as data collection component for various
> @@ -281,6 +297,8 @@ static void __tpdm_enable(struct tpdm_drvdata *drvdata)
>   
>   	if (tpdm_has_dsb_dataset(drvdata))
>   		tpdm_enable_dsb(drvdata);
> +	if (tpdm_has_cmb_dataset(drvdata))
> +		tpdm_enable_cmb(drvdata);
>   

Don't we need to add this check in the "property read" section ?
Otherwise, we could generate warnings unnecessarily ?

i.e, if (tpdm_has_cmb_..())
	  rc |= fwnode_..read_property(cmb-elem-size...)

Similarly for DSB.

>   	CS_LOCK(drvdata->base);
>   }
> @@ -314,6 +332,17 @@ static void tpdm_disable_dsb(struct tpdm_drvdata *drvdata)
>   	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>   }
>   
> +static void tpdm_disable_cmb(struct tpdm_drvdata *drvdata)
> +{
> +	u32 val;
> +
> +	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
> +	val &= ~TPDM_CMB_CR_ENA;
> +
> +	/* Set the enable bit of CMB control register to 0 */
> +	writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
> +}
> +
>   /* TPDM disable operations */
>   static void __tpdm_disable(struct tpdm_drvdata *drvdata)
>   {
> @@ -321,6 +350,8 @@ static void __tpdm_disable(struct tpdm_drvdata *drvdata)
>   
>   	if (tpdm_has_dsb_dataset(drvdata))
>   		tpdm_disable_dsb(drvdata);
> +	if (tpdm_has_cmb_dataset(drvdata))
> +		tpdm_disable_cmb(drvdata);

minor nit: Instead of having these :

	if (tpdm_has_XY_()
		tpdm_{enable/disable}_XY_()
I prefer :

	tpdm_{enable/disable}_XY_

and the helper take care of returning early if the feature is
not present.


Suzuki


>   
>   	CS_LOCK(drvdata->base);
>   }
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index 4115b2a17b8d..0098c58dfdd6 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -9,6 +9,12 @@
>   /* The max number of the datasets that TPDM supports */
>   #define TPDM_DATASETS       7
>   
> +/* CMB Subunit Registers */
> +#define TPDM_CMB_CR		(0xA00)
> +
> +/* Enable bit for CMB subunit */
> +#define TPDM_CMB_CR_ENA		BIT(0)
> +
>   /* DSB Subunit Registers */
>   #define TPDM_DSB_CR		(0x780)
>   #define TPDM_DSB_TIER		(0x784)
> @@ -79,10 +85,12 @@
>    *
>    * PERIPHIDR0[0] : Fix to 1 if ImplDef subunit present, else 0
>    * PERIPHIDR0[1] : Fix to 1 if DSB subunit present, else 0
> + * PERIPHIDR0[2] : Fix to 1 if CMB subunit present, else 0
>    */
>   
>   #define TPDM_PIDR0_DS_IMPDEF	BIT(0)
>   #define TPDM_PIDR0_DS_DSB	BIT(1)
> +#define TPDM_PIDR0_DS_CMB	BIT(2)
>   
>   #define TPDM_DSB_MAX_LINES	256
>   /* MAX number of EDCR registers */
Tao Zhang Dec. 19, 2023, 9:22 a.m. UTC | #2 
On 12/18/2023 6:34 PM, Suzuki K Poulose wrote:
> On 21/11/2023 02:24, Tao Zhang wrote:
>> CMB (continuous multi-bit) is one of TPDM's dataset type. CMB subunit
>> can be enabled for data collection by writing 1 to the first bit of
>> CMB_CR register. This change is to add enable/disable function for
>> CMB dataset by writing CMB_CR register.
>>
>> Reviewed-by: James Clark <james.clark@arm.com>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-tpdm.c | 31 ++++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.h |  8 +++++
>>   2 files changed, 39 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index 97654aa4b772..c8bb38822e08 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -131,6 +131,11 @@ static bool tpdm_has_dsb_dataset(struct 
>> tpdm_drvdata *drvdata)
>>       return (drvdata->datasets & TPDM_PIDR0_DS_DSB);
>>   }
>>   +static bool tpdm_has_cmb_dataset(struct tpdm_drvdata *drvdata)
>> +{
>> +    return (drvdata->datasets & TPDM_PIDR0_DS_CMB);
>> +}
>> +
>>   static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
>>                      struct attribute *attr, int n)
>>   {
>> @@ -267,6 +272,17 @@ static void tpdm_enable_dsb(struct tpdm_drvdata 
>> *drvdata)
>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>   }
>>   +static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>> +{
>> +    u32 val;
>> +
>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>> +    val |= TPDM_CMB_CR_ENA;
>> +
>> +    /* Set the enable bit of CMB control register to 1 */
>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
>> +}
>> +
>>   /*
>>    * TPDM enable operations
>>    * The TPDM or Monitor serves as data collection component for various
>> @@ -281,6 +297,8 @@ static void __tpdm_enable(struct tpdm_drvdata 
>> *drvdata)
>>         if (tpdm_has_dsb_dataset(drvdata))
>>           tpdm_enable_dsb(drvdata);
>> +    if (tpdm_has_cmb_dataset(drvdata))
>> +        tpdm_enable_cmb(drvdata);
>
> Don't we need to add this check in the "property read" section ?
> Otherwise, we could generate warnings unnecessarily ?
>
> i.e, if (tpdm_has_cmb_..())
>       rc |= fwnode_..read_property(cmb-elem-size...)
>
> Similarly for DSB.

TPDM and TPDA are two independent hardware. If you want to modify them 
in this way, the

two independent drivers will be coupled to each other. At the same time, 
this configuration

is manually set in the devicetree by the users, and this check cannot 
avoid manual setting errors.

  Even if the configuration is wrong, it will not cause the driver to 
stop working, it will only cause

the data to be lost from the TPDM.

>
>>       CS_LOCK(drvdata->base);
>>   }
>> @@ -314,6 +332,17 @@ static void tpdm_disable_dsb(struct tpdm_drvdata 
>> *drvdata)
>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>   }
>>   +static void tpdm_disable_cmb(struct tpdm_drvdata *drvdata)
>> +{
>> +    u32 val;
>> +
>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>> +    val &= ~TPDM_CMB_CR_ENA;
>> +
>> +    /* Set the enable bit of CMB control register to 0 */
>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
>> +}
>> +
>>   /* TPDM disable operations */
>>   static void __tpdm_disable(struct tpdm_drvdata *drvdata)
>>   {
>> @@ -321,6 +350,8 @@ static void __tpdm_disable(struct tpdm_drvdata 
>> *drvdata)
>>         if (tpdm_has_dsb_dataset(drvdata))
>>           tpdm_disable_dsb(drvdata);
>> +    if (tpdm_has_cmb_dataset(drvdata))
>> +        tpdm_disable_cmb(drvdata);
>
> minor nit: Instead of having these :
>
>     if (tpdm_has_XY_()
>         tpdm_{enable/disable}_XY_()
> I prefer :
>
>     tpdm_{enable/disable}_XY_
>
> and the helper take care of returning early if the feature is
> not present.
Does the following sample modification meet your expectation?
static void tpdm_disable_dsb(struct tpdm_drvdata *drvdata)
{
     u32 val;

     if (tpdm_has_dsb_dataset(drvdata)) {
         /* Set the enable bit of DSB control register to 0 */
         val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
         val &= ~TPDM_DSB_CR_ENA;
         writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
     }
}

static void tpdm_disable_cmb(struct tpdm_drvdata *drvdata)
{
     u32 val;

     if (tpdm_has_cmb_dataset(drvdata)) {
         val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
         val &= ~TPDM_CMB_CR_ENA;

         /* Set the enable bit of CMB control register to 0 */
         writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
     }
}

/* TPDM disable operations */
static void __tpdm_disable(struct tpdm_drvdata *drvdata)
{
     CS_UNLOCK(drvdata->base);

     tpdm_disable_dsb(drvdata);
     tpdm_disable_cmb(drvdata);

     CS_LOCK(drvdata->base);

}


Best,

Tao

>
>
> Suzuki
>
>
>>         CS_LOCK(drvdata->base);
>>   }
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>> index 4115b2a17b8d..0098c58dfdd6 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -9,6 +9,12 @@
>>   /* The max number of the datasets that TPDM supports */
>>   #define TPDM_DATASETS       7
>>   +/* CMB Subunit Registers */
>> +#define TPDM_CMB_CR        (0xA00)
>> +
>> +/* Enable bit for CMB subunit */
>> +#define TPDM_CMB_CR_ENA        BIT(0)
>> +
>>   /* DSB Subunit Registers */
>>   #define TPDM_DSB_CR        (0x780)
>>   #define TPDM_DSB_TIER        (0x784)
>> @@ -79,10 +85,12 @@
>>    *
>>    * PERIPHIDR0[0] : Fix to 1 if ImplDef subunit present, else 0
>>    * PERIPHIDR0[1] : Fix to 1 if DSB subunit present, else 0
>> + * PERIPHIDR0[2] : Fix to 1 if CMB subunit present, else 0
>>    */
>>     #define TPDM_PIDR0_DS_IMPDEF    BIT(0)
>>   #define TPDM_PIDR0_DS_DSB    BIT(1)
>> +#define TPDM_PIDR0_DS_CMB    BIT(2)
>>     #define TPDM_DSB_MAX_LINES    256
>>   /* MAX number of EDCR registers */
>
Suzuki K Poulose Dec. 19, 2023, 1:56 p.m. UTC | #3 
On 19/12/2023 09:22, Tao Zhang wrote:
> 
> On 12/18/2023 6:34 PM, Suzuki K Poulose wrote:
>> On 21/11/2023 02:24, Tao Zhang wrote:
>>> CMB (continuous multi-bit) is one of TPDM's dataset type. CMB subunit
>>> can be enabled for data collection by writing 1 to the first bit of
>>> CMB_CR register. This change is to add enable/disable function for
>>> CMB dataset by writing CMB_CR register.
>>>
>>> Reviewed-by: James Clark <james.clark@arm.com>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-tpdm.c | 31 ++++++++++++++++++++
>>>   drivers/hwtracing/coresight/coresight-tpdm.h |  8 +++++
>>>   2 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> index 97654aa4b772..c8bb38822e08 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> @@ -131,6 +131,11 @@ static bool tpdm_has_dsb_dataset(struct 
>>> tpdm_drvdata *drvdata)
>>>       return (drvdata->datasets & TPDM_PIDR0_DS_DSB);
>>>   }
>>>   +static bool tpdm_has_cmb_dataset(struct tpdm_drvdata *drvdata)
>>> +{
>>> +    return (drvdata->datasets & TPDM_PIDR0_DS_CMB);
>>> +}
>>> +
>>>   static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
>>>                      struct attribute *attr, int n)
>>>   {
>>> @@ -267,6 +272,17 @@ static void tpdm_enable_dsb(struct tpdm_drvdata 
>>> *drvdata)
>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>   }
>>>   +static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>> +{
>>> +    u32 val;
>>> +
>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>> +    val |= TPDM_CMB_CR_ENA;
>>> +
>>> +    /* Set the enable bit of CMB control register to 1 */
>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
>>> +}
>>> +
>>>   /*
>>>    * TPDM enable operations
>>>    * The TPDM or Monitor serves as data collection component for various
>>> @@ -281,6 +297,8 @@ static void __tpdm_enable(struct tpdm_drvdata 
>>> *drvdata)
>>>         if (tpdm_has_dsb_dataset(drvdata))
>>>           tpdm_enable_dsb(drvdata);
>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>> +        tpdm_enable_cmb(drvdata);
>>
>> Don't we need to add this check in the "property read" section ?
>> Otherwise, we could generate warnings unnecessarily ?
>>
>> i.e, if (tpdm_has_cmb_..())
>>       rc |= fwnode_..read_property(cmb-elem-size...)
>>
>> Similarly for DSB.
> 
> TPDM and TPDA are two independent hardware. If you want to modify them 
> in this way, the
> 

You don't have to, as long as the header files are included ?

Read my response in the other patch, where it applies.

Suzuki

> two independent drivers will be coupled to each other. At the same time, 
> this configuration
> 
> is manually set in the devicetree by the users, and this check cannot 
> avoid manual setting errors.
> 
>   Even if the configuration is wrong, it will not cause the driver to 
> stop working, it will only cause
> 
> the data to be lost from the TPDM.
> 
>>
>>>       CS_LOCK(drvdata->base);
>>>   }
>>> @@ -314,6 +332,17 @@ static void tpdm_disable_dsb(struct tpdm_drvdata 
>>> *drvdata)
>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>   }
>>>   +static void tpdm_disable_cmb(struct tpdm_drvdata *drvdata)
>>> +{
>>> +    u32 val;
>>> +
>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>> +    val &= ~TPDM_CMB_CR_ENA;
>>> +
>>> +    /* Set the enable bit of CMB control register to 0 */
>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
>>> +}
>>> +
>>>   /* TPDM disable operations */
>>>   static void __tpdm_disable(struct tpdm_drvdata *drvdata)
>>>   {
>>> @@ -321,6 +350,8 @@ static void __tpdm_disable(struct tpdm_drvdata 
>>> *drvdata)
>>>         if (tpdm_has_dsb_dataset(drvdata))
>>>           tpdm_disable_dsb(drvdata);
>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>> +        tpdm_disable_cmb(drvdata);
>>
>> minor nit: Instead of having these :
>>
>>     if (tpdm_has_XY_()
>>         tpdm_{enable/disable}_XY_()
>> I prefer :
>>
>>     tpdm_{enable/disable}_XY_
>>
>> and the helper take care of returning early if the feature is
>> not present.
> Does the following sample modification meet your expectation?
> static void tpdm_disable_dsb(struct tpdm_drvdata *drvdata)
> {
>      u32 val;
> 
>      if (tpdm_has_dsb_dataset(drvdata)) {
>          /* Set the enable bit of DSB control register to 0 */
>          val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
>          val &= ~TPDM_DSB_CR_ENA;
>          writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>      }
> }
> 
> static void tpdm_disable_cmb(struct tpdm_drvdata *drvdata)
> {
>      u32 val;
> 
>      if (tpdm_has_cmb_dataset(drvdata)) {
>          val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>          val &= ~TPDM_CMB_CR_ENA;
> 
>          /* Set the enable bit of CMB control register to 0 */
>          writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
>      }
> }
> 
> /* TPDM disable operations */
> static void __tpdm_disable(struct tpdm_drvdata *drvdata)
> {
>      CS_UNLOCK(drvdata->base);
> 
>      tpdm_disable_dsb(drvdata);
>      tpdm_disable_cmb(drvdata);
> 

Yes, thats exactly I was looking for.

>      CS_LOCK(drvdata->base);
> 
> }
> 
> 
> Best,
> 
> Tao
> 
>>
>>
>> Suzuki
>>
>>
>>>         CS_LOCK(drvdata->base);
>>>   }
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>> index 4115b2a17b8d..0098c58dfdd6 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>> @@ -9,6 +9,12 @@
>>>   /* The max number of the datasets that TPDM supports */
>>>   #define TPDM_DATASETS       7
>>>   +/* CMB Subunit Registers */
>>> +#define TPDM_CMB_CR        (0xA00)
>>> +
>>> +/* Enable bit for CMB subunit */
>>> +#define TPDM_CMB_CR_ENA        BIT(0)
>>> +
>>>   /* DSB Subunit Registers */
>>>   #define TPDM_DSB_CR        (0x780)
>>>   #define TPDM_DSB_TIER        (0x784)
>>> @@ -79,10 +85,12 @@
>>>    *
>>>    * PERIPHIDR0[0] : Fix to 1 if ImplDef subunit present, else 0
>>>    * PERIPHIDR0[1] : Fix to 1 if DSB subunit present, else 0
>>> + * PERIPHIDR0[2] : Fix to 1 if CMB subunit present, else 0
>>>    */
>>>     #define TPDM_PIDR0_DS_IMPDEF    BIT(0)
>>>   #define TPDM_PIDR0_DS_DSB    BIT(1)
>>> +#define TPDM_PIDR0_DS_CMB    BIT(2)
>>>     #define TPDM_DSB_MAX_LINES    256
>>>   /* MAX number of EDCR registers */
>>
Tao Zhang Dec. 20, 2023, 8:56 a.m. UTC | #4 
On 12/19/2023 9:56 PM, Suzuki K Poulose wrote:
> On 19/12/2023 09:22, Tao Zhang wrote:
>>
>> On 12/18/2023 6:34 PM, Suzuki K Poulose wrote:
>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>> CMB (continuous multi-bit) is one of TPDM's dataset type. CMB subunit
>>>> can be enabled for data collection by writing 1 to the first bit of
>>>> CMB_CR register. This change is to add enable/disable function for
>>>> CMB dataset by writing CMB_CR register.
>>>>
>>>> Reviewed-by: James Clark <james.clark@arm.com>
>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>> Signed-off-by: Jinlong Mao <quic_jinlmao@quicinc.com>
>>>> ---
>>>>   drivers/hwtracing/coresight/coresight-tpdm.c | 31 
>>>> ++++++++++++++++++++
>>>>   drivers/hwtracing/coresight/coresight-tpdm.h |  8 +++++
>>>>   2 files changed, 39 insertions(+)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c 
>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> index 97654aa4b772..c8bb38822e08 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> @@ -131,6 +131,11 @@ static bool tpdm_has_dsb_dataset(struct 
>>>> tpdm_drvdata *drvdata)
>>>>       return (drvdata->datasets & TPDM_PIDR0_DS_DSB);
>>>>   }
>>>>   +static bool tpdm_has_cmb_dataset(struct tpdm_drvdata *drvdata)
>>>> +{
>>>> +    return (drvdata->datasets & TPDM_PIDR0_DS_CMB);
>>>> +}
>>>> +
>>>>   static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
>>>>                      struct attribute *attr, int n)
>>>>   {
>>>> @@ -267,6 +272,17 @@ static void tpdm_enable_dsb(struct 
>>>> tpdm_drvdata *drvdata)
>>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>>   }
>>>>   +static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>> +    val |= TPDM_CMB_CR_ENA;
>>>> +
>>>> +    /* Set the enable bit of CMB control register to 1 */
>>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
>>>> +}
>>>> +
>>>>   /*
>>>>    * TPDM enable operations
>>>>    * The TPDM or Monitor serves as data collection component for 
>>>> various
>>>> @@ -281,6 +297,8 @@ static void __tpdm_enable(struct tpdm_drvdata 
>>>> *drvdata)
>>>>         if (tpdm_has_dsb_dataset(drvdata))
>>>>           tpdm_enable_dsb(drvdata);
>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>> +        tpdm_enable_cmb(drvdata);
>>>
>>> Don't we need to add this check in the "property read" section ?
>>> Otherwise, we could generate warnings unnecessarily ?
>>>
>>> i.e, if (tpdm_has_cmb_..())
>>>       rc |= fwnode_..read_property(cmb-elem-size...)
>>>
>>> Similarly for DSB.
>>
>> TPDM and TPDA are two independent hardware. If you want to modify 
>> them in this way, the
>>
>
> You don't have to, as long as the header files are included ?
>
> Read my response in the other patch, where it applies.

Got it. I will update in the next patch series.


Best,

Tao

>
> Suzuki
>
>> two independent drivers will be coupled to each other. At the same 
>> time, this configuration
>>
>> is manually set in the devicetree by the users, and this check cannot 
>> avoid manual setting errors.
>>
>>   Even if the configuration is wrong, it will not cause the driver to 
>> stop working, it will only cause
>>
>> the data to be lost from the TPDM.
>>
>>>
>>>>       CS_LOCK(drvdata->base);
>>>>   }
>>>> @@ -314,6 +332,17 @@ static void tpdm_disable_dsb(struct 
>>>> tpdm_drvdata *drvdata)
>>>>       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>>>   }
>>>>   +static void tpdm_disable_cmb(struct tpdm_drvdata *drvdata)
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>>> +    val &= ~TPDM_CMB_CR_ENA;
>>>> +
>>>> +    /* Set the enable bit of CMB control register to 0 */
>>>> +    writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
>>>> +}
>>>> +
>>>>   /* TPDM disable operations */
>>>>   static void __tpdm_disable(struct tpdm_drvdata *drvdata)
>>>>   {
>>>> @@ -321,6 +350,8 @@ static void __tpdm_disable(struct tpdm_drvdata 
>>>> *drvdata)
>>>>         if (tpdm_has_dsb_dataset(drvdata))
>>>>           tpdm_disable_dsb(drvdata);
>>>> +    if (tpdm_has_cmb_dataset(drvdata))
>>>> +        tpdm_disable_cmb(drvdata);
>>>
>>> minor nit: Instead of having these :
>>>
>>>     if (tpdm_has_XY_()
>>>         tpdm_{enable/disable}_XY_()
>>> I prefer :
>>>
>>>     tpdm_{enable/disable}_XY_
>>>
>>> and the helper take care of returning early if the feature is
>>> not present.
>> Does the following sample modification meet your expectation?
>> static void tpdm_disable_dsb(struct tpdm_drvdata *drvdata)
>> {
>>      u32 val;
>>
>>      if (tpdm_has_dsb_dataset(drvdata)) {
>>          /* Set the enable bit of DSB control register to 0 */
>>          val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
>>          val &= ~TPDM_DSB_CR_ENA;
>>          writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>>      }
>> }
>>
>> static void tpdm_disable_cmb(struct tpdm_drvdata *drvdata)
>> {
>>      u32 val;
>>
>>      if (tpdm_has_cmb_dataset(drvdata)) {
>>          val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
>>          val &= ~TPDM_CMB_CR_ENA;
>>
>>          /* Set the enable bit of CMB control register to 0 */
>>          writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
>>      }
>> }
>>
>> /* TPDM disable operations */
>> static void __tpdm_disable(struct tpdm_drvdata *drvdata)
>> {
>>      CS_UNLOCK(drvdata->base);
>>
>>      tpdm_disable_dsb(drvdata);
>>      tpdm_disable_cmb(drvdata);
>>
>
> Yes, thats exactly I was looking for.

Got it. I will update in the next patch series.


Best,

Tao

>
>>      CS_LOCK(drvdata->base);
>>
>> }
>>
>>
>> Best,
>>
>> Tao
>>
>>>
>>>
>>> Suzuki
>>>
>>>
>>>>         CS_LOCK(drvdata->base);
>>>>   }
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h 
>>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> index 4115b2a17b8d..0098c58dfdd6 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> @@ -9,6 +9,12 @@
>>>>   /* The max number of the datasets that TPDM supports */
>>>>   #define TPDM_DATASETS       7
>>>>   +/* CMB Subunit Registers */
>>>> +#define TPDM_CMB_CR        (0xA00)
>>>> +
>>>> +/* Enable bit for CMB subunit */
>>>> +#define TPDM_CMB_CR_ENA        BIT(0)
>>>> +
>>>>   /* DSB Subunit Registers */
>>>>   #define TPDM_DSB_CR        (0x780)
>>>>   #define TPDM_DSB_TIER        (0x784)
>>>> @@ -79,10 +85,12 @@
>>>>    *
>>>>    * PERIPHIDR0[0] : Fix to 1 if ImplDef subunit present, else 0
>>>>    * PERIPHIDR0[1] : Fix to 1 if DSB subunit present, else 0
>>>> + * PERIPHIDR0[2] : Fix to 1 if CMB subunit present, else 0
>>>>    */
>>>>     #define TPDM_PIDR0_DS_IMPDEF    BIT(0)
>>>>   #define TPDM_PIDR0_DS_DSB    BIT(1)
>>>> +#define TPDM_PIDR0_DS_CMB    BIT(2)
>>>>     #define TPDM_DSB_MAX_LINES    256
>>>>   /* MAX number of EDCR registers */
>>>
>
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index 97654aa4b772..c8bb38822e08 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -131,6 +131,11 @@  static bool tpdm_has_dsb_dataset(struct tpdm_drvdata *drvdata)
 	return (drvdata->datasets & TPDM_PIDR0_DS_DSB);
 }
 
+static bool tpdm_has_cmb_dataset(struct tpdm_drvdata *drvdata)
+{
+	return (drvdata->datasets & TPDM_PIDR0_DS_CMB);
+}
+
 static umode_t tpdm_dsb_is_visible(struct kobject *kobj,
 				   struct attribute *attr, int n)
 {
@@ -267,6 +272,17 @@  static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
 }
 
+static void tpdm_enable_cmb(struct tpdm_drvdata *drvdata)
+{
+	u32 val;
+
+	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
+	val |= TPDM_CMB_CR_ENA;
+
+	/* Set the enable bit of CMB control register to 1 */
+	writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
+}
+
 /*
  * TPDM enable operations
  * The TPDM or Monitor serves as data collection component for various
@@ -281,6 +297,8 @@  static void __tpdm_enable(struct tpdm_drvdata *drvdata)
 
 	if (tpdm_has_dsb_dataset(drvdata))
 		tpdm_enable_dsb(drvdata);
+	if (tpdm_has_cmb_dataset(drvdata))
+		tpdm_enable_cmb(drvdata);
 
 	CS_LOCK(drvdata->base);
 }
@@ -314,6 +332,17 @@  static void tpdm_disable_dsb(struct tpdm_drvdata *drvdata)
 	writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
 }
 
+static void tpdm_disable_cmb(struct tpdm_drvdata *drvdata)
+{
+	u32 val;
+
+	val = readl_relaxed(drvdata->base + TPDM_CMB_CR);
+	val &= ~TPDM_CMB_CR_ENA;
+
+	/* Set the enable bit of CMB control register to 0 */
+	writel_relaxed(val, drvdata->base + TPDM_CMB_CR);
+}
+
 /* TPDM disable operations */
 static void __tpdm_disable(struct tpdm_drvdata *drvdata)
 {
@@ -321,6 +350,8 @@  static void __tpdm_disable(struct tpdm_drvdata *drvdata)
 
 	if (tpdm_has_dsb_dataset(drvdata))
 		tpdm_disable_dsb(drvdata);
+	if (tpdm_has_cmb_dataset(drvdata))
+		tpdm_disable_cmb(drvdata);
 
 	CS_LOCK(drvdata->base);
 }
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 4115b2a17b8d..0098c58dfdd6 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -9,6 +9,12 @@ 
 /* The max number of the datasets that TPDM supports */
 #define TPDM_DATASETS       7
 
+/* CMB Subunit Registers */
+#define TPDM_CMB_CR		(0xA00)
+
+/* Enable bit for CMB subunit */
+#define TPDM_CMB_CR_ENA		BIT(0)
+
 /* DSB Subunit Registers */
 #define TPDM_DSB_CR		(0x780)
 #define TPDM_DSB_TIER		(0x784)
@@ -79,10 +85,12 @@ 
  *
  * PERIPHIDR0[0] : Fix to 1 if ImplDef subunit present, else 0
  * PERIPHIDR0[1] : Fix to 1 if DSB subunit present, else 0
+ * PERIPHIDR0[2] : Fix to 1 if CMB subunit present, else 0
  */
 
 #define TPDM_PIDR0_DS_IMPDEF	BIT(0)
 #define TPDM_PIDR0_DS_DSB	BIT(1)
+#define TPDM_PIDR0_DS_CMB	BIT(2)
 
 #define TPDM_DSB_MAX_LINES	256
 /* MAX number of EDCR registers */