Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1243381 > unrolled thread

Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt controller.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. Thomas Gleixner <tglx@linutronix.de> - 2015-10-09 15:50 +0200
    Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. "majun (F)" <majun258@huawei.com> - 2015-10-10 11:10 +0200
      Re: [PATCH v5 1/3] initialize each mbigen device node as a  interrupt controller. Marc Zyngier <marc.zyngier@arm.com> - 2015-10-10 12:20 +0200
        Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. Thomas Gleixner <tglx@linutronix.de> - 2015-10-11 12:00 +0200
          Re: [PATCH v5 1/3] initialize each mbigen device node as a  interrupt controller. Marc Zyngier <marc.zyngier@arm.com> - 2015-10-11 13:10 +0200
            Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. Thomas Gleixner <tglx@linutronix.de> - 2015-10-11 18:50 +0200
              Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. "majun (F)" <majun258@huawei.com> - 2015-10-13 08:40 +0200
                Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. Thomas Gleixner <tglx@linutronix.de> - 2015-10-13 09:00 +0200
                  Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. "majun (F)" <majun258@huawei.com> - 2015-10-14 10:30 +0200
                    Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. Thomas Gleixner <tglx@linutronix.de> - 2015-10-14 10:30 +0200
                      Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. "majun (F)" <majun258@huawei.com> - 2015-10-14 11:10 +0200
              Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. Marc Zyngier <marc.zyngier@arm.com> - 2015-10-14 11:00 +0200
                Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. Thomas Gleixner <tglx@linutronix.de> - 2015-10-14 11:20 +0200
                  Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt  controller. Marc Zyngier <marc.zyngier@arm.com> - 2015-10-14 11:50 +0200

#1243381 — Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt controller.

On Sun, 4 Oct 2015, majun (F) wrote:
> >> +	info->reg_offset = get_mbigen_vec_reg_addr(info->nid, info->local_pin_offset);
> > 
> > So you fill in a structure with 5 fields and the only information
> > which is ever used is local_pin_offset.
> > 
> > What's the point of this exercise? 
> 
> Besides local_pin_offset , nid, and reg_offset are also useful
> information which will be used in next patch.

This is horrible to review, really.

Split your patches into smaller pieces then. Add the core
functionality and then introduce the other things when you actually
use them.

> > And that msi_irq information comes from where? Nothing in that code
> > initializes it.
> 
> msi_irq is is initialized in next patch.

See above.
 
> > All you ever need from this is local_pin_offset and the base address
> > for that calculation in the eoi callback.
> 
> dev_irq is stored for easily using in next patch when interrupt happened.

Ditto.
 
> >> +
> >> +	/* add this mbigen device into a global list*/
> >> +	spin_lock(&mbigen_device_lock);
> >> +	list_add(&mgn_dev->global_entry, &mbigen_device_list);
> >> +	spin_unlock(&mbigen_device_lock);
> > 
> > And that global list is used whatfor? I can't see anything which makes
> > use of it.
> 
> This global list is used to find out mbigen device when initializing the mbigen
> device as a platform device in next patch.

Sorry, this is unreviewable.
 
> Because there are several mbigen chips in this system, and each mbigen chip also
> contains several mbgien devices.
> 
> I need a list contains all of the mbigen devices which connect to these mbigen
> chips.
> Then, during mbigen chip initializing, we can use this list to find out mbigen devices
> and pass mbigen_device data structure.
> 
> > 
> > That's a complete disaster and I'm not even thinking about looking at
> > the next patch in this series.
> > 
> > Can you please explain in a simple ASCII picture how your irq chip
> > hierarchy looks like and what kind of data you need for each hierarchy
> > level?
> 
> Mbigen chip hardware structure shows as below:
> 
> 		mbigen chip
> |---------------------|-------------------|
> mgn_node0	  mgn_node1		mgn_node2
>  |		 |-------|		|-------|------|
> dev1		dev1    dev2		dev1   dev3   dev4
> 
> 
> 
> Irq chip hierarchy stucture:
> 		
> 	ITS
> 	 |
> 	ITS-pMSI
> 	 | (virq1)
> |--------| -----------------|
> mbigen-device1		mbigen-device2
>  | (virq2)		 | (virq2)
> devices(uart)		device(gmac)

That picture is wrong as it suggests that uart and gmac share the same
virq.
 
> I named virq1 as msi_irq , virq2 as dev-irq and ,virq1 != virq2.
> 
> Each virq2 has a corresponding virq1.

Whatfor?
 
> Mbigen-device is a special hardware.

Everything is special hardware.

> On the one hand, it's a platform device for ITS. We need to
> allocate the msi-irqs for it.(handled in patch 2/3)
> 
> On the other hand, it's a interrupt controller for the devices
> connected to it.(handled in current patch).
>
> To bind these two different irqs, I made a data sutruce named
> mbigen_irq_data which contains some information of this irq,
> including private index, pin_offset, nid, and local_pin_offset.
>
> All these information can help us to find the corresponding reg addr
> and msi_irq quickly.

This is completely wrong. Why would you need two linux virq numbers
for one interrupt?

This needs to be expressed in one hierarchy. mbigen is just a
translator between wired interrupts and MSI, nothing else.

So the hierarchy is:

  mbigen -> ITS-MSI -> ITS -> GIC

No need for extra levels of indirection. Your mbigen irqchip callbacks
are simply doing:

    parent->callback(parent_data);

and you get that for free when using the hierarchy. No need for that
chained interrupt handler either.

Thanks,

	tglx


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1243855

在 2015/10/9 21:47, Thomas Gleixner 写道:
> On Sun, 4 Oct 2015, majun (F) wrote:
>>>> +	info->reg_offset = get_mbigen_vec_reg_addr(info->nid, info->local_pin_offset);
>>>
>>> So you fill in a structure with 5 fields and the only information
[...]
>> On the other hand, it's a interrupt controller for the devices
>> connected to it.(handled in current patch).
>>
>> To bind these two different irqs, I made a data sutruce named
>> mbigen_irq_data which contains some information of this irq,
>> including private index, pin_offset, nid, and local_pin_offset.
>>
>> All these information can help us to find the corresponding reg addr
>> and msi_irq quickly.
> 
> This is completely wrong. Why would you need two linux virq numbers
> for one interrupt?
> 
> This needs to be expressed in one hierarchy. mbigen is just a
> translator between wired interrupts and MSI, nothing else.
> 
> So the hierarchy is:
> 
>   mbigen -> ITS-MSI -> ITS -> GIC

I think maybe you mean: mbigen -> ITS-pMSI -> ITS- > GIC

But there is a problem If i make the structure like you said.

For example, my hardware structure likes below:

uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
     virq1

virq1 means the virq number allocted by irq_of_parse_and_map() function
when system parse the uart dts node in initializing  stage.

To create a ITS device, I need to call msi_domain_alloc_irqs() function
in my mbigen alloc function.

In this function, a new virq number(named as virq2 ) which different from
virq1 is allocated.
So, this is a big problem.

If we want to use the hierarchy structure, I think

mbigen -> ITS -> GIC

maybe is a possible way .

The only problem is I need to do some change in ITS driver.

I mean move its_create_device() and its_find_device() into
 its_irq_domain_alloc()

But this solution is similar to my v3 patch.

Thanks!
Ma Jun

















> 
> No need for extra levels of indirection. Your mbigen irqchip callbacks
> are simply doing:
> 
>     parent->callback(parent_data);
> 
> and you get that for free when using the hierarchy. No need for that
> chained interrupt handler either.
> 



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243875 — Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt controller.

On Sat, 10 Oct 2015 17:01:32 +0800
"majun (F)" <majun258@huawei.com> wrote:

> 
> 
> 在 2015/10/9 21:47, Thomas Gleixner 写道:
> > On Sun, 4 Oct 2015, majun (F) wrote:
> >>>> +	info->reg_offset = get_mbigen_vec_reg_addr(info->nid, info->local_pin_offset);
> >>>
> >>> So you fill in a structure with 5 fields and the only information
> [...]
> >> On the other hand, it's a interrupt controller for the devices
> >> connected to it.(handled in current patch).
> >>
> >> To bind these two different irqs, I made a data sutruce named
> >> mbigen_irq_data which contains some information of this irq,
> >> including private index, pin_offset, nid, and local_pin_offset.
> >>
> >> All these information can help us to find the corresponding reg addr
> >> and msi_irq quickly.
> > 
> > This is completely wrong. Why would you need two linux virq numbers
> > for one interrupt?
> > 
> > This needs to be expressed in one hierarchy. mbigen is just a
> > translator between wired interrupts and MSI, nothing else.
> > 
> > So the hierarchy is:
> > 
> >   mbigen -> ITS-MSI -> ITS -> GIC
> 
> I think maybe you mean: mbigen -> ITS-pMSI -> ITS- > GIC
> 
> But there is a problem If i make the structure like you said.
> 
> For example, my hardware structure likes below:
> 
> uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
>      virq1
> 
> virq1 means the virq number allocted by irq_of_parse_and_map() function
> when system parse the uart dts node in initializing  stage.
> 
> To create a ITS device, I need to call msi_domain_alloc_irqs() function
> in my mbigen alloc function.
> 
> In this function, a new virq number(named as virq2 ) which different from
> virq1 is allocated.
> So, this is a big problem.

I think I see what your problem is:
- The wired interrupt (uart -> mbigen) is allocated through DT (and
  must be available early, because of of_platform_populate),
- The MSI (mgigen -> ITS) is dynamic (and allocated much later,
  because the device model kicks in after irqchip init, and we cannot
  allocate MSIs without a device).

So we end-up with two virqs that need to be reconciled one way or
another. Is that an accurate description of your problem?

This is a silly situation, because there is no multiplexing at all
(there is exactly one wired interrupt per MSI). It is just that we
don't have the infrastructure to connect the two virtual interrupts.

Thomas, what would you think of extending the MSI layer a bit so that
instead of allocating the virqs dynamically, we could take an
optional array of virqs that would be used? It should be pretty easy to
hack this into msi_domain_alloc_irqs(), and modify the existing callers
(or have a separate entry point altogether). I'm still a bit fuzzy on
how we stitch the domains themselves (we need to set ->parent on
allocating the MSIs, which leaves it being a NULL pointer until then),
but I think there is a way. Something like:

int msi_domain_alloc_stitch_irqs(struct irq_domain *domain,
				 struct irq_domain *parent,
				 struct device *dev,
				 int *virqs, int nvecs);

Naming is crap, but you'll get the idea. Thoughts?

> If we want to use the hierarchy structure, I think
> 
> mbigen -> ITS -> GIC
> 
> maybe is a possible way .
> 
> The only problem is I need to do some change in ITS driver.
> 
> I mean move its_create_device() and its_find_device() into
>  its_irq_domain_alloc()
> 
> But this solution is similar to my v3 patch.

And that was really bad. Let's not go back there.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244092

On Sat, 10 Oct 2015, Marc Zyngier wrote:
> On Sat, 10 Oct 2015 17:01:32 +0800
> "majun (F)" <majun258@huawei.com> wrote:
> > But there is a problem If i make the structure like you said.
> > 
> > For example, my hardware structure likes below:
> > 
> > uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
> >      virq1
> > 
> > virq1 means the virq number allocted by irq_of_parse_and_map() function
> > when system parse the uart dts node in initializing  stage.
> > 
> > To create a ITS device, I need to call msi_domain_alloc_irqs() function
> > in my mbigen alloc function.
> > 
> > In this function, a new virq number(named as virq2 ) which different from
> > virq1 is allocated.
> > So, this is a big problem.
> 
> I think I see what your problem is:
> - The wired interrupt (uart -> mbigen) is allocated through DT (and
>   must be available early, because of of_platform_populate),
> - The MSI (mgigen -> ITS) is dynamic (and allocated much later,
>   because the device model kicks in after irqchip init, and we cannot
>   allocate MSIs without a device).

Why do we need that wired interrupt at all? 

We can make mbigen the 'msi-parent' of the device and let the
msi_domain_ops::msi_prepare() callback figure out the actual wiring
through device->fwnode.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244097 — Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt controller.

On Sun, 11 Oct 2015 11:54:49 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Sat, 10 Oct 2015, Marc Zyngier wrote:
> > On Sat, 10 Oct 2015 17:01:32 +0800
> > "majun (F)" <majun258@huawei.com> wrote:
> > > But there is a problem If i make the structure like you said.
> > > 
> > > For example, my hardware structure likes below:
> > > 
> > > uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
> > >      virq1
> > > 
> > > virq1 means the virq number allocted by irq_of_parse_and_map() function
> > > when system parse the uart dts node in initializing  stage.
> > > 
> > > To create a ITS device, I need to call msi_domain_alloc_irqs() function
> > > in my mbigen alloc function.
> > > 
> > > In this function, a new virq number(named as virq2 ) which different from
> > > virq1 is allocated.
> > > So, this is a big problem.
> > 
> > I think I see what your problem is:
> > - The wired interrupt (uart -> mbigen) is allocated through DT (and
> >   must be available early, because of of_platform_populate),
> > - The MSI (mgigen -> ITS) is dynamic (and allocated much later,
> >   because the device model kicks in after irqchip init, and we cannot
> >   allocate MSIs without a device).
> 
> Why do we need that wired interrupt at all? 
> 
> We can make mbigen the 'msi-parent' of the device and let the
> msi_domain_ops::msi_prepare() callback figure out the actual wiring
> through device->fwnode.

That's because the device behind the mbigen can't do any MSI at all.
Think of a 8250 uart, for example.

If we make the mbigen the msi-parent of the uart, then we need to teach
the 8250 driver to request MSIs. It also means that the DT doesn't
represent the HW anymore (this wired interrupt actually exists).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244155

On Sun, 11 Oct 2015, Marc Zyngier wrote:
> On Sun, 11 Oct 2015 11:54:49 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Sat, 10 Oct 2015, Marc Zyngier wrote:
> > > On Sat, 10 Oct 2015 17:01:32 +0800
> > > "majun (F)" <majun258@huawei.com> wrote:
> > > > But there is a problem If i make the structure like you said.
> > > > 
> > > > For example, my hardware structure likes below:
> > > > 
> > > > uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
> > > >      virq1
> > > > 
> > > > virq1 means the virq number allocted by irq_of_parse_and_map() function
> > > > when system parse the uart dts node in initializing  stage.
> > > > 
> > > > To create a ITS device, I need to call msi_domain_alloc_irqs() function
> > > > in my mbigen alloc function.
> > > > 
> > > > In this function, a new virq number(named as virq2 ) which different from
> > > > virq1 is allocated.
> > > > So, this is a big problem.
> > > 
> > > I think I see what your problem is:
> > > - The wired interrupt (uart -> mbigen) is allocated through DT (and
> > >   must be available early, because of of_platform_populate),
> > > - The MSI (mgigen -> ITS) is dynamic (and allocated much later,
> > >   because the device model kicks in after irqchip init, and we cannot
> > >   allocate MSIs without a device).
> > 
> > Why do we need that wired interrupt at all? 
> > 
> > We can make mbigen the 'msi-parent' of the device and let the
> > msi_domain_ops::msi_prepare() callback figure out the actual wiring
> > through device->fwnode.
> 
> That's because the device behind the mbigen can't do any MSI at all.
> Think of a 8250 uart, for example.
> 
> If we make the mbigen the msi-parent of the uart, then we need to teach
> the 8250 driver to request MSIs.

I really do not understand why of_platform_populate cares about
interrupt allocations. That's outright stupid. We should care about
that at device probe time, i.e. at the point where the driver is
registered and probed if there is matching platform data. If we do it
in of_platform_populate then we allocate interrupts even for devices
which do not have a driver, i.e. we just waste memory.

So we better teach a couple of drivers to handle that instead of
inventing horrible workarounds.

> It also means that the DT doesn't represent the HW anymore (this
> wired interrupt actually exists).

I think the abstraction here is wrong. If it would be correct, then
PCI-MSI would be wrong. The MSI part of PCI is a MSI producer, mbigen
is as well. Technically MSI is not integral part of the PCI device, it
just happens to have it's configuration registers in the PCI
configuration space of the device:

    [PCI-BUS]------[Interrupt mode selector]
    	      	|        |
    	      	|        |
		------[Legacy irq gate]<-----
		|        |                  |
		|        |                  |---[Device interrupt]
		|        |                  |
		------[MSI unit]<------------

So you have a 'wire' from the device to the MSI unit, but we do not
care about that 'wire'. All we care about are the MSI configuration
registers. We find them via the PCI device which gives us the address
of the PCI configuration space.

So now in the mbigen case this looks like this:

    [MSI-BUS] ----- [MBIGEN]<-------------------[Device interrupt]

Again, you have a 'wire' from the device to the MSI unit (MBIGEN) and
we do not care about that 'wire' either. What we care about is how we
find the MSI (mbigen) configuration registers for a particular
device. So we need a DT/ACPI entry which describes those configuration
registers and whatever supplementary information is required. That
will make the mbigen driver extremly simple.

Thanks,

	tglx



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245378

Hi Thomas:

