| Message ID | 20160514190435.GK16305@toto (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, May 14, 2016 at 09:04:35PM +0200, Edgar E. Iglesias wrote: [...] > > Was PCIe working on Xen 4.6? If yes, it's the regression and we could > > consider it as a blocker for the release (CC Wei for that). > > No, these PCIe nodes were added recently. > The device tree bindings were different before but after discussing > with the upstream Linux folks they got changed. > OK, so this is not a blocker for 4.7. It would be OK to have a board specific quirk for 4.7 though so that people just don't blindly stumble upon this issue. From my point of view I would happily accept one such patch because you're the de facto maintainer of that board. :-) Anything that touches common code will be scrutinised though. The patch you posted looks very hacky to me at first glance. Wei.
Hello Edgar, On 14/05/16 20:04, Edgar E. Iglesias wrote: > On Sat, May 14, 2016 at 07:15:55PM +0100, Julien Grall wrote: > You can see that it is used in the interrupt-map properties. > IIUC, the interrupt lines connected to the pcie_intc controller > are simply going to be combined into the "intx" line IRQ 116 on > going from the pcie bridge towards the gic. > > I don't think we need to do much than to ignore the node but what > we present to dom0 must look the same.. I think you are right. I expect the platform to only have one main interrupt controller (i.e the GIC), the others would be connected to the main one. Therefore, the interrupts will have no meaning for the GIC and could be ignored. FWIW, we already have a such check in handle_device. > >> >>> >>> Disabling the pcie node for zynqmp boards gets dom0 to boot (obviously >>> without PCIe support). >>> >>> Does it make sense to try to fix this problem this late inte the >>> release cycle? (I can have a closer look and propose a possible fix >>> for discussion) >> >> I would try to fix it in Xen 4.7 if the patch is simple. Otherwise we could >> backport it after the release. > > OK, great. > I'll have a closer look too then. > > I was trying this, but it may be too permissive: > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index 0ed86a7..68cb162 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -1176,6 +1176,15 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, > for ( i = 0; i < pintsize; i++ ) > dt_raw_irq.specifier[i] = dt_read_number(imap + i, 1); > > + if (dt_raw_irq.controller != dt_interrupt_controller) { Coding style. > + /* If this is not the main interrupt controller, we assume > + * it's part of a bus-bridge and ignore remapping IRQs for it. > + * Xen only supports one interrupt controller at the moment. */ This comment is not true for every case. The interrupt controller may not be part of the bus-bridge. One platform with multiple interrupt controller, I expect to always see one main controller, the others would be connected to the main one. So, the interrupt will have no meaning for the GIC. > + imap += pintsize; > + imaplen -= pintsize; I would add a dt_printk to mention that the IRQ has been skipped. > + continue; > + } Please send a formal patch to the ML. What would be the drawback if this patch doesn't reach Xen 4.7? I.e Are PCI devices necessary to run the platform? Regards,
On Mon, May 16, 2016 at 02:20:24PM +0100, Julien Grall wrote: > Hello Edgar, > > On 14/05/16 20:04, Edgar E. Iglesias wrote: > >On Sat, May 14, 2016 at 07:15:55PM +0100, Julien Grall wrote: > >You can see that it is used in the interrupt-map properties. > >IIUC, the interrupt lines connected to the pcie_intc controller > >are simply going to be combined into the "intx" line IRQ 116 on > >going from the pcie bridge towards the gic. > > > >I don't think we need to do much than to ignore the node but what > >we present to dom0 must look the same.. > > I think you are right. I expect the platform to only have one main interrupt > controller (i.e the GIC), the others would be connected to the main one. > > Therefore, the interrupts will have no meaning for the GIC and could be > ignored. > > FWIW, we already have a such check in handle_device. Thanks Julien, I'll have a look at handle_device. > > > > >> > >>> > >>>Disabling the pcie node for zynqmp boards gets dom0 to boot (obviously > >>>without PCIe support). > >>> > >>>Does it make sense to try to fix this problem this late inte the > >>>release cycle? (I can have a closer look and propose a possible fix > >>>for discussion) > >> > >>I would try to fix it in Xen 4.7 if the patch is simple. Otherwise we could > >>backport it after the release. > > > >OK, great. > >I'll have a closer look too then. > > > >I was trying this, but it may be too permissive: > > > >diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > >index 0ed86a7..68cb162 100644 > >--- a/xen/common/device_tree.c > >+++ b/xen/common/device_tree.c > >@@ -1176,6 +1176,15 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, > > for ( i = 0; i < pintsize; i++ ) > > dt_raw_irq.specifier[i] = dt_read_number(imap + i, 1); > > > >+ if (dt_raw_irq.controller != dt_interrupt_controller) { > > Coding style. > > >+ /* If this is not the main interrupt controller, we assume > >+ * it's part of a bus-bridge and ignore remapping IRQs for it. > >+ * Xen only supports one interrupt controller at the moment. */ > > This comment is not true for every case. The interrupt controller may not be > part of the bus-bridge. > > One platform with multiple interrupt controller, I expect to always see one > main controller, the others would be connected to the main one. So, the > interrupt will have no meaning for the GIC. > > >+ imap += pintsize; > >+ imaplen -= pintsize; > > I would add a dt_printk to mention that the IRQ has been skipped. > > >+ continue; > >+ } > > Please send a formal patch to the ML. What would be the drawback if this > patch doesn't reach Xen 4.7? I.e Are PCI devices necessary to run the > platform? No, they are not necessary. We have a few options. 1. If we do nothing, the ZynqMP boards with PCIe support won't even boot dom0. 2. A safe option is to disable the PCIe node in the ZynqMP platform. That'll boot dom0 but PCIe won't be functional. 3. If we get something like this patch in, we'll get dom0 to boot with PCIe support hopefully working in dom0. Thanks for the review. I'll spin a proper patch and we can discuss/consider if it's worth merging for 4.7 or if we go with option nr 2. Best regards, Edgar
On 16/05/16 14:41, Edgar E. Iglesias wrote: > On Mon, May 16, 2016 at 02:20:24PM +0100, Julien Grall wrote: > No, they are not necessary. We have a few options. > > 1. If we do nothing, the ZynqMP boards with PCIe support won't even boot > dom0. > > 2. A safe option is to disable the PCIe node in the ZynqMP platform. > That'll boot dom0 but PCIe won't be functional. > > 3. If we get something like this patch in, we'll get dom0 to boot with > PCIe support hopefully working in dom0. > > Thanks for the review. I'll spin a proper patch and we can discuss/consider > if it's worth merging for 4.7 or if we go with option nr 2. Please mention the options in the patch, and if possible the pros/cons. Regards,
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 0ed86a7..68cb162 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -1176,6 +1176,15 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, for ( i = 0; i < pintsize; i++ ) dt_raw_irq.specifier[i] = dt_read_number(imap + i, 1); + if (dt_raw_irq.controller != dt_interrupt_controller) { + /* If this is not the main interrupt controller, we assume + * it's part of a bus-bridge and ignore remapping IRQs for it. + * Xen only supports one interrupt controller at the moment. */ + imap += pintsize; + imaplen -= pintsize; + continue; + } > > >