| Message ID | alpine.DEB.2.21.2102081544230.8948@sstabellini-ThinkPad-T480s (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: workaround missing device_type property in pci/pcie nodes | expand |
Hi Stefano, > On 8 Feb 2021, at 23:56, Stefano Stabellini <sstabellini@kernel.org> wrote: > > PCI buses differ from default buses in a few important ways, so it is > important to detect them properly. Normally, PCI buses are expected to > have the following property: > > device_type = "pci" > > In reality, it is not always the case. To handle PCI bus nodes that > don't have the device_type property, also consider the node name: if the > node name is "pcie" or "pci" then consider the bus as a PCI bus. > > This commit is based on the Linux kernel commit > d1ac0002dd29 "of: address: Work around missing device_type property in > pcie nodes". > > This fixes Xen boot on RPi4. We are really handling here a wrong device-tree bug that could easily be fixed by the user. We should at least mention in the commit message that this is a workaround to solve RPi4 buggy device tree. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index 18825e333e..f1a96a3b90 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -563,14 +563,28 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr) > * PCI bus specific translator > */ > > +static bool_t dt_node_is_pci(const struct dt_device_node *np) > +{ > + bool is_pci = !strcmp(np->name, "pcie") || !strcmp(np->name, "pci"); The Linux commit is a bit more restrictive and only does that for “pcie”. Any reason why you also want to have this workaround done also for “pci” ? Cheers Bertrand > + > + if (is_pci) > + printk(XENLOG_WARNING "%s: Missing device_type\n", np->full_name); > + > + return is_pci; > +} > + > static bool_t dt_bus_pci_match(const struct dt_device_node *np) > { > /* > * "pciex" is PCI Express "vci" is for the /chaos bridge on 1st-gen PCI > * powermacs "ht" is hypertransport > + * > + * If none of the device_type match, and that the node name is > + * "pcie" or "pci", accept the device as PCI (with a warning). > */ > return !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") || > - !strcmp(np->type, "vci") || !strcmp(np->type, "ht"); > + !strcmp(np->type, "vci") || !strcmp(np->type, "ht") || > + dt_node_is_pci(np); > } > > static void dt_bus_pci_count_cells(const struct dt_device_node *np, >
On Tue, 9 Feb 2021, Bertrand Marquis wrote: > Hi Stefano, > > > On 8 Feb 2021, at 23:56, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > PCI buses differ from default buses in a few important ways, so it is > > important to detect them properly. Normally, PCI buses are expected to > > have the following property: > > > > device_type = "pci" > > > > In reality, it is not always the case. To handle PCI bus nodes that > > don't have the device_type property, also consider the node name: if the > > node name is "pcie" or "pci" then consider the bus as a PCI bus. > > > > This commit is based on the Linux kernel commit > > d1ac0002dd29 "of: address: Work around missing device_type property in > > pcie nodes". > > > > This fixes Xen boot on RPi4. > > We are really handling here a wrong device-tree bug that could easily be fixed > by the user. > We should at least mention in the commit message that this is a workaround > to solve RPi4 buggy device tree. Not sure if it can be "easily" fixed by the user -- it took me a few hours to figure out what the problem was, and I know Xen and device tree pretty well :-) Yes it would be good to have a link to the discussion in the commit message, using the Link tag. It could be done on commit, or I can add it to the next version. Link: https://lore.kernel.org/xen-devel/YBmQQ3Tzu++AadKx@mattapan.m5p.com/ > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > > index 18825e333e..f1a96a3b90 100644 > > --- a/xen/common/device_tree.c > > +++ b/xen/common/device_tree.c > > @@ -563,14 +563,28 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr) > > * PCI bus specific translator > > */ > > > > +static bool_t dt_node_is_pci(const struct dt_device_node *np) > > +{ > > + bool is_pci = !strcmp(np->name, "pcie") || !strcmp(np->name, "pci"); > > The Linux commit is a bit more restrictive and only does that for “pcie”. > Any reason why you also want to have this workaround done also for “pci” ? Yes, that's because in our case the offending node is named "pci" not "pcie" so the original Linux commit wouldn't cover it. > > + > > + if (is_pci) > > + printk(XENLOG_WARNING "%s: Missing device_type\n", np->full_name); > > + > > + return is_pci; > > +} > > + > > static bool_t dt_bus_pci_match(const struct dt_device_node *np) > > { > > /* > > * "pciex" is PCI Express "vci" is for the /chaos bridge on 1st-gen PCI > > * powermacs "ht" is hypertransport > > + * > > + * If none of the device_type match, and that the node name is > > + * "pcie" or "pci", accept the device as PCI (with a warning). > > */ > > return !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") || > > - !strcmp(np->type, "vci") || !strcmp(np->type, "ht"); > > + !strcmp(np->type, "vci") || !strcmp(np->type, "ht") || > > + dt_node_is_pci(np); > > } > > > > static void dt_bus_pci_count_cells(const struct dt_device_node *np, > > > >
Hi Stefano, On 09/02/2021 17:47, Stefano Stabellini wrote: > On Tue, 9 Feb 2021, Bertrand Marquis wrote: >> Hi Stefano, >> >>> On 8 Feb 2021, at 23:56, Stefano Stabellini <sstabellini@kernel.org> wrote: >>> >>> PCI buses differ from default buses in a few important ways, so it is >>> important to detect them properly. Normally, PCI buses are expected to >>> have the following property: >>> >>> device_type = "pci" >>> >>> In reality, it is not always the case. To handle PCI bus nodes that >>> don't have the device_type property, also consider the node name: if the >>> node name is "pcie" or "pci" then consider the bus as a PCI bus. >>> >>> This commit is based on the Linux kernel commit >>> d1ac0002dd29 "of: address: Work around missing device_type property in >>> pcie nodes". >>> >>> This fixes Xen boot on RPi4. I am a bit confused with this sentence... How did you manage to boot Xen on RPi4 before hand? >> >> We are really handling here a wrong device-tree bug that could easily be fixed >> by the user. >> We should at least mention in the commit message that this is a workaround >> to solve RPi4 buggy device tree. > > Not sure if it can be "easily" fixed by the user -- it took me a few > hours to figure out what the problem was, and I know Xen and device tree > pretty well :-) > > Yes it would be good to have a link to the discussion in the commit > message, using the Link tag. It could be done on commit, or I can add it > to the next version. A summary of the discussion would be useful in the commit message so a reader can easily make the connection between the Linux commit and the Xen one. > > Link: https://lore.kernel.org/xen-devel/YBmQQ3Tzu++AadKx@mattapan.m5p.com/ > > >>> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >>> >>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >>> index 18825e333e..f1a96a3b90 100644 >>> --- a/xen/common/device_tree.c >>> +++ b/xen/common/device_tree.c >>> @@ -563,14 +563,28 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr) >>> * PCI bus specific translator >>> */ >>> >>> +static bool_t dt_node_is_pci(const struct dt_device_node *np) >>> +{ >>> + bool is_pci = !strcmp(np->name, "pcie") || !strcmp(np->name, "pci"); >> >> The Linux commit is a bit more restrictive and only does that for “pcie”. >> Any reason why you also want to have this workaround done also for “pci” ? > > Yes, that's because in our case the offending node is named "pci" not > "pcie" so the original Linux commit wouldn't cover it. > > >>> + >>> + if (is_pci) >>> + printk(XENLOG_WARNING "%s: Missing device_type\n", np->full_name); >>> + >>> + return is_pci; >>> +} >>> + >>> static bool_t dt_bus_pci_match(const struct dt_device_node *np) >>> { >>> /* >>> * "pciex" is PCI Express "vci" is for the /chaos bridge on 1st-gen PCI >>> * powermacs "ht" is hypertransport >>> + * >>> + * If none of the device_type match, and that the node name is >>> + * "pcie" or "pci", accept the device as PCI (with a warning). >>> */ >>> return !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") || >>> - !strcmp(np->type, "vci") || !strcmp(np->type, "ht"); >>> + !strcmp(np->type, "vci") || !strcmp(np->type, "ht") || >>> + dt_node_is_pci(np); >>> } >>> >>> static void dt_bus_pci_count_cells(const struct dt_device_node *np, >>> >>
On Tue, 9 Feb 2021, Julien Grall wrote: > Hi Stefano, > > On 09/02/2021 17:47, Stefano Stabellini wrote: > > On Tue, 9 Feb 2021, Bertrand Marquis wrote: > > > Hi Stefano, > > > > > > > On 8 Feb 2021, at 23:56, Stefano Stabellini <sstabellini@kernel.org> > > > > wrote: > > > > > > > > PCI buses differ from default buses in a few important ways, so it is > > > > important to detect them properly. Normally, PCI buses are expected to > > > > have the following property: > > > > > > > > device_type = "pci" > > > > > > > > In reality, it is not always the case. To handle PCI bus nodes that > > > > don't have the device_type property, also consider the node name: if the > > > > node name is "pcie" or "pci" then consider the bus as a PCI bus. > > > > > > > > This commit is based on the Linux kernel commit > > > > d1ac0002dd29 "of: address: Work around missing device_type property in > > > > pcie nodes". > > > > > > > > This fixes Xen boot on RPi4. > I am a bit confused with this sentence... How did you manage to boot Xen on > RPi4 before hand? The older rpi kernel that I was using didn't have the problematic pci node in device tree at all. > > > We are really handling here a wrong device-tree bug that could easily be > > > fixed > > > by the user. > > > We should at least mention in the commit message that this is a workaround > > > to solve RPi4 buggy device tree. > > > > Not sure if it can be "easily" fixed by the user -- it took me a few > > hours to figure out what the problem was, and I know Xen and device tree > > pretty well :-) > > > > Yes it would be good to have a link to the discussion in the commit > > message, using the Link tag. It could be done on commit, or I can add it > > to the next version. > > A summary of the discussion would be useful in the commit message so a reader > can easily make the connection between the Linux commit and the Xen one. OK, good idea > > > > Link: https://lore.kernel.org/xen-devel/YBmQQ3Tzu++AadKx@mattapan.m5p.com/ > > > > > > > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > > > > > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > > > > index 18825e333e..f1a96a3b90 100644 > > > > --- a/xen/common/device_tree.c > > > > +++ b/xen/common/device_tree.c > > > > @@ -563,14 +563,28 @@ static unsigned int dt_bus_default_get_flags(const > > > > __be32 *addr) > > > > * PCI bus specific translator > > > > */ > > > > > > > > +static bool_t dt_node_is_pci(const struct dt_device_node *np) > > > > +{ > > > > + bool is_pci = !strcmp(np->name, "pcie") || !strcmp(np->name, > > > > "pci"); > > > > > > The Linux commit is a bit more restrictive and only does that for “pcie”. > > > Any reason why you also want to have this workaround done also for “pci” ? > > > > Yes, that's because in our case the offending node is named "pci" not > > "pcie" so the original Linux commit wouldn't cover it. > > > > > > > > + > > > > + if (is_pci) > > > > + printk(XENLOG_WARNING "%s: Missing device_type\n", > > > > np->full_name); > > > > + > > > > + return is_pci; > > > > +} > > > > + > > > > static bool_t dt_bus_pci_match(const struct dt_device_node *np) > > > > { > > > > /* > > > > * "pciex" is PCI Express "vci" is for the /chaos bridge on 1st-gen > > > > PCI > > > > * powermacs "ht" is hypertransport > > > > + * > > > > + * If none of the device_type match, and that the node name is > > > > + * "pcie" or "pci", accept the device as PCI (with a warning). > > > > */ > > > > return !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") || > > > > - !strcmp(np->type, "vci") || !strcmp(np->type, "ht"); > > > > + !strcmp(np->type, "vci") || !strcmp(np->type, "ht") || > > > > + dt_node_is_pci(np); > > > > } > > > > > > > > static void dt_bus_pci_count_cells(const struct dt_device_node *np,
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 18825e333e..f1a96a3b90 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -563,14 +563,28 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr) * PCI bus specific translator */ +static bool_t dt_node_is_pci(const struct dt_device_node *np) +{ + bool is_pci = !strcmp(np->name, "pcie") || !strcmp(np->name, "pci"); + + if (is_pci) + printk(XENLOG_WARNING "%s: Missing device_type\n", np->full_name); + + return is_pci; +} + static bool_t dt_bus_pci_match(const struct dt_device_node *np) { /* * "pciex" is PCI Express "vci" is for the /chaos bridge on 1st-gen PCI * powermacs "ht" is hypertransport + * + * If none of the device_type match, and that the node name is + * "pcie" or "pci", accept the device as PCI (with a warning). */ return !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") || - !strcmp(np->type, "vci") || !strcmp(np->type, "ht"); + !strcmp(np->type, "vci") || !strcmp(np->type, "ht") || + dt_node_is_pci(np); } static void dt_bus_pci_count_cells(const struct dt_device_node *np,
PCI buses differ from default buses in a few important ways, so it is important to detect them properly. Normally, PCI buses are expected to have the following property: device_type = "pci" In reality, it is not always the case. To handle PCI bus nodes that don't have the device_type property, also consider the node name: if the node name is "pcie" or "pci" then consider the bus as a PCI bus. This commit is based on the Linux kernel commit d1ac0002dd29 "of: address: Work around missing device_type property in pcie nodes". This fixes Xen boot on RPi4. Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>