在 2015/10/12 0:45, Thomas Gleixner 写道:
> On Sun, 11 Oct 2015, Marc Zyngier wrote:
>> On Sun, 11 Oct 2015 11:54:49 +0200
>> Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>>> On Sat, 10 Oct 2015, Marc Zyngier wrote:
>>>> On Sat, 10 Oct 2015 17:01:32 +0800
>>>> "majun (F)" <majun258@huawei.com> wrote:
>>>>> But there is a problem If i make the structure like you said.
>>>>>
>>>>> For example, my hardware structure likes below:
>>>>>
>>>>> uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
>>>>>      virq1
>>>>>
>>>>> virq1 means the virq number allocted by irq_of_parse_and_map() function
>>>>> when system parse the uart dts node in initializing  stage.
>>>>>
>>>>> To create a ITS device, I need to call msi_domain_alloc_irqs() function
>>>>> in my mbigen alloc function.
>>>>>
>>>>> In this function, a new virq number(named as virq2 ) which different from
>>>>> virq1 is allocated.
>>>>> So, this is a big problem.
>>>>
>>>> I think I see what your problem is:
>>>> - The wired interrupt (uart -> mbigen) is allocated through DT (and
>>>>   must be available early, because of of_platform_populate),
>>>> - The MSI (mgigen -> ITS) is dynamic (and allocated much later,
>>>>   because the device model kicks in after irqchip init, and we cannot
>>>>   allocate MSIs without a device).
>>>
>>> Why do we need that wired interrupt at all? 
>>>
>>> We can make mbigen the 'msi-parent' of the device and let the
>>> msi_domain_ops::msi_prepare() callback figure out the actual wiring
>>> through device->fwnode.
>>
>> That's because the device behind the mbigen can't do any MSI at all.
>> Think of a 8250 uart, for example.
>>
>> If we make the mbigen the msi-parent of the uart, then we need to teach
>> the 8250 driver to request MSIs.
> 
> I really do not understand why of_platform_populate cares about
> interrupt allocations. That's outright stupid. We should care about
> that at device probe time, i.e. at the point where the driver is
> registered and probed if there is matching platform data. If we do it
> in of_platform_populate then we allocate interrupts even for devices
> which do not have a driver, i.e. we just waste memory.
> 
> So we better teach a couple of drivers to handle that instead of
> inventing horrible workarounds.
> 
>> It also means that the DT doesn't represent the HW anymore (this
>> wired interrupt actually exists).
> 
> I think the abstraction here is wrong. If it would be correct, then
> PCI-MSI would be wrong. The MSI part of PCI is a MSI producer, mbigen
> is as well. Technically MSI is not integral part of the PCI device, it
> just happens to have it's configuration registers in the PCI
> configuration space of the device:
> 
>     [PCI-BUS]------[Interrupt mode selector]
>     	      	|        |
>     	      	|        |
> 		------[Legacy irq gate]<-----
> 		|        |                  |
> 		|        |                  |---[Device interrupt]
> 		|        |                  |
> 		------[MSI unit]<------------
> 
> So you have a 'wire' from the device to the MSI unit, but we do not
> care about that 'wire'. All we care about are the MSI configuration
> registers. We find them via the PCI device which gives us the address
> of the PCI configuration space.
> 
> So now in the mbigen case this looks like this:
> 
>     [MSI-BUS] ----- [MBIGEN]<-------------------[Device interrupt]
> 
> Again, you have a 'wire' from the device to the MSI unit (MBIGEN) and
> we do not care about that 'wire' either. What we care about is how we
> find the MSI (mbigen) configuration registers for a particular
> device. So we need a DT/ACPI entry which describes those configuration
> registers and whatever supplementary information is required. That
> will make the mbigen driver extremly simple.
> 

According to your suggestions, I tried to make the hardware structure likes below:

device(8250 uart) -> mbigne -> ITS-pMSI --> ITS --> GIC

And  8250 uart dts node is:

 8250_uart {
	compatible = "xxx";
	msi-parent = < &mbigen>;
	config_addr = <xxxxx> ; /* configuration register */
	interrupts = <x>;
	interrupt-parent = ?
}

My question is what's the interrupt-parent should be?



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245388

[Multipart message — attachments visible in raw view] — view raw

Majun,

On Tue, 13 Oct 2015, majun (F) wrote:
> 在 2015/10/12 0:45, Thomas Gleixner 写道:
> > So now in the mbigen case this looks like this:
> > 
> >     [MSI-BUS] ----- [MBIGEN]<-------------------[Device interrupt]
> > 
> > Again, you have a 'wire' from the device to the MSI unit (MBIGEN) and
> > we do not care about that 'wire' either. What we care about is how we
> > find the MSI (mbigen) configuration registers for a particular
> > device. So we need a DT/ACPI entry which describes those configuration
> > registers and whatever supplementary information is required. That
> > will make the mbigen driver extremly simple.
> > 
> 
> According to your suggestions, I tried to make the hardware structure likes below:
> 
> device(8250 uart) -> mbigne -> ITS-pMSI --> ITS --> GIC

I'm not sure whether mbigen should be connected to ITS-pMSI (I assume
you mean ITS-PCI-MSI).

mbigen is a seperate MSI domain, so it should connect to ITS, but I
leave that to Marc.
 
> And  8250 uart dts node is:
> 
>  8250_uart {
> 	compatible = "xxx";
> 	msi-parent = < &mbigen>;
> 	config_addr = <xxxxx> ; /* configuration register */
> 	interrupts = <x>;
> 	interrupt-parent = ?
> }
> 
> My question is what's the interrupt-parent should be?

There is no interrupt parent for 8250_uart. Why would you want that?
I'm really not a DT expert, but I think you want something like this:

  8250_uart {
 	compatible = "xxx";
 	msi-parent = < &mbigen_node5>;
 	interrupt-map = <&mbigen5 0>;
  };

and then have

  mbigen_node5 {
  	...
	reg = <....>;
  };

