Re: [PATCH 2/4] MIPS: pci: Add driver for MT7621 PCIe controller

From: Pali Rohár
Date: Wed Jun 02 2021 - 08:23:44 EST

On Wednesday 02 June 2021 14:16:26 Sergio Paracuellos wrote:
> Hi Pali,
>
> On Mon, May 31, 2021 at 4:19 PM Sergio Paracuellos
> <sergio.paracuellos@xxxxxxxxx> wrote:
> >
> > On Mon, May 31, 2021 at 3:50 PM Pali Rohár <pali@xxxxxxxxxx> wrote:
> > >
> > > On Monday 31 May 2021 15:39:55 Sergio Paracuellos wrote:
> > > > Hi Pali,
> > > >
> > > > Thanks for your comments.
> > > >
> > > > On Mon, May 31, 2021 at 3:14 PM Pali Rohár <pali@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Saturday 15 May 2021 14:40:53 Sergio Paracuellos wrote:
> > > > > > This patch adds a driver for the PCIe controller of MT7621 SoC.
> > > > > >
> > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> > > > > > ---
> > > > > > arch/mips/pci/Makefile | 1 +
> > > > > > arch/mips/pci/pci-mt7621.c | 624 +++++++++++++++++++++++++++++++++++++
> > > > > > arch/mips/ralink/Kconfig | 9 +-
> > > > > > 3 files changed, 633 insertions(+), 1 deletion(-)
> > > > > > create mode 100644 arch/mips/pci/pci-mt7621.c
> > > > > >
> > > > > > diff --git a/arch/mips/pci/Makefile b/arch/mips/pci/Makefile
> > > > > > index f3eecc065e5c..178c550739c4 100644
> > > > > > --- a/arch/mips/pci/Makefile
> > > > > > +++ b/arch/mips/pci/Makefile
> > > > > > @@ -24,6 +24,7 @@ obj-$(CONFIG_PCI_AR2315) += pci-ar2315.o
> > > > > > obj-$(CONFIG_SOC_AR71XX) += pci-ar71xx.o
> > > > > > obj-$(CONFIG_PCI_AR724X) += pci-ar724x.o
> > > > > > obj-$(CONFIG_PCI_XTALK_BRIDGE) += pci-xtalk-bridge.o
> > > > > > +obj-$(CONFIG_PCI_MT7621) += pci-mt7621.o
> > > > > > #
> > > > > > # These are still pretty much in the old state, watch, go blind.
> > > > > > #
> > > > > > diff --git a/arch/mips/pci/pci-mt7621.c b/arch/mips/pci/pci-mt7621.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..fe1945819d25
> > > > > > --- /dev/null
> > > > > > +++ b/arch/mips/pci/pci-mt7621.c
> > > > > ...
> > > > > > +static int mt7621_pcie_enable_ports(struct mt7621_pcie *pcie)
> > > > > > +{
> > > > > > + struct device *dev = pcie->dev;
> > > > > > + struct mt7621_pcie_port *port;
> > > > > > + u8 num_slots_enabled = 0;
> > > > > > + u32 slot;
> > > > > > + u32 val;
> > > > > > + int err;
> > > > > > +
> > > > > > + /* Setup MEMWIN and IOWIN */
> > > > > > + pcie_write(pcie, 0xffffffff, RALINK_PCI_MEMBASE);
> > > > > > + pcie_write(pcie, pcie->io.start, RALINK_PCI_IOBASE);
> > > > > > +
> > > > > > + list_for_each_entry(port, &pcie->ports, list) {
> > > > > > + if (port->enabled) {
> > > > > > + err = clk_prepare_enable(port->clk);
> > > > > > + if (err) {
> > > > > > + dev_err(dev, "enabling clk pcie%d\n", slot);
> > > > > > + return err;
> > > > > > + }
> > > > > > +
> > > > > > + mt7621_pcie_enable_port(port);
> > > > > > + dev_info(dev, "PCIE%d enabled\n", port->slot);
> > > > > > + num_slots_enabled++;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + for (slot = 0; slot < num_slots_enabled; slot++) {
> > > > > > + val = read_config(pcie, slot, PCI_COMMAND);
> > > > > > + val |= PCI_COMMAND_MASTER;
> > > > > > + write_config(pcie, slot, PCI_COMMAND, val);
> > > > >
> > > > > Hello! Is this part of code correct? Because it looks strange if PCIe
> > > > > controller driver automatically enables PCI bus mastering, prior device
> > > > > driver initialize itself.
> > > > >
> > > > > Moreover kernel has already function pci_set_master() for this purpose
> > > > > which is used by device drivers.
> > > > >
> > > > > So I think this code can confuse some device drivers...
> > > >
> > > > I agree that we have pci_set_master() to be used in pci device driver
> > > > code. Original controller driver set this bit for enabled slots. Since
> > > > there is no documentation at all for the PCI in this SoC
> > >
> > > I see... this is really a big problem to do any driver development...
> >
> > For sure it is :(.
> >
> > >
> > > > I have
> > > > maintained the setting in the driver in a cleaner way. See original
> > > > driver code and the setting here [0]. There is no other reason than
> > > > that. I am ok with removing this from here and testing with my two
> > > > devices that everything is still ok if having this setting in the pci
> > > > controller driver is a real problem.
> > >
> > > You can run lspci -nnvv with and without PCI_COMMAND_MASTER code and
> > > then compare outputs.
> > >
> > > Device drivers for sure enable PCI_COMMAND_MASTER at the time when it is
> > > needed, so it is possible that there would be no difference in lspci
> > > output.
> >
> > Thanks. I will take this into account when v2 is submitted after more
> > review comments come :).
>
> I have tested to remove this and check lspci -nnvv output with and
> without PCI_COMMAND_MASTER code and, as you pointed out, there is no
> difference between them. Also, both boards are working without
> regressions at all. So I will remove this code for next version.

Perfect!

> Thanks,
> Sergio Paracuellos
> >
> > >
> > > > [0]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-pci/pci-mt7621.c?h=v4.18#n676
> > > >
> > > > Best regards,
> > > > Sergio Paracuellos
> > > > >
> > > > > > + /* configure RC FTS number to 250 when it leaves L0s */
> > > > > > + val = read_config(pcie, slot, PCIE_FTS_NUM);
> > > > > > + val &= ~PCIE_FTS_NUM_MASK;
> > > > > > + val |= PCIE_FTS_NUM_L0(0x50);
> > > > > > + write_config(pcie, slot, PCIE_FTS_NUM, val);

Could you look also what is doing this code (PCIE_FTS_NUM)? It is marked
as MT specific register. But from this code for me it looks like that it
just access config space of some device and therefore it could be some
standard PCIe register. Just with hardcoded calculated offset.

Could you provide output from lspci -nnvv? So other people could look at
it and maybe we decode what is this code doing and if it is needed.

> > > > > > + }
> > > > > > +
> > > > > > + return 0;
> > > > > > +}