linux-kernel - Re: [PATCH v4 3/3] counter: capture-tiecap: capture driver support for ECAP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <21a66361-5180-65e1-f075-2163bf46af08@baylibre.com>
Date:   Tue, 16 Aug 2022 17:28:32 +0200
From:   Julien Panis <jpanis@...libre.com>
To:     William Breathitt Gray <william.gray@...aro.org>
Cc:     vilhelm.gray@...il.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        mranostay@...com
Subject: Re: [PATCH v4 3/3] counter: capture-tiecap: capture driver support
 for ECAP



On 16/08/2022 17:12, William Breathitt Gray wrote:
> On Tue, Aug 16, 2022 at 09:58:10AM +0200, Julien Panis wrote:
>> On 14/08/2022 19:03, William Breathitt Gray wrote:
>>> On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
>>>> +static int ecap_cnt_function_read(struct counter_device *counter,
>>>> +				  struct counter_count *count,
>>>> +				  enum counter_function *function)
>>>> +{
>>>> +	*function = COUNTER_FUNCTION_INCREASE;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int ecap_cnt_action_read(struct counter_device *counter,
>>>> +				struct counter_count *count,
>>>> +				struct counter_synapse *synapse,
>>>> +				enum counter_synapse_action *action)
>>>> +{
>>>> +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
>>>> +
>>>> +	return 0;
>>>> +}
>>> Right now you have a Signal defined for the ECAPSIG line, but there is
>>> at least one more relevant Signal to define: the clock updating ECAPCNT.
>>> The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
>>> clock Signal, but for the ECAPSIG Signal you will need to report a
>>> Synapse action based on the polarity of the next capture (i.e. whether
>>> high or low).
>> Just to be sure : by using the word ECAPCNT, I guess that you speak about
>> the
>> Mod4 counter (0->1->2->3->0...), don't you ? (2 bits)
>> Or do you speak about ECAP_TSCNT_REG register content ? (32 bits)
> Sorry for the confusion, I'm talking about ECAP_TSCNT_REG (32-bit) here.
> You should rename this Count in your ecap_cnt_counts array from
> "ECAPCNT" to "Time-Stamp Counter" to make it clearer to users as well;
> it would be prudent to rename "ECAPSIG" too.
>
> I didn't know that there was a register exposing the Mod4 counter value.
> If that's true, then define a Count for the Mod4 counter in your
> ecap_cnt_counts array.

There is no dedicated register for that, but it would be possible to 
expose this value
(when interruptions are triggered, we would just need to parse flags 
1/2/3/4 to determine
mod4 counter current state). That would not be very useful, though.

>
>>>> +static struct counter_comp ecap_cnt_count_ext[] = {
>>>> +	COUNTER_COMP_COUNT_U64("capture1", ecap_cnt_cap1_read, NULL),
>>>> +	COUNTER_COMP_COUNT_U64("capture2", ecap_cnt_cap2_read, NULL),
>>>> +	COUNTER_COMP_COUNT_U64("capture3", ecap_cnt_cap3_read, NULL),
>>>> +	COUNTER_COMP_COUNT_U64("capture4", ecap_cnt_cap4_read, NULL),
>>>> +	COUNTER_COMP_ENABLE(ecap_cnt_enable_read, ecap_cnt_enable_write),
>>> I just want to verify: this enable extension should disable the ECAPCNT
>>> count itself (i.e. no more increasing count value). Is that what's
>>> happening here, or is this meant to disable just the captures?
>> Yes, it is what's happening here : no more increasing count value.
> Okay that's good. By the way, COUNTER_COMP_ENABLE ensures the enable
> value passed to ecap_cnt_enable_write() is either 0 or 1, so you don't
> need the enable > 1 check in your callback.
>
>>>> +static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
>>>> +{
>>>> +	struct counter_device *counter_dev = dev_id;
>>>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
>>>> +	unsigned int clr = 0;
>>>> +	unsigned int flg;
>>>> +	int i;
>>>> +	unsigned long flags;
>>>> +
>>>> +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
>>>> +
>>>> +	for (i = ECAP_NB_CAP - 1 ; i < ECAP_NB_CEVT ; i++) {
>>> Would you walk me through the logic for this loop. Is this for-loop
>>> intended to loop through all four capture indices? ECAP_NB_CAP and
>>> ECAP_NB_CEVT are defines, so your for-loop has i=3 and i<5; is this what
>>> you want?
>> In previous versions (IIO subsys), this for-loop was intended to loop
>> through all 4 capture indices
>> and overflow flag.
>> In this version, it has been modified to loop only for the last capture
>> indice (the 4th)
>> and overflow flag : yes, this is intentional. Only 1 event has to be pushed
>> so that the user
>> gets all 4 captured timestamps in a single-reading (using 4 watches).
>> But if I understand well your previous suggestion, you would like tracking
>> Mod4 counter value,
>> don't you ? So, I will change back this for-loop, so that it loops for all
>> capture indices (and
>> overflow flag) -> For all 4 capture indices, Mod4 count will be updated. And
>> event will still be
>> pushed only for the 4th capture indice.
> Instead of limiting the event push to just the 4th capture, I'd push
> COUNTER_EVENT_CAPTURE on every capture but delegate them to their own
> channels::
>
>      counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, i);

I prefer using only 1 signal if you don't mind. I think it's less 
confusing for the user.

>
> The captures operate as a circular buffer, so the user can determine the
> current capture index based on the watch channel reported and perform a
> modulo to read the buffers in right sequence. Alternatively, they can
> watch just channel 3 if they want to process only four captures at a
> time.
>
> William Breathitt Gray