[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-tegra
Subject:    Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
From:       Ashish Mhetre <amhetre () nvidia ! com>
Date:       2022-03-31 21:55:02
Message-ID: 1d9e2055-ac22-334a-f003-911034ef75f4 () nvidia ! com
[Download RAW message or body]



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.
[prev in list] [next in list] [prev in thread] [next in thread]