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?
