Hi Bjorn,

On Fri, Oct 29, 2021 at 8:49 PM Bjorn Helgaas [off-list ref] wrote:

On Fri, Oct 29, 2021 at 07:28:47AM +0200, Sergio Paracuellos wrote:

quoted

On Thu, Oct 28, 2021 at 10:47 PM Bjorn Helgaas [off-list ref] wrote:

quoted

On Thu, Oct 28, 2021 at 11:59:17AM +0200, Sergio Paracuellos wrote:

quoted

On Thu, Oct 28, 2021 at 11:34 AM Sergio Paracuellos
[off-list ref] wrote:

quoted

On Thu, Oct 28, 2021 at 11:24 AM Thomas Bogendoerfer
[off-list ref] wrote:

quoted

On Thu, Oct 28, 2021 at 06:11:18AM +0200, Sergio Paracuellos wrote:

quoted

On Thu, Oct 28, 2021 at 6:05 AM Yanteng Si [off-list ref] wrote:

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel