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

Ashish Mhetre
2022-03-31T21:55:22Z

On 4/1/2022 1:19 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 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.

I tried using FIELD_PREP() and FIELD_GET() for our use-case but
compilation is failing because these macros require the mask to be
compile time constant and our mask "mc->soc->ch_intmask" cannot qualify
to be compile time constant.