| Message ID | 20200424130847.328584-2-jiaxun.yang@flygoat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Loongson PCI Generic Driver | expand |
On Fri, Apr 24, 2020 at 8:09 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: > > There are some platforms that don't support I/O space remapping > like MIPS. However, our PCI code will try to remap iospace > unconditionally and reject io resources on these platforms. > > So we should remove I/O space remapping check and use a range > check instead on these platforms. > > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > -- > v4: Fix a typo in commit message. > v5: Commit message massage > --- > drivers/pci/of.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 81ceeaa6f1d5..36e8761b66c6 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -547,12 +547,21 @@ int pci_parse_request_of_pci_ranges(struct device *dev, > > switch (resource_type(res)) { > case IORESOURCE_IO: > +#if defined(PCI_IOBASE) && defined(CONFIG_MMU) We already have the same condition in pci_remap_iospace(). All this does is suppress a WARN. If a WARN is not desired, then change it. Perhaps just a single line rather than backtrace would be okay. Whether to WARN or not shouldn't be a decision for firmware code. Though really, if I/O space can never be supported, then it's an error in your DT to describe it. > err = devm_pci_remap_iospace(dev, res, iobase); > if (err) { > dev_warn(dev, "error %d: failed to map resource %pR\n", > err, res); > resource_list_destroy_entry(win); > } > +#else > + /* Simply check if IO is inside the range */ Why do you care if it's never used? > + if (res->end > IO_SPACE_LIMIT) { > + dev_warn(dev, "resource %pR out of the I/O range\n", > + res); > + resource_list_destroy_entry(win); > + } > +#endif > break; > case IORESOURCE_MEM: > res_valid |= !(res->flags & IORESOURCE_PREFETCH); > -- > 2.26.0.rc2 >
于 2020年4月25日 GMT+08:00 上午1:47:23, Rob Herring <robh+dt@kernel.org> 写到: >On Fri, Apr 24, 2020 at 8:09 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: >> >> There are some platforms that don't support I/O space remapping >> like MIPS. However, our PCI code will try to remap iospace >> unconditionally and reject io resources on these platforms. >> >> So we should remove I/O space remapping check and use a range >> check instead on these platforms. >> >> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> >> -- >> v4: Fix a typo in commit message. >> v5: Commit message massage >> --- >> drivers/pci/of.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/pci/of.c b/drivers/pci/of.c >> index 81ceeaa6f1d5..36e8761b66c6 100644 >> --- a/drivers/pci/of.c >> +++ b/drivers/pci/of.c >> @@ -547,12 +547,21 @@ int pci_parse_request_of_pci_ranges(struct device *dev, >> >> switch (resource_type(res)) { >> case IORESOURCE_IO: >> +#if defined(PCI_IOBASE) && defined(CONFIG_MMU) > >We already have the same condition in pci_remap_iospace(). All this >does is suppress a WARN. If a WARN is not desired, then change it. >Perhaps just a single line rather than backtrace would be okay. >Whether to WARN or not shouldn't be a decision for firmware code. > >Though really, if I/O space can never be supported, then it's an error >in your DT to describe it. In MIPS world, we do have inb/oub family of I/O functions to support I/O space. But we're not using remap_iospace as it's breaking some of our assumptions. We have our own inb/outb implementation. In that case, I think make a range check instead of remapping would be more practical. Thanks. > >> err = devm_pci_remap_iospace(dev, res, iobase); >> if (err) { >> dev_warn(dev, "error %d: failed to map resource %pR\n", >> err, res); >> resource_list_destroy_entry(win); >> } >> +#else >> + /* Simply check if IO is inside the range */ > >Why do you care if it's never used? > >> + if (res->end > IO_SPACE_LIMIT) { >> + dev_warn(dev, "resource %pR out of the I/O range\n", >> + res); >> + resource_list_destroy_entry(win); >> + } >> +#endif >> break; >> case IORESOURCE_MEM: >> res_valid |= !(res->flags & IORESOURCE_PREFETCH); >> -- >> 2.26.0.rc2 >>
On Fri, Apr 24, 2020 at 1:03 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: > > > > 于 2020年4月25日 GMT+08:00 上午1:47:23, Rob Herring <robh+dt@kernel.org> 写到: > >On Fri, Apr 24, 2020 at 8:09 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: > >> > >> There are some platforms that don't support I/O space remapping > >> like MIPS. However, our PCI code will try to remap iospace > >> unconditionally and reject io resources on these platforms. > >> > >> So we should remove I/O space remapping check and use a range > >> check instead on these platforms. > >> > >> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > >> -- > >> v4: Fix a typo in commit message. > >> v5: Commit message massage > >> --- > >> drivers/pci/of.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/drivers/pci/of.c b/drivers/pci/of.c > >> index 81ceeaa6f1d5..36e8761b66c6 100644 > >> --- a/drivers/pci/of.c > >> +++ b/drivers/pci/of.c > >> @@ -547,12 +547,21 @@ int pci_parse_request_of_pci_ranges(struct device *dev, > >> > >> switch (resource_type(res)) { > >> case IORESOURCE_IO: > >> +#if defined(PCI_IOBASE) && defined(CONFIG_MMU) > > > >We already have the same condition in pci_remap_iospace(). All this > >does is suppress a WARN. If a WARN is not desired, then change it. > >Perhaps just a single line rather than backtrace would be okay. > >Whether to WARN or not shouldn't be a decision for firmware code. > > > >Though really, if I/O space can never be supported, then it's an error > >in your DT to describe it. > > In MIPS world, we do have inb/oub family of I/O functions > to support I/O space. But we're not using remap_iospace as it's breaking > some of our assumptions. I suspect that's largely because most MIPS drivers pre-date the common iospace handling. Really MIPS should start using this. > We have our own inb/outb implementation. That's orthogonal to mapping the iospace. > In that case, I think make a range check instead of remapping would > be more practical. Not the kernel's job to validate the DT especially if you aren't using this data anywhere. Just remove the WARN, make it a single line print, or add !IS_ENABLED(CONFIG_MIPS) where the warning is. Rob
diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 81ceeaa6f1d5..36e8761b66c6 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -547,12 +547,21 @@ int pci_parse_request_of_pci_ranges(struct device *dev, switch (resource_type(res)) { case IORESOURCE_IO: +#if defined(PCI_IOBASE) && defined(CONFIG_MMU) err = devm_pci_remap_iospace(dev, res, iobase); if (err) { dev_warn(dev, "error %d: failed to map resource %pR\n", err, res); resource_list_destroy_entry(win); } +#else + /* Simply check if IO is inside the range */ + if (res->end > IO_SPACE_LIMIT) { + dev_warn(dev, "resource %pR out of the I/O range\n", + res); + resource_list_destroy_entry(win); + } +#endif break; case IORESOURCE_MEM: res_valid |= !(res->flags & IORESOURCE_PREFETCH);
There are some platforms that don't support I/O space remapping like MIPS. However, our PCI code will try to remap iospace unconditionally and reject io resources on these platforms. So we should remove I/O space remapping check and use a range check instead on these platforms. Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> -- v4: Fix a typo in commit message. v5: Commit message massage --- drivers/pci/of.c | 9 +++++++++ 1 file changed, 9 insertions(+)