[PATCH v2 0/3] MIPS: Fix build error ERROR: modpost:

[PATCH v2 0/3] MIPS: Fix build error ERROR: modpost:

[Yanteng Si]

v2:

* Pick Sevrgio's Acked-by tag to Patch 1/3;
* Rewrite prefix;
* Use EXPORT_SYMBOL_GPL();
* Add haojun's patch into my thread and rewrite commit message.(Patch 3/3)

note: These errors are generated in the *linux-next.git*.

https://lore.kernel.org/linux-mips/cover.1635333327.git.siyanteng@loongson.cn/T/#t

v1:

Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
the MT7621 PCIe host controller driver is built as a module but no MODULE_*() attributes
were specified, causing a build error due to missing license information. At the same time,
modpost complains once these drivers become modules.

ERROR: modpost: missing MODULE_LICENSE() in drivers/pci/controller/pcie-mt7621.o
ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!

Let's just fix them.

Wang Haojun (1):
  MIPS: Export board_be_handler to modules

Yanteng Si (2):
  MIPS: mt7621: Add MODULE_* macros to MT7621 PCIe host controller
    driver
  MIPS: Export mips_cm/cpc/gcr_* to modules

 arch/mips/kernel/mips-cm.c           | 5 +++++
 arch/mips/kernel/mips-cpc.c          | 1 +
 arch/mips/kernel/traps.c             | 1 +
 drivers/pci/controller/pcie-mt7621.c | 2 ++
 4 files changed, 9 insertions(+)

-- 
2.27.0

[PATCH v2 1/3] PCI: mt7621: Add MODULE_* macros to MT7621 PCIe host controller driver

[Yanteng Si]

Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
the MT7621 PCIe host controller driver is built as a module but no MODULE_*() attributes
were specified, 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 attributes to the driver.

Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
Acked-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 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 b60dfb45ef7b..668b737f86fb 100644
--- a/drivers/pci/controller/pcie-mt7621.c
+++ b/drivers/pci/controller/pcie-mt7621.c
@@ -598,3 +598,5 @@ static struct platform_driver mt7621_pci_driver = {
 	},
 };
 builtin_platform_driver(mt7621_pci_driver);
+
+MODULE_LICENSE("GPL v2");
\ No newline at end of file
-- 
2.27.0

[PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Yanteng Si]

Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
the MT7621 PCIe host controller driver is built as a module but modpost complains once these
drivers become modules.

ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!

Let's just export them.

Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
---
 arch/mips/kernel/mips-cm.c  | 5 +++++
 arch/mips/kernel/mips-cpc.c | 1 +
 2 files changed, 6 insertions(+)

diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c
index 90f1c3df1f0e..892258760bf3 100644
--- a/arch/mips/kernel/mips-cm.c
+++ b/arch/mips/kernel/mips-cm.c
@@ -12,8 +12,11 @@
 #include <asm/mipsregs.h>
 
 void __iomem *mips_gcr_base;
+EXPORT_SYMBOL_GPL(mips_gcr_base);
+
 void __iomem *mips_cm_l2sync_base;
 int mips_cm_is64;
+EXPORT_SYMBOL_GPL(mips_cm_is64);
 
 static char *cm2_tr[8] = {
 	"mem",	"gcr",	"gic",	"mmio",
@@ -353,6 +356,7 @@ void mips_cm_lock_other(unsigned int cluster, unsigned int core,
 	 */
 	mb();
 }
+EXPORT_SYMBOL_GPL(mips_cm_lock_other);
 
 void mips_cm_unlock_other(void)
 {
@@ -369,6 +373,7 @@ void mips_cm_unlock_other(void)
 
 	preempt_enable();
 }
+EXPORT_SYMBOL_GPL(mips_cm_unlock_other);
 
 void mips_cm_error_report(void)
 {
diff --git a/arch/mips/kernel/mips-cpc.c b/arch/mips/kernel/mips-cpc.c
index 8d2535123f11..26392d18729c 100644
--- a/arch/mips/kernel/mips-cpc.c
+++ b/arch/mips/kernel/mips-cpc.c
@@ -13,6 +13,7 @@
 #include <asm/mips-cps.h>
 
 void __iomem *mips_cpc_base;
+EXPORT_SYMBOL_GPL(mips_cpc_base);
 
 static DEFINE_PER_CPU_ALIGNED(spinlock_t, cpc_core_lock);
 
-- 
2.27.0

[PATCH v2 3/3] MIPS: traps: export a missing symbols to be able to use it from driver code

[Yanteng Si]

From: Wang Haojun <wanghaojun@loongson.cn>

Since commit 707a4cdf86e5 ("bus: brcmstb_gisb: Allow building as module")
the brcmstb_gisb bus driver is built as a module but modpost complains once
these drivers become modules.

ERROR: modpost: "board_be_handler" [drivers/bus/brcmstb_gisb.ko] undefined!

Let's just export it.

Signed-off-by: Wang Haojun <wanghaojun@loongson.cn>
Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
---
 arch/mips/kernel/traps.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 6f07362de5ce..409bcc755b89 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -104,6 +104,7 @@ extern void tlb_do_page_fault_0(void);
 
 void (*board_be_init)(void);
 int (*board_be_handler)(struct pt_regs *regs, int is_fixup);
+EXPORT_SYMBOL_GPL(board_be_handler);
 void (*board_nmi_handler_setup)(void);
 void (*board_ejtag_handler_setup)(void);
 void (*board_bind_eic_interrupt)(int irq, int regset);
-- 
2.27.0

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Sergio Paracuellos]

On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
>
> Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> drivers become modules.
>
> ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
>
> Let's just export them.
>
> Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> ---
>  arch/mips/kernel/mips-cm.c  | 5 +++++
>  arch/mips/kernel/mips-cpc.c | 1 +
>  2 files changed, 6 insertions(+)
>

Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Thomas Bogendoerfer]

On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> >
> > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > drivers become modules.
> >
> > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> >
> > Let's just export them.
> >
> > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > ---
> >  arch/mips/kernel/mips-cm.c  | 5 +++++
> >  arch/mips/kernel/mips-cpc.c | 1 +
> >  2 files changed, 6 insertions(+)
> >
> 
> Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

could we instead make the pcie-mt761 driver non modular ? Exporting
all MIPS specific stuff for just making an essential driver modular
doesn't IMHO make much sense.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Sergio Paracuellos]

Hi Thomas,

On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> > On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> > >
> > > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > > drivers become modules.
> > >
> > > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > >
> > > Let's just export them.
> > >
> > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > > ---
> > >  arch/mips/kernel/mips-cm.c  | 5 +++++
> > >  arch/mips/kernel/mips-cpc.c | 1 +
> > >  2 files changed, 6 insertions(+)
> > >
> >
> > Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>
> could we instead make the pcie-mt761 driver non modular ? Exporting
> all MIPS specific stuff for just making an essential driver modular
> doesn't IMHO make much sense.

The driver is modular because I have been advised other times that new
drivers should be able to be compiled as modules and we should avoid
using 'bool' in Kconfig for new drivers. That's the only reason. I am
also always including as 'y' the driver since for me not having pci in
my boards has no sense... I am ok in changing Kconfig to be 'bool'
instead of 'tristate', but I don't know what should be the correct
thing to do in this case. Thoughts?

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 ]

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Sergio Paracuellos]

On Thu, Oct 28, 2021 at 11:34 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi Thomas,
>
> On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> >
> > On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> > > On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> > > >
> > > > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > > > drivers become modules.
> > > >
> > > > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > >
> > > > Let's just export them.
> > > >
> > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > > > ---
> > > >  arch/mips/kernel/mips-cm.c  | 5 +++++
> > > >  arch/mips/kernel/mips-cpc.c | 1 +
> > > >  2 files changed, 6 insertions(+)
> > > >
> > >
> > > Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >
> > could we instead make the pcie-mt761 driver non modular ? Exporting
> > all MIPS specific stuff for just making an essential driver modular
> > doesn't IMHO make much sense.
>
> The driver is modular because I have been advised other times that new
> drivers should be able to be compiled as modules and we should avoid
> using 'bool' in Kconfig for new drivers. That's the only reason. I am
> also always including as 'y' the driver since for me not having pci in
> my boards has no sense... I am ok in changing Kconfig to be 'bool'
> instead of 'tristate', but I don't know what should be the correct
> thing to do in this case. Thoughts?

I guess we also want the driver to at least be compile tested in
'allmodconfig' and other similars...

>
> 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 ]

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Bjorn Helgaas]

On Thu, Oct 28, 2021 at 11:59:17AM +0200, Sergio Paracuellos wrote:
> On Thu, Oct 28, 2021 at 11:34 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
> > <tsbogend@alpha.franken.de> wrote:
> > > On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> > > > On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> > > > >
> > > > > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > > > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > > > > drivers become modules.
> > > > >
> > > > > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > >
> > > > > Let's just export them.
> > > > >
> > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > > > > ---
> > > > >  arch/mips/kernel/mips-cm.c  | 5 +++++
> > > > >  arch/mips/kernel/mips-cpc.c | 1 +
> > > > >  2 files changed, 6 insertions(+)
> > > > >
> > > >
> > > > Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > >
> > > could we instead make the pcie-mt761 driver non modular ? Exporting
> > > all MIPS specific stuff for just making an essential driver modular
> > > doesn't IMHO make much sense.
> >
> > The driver is modular because I have been advised other times that new
> > drivers should be able to be compiled as modules and we should avoid
> > using 'bool' in Kconfig for new drivers. That's the only reason. I am
> > also always including as 'y' the driver since for me not having pci in
> > my boards has no sense... I am ok in changing Kconfig to be 'bool'
> > instead of 'tristate', but I don't know what should be the correct
> > thing to do in this case. Thoughts?
> 
> I guess we also want the driver to at least be compile tested in
> 'allmodconfig' and other similars...

Sounds like the systems that actually use this driver require it to be
built-in, and the only benefit of exporting these symbols is that we
would get better compile test coverage.

If that's the case, I agree that it's better to just make it
non-modular.

Bjorn

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Sergio Paracuellos]

