Hi all,
MIPS specific code can be removed from driver and put into ralink mt7621
instead which is a more accurate place to do this. To make this possible
we need to have access to 'bridge->windows' in 'pcibios_root_bridge_prepare()'
which has been implemented for ralink mt7621 platform (there is no real
need to implement this for any other platforms since those ones haven't got
I/O coherency units). This also allow us to properly enable this driver to
completely be enabled for COMPILE_TEST. This patchset appoarch:
- Move windows list splice in 'pci_register_host_bridge()' after function
'pcibios_root_bridge_prepare()' is called.
- Implement 'pcibios_root_bridge_prepare()' for ralink mt7621.
- Avoid custom MIPs code in pcie-mt7621 driver.
- Add missing 'MODULE_LICENSE()' to pcie-mt7621 driver to avoid compile test
module compilation to complain (already sent patch from Yanteng Si that
I have rewrite commit message and long description a bit.
- Remove MIPS conditional code from Kconfig.
This patchset also fix some errors reported by Kernel Test Robot about
implicit mips functions used in driver code and fix errors in driver when
is compiled as a module [1] (mips:allmodconfig).
There was an ongoing discussion about this here [0] but I preferred to send
my proposal for better review and understanding:
[0]: https://lore.kernel.org/linux-mips/CAMhs-H8ShoaYiFOOzJaGC68nZz=V365RXN_Kjuj=fPFENGJiiw@mail.gmail.com/T/#t
[1]: https://lkml.org/lkml/2021/11/14/436
Thanks in advance for your time.
Best regards,
Sergio Paracuellos
Sergio Paracuellos (5):
PCI: let 'pcibios_root_bridge_prepare()' access to 'bridge->windows'
MIPS: ralink: implement 'pcibios_root_bridge_prepare()'
PCI: mt7621: avoid custom MIPS code in driver code
PCI: mt7621: Add missing 'MODULE_LICENSE()' definition
PCI: mt7621: Kconfig: completely enable driver for 'COMPILE_TEST'
arch/mips/ralink/mt7621.c | 30 +++++++++++++++++++++
drivers/pci/controller/Kconfig | 2 +-
drivers/pci/controller/pcie-mt7621.c | 39 ++--------------------------
drivers/pci/probe.c | 4 +--
4 files changed, 35 insertions(+), 40 deletions(-)
--
2.33.0
Driver code is setting up MIPS specific I/O coherency units addresses config.
This MIPS specific thing has been moved to be done when PCI code call the
'pcibios_root_bridge_prepare()' function which has been implemented for MIPS
ralink mt7621 platform. Hence, remove MIPS specific code from driver code.
After this changes there is also no need to add any MIPS specific includes
to avoid some errors reported by Kernet Tets Robot with W=1 builds.
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Sergio Paracuellos <[email protected]>
---
drivers/pci/controller/pcie-mt7621.c | 37 ----------------------------
1 file changed, 37 deletions(-)
diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
index b60dfb45ef7b..9cf541f5de9c 100644
--- a/drivers/pci/controller/pcie-mt7621.c
+++ b/drivers/pci/controller/pcie-mt7621.c
@@ -208,37 +208,6 @@ static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
reset_control_assert(port->pcie_rst);
}
-static int setup_cm_memory_region(struct pci_host_bridge *host)
-{
- struct mt7621_pcie *pcie = pci_host_bridge_priv(host);
- struct device *dev = pcie->dev;
- struct resource_entry *entry;
- resource_size_t mask;
-
- entry = resource_list_first_type(&host->windows, IORESOURCE_MEM);
- if (!entry) {
- dev_err(dev, "cannot get memory resource\n");
- return -EINVAL;
- }
-
- if (mips_cps_numiocu(0)) {
- /*
- * FIXME: hardware doesn't accept mask values with 1s after
- * 0s (e.g. 0xffef), so it would be great to warn if that's
- * about to happen
- */
- mask = ~(entry->res->end - entry->res->start);
-
- write_gcr_reg1_base(entry->res->start);
- write_gcr_reg1_mask(mask | CM_GCR_REGn_MASK_CMTGT_IOCU0);
- dev_info(dev, "PCI coherence region base: 0x%08llx, mask/settings: 0x%08llx\n",
- (unsigned long long)read_gcr_reg1_base(),
- (unsigned long long)read_gcr_reg1_mask());
- }
-
- return 0;
-}
-
static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
struct device_node *node,
int slot)
@@ -557,12 +526,6 @@ static int mt7621_pci_probe(struct platform_device *pdev)
goto remove_resets;
}
- err = setup_cm_memory_region(bridge);
- if (err) {
- dev_err(dev, "error setting up iocu mem regions\n");
- goto remove_resets;
- }
-
return mt7621_pcie_register_host(bridge);
remove_resets:
--
2.33.0
When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
already available. However this windows are being moved temporarily from
there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
move this windows movement after call this function. This is interesting for
MIPS ralink mt7621 platform to be able to properly set I/O coherence units
with this information and avoid custom MIPs code in generic PCIe controller
drivers.
Signed-off-by: Sergio Paracuellos <[email protected]>
---
drivers/pci/probe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 087d3658f75c..372a70efccc6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -898,8 +898,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
bridge->bus = bus;
- /* Temporarily move resources off the list */
- list_splice_init(&bridge->windows, &resources);
bus->sysdata = bridge->sysdata;
bus->ops = bridge->ops;
bus->number = bus->busn_res.start = bridge->busnr;
@@ -925,6 +923,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
if (err)
goto free;
+ /* Temporarily move resources off the list */
+ list_splice_init(&bridge->windows, &resources);
err = device_add(&bridge->dev);
if (err) {
put_device(&bridge->dev);
--
2.33.0
PCI core code call 'pcibios_root_bridge_prepare()' function inside function
'pci_register_host_bridge()'. This point is very good way to properly enter
into this MIPS ralink specific code to properly setup I/O coherency units
with PCI memory addresses.
Signed-off-by: Sergio Paracuellos <[email protected]>
---
arch/mips/ralink/mt7621.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/arch/mips/ralink/mt7621.c b/arch/mips/ralink/mt7621.c
index bd71f5b14238..7649416c1cd7 100644
--- a/arch/mips/ralink/mt7621.c
+++ b/arch/mips/ralink/mt7621.c
@@ -10,6 +10,7 @@
#include <linux/slab.h>
#include <linux/sys_soc.h>
#include <linux/memblock.h>
+#include <linux/pci.h>
#include <asm/bootinfo.h>
#include <asm/mipsregs.h>
@@ -22,6 +23,35 @@
static void *detect_magic __initdata = detect_memory_region;
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+ struct resource_entry *entry;
+ resource_size_t mask;
+
+ entry = resource_list_first_type(&bridge->windows, IORESOURCE_MEM);
+ if (!entry) {
+ pr_err("Cannot get memory resource\n");
+ return -EINVAL;
+ }
+
+ if (mips_cps_numiocu(0)) {
+ /*
+ * FIXME: hardware doesn't accept mask values with 1s after
+ * 0s (e.g. 0xffef), so it would be great to warn if that's
+ * about to happen
+ */
+ mask = ~(entry->res->end - entry->res->start);
+
+ write_gcr_reg1_base(entry->res->start);
+ write_gcr_reg1_mask(mask | CM_GCR_REGn_MASK_CMTGT_IOCU0);
+ pr_info("PCI coherence region base: 0x%08llx, mask/settings: 0x%08llx\n",
+ (unsigned long long)read_gcr_reg1_base(),
+ (unsigned long long)read_gcr_reg1_mask());
+ }
+
+ return 0;
+}
+
phys_addr_t mips_cpc_default_phys_base(void)
{
panic("Cannot detect cpc address");
--
2.33.0
MT7621 PCIe host controller driver can be built as a module but there is no
'MODULE_LICENSE()' specified in code, causing a build error due to missing
license information.
ERROR: modpost: missing MODULE_LICENSE() in drivers/pci/controller/pcie-mt7621.o
Fix this by adding 'MODULE_LICENSE()' to the driver.
Fixes: 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
Signed-off-by: Yanteng Si <[email protected]>
Signed-off-by: Sergio Paracuellos <[email protected]>
---
drivers/pci/controller/pcie-mt7621.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
index 9cf541f5de9c..a120a61ede07 100644
--- a/drivers/pci/controller/pcie-mt7621.c
+++ b/drivers/pci/controller/pcie-mt7621.c
@@ -561,3 +561,5 @@ static struct platform_driver mt7621_pci_driver = {
},
};
builtin_platform_driver(mt7621_pci_driver);
+
+MODULE_LICENSE("GPL v2");
--
2.33.0
Since all MIPS specific code has been moved from the controller driver side
there is no more stoppers to enable the driver to be completely enable for
'COMPILE_TEST'. Hence, remove MIPS conditional statement.
Signed-off-by: Sergio Paracuellos <[email protected]>
---
drivers/pci/controller/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index e917bb3652bb..105ec7dcccc9 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -332,7 +332,7 @@ config PCIE_APPLE
config PCIE_MT7621
tristate "MediaTek MT7621 PCIe Controller"
- depends on (RALINK && SOC_MT7621) || (MIPS && COMPILE_TEST)
+ depends on (RALINK && SOC_MT7621) || COMPILE_TEST
select PHY_MT7621_PCI
default SOC_MT7621
help
--
2.33.0
Hi Sergio and Yanteng,
Thank you for taking care of this!
> MT7620 PCIe host controller driver can be built as a module but there is no
> 'MODULE_LICENSE()' specified in code, causing a build error due to missing
> license information.
>
> ERROR: modpost: missing MODULE_LICENSE() in drivers/pci/controller/pcie-mt7621.o
>
> Fix this by adding 'MODULE_LICENSE()' to the driver.
>
> Fixes: 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> Signed-off-by: Yanteng Si <[email protected]>
> Signed-off-by: Sergio Paracuellos <[email protected]>
> ---
> drivers/pci/controller/pcie-mt7621.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> index 9cf541f5de9c..a120a61ede07 100644
> --- a/drivers/pci/controller/pcie-mt7621.c
> +++ b/drivers/pci/controller/pcie-mt7621.c
> @@ -561,3 +561,5 @@ static struct platform_driver mt7621_pci_driver = {
> },
> };
> builtin_platform_driver(mt7621_pci_driver);
> +
> +MODULE_LICENSE("GPL v2");
A question here about the builtin_platform_driver() use in this driver,
especially since it's set as tristate in Kconfig, thus I am not sure if
using builtin_platform_driver() over module_platform_driver() is correct?
Unless this is more because you need to reply on device_initcall() for the
driver to properly initialise?
Otherwise,
Reviewed-by: Krzysztof Wilczyński <[email protected]>
Krzysztof
On Mon, Nov 15, 2021 at 1:44 PM Krzysztof Wilczyński <[email protected]> wrote:
> > MT7620 PCIe host controller driver can be built as a module but there is no
> > 'MODULE_LICENSE()' specified in code, causing a build error due to missing
> > license information.
> >
> > ERROR: modpost: missing MODULE_LICENSE() in drivers/pci/controller/pcie-mt7621.o
> >
> > Fix this by adding 'MODULE_LICENSE()' to the driver.
> >
> > Fixes: 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > Signed-off-by: Yanteng Si <[email protected]>
> > Signed-off-by: Sergio Paracuellos <[email protected]>
> > ---
> > drivers/pci/controller/pcie-mt7621.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> > index 9cf541f5de9c..a120a61ede07 100644
> > --- a/drivers/pci/controller/pcie-mt7621.c
> > +++ b/drivers/pci/controller/pcie-mt7621.c
> > @@ -561,3 +561,5 @@ static struct platform_driver mt7621_pci_driver = {
> > },
> > };
> > builtin_platform_driver(mt7621_pci_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
>
> A question here about the builtin_platform_driver() use in this driver,
> especially since it's set as tristate in Kconfig, thus I am not sure if
> using builtin_platform_driver() over module_platform_driver() is correct?
>
> Unless this is more because you need to reply on device_initcall() for the
> driver to properly initialise?
builtin_platform_driver() does the right thing for loadable modules that
have no module-unload and are not intended to be removable.
This is often use for PCI drivers, but after Rob reworked this code a while
back, it should actually be possible to reliably remove and reload PCI
host bridge drivers, and it would be good to eventually lift the restriction
here as well.
Arnd
Hi Arnd,
On Mon, Nov 15, 2021 at 2:00 PM Arnd Bergmann <[email protected]> wrote:
>
> On Mon, Nov 15, 2021 at 1:44 PM Krzysztof Wilczyński <[email protected]> wrote:
> > > MT7620 PCIe host controller driver can be built as a module but there is no
> > > 'MODULE_LICENSE()' specified in code, causing a build error due to missing
> > > license information.
> > >
> > > ERROR: modpost: missing MODULE_LICENSE() in drivers/pci/controller/pcie-mt7621.o
> > >
> > > Fix this by adding 'MODULE_LICENSE()' to the driver.
> > >
> > > Fixes: 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > Signed-off-by: Yanteng Si <[email protected]>
> > > Signed-off-by: Sergio Paracuellos <[email protected]>
> > > ---
> > > drivers/pci/controller/pcie-mt7621.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> > > index 9cf541f5de9c..a120a61ede07 100644
> > > --- a/drivers/pci/controller/pcie-mt7621.c
> > > +++ b/drivers/pci/controller/pcie-mt7621.c
> > > @@ -561,3 +561,5 @@ static struct platform_driver mt7621_pci_driver = {
> > > },
> > > };
> > > builtin_platform_driver(mt7621_pci_driver);
> > > +
> > > +MODULE_LICENSE("GPL v2");
> >
> > A question here about the builtin_platform_driver() use in this driver,
> > especially since it's set as tristate in Kconfig, thus I am not sure if
> > using builtin_platform_driver() over module_platform_driver() is correct?
> >
> > Unless this is more because you need to reply on device_initcall() for the
> > driver to properly initialise?
>
> builtin_platform_driver() does the right thing for loadable modules that
> have no module-unload and are not intended to be removable.
Yes, this is the current state of this controller driver and the
reason for 'builtin_platform_driver()' being used.
>
> This is often use for PCI drivers, but after Rob reworked this code a while
> back, it should actually be possible to reliably remove and reload PCI
> host bridge drivers, and it would be good to eventually lift the restriction
> here as well.
I see. Thanks for letting me know. I will search for a way to
accomplish this but that will be a different patch series.
Best regards,
Sergio Paracuellos
>
> Arnd
On Mon, Nov 15, 2021 at 2:51 PM Sergio Paracuellos
<[email protected]> wrote:
> On Mon, Nov 15, 2021 at 2:00 PM Arnd Bergmann <[email protected]> wrote:
> > On Mon, Nov 15, 2021 at 1:44 PM Krzysztof Wilczyński <[email protected]> wrote:
> > This is often use for PCI drivers, but after Rob reworked this code a while
> > back, it should actually be possible to reliably remove and reload PCI
> > host bridge drivers, and it would be good to eventually lift the restriction
> > here as well.
>
> I see. Thanks for letting me know. I will search for a way to
> accomplish this but that will be a different patch series.
Right, that is what I meant. I don't think it will be difficult, but
there is no point
intermixing it with your current work.
Arnd
Hi Arnd,
[...]
> > > builtin_platform_driver(mt7621_pci_driver);
> > > +
> > > +MODULE_LICENSE("GPL v2");
> >
> > A question here about the builtin_platform_driver() use in this driver,
> > especially since it's set as tristate in Kconfig, thus I am not sure if
> > using builtin_platform_driver() over module_platform_driver() is correct?
> >
> > Unless this is more because you need to reply on device_initcall() for the
> > driver to properly initialise?
>
> builtin_platform_driver() does the right thing for loadable modules that
> have no module-unload and are not intended to be removable.
>
> This is often use for PCI drivers, but after Rob reworked this code a while
> back, it should actually be possible to reliably remove and reload PCI
> host bridge drivers, and it would be good to eventually lift the restriction
> here as well.
Thank you for letting me know. Much appreciated. I assumed in the past
that with tristate in Kconfig the module_platform_driver() would be the
preferred route.
Krzysztof
Hello,
[...]
> ERROR: modpost: missing MODULE_LICENSE() in drivers/pci/controller/pcie-mt7621.o
>
> Fix this by adding 'MODULE_LICENSE()' to the driver.
To add for posterity, should someone stumble upon this in the future. Lack
of MODULE_LICENSE() used to be a warning, but that has been changed quite
recently in the following commit:
commit 1d6cd3929360 ("modpost: turn missing MODULE_LICENSE() into error")
Krzysztof
On Mon, Nov 15, 2021 at 08:08:04AM +0100, Sergio Paracuellos wrote:
> Hi all,
>
> MIPS specific code can be removed from driver and put into ralink mt7621
> instead which is a more accurate place to do this. To make this possible
> we need to have access to 'bridge->windows' in 'pcibios_root_bridge_prepare()'
> which has been implemented for ralink mt7621 platform (there is no real
> need to implement this for any other platforms since those ones haven't got
> I/O coherency units). This also allow us to properly enable this driver to
> completely be enabled for COMPILE_TEST. This patchset appoarch:
> - Move windows list splice in 'pci_register_host_bridge()' after function
> 'pcibios_root_bridge_prepare()' is called.
> - Implement 'pcibios_root_bridge_prepare()' for ralink mt7621.
> - Avoid custom MIPs code in pcie-mt7621 driver.
> - Add missing 'MODULE_LICENSE()' to pcie-mt7621 driver to avoid compile test
> module compilation to complain (already sent patch from Yanteng Si that
> I have rewrite commit message and long description a bit.
> - Remove MIPS conditional code from Kconfig.
>
> This patchset also fix some errors reported by Kernel Test Robot about
> implicit mips functions used in driver code and fix errors in driver when
> is compiled as a module [1] (mips:allmodconfig).
>
> There was an ongoing discussion about this here [0] but I preferred to send
> my proposal for better review and understanding:
so what's the plan with this patchset ? Going in as fix, probably via
pci tree ? Or is material for next release ? If the latter can we first
fix the allmodconfig by making the Kconfig symbol bool ?
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
On Wed, Nov 17, 2021 at 1:41 PM Thomas Bogendoerfer
<[email protected]> wrote:
>
> On Mon, Nov 15, 2021 at 08:08:04AM +0100, Sergio Paracuellos wrote:
> > Hi all,
> >
> > MIPS specific code can be removed from driver and put into ralink mt7621
> > instead which is a more accurate place to do this. To make this possible
> > we need to have access to 'bridge->windows' in 'pcibios_root_bridge_prepare()'
> > which has been implemented for ralink mt7621 platform (there is no real
> > need to implement this for any other platforms since those ones haven't got
> > I/O coherency units). This also allow us to properly enable this driver to
> > completely be enabled for COMPILE_TEST. This patchset appoarch:
> > - Move windows list splice in 'pci_register_host_bridge()' after function
> > 'pcibios_root_bridge_prepare()' is called.
> > - Implement 'pcibios_root_bridge_prepare()' for ralink mt7621.
> > - Avoid custom MIPs code in pcie-mt7621 driver.
> > - Add missing 'MODULE_LICENSE()' to pcie-mt7621 driver to avoid compile test
> > module compilation to complain (already sent patch from Yanteng Si that
> > I have rewrite commit message and long description a bit.
> > - Remove MIPS conditional code from Kconfig.
> >
> > This patchset also fix some errors reported by Kernel Test Robot about
> > implicit mips functions used in driver code and fix errors in driver when
> > is compiled as a module [1] (mips:allmodconfig).
> >
> > There was an ongoing discussion about this here [0] but I preferred to send
> > my proposal for better review and understanding:
>
> so what's the plan with this patchset ? Going in as fix, probably via
> pci tree ? Or is material for next release ? If the latter can we first
> fix the allmodconfig by making the Kconfig symbol bool ?
If the approach is considered valid I guess it should go as a fix to
avoid changing first to 'bool' the Kconfig symbol. If it is not a
valid approach I will send patches with a possible new requested
approach or just making the symbol bool and adding specific mips
includes to driver code to avoid mips implicit functions errors.
Best regards,
Sergio Paracuellos
>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea. [ RFC1925, 2.3 ]
[+cc Thierry]
In subject,
PCI: Let pcibios_root_bridge_prepare() access bridge->windows
On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote:
> When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
> already available. However this windows are being moved temporarily from
> there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
> move this windows movement after call this function. This is interesting for
> MIPS ralink mt7621 platform to be able to properly set I/O coherence units
> with this information and avoid custom MIPs code in generic PCIe controller
> drivers.
>
> Signed-off-by: Sergio Paracuellos <[email protected]>
> ---
> drivers/pci/probe.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 087d3658f75c..372a70efccc6 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -898,8 +898,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>
> bridge->bus = bus;
>
> - /* Temporarily move resources off the list */
> - list_splice_init(&bridge->windows, &resources);
Arnd added this with 37d6a0a6f470 ("PCI: Add
pci_register_host_bridge() interface") [1].
I can't remember why this was done, but we did go to some trouble to
move things around, so there must have been a good reason.
Arnd or Thierry, do you remember?
> bus->sysdata = bridge->sysdata;
> bus->ops = bridge->ops;
> bus->number = bus->busn_res.start = bridge->busnr;
> @@ -925,6 +923,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> if (err)
> goto free;
>
> + /* Temporarily move resources off the list */
> + list_splice_init(&bridge->windows, &resources);
> err = device_add(&bridge->dev);
> if (err) {
> put_device(&bridge->dev);
> --
> 2.33.0
>
[1] https://git.kernel.org/linus/37d6a0a6f470
s/avoid custom/Avoid custom/ in subject.
On Mon, Nov 15, 2021 at 08:08:07AM +0100, Sergio Paracuellos wrote:
> Driver code is setting up MIPS specific I/O coherency units addresses config.
> This MIPS specific thing has been moved to be done when PCI code call the
> 'pcibios_root_bridge_prepare()' function which has been implemented for MIPS
> ralink mt7621 platform. Hence, remove MIPS specific code from driver code.
> After this changes there is also no need to add any MIPS specific includes
> to avoid some errors reported by Kernet Tets Robot with W=1 builds.
s/this changes/this change/
s/Tets/Test/
The patch doesn't touch any #include lines, so I'm not sure what the
last sentence is telling us.
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Sergio Paracuellos <[email protected]>
> ---
> drivers/pci/controller/pcie-mt7621.c | 37 ----------------------------
> 1 file changed, 37 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> index b60dfb45ef7b..9cf541f5de9c 100644
> --- a/drivers/pci/controller/pcie-mt7621.c
> +++ b/drivers/pci/controller/pcie-mt7621.c
> @@ -208,37 +208,6 @@ static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> reset_control_assert(port->pcie_rst);
> }
>
> -static int setup_cm_memory_region(struct pci_host_bridge *host)
> -{
> - struct mt7621_pcie *pcie = pci_host_bridge_priv(host);
> - struct device *dev = pcie->dev;
> - struct resource_entry *entry;
> - resource_size_t mask;
> -
> - entry = resource_list_first_type(&host->windows, IORESOURCE_MEM);
> - if (!entry) {
> - dev_err(dev, "cannot get memory resource\n");
> - return -EINVAL;
> - }
> -
> - if (mips_cps_numiocu(0)) {
> - /*
> - * FIXME: hardware doesn't accept mask values with 1s after
> - * 0s (e.g. 0xffef), so it would be great to warn if that's
> - * about to happen
> - */
> - mask = ~(entry->res->end - entry->res->start);
> -
> - write_gcr_reg1_base(entry->res->start);
> - write_gcr_reg1_mask(mask | CM_GCR_REGn_MASK_CMTGT_IOCU0);
> - dev_info(dev, "PCI coherence region base: 0x%08llx, mask/settings: 0x%08llx\n",
> - (unsigned long long)read_gcr_reg1_base(),
> - (unsigned long long)read_gcr_reg1_mask());
> - }
> -
> - return 0;
> -}
> -
> static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
> struct device_node *node,
> int slot)
> @@ -557,12 +526,6 @@ static int mt7621_pci_probe(struct platform_device *pdev)
> goto remove_resets;
> }
>
> - err = setup_cm_memory_region(bridge);
> - if (err) {
> - dev_err(dev, "error setting up iocu mem regions\n");
> - goto remove_resets;
> - }
> -
> return mt7621_pcie_register_host(bridge);
>
> remove_resets:
> --
> 2.33.0
>
Hi Bjorn,
On Wed, Dec 1, 2021 at 7:16 PM Bjorn Helgaas <[email protected]> wrote:
>
> s/avoid custom/Avoid custom/ in subject.
Ok, I will change this and send v2 of this series after the PATCH 1
change in the series is clear and validated that it is an accepted way
to go.
>
> On Mon, Nov 15, 2021 at 08:08:07AM +0100, Sergio Paracuellos wrote:
> > Driver code is setting up MIPS specific I/O coherency units addresses config.
> > This MIPS specific thing has been moved to be done when PCI code call the
> > 'pcibios_root_bridge_prepare()' function which has been implemented for MIPS
> > ralink mt7621 platform. Hence, remove MIPS specific code from driver code.
> > After this changes there is also no need to add any MIPS specific includes
> > to avoid some errors reported by Kernet Tets Robot with W=1 builds.
>
> s/this changes/this change/
> s/Tets/Test/
Ups, true. Thanks :).
>
> The patch doesn't touch any #include lines, so I'm not sure what the
> last sentence is telling us.
Kernel test robot reported implicit declarations because of the use of
MIPS specific functions without explicit include added in driver code
[0].
After this change, this issue also disappears and that is why
'Reported-by' tag is added and this sentence included in the commit
message. Let me know the way you prefer me to write the commit message
to take into account this fact.
Thanks,
Sergio Paracuellos
[0]: https://lore.kernel.org/lkml/CAMhs-H_yugWd4v1OnBR8iqTVQS_T-S3pdrJbZq=MC646QSyb4Q@mail.gmail.com/T/
>
> > Reported-by: kernel test robot <[email protected]>
> > Signed-off-by: Sergio Paracuellos <[email protected]>
> > ---
> > drivers/pci/controller/pcie-mt7621.c | 37 ----------------------------
> > 1 file changed, 37 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> > index b60dfb45ef7b..9cf541f5de9c 100644
> > --- a/drivers/pci/controller/pcie-mt7621.c
> > +++ b/drivers/pci/controller/pcie-mt7621.c
> > @@ -208,37 +208,6 @@ static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> > reset_control_assert(port->pcie_rst);
> > }
> >
> > -static int setup_cm_memory_region(struct pci_host_bridge *host)
> > -{
> > - struct mt7621_pcie *pcie = pci_host_bridge_priv(host);
> > - struct device *dev = pcie->dev;
> > - struct resource_entry *entry;
> > - resource_size_t mask;
> > -
> > - entry = resource_list_first_type(&host->windows, IORESOURCE_MEM);
> > - if (!entry) {
> > - dev_err(dev, "cannot get memory resource\n");
> > - return -EINVAL;
> > - }
> > -
> > - if (mips_cps_numiocu(0)) {
> > - /*
> > - * FIXME: hardware doesn't accept mask values with 1s after
> > - * 0s (e.g. 0xffef), so it would be great to warn if that's
> > - * about to happen
> > - */
> > - mask = ~(entry->res->end - entry->res->start);
> > -
> > - write_gcr_reg1_base(entry->res->start);
> > - write_gcr_reg1_mask(mask | CM_GCR_REGn_MASK_CMTGT_IOCU0);
> > - dev_info(dev, "PCI coherence region base: 0x%08llx, mask/settings: 0x%08llx\n",
> > - (unsigned long long)read_gcr_reg1_base(),
> > - (unsigned long long)read_gcr_reg1_mask());
> > - }
> > -
> > - return 0;
> > -}
> > -
> > static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
> > struct device_node *node,
> > int slot)
> > @@ -557,12 +526,6 @@ static int mt7621_pci_probe(struct platform_device *pdev)
> > goto remove_resets;
> > }
> >
> > - err = setup_cm_memory_region(bridge);
> > - if (err) {
> > - dev_err(dev, "error setting up iocu mem regions\n");
> > - goto remove_resets;
> > - }
> > -
> > return mt7621_pcie_register_host(bridge);
> >
> > remove_resets:
> > --
> > 2.33.0
> >
For subject:
PCI: mt7621: Allow COMPILE_TEST for all arches
On Mon, Nov 15, 2021 at 08:08:09AM +0100, Sergio Paracuellos wrote:
> Since all MIPS specific code has been moved from the controller driver side
> there is no more stoppers to enable the driver to be completely enable for
> 'COMPILE_TEST'. Hence, remove MIPS conditional statement.
>
> Signed-off-by: Sergio Paracuellos <[email protected]>
> ---
> drivers/pci/controller/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index e917bb3652bb..105ec7dcccc9 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -332,7 +332,7 @@ config PCIE_APPLE
>
> config PCIE_MT7621
> tristate "MediaTek MT7621 PCIe Controller"
> - depends on (RALINK && SOC_MT7621) || (MIPS && COMPILE_TEST)
> + depends on (RALINK && SOC_MT7621) || COMPILE_TEST
> select PHY_MT7621_PCI
> default SOC_MT7621
> help
> --
> 2.33.0
>
On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote:
> [+cc Thierry]
>
> In subject,
>
> PCI: Let pcibios_root_bridge_prepare() access bridge->windows
>
> On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote:
> > When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
> > already available. However this windows are being moved temporarily from
> > there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
> > move this windows movement after call this function. This is interesting for
> > MIPS ralink mt7621 platform to be able to properly set I/O coherence units
> > with this information and avoid custom MIPs code in generic PCIe controller
> > drivers.
> >
> > Signed-off-by: Sergio Paracuellos <[email protected]>
> > ---
> > drivers/pci/probe.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 087d3658f75c..372a70efccc6 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -898,8 +898,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> >
> > bridge->bus = bus;
> >
> > - /* Temporarily move resources off the list */
> > - list_splice_init(&bridge->windows, &resources);
>
> Arnd added this with 37d6a0a6f470 ("PCI: Add
> pci_register_host_bridge() interface") [1].
>
> I can't remember why this was done, but we did go to some trouble to
> move things around, so there must have been a good reason.
>
> Arnd or Thierry, do you remember?
Nobody seems to remember, so I think we should go ahead and make this
change after the usual due diligence (audit the code between the old
site and the new site to look for any uses of bridge->windows).
I think this would be material for v5.17.
> > bus->sysdata = bridge->sysdata;
> > bus->ops = bridge->ops;
> > bus->number = bus->busn_res.start = bridge->busnr;
> > @@ -925,6 +923,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> > if (err)
> > goto free;
> >
> > + /* Temporarily move resources off the list */
> > + list_splice_init(&bridge->windows, &resources);
> > err = device_add(&bridge->dev);
> > if (err) {
> > put_device(&bridge->dev);
> > --
> > 2.33.0
> >
>
> [1] https://git.kernel.org/linus/37d6a0a6f470
On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote:
> When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
> already available. However this windows are being moved temporarily from
> there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
> move this windows movement after call this function. This is interesting for
> MIPS ralink mt7621 platform to be able to properly set I/O coherence units
> with this information and avoid custom MIPs code in generic PCIe controller
> drivers.
Oops, forgot to mention:
s/this windows/these windows/
s/MIPs/MIPS/
You can drop the single quote around function names, too; the "()" is
enough of a hint.
And s/PCI: let/PCI: Let/ in the subject.
On Wed, Dec 1, 2021 at 9:12 PM Bjorn Helgaas <[email protected]> wrote:
>
> For subject:
>
> PCI: mt7621: Allow COMPILE_TEST for all arches
Understood, thanks.
Best regards,
Sergio Paracuellos
>
> On Mon, Nov 15, 2021 at 08:08:09AM +0100, Sergio Paracuellos wrote:
> > Since all MIPS specific code has been moved from the controller driver side
> > there is no more stoppers to enable the driver to be completely enable for
> > 'COMPILE_TEST'. Hence, remove MIPS conditional statement.
> >
> > Signed-off-by: Sergio Paracuellos <[email protected]>
> > ---
> > drivers/pci/controller/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > index e917bb3652bb..105ec7dcccc9 100644
> > --- a/drivers/pci/controller/Kconfig
> > +++ b/drivers/pci/controller/Kconfig
> > @@ -332,7 +332,7 @@ config PCIE_APPLE
> >
> > config PCIE_MT7621
> > tristate "MediaTek MT7621 PCIe Controller"
> > - depends on (RALINK && SOC_MT7621) || (MIPS && COMPILE_TEST)
> > + depends on (RALINK && SOC_MT7621) || COMPILE_TEST
> > select PHY_MT7621_PCI
> > default SOC_MT7621
> > help
> > --
> > 2.33.0
> >
On Wed, Dec 1, 2021 at 9:24 PM Bjorn Helgaas <[email protected]> wrote:
> On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote:
> >
> > Arnd added this with 37d6a0a6f470 ("PCI: Add
> > pci_register_host_bridge() interface") [1].
> >
> > I can't remember why this was done, but we did go to some trouble to
> > move things around, so there must have been a good reason.
> >
> > Arnd or Thierry, do you remember?
>
> Nobody seems to remember, so I think we should go ahead and make this
> change after the usual due diligence (audit the code between the old
> site and the new site to look for any uses of bridge->windows).
>
> I think this would be material for v5.17.
Sorry I forgot to reply to your earlier mail. I think this is fine, as far as I
remember, the only reason the bridge windows are moved from the list
and added back one at a time was to preserve the exact orderorks.
We could probably even skip that step entirely and iterate throughing
that was there originally, to keep the behavior after a series of reworks.
We could probably even skip that step entirely and iterate through
bridge->windows instead of the local list to simplify this.
For Sergio's patch:
Acked-by: Arnd Bergmann <[email protected]>
On Wed, Dec 1, 2021 at 9:24 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote:
> > [+cc Thierry]
> >
> > In subject,
> >
> > PCI: Let pcibios_root_bridge_prepare() access bridge->windows
> >
> > On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote:
> > > When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
> > > already available. However this windows are being moved temporarily from
> > > there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
> > > move this windows movement after call this function. This is interesting for
> > > MIPS ralink mt7621 platform to be able to properly set I/O coherence units
> > > with this information and avoid custom MIPs code in generic PCIe controller
> > > drivers.
> > >
> > > Signed-off-by: Sergio Paracuellos <[email protected]>
> > > ---
> > > drivers/pci/probe.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 087d3658f75c..372a70efccc6 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -898,8 +898,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> > >
> > > bridge->bus = bus;
> > >
> > > - /* Temporarily move resources off the list */
> > > - list_splice_init(&bridge->windows, &resources);
> >
> > Arnd added this with 37d6a0a6f470 ("PCI: Add
> > pci_register_host_bridge() interface") [1].
> >
> > I can't remember why this was done, but we did go to some trouble to
> > move things around, so there must have been a good reason.
> >
> > Arnd or Thierry, do you remember?
>
> Nobody seems to remember, so I think we should go ahead and make this
> change after the usual due diligence (audit the code between the old
> site and the new site to look for any uses of bridge->windows).
It seems any user of the pcibios_root_bridge_prepare() does nothing
with 'bridge->windows'. But I don't get the point of passing around a
complete bridge pointer if windows are temporarily removed from there.
That is an incomplete bridge and after parsing windows from dts are
supposed to be there... What do you mean with 'audit the code between
the old and new site'?
>
> I think this would be material for v5.17.
Do you prefer me to parse dts again inside
pcibios_root_bridge_prepare() for ralink mt7621?. Not real sense since
'windows' should be already there, but it would be a way to get this
patchset added for v5.16. Something like (not tested yet but it should
work):
int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
{
resource_size_t mask;
struct device_node *np;
struct of_range range;
struct of_range_parser parser;
struct resource res;
np = of_find_compatible_node(NULL, NULL, "mediatek,mt7621-pci");
if (!np) {
pr_err("Cannot find pci node\n");
return -ENODEV;
}
if (of_range_parser_init(&parser, np)) {
pr_err("Error parsing resources\n");
of_node_put(np);
return -EINVAL;
}
for_each_of_range(&parser, &range) {
switch (range.flags & IORESOURCE_TYPE_BITS) {
case IORESOURCE_MEM:
res.start = range.cpu_addr;
res.end = range.cpu_addr + range.size - 1;
break;
}
}
if (mips_cps_numiocu(0)) {
mask = ~(res.end - res.start);
write_gcr_reg1_base(res.start);
write_gcr_reg1_mask(mask | CM_GCR_REGn_MASK_CMTGT_IOCU0);
pr_info("PCI coherence region base: 0x%08llx,
mask/settings: 0x%08llx\n",
(unsigned long long)read_gcr_reg1_base(),
(unsigned long long)read_gcr_reg1_mask());
}
return 0;
}
We can change this for v5.17 with the change in PCI core.
I have just seen Arnd's Acked-by for PATCH 1 while I was writing this,
so I am not sure if we can consider now this patchset as it is with
proposed changes for v5.16.
Thanks in advance for clarification.
Best regards,
Sergio Paracuellos
>
> > > bus->sysdata = bridge->sysdata;
> > > bus->ops = bridge->ops;
> > > bus->number = bus->busn_res.start = bridge->busnr;
> > > @@ -925,6 +923,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> > > if (err)
> > > goto free;
> > >
> > > + /* Temporarily move resources off the list */
> > > + list_splice_init(&bridge->windows, &resources);
> > > err = device_add(&bridge->dev);
> > > if (err) {
> > > put_device(&bridge->dev);
> > > --
> > > 2.33.0
> > >
> >
> > [1] https://git.kernel.org/linus/37d6a0a6f470
[+cc Guenter from other thread:
https://lore.kernel.org/r/[email protected]]
On Wed, Dec 01, 2021 at 09:56:22PM +0100, Sergio Paracuellos wrote:
> On Wed, Dec 1, 2021 at 9:24 PM Bjorn Helgaas <[email protected]> wrote:
> > On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote:
> > > [+cc Thierry]
> > >
> > > In subject,
> > >
> > > PCI: Let pcibios_root_bridge_prepare() access bridge->windows
> > >
> > > On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote:
> > > > When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
> > > > already available. However this windows are being moved temporarily from
> > > > there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
> > > > move this windows movement after call this function. This is interesting for
> > > > MIPS ralink mt7621 platform to be able to properly set I/O coherence units
> > > > with this information and avoid custom MIPs code in generic PCIe controller
> > > > drivers.
> > > >
> > > > Signed-off-by: Sergio Paracuellos <[email protected]>
> > > > ---
> > > > drivers/pci/probe.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 087d3658f75c..372a70efccc6 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -898,8 +898,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> > > >
> > > > bridge->bus = bus;
> > > >
> > > > - /* Temporarily move resources off the list */
> > > > - list_splice_init(&bridge->windows, &resources);
> > >
> > > Arnd added this with 37d6a0a6f470 ("PCI: Add
> > > pci_register_host_bridge() interface") [1].
> > >
> > > I can't remember why this was done, but we did go to some trouble to
> > > move things around, so there must have been a good reason.
> > >
> > > Arnd or Thierry, do you remember?
> >
> > Nobody seems to remember, so I think we should go ahead and make this
> > change after the usual due diligence (audit the code between the old
> > site and the new site to look for any uses of bridge->windows).
>
> It seems any user of the pcibios_root_bridge_prepare() does nothing
> with 'bridge->windows'. But I don't get the point of passing around a
> complete bridge pointer if windows are temporarily removed from there.
> That is an incomplete bridge and after parsing windows from dts are
> supposed to be there... What do you mean with 'audit the code between
> the old and new site'?
I mean "look at all the code that is run between the old site and the
new site to make sure that none of that code depends on
bridge->windows being temporarily emptied."
> > I think this would be material for v5.17.
>
> Do you prefer me to parse dts again inside
> pcibios_root_bridge_prepare() for ralink mt7621?. Not real sense since
> 'windows' should be already there, but it would be a way to get this
> patchset added for v5.16. Something like (not tested yet but it should
> work):
This is definitely too big for v5.16, regardless of which way you go.
For v5.16, the only thing that's practical is to avoid building as a
module. It'd be *nice* if it could be built as a module, but it is
not a requirement.
Bjorn