Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge

[Mason]

On 24/03/2017 19:47, Marc Zyngier wrote:

> On 23/03/17 17:03, Mason wrote:
>
>> On 23/03/2017 15:22, Marc Zyngier wrote:
>>
>>> On 23/03/17 13:05, Mason wrote:
>>>
>>>> +	writel_relaxed(status, pcie->msi_status); /* clear IRQs */
>>>
>>> Why isn't this your irq_ack method instead of open-coding it?
>>
>> I based my driver on the Altera driver, and I did it like
>> I thought they did. I will try fixing my code.
> 
> Doesn't make it right, unfortunately. I wish you would try to understand
> the API first instead of copy-pasting things (including potential bugs).

So far, I have not been able to get the irqchip framework to
call the irq_ack functions I registered.

Should I pass a different handler than handle_simple_irq
to irq_domain_set_info?

	irq_domain_set_info(domain, virq, pos, &tango_msi_chip,
		domain->host_data, handle_simple_irq, NULL, NULL);


When an MSI packet arrives at the MSI doorbell address, the controller
reads the packet's data; this is the MSI number "num". It sets bit "num"
to 1 in the status regs, and raises IRQ line 55 on the system intc.
The IRQ signal remains high, until software clears it by writing 1
in bit "num" of the status regs.

Is this an edge interrupt or a level interrupt?

I was told if the interrupt request is triggered by an event, then
it is an edge interrupt. The reception of an MSI packet is an event.
But the IRQ remains high, so this feels like a level high.
I'm hopelessly confused :-(


>>>> +	mutex_lock(&pcie->lock);
>>>> +
>>>> +	mask = readl_relaxed(pcie->msi_mask);
>>>
>>> Do you really need to read this from the HW each time you allocate an
>>> interrupt? That feels pretty crazy. You're much better off having an
>>> in-memory bitmap that will make things more efficient [...]

I have one remaining issue with bitmaps.

My HW regs are 32b. How do I grab e.g. bits 96-127?
All I can think of is
  u32 val = ((u32 *)bitmap)[3];

Is this acceptable?

mrutland mentioned bitmap_to_u32array() but IIUC it is used to
copy an entire bitmap.


>>>> +	if (pos < MSI_COUNT)
>>>> +		writel(mask | BIT(pos), pcie->msi_mask);
>>>
>>> And it would make a lot more sense to move this write (which should be
>>> relaxed) to irq_unmask. Also, calling msi_mask for something that is an
>>> enable register is a bit counter intuitive.
>>
>> I don't have as much experience as you.
>> I just used the names in the HW documentation.
>> I think it is the "mask" (as in bitmap) of enabled MSIs.
>> I will change "mask" to "enable".
>>
>> Are you saying I should not use pci_msi_mask_irq and pci_msi_unmask_irq,
>> but register custom implementations? I should still call these in my
>> custom functions, right?
> 
> You can call both in your own mask/unmask methods. They serve different
> purpose (one is at the endpoint level, the other is at the MSI
> controller level).

So, if I understand correctly, I should check for an available MSIs
using the in-memory bitmap in tango_irq_domain_alloc(), but I would
defer actually enabling the MSI until irq_unmask?

I think work on bitmap and on the underlying HW regs need to be
protected under the same spinlock, correct?


>> Note: I don't have an "interrupt-map" prop because rev1 doesn't support
>> legacy PCI interrupts (INTx). But I see the PCI framework wrongly mapping
>> intA to my system's interrupt #1, presumably because I am lacking an
>> interrupt-map?
> 
> Probably. I don't think it is legal not to have an interrupt-map.

My understanding is that the interrupt-map actually specifies how to
map the legacy IRQs. My platform does not support legacy IRQs; maybe
there is some binding to say that? Maybe this is more a question for
the PCI folks.

Regards.