So the other devices which are connected to mbigen_node5 have the same
msi-parent. But then again, please discuss that with Marc and the DT
wizards.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1246398

Hi Thomas:

在 2015/10/13 14:55, Thomas Gleixner 写道:
> Majun,
> 
> On Tue, 13 Oct 2015, majun (F) wrote:
>> 在 2015/10/12 0:45, Thomas Gleixner 写道:
>>> So now in the mbigen case this looks like this:
>>>
>>>     [MSI-BUS] ----- [MBIGEN]<-------------------[Device interrupt]
>>>
>>> Again, you have a 'wire' from the device to the MSI unit (MBIGEN) and
>>> we do not care about that 'wire' either. What we care about is how we
>>> find the MSI (mbigen) configuration registers for a particular
>>> device. So we need a DT/ACPI entry which describes those configuration
>>> registers and whatever supplementary information is required. That
>>> will make the mbigen driver extremly simple.
>>>
>>
>> According to your suggestions, I tried to make the hardware structure likes below:
>>
>> device(8250 uart) -> mbigne -> ITS-pMSI --> ITS --> GIC
> 
> I'm not sure whether mbigen should be connected to ITS-pMSI (I assume
> you mean ITS-PCI-MSI).

ITS domain has two child domains. One is ITS-pMSI for Non-PCI devices,
the other one is ITS-MSI for PCI devices.

> 
> mbigen is a seperate MSI domain, so it should connect to ITS, but I
> leave that to Marc.

I also think mbigen should connected to ITS.

Now, the hierarchy structure is
MBIGEN -> ITS -> GIC.

This structure is really similar to the structure in my v3 patch except the
dts.

>  
>> And  8250 uart dts node is:
>>
>>  8250_uart {
>> 	compatible = "xxx";
>> 	msi-parent = < &mbigen>;
>> 	config_addr = <xxxxx> ; /* configuration register */
>> 	interrupts = <x>;
>> 	interrupt-parent = ?
>> }
>>
>> My question is what's the interrupt-parent should be?
> 
> There is no interrupt parent for 8250_uart. Why would you want that?
> I'm really not a DT expert, but I think you want something like this:
> 
>   8250_uart {
>  	compatible = "xxx";
>  	msi-parent = < &mbigen_node5>;
>  	interrupt-map = <&mbigen5 0>;
>   };
> 

Maybe you mean
interrupt-map = <&mbigen_node5 0>;

I have some questions about this

[1]: I noticed there is no interrupts property,
So, do you mean we don't need this property here ?

[2]: I am confused about interrupt-map.

This property is parsed in function of_irq_parse_raw().
In this fucntion

"
ipar = of_node_get(out_irq->np);
of_get_property(ipar, "interrupt-map", &imaplen);
"
When this function is called by of_irq_parse_one(),
the input para out_irq->np pointed to GIC or mbigen (depends on dts).

So, of_get_property() tried to get the interrupt-map from
GIC or mbgien node.

But there is no interrupt-map property in GIC or mbigen node.



> and then have
> 
>   mbigen_node5 {
>   	...
> 	reg = <....>;
>   };
> 
> So the other devices which are connected to mbigen_node5 have the same
> msi-parent. But then again, please discuss that with Marc and the DT
> wizards.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246402

On Wed, 14 Oct 2015, majun (F) wrote:
> But there is no interrupt-map property in GIC or mbigen node.

Again: I'm not a DT expert. Please discuss that with Marc and the DT
wizards.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246434

在 2015/10/14 16:20, Thomas Gleixner 写道:
> On Wed, 14 Oct 2015, majun (F) wrote:
>> But there is no interrupt-map property in GIC or mbigen node.
> 
> Again: I'm not a DT expert. Please discuss that with Marc and the DT
> wizards.
> 
> Thanks,
> 
> 	tglx
> 
> .

Ok.
Marc, what's your suggestion ? much appreciated

Thanks!
Ma Jun


> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246425

Hi Thomas,

Sorry it took me so long to come back to you on that one, I really
needed to wrap my head around it.

