Messages in this thread | | | Date | Fri, 14 Jul 2023 11:24:29 +0100 | Subject | Re: [PATCH v6 09/13] Add nodes for dsb edge control | From | Suzuki K Poulose <> |
| |
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
| |