Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward

[Dmitry Osipenko]

On 3/30/22 14:22, Ashish Mhetre wrote:
...
>>> If we are to remove this callback then how to handle unknown interrupt
>>> channel error?
>>
>> Create a common helper function that returns ID of the raised channel or
>> errorno if not bits are set.
>>
> So something like this:
> 
> int status_to_channel(const struct tegra_mc *mc, u32 status,
>               unsigned int *mc_channel)
> {
>     if ((status & mc->soc->ch_intmask) == 0)
>         return -EINVAL;
> 
>     *mc_channel = __ffs((status & mc->soc->ch_intmask) >>
>                  mc->soc->status_reg_chan_shift);
> 
>     return 0;
> }
> 
> Correct?

Yes

>>> Also we want to handle interrupts on one channel at a time and then
>>> clear it from status register. There can be interrupts on multiple
>>> channel. So multiple bits from status will be set. Hence it will be
>>> hard to parameterize shift such that it gives appropriate channel.
>>> So I think current approach is fine. Please correct me if I am wrong
>>> somewhere.
>>
>> You may do the following:
>>
>> 1. find the first channel bit set in the status reg
>> 2. handle that channel
>> 3. clear only the handled status bit, don't clear the other bits
>> 4. return from interrupt
>>
>> If there are other bits set, then interrupt handler will fire again and
>> next channel will be handled.
> 
> For clearing status bit after handling, we can retrieve channel bit by
> something like this:
> 
> ch_bit = BIT(*mc_channel) << mc->soc->status_reg_chan_shift;
> 
> Correct?

Yes

Perhaps using FIELD_PREP() and alike helpers could make it look nice in
the code.