On 11/10/15 17:45, Thomas Gleixner wrote:
> On Sun, 11 Oct 2015, Marc Zyngier wrote:
>> On Sun, 11 Oct 2015 11:54:49 +0200
>> Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>>> On Sat, 10 Oct 2015, Marc Zyngier wrote:
>>>> On Sat, 10 Oct 2015 17:01:32 +0800
>>>> "majun (F)" <majun258@huawei.com> wrote:
>>>>> But there is a problem If i make the structure like you said.
>>>>>
>>>>> For example, my hardware structure likes below:
>>>>>
>>>>> uart ------> mbigen --> ITS-pMSI --> ITS --> GIC
>>>>>      virq1
>>>>>
>>>>> virq1 means the virq number allocted by irq_of_parse_and_map() function
>>>>> when system parse the uart dts node in initializing  stage.
>>>>>
>>>>> To create a ITS device, I need to call msi_domain_alloc_irqs() function
>>>>> in my mbigen alloc function.
>>>>>
>>>>> In this function, a new virq number(named as virq2 ) which different from
>>>>> virq1 is allocated.
>>>>> So, this is a big problem.
>>>>
>>>> I think I see what your problem is:
>>>> - The wired interrupt (uart -> mbigen) is allocated through DT (and
>>>>   must be available early, because of of_platform_populate),
>>>> - The MSI (mgigen -> ITS) is dynamic (and allocated much later,
>>>>   because the device model kicks in after irqchip init, and we cannot
>>>>   allocate MSIs without a device).
>>>
>>> Why do we need that wired interrupt at all? 
>>>
>>> We can make mbigen the 'msi-parent' of the device and let the
>>> msi_domain_ops::msi_prepare() callback figure out the actual wiring
>>> through device->fwnode.
>>
>> That's because the device behind the mbigen can't do any MSI at all.
>> Think of a 8250 uart, for example.
>>
>> If we make the mbigen the msi-parent of the uart, then we need to teach
>> the 8250 driver to request MSIs.
> 
> I really do not understand why of_platform_populate cares about
> interrupt allocations. That's outright stupid. We should care about
> that at device probe time, i.e. at the point where the driver is
> registered and probed if there is matching platform data. If we do it
> in of_platform_populate then we allocate interrupts even for devices
> which do not have a driver, i.e. we just waste memory.

This was introduce for a specific reason: being able to convert systems
from board files to DT without having to DT-ify drivers. Interrupt
numbers magically appear as part of the resource array, and everything
just works.

> 
> So we better teach a couple of drivers to handle that instead of
> inventing horrible workarounds.
> 
>> It also means that the DT doesn't represent the HW anymore (this
>> wired interrupt actually exists).
> 
> I think the abstraction here is wrong. If it would be correct, then
> PCI-MSI would be wrong. The MSI part of PCI is a MSI producer, mbigen
> is as well. Technically MSI is not integral part of the PCI device, it
> just happens to have it's configuration registers in the PCI
> configuration space of the device:

The main difference is that the MSI layer is actually specified in PCI.
Yes, the wire is conveniently hidden from us, but that's also because it
can be hidden: by construction, you have an MSI generator per device.

Mbigen breaks this rule: you can have an MSI generator that covers
multiple devices, or a single device that spans multiple generators.
Nothing that can't be overcome, but that makes things a bit ugly.

>     [PCI-BUS]------[Interrupt mode selector]
>     	      	|        |
>     	      	|        |
> 		------[Legacy irq gate]<-----
> 		|        |                  |
> 		|        |                  |---[Device interrupt]
> 		|        |                  |
> 		------[MSI unit]<------------
> 
> So you have a 'wire' from the device to the MSI unit, but we do not
> care about that 'wire'. All we care about are the MSI configuration
> registers. We find them via the PCI device which gives us the address
> of the PCI configuration space.
> 
> So now in the mbigen case this looks like this:
> 
>     [MSI-BUS] ----- [MBIGEN]<-------------------[Device interrupt]
> 
> Again, you have a 'wire' from the device to the MSI unit (MBIGEN) and
> we do not care about that 'wire' either. What we care about is how we
> find the MSI (mbigen) configuration registers for a particular
> device. So we need a DT/ACPI entry which describes those configuration
> registers and whatever supplementary information is required. That
> will make the mbigen driver extremly simple.

It makes mbigen simple indeed, but it also makes other parts more complex:

- mbigen needs to implement its own MSI layer (we cannot use the
platform one for that)

- it effectively becomes part of the ITS driver (it needs to pass the
device IDs down to the core ITS code instead of relying on the ITS
platform MSI code).

