diff mbox series

[v6,09/13] Add nodes for dsb edge control

Message ID 1687246361-23607-10-git-send-email-quic_taozha@quicinc.com
State New
Headers
Series Add support to configure TPDM DSB subunit |

Commit Message

Tao Zhang June 20, 2023, 7:32 a.m. UTC 
  Add the nodes to set value for DSB edge control and DSB edge
control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
resgisters to configure edge control. DSB edge detection control
00: Rising edge detection
01: Falling edge detection
10: Rising and falling edge detection (toggle detection)
And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
configure mask. Eight 32 bit registers providing DSB interface
edge detection mask control.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  32 +++++
 drivers/hwtracing/coresight/coresight-tpdm.c       | 143 ++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tpdm.h       |  22 ++++
 3 files changed, 196 insertions(+), 1 deletion(-)

Comments

Greg KH June 20, 2023, 7:37 a.m. UTC | #1 
On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
> Add the nodes to set value for DSB edge control and DSB edge
> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
> resgisters to configure edge control. DSB edge detection control
> 00: Rising edge detection
> 01: Falling edge detection
> 10: Rising and falling edge detection (toggle detection)
> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
> configure mask. Eight 32 bit registers providing DSB interface
> edge detection mask control.
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>  .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  32 +++++
>  drivers/hwtracing/coresight/coresight-tpdm.c       | 143 ++++++++++++++++++++-
>  drivers/hwtracing/coresight/coresight-tpdm.h       |  22 ++++
>  3 files changed, 196 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> index 2a82cd0..34189e4a 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -60,3 +60,35 @@ Description:
>  		Bit[3] : Set to 0 for low performance mode.
>  				 Set to 1 for high performance mode.
>  		Bit[4:8] : Select byte lane for high performance mode.
> +
> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
> +Date:		March 2023
> +KernelVersion	6.5
> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +		Read/Write a set of the edge control registers of the DSB
> +		in TPDM.
> +
> +		Expected format is the following:
> +		<integer1> <integer2> <integer3>

sysfs is "one value", not 3.  Please never have to parse a sysfs file.

> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buf)
> +{
> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	ssize_t size = 0;
> +	unsigned long bytes;
> +	int i;
> +
> +	spin_lock(&drvdata->spinlock);
> +	for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
> +		bytes = sysfs_emit_at(buf, size,
> +				  "Index:0x%x Val:0x%x\n", i,

Again, no, one value, no "string" needed to parse anything.

greg k-h
Tao Zhang June 20, 2023, 8:31 a.m. UTC | #2 
On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
> On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
>> Add the nodes to set value for DSB edge control and DSB edge
>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>> resgisters to configure edge control. DSB edge detection control
>> 00: Rising edge detection
>> 01: Falling edge detection
>> 10: Rising and falling edge detection (toggle detection)
>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
>> configure mask. Eight 32 bit registers providing DSB interface
>> edge detection mask control.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  32 +++++
>>   drivers/hwtracing/coresight/coresight-tpdm.c       | 143 ++++++++++++++++++++-
>>   drivers/hwtracing/coresight/coresight-tpdm.h       |  22 ++++
>>   3 files changed, 196 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> index 2a82cd0..34189e4a 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -60,3 +60,35 @@ Description:
>>   		Bit[3] : Set to 0 for low performance mode.
>>   				 Set to 1 for high performance mode.
>>   		Bit[4:8] : Select byte lane for high performance mode.
>> +
>> +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>> +Date:		March 2023
>> +KernelVersion	6.5
>> +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +		Read/Write a set of the edge control registers of the DSB
>> +		in TPDM.
>> +
>> +		Expected format is the following:
>> +		<integer1> <integer2> <integer3>
> sysfs is "one value", not 3.  Please never have to parse a sysfs file.

Do you mean sysfs file can only accept "one value"?

I see that more than one value are written to the sysfs file 
"trigout_attach".

>
>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>> +				       struct device_attribute *attr,
>> +				       char *buf)
>> +{
>> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +	ssize_t size = 0;
>> +	unsigned long bytes;
>> +	int i;
>> +
>> +	spin_lock(&drvdata->spinlock);
>> +	for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>> +		bytes = sysfs_emit_at(buf, size,
>> +				  "Index:0x%x Val:0x%x\n", i,
> Again, no, one value, no "string" needed to parse anything.

I also see other sysfs files can be read more than one value in other 
drivers.

Is this "one value" limitation the usage rule of Linux sysfs system?

Or am I misunderstanding what you mean?


Best,

Tao

>
> greg k-h
Greg KH June 20, 2023, 8:49 a.m. UTC | #3 
On Tue, Jun 20, 2023 at 04:31:59PM +0800, Tao Zhang wrote:
> 
> On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
> > On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
> > > Add the nodes to set value for DSB edge control and DSB edge
> > > control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
> > > resgisters to configure edge control. DSB edge detection control
> > > 00: Rising edge detection
> > > 01: Falling edge detection
> > > 10: Rising and falling edge detection (toggle detection)
> > > And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
> > > configure mask. Eight 32 bit registers providing DSB interface
> > > edge detection mask control.
> > > 
> > > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> > > ---
> > >   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  32 +++++
> > >   drivers/hwtracing/coresight/coresight-tpdm.c       | 143 ++++++++++++++++++++-
> > >   drivers/hwtracing/coresight/coresight-tpdm.h       |  22 ++++
> > >   3 files changed, 196 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> > > index 2a82cd0..34189e4a 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> > > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> > > @@ -60,3 +60,35 @@ Description:
> > >   		Bit[3] : Set to 0 for low performance mode.
> > >   				 Set to 1 for high performance mode.
> > >   		Bit[4:8] : Select byte lane for high performance mode.
> > > +
> > > +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
> > > +Date:		March 2023
> > > +KernelVersion	6.5
> > > +Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> > > +Description:
> > > +		Read/Write a set of the edge control registers of the DSB
> > > +		in TPDM.
> > > +
> > > +		Expected format is the following:
> > > +		<integer1> <integer2> <integer3>
> > sysfs is "one value", not 3.  Please never have to parse a sysfs file.
> 
> Do you mean sysfs file can only accept "one value"?

Yes.

> I see that more than one value are written to the sysfs file
> "trigout_attach".

Then someone missed that and it needs to be fixed.

> > > +static ssize_t dsb_edge_ctrl_show(struct device *dev,
> > > +				       struct device_attribute *attr,
> > > +				       char *buf)
> > > +{
> > > +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> > > +	ssize_t size = 0;
> > > +	unsigned long bytes;
> > > +	int i;
> > > +
> > > +	spin_lock(&drvdata->spinlock);
> > > +	for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
> > > +		bytes = sysfs_emit_at(buf, size,
> > > +				  "Index:0x%x Val:0x%x\n", i,
> > Again, no, one value, no "string" needed to parse anything.
> 
> I also see other sysfs files can be read more than one value in other
> drivers.

Again, they are broken, please send patches to fix them.

> Is this "one value" limitation the usage rule of Linux sysfs system?

Yes.

thanks,

greg k-h
Suzuki K Poulose June 20, 2023, 1:41 p.m. UTC | #4 
On 20/06/2023 09:31, Tao Zhang wrote:
> 
> On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
>> On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
>>> Add the nodes to set value for DSB edge control and DSB edge
>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>>> resgisters to configure edge control. DSB edge detection control
>>> 00: Rising edge detection
>>> 01: Falling edge detection
>>> 10: Rising and falling edge detection (toggle detection)
>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
>>> configure mask. Eight 32 bit registers providing DSB interface
>>> edge detection mask control.
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> ---
>>>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  32 +++++
>>>   drivers/hwtracing/coresight/coresight-tpdm.c       | 143 
>>> ++++++++++++++++++++-
>>>   drivers/hwtracing/coresight/coresight-tpdm.h       |  22 ++++
>>>   3 files changed, 196 insertions(+), 1 deletion(-)
>>>
>>> diff --git 
>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> index 2a82cd0..34189e4a 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> @@ -60,3 +60,35 @@ Description:
>>>           Bit[3] : Set to 0 for low performance mode.
>>>                    Set to 1 for high performance mode.
>>>           Bit[4:8] : Select byte lane for high performance mode.
>>> +
>>> +What:        /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>>> +Date:        March 2023
>>> +KernelVersion    6.5
>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang 
>>> (QUIC) <quic_taozha@quicinc.com>
>>> +Description:
>>> +        Read/Write a set of the edge control registers of the DSB
>>> +        in TPDM.
>>> +
>>> +        Expected format is the following:
>>> +        <integer1> <integer2> <integer3>
>> sysfs is "one value", not 3.  Please never have to parse a sysfs file.
> 
> Do you mean sysfs file can only accept "one value"?
> 
> I see that more than one value are written to the sysfs file 
> "trigout_attach".
> 
>>
>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>>> +                       struct device_attribute *attr,
>>> +                       char *buf)
>>> +{
>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    ssize_t size = 0;
>>> +    unsigned long bytes;
>>> +    int i;
>>> +
>>> +    spin_lock(&drvdata->spinlock);
>>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>>> +        bytes = sysfs_emit_at(buf, size,
>>> +                  "Index:0x%x Val:0x%x\n", i,
>> Again, no, one value, no "string" needed to parse anything.
> 
> I also see other sysfs files can be read more than one value in other 
> drivers.
> 
> Is this "one value" limitation the usage rule of Linux sysfs system?
> 
> Or am I misunderstanding what you mean?

Please fix the other sysfs tunables in the following patches.

Kind regards
Suzuki
Greg KH June 28, 2023, 6:13 a.m. UTC | #5 
On Wed, Jun 28, 2023 at 11:11:29AM +0800, Tao Zhang wrote:
> 
> On 6/20/2023 4:49 PM, Greg Kroah-Hartman wrote:
> > On Tue, Jun 20, 2023 at 04:31:59PM +0800, Tao Zhang wrote:
> > > On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
> > > > On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
> > > > > Add the nodes to set value for DSB edge control and DSB edge
> > > > > control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
> > > > > resgisters to configure edge control. DSB edge detection control
> > > > > 00: Rising edge detection
> > > > > 01: Falling edge detection
> > > > > 10: Rising and falling edge detection (toggle detection)
> > > > > And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
> > > > > configure mask. Eight 32 bit registers providing DSB interface
> > > > > edge detection mask control.
> > > > > 
> > > > > Signed-off-by: Tao Zhang<quic_taozha@quicinc.com>
> > > > > ---
> > > > >    .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  32 +++++
> > > > >    drivers/hwtracing/coresight/coresight-tpdm.c       | 143 ++++++++++++++++++++-
> > > > >    drivers/hwtracing/coresight/coresight-tpdm.h       |  22 ++++
> > > > >    3 files changed, 196 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> > > > > index 2a82cd0..34189e4a 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> > > > > @@ -60,3 +60,35 @@ Description:
> > > > >    		Bit[3] : Set to 0 for low performance mode.
> > > > >    				 Set to 1 for high performance mode.
> > > > >    		Bit[4:8] : Select byte lane for high performance mode.
> > > > > +
> > > > > +What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
> > > > > +Date:		March 2023
> > > > > +KernelVersion	6.5
> > > > > +Contact:	Jinlong Mao (QUIC)<quic_jinlmao@quicinc.com>, Tao Zhang (QUIC)<quic_taozha@quicinc.com>
> > > > > +Description:
> > > > > +		Read/Write a set of the edge control registers of the DSB
> > > > > +		in TPDM.
> > > > > +
> > > > > +		Expected format is the following:
> > > > > +		<integer1> <integer2> <integer3>
> > > > sysfs is "one value", not 3.  Please never have to parse a sysfs file.
> > > Do you mean sysfs file can only accept "one value"?
> > Yes.
> 
> Hi Greg,
> 
> 
> I‘d like to clarify the usage of this sysfs file again.
> 
> In the current design, three integers will be written to "dsb_edge_ctrl" to
> configure DSB edge detection.
> 
> Integer #1: The start number of edge detection which needs to be configured.
> 
> Integer #2: The end number of edge detection which needs to be configured.
> 
> Integer #3: The type of the edge detection needs to be configured.

All of this is wrong.  Again, sysfs is "one value per file"

> Below is an example.
> 
> echo 0x3 0x25 0x1 > dsb_edge_ctrl
> 
> It will configure edge detection #3 to #37 as "falling edge detection".
> 
> Since these three integers are interrelated and written to achieve the same
> function, can we use these three integers as "one tuple" here?

Nope!

sorry,

greg k-h
Tao Zhang July 12, 2023, 1:53 p.m. UTC | #6 
On 6/20/2023 9:41 PM, Suzuki K Poulose wrote:
> On 20/06/2023 09:31, Tao Zhang wrote:
>>
>> On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
>>> On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
>>>> Add the nodes to set value for DSB edge control and DSB edge
>>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>>>> resgisters to configure edge control. DSB edge detection control
>>>> 00: Rising edge detection
>>>> 01: Falling edge detection
>>>> 10: Rising and falling edge detection (toggle detection)
>>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
>>>> configure mask. Eight 32 bit registers providing DSB interface
>>>> edge detection mask control.
>>>>
>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>> ---
>>>>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  32 +++++
>>>>   drivers/hwtracing/coresight/coresight-tpdm.c       | 143 
>>>> ++++++++++++++++++++-
>>>>   drivers/hwtracing/coresight/coresight-tpdm.h       |  22 ++++
>>>>   3 files changed, 196 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git 
>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm 
>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> index 2a82cd0..34189e4a 100644
>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> @@ -60,3 +60,35 @@ Description:
>>>>           Bit[3] : Set to 0 for low performance mode.
>>>>                    Set to 1 for high performance mode.
>>>>           Bit[4:8] : Select byte lane for high performance mode.
>>>> +
>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>>>> +Date:        March 2023
>>>> +KernelVersion    6.5
>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao 
>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>> +Description:
>>>> +        Read/Write a set of the edge control registers of the DSB
>>>> +        in TPDM.
>>>> +
>>>> +        Expected format is the following:
>>>> +        <integer1> <integer2> <integer3>
>>> sysfs is "one value", not 3.  Please never have to parse a sysfs file.
>>
>> Do you mean sysfs file can only accept "one value"?
>>
>> I see that more than one value are written to the sysfs file 
>> "trigout_attach".
>>
>>>
>>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>>>> +                       struct device_attribute *attr,
>>>> +                       char *buf)
>>>> +{
>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +    ssize_t size = 0;
>>>> +    unsigned long bytes;
>>>> +    int i;
>>>> +
>>>> +    spin_lock(&drvdata->spinlock);
>>>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>>>> +        bytes = sysfs_emit_at(buf, size,
>>>> +                  "Index:0x%x Val:0x%x\n", i,
>>> Again, no, one value, no "string" needed to parse anything.
>>
>> I also see other sysfs files can be read more than one value in other 
>> drivers.
>>
>> Is this "one value" limitation the usage rule of Linux sysfs system?
>>
>> Or am I misunderstanding what you mean?
>
> Please fix the other sysfs tunables in the following patches.

List a new solution for the similar cases below, please see if this 
design is reasonable?

1. Two SysFS files("dsb_edge_ctrl_idx" and "dsb_edge_ctrl_val") will be 
created in this case.

2. First write to the node "dsb_edge_ctrl_idx" to set the index number 
of the edge detection.

3. Then write to the node "dsb_edge_ctrl_val" to set the value of the 
edge detection.

For example, if we need need to set "Falling edge detection" to the edge 
detection #220-#222, we can issue the following commands.

echo 0xdc > tpdm1/dsb_edge_ctrl_idx

echo 0x1 > tpdm1/dsb_edge_ctrl_val

echo 0xdd > tpdm1/dsb_edge_ctrl_idx

echo 0x1 > tpdm1/dsb_edge_ctrl_val

echo 0xde > tpdm1/dsb_edge_ctrl_idx

echo 0x1 > tpdm1/dsb_edge_ctrl_val

If this design is acceptable, we will rewrite other similar nodes based 
on this solution.

Let me know if you have any concerns or good suggestions for this solution.


Best,

Tao

>
> Kind regards
> Suzuki
>
>
Mike Leach July 13, 2023, 8:54 a.m. UTC | #7 
HI Tao,

On Wed, 12 Jul 2023 at 14:53, Tao Zhang <quic_taozha@quicinc.com> wrote:
>
>
> On 6/20/2023 9:41 PM, Suzuki K Poulose wrote:
> > On 20/06/2023 09:31, Tao Zhang wrote:
> >>
> >> On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
> >>> On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
> >>>> Add the nodes to set value for DSB edge control and DSB edge
> >>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
> >>>> resgisters to configure edge control. DSB edge detection control
> >>>> 00: Rising edge detection
> >>>> 01: Falling edge detection
> >>>> 10: Rising and falling edge detection (toggle detection)
> >>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
> >>>> configure mask. Eight 32 bit registers providing DSB interface
> >>>> edge detection mask control.
> >>>>
> >>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> >>>> ---
> >>>>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  32 +++++
> >>>>   drivers/hwtracing/coresight/coresight-tpdm.c       | 143
> >>>> ++++++++++++++++++++-
> >>>>   drivers/hwtracing/coresight/coresight-tpdm.h       |  22 ++++
> >>>>   3 files changed, 196 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git
> >>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> >>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> >>>> index 2a82cd0..34189e4a 100644
> >>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> >>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> >>>> @@ -60,3 +60,35 @@ Description:
> >>>>           Bit[3] : Set to 0 for low performance mode.
> >>>>                    Set to 1 for high performance mode.
> >>>>           Bit[4:8] : Select byte lane for high performance mode.
> >>>> +
> >>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
> >>>> +Date:        March 2023
> >>>> +KernelVersion    6.5
> >>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao
> >>>> Zhang (QUIC) <quic_taozha@quicinc.com>
> >>>> +Description:
> >>>> +        Read/Write a set of the edge control registers of the DSB
> >>>> +        in TPDM.
> >>>> +
> >>>> +        Expected format is the following:
> >>>> +        <integer1> <integer2> <integer3>
> >>> sysfs is "one value", not 3.  Please never have to parse a sysfs file.
> >>
> >> Do you mean sysfs file can only accept "one value"?
> >>
> >> I see that more than one value are written to the sysfs file
> >> "trigout_attach".
> >>
> >>>
> >>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
> >>>> +                       struct device_attribute *attr,
> >>>> +                       char *buf)
> >>>> +{
> >>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> >>>> +    ssize_t size = 0;
> >>>> +    unsigned long bytes;
> >>>> +    int i;
> >>>> +
> >>>> +    spin_lock(&drvdata->spinlock);
> >>>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
> >>>> +        bytes = sysfs_emit_at(buf, size,
> >>>> +                  "Index:0x%x Val:0x%x\n", i,
> >>> Again, no, one value, no "string" needed to parse anything.
> >>
> >> I also see other sysfs files can be read more than one value in other
> >> drivers.
> >>
> >> Is this "one value" limitation the usage rule of Linux sysfs system?
> >>
> >> Or am I misunderstanding what you mean?
> >
> > Please fix the other sysfs tunables in the following patches.
>
> List a new solution for the similar cases below, please see if this
> design is reasonable?
>
> 1. Two SysFS files("dsb_edge_ctrl_idx" and "dsb_edge_ctrl_val") will be
> created in this case.
>
> 2. First write to the node "dsb_edge_ctrl_idx" to set the index number
> of the edge detection.
>
> 3. Then write to the node "dsb_edge_ctrl_val" to set the value of the
> edge detection.
>
> For example, if we need need to set "Falling edge detection" to the edge
> detection #220-#222, we can issue the following commands.
>
> echo 0xdc > tpdm1/dsb_edge_ctrl_idx
>
> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>
> echo 0xdd > tpdm1/dsb_edge_ctrl_idx
>
> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>
> echo 0xde > tpdm1/dsb_edge_ctrl_idx
>
> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>
> If this design is acceptable, we will rewrite other similar nodes based
> on this solution.
>

This index / value model is used in the coresight drivers so should be
OK - eg etm4 has cntr_idx / cntrldvr / cntr_val / cntr_ctrl, where
index selects the counter, and the other val registers are applied to
that counter.

Mike

> Let me know if you have any concerns or good suggestions for this solution.
>
>
> Best,
>
> Tao
>
> >
> > Kind regards
> > Suzuki
> >
> >
Suzuki K Poulose July 13, 2023, 9:34 a.m. UTC | #8 
On 13/07/2023 09:54, Mike Leach wrote:
> HI Tao,
> 
> On Wed, 12 Jul 2023 at 14:53, Tao Zhang <quic_taozha@quicinc.com> wrote:
>>
>>
>> On 6/20/2023 9:41 PM, Suzuki K Poulose wrote:
>>> On 20/06/2023 09:31, Tao Zhang wrote:
>>>>
>>>> On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
>>>>> On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
>>>>>> Add the nodes to set value for DSB edge control and DSB edge
>>>>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>>>>>> resgisters to configure edge control. DSB edge detection control
>>>>>> 00: Rising edge detection
>>>>>> 01: Falling edge detection
>>>>>> 10: Rising and falling edge detection (toggle detection)
>>>>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
>>>>>> configure mask. Eight 32 bit registers providing DSB interface
>>>>>> edge detection mask control.
>>>>>>
>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>> ---
>>>>>>    .../ABI/testing/sysfs-bus-coresight-devices-tpdm   |  32 +++++
>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.c       | 143
>>>>>> ++++++++++++++++++++-
>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.h       |  22 ++++
>>>>>>    3 files changed, 196 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> index 2a82cd0..34189e4a 100644
>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>> @@ -60,3 +60,35 @@ Description:
>>>>>>            Bit[3] : Set to 0 for low performance mode.
>>>>>>                     Set to 1 for high performance mode.
>>>>>>            Bit[4:8] : Select byte lane for high performance mode.
>>>>>> +
>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>>>>>> +Date:        March 2023
>>>>>> +KernelVersion    6.5
>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao
>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>> +Description:
>>>>>> +        Read/Write a set of the edge control registers of the DSB
>>>>>> +        in TPDM.
>>>>>> +
>>>>>> +        Expected format is the following:
>>>>>> +        <integer1> <integer2> <integer3>
>>>>> sysfs is "one value", not 3.  Please never have to parse a sysfs file.
>>>>
>>>> Do you mean sysfs file can only accept "one value"?
>>>>
>>>> I see that more than one value are written to the sysfs file
>>>> "trigout_attach".
>>>>
>>>>>
>>>>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>>>>>> +                       struct device_attribute *attr,
>>>>>> +                       char *buf)
>>>>>> +{
>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>> +    ssize_t size = 0;
>>>>>> +    unsigned long bytes;
>>>>>> +    int i;
>>>>>> +
>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>>>>>> +        bytes = sysfs_emit_at(buf, size,
>>>>>> +                  "Index:0x%x Val:0x%x\n", i,
>>>>> Again, no, one value, no "string" needed to parse anything.
>>>>
>>>> I also see other sysfs files can be read more than one value in other
>>>> drivers.
>>>>
>>>> Is this "one value" limitation the usage rule of Linux sysfs system?
>>>>
>>>> Or am I misunderstanding what you mean?
>>>
>>> Please fix the other sysfs tunables in the following patches.
>>
>> List a new solution for the similar cases below, please see if this
>> design is reasonable?
>>
>> 1. Two SysFS files("dsb_edge_ctrl_idx" and "dsb_edge_ctrl_val") will be
>> created in this case.
>>
>> 2. First write to the node "dsb_edge_ctrl_idx" to set the index number
>> of the edge detection.
>>
>> 3. Then write to the node "dsb_edge_ctrl_val" to set the value of the
>> edge detection.
>>
>> For example, if we need need to set "Falling edge detection" to the edge
>> detection #220-#222, we can issue the following commands.
>>
>> echo 0xdc > tpdm1/dsb_edge_ctrl_idx
>>
>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>
>> echo 0xdd > tpdm1/dsb_edge_ctrl_idx
>>
>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>
>> echo 0xde > tpdm1/dsb_edge_ctrl_idx
>>
>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>
>> If this design is acceptable, we will rewrite other similar nodes based
>> on this solution.
>>
> 
> This index / value model is used in the coresight drivers so should be
> OK - eg etm4 has cntr_idx / cntrldvr / cntr_val / cntr_ctrl, where
> index selects the counter, and the other val registers are applied to
> that counter.

True. That model is useful when there are variable number of "counters".
I guess it doesn't hurt to have a 64bit (or even 32bit) file for each
EDCR.

e.g, edcr0...edcr15

Given there are only 16 of them, it is fine to keep a file for each.
This may be grouped under "mgmt" similar to what we have for other
components. That way, it can be easily hidden by checking for the
presence of DSB.

Suzuki
Tao Zhang July 13, 2023, 4:13 p.m. UTC | #9 
On 7/13/2023 5:34 PM, Suzuki K Poulose wrote:
> On 13/07/2023 09:54, Mike Leach wrote:
>> HI Tao,
>>
>> On Wed, 12 Jul 2023 at 14:53, Tao Zhang <quic_taozha@quicinc.com> wrote:
>>>
>>>
>>> On 6/20/2023 9:41 PM, Suzuki K Poulose wrote:
>>>> On 20/06/2023 09:31, Tao Zhang wrote:
>>>>>
>>>>> On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
>>>>>> On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
>>>>>>> Add the nodes to set value for DSB edge control and DSB edge
>>>>>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>>>>>>> resgisters to configure edge control. DSB edge detection control
>>>>>>> 00: Rising edge detection
>>>>>>> 01: Falling edge detection
>>>>>>> 10: Rising and falling edge detection (toggle detection)
>>>>>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
>>>>>>> configure mask. Eight 32 bit registers providing DSB interface
>>>>>>> edge detection mask control.
>>>>>>>
>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>> ---
>>>>>>>    .../ABI/testing/sysfs-bus-coresight-devices-tpdm |  32 +++++
>>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.c | 143
>>>>>>> ++++++++++++++++++++-
>>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.h |  22 ++++
>>>>>>>    3 files changed, 196 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> index 2a82cd0..34189e4a 100644
>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>> @@ -60,3 +60,35 @@ Description:
>>>>>>>            Bit[3] : Set to 0 for low performance mode.
>>>>>>>                     Set to 1 for high performance mode.
>>>>>>>            Bit[4:8] : Select byte lane for high performance mode.
>>>>>>> +
>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>>>>>>> +Date:        March 2023
>>>>>>> +KernelVersion    6.5
>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao
>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>> +Description:
>>>>>>> +        Read/Write a set of the edge control registers of the DSB
>>>>>>> +        in TPDM.
>>>>>>> +
>>>>>>> +        Expected format is the following:
>>>>>>> +        <integer1> <integer2> <integer3>
>>>>>> sysfs is "one value", not 3.  Please never have to parse a sysfs 
>>>>>> file.
>>>>>
>>>>> Do you mean sysfs file can only accept "one value"?
>>>>>
>>>>> I see that more than one value are written to the sysfs file
>>>>> "trigout_attach".
>>>>>
>>>>>>
>>>>>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>>>>>>> +                       struct device_attribute *attr,
>>>>>>> +                       char *buf)
>>>>>>> +{
>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>> +    ssize_t size = 0;
>>>>>>> +    unsigned long bytes;
>>>>>>> +    int i;
>>>>>>> +
>>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>>>>>>> +        bytes = sysfs_emit_at(buf, size,
>>>>>>> +                  "Index:0x%x Val:0x%x\n", i,
>>>>>> Again, no, one value, no "string" needed to parse anything.
>>>>>
>>>>> I also see other sysfs files can be read more than one value in other
>>>>> drivers.
>>>>>
>>>>> Is this "one value" limitation the usage rule of Linux sysfs system?
>>>>>
>>>>> Or am I misunderstanding what you mean?
>>>>
>>>> Please fix the other sysfs tunables in the following patches.
>>>
>>> List a new solution for the similar cases below, please see if this
>>> design is reasonable?
>>>
>>> 1. Two SysFS files("dsb_edge_ctrl_idx" and "dsb_edge_ctrl_val") will be
>>> created in this case.
>>>
>>> 2. First write to the node "dsb_edge_ctrl_idx" to set the index number
>>> of the edge detection.
>>>
>>> 3. Then write to the node "dsb_edge_ctrl_val" to set the value of the
>>> edge detection.
>>>
>>> For example, if we need need to set "Falling edge detection" to the 
>>> edge
>>> detection #220-#222, we can issue the following commands.
>>>
>>> echo 0xdc > tpdm1/dsb_edge_ctrl_idx
>>>
>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>
>>> echo 0xdd > tpdm1/dsb_edge_ctrl_idx
>>>
>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>
>>> echo 0xde > tpdm1/dsb_edge_ctrl_idx
>>>
>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>
>>> If this design is acceptable, we will rewrite other similar nodes based
>>> on this solution.
>>>
>>
>> This index / value model is used in the coresight drivers so should be
>> OK - eg etm4 has cntr_idx / cntrldvr / cntr_val / cntr_ctrl, where
>> index selects the counter, and the other val registers are applied to
>> that counter.
>
> True. That model is useful when there are variable number of "counters".
> I guess it doesn't hurt to have a 64bit (or even 32bit) file for each
> EDCR.
>
> e.g, edcr0...edcr15
>
> Given there are only 16 of them, it is fine to keep a file for each.
> This may be grouped under "mgmt" similar to what we have for other
> components. That way, it can be easily hidden by checking for the
> presence of DSB.

The number of EDCR registers is not fixed. The maximum range is [0:15].

But the address of the maximum number of the registers will be reserved 
first,

and can be accessed safely even if the TPDM doesn't have the maximum number

of  EDCR registers.

And we are not able to dynamically know the number of EDCR registers per DSB

TPDM.

Can we use our proposal in this case?


Best,

Tao

>
> Suzuki
>
Suzuki K Poulose July 13, 2023, 4:37 p.m. UTC | #10 
On 13/07/2023 17:13, Tao Zhang wrote:
> 
> On 7/13/2023 5:34 PM, Suzuki K Poulose wrote:
>> On 13/07/2023 09:54, Mike Leach wrote:
>>> HI Tao,
>>>
>>> On Wed, 12 Jul 2023 at 14:53, Tao Zhang <quic_taozha@quicinc.com> wrote:
>>>>
>>>>
>>>> On 6/20/2023 9:41 PM, Suzuki K Poulose wrote:
>>>>> On 20/06/2023 09:31, Tao Zhang wrote:
>>>>>>
>>>>>> On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
>>>>>>> On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
>>>>>>>> Add the nodes to set value for DSB edge control and DSB edge
>>>>>>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>>>>>>>> resgisters to configure edge control. DSB edge detection control
>>>>>>>> 00: Rising edge detection
>>>>>>>> 01: Falling edge detection
>>>>>>>> 10: Rising and falling edge detection (toggle detection)
>>>>>>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
>>>>>>>> configure mask. Eight 32 bit registers providing DSB interface
>>>>>>>> edge detection mask control.
>>>>>>>>
>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>>> ---
>>>>>>>>    .../ABI/testing/sysfs-bus-coresight-devices-tpdm |  32 +++++
>>>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.c | 143
>>>>>>>> ++++++++++++++++++++-
>>>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.h |  22 ++++
>>>>>>>>    3 files changed, 196 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> index 2a82cd0..34189e4a 100644
>>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> @@ -60,3 +60,35 @@ Description:
>>>>>>>>            Bit[3] : Set to 0 for low performance mode.
>>>>>>>>                     Set to 1 for high performance mode.
>>>>>>>>            Bit[4:8] : Select byte lane for high performance mode.
>>>>>>>> +
>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>>>>>>>> +Date:        March 2023
>>>>>>>> +KernelVersion    6.5
>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao
>>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>> +Description:
>>>>>>>> +        Read/Write a set of the edge control registers of the DSB
>>>>>>>> +        in TPDM.
>>>>>>>> +
>>>>>>>> +        Expected format is the following:
>>>>>>>> +        <integer1> <integer2> <integer3>
>>>>>>> sysfs is "one value", not 3.  Please never have to parse a sysfs 
>>>>>>> file.
>>>>>>
>>>>>> Do you mean sysfs file can only accept "one value"?
>>>>>>
>>>>>> I see that more than one value are written to the sysfs file
>>>>>> "trigout_attach".
>>>>>>
>>>>>>>
>>>>>>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>>>>>>>> +                       struct device_attribute *attr,
>>>>>>>> +                       char *buf)
>>>>>>>> +{
>>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>>> +    ssize_t size = 0;
>>>>>>>> +    unsigned long bytes;
>>>>>>>> +    int i;
>>>>>>>> +
>>>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>>>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>>>>>>>> +        bytes = sysfs_emit_at(buf, size,
>>>>>>>> +                  "Index:0x%x Val:0x%x\n", i,
>>>>>>> Again, no, one value, no "string" needed to parse anything.
>>>>>>
>>>>>> I also see other sysfs files can be read more than one value in other
>>>>>> drivers.
>>>>>>
>>>>>> Is this "one value" limitation the usage rule of Linux sysfs system?
>>>>>>
>>>>>> Or am I misunderstanding what you mean?
>>>>>
>>>>> Please fix the other sysfs tunables in the following patches.
>>>>
>>>> List a new solution for the similar cases below, please see if this
>>>> design is reasonable?
>>>>
>>>> 1. Two SysFS files("dsb_edge_ctrl_idx" and "dsb_edge_ctrl_val") will be
>>>> created in this case.
>>>>
>>>> 2. First write to the node "dsb_edge_ctrl_idx" to set the index number
>>>> of the edge detection.
>>>>
>>>> 3. Then write to the node "dsb_edge_ctrl_val" to set the value of the
>>>> edge detection.
>>>>
>>>> For example, if we need need to set "Falling edge detection" to the 
>>>> edge
>>>> detection #220-#222, we can issue the following commands.
>>>>
>>>> echo 0xdc > tpdm1/dsb_edge_ctrl_idx
>>>>
>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>
>>>> echo 0xdd > tpdm1/dsb_edge_ctrl_idx
>>>>
>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>
>>>> echo 0xde > tpdm1/dsb_edge_ctrl_idx
>>>>
>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>
>>>> If this design is acceptable, we will rewrite other similar nodes based
>>>> on this solution.
>>>>
>>>
>>> This index / value model is used in the coresight drivers so should be
>>> OK - eg etm4 has cntr_idx / cntrldvr / cntr_val / cntr_ctrl, where
>>> index selects the counter, and the other val registers are applied to
>>> that counter.
>>
>> True. That model is useful when there are variable number of "counters".
>> I guess it doesn't hurt to have a 64bit (or even 32bit) file for each
>> EDCR.
>>
>> e.g, edcr0...edcr15
>>
>> Given there are only 16 of them, it is fine to keep a file for each.
>> This may be grouped under "mgmt" similar to what we have for other
>> components. That way, it can be easily hidden by checking for the
>> presence of DSB.
> 
> The number of EDCR registers is not fixed. The maximum range is [0:15].
> 
> But the address of the maximum number of the registers will be reserved 
> first,
> 
> and can be accessed safely even if the TPDM doesn't have the maximum number
> 
> of  EDCR registers.
> 
> And we are not able to dynamically know the number of EDCR registers per 
> DSB
> 
> TPDM.
> 
> Can we use our proposal in this case?

Please provide a file edcrN for each of the 0 <= N < 16. That way it is
easier to avoid locking the index. It doesn't matter how many EDCRs are
supported, there is a maximum limit and it is always guaranteed to be
write safe, if some are not implemented. Thus it is much easier from a 
programming perspective too.

Suzuki



> 
> 
> Best,
> 
> Tao
> 
>>
>> Suzuki
>>
Tao Zhang July 14, 2023, 5:50 a.m. UTC | #11 
On 7/14/2023 12:37 AM, Suzuki K Poulose wrote:
> On 13/07/2023 17:13, Tao Zhang wrote:
>>
>> On 7/13/2023 5:34 PM, Suzuki K Poulose wrote:
>>> On 13/07/2023 09:54, Mike Leach wrote:
>>>> HI Tao,
>>>>
>>>> On Wed, 12 Jul 2023 at 14:53, Tao Zhang <quic_taozha@quicinc.com> 
>>>> wrote:
>>>>>
>>>>>
>>>>> On 6/20/2023 9:41 PM, Suzuki K Poulose wrote:
>>>>>> On 20/06/2023 09:31, Tao Zhang wrote:
>>>>>>>
>>>>>>> On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
>>>>>>>> On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
>>>>>>>>> Add the nodes to set value for DSB edge control and DSB edge
>>>>>>>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>>>>>>>>> resgisters to configure edge control. DSB edge detection control
>>>>>>>>> 00: Rising edge detection
>>>>>>>>> 01: Falling edge detection
>>>>>>>>> 10: Rising and falling edge detection (toggle detection)
>>>>>>>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR 
>>>>>>>>> registers to
>>>>>>>>> configure mask. Eight 32 bit registers providing DSB interface
>>>>>>>>> edge detection mask control.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>>>> ---
>>>>>>>>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm |  32 +++++
>>>>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.c | 143
>>>>>>>>> ++++++++++++++++++++-
>>>>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.h | 22 ++++
>>>>>>>>>    3 files changed, 196 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> index 2a82cd0..34189e4a 100644
>>>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>> @@ -60,3 +60,35 @@ Description:
>>>>>>>>>            Bit[3] : Set to 0 for low performance mode.
>>>>>>>>>                     Set to 1 for high performance mode.
>>>>>>>>>            Bit[4:8] : Select byte lane for high performance mode.
>>>>>>>>> +
>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>>>>>>>>> +Date:        March 2023
>>>>>>>>> +KernelVersion    6.5
>>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao
>>>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>> +Description:
>>>>>>>>> +        Read/Write a set of the edge control registers of the 
>>>>>>>>> DSB
>>>>>>>>> +        in TPDM.
>>>>>>>>> +
>>>>>>>>> +        Expected format is the following:
>>>>>>>>> +        <integer1> <integer2> <integer3>
>>>>>>>> sysfs is "one value", not 3.  Please never have to parse a 
>>>>>>>> sysfs file.
>>>>>>>
>>>>>>> Do you mean sysfs file can only accept "one value"?
>>>>>>>
>>>>>>> I see that more than one value are written to the sysfs file
>>>>>>> "trigout_attach".
>>>>>>>
>>>>>>>>
>>>>>>>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>>>>>>>>> +                       struct device_attribute *attr,
>>>>>>>>> +                       char *buf)
>>>>>>>>> +{
>>>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>>>> +    ssize_t size = 0;
>>>>>>>>> +    unsigned long bytes;
>>>>>>>>> +    int i;
>>>>>>>>> +
>>>>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>>>>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>>>>>>>>> +        bytes = sysfs_emit_at(buf, size,
>>>>>>>>> +                  "Index:0x%x Val:0x%x\n", i,
>>>>>>>> Again, no, one value, no "string" needed to parse anything.
>>>>>>>
>>>>>>> I also see other sysfs files can be read more than one value in 
>>>>>>> other
>>>>>>> drivers.
>>>>>>>
>>>>>>> Is this "one value" limitation the usage rule of Linux sysfs 
>>>>>>> system?
>>>>>>>
>>>>>>> Or am I misunderstanding what you mean?
>>>>>>
>>>>>> Please fix the other sysfs tunables in the following patches.
>>>>>
>>>>> List a new solution for the similar cases below, please see if this
>>>>> design is reasonable?
>>>>>
>>>>> 1. Two SysFS files("dsb_edge_ctrl_idx" and "dsb_edge_ctrl_val") 
>>>>> will be
>>>>> created in this case.
>>>>>
>>>>> 2. First write to the node "dsb_edge_ctrl_idx" to set the index 
>>>>> number
>>>>> of the edge detection.
>>>>>
>>>>> 3. Then write to the node "dsb_edge_ctrl_val" to set the value of the
>>>>> edge detection.
>>>>>
>>>>> For example, if we need need to set "Falling edge detection" to 
>>>>> the edge
>>>>> detection #220-#222, we can issue the following commands.
>>>>>
>>>>> echo 0xdc > tpdm1/dsb_edge_ctrl_idx
>>>>>
>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>
>>>>> echo 0xdd > tpdm1/dsb_edge_ctrl_idx
>>>>>
>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>
>>>>> echo 0xde > tpdm1/dsb_edge_ctrl_idx
>>>>>
>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>
>>>>> If this design is acceptable, we will rewrite other similar nodes 
>>>>> based
>>>>> on this solution.
>>>>>
>>>>
>>>> This index / value model is used in the coresight drivers so should be
>>>> OK - eg etm4 has cntr_idx / cntrldvr / cntr_val / cntr_ctrl, where
>>>> index selects the counter, and the other val registers are applied to
>>>> that counter.
>>>
>>> True. That model is useful when there are variable number of 
>>> "counters".
>>> I guess it doesn't hurt to have a 64bit (or even 32bit) file for each
>>> EDCR.
>>>
>>> e.g, edcr0...edcr15
>>>
>>> Given there are only 16 of them, it is fine to keep a file for each.
>>> This may be grouped under "mgmt" similar to what we have for other
>>> components. That way, it can be easily hidden by checking for the
>>> presence of DSB.
>>
>> The number of EDCR registers is not fixed. The maximum range is [0:15].
>>
>> But the address of the maximum number of the registers will be 
>> reserved first,
>>
>> and can be accessed safely even if the TPDM doesn't have the maximum 
>> number
>>
>> of  EDCR registers.
>>
>> And we are not able to dynamically know the number of EDCR registers 
>> per DSB
>>
>> TPDM.
>>
>> Can we use our proposal in this case?
>
> Please provide a file edcrN for each of the 0 <= N < 16. That way it is
> easier to avoid locking the index. It doesn't matter how many EDCRs are
> supported, there is a maximum limit and it is always guaranteed to be
> write safe, if some are not implemented. Thus it is much easier from a 
> programming perspective too.

Hi Suzuki,


Thanks for the suggestion.

I'd like to further clarify our proposal below in case I didn't express 
it clearly before.

1. In our design, the users don't need to know the mapping between the 
number of the edge detection

and the control bits in EDCRN registers. They only need to focus on the 
edge detection they need, don't

need to care about the design of the HW.

2. For example, if there are two users configure in the same test. One 
needs to configure edge detection #7

as "Falling edge detection". The other one needs to configure edge 
detection #8 as "toggle detection". They will

issue the following commands to implement it.

echo 0x7 > tpdm1/dsb_edge_ctrl_idx

echo 0x1 > tpdm1/dsb_edge_ctrl_val

echo 0x8 > tpdm1/dsb_edge_ctrl_idx

echo 0x2 > tpdm1/dsb_edge_ctrl_val

The value written to edcr0 will be 0x24000 in our proposal.

But in the solution of "tpdm1/dsb_edge_ctrl/edcrN 0 <= N < 16".

One user calculates that he needs to write 0x4000 to edcr0.

echo 0x4000 > tpdm1/dsb_edge_ctrl/edcr0

The other one calculates that he needs to write 0x20000 to edcr0.

echo 0x20000 > tpdm1/dsb_edge_ctrl/edcr0

The last write will overwrite the previous value in this case and 0x20000

will be written to the edcr0 finally.

3. Some DSB TPDMs may not have 16 EDCR registers. For example, TPDM2

may only have 7 EDCR registers. If we still create 16 edcr file at 
tpdm2/dsb_edge_ctrl,

this may confuse users.

Based on the above points, is it possible to re-evaluate our proposal?


Best,

Tao

>
> Suzuki
>
>
>
>>
>>
>> Best,
>>
>> Tao
>>
>>>
>>> Suzuki
>>>
>
Suzuki K Poulose July 14, 2023, 10:24 a.m. UTC | #12 
On 14/07/2023 06:50, Tao Zhang wrote:
> 
> On 7/14/2023 12:37 AM, Suzuki K Poulose wrote:
>> On 13/07/2023 17:13, Tao Zhang wrote:
>>>
>>> On 7/13/2023 5:34 PM, Suzuki K Poulose wrote:
>>>> On 13/07/2023 09:54, Mike Leach wrote:
>>>>> HI Tao,
>>>>>
>>>>> On Wed, 12 Jul 2023 at 14:53, Tao Zhang <quic_taozha@quicinc.com> 
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 6/20/2023 9:41 PM, Suzuki K Poulose wrote:
>>>>>>> On 20/06/2023 09:31, Tao Zhang wrote:
>>>>>>>>
>>>>>>>> On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
>>>>>>>>> On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
>>>>>>>>>> Add the nodes to set value for DSB edge control and DSB edge
>>>>>>>>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>>>>>>>>>> resgisters to configure edge control. DSB edge detection control
>>>>>>>>>> 00: Rising edge detection
>>>>>>>>>> 01: Falling edge detection
>>>>>>>>>> 10: Rising and falling edge detection (toggle detection)
>>>>>>>>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR 
>>>>>>>>>> registers to
>>>>>>>>>> configure mask. Eight 32 bit registers providing DSB interface
>>>>>>>>>> edge detection mask control.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>>>>> ---
>>>>>>>>>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm |  32 +++++
>>>>>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.c | 143
>>>>>>>>>> ++++++++++++++++++++-
>>>>>>>>>>    drivers/hwtracing/coresight/coresight-tpdm.h | 22 ++++
>>>>>>>>>>    3 files changed, 196 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git
>>>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>>> index 2a82cd0..34189e4a 100644
>>>>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>>> @@ -60,3 +60,35 @@ Description:
>>>>>>>>>>            Bit[3] : Set to 0 for low performance mode.
>>>>>>>>>>                     Set to 1 for high performance mode.
>>>>>>>>>>            Bit[4:8] : Select byte lane for high performance mode.
>>>>>>>>>> +
>>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>>>>>>>>>> +Date:        March 2023
>>>>>>>>>> +KernelVersion    6.5
>>>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao
>>>>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>>> +Description:
>>>>>>>>>> +        Read/Write a set of the edge control registers of the 
>>>>>>>>>> DSB
>>>>>>>>>> +        in TPDM.
>>>>>>>>>> +
>>>>>>>>>> +        Expected format is the following:
>>>>>>>>>> +        <integer1> <integer2> <integer3>
>>>>>>>>> sysfs is "one value", not 3.  Please never have to parse a 
>>>>>>>>> sysfs file.
>>>>>>>>
>>>>>>>> Do you mean sysfs file can only accept "one value"?
>>>>>>>>
>>>>>>>> I see that more than one value are written to the sysfs file
>>>>>>>> "trigout_attach".
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>>>>>>>>>> +                       struct device_attribute *attr,
>>>>>>>>>> +                       char *buf)
>>>>>>>>>> +{
>>>>>>>>>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>>>>>> +    ssize_t size = 0;
>>>>>>>>>> +    unsigned long bytes;
>>>>>>>>>> +    int i;
>>>>>>>>>> +
>>>>>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>>>>>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>>>>>>>>>> +        bytes = sysfs_emit_at(buf, size,
>>>>>>>>>> +                  "Index:0x%x Val:0x%x\n", i,
>>>>>>>>> Again, no, one value, no "string" needed to parse anything.
>>>>>>>>
>>>>>>>> I also see other sysfs files can be read more than one value in 
>>>>>>>> other
>>>>>>>> drivers.
>>>>>>>>
>>>>>>>> Is this "one value" limitation the usage rule of Linux sysfs 
>>>>>>>> system?
>>>>>>>>
>>>>>>>> Or am I misunderstanding what you mean?
>>>>>>>
>>>>>>> Please fix the other sysfs tunables in the following patches.
>>>>>>
>>>>>> List a new solution for the similar cases below, please see if this
>>>>>> design is reasonable?
>>>>>>
>>>>>> 1. Two SysFS files("dsb_edge_ctrl_idx" and "dsb_edge_ctrl_val") 
>>>>>> will be
>>>>>> created in this case.
>>>>>>
>>>>>> 2. First write to the node "dsb_edge_ctrl_idx" to set the index 
>>>>>> number
>>>>>> of the edge detection.
>>>>>>
>>>>>> 3. Then write to the node "dsb_edge_ctrl_val" to set the value of the
>>>>>> edge detection.
>>>>>>
>>>>>> For example, if we need need to set "Falling edge detection" to 
>>>>>> the edge
>>>>>> detection #220-#222, we can issue the following commands.
>>>>>>
>>>>>> echo 0xdc > tpdm1/dsb_edge_ctrl_idx
>>>>>>
>>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>>
>>>>>> echo 0xdd > tpdm1/dsb_edge_ctrl_idx
>>>>>>
>>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>>
>>>>>> echo 0xde > tpdm1/dsb_edge_ctrl_idx
>>>>>>
>>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>>
>>>>>> If this design is acceptable, we will rewrite other similar nodes 
>>>>>> based
>>>>>> on this solution.
>>>>>>
>>>>>
>>>>> This index / value model is used in the coresight drivers so should be
>>>>> OK - eg etm4 has cntr_idx / cntrldvr / cntr_val / cntr_ctrl, where
>>>>> index selects the counter, and the other val registers are applied to
>>>>> that counter.
>>>>
>>>> True. That model is useful when there are variable number of 
>>>> "counters".
>>>> I guess it doesn't hurt to have a 64bit (or even 32bit) file for each
>>>> EDCR.
>>>>
>>>> e.g, edcr0...edcr15
>>>>
>>>> Given there are only 16 of them, it is fine to keep a file for each.
>>>> This may be grouped under "mgmt" similar to what we have for other
>>>> components. That way, it can be easily hidden by checking for the
>>>> presence of DSB.
>>>
>>> The number of EDCR registers is not fixed. The maximum range is [0:15].
>>>
>>> But the address of the maximum number of the registers will be 
>>> reserved first,
>>>
>>> and can be accessed safely even if the TPDM doesn't have the maximum 
>>> number
>>>
>>> of  EDCR registers.
>>>
>>> And we are not able to dynamically know the number of EDCR registers 
>>> per DSB
>>>
>>> TPDM.
>>>
>>> Can we use our proposal in this case?
>>
>> Please provide a file edcrN for each of the 0 <= N < 16. That way it is
>> easier to avoid locking the index. It doesn't matter how many EDCRs are
>> supported, there is a maximum limit and it is always guaranteed to be
>> write safe, if some are not implemented. Thus it is much easier from a 
>> programming perspective too.
> 
> Hi Suzuki,
> 
> 
> Thanks for the suggestion.
> 
> I'd like to further clarify our proposal below in case I didn't express 
> it clearly before.
> 
> 1. In our design, the users don't need to know the mapping between the 
> number of the edge detection
> 
> and the control bits in EDCRN registers. They only need to focus on the 
> edge detection they need, don't
> 
> need to care about the design of the HW.

Agreed

> 
> 2. For example, if there are two users configure in the same test. One 
> needs to configure edge detection #7
> 
> as "Falling edge detection". The other one needs to configure edge 
> detection #8 as "toggle detection". They will
> 
> issue the following commands to implement it.
> 
> echo 0x7 > tpdm1/dsb_edge_ctrl_idx
> 
> echo 0x1 > tpdm1/dsb_edge_ctrl_val
> 
> echo 0x8 > tpdm1/dsb_edge_ctrl_idx
> 
> echo 0x2 > tpdm1/dsb_edge_ctrl_val
> 
> The value written to edcr0 will be 0x24000 in our proposal.
> 
> But in the solution of "tpdm1/dsb_edge_ctrl/edcrN 0 <= N < 16".
> 
> One user calculates that he needs to write 0x4000 to edcr0.
> 
> echo 0x4000 > tpdm1/dsb_edge_ctrl/edcr0
> 
> The other one calculates that he needs to write 0x20000 to edcr0.
> 
> echo 0x20000 > tpdm1/dsb_edge_ctrl/edcr0
> 
> The last write will overwrite the previous value in this case and 0x20000
> 
> will be written to the edcr0 finally.

The solution of edcrN expects the users follow a Read-Modify-Write.
But given you want to control individual lines separately (which are 256
in number), I am fine with the _idx/value solution.

> 
> 3. Some DSB TPDMs may not have 16 EDCR registers. For example, TPDM2
> 
> may only have 7 EDCR registers. If we still create 16 edcr file at 
> tpdm2/dsb_edge_ctrl,
> 
> this may confuse users.

This is not relevant. The user can't know the maximum number anyway. If 
the user knows TPDM2 has only 7 EDCR, then don't bother about the other
files.

Please go ahead with the _idx /_value

Suzuki
Tao Zhang July 14, 2023, 2:39 p.m. UTC | #13 
On 7/14/2023 6:24 PM, Suzuki K Poulose wrote:
> On 14/07/2023 06:50, Tao Zhang wrote:
>>
>> On 7/14/2023 12:37 AM, Suzuki K Poulose wrote:
>>> On 13/07/2023 17:13, Tao Zhang wrote:
>>>>
>>>> On 7/13/2023 5:34 PM, Suzuki K Poulose wrote:
>>>>> On 13/07/2023 09:54, Mike Leach wrote:
>>>>>> HI Tao,
>>>>>>
>>>>>> On Wed, 12 Jul 2023 at 14:53, Tao Zhang <quic_taozha@quicinc.com> 
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 6/20/2023 9:41 PM, Suzuki K Poulose wrote:
>>>>>>>> On 20/06/2023 09:31, Tao Zhang wrote:
>>>>>>>>>
>>>>>>>>> On 6/20/2023 3:37 PM, Greg Kroah-Hartman wrote:
>>>>>>>>>> On Tue, Jun 20, 2023 at 03:32:37PM +0800, Tao Zhang wrote:
>>>>>>>>>>> Add the nodes to set value for DSB edge control and DSB edge
>>>>>>>>>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>>>>>>>>>>> resgisters to configure edge control. DSB edge detection 
>>>>>>>>>>> control
>>>>>>>>>>> 00: Rising edge detection
>>>>>>>>>>> 01: Falling edge detection
>>>>>>>>>>> 10: Rising and falling edge detection (toggle detection)
>>>>>>>>>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR 
>>>>>>>>>>> registers to
>>>>>>>>>>> configure mask. Eight 32 bit registers providing DSB interface
>>>>>>>>>>> edge detection mask control.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>>>>>>>>>> ---
>>>>>>>>>>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm |  32 +++++
>>>>>>>>>>> drivers/hwtracing/coresight/coresight-tpdm.c | 143
>>>>>>>>>>> ++++++++++++++++++++-
>>>>>>>>>>> drivers/hwtracing/coresight/coresight-tpdm.h | 22 ++++
>>>>>>>>>>>    3 files changed, 196 insertions(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git
>>>>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>>>> index 2a82cd0..34189e4a 100644
>>>>>>>>>>> --- 
>>>>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>>>> +++ 
>>>>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>>>>> @@ -60,3 +60,35 @@ Description:
>>>>>>>>>>>            Bit[3] : Set to 0 for low performance mode.
>>>>>>>>>>>                     Set to 1 for high performance mode.
>>>>>>>>>>>            Bit[4:8] : Select byte lane for high performance 
>>>>>>>>>>> mode.
>>>>>>>>>>> +
>>>>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>>>>>>>>>>> +Date:        March 2023
>>>>>>>>>>> +KernelVersion    6.5
>>>>>>>>>>> +Contact:    Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao
>>>>>>>>>>> Zhang (QUIC) <quic_taozha@quicinc.com>
>>>>>>>>>>> +Description:
>>>>>>>>>>> +        Read/Write a set of the edge control registers of 
>>>>>>>>>>> the DSB
>>>>>>>>>>> +        in TPDM.
>>>>>>>>>>> +
>>>>>>>>>>> +        Expected format is the following:
>>>>>>>>>>> +        <integer1> <integer2> <integer3>
>>>>>>>>>> sysfs is "one value", not 3.  Please never have to parse a 
>>>>>>>>>> sysfs file.
>>>>>>>>>
>>>>>>>>> Do you mean sysfs file can only accept "one value"?
>>>>>>>>>
>>>>>>>>> I see that more than one value are written to the sysfs file
>>>>>>>>> "trigout_attach".
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>>>>>>>>>>> +                       struct device_attribute *attr,
>>>>>>>>>>> +                       char *buf)
>>>>>>>>>>> +{
>>>>>>>>>>> +    struct tpdm_drvdata *drvdata = 
>>>>>>>>>>> dev_get_drvdata(dev->parent);
>>>>>>>>>>> +    ssize_t size = 0;
>>>>>>>>>>> +    unsigned long bytes;
>>>>>>>>>>> +    int i;
>>>>>>>>>>> +
>>>>>>>>>>> +    spin_lock(&drvdata->spinlock);
>>>>>>>>>>> +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>>>>>>>>>>> +        bytes = sysfs_emit_at(buf, size,
>>>>>>>>>>> +                  "Index:0x%x Val:0x%x\n", i,
>>>>>>>>>> Again, no, one value, no "string" needed to parse anything.
>>>>>>>>>
>>>>>>>>> I also see other sysfs files can be read more than one value 
>>>>>>>>> in other
>>>>>>>>> drivers.
>>>>>>>>>
>>>>>>>>> Is this "one value" limitation the usage rule of Linux sysfs 
>>>>>>>>> system?
>>>>>>>>>
>>>>>>>>> Or am I misunderstanding what you mean?
>>>>>>>>
>>>>>>>> Please fix the other sysfs tunables in the following patches.
>>>>>>>
>>>>>>> List a new solution for the similar cases below, please see if this
>>>>>>> design is reasonable?
>>>>>>>
>>>>>>> 1. Two SysFS files("dsb_edge_ctrl_idx" and "dsb_edge_ctrl_val") 
>>>>>>> will be
>>>>>>> created in this case.
>>>>>>>
>>>>>>> 2. First write to the node "dsb_edge_ctrl_idx" to set the index 
>>>>>>> number
>>>>>>> of the edge detection.
>>>>>>>
>>>>>>> 3. Then write to the node "dsb_edge_ctrl_val" to set the value 
>>>>>>> of the
>>>>>>> edge detection.
>>>>>>>
>>>>>>> For example, if we need need to set "Falling edge detection" to 
>>>>>>> the edge
>>>>>>> detection #220-#222, we can issue the following commands.
>>>>>>>
>>>>>>> echo 0xdc > tpdm1/dsb_edge_ctrl_idx
>>>>>>>
>>>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>>>
>>>>>>> echo 0xdd > tpdm1/dsb_edge_ctrl_idx
>>>>>>>
>>>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>>>
>>>>>>> echo 0xde > tpdm1/dsb_edge_ctrl_idx
>>>>>>>
>>>>>>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>>>>>>
>>>>>>> If this design is acceptable, we will rewrite other similar 
>>>>>>> nodes based
>>>>>>> on this solution.
>>>>>>>
>>>>>>
>>>>>> This index / value model is used in the coresight drivers so 
>>>>>> should be
>>>>>> OK - eg etm4 has cntr_idx / cntrldvr / cntr_val / cntr_ctrl, where
>>>>>> index selects the counter, and the other val registers are 
>>>>>> applied to
>>>>>> that counter.
>>>>>
>>>>> True. That model is useful when there are variable number of 
>>>>> "counters".
>>>>> I guess it doesn't hurt to have a 64bit (or even 32bit) file for each
>>>>> EDCR.
>>>>>
>>>>> e.g, edcr0...edcr15
>>>>>
>>>>> Given there are only 16 of them, it is fine to keep a file for each.
>>>>> This may be grouped under "mgmt" similar to what we have for other
>>>>> components. That way, it can be easily hidden by checking for the
>>>>> presence of DSB.
>>>>
>>>> The number of EDCR registers is not fixed. The maximum range is 
>>>> [0:15].
>>>>
>>>> But the address of the maximum number of the registers will be 
>>>> reserved first,
>>>>
>>>> and can be accessed safely even if the TPDM doesn't have the 
>>>> maximum number
>>>>
>>>> of  EDCR registers.
>>>>
>>>> And we are not able to dynamically know the number of EDCR 
>>>> registers per DSB
>>>>
>>>> TPDM.
>>>>
>>>> Can we use our proposal in this case?
>>>
>>> Please provide a file edcrN for each of the 0 <= N < 16. That way it is
>>> easier to avoid locking the index. It doesn't matter how many EDCRs are
>>> supported, there is a maximum limit and it is always guaranteed to be
>>> write safe, if some are not implemented. Thus it is much easier from 
>>> a programming perspective too.
>>
>> Hi Suzuki,
>>
>>
>> Thanks for the suggestion.
>>
>> I'd like to further clarify our proposal below in case I didn't 
>> express it clearly before.
>>
>> 1. In our design, the users don't need to know the mapping between 
>> the number of the edge detection
>>
>> and the control bits in EDCRN registers. They only need to focus on 
>> the edge detection they need, don't
>>
>> need to care about the design of the HW.
>
> Agreed
>
>>
>> 2. For example, if there are two users configure in the same test. 
>> One needs to configure edge detection #7
>>
>> as "Falling edge detection". The other one needs to configure edge 
>> detection #8 as "toggle detection". They will
>>
>> issue the following commands to implement it.
>>
>> echo 0x7 > tpdm1/dsb_edge_ctrl_idx
>>
>> echo 0x1 > tpdm1/dsb_edge_ctrl_val
>>
>> echo 0x8 > tpdm1/dsb_edge_ctrl_idx
>>
>> echo 0x2 > tpdm1/dsb_edge_ctrl_val
>>
>> The value written to edcr0 will be 0x24000 in our proposal.
>>
>> But in the solution of "tpdm1/dsb_edge_ctrl/edcrN 0 <= N < 16".
>>
>> One user calculates that he needs to write 0x4000 to edcr0.
>>
>> echo 0x4000 > tpdm1/dsb_edge_ctrl/edcr0
>>
>> The other one calculates that he needs to write 0x20000 to edcr0.
>>
>> echo 0x20000 > tpdm1/dsb_edge_ctrl/edcr0
>>
>> The last write will overwrite the previous value in this case and 
>> 0x20000
>>
>> will be written to the edcr0 finally.
>
> The solution of edcrN expects the users follow a Read-Modify-Write.
> But given you want to control individual lines separately (which are 256
> in number), I am fine with the _idx/value solution.
>
>>
>> 3. Some DSB TPDMs may not have 16 EDCR registers. For example, TPDM2
>>
>> may only have 7 EDCR registers. If we still create 16 edcr file at 
>> tpdm2/dsb_edge_ctrl,
>>
>> this may confuse users.
>
> This is not relevant. The user can't know the maximum number anyway. 
> If the user knows TPDM2 has only 7 EDCR, then don't bother about the 
> other
> files.
>
> Please go ahead with the _idx /_value

Thanks. I will update in the next patch series.


Best,

Tao

>
> Suzuki
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index 2a82cd0..34189e4a 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -60,3 +60,35 @@  Description:
 		Bit[3] : Set to 0 for low performance mode.
 				 Set to 1 for high performance mode.
 		Bit[4:8] : Select byte lane for high performance mode.
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
+Date:		March 2023
+KernelVersion	6.5
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		Read/Write a set of the edge control registers of the DSB
+		in TPDM.
+
+		Expected format is the following:
+		<integer1> <integer2> <integer3>
+
+		Where:
+		<integer1> : Start EDCR register number
+		<integer2> : End EDCR register number
+		<integer3> : The value need to be written
+
+What:		/sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask
+Date:		March 2023
+KernelVersion	6.5
+Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
+Description:
+		Read/Write a set of the edge control mask registers of the
+		DSB in TPDM.
+
+		Expected format is the following:
+		<integer1> <integer2> <integer3>
+
+		Where:
+		<integer1> : Start EDCMR register number
+		<integer2> : End EDCMR register number
+		<integer3> : The value need to be written
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
index c38760b..fc92900 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.c
+++ b/drivers/hwtracing/coresight/coresight-tpdm.c
@@ -71,7 +71,14 @@  static void set_dsb_perf_mode(struct tpdm_drvdata *drvdata, u32 *val)
 
 static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
 {
-	u32 val;
+	u32 val, i;
+
+	for (i = 0; i < TPDM_DSB_MAX_EDCR; i++)
+		writel_relaxed(drvdata->dsb->edge_ctrl[i],
+			   drvdata->base + TPDM_DSB_EDCR(i));
+	for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++)
+		writel_relaxed(drvdata->dsb->edge_ctrl_mask[i],
+			   drvdata->base + TPDM_DSB_EDCMR(i));
 
 	val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
 	/* Set trigger timestamp */
@@ -302,6 +309,138 @@  static ssize_t dsb_mode_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(dsb_mode);
 
+static ssize_t dsb_edge_ctrl_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
+	unsigned long bytes;
+	int i;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
+		bytes = sysfs_emit_at(buf, size,
+				  "Index:0x%x Val:0x%x\n", i,
+				  drvdata->dsb->edge_ctrl[i]);
+		if (bytes <= 0)
+			break;
+		size += bytes;
+	}
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+
+/*
+ * value 1: Start EDCR register number
+ * value 2: End EDCR register number
+ * value 3: The value need to be written
+ * The EDCR registers can include up to 16 32-bit registers, and each
+ * one can be configured to control up to 16 edge detections(2 bits
+ * control one edge detection). So a total 256 edge detections can be
+ * configured. So the starting number(value 1) and ending number(value 2)
+ * cannot be greater than 256, and value 1 should be less than value 2.
+ * The following values are the rage of value 3.
+ * 0 - Rising edge detection
+ * 1 - Falling edge detection
+ * 2 - Rising and falling edge detection (toggle detection)
+ */
+static ssize_t dsb_edge_ctrl_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf,
+					size_t size)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val, mask, start, end, edge_ctrl;
+	int i, reg;
+
+	if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3)
+		return -EINVAL;
+	if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) ||
+	    (start > end) || (edge_ctrl > 0x2))
+		return -EPERM;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = start; i <= end; i++) {
+		/*
+		 * There are 2 bit per DSB Edge Control line.
+		 * Thus we have 16 lines in a 32bit word.
+		 */
+		reg = EDCR_TO_WORD_IDX(i);
+		mask = EDCR_TO_WORD_MASK(i);
+		val = drvdata->dsb->edge_ctrl[reg];
+		val &= ~EDCR_TO_WORD_MASK(i);
+		val |= EDCR_TO_WORD_VAL(edge_ctrl, i);
+		drvdata->dsb->edge_ctrl[reg] = val;
+	}
+	spin_unlock(&drvdata->spinlock);
+
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_edge_ctrl);
+
+static ssize_t dsb_edge_ctrl_mask_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
+	unsigned long bytes;
+	int i;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) {
+		bytes = sysfs_emit_at(buf, size,
+				  "Index:0x%x Val:0x%x\n", i,
+				  drvdata->dsb->edge_ctrl_mask[i]);
+		if (bytes <= 0)
+			break;
+		size += bytes;
+	}
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+
+/*
+ * value 1: Start EDCMR register number
+ * value 2: End EDCMR register number
+ * value 3: The value need to be written
+ */
+static ssize_t dsb_edge_ctrl_mask_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf,
+					     size_t size)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long start, end, val;
+	u32 set;
+	int i, reg;
+
+	if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3)
+		return -EINVAL;
+	if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES)
+		|| (start > end) || (val & ~1UL))
+		return -EPERM;
+
+	spin_lock(&drvdata->spinlock);
+	for (i = start; i <= end; i++) {
+		/*
+		 * There is 1 bit per DSB Edge Control Mark line.
+		 * Thus we have 32 lines in a 32bit word.
+		 */
+		reg = EDCMR_TO_WORD_IDX(i);
+		set = drvdata->dsb->edge_ctrl_mask[reg];
+		if (val)
+			set |= BIT(EDCR_TO_WORD_SHIFT(i));
+		else
+			set &= ~BIT(EDCR_TO_WORD_SHIFT(i));
+		drvdata->dsb->edge_ctrl_mask[reg] = set;
+	}
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(dsb_edge_ctrl_mask);
+
 static ssize_t dsb_trig_type_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
@@ -374,6 +513,8 @@  static DEVICE_ATTR_RW(dsb_trig_ts);
 
 static struct attribute *tpdm_dsb_attrs[] = {
 	&dev_attr_dsb_mode.attr,
+	&dev_attr_dsb_edge_ctrl.attr,
+	&dev_attr_dsb_edge_ctrl_mask.attr,
 	&dev_attr_dsb_trig_ts.attr,
 	&dev_attr_dsb_trig_type.attr,
 	NULL,
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
index 49fffb1..f81bfe8 100644
--- a/drivers/hwtracing/coresight/coresight-tpdm.h
+++ b/drivers/hwtracing/coresight/coresight-tpdm.h
@@ -12,6 +12,8 @@ 
 /* DSB Subunit Registers */
 #define TPDM_DSB_CR		(0x780)
 #define TPDM_DSB_TIER		(0x784)
+#define TPDM_DSB_EDCR(n)	(0x808 + (n * 4))
+#define TPDM_DSB_EDCMR(n)	(0x848 + (n * 4))
 
 /* Enable bit for DSB subunit */
 #define TPDM_DSB_CR_ENA		BIT(0)
@@ -34,6 +36,16 @@ 
 #define TPDM_DSB_TEST_MODE		GENMASK(10, 9)
 #define TPDM_DSB_HPSEL		GENMASK(6, 2)
 
+#define EDCRS_PER_WORD				16
+#define EDCR_TO_WORD_IDX(r)			((r) / EDCRS_PER_WORD)
+#define EDCR_TO_WORD_SHIFT(r)		((r % EDCRS_PER_WORD) * 2)
+#define EDCR_TO_WORD_VAL(val, r)	(val << EDCR_TO_WORD_SHIFT(r))
+#define EDCR_TO_WORD_MASK(r)		EDCR_TO_WORD_VAL(0x3, r)
+
+#define EDCMRS_PER_WORD				32
+#define EDCMR_TO_WORD_IDX(r)		((r) / EDCMRS_PER_WORD)
+#define EDCMR_TO_WORD_SHIFT(r)		((r) % EDCMRS_PER_WORD)
+
 /* TPDM integration test registers */
 #define TPDM_ITATBCNTRL		(0xEF0)
 #define TPDM_ITCNTRL		(0xF00)
@@ -60,14 +72,24 @@ 
 #define TPDM_PIDR0_DS_IMPDEF	BIT(0)
 #define TPDM_PIDR0_DS_DSB	BIT(1)
 
+#define TPDM_DSB_MAX_LINES	256
+/* MAX number of EDCR registers */
+#define TPDM_DSB_MAX_EDCR	16
+/* MAX number of EDCMR registers */
+#define TPDM_DSB_MAX_EDCMR	8
+
 /**
  * struct dsb_dataset - specifics associated to dsb dataset
  * @mode:             DSB programming mode
+ * @edge_ctrl:        Save value for edge control
+ * @edge_ctrl_mask:   Save value for edge control mask
  * @trig_ts:          Enable/Disable trigger timestamp.
  * @trig_type:        Enable/Disable trigger type.
  */
 struct dsb_dataset {
 	u32				mode;
+	u32			edge_ctrl[TPDM_DSB_MAX_EDCR];
+	u32			edge_ctrl_mask[TPDM_DSB_MAX_EDCMR];
 	bool			trig_ts;
 	bool			trig_type;
 };