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

[Ashish Mhetre <amhetre@nvidia.com>]

On 3/30/2022 4:06 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 3/30/22 13:16, Ashish Mhetre wrote:
>>
>>
>> On 3/30/2022 5:31 AM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 3/22/22 20:34, Ashish Mhetre wrote:
>>>>>> +     switch (status & mc->soc->int_channel_mask) {
>>>>>> +     case BIT(0):
>>>>>> +             *mc_channel = 0;
>>>>>> +             break;
>>>>>> +
>>>>>> +     case BIT(1):
>>>>>> +             *mc_channel = 1;
>>>>>> +             break;
>>>>>> +
>>>>>> +     case BIT(2):
>>>>>> +             *mc_channel = 2;
>>>>>> +             break;
>>>>>> +
>>>>>> +     case BIT(3):
>>>>>> +             *mc_channel = 3;
>>>>>> +             break;
>>>>>> +
>>>>>> +     case BIT(24):
>>>>>> +             *mc_channel = MC_BROADCAST_CHANNEL;
>>>>>> +             break;
>>>>>> +
>>>>>> +     default:
>>>>>> +             pr_err("Unknown interrupt source\n");
>>>>>
>>>>> dev_err_ratelimited("unknown interrupt channel 0x%08x\n", status) and
>>>>> should be moved to the common interrupt handler.
>>>>>
>>>> So return just error from default case and handle error in common
>>>> interrupt handler with this print, right? I'll update this in next
>>>> version.
>>>
>>> Yes, just move out the common print.
>>>
>>> Although, you could parameterize the shift per SoC and then have a
>>> common helper that does "status >> intmask_chan_shift", couldn't you?
>>
>> Do you mean shift to get the channel, like
>> "channel = status >> intmask_chan_shift"?
>> So to get rid of this callback completely and adding a variable in
>> tegra_mc_soc for intmask_chan_shift, right? Or compute shift in this
>> callback and use it in common handler?
> 
> Add variable to tegra_mc_soc.
> 
> The intmask_chan_shift is a misnomer, perhaps something like
> status_reg_chan_shift will be a better name.
> 
Okay, I will do this.

>> 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?

>> 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?