On Thu, Oct 28, 2021 at 10:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Oct 28, 2021 at 11:59:17AM +0200, Sergio Paracuellos wrote:
> > On Thu, Oct 28, 2021 at 11:34 AM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
> > > <tsbogend@alpha.franken.de> wrote:
> > > > On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> > > > > On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> > > > > >
> > > > > > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > > > > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > > > > > drivers become modules.
> > > > > >
> > > > > > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > >
> > > > > > Let's just export them.
> > > > > >
> > > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > > > > > ---
> > > > > >  arch/mips/kernel/mips-cm.c  | 5 +++++
> > > > > >  arch/mips/kernel/mips-cpc.c | 1 +
> > > > > >  2 files changed, 6 insertions(+)
> > > > > >
> > > > >
> > > > > Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > >
> > > > could we instead make the pcie-mt761 driver non modular ? Exporting
> > > > all MIPS specific stuff for just making an essential driver modular
> > > > doesn't IMHO make much sense.
> > >
> > > The driver is modular because I have been advised other times that new
> > > drivers should be able to be compiled as modules and we should avoid
> > > using 'bool' in Kconfig for new drivers. That's the only reason. I am
> > > also always including as 'y' the driver since for me not having pci in
> > > my boards has no sense... I am ok in changing Kconfig to be 'bool'
> > > instead of 'tristate', but I don't know what should be the correct
> > > thing to do in this case. Thoughts?
> >
> > I guess we also want the driver to at least be compile tested in
> > 'allmodconfig' and other similars...15692a80d949
>
> Sounds like the systems that actually use this driver require it to be
> built-in, and the only benefit of exporting these symbols is that we
> would get better compile test coverage.
>
> If that's the case, I agree that it's better to just make it
> non-modular.