- this probably means introducing some new probing infrastructure so
that the device msi_domain field can be populated with the right domain
(I'm not sure the platform probing does the right thing in that case)

- drivers that aren't MSI aware need to be extended to talk to this new
MSI provider.

- drivers that already support MSI natively (like the ARM SMMUv3, see
http://www.spinics.net/lists/arm-kernel/msg452057.html) will have to be
hacked to also work with this new method.

To me, it feels like we're spreading the complexity across multiple
layers instead of keeping it localized. It also means that next time
some crazy HW dude comes up with a similar idea (and I have little doubt
this will happen sooner than later), we'll have to replicate the same
thing again (though we could put all that behind another abstraction layer).

I would have preferred a solution where the MSI domain is allowed to be
sandwiched between two non-MSI domains, and expose the top level
irqchip. This means fixing the following:

- Either find a way to prevent DT doing these early IRQ allocations
(this could be easily done by simply not registering the irqchip), or be
able to elegantly reuse them.

- Add an API allowing an MSI domain to be the parent of another domain.

Once we have this, we can use the platform MSI layer for the mbigen
without much complexity (well, not more that any other stacked irqchip,
the madness of the mbigen programming interface notwithstanding), and
drivers stay untouched. It would also give us a 'standard' way to deal
with the above HW dude. I'd be happy to prototype it.

Now, I'm not going to make that a religious affair. If you still think
I'm severely misguided, I'll stop arguing and we'll make it work
according to your plans.

You can now open fire! ;-)

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246443

Marc,

On Wed, 14 Oct 2015, Marc Zyngier wrote:
> To me, it feels like we're spreading the complexity across multiple
> layers instead of keeping it localized. It also means that next time
> some crazy HW dude comes up with a similar idea (and I have little doubt
> this will happen sooner than later), we'll have to replicate the same
> thing again (though we could put all that behind another abstraction layer).
> 
> I would have preferred a solution where the MSI domain is allowed to be
> sandwiched between two non-MSI domains, and expose the top level
> irqchip. This means fixing the following:
> 
> - Either find a way to prevent DT doing these early IRQ allocations
> (this could be easily done by simply not registering the irqchip), or be
> able to elegantly reuse them.

The reuse part makes me shudder. We really should not go there. It's a
blatant layering violation.

> - Add an API allowing an MSI domain to be the parent of another domain.
> 
> Once we have this, we can use the platform MSI layer for the mbigen
> without much complexity (well, not more that any other stacked irqchip,
> the madness of the mbigen programming interface notwithstanding), and
> drivers stay untouched. It would also give us a 'standard' way to deal
> with the above HW dude. I'd be happy to prototype it.

Ok, I have a better understanding of it now.

I have no objections to your approach as long as it provides us a
clean way to use a full hierarchy without weird interfaces to reuse
irq descriptors etc. If you can find a way which just follows the
proper hierarchy design, I'm certainly not in your way.

OTOH, the platform msi driver is not a huge amount of code and from my
understanding of the hardware it looks weird to have this intermediate
layer. Making mbigen a direct child of ITS feels just more natural to
me. I'm pretty sure that this can be done without the earlier proposed
horrible modifications to ITS. It just should fall into place.

It would be really great just to have shell implementations,
i.e. without the mbigen specific stuff - for both models so we can
compare and contrast the results. That means just the interfaces and
the hookup to the various layers.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246468

On 14/10/15 10:17, Thomas Gleixner wrote:
> Marc,
> 
> On Wed, 14 Oct 2015, Marc Zyngier wrote:
>> To me, it feels like we're spreading the complexity across multiple
>> layers instead of keeping it localized. It also means that next time
>> some crazy HW dude comes up with a similar idea (and I have little doubt
>> this will happen sooner than later), we'll have to replicate the same
>> thing again (though we could put all that behind another abstraction layer).
>>
>> I would have preferred a solution where the MSI domain is allowed to be
>> sandwiched between two non-MSI domains, and expose the top level
>> irqchip. This means fixing the following:
>>
>> - Either find a way to prevent DT doing these early IRQ allocations
>> (this could be easily done by simply not registering the irqchip), or be
>> able to elegantly reuse them.
> 
> The reuse part makes me shudder. We really should not go there. It's a
> blatant layering violation.
> 
>> - Add an API allowing an MSI domain to be the parent of another domain.
>>
>> Once we have this, we can use the platform MSI layer for the mbigen
>> without much complexity (well, not more that any other stacked irqchip,
>> the madness of the mbigen programming interface notwithstanding), and
>> drivers stay untouched. It would also give us a 'standard' way to deal
>> with the above HW dude. I'd be happy to prototype it.
> 
> Ok, I have a better understanding of it now.
> 
> I have no objections to your approach as long as it provides us a
> clean way to use a full hierarchy without weird interfaces to reuse
> irq descriptors etc. If you can find a way which just follows the
> proper hierarchy design, I'm certainly not in your way.
> 
> OTOH, the platform msi driver is not a huge amount of code and from my
> understanding of the hardware it looks weird to have this intermediate
> layer. Making mbigen a direct child of ITS feels just more natural to
> me. I'm pretty sure that this can be done without the earlier proposed
> horrible modifications to ITS. It just should fall into place.
> 
> It would be really great just to have shell implementations,
> i.e. without the mbigen specific stuff - for both models so we can
> compare and contrast the results. That means just the interfaces and
> the hookup to the various layers.

OK, let's do that. I'll try to post something before Friday so that we
can really evaluate what the knock-on effect is.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web