I agree and that was my reasoning for sending a patch to also convert
to bool the phy driver that this PCIe controller uses. When the pull
request was sent from Vinod to Greg, Greg refused to take it because
of that commit and the commit was reverted and a new pull request was
sent including this revert. This is commit 15692a80d949 ("phy: Revert
"phy: ralink: Kconfig: convert mt7621-pci-phy into 'bool'""). Because
of this I also changed the PCIe controller Kconfig from bool to
tristate when I sent v3 of the series which at the end were the ones
that was finally taken. There are also other ralink related symbols
that have been exported to allow to compile other drivers as a
modules, like the watchdog. See the commit fef532ea0cd8 ("MIPS:
ralink: export rt_sysc_membase for rt2880_wdt.c"). So, as I said, I
agree and I am using the driver as if it were a bool and also ralink
systems normally require all drivers built-in, but I think we have to
take into account also the "historical facts" here. In any case,
Bjorn, let me know if you want me to send whatever patch might be
needed.

Best regards,
    Sergio Paracuellos

>
> Bjorn

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Bjorn Helgaas]

On Fri, Oct 29, 2021 at 07:28:47AM +0200, Sergio Paracuellos wrote:
> On Thu, Oct 28, 2021 at 10:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Oct 28, 2021 at 11:59:17AM +0200, Sergio Paracuellos wrote:
> > > On Thu, Oct 28, 2021 at 11:34 AM Sergio Paracuellos
> > > <sergio.paracuellos@gmail.com> wrote:
> > > > On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
> > > > <tsbogend@alpha.franken.de> wrote:
> > > > > On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> > > > > > On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> > > > > > >
> > > > > > > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > > > > > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > > > > > > drivers become modules.
> > > > > > >
> > > > > > > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > >
> > > > > > > Let's just export them.
> > > > > > >
> > > > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > > > > > > ---
> > > > > > >  arch/mips/kernel/mips-cm.c  | 5 +++++
> > > > > > >  arch/mips/kernel/mips-cpc.c | 1 +
> > > > > > >  2 files changed, 6 insertions(+)
> > > > > > >
> > > > > >
> > > > > > Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > >
> > > > > could we instead make the pcie-mt761 driver non modular ? Exporting
> > > > > all MIPS specific stuff for just making an essential driver modular
> > > > > doesn't IMHO make much sense.
> > > >
> > > > The driver is modular because I have been advised other times that new
> > > > drivers should be able to be compiled as modules and we should avoid
> > > > using 'bool' in Kconfig for new drivers. That's the only reason. I am
> > > > also always including as 'y' the driver since for me not having pci in
> > > > my boards has no sense... I am ok in changing Kconfig to be 'bool'
> > > > instead of 'tristate', but I don't know what should be the correct
> > > > thing to do in this case. Thoughts?
> > >
> > > I guess we also want the driver to at least be compile tested in
> > > 'allmodconfig' and other similars...15692a80d949
> >
> > Sounds like the systems that actually use this driver require it to be
> > built-in, and the only benefit of exporting these symbols is that we
> > would get better compile test coverage.
> >
> > If that's the case, I agree that it's better to just make it
> > non-modular.
> 
> I agree and that was my reasoning for sending a patch to also convert
> to bool the phy driver that this PCIe controller uses. When the pull
> request was sent from Vinod to Greg, Greg refused to take it because
> of that commit and the commit was reverted and a new pull request was
> sent including this revert. This is commit 15692a80d949 ("phy: Revert
> "phy: ralink: Kconfig: convert mt7621-pci-phy into 'bool'""). Because
> of this I also changed the PCIe controller Kconfig from bool to
> tristate when I sent v3 of the series which at the end were the ones
> that was finally taken. There are also other ralink related symbols
> that have been exported to allow to compile other drivers as a
> modules, like the watchdog. See the commit fef532ea0cd8 ("MIPS:
> ralink: export rt_sysc_membase for rt2880_wdt.c"). So, as I said, I
> agree and I am using the driver as if it were a bool and also ralink
> systems normally require all drivers built-in, but I think we have to
> take into account also the "historical facts" here. In any case,
> Bjorn, let me know if you want me to send whatever patch might be
> needed.

I didn't see the conversation with Greg, so I don't know the whole
story.

For pcie-mt7621.c, it looks like the only problem is
setup_cm_memory_region(), which does a little coherency-related stuff.
If we could move that to arch/mips, we could still make this tristate.

One way might be to implement a pcibios_root_bridge_prepare() for mips
and put the setup_cm_memory_region() stuff in there.  It's not *ideal*
because that's a strong/weak function arrangement that doesn't allow
for multiple host bridges, but that's probably not an issue here.

If we can't do that, I think making it bool is probably the right
answer, but it would be worth a brief comment in the commit log to
explain the issue.

Bjorn

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Sergio Paracuellos]

Hi Bjorn,

On Fri, Oct 29, 2021 at 8:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Oct 29, 2021 at 07:28:47AM +0200, Sergio Paracuellos wrote:
> > On Thu, Oct 28, 2021 at 10:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Thu, Oct 28, 2021 at 11:59:17AM +0200, Sergio Paracuellos wrote:
> > > > On Thu, Oct 28, 2021 at 11:34 AM Sergio Paracuellos
> > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
> > > > > <tsbogend@alpha.franken.de> wrote:
> > > > > > On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> > > > > > > On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > > > > > > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > > > > > > > drivers become modules.
> > > > > > > >
> > > > > > > > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > >
> > > > > > > > Let's just export them.
> > > > > > > >
> > > > > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > > > > > > > ---
> > > > > > > >  arch/mips/kernel/mips-cm.c  | 5 +++++
> > > > > > > >  arch/mips/kernel/mips-cpc.c | 1 +
> > > > > > > >  2 files changed, 6 insertions(+)
> > > > > > > >
> > > > > > >
> > > > > > > Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > >
> > > > > > could we instead make the pcie-mt761 driver non modular ? Exporting
> > > > > > all MIPS specific stuff for just making an essential driver modular
> > > > > > doesn't IMHO make much sense.
> > > > >
> > > > > The driver is modular because I have been advised other times that new
> > > > > drivers should be able to be compiled as modules and we should avoid
> > > > > using 'bool' in Kconfig for new drivers. That's the only reason. I am
> > > > > also always including as 'y' the driver since for me not having pci in
> > > > > my boards has no sense... I am ok in changing Kconfig to be 'bool'
> > > > > instead of 'tristate', but I don't know what should be the correct
> > > > > thing to do in this case. Thoughts?
> > > >
> > > > I guess we also want the driver to at least be compile tested in
> > > > 'allmodconfig' and other similars...15692a80d949
> > >
> > > Sounds like the systems that actually use this driver require it to be
> > > built-in, and the only benefit of exporting these symbols is that we
> > > would get better compile test coverage.
> > >
> > > If that's the case, I agree that it's better to just make it
> > > non-modular.
> >
> > I agree and that was my reasoning for sending a patch to also convert
> > to bool the phy driver that this PCIe controller uses. When the pull
> > request was sent from Vinod to Greg, Greg refused to take it because
> > of that commit and the commit was reverted and a new pull request was
> > sent including this revert. This is commit 15692a80d949 ("phy: Revert
> > "phy: ralink: Kconfig: convert mt7621-pci-phy into 'bool'""). Because
> > of this I also changed the PCIe controller Kconfig from bool to
> > tristate when I sent v3 of the series which at the end were the ones
> > that was finally taken. There are also other ralink related symbols
> > that have been exported to allow to compile other drivers as a
> > modules, like the watchdog. See the commit fef532ea0cd8 ("MIPS:
> > ralink: export rt_sysc_membase for rt2880_wdt.c"). So, as I said, I
> > agree and I am using the driver as if it were a bool and also ralink
> > systems normally require all drivers built-in, but I think we have to
> > take into account also the "historical facts" here. In any case,
> > Bjorn, let me know if you want me to send whatever patch might be
> > needed.
>
> I didn't see the conversation with Greg, so I don't know the whole
> story.

Here it is: https://www.spinics.net/lists/kernel/msg3986821.html

>
> For pcie-mt7621.c, it looks like the only problem is
> setup_cm_memory_region(), which does a little coherency-related stuff.
> If we could move that to arch/mips, we could still make this tristate.

Yes, the only mips specific function used in the driver is
'setup_cm_memory_region()'.

>
> One way might be to implement a pcibios_root_bridge_prepare() for mips
> and put the setup_cm_memory_region() stuff in there.  It's not *ideal*
> because that's a strong/weak function arrangement that doesn't allow
> for multiple host bridges, but that's probably not an issue here.
>
> If we can't do that, I think making it bool is probably the right
> answer, but it would be worth a brief comment in the commit log to
> explain the issue.

Do you mean to implement 'pcibios_root_bridge_prepare()' for MIPS
ralink? I guess this means to parse device tree and so on only to get
memory range addresses to be added to the MIPS I/O coherence regions
to make things work and then re-parse it again in the driver to do the
proper PCI setup... We end up in an arch generic driver but at the end
this controller is only present in ralink MIPS, so I am not sure that
implementing 'pcibios_root_bridge_prepare()' is worthy here... I can
explore and try to implement it if you think that it really makes
sense... but, IMHO if this is the case, just making it bool looks like
the correct thing to do.

Best regards,
     Sergio Paracuellos
>
> Bjorn

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Bjorn Helgaas]

On Fri, Oct 29, 2021 at 09:37:53PM +0200, Sergio Paracuellos wrote:
> On Fri, Oct 29, 2021 at 8:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Oct 29, 2021 at 07:28:47AM +0200, Sergio Paracuellos wrote:
> > > On Thu, Oct 28, 2021 at 10:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Thu, Oct 28, 2021 at 11:59:17AM +0200, Sergio Paracuellos wrote:
> > > > > On Thu, Oct 28, 2021 at 11:34 AM Sergio Paracuellos
> > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
> > > > > > <tsbogend@alpha.franken.de> wrote:
> > > > > > > On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> > > > > > > > On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > > > > > > > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > > > > > > > > drivers become modules.
> > > > > > > > >
> > > > > > > > > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > >
> > > > > > > > > Let's just export them.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > > > > > > > > ---
> > > > > > > > >  arch/mips/kernel/mips-cm.c  | 5 +++++
> > > > > > > > >  arch/mips/kernel/mips-cpc.c | 1 +
> > > > > > > > >  2 files changed, 6 insertions(+)
> > > > > > > > >
> > > > > > > >
> > > > > > > > Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > > >
> > > > > > > could we instead make the pcie-mt761 driver non modular ? Exporting
> > > > > > > all MIPS specific stuff for just making an essential driver modular
> > > > > > > doesn't IMHO make much sense.
> > > > > >
> > > > > > The driver is modular because I have been advised other times that new
> > > > > > drivers should be able to be compiled as modules and we should avoid
> > > > > > using 'bool' in Kconfig for new drivers. That's the only reason. I am
> > > > > > also always including as 'y' the driver since for me not having pci in
> > > > > > my boards has no sense... I am ok in changing Kconfig to be 'bool'
> > > > > > instead of 'tristate', but I don't know what should be the correct
> > > > > > thing to do in this case. Thoughts?
> > > > >
> > > > > I guess we also want the driver to at least be compile tested in
> > > > > 'allmodconfig' and other similars...15692a80d949
> > > >
> > > > Sounds like the systems that actually use this driver require it to be
> > > > built-in, and the only benefit of exporting these symbols is that we
> > > > would get better compile test coverage.
> > > >
> > > > If that's the case, I agree that it's better to just make it
> > > > non-modular.
> > >
> > > I agree and that was my reasoning for sending a patch to also convert
> > > to bool the phy driver that this PCIe controller uses. When the pull
> > > request was sent from Vinod to Greg, Greg refused to take it because
> > > of that commit and the commit was reverted and a new pull request was
> > > sent including this revert. This is commit 15692a80d949 ("phy: Revert
> > > "phy: ralink: Kconfig: convert mt7621-pci-phy into 'bool'""). Because
> > > of this I also changed the PCIe controller Kconfig from bool to
> > > tristate when I sent v3 of the series which at the end were the ones
> > > that was finally taken. There are also other ralink related symbols
> > > that have been exported to allow to compile other drivers as a
> > > modules, like the watchdog. See the commit fef532ea0cd8 ("MIPS:
> > > ralink: export rt_sysc_membase for rt2880_wdt.c"). So, as I said, I
> > > agree and I am using the driver as if it were a bool and also ralink
> > > systems normally require all drivers built-in, but I think we have to
> > > take into account also the "historical facts" here. In any case,
> > > Bjorn, let me know if you want me to send whatever patch might be
> > > needed.
> >
> > I didn't see the conversation with Greg, so I don't know the whole
> > story.
> 
> Here it is: https://www.spinics.net/lists/kernel/msg3986821.html
> 
> > For pcie-mt7621.c, it looks like the only problem is
> > setup_cm_memory_region(), which does a little coherency-related stuff.
> > If we could move that to arch/mips, we could still make this tristate.
> 
> Yes, the only mips specific function used in the driver is
> 'setup_cm_memory_region()'.
> 
> > One way might be to implement a pcibios_root_bridge_prepare() for mips
> > and put the setup_cm_memory_region() stuff in there.  It's not *ideal*
> > because that's a strong/weak function arrangement that doesn't allow
> > for multiple host bridges, but that's probably not an issue here.
> >
> > If we can't do that, I think making it bool is probably the right
> > answer, but it would be worth a brief comment in the commit log to
> > explain the issue.
> 
> Do you mean to implement 'pcibios_root_bridge_prepare()' for MIPS
> ralink? I guess this means to parse device tree and so on only to get
> memory range addresses to be added to the MIPS I/O coherence regions
> to make things work and then re-parse it again in the driver to do the
> proper PCI setup... We end up in an arch generic driver but at the end
> this controller is only present in ralink MIPS, so I am not sure that
> implementing 'pcibios_root_bridge_prepare()' is worthy here... I can
> explore and try to implement it if you think that it really makes
> sense... but, IMHO if this is the case, just making it bool looks like
> the correct thing to do.

It should be trivial to put the contents of setup_cm_memory_region()
into a ralink function called pcibios_root_bridge_prepare().

pcibios_root_bridge_prepare() is called with the same "struct
pci_host_bridge *" argument as setup_cm_memory_region(), and it's
called slightly later, so the window resources are already set up, so
no DT parsing is required.  It looks like a simple move and rename to
me.

Bjorn

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Sergio Paracuellos]

On Fri, Oct 29, 2021 at 9:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Oct 29, 2021 at 09:37:53PM +0200, Sergio Paracuellos wrote:
> > On Fri, Oct 29, 2021 at 8:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, Oct 29, 2021 at 07:28:47AM +0200, Sergio Paracuellos wrote:
> > > > On Thu, Oct 28, 2021 at 10:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Thu, Oct 28, 2021 at 11:59:17AM +0200, Sergio Paracuellos wrote:
> > > > > > On Thu, Oct 28, 2021 at 11:34 AM Sergio Paracuellos
> > > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > > On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
> > > > > > > <tsbogend@alpha.franken.de> wrote:
> > > > > > > > On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> > > > > > > > > On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > > > > > > > > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > > > > > > > > > drivers become modules.
> > > > > > > > > >
> > > > > > > > > > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > >
> > > > > > > > > > Let's just export them.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > > > > > > > > > ---
> > > > > > > > > >  arch/mips/kernel/mips-cm.c  | 5 +++++
> > > > > > > > > >  arch/mips/kernel/mips-cpc.c | 1 +
> > > > > > > > > >  2 files changed, 6 insertions(+)
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > > > >
> > > > > > > > could we instead make the pcie-mt761 driver non modular ? Exporting
> > > > > > > > all MIPS specific stuff for just making an essential driver modular
> > > > > > > > doesn't IMHO make much sense.
> > > > > > >
> > > > > > > The driver is modular because I have been advised other times that new
> > > > > > > drivers should be able to be compiled as modules and we should avoid
> > > > > > > using 'bool' in Kconfig for new drivers. That's the only reason. I am
> > > > > > > also always including as 'y' the driver since for me not having pci in
> > > > > > > my boards has no sense... I am ok in changing Kconfig to be 'bool'
> > > > > > > instead of 'tristate', but I don't know what should be the correct
> > > > > > > thing to do in this case. Thoughts?
> > > > > >
> > > > > > I guess we also want the driver to at least be compile tested in
> > > > > > 'allmodconfig' and other similars...15692a80d949
> > > > >
> > > > > Sounds like the systems that actually use this driver require it to be
> > > > > built-in, and the only benefit of exporting these symbols is that we
> > > > > would get better compile test coverage.
> > > > >
> > > > > If that's the case, I agree that it's better to just make it
> > > > > non-modular.
> > > >
> > > > I agree and that was my reasoning for sending a patch to also convert
> > > > to bool the phy driver that this PCIe controller uses. When the pull
> > > > request was sent from Vinod to Greg, Greg refused to take it because
> > > > of that commit and the commit was reverted and a new pull request was
> > > > sent including this revert. This is commit 15692a80d949 ("phy: Revert
> > > > "phy: ralink: Kconfig: convert mt7621-pci-phy into 'bool'""). Because
> > > > of this I also changed the PCIe controller Kconfig from bool to
> > > > tristate when I sent v3 of the series which at the end were the ones
> > > > that was finally taken. There are also other ralink related symbols
> > > > that have been exported to allow to compile other drivers as a
> > > > modules, like the watchdog. See the commit fef532ea0cd8 ("MIPS:
> > > > ralink: export rt_sysc_membase for rt2880_wdt.c"). So, as I said, I
> > > > agree and I am using the driver as if it were a bool and also ralink
> > > > systems normally require all drivers built-in, but I think we have to
> > > > take into account also the "historical facts" here. In any case,
> > > > Bjorn, let me know if you want me to send whatever patch might be
> > > > needed.
> > >
> > > I didn't see the conversation with Greg, so I don't know the whole
> > > story.
> >
> > Here it is: https://www.spinics.net/lists/kernel/msg3986821.html
> >
> > > For pcie-mt7621.c, it looks like the only problem is
> > > setup_cm_memory_region(), which does a little coherency-related stuff.
> > > If we could move that to arch/mips, we could still make this tristate.
> >
> > Yes, the only mips specific function used in the driver is
> > 'setup_cm_memory_region()'.
> >
> > > One way might be to implement a pcibios_root_bridge_prepare() for mips
> > > and put the setup_cm_memory_region() stuff in there.  It's not *ideal*
> > > because that's a strong/weak function arrangement that doesn't allow
> > > for multiple host bridges, but that's probably not an issue here.
> > >
> > > If we can't do that, I think making it bool is probably the right
> > > answer, but it would be worth a brief comment in the commit log to
> > > explain the issue.
> >
> > Do you mean to implement 'pcibios_root_bridge_prepare()' for MIPS
> > ralink? I guess this means to parse device tree and so on only to get
> > memory range addresses to be added to the MIPS I/O coherence regions
> > to make things work and then re-parse it again in the driver to do the
> > proper PCI setup... We end up in an arch generic driver but at the end
> > this controller is only present in ralink MIPS, so I am not sure that
> > implementing 'pcibios_root_bridge_prepare()' is worthy here... I can
> > explore and try to implement it if you think that it really makes
> > sense... but, IMHO if this is the case, just making it bool looks like
> > the correct thing to do.
>
> It should be trivial to put the contents of setup_cm_memory_region()
> into a ralink function called pcibios_root_bridge_prepare().
>
> pcibios_root_bridge_prepare() is called with the same "struct
> pci_host_bridge *" argument as setup_cm_memory_region(), and it's
> called slightly later, so the window resources are already set up, so
> no DT parsing is required.  It looks like a simple move and rename to
> me.

I see. Thanks Bjorn. I will try the approach during the weekend and
report if it works.

Best regards,
    Sergio Paracuellos

>
> Bjorn

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Sergio Paracuellos]

Hi Bjorn,

On Fri, Oct 29, 2021 at 10:27 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> On Fri, Oct 29, 2021 at 9:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Fri, Oct 29, 2021 at 09:37:53PM +0200, Sergio Paracuellos wrote:
> > > On Fri, Oct 29, 2021 at 8:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Fri, Oct 29, 2021 at 07:28:47AM +0200, Sergio Paracuellos wrote:
> > > > > On Thu, Oct 28, 2021 at 10:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > On Thu, Oct 28, 2021 at 11:59:17AM +0200, Sergio Paracuellos wrote:
> > > > > > > On Thu, Oct 28, 2021 at 11:34 AM Sergio Paracuellos
> > > > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > > > On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
> > > > > > > > <tsbogend@alpha.franken.de> wrote:
> > > > > > > > > On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> > > > > > > > > > On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > > > > > > > > > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > > > > > > > > > > drivers become modules.
> > > > > > > > > > >
> > > > > > > > > > > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > >
> > > > > > > > > > > Let's just export them.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > > > > > > > > > > ---
> > > > > > > > > > >  arch/mips/kernel/mips-cm.c  | 5 +++++
> > > > > > > > > > >  arch/mips/kernel/mips-cpc.c | 1 +
> > > > > > > > > > >  2 files changed, 6 insertions(+)
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > > > > >
> > > > > > > > > could we instead make the pcie-mt761 driver non modular ? Exporting
> > > > > > > > > all MIPS specific stuff for just making an essential driver modular
> > > > > > > > > doesn't IMHO make much sense.
> > > > > > > >
> > > > > > > > The driver is modular because I have been advised other times that new
> > > > > > > > drivers should be able to be compiled as modules and we should avoid
> > > > > > > > using 'bool' in Kconfig for new drivers. That's the only reason. I am
> > > > > > > > also always including as 'y' the driver since for me not having pci in
> > > > > > > > my boards has no sense... I am ok in changing Kconfig to be 'bool'
> > > > > > > > instead of 'tristate', but I don't know what should be the correct
> > > > > > > > thing to do in this case. Thoughts?
> > > > > > >
> > > > > > > I guess we also want the driver to at least be compile tested in
> > > > > > > 'allmodconfig' and other similars...15692a80d949
> > > > > >
> > > > > > Sounds like the systems that actually use this driver require it to be
> > > > > > built-in, and the only benefit of exporting these symbols is that we
> > > > > > would get better compile test coverage.
> > > > > >
> > > > > > If that's the case, I agree that it's better to just make it
> > > > > > non-modular.
> > > > >
> > > > > I agree and that was my reasoning for sending a patch to also convert
> > > > > to bool the phy driver that this PCIe controller uses. When the pull
> > > > > request was sent from Vinod to Greg, Greg refused to take it because
> > > > > of that commit and the commit was reverted and a new pull request was
> > > > > sent including this revert. This is commit 15692a80d949 ("phy: Revert
> > > > > "phy: ralink: Kconfig: convert mt7621-pci-phy into 'bool'""). Because
> > > > > of this I also changed the PCIe controller Kconfig from bool to
> > > > > tristate when I sent v3 of the series which at the end were the ones
> > > > > that was finally taken. There are also other ralink related symbols
> > > > > that have been exported to allow to compile other drivers as a
> > > > > modules, like the watchdog. See the commit fef532ea0cd8 ("MIPS:
> > > > > ralink: export rt_sysc_membase for rt2880_wdt.c"). So, as I said, I
> > > > > agree and I am using the driver as if it were a bool and also ralink
> > > > > systems normally require all drivers built-in, but I think we have to
> > > > > take into account also the "historical facts" here. In any case,
> > > > > Bjorn, let me know if you want me to send whatever patch might be
> > > > > needed.
> > > >
> > > > I didn't see the conversation with Greg, so I don't know the whole
> > > > story.
> > >
> > > Here it is: https://www.spinics.net/lists/kernel/msg3986821.html
> > >
> > > > For pcie-mt7621.c, it looks like the only problem is
> > > > setup_cm_memory_region(), which does a little coherency-related stuff.
> > > > If we could move that to arch/mips, we could still make this tristate.
> > >
> > > Yes, the only mips specific function used in the driver is
> > > 'setup_cm_memory_region()'.
> > >
> > > > One way might be to implement a pcibios_root_bridge_prepare() for mips
> > > > and put the setup_cm_memory_region() stuff in there.  It's not *ideal*
> > > > because that's a strong/weak function arrangement that doesn't allow
> > > > for multiple host bridges, but that's probably not an issue here.
> > > >
> > > > If we can't do that, I think making it bool is probably the right
> > > > answer, but it would be worth a brief comment in the commit log to
> > > > explain the issue.
> > >
> > > Do you mean to implement 'pcibios_root_bridge_prepare()' for MIPS
> > > ralink? I guess this means to parse device tree and so on only to get
> > > memory range addresses to be added to the MIPS I/O coherence regions
> > > to make things work and then re-parse it again in the driver to do the
> > > proper PCI setup... We end up in an arch generic driver but at the end
> > > this controller is only present in ralink MIPS, so I am not sure that
> > > implementing 'pcibios_root_bridge_prepare()' is worthy here... I can
> > > explore and try to implement it if you think that it really makes
> > > sense... but, IMHO if this is the case, just making it bool looks like
> > > the correct thing to do.
> >
> > It should be trivial to put the contents of setup_cm_memory_region()
> > into a ralink function called pcibios_root_bridge_prepare().
> >
> > pcibios_root_bridge_prepare() is called with the same "struct
> > pci_host_bridge *" argument as setup_cm_memory_region(), and it's
> > called slightly later, so the window resources are already set up, so
> > no DT parsing is required.  It looks like a simple move and rename to
> > me.
>
> I see. Thanks Bjorn. I will try the approach during the weekend and
> report if it works.

I have tested the change from 'setup_cm_memory_region()' code into
'pcibios_root_bridge_prepare()' just by moving and renaming it from
the PCIe controller code. The function is properly being called.
However, it looks like at that point, windows are not setup yet (no
windows present at all in bridge->windows) so the system is not able
to get the IORESOURCE_MEM resource to set up the IO coherency unit and
the PCI failed to start:

[   16.785359] mt7621-pci 1e140000.pcie: host bridge /pcie@1e140000 ranges:
[   16.798719] mt7621-pci 1e140000.pcie:   No bus range found for
/pcie@1e140000, using [bus 00-ff]
[   16.816248] mt7621-pci 1e140000.pcie:      MEM
0x0060000000..0x006fffffff -> 0x0060000000
[   16.861310] mt7621-pci 1e140000.pcie:       IO
0x001e160000..0x001e16ffff -> 0x0000000000
[   17.179230] mt7621-pci 1e140000.pcie: PCIE0 enabled
[   17.188954] mt7621-pci 1e140000.pcie: PCIE1 enabled
[   17.198678] mt7621-pci 1e140000.pcie: PCIE2 enabled
[   17.208415] Cannot get memory resource
[   17.215884] mt7621-pci 1e140000.pcie: Scanning root bridge failed
[   17.228454] mt7621-pci: probe of 1e140000.pcie failed with error -22

FWIW, when the function is called, I have also tried to set up
hardcoded addresses. Doing that the IO coherency unit was properly set
up and PCI properly worked (expected). So, using this
'pcibios_root_bridge_prepare()' funcion looks like a possible way to
go but we need the addresses properly being passed into the function.
I've also tried to list 'bridge->dma_ranges' and get resources from
there instead of using the not already setup 'bridge->windows'. There
is nothing inside that list also. 'bridge->bus->resources' is also
empty... Am I missing something? I was expecting the bridge passed
around to be the same that was in PCIe controller code, and it seems
it is (I printed the bridge pointer itself in driver code before
calling 'mt7621_pcie_register_host()' and in
'pcibios_root_bridge_prepare()' at the begging of the function and the
pointer is the same) but windows and other stuff are not already
present there...

Thanks in advance for clarification.

Best regards,
     Sergio Paracuellos

>
> Best regards,
>     Sergio Paracuellos
>
> >
> > Bjorn

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Sergio Paracuellos]

On Sat, Oct 30, 2021 at 7:21 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi Bjorn,
>
> On Fri, Oct 29, 2021 at 10:27 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > On Fri, Oct 29, 2021 at 9:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > On Fri, Oct 29, 2021 at 09:37:53PM +0200, Sergio Paracuellos wrote:
> > > > On Fri, Oct 29, 2021 at 8:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Fri, Oct 29, 2021 at 07:28:47AM +0200, Sergio Paracuellos wrote:
> > > > > > On Thu, Oct 28, 2021 at 10:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > On Thu, Oct 28, 2021 at 11:59:17AM +0200, Sergio Paracuellos wrote:
> > > > > > > > On Thu, Oct 28, 2021 at 11:34 AM Sergio Paracuellos
> > > > > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > > > > On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
> > > > > > > > > <tsbogend@alpha.franken.de> wrote:
> > > > > > > > > > On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> > > > > > > > > > > On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > > > > > > > > > > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > > > > > > > > > > > drivers become modules.
> > > > > > > > > > > >
> > > > > > > > > > > > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > >
> > > > > > > > > > > > Let's just export them.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  arch/mips/kernel/mips-cm.c  | 5 +++++
> > > > > > > > > > > >  arch/mips/kernel/mips-cpc.c | 1 +
> > > > > > > > > > > >  2 files changed, 6 insertions(+)
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > > > > > >
> > > > > > > > > > could we instead make the pcie-mt761 driver non modular ? Exporting
> > > > > > > > > > all MIPS specific stuff for just making an essential driver modular
> > > > > > > > > > doesn't IMHO make much sense.
> > > > > > > > >
> > > > > > > > > The driver is modular because I have been advised other times that new
> > > > > > > > > drivers should be able to be compiled as modules and we should avoid
> > > > > > > > > using 'bool' in Kconfig for new drivers. That's the only reason. I am
> > > > > > > > > also always including as 'y' the driver since for me not having pci in
> > > > > > > > > my boards has no sense... I am ok in changing Kconfig to be 'bool'
> > > > > > > > > instead of 'tristate', but I don't know what should be the correct
> > > > > > > > > thing to do in this case. Thoughts?
> > > > > > > >
> > > > > > > > I guess we also want the driver to at least be compile tested in
> > > > > > > > 'allmodconfig' and other similars...15692a80d949
> > > > > > >
> > > > > > > Sounds like the systems that actually use this driver require it to be
> > > > > > > built-in, and the only benefit of exporting these symbols is that we
> > > > > > > would get better compile test coverage.
> > > > > > >
> > > > > > > If that's the case, I agree that it's better to just make it
> > > > > > > non-modular.
> > > > > >
> > > > > > I agree and that was my reasoning for sending a patch to also convert
> > > > > > to bool the phy driver that this PCIe controller uses. When the pull
> > > > > > request was sent from Vinod to Greg, Greg refused to take it because
> > > > > > of that commit and the commit was reverted and a new pull request was
> > > > > > sent including this revert. This is commit 15692a80d949 ("phy: Revert
> > > > > > "phy: ralink: Kconfig: convert mt7621-pci-phy into 'bool'""). Because
> > > > > > of this I also changed the PCIe controller Kconfig from bool to
> > > > > > tristate when I sent v3 of the series which at the end were the ones
> > > > > > that was finally taken. There are also other ralink related symbols
> > > > > > that have been exported to allow to compile other drivers as a
> > > > > > modules, like the watchdog. See the commit fef532ea0cd8 ("MIPS:
> > > > > > ralink: export rt_sysc_membase for rt2880_wdt.c"). So, as I said, I
> > > > > > agree and I am using the driver as if it were a bool and also ralink
> > > > > > systems normally require all drivers built-in, but I think we have to
> > > > > > take into account also the "historical facts" here. In any case,
> > > > > > Bjorn, let me know if you want me to send whatever patch might be
> > > > > > needed.
> > > > >
> > > > > I didn't see the conversation with Greg, so I don't know the whole
> > > > > story.
> > > >
> > > > Here it is: https://www.spinics.net/lists/kernel/msg3986821.html
> > > >
> > > > > For pcie-mt7621.c, it looks like the only problem is
> > > > > setup_cm_memory_region(), which does a little coherency-related stuff.
> > > > > If we could move that to arch/mips, we could still make this tristate.
> > > >
> > > > Yes, the only mips specific function used in the driver is
> > > > 'setup_cm_memory_region()'.
> > > >
> > > > > One way might be to implement a pcibios_root_bridge_prepare() for mips
> > > > > and put the setup_cm_memory_region() stuff in there.  It's not *ideal*
> > > > > because that's a strong/weak function arrangement that doesn't allow
> > > > > for multiple host bridges, but that's probably not an issue here.
> > > > >
> > > > > If we can't do that, I think making it bool is probably the right
> > > > > answer, but it would be worth a brief comment in the commit log to
> > > > > explain the issue.
> > > >
> > > > Do you mean to implement 'pcibios_root_bridge_prepare()' for MIPS
> > > > ralink? I guess this means to parse device tree and so on only to get
> > > > memory range addresses to be added to the MIPS I/O coherence regions
> > > > to make things work and then re-parse it again in the driver to do the
> > > > proper PCI setup... We end up in an arch generic driver but at the end
> > > > this controller is only present in ralink MIPS, so I am not sure that
> > > > implementing 'pcibios_root_bridge_prepare()' is worthy here... I can
> > > > explore and try to implement it if you think that it really makes
> > > > sense... but, IMHO if this is the case, just making it bool looks like
> > > > the correct thing to do.
> > >
> > > It should be trivial to put the contents of setup_cm_memory_region()
> > > into a ralink function called pcibios_root_bridge_prepare().
> > >
> > > pcibios_root_bridge_prepare() is called with the same "struct
> > > pci_host_bridge *" argument as setup_cm_memory_region(), and it's
> > > called slightly later, so the window resources are already set up, so
> > > no DT parsing is required.  It looks like a simple move and rename to
> > > me.
> >
> > I see. Thanks Bjorn. I will try the approach during the weekend and
> > report if it works.
>
> I have tested the change from 'setup_cm_memory_region()' code into
> 'pcibios_root_bridge_prepare()' just by moving and renaming it from
> the PCIe controller code. The function is properly being called.
> However, it looks like at that point, windows are not setup yet (no
> windows present at all in bridge->windows) so the system is not able
> to get the IORESOURCE_MEM resource to set up the IO coherency unit and
> the PCI failed to start:
>
> [   16.785359] mt7621-pci 1e140000.pcie: host bridge /pcie@1e140000 ranges:
> [   16.798719] mt7621-pci 1e140000.pcie:   No bus range found for
> /pcie@1e140000, using [bus 00-ff]
> [   16.816248] mt7621-pci 1e140000.pcie:      MEM
> 0x0060000000..0x006fffffff -> 0x0060000000
> [   16.861310] mt7621-pci 1e140000.pcie:       IO
> 0x001e160000..0x001e16ffff -> 0x0000000000
> [   17.179230] mt7621-pci 1e140000.pcie: PCIE0 enabled
> [   17.188954] mt7621-pci 1e140000.pcie: PCIE1 enabled
> [   17.198678] mt7621-pci 1e140000.pcie: PCIE2 enabled
> [   17.208415] Cannot get memory resource
> [   17.215884] mt7621-pci 1e140000.pcie: Scanning root bridge failed
> [   17.228454] mt7621-pci: probe of 1e140000.pcie failed with error -22
>
> FWIW, when the function is called, I have also tried to set up
> hardcoded addresses. Doing that the IO coherency unit was properly set
> up and PCI properly worked (expected). So, using this
> 'pcibios_root_bridge_prepare()' funcion looks like a possible way to
> go but we need the addresses properly being passed into the function.
> I've also tried to list 'bridge->dma_ranges' and get resources from
> there instead of using the not already setup 'bridge->windows'. There
> is nothing inside that list also. 'bridge->bus->resources' is also
> empty... Am I missing something? I was expecting the bridge passed
> around to be the same that was in PCIe controller code, and it seems
> it is (I printed the bridge pointer itself in driver code before
> calling 'mt7621_pcie_register_host()' and in
> 'pcibios_root_bridge_prepare()' at the begging of the function and the
> pointer is the same) but windows and other stuff are not already
> present there...

Looking into [0] it looks like resources are temporarily removed from
the list just before call 'pcibios_root_bridge_prepare()'. Hence the
behaviour I am seeing when trying to get them...

[0]: https://elixir.bootlin.com/linux/latest/source/drivers/pci/probe.c#L915

Best regards,
    Sergio Paracuellos

>
> Thanks in advance for clarification.
>
> Best regards,
>      Sergio Paracuellos
>
> >
> > Best regards,
> >     Sergio Paracuellos
> >
> > >
> > > Bjorn

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Sergio Paracuellos]

Hi Bjorn,

On Sat, Oct 30, 2021 at 7:38 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> On Sat, Oct 30, 2021 at 7:21 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > Hi Bjorn,
> >
> > On Fri, Oct 29, 2021 at 10:27 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > >
> > > On Fri, Oct 29, 2021 at 9:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >
> > > > On Fri, Oct 29, 2021 at 09:37:53PM +0200, Sergio Paracuellos wrote:
> > > > > On Fri, Oct 29, 2021 at 8:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > On Fri, Oct 29, 2021 at 07:28:47AM +0200, Sergio Paracuellos wrote:
> > > > > > > On Thu, Oct 28, 2021 at 10:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > On Thu, Oct 28, 2021 at 11:59:17AM +0200, Sergio Paracuellos wrote:
> > > > > > > > > On Thu, Oct 28, 2021 at 11:34 AM Sergio Paracuellos
> > > > > > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > > > > > On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
> > > > > > > > > > <tsbogend@alpha.franken.de> wrote:
> > > > > > > > > > > On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:
> > > > > > > > > > > > On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si <siyanteng01@gmail.com> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Since commit 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
> > > > > > > > > > > > > the MT7621 PCIe host controller driver is built as a module but modpost complains once these
> > > > > > > > > > > > > drivers become modules.
> > > > > > > > > > > > >
> > > > > > > > > > > > > ERROR: modpost: "mips_cm_unlock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > > > ERROR: modpost: "mips_cpc_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > > > ERROR: modpost: "mips_cm_lock_other" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > > > ERROR: modpost: "mips_cm_is64" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > > > ERROR: modpost: "mips_gcr_base" [drivers/pci/controller/pcie-mt7621.ko] undefined!
> > > > > > > > > > > > >
> > > > > > > > > > > > > Let's just export them.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  arch/mips/kernel/mips-cm.c  | 5 +++++
> > > > > > > > > > > > >  arch/mips/kernel/mips-cpc.c | 1 +
> > > > > > > > > > > > >  2 files changed, 6 insertions(+)
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > > > > > > >
> > > > > > > > > > > could we instead make the pcie-mt761 driver non modular ? Exporting
> > > > > > > > > > > all MIPS specific stuff for just making an essential driver modular
> > > > > > > > > > > doesn't IMHO make much sense.
> > > > > > > > > >
> > > > > > > > > > The driver is modular because I have been advised other times that new
> > > > > > > > > > drivers should be able to be compiled as modules and we should avoid
> > > > > > > > > > using 'bool' in Kconfig for new drivers. That's the only reason. I am
> > > > > > > > > > also always including as 'y' the driver since for me not having pci in
> > > > > > > > > > my boards has no sense... I am ok in changing Kconfig to be 'bool'
> > > > > > > > > > instead of 'tristate', but I don't know what should be the correct
> > > > > > > > > > thing to do in this case. Thoughts?
> > > > > > > > >
> > > > > > > > > I guess we also want the driver to at least be compile tested in
> > > > > > > > > 'allmodconfig' and other similars...15692a80d949
> > > > > > > >
> > > > > > > > Sounds like the systems that actually use this driver require it to be
> > > > > > > > built-in, and the only benefit of exporting these symbols is that we
> > > > > > > > would get better compile test coverage.
> > > > > > > >
> > > > > > > > If that's the case, I agree that it's better to just make it
> > > > > > > > non-modular.
> > > > > > >
> > > > > > > I agree and that was my reasoning for sending a patch to also convert
> > > > > > > to bool the phy driver that this PCIe controller uses. When the pull
> > > > > > > request was sent from Vinod to Greg, Greg refused to take it because
> > > > > > > of that commit and the commit was reverted and a new pull request was
> > > > > > > sent including this revert. This is commit 15692a80d949 ("phy: Revert
> > > > > > > "phy: ralink: Kconfig: convert mt7621-pci-phy into 'bool'""). Because
> > > > > > > of this I also changed the PCIe controller Kconfig from bool to
> > > > > > > tristate when I sent v3 of the series which at the end were the ones
> > > > > > > that was finally taken. There are also other ralink related symbols
> > > > > > > that have been exported to allow to compile other drivers as a
> > > > > > > modules, like the watchdog. See the commit fef532ea0cd8 ("MIPS:
> > > > > > > ralink: export rt_sysc_membase for rt2880_wdt.c"). So, as I said, I
> > > > > > > agree and I am using the driver as if it were a bool and also ralink
> > > > > > > systems normally require all drivers built-in, but I think we have to
> > > > > > > take into account also the "historical facts" here. In any case,
> > > > > > > Bjorn, let me know if you want me to send whatever patch might be
> > > > > > > needed.
> > > > > >
> > > > > > I didn't see the conversation with Greg, so I don't know the whole
> > > > > > story.
> > > > >
> > > > > Here it is: https://www.spinics.net/lists/kernel/msg3986821.html
> > > > >
> > > > > > For pcie-mt7621.c, it looks like the only problem is
> > > > > > setup_cm_memory_region(), which does a little coherency-related stuff.
> > > > > > If we could move that to arch/mips, we could still make this tristate.
> > > > >
> > > > > Yes, the only mips specific function used in the driver is
> > > > > 'setup_cm_memory_region()'.
> > > > >
> > > > > > One way might be to implement a pcibios_root_bridge_prepare() for mips
> > > > > > and put the setup_cm_memory_region() stuff in there.  It's not *ideal*
> > > > > > because that's a strong/weak function arrangement that doesn't allow
> > > > > > for multiple host bridges, but that's probably not an issue here.
> > > > > >
> > > > > > If we can't do that, I think making it bool is probably the right
> > > > > > answer, but it would be worth a brief comment in the commit log to
> > > > > > explain the issue.
> > > > >
> > > > > Do you mean to implement 'pcibios_root_bridge_prepare()' for MIPS
> > > > > ralink? I guess this means to parse device tree and so on only to get
> > > > > memory range addresses to be added to the MIPS I/O coherence regions
> > > > > to make things work and then re-parse it again in the driver to do the
> > > > > proper PCI setup... We end up in an arch generic driver but at the end
> > > > > this controller is only present in ralink MIPS, so I am not sure that
> > > > > implementing 'pcibios_root_bridge_prepare()' is worthy here... I can
> > > > > explore and try to implement it if you think that it really makes
> > > > > sense... but, IMHO if this is the case, just making it bool looks like
> > > > > the correct thing to do.
> > > >
> > > > It should be trivial to put the contents of setup_cm_memory_region()
> > > > into a ralink function called pcibios_root_bridge_prepare().
> > > >
> > > > pcibios_root_bridge_prepare() is called with the same "struct
> > > > pci_host_bridge *" argument as setup_cm_memory_region(), and it's
> > > > called slightly later, so the window resources are already set up, so
> > > > no DT parsing is required.  It looks like a simple move and rename to
> > > > me.
> > >
> > > I see. Thanks Bjorn. I will try the approach during the weekend and
> > > report if it works.
> >
> > I have tested the change from 'setup_cm_memory_region()' code into
> > 'pcibios_root_bridge_prepare()' just by moving and renaming it from
> > the PCIe controller code. The function is properly being called.
> > However, it looks like at that point, windows are not setup yet (no
> > windows present at all in bridge->windows) so the system is not able
> > to get the IORESOURCE_MEM resource to set up the IO coherency unit and
> > the PCI failed to start:
> >
> > [   16.785359] mt7621-pci 1e140000.pcie: host bridge /pcie@1e140000 ranges:
> > [   16.798719] mt7621-pci 1e140000.pcie:   No bus range found for
> > /pcie@1e140000, using [bus 00-ff]
> > [   16.816248] mt7621-pci 1e140000.pcie:      MEM
> > 0x0060000000..0x006fffffff -> 0x0060000000
> > [   16.861310] mt7621-pci 1e140000.pcie:       IO
> > 0x001e160000..0x001e16ffff -> 0x0000000000
> > [   17.179230] mt7621-pci 1e140000.pcie: PCIE0 enabled
> > [   17.188954] mt7621-pci 1e140000.pcie: PCIE1 enabled
> > [   17.198678] mt7621-pci 1e140000.pcie: PCIE2 enabled
> > [   17.208415] Cannot get memory resource
> > [   17.215884] mt7621-pci 1e140000.pcie: Scanning root bridge failed
> > [   17.228454] mt7621-pci: probe of 1e140000.pcie failed with error -22
> >
> > FWIW, when the function is called, I have also tried to set up
> > hardcoded addresses. Doing that the IO coherency unit was properly set
> > up and PCI properly worked (expected). So, using this
> > 'pcibios_root_bridge_prepare()' funcion looks like a possible way to
> > go but we need the addresses properly being passed into the function.
> > I've also tried to list 'bridge->dma_ranges' and get resources from
> > there instead of using the not already setup 'bridge->windows'. There
> > is nothing inside that list also. 'bridge->bus->resources' is also
> > empty... Am I missing something? I was expecting the bridge passed
> > around to be the same that was in PCIe controller code, and it seems
> > it is (I printed the bridge pointer itself in driver code before
> > calling 'mt7621_pcie_register_host()' and in
> > 'pcibios_root_bridge_prepare()' at the begging of the function and the
> > pointer is the same) but windows and other stuff are not already
> > present there...
>
> Looking into [0] it looks like resources are temporarily removed from
> the list just before call 'pcibios_root_bridge_prepare()'. Hence the
> behaviour I am seeing when trying to get them...
>
> [0]: https://elixir.bootlin.com/linux/latest/source/drivers/pci/probe.c#L915

Can you explain to me, why are resources temporarily removed from the
'bridge->windows' list?

Would moving that list split to be done after
'pcibios_root_bridge_prepare()' is called a possibility?

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4289030b0fff..2132df91ad8b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -891,8 +891,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->msi = bridge->msi;
        bus->ops = bridge->ops;
@@ -916,6 +914,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);

Obviously doing this works and windows are passed into mips ralink
specific 'pcibios_root_bridge_prepare()' and the PCIe subsystem is
properly working.

The advantages I see to this approach are that doing in this way lets us to:
- Remove specific mips code from the driver controller.
- Allow the driver to be compile tested for any architecture.

And the changes would be the following patches:
1) Small 'drivers/pci/probe.c' change.
2) Move mips specific code into 'arch/mips/ralink/mt76721.c' (since
other mips ralink stuff haven't got IO coherency units) to be inside
'pcibios_root_bridge_prepare()'.
3) Add MODULE_LICENSE macro to the PCIe controller driver to avoid
complaints when the driver is compiled as a module .
4) Update PCIe controller driver's Kconfig to avoid MIPS COMPILE_TEST
conditional and completely enable it for COMPILE_TEST.

When you have time, please, let me know your thoughts about this.

Thanks in advance for your time.

Best regards,
    Sergio Paracuellos

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Bjorn Helgaas]

[+cc Arnd]

On Sun, Nov 07, 2021 at 08:00:56AM +0100, Sergio Paracuellos wrote:
> On Sat, Oct 30, 2021 at 7:38 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > On Sat, Oct 30, 2021 at 7:21 AM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > On Fri, Oct 29, 2021 at 10:27 PM Sergio Paracuellos
> > > <sergio.paracuellos@gmail.com> wrote:
> > > > On Fri, Oct 29, 2021 at 9:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Fri, Oct 29, 2021 at 09:37:53PM +0200, Sergio Paracuellos wrote:
> > > > > > On Fri, Oct 29, 2021 at 8:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:

> > > > > > > One way might be to implement a
> > > > > > > pcibios_root_bridge_prepare() for mips and put the
> > > > > > > setup_cm_memory_region() stuff in there.  It's not
> > > > > > > *ideal* because that's a strong/weak function
> > > > > > > arrangement that doesn't allow for multiple host
> > > > > > > bridges, but that's probably not an issue here.
> > > > > > >
> > > > > > > If we can't do that, I think making it bool is probably
> > > > > > > the right answer, but it would be worth a brief comment
> > > > > > > in the commit log to explain the issue.
> > > > > >
> > > > > > Do you mean to implement 'pcibios_root_bridge_prepare()'
> > > > > > for MIPS ralink? I guess this means to parse device tree
> > > > > > and so on only to get memory range addresses to be added
> > > > > > to the MIPS I/O coherence regions to make things work and
> > > > > > then re-parse it again in the driver to do the proper PCI
> > > > > > setup... We end up in an arch generic driver but at the
> > > > > > end this controller is only present in ralink MIPS, so I
> > > > > > am not sure that implementing
> > > > > > 'pcibios_root_bridge_prepare()' is worthy here... I can
> > > > > > explore and try to implement it if you think that it
> > > > > > really makes sense... but, IMHO if this is the case, just
> > > > > > making it bool looks like the correct thing to do.
> > > > >
> > > > > It should be trivial to put the contents of
> > > > > setup_cm_memory_region() into a ralink function called
> > > > > pcibios_root_bridge_prepare().
> > > > >
> > > > > pcibios_root_bridge_prepare() is called with the same
> > > > > "struct pci_host_bridge *" argument as
> > > > > setup_cm_memory_region(), and it's called slightly later, so
> > > > > the window resources are already set up, so no DT parsing is
> > > > > required.  It looks like a simple move and rename to me.
> > > >
> > > > I see. Thanks Bjorn. I will try the approach during the
> > > > weekend and report if it works.
> > >
> > > I have tested the change from 'setup_cm_memory_region()' code
> > > into 'pcibios_root_bridge_prepare()' just by moving and renaming
> > > it from the PCIe controller code. The function is properly being
> > > called.  However, it looks like at that point, windows are not
> > > setup yet (no windows present at all in bridge->windows) so the
> > > system is not able to get the IORESOURCE_MEM resource to set up
> > > the IO coherency unit and the PCI failed to start:
> > >
> > > [   16.785359] mt7621-pci 1e140000.pcie: host bridge /pcie@1e140000 ranges:
> > > [   16.798719] mt7621-pci 1e140000.pcie:   No bus range found for
> > > /pcie@1e140000, using [bus 00-ff]
> > > [   16.816248] mt7621-pci 1e140000.pcie:      MEM
> > > 0x0060000000..0x006fffffff -> 0x0060000000
> > > [   16.861310] mt7621-pci 1e140000.pcie:       IO
> > > 0x001e160000..0x001e16ffff -> 0x0000000000
> > > [   17.179230] mt7621-pci 1e140000.pcie: PCIE0 enabled
> > > [   17.188954] mt7621-pci 1e140000.pcie: PCIE1 enabled
> > > [   17.198678] mt7621-pci 1e140000.pcie: PCIE2 enabled
> > > [   17.208415] Cannot get memory resource
> > > [   17.215884] mt7621-pci 1e140000.pcie: Scanning root bridge failed
> > > [   17.228454] mt7621-pci: probe of 1e140000.pcie failed with error -22
> > >
> > > FWIW, when the function is called, I have also tried to set up
> > > hardcoded addresses. Doing that the IO coherency unit was
> > > properly set up and PCI properly worked (expected). So, using
> > > this 'pcibios_root_bridge_prepare()' funcion looks like a
> > > possible way to go but we need the addresses properly being
> > > passed into the function.  I've also tried to list
> > > 'bridge->dma_ranges' and get resources from there instead of
> > > using the not already setup 'bridge->windows'. There is nothing
> > > inside that list also. 'bridge->bus->resources' is also empty...
> > > Am I missing something? I was expecting the bridge passed around
> > > to be the same that was in PCIe controller code, and it seems it
> > > is (I printed the bridge pointer itself in driver code before
> > > calling 'mt7621_pcie_register_host()' and in
> > > 'pcibios_root_bridge_prepare()' at the begging of the function
> > > and the pointer is the same) but windows and other stuff are not
> > > already present there...
> >
> > Looking into [0] it looks like resources are temporarily removed
> > from the list just before call 'pcibios_root_bridge_prepare()'.
> > Hence the behaviour I am seeing when trying to get them...
> >
> > [0]:
> > https://elixir.bootlin.com/linux/latest/source/drivers/pci/probe.c#L915
> 
> Can you explain to me, why are resources temporarily removed from
> the 'bridge->windows' list?
> 
> Would moving that list split to be done after
> 'pcibios_root_bridge_prepare()' is called a possibility?

I don't know why the windows are managed that way.  That was added by
37d6a0a6f470 ("PCI: Add pci_register_host_bridge() interface").   I
cc'd Arnd just in case he remembers, but that was a long time ago.

I don't see any use of bridge->windows in any of the
pcibios_root_bridge_prepare() functions.  It doesn't *look* like it
should be used until the coalesce/add code near the end.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4289030b0fff..2132df91ad8b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -891,8 +891,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->msi = bridge->msi;
>         bus->ops = bridge->ops;
> @@ -916,6 +914,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);
> 
> Obviously doing this works and windows are passed into mips ralink
> specific 'pcibios_root_bridge_prepare()' and the PCIe subsystem is
> properly working.
> 
> The advantages I see to this approach are that doing in this way lets us to:
> - Remove specific mips code from the driver controller.
> - Allow the driver to be compile tested for any architecture.
> 
> And the changes would be the following patches:
> 1) Small 'drivers/pci/probe.c' change.
> 2) Move mips specific code into 'arch/mips/ralink/mt76721.c' (since
> other mips ralink stuff haven't got IO coherency units) to be inside
> 'pcibios_root_bridge_prepare()'.
> 3) Add MODULE_LICENSE macro to the PCIe controller driver to avoid
> complaints when the driver is compiled as a module .
> 4) Update PCIe controller driver's Kconfig to avoid MIPS COMPILE_TEST
> conditional and completely enable it for COMPILE_TEST.
> 
> When you have time, please, let me know your thoughts about this.
> 
> Thanks in advance for your time.
> 
> Best regards,
>     Sergio Paracuellos

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Sergio Paracuellos]

On Tue, Nov 9, 2021 at 11:41 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Arnd]
>
> On Sun, Nov 07, 2021 at 08:00:56AM +0100, Sergio Paracuellos wrote:
> > On Sat, Oct 30, 2021 at 7:38 AM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > On Sat, Oct 30, 2021 at 7:21 AM Sergio Paracuellos
> > > <sergio.paracuellos@gmail.com> wrote:
> > > > On Fri, Oct 29, 2021 at 10:27 PM Sergio Paracuellos
> > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > On Fri, Oct 29, 2021 at 9:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > On Fri, Oct 29, 2021 at 09:37:53PM +0200, Sergio Paracuellos wrote:
> > > > > > > On Fri, Oct 29, 2021 at 8:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> > > > > > > > One way might be to implement a
> > > > > > > > pcibios_root_bridge_prepare() for mips and put the
> > > > > > > > setup_cm_memory_region() stuff in there.  It's not
> > > > > > > > *ideal* because that's a strong/weak function
> > > > > > > > arrangement that doesn't allow for multiple host
> > > > > > > > bridges, but that's probably not an issue here.
> > > > > > > >
> > > > > > > > If we can't do that, I think making it bool is probably
> > > > > > > > the right answer, but it would be worth a brief comment
> > > > > > > > in the commit log to explain the issue.
> > > > > > >
> > > > > > > Do you mean to implement 'pcibios_root_bridge_prepare()'
> > > > > > > for MIPS ralink? I guess this means to parse device tree
> > > > > > > and so on only to get memory range addresses to be added
> > > > > > > to the MIPS I/O coherence regions to make things work and
> > > > > > > then re-parse it again in the driver to do the proper PCI
> > > > > > > setup... We end up in an arch generic driver but at the
> > > > > > > end this controller is only present in ralink MIPS, so I
> > > > > > > am not sure that implementing
> > > > > > > 'pcibios_root_bridge_prepare()' is worthy here... I can
> > > > > > > explore and try to implement it if you think that it
> > > > > > > really makes sense... but, IMHO if this is the case, just
> > > > > > > making it bool looks like the correct thing to do.
> > > > > >
> > > > > > It should be trivial to put the contents of
> > > > > > setup_cm_memory_region() into a ralink function called
> > > > > > pcibios_root_bridge_prepare().
> > > > > >
> > > > > > pcibios_root_bridge_prepare() is called with the same
> > > > > > "struct pci_host_bridge *" argument as
> > > > > > setup_cm_memory_region(), and it's called slightly later, so
> > > > > > the window resources are already set up, so no DT parsing is
> > > > > > required.  It looks like a simple move and rename to me.
> > > > >
> > > > > I see. Thanks Bjorn. I will try the approach during the
> > > > > weekend and report if it works.
> > > >
> > > > I have tested the change from 'setup_cm_memory_region()' code
> > > > into 'pcibios_root_bridge_prepare()' just by moving and renaming
> > > > it from the PCIe controller code. The function is properly being
> > > > called.  However, it looks like at that point, windows are not
> > > > setup yet (no windows present at all in bridge->windows) so the
> > > > system is not able to get the IORESOURCE_MEM resource to set up
> > > > the IO coherency unit and the PCI failed to start:
> > > >
> > > > [   16.785359] mt7621-pci 1e140000.pcie: host bridge /pcie@1e140000 ranges:
> > > > [   16.798719] mt7621-pci 1e140000.pcie:   No bus range found for
> > > > /pcie@1e140000, using [bus 00-ff]
> > > > [   16.816248] mt7621-pci 1e140000.pcie:      MEM
> > > > 0x0060000000..0x006fffffff -> 0x0060000000
> > > > [   16.861310] mt7621-pci 1e140000.pcie:       IO
> > > > 0x001e160000..0x001e16ffff -> 0x0000000000
> > > > [   17.179230] mt7621-pci 1e140000.pcie: PCIE0 enabled
> > > > [   17.188954] mt7621-pci 1e140000.pcie: PCIE1 enabled
> > > > [   17.198678] mt7621-pci 1e140000.pcie: PCIE2 enabled
> > > > [   17.208415] Cannot get memory resource
> > > > [   17.215884] mt7621-pci 1e140000.pcie: Scanning root bridge failed
> > > > [   17.228454] mt7621-pci: probe of 1e140000.pcie failed with error -22
> > > >
> > > > FWIW, when the function is called, I have also tried to set up
> > > > hardcoded addresses. Doing that the IO coherency unit was
> > > > properly set up and PCI properly worked (expected). So, using
> > > > this 'pcibios_root_bridge_prepare()' funcion looks like a
> > > > possible way to go but we need the addresses properly being
> > > > passed into the function.  I've also tried to list
> > > > 'bridge->dma_ranges' and get resources from there instead of
> > > > using the not already setup 'bridge->windows'. There is nothing
> > > > inside that list also. 'bridge->bus->resources' is also empty...
> > > > Am I missing something? I was expecting the bridge passed around
> > > > to be the same that was in PCIe controller code, and it seems it
> > > > is (I printed the bridge pointer itself in driver code before
> > > > calling 'mt7621_pcie_register_host()' and in
> > > > 'pcibios_root_bridge_prepare()' at the begging of the function
> > > > and the pointer is the same) but windows and other stuff are not
> > > > already present there...
> > >
> > > Looking into [0] it looks like resources are temporarily removed
> > > from the list just before call 'pcibios_root_bridge_prepare()'.
> > > Hence the behaviour I am seeing when trying to get them...
> > >
> > > [0]:
> > > https://elixir.bootlin.com/linux/latest/source/drivers/pci/probe.c#L915
> >
> > Can you explain to me, why are resources temporarily removed from
> > the 'bridge->windows' list?
> >
> > Would moving that list split to be done after
> > 'pcibios_root_bridge_prepare()' is called a possibility?
>
> I don't know why the windows are managed that way.  That was added by
> 37d6a0a6f470 ("PCI: Add pci_register_host_bridge() interface").   I
> cc'd Arnd just in case he remembers, but that was a long time ago.

Thanks, 2016 is not so far to already remember why things were done :)).

>
> I don't see any use of bridge->windows in any of the
> pcibios_root_bridge_prepare() functions.  It doesn't *look* like it
> should be used until the coalesce/add code near the end.

I checked them also, and nobody uses 'bridge->windows' there but all the
information is already there from dt parsing process when the
'pci_register_host_bridge()'
is called and we are putting temporally in a 'resources' list to add
again all of them at the end.
All the calls before 'pcibios_root_bridge_prepare()' don't do anything
related with windows
either, that's why I asked to move the split after calling it since,
at a first glance, it does not
look harmful. Let's wait for Arnd explanation about why things are
being doing in this way
and if is this a possible way to proceed.

Best regards,
     Sergio Paracuellos

>
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 4289030b0fff..2132df91ad8b 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -891,8 +891,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->msi = bridge->msi;
> >         bus->ops = bridge->ops;
> > @@ -916,6 +914,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);
> >
> > Obviously doing this works and windows are passed into mips ralink
> > specific 'pcibios_root_bridge_prepare()' and the PCIe subsystem is
> > properly working.
> >
> > The advantages I see to this approach are that doing in this way lets us to:
> > - Remove specific mips code from the driver controller.
> > - Allow the driver to be compile tested for any architecture.
> >
> > And the changes would be the following patches:
> > 1) Small 'drivers/pci/probe.c' change.
> > 2) Move mips specific code into 'arch/mips/ralink/mt76721.c' (since
> > other mips ralink stuff haven't got IO coherency units) to be inside
> > 'pcibios_root_bridge_prepare()'.
> > 3) Add MODULE_LICENSE macro to the PCIe controller driver to avoid
> > complaints when the driver is compiled as a module .
> > 4) Update PCIe controller driver's Kconfig to avoid MIPS COMPILE_TEST
> > conditional and completely enable it for COMPILE_TEST.
> >
> > When you have time, please, let me know your thoughts about this.
> >
> > Thanks in advance for your time.
> >
> > Best regards,
> >     Sergio Paracuellos

Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

[Sergio Paracuellos]

On Wed, Nov 10, 2021 at 7:00 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> On Tue, Nov 9, 2021 at 11:41 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > [+cc Arnd]
> >
> > On Sun, Nov 07, 2021 at 08:00:56AM +0100, Sergio Paracuellos wrote:
> > > On Sat, Oct 30, 2021 at 7:38 AM Sergio Paracuellos
> > > <sergio.paracuellos@gmail.com> wrote:
> > > > On Sat, Oct 30, 2021 at 7:21 AM Sergio Paracuellos
> > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > On Fri, Oct 29, 2021 at 10:27 PM Sergio Paracuellos
> > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > On Fri, Oct 29, 2021 at 9:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > On Fri, Oct 29, 2021 at 09:37:53PM +0200, Sergio Paracuellos wrote:
> > > > > > > > On Fri, Oct 29, 2021 at 8:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > > > > > > > > One way might be to implement a
> > > > > > > > > pcibios_root_bridge_prepare() for mips and put the
> > > > > > > > > setup_cm_memory_region() stuff in there.  It's not
> > > > > > > > > *ideal* because that's a strong/weak function
> > > > > > > > > arrangement that doesn't allow for multiple host
> > > > > > > > > bridges, but that's probably not an issue here.
> > > > > > > > >
> > > > > > > > > If we can't do that, I think making it bool is probably
> > > > > > > > > the right answer, but it would be worth a brief comment
> > > > > > > > > in the commit log to explain the issue.
> > > > > > > >
> > > > > > > > Do you mean to implement 'pcibios_root_bridge_prepare()'
> > > > > > > > for MIPS ralink? I guess this means to parse device tree
> > > > > > > > and so on only to get memory range addresses to be added
> > > > > > > > to the MIPS I/O coherence regions to make things work and
> > > > > > > > then re-parse it again in the driver to do the proper PCI
> > > > > > > > setup... We end up in an arch generic driver but at the
> > > > > > > > end this controller is only present in ralink MIPS, so I
> > > > > > > > am not sure that implementing
> > > > > > > > 'pcibios_root_bridge_prepare()' is worthy here... I can
> > > > > > > > explore and try to implement it if you think that it
> > > > > > > > really makes sense... but, IMHO if this is the case, just
> > > > > > > > making it bool looks like the correct thing to do.
> > > > > > >
> > > > > > > It should be trivial to put the contents of
> > > > > > > setup_cm_memory_region() into a ralink function called
> > > > > > > pcibios_root_bridge_prepare().
> > > > > > >
> > > > > > > pcibios_root_bridge_prepare() is called with the same
> > > > > > > "struct pci_host_bridge *" argument as
> > > > > > > setup_cm_memory_region(), and it's called slightly later, so
> > > > > > > the window resources are already set up, so no DT parsing is
> > > > > > > required.  It looks like a simple move and rename to me.
> > > > > >
> > > > > > I see. Thanks Bjorn. I will try the approach during the
> > > > > > weekend and report if it works.
> > > > >
> > > > > I have tested the change from 'setup_cm_memory_region()' code
> > > > > into 'pcibios_root_bridge_prepare()' just by moving and renaming
> > > > > it from the PCIe controller code. The function is properly being
> > > > > called.  However, it looks like at that point, windows are not
> > > > > setup yet (no windows present at all in bridge->windows) so the
> > > > > system is not able to get the IORESOURCE_MEM resource to set up
> > > > > the IO coherency unit and the PCI failed to start:
> > > > >
> > > > > [   16.785359] mt7621-pci 1e140000.pcie: host bridge /pcie@1e140000 ranges:
> > > > > [   16.798719] mt7621-pci 1e140000.pcie:   No bus range found for
> > > > > /pcie@1e140000, using [bus 00-ff]
> > > > > [   16.816248] mt7621-pci 1e140000.pcie:      MEM
> > > > > 0x0060000000..0x006fffffff -> 0x0060000000
> > > > > [   16.861310] mt7621-pci 1e140000.pcie:       IO
> > > > > 0x001e160000..0x001e16ffff -> 0x0000000000
> > > > > [   17.179230] mt7621-pci 1e140000.pcie: PCIE0 enabled
> > > > > [   17.188954] mt7621-pci 1e140000.pcie: PCIE1 enabled
> > > > > [   17.198678] mt7621-pci 1e140000.pcie: PCIE2 enabled
> > > > > [   17.208415] Cannot get memory resource
> > > > > [   17.215884] mt7621-pci 1e140000.pcie: Scanning root bridge failed
> > > > > [   17.228454] mt7621-pci: probe of 1e140000.pcie failed with error -22
> > > > >
> > > > > FWIW, when the function is called, I have also tried to set up
> > > > > hardcoded addresses. Doing that the IO coherency unit was
> > > > > properly set up and PCI properly worked (expected). So, using
> > > > > this 'pcibios_root_bridge_prepare()' funcion looks like a
> > > > > possible way to go but we need the addresses properly being
> > > > > passed into the function.  I've also tried to list
> > > > > 'bridge->dma_ranges' and get resources from there instead of
> > > > > using the not already setup 'bridge->windows'. There is nothing
> > > > > inside that list also. 'bridge->bus->resources' is also empty...
> > > > > Am I missing something? I was expecting the bridge passed around
> > > > > to be the same that was in PCIe controller code, and it seems it
> > > > > is (I printed the bridge pointer itself in driver code before
> > > > > calling 'mt7621_pcie_register_host()' and in
> > > > > 'pcibios_root_bridge_prepare()' at the begging of the function
> > > > > and the pointer is the same) but windows and other stuff are not
> > > > > already present there...
> > > >
> > > > Looking into [0] it looks like resources are temporarily removed
> > > > from the list just before call 'pcibios_root_bridge_prepare()'.
> > > > Hence the behaviour I am seeing when trying to get them...
> > > >
> > > > [0]:
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/pci/probe.c#L915
> > >
> > > Can you explain to me, why are resources temporarily removed from
> > > the 'bridge->windows' list?
> > >
> > > Would moving that list split to be done after
> > > 'pcibios_root_bridge_prepare()' is called a possibility?
> >
> > I don't know why the windows are managed that way.  That was added by
> > 37d6a0a6f470 ("PCI: Add pci_register_host_bridge() interface").   I
> > cc'd Arnd just in case he remembers, but that was a long time ago.
>
> Thanks, 2016 is not so far to already remember why things were done :)).
>
> >
> > I don't see any use of bridge->windows in any of the
> > pcibios_root_bridge_prepare() functions.  It doesn't *look* like it
> > should be used until the coalesce/add code near the end.
>
> I checked them also, and nobody uses 'bridge->windows' there but all the
> information is already there from dt parsing process when the
> 'pci_register_host_bridge()'
> is called and we are putting temporally in a 'resources' list to add
> again all of them at the end.
> All the calls before 'pcibios_root_bridge_prepare()' don't do anything
> related with windows
> either, that's why I asked to move the split after calling it since,
> at a first glance, it does not
> look harmful. Let's wait for Arnd explanation about why things are
> being doing in this way
> and if is this a possible way to proceed.
>
> Best regards,
>      Sergio Paracuellos
>
> >
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 4289030b0fff..2132df91ad8b 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -891,8 +891,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->msi = bridge->msi;
> > >         bus->ops = bridge->ops;
> > > @@ -916,6 +914,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);
> > >
> > > Obviously doing this works and windows are passed into mips ralink
> > > specific 'pcibios_root_bridge_prepare()' and the PCIe subsystem is
> > > properly working.
> > >
> > > The advantages I see to this approach are that doing in this way lets us to:
> > > - Remove specific mips code from the driver controller.
> > > - Allow the driver to be compile tested for any architecture.
> > >
> > > And the changes would be the following patches:
> > > 1) Small 'drivers/pci/probe.c' change.
> > > 2) Move mips specific code into 'arch/mips/ralink/mt76721.c' (since
> > > other mips ralink stuff haven't got IO coherency units) to be inside
> > > 'pcibios_root_bridge_prepare()'.
> > > 3) Add MODULE_LICENSE macro to the PCIe controller driver to avoid
> > > complaints when the driver is compiled as a module .
> > > 4) Update PCIe controller driver's Kconfig to avoid MIPS COMPILE_TEST
> > > conditional and completely enable it for COMPILE_TEST.

I have sent this just to a clear image of a possible way to go:

https://lkml.org/lkml/2021/11/15/48

Thanks,
    Sergio Paracuellos

> > >
> > > When you have time, please, let me know your thoughts about this.
> > >
> > > Thanks in advance for your time.
> > >
> > > Best regards,
> > >     Sergio Paracuellos