Message ID | 20210922050035.18162-1-sergio.paracuellos@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver | expand |
On Wed, 22 Sep 2021 07:00:32 +0200, Sergio Paracuellos wrote: > MediaTek MT7621 PCIe subsys supports single Root complex (RC) > with 3 Root Ports. Each Root Ports supports a Gen1 1-lane Link. > Topology is as follows: > > > MT7621 PCIe HOST Topology > > [...] Applied to pci/mt7621, thanks! [1/3] dt-bindings: mt7621-pci: PCIe binding documentation for MT7621 SoCs https://git.kernel.org/lpieralisi/pci/c/e5bc5605e7 [2/3] PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver https://git.kernel.org/lpieralisi/pci/c/5797a2b2bc [3/3] MAINTAINERS: add myself as maintainer of the MT7621 PCI controller driver https://git.kernel.org/lpieralisi/pci/c/eb1d7d438c Thanks, Lorenzo
On Wed, Oct 20, 2021 at 4:23 PM Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Wed, 22 Sep 2021 07:00:32 +0200, Sergio Paracuellos wrote: > > MediaTek MT7621 PCIe subsys supports single Root complex (RC) > > with 3 Root Ports. Each Root Ports supports a Gen1 1-lane Link. > > Topology is as follows: > > > > > > MT7621 PCIe HOST Topology > > > > [...] > > Applied to pci/mt7621, thanks! > > [1/3] dt-bindings: mt7621-pci: PCIe binding documentation for MT7621 SoCs > https://git.kernel.org/lpieralisi/pci/c/e5bc5605e7 > [2/3] PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver > https://git.kernel.org/lpieralisi/pci/c/5797a2b2bc > [3/3] MAINTAINERS: add myself as maintainer of the MT7621 PCI controller driver > https://git.kernel.org/lpieralisi/pci/c/eb1d7d438c > > Thanks, > Lorenzo Thanks, Lorenzo. Best regards, Sergio Paracuellos
On Wed, Oct 20, 2021 at 03:23:45PM +0100, Lorenzo Pieralisi wrote: > On Wed, 22 Sep 2021 07:00:32 +0200, Sergio Paracuellos wrote: > > MediaTek MT7621 PCIe subsys supports single Root complex (RC) > > with 3 Root Ports. Each Root Ports supports a Gen1 1-lane Link. > > Topology is as follows: > > > > > > MT7621 PCIe HOST Topology > > > > [...] > > Applied to pci/mt7621, thanks! > > [1/3] dt-bindings: mt7621-pci: PCIe binding documentation for MT7621 SoCs > https://git.kernel.org/lpieralisi/pci/c/e5bc5605e7 > [2/3] PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver > https://git.kernel.org/lpieralisi/pci/c/5797a2b2bc > [3/3] MAINTAINERS: add myself as maintainer of the MT7621 PCI controller driver > https://git.kernel.org/lpieralisi/pci/c/eb1d7d438c Since this is a PCIe (not conventional PCI) controller, I vote for renaming these from: PCI_MT7621 Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml drivers/pci/controller/pci-mt7621.c to: PCIE_MT7621 Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml drivers/pci/controller/pcie-mt7621.c We have a mix of these, with many of the early PCIe drivers being named "pci", but I think that was my mistake and there's no reason to continue it. I can do this locally unless somebody objects.
Hi Bjorn, On Thu, Oct 21, 2021 at 5:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Oct 20, 2021 at 03:23:45PM +0100, Lorenzo Pieralisi wrote: > > On Wed, 22 Sep 2021 07:00:32 +0200, Sergio Paracuellos wrote: > > > MediaTek MT7621 PCIe subsys supports single Root complex (RC) > > > with 3 Root Ports. Each Root Ports supports a Gen1 1-lane Link. > > > Topology is as follows: > > > > > > > > > MT7621 PCIe HOST Topology > > > > > > [...] > > > > Applied to pci/mt7621, thanks! > > > > [1/3] dt-bindings: mt7621-pci: PCIe binding documentation for MT7621 SoCs > > https://git.kernel.org/lpieralisi/pci/c/e5bc5605e7 > > [2/3] PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver > > https://git.kernel.org/lpieralisi/pci/c/5797a2b2bc > > [3/3] MAINTAINERS: add myself as maintainer of the MT7621 PCI controller driver > > https://git.kernel.org/lpieralisi/pci/c/eb1d7d438c > > Since this is a PCIe (not conventional PCI) controller, I vote for > renaming these from: > > PCI_MT7621 > Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml > drivers/pci/controller/pci-mt7621.c > > to: > > PCIE_MT7621 > Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml > drivers/pci/controller/pcie-mt7621.c > > We have a mix of these, with many of the early PCIe drivers being > named "pci", but I think that was my mistake and there's no reason to > continue it. I see. > > I can do this locally unless somebody objects. I have no problem at all. Only one question. Do you mean to change compatible string also, or only the name of the file? Let me know if I have to do anything. Thanks, Sergio Paracuellos
On Thu, Oct 21, 2021 at 07:27:21PM +0200, Sergio Paracuellos wrote: > On Thu, Oct 21, 2021 at 5:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > Since this is a PCIe (not conventional PCI) controller, I vote for > > renaming these from: > > > > PCI_MT7621 > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml > > drivers/pci/controller/pci-mt7621.c > > > > to: > > > > PCIE_MT7621 > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml > > drivers/pci/controller/pcie-mt7621.c > > > > We have a mix of these, with many of the early PCIe drivers being > > named "pci", but I think that was my mistake and there's no reason to > > continue it. > > I see. > > > > > I can do this locally unless somebody objects. > > I have no problem at all. Only one question. Do you mean to change > compatible string also, or only the name of the file? Let me know if I > have to do anything. I didn't change the compatible string, to avoid a DT incompatibility. But I *did* change the Kconfig symbol to PCIE_MT7621, which could require changes to out-of-tree .configs. I'm open to suggestions either way for both things.
On Thu, Oct 21, 2021 at 8:11 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Oct 21, 2021 at 07:27:21PM +0200, Sergio Paracuellos wrote: > > On Thu, Oct 21, 2021 at 5:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > Since this is a PCIe (not conventional PCI) controller, I vote for > > > renaming these from: > > > > > > PCI_MT7621 > > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml > > > drivers/pci/controller/pci-mt7621.c > > > > > > to: > > > > > > PCIE_MT7621 > > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml > > > drivers/pci/controller/pcie-mt7621.c > > > > > > We have a mix of these, with many of the early PCIe drivers being > > > named "pci", but I think that was my mistake and there's no reason to > > > continue it. > > > > I see. > > > > > > > > I can do this locally unless somebody objects. > > > > I have no problem at all. Only one question. Do you mean to change > > compatible string also, or only the name of the file? Let me know if I > > have to do anything. > > I didn't change the compatible string, to avoid a DT incompatibility. > But I *did* change the Kconfig symbol to PCIE_MT7621, which could > require changes to out-of-tree .configs. I'm open to suggestions > either way for both things. IMHO, I do think we should not worry about out-of-tree stuff at all. If the correct way to define the Kconfig symbol or the compatible string is to change them, just do that. MT7621 SoC is extensively used by openWRT community. As far as I have seen until now, the way of doing things there is to take the latest long term kernel (now they are using 5.4 as stable and 5.10 as testing kernel), apply a bunch of patches they have and do a complete build of both kernel, device tree and rootfs. So I guess it is not a big problem if we also change compatible string since when an update is performed for a device all of the stuff is just replaced. Maybe I am wrong and John has a different opinion... John, any comments on this? Best regards, Sergio Paracuellos
On Thu, Oct 21, 2021 at 09:23:35PM +0200, Sergio Paracuellos wrote: > On Thu, Oct 21, 2021 at 8:11 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Thu, Oct 21, 2021 at 07:27:21PM +0200, Sergio Paracuellos wrote: > > > On Thu, Oct 21, 2021 at 5:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > Since this is a PCIe (not conventional PCI) controller, I vote for > > > > renaming these from: > > > > > > > > PCI_MT7621 > > > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml > > > > drivers/pci/controller/pci-mt7621.c > > > > > > > > to: > > > > > > > > PCIE_MT7621 > > > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml > > > > drivers/pci/controller/pcie-mt7621.c > > > > > > > > We have a mix of these, with many of the early PCIe drivers being > > > > named "pci", but I think that was my mistake and there's no reason to > > > > continue it. > > > > > > I see. > > > > > > > > > > > I can do this locally unless somebody objects. > > > > > > I have no problem at all. Only one question. Do you mean to change > > > compatible string also, or only the name of the file? Let me know if I > > > have to do anything. > > > > I didn't change the compatible string, to avoid a DT incompatibility. > > But I *did* change the Kconfig symbol to PCIE_MT7621, which could > > require changes to out-of-tree .configs. I'm open to suggestions > > either way for both things. > > IMHO, I do think we should not worry about out-of-tree stuff at all. For Kconfig I tend to agree. For DT I see some "bindings" in the staging tree are being deleted and published as official DT bindings with this patchset but I believe we still have to keep the compatible string backward compatibility regardless because there may be firmware out there using it. Rob, what's the standard policy that should be used in this case ? Thanks, Lorenzo > If the correct way to define the Kconfig symbol or the compatible > string is to change them, just do that. MT7621 SoC is extensively used > by openWRT community. As far as I have seen until now, the way of > doing things there is to take the latest long term kernel (now they > are using 5.4 as stable and 5.10 as testing kernel), apply a bunch of > patches they have and do a complete build of both kernel, device tree > and rootfs. So I guess it is not a big problem if we also change > compatible string since when an update is performed for a device all > of the stuff is just replaced. Maybe I am wrong and John has a > different opinion... John, any comments on this? > > Best regards, > Sergio Paracuellos
On Fri, Oct 22, 2021 at 10:35 AM Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Thu, Oct 21, 2021 at 09:23:35PM +0200, Sergio Paracuellos wrote: > > On Thu, Oct 21, 2021 at 8:11 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > On Thu, Oct 21, 2021 at 07:27:21PM +0200, Sergio Paracuellos wrote: > > > > On Thu, Oct 21, 2021 at 5:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > Since this is a PCIe (not conventional PCI) controller, I vote for > > > > > renaming these from: > > > > > > > > > > PCI_MT7621 > > > > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml > > > > > drivers/pci/controller/pci-mt7621.c > > > > > > > > > > to: > > > > > > > > > > PCIE_MT7621 > > > > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml > > > > > drivers/pci/controller/pcie-mt7621.c > > > > > > > > > > We have a mix of these, with many of the early PCIe drivers being > > > > > named "pci", but I think that was my mistake and there's no reason to > > > > > continue it. > > > > > > > > I see. > > > > > > > > > > > > > > I can do this locally unless somebody objects. > > > > > > > > I have no problem at all. Only one question. Do you mean to change > > > > compatible string also, or only the name of the file? Let me know if I > > > > have to do anything. > > > > > > I didn't change the compatible string, to avoid a DT incompatibility. > > > But I *did* change the Kconfig symbol to PCIE_MT7621, which could > > > require changes to out-of-tree .configs. I'm open to suggestions > > > either way for both things. > > > > IMHO, I do think we should not worry about out-of-tree stuff at all. > > For Kconfig I tend to agree. For DT I see some "bindings" in the staging > tree are being deleted and published as official DT bindings with this > patchset but I believe we still have to keep the compatible string > backward compatibility regardless because there may be firmware out > there using it. The bindings txt file removed in staging with this patchset was also added by me three years ago[0], and has been changing until the YAML bindings are reviewed by Rob and driver updated accordly in this patchset. OpenWRT maintains its own file[1] which I don't know is updated or not according to the one in staging which I am pretending to properly mainline for 5.17. But yes, I agree there might be firmware out there using current compatible string. [0]: Commit 5451e22618b8 ("staging: mt7621-pci: dt-bindings: add dt bindings for mt7621 pcie controller") [1]: https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621.dtsi Best regards, Sergio Paracuellos > > Rob, what's the standard policy that should be used in this case ? > > Thanks, > Lorenzo > > > If the correct way to define the Kconfig symbol or the compatible > > string is to change them, just do that. MT7621 SoC is extensively used > > by openWRT community. As far as I have seen until now, the way of > > doing things there is to take the latest long term kernel (now they > > are using 5.4 as stable and 5.10 as testing kernel), apply a bunch of > > patches they have and do a complete build of both kernel, device tree > > and rootfs. So I guess it is not a big problem if we also change > > compatible string since when an update is performed for a device all > > of the stuff is just replaced. Maybe I am wrong and John has a > > different opinion... John, any comments on this? > > > > Best regards, > > Sergio Paracuellos
On Fri, Oct 22, 2021 at 11:13:39AM +0200, Sergio Paracuellos wrote: > On Fri, Oct 22, 2021 at 10:35 AM Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > > > On Thu, Oct 21, 2021 at 09:23:35PM +0200, Sergio Paracuellos wrote: > > > On Thu, Oct 21, 2021 at 8:11 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > > > On Thu, Oct 21, 2021 at 07:27:21PM +0200, Sergio Paracuellos wrote: > > > > > On Thu, Oct 21, 2021 at 5:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > Since this is a PCIe (not conventional PCI) controller, I > > > > > > vote for renaming these from: > > > > > > > > > > > > PCI_MT7621 > > > > > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml > > > > > > drivers/pci/controller/pci-mt7621.c > > > > > > > > > > > > to: > > > > > > > > > > > > PCIE_MT7621 > > > > > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml > > > > > > drivers/pci/controller/pcie-mt7621.c > > > > > > > > > > > > We have a mix of these, with many of the early PCIe > > > > > > drivers being named "pci", but I think that was my mistake > > > > > > and there's no reason to continue it. > > > > > > > > > > I see. > > > > > > > > > > > I can do this locally unless somebody objects. > > > > > > > > > > I have no problem at all. Only one question. Do you mean to > > > > > change compatible string also, or only the name of the file? > > > > > Let me know if I have to do anything. > > > > > > > > I didn't change the compatible string, to avoid a DT > > > > incompatibility. But I *did* change the Kconfig symbol to > > > > PCIE_MT7621, which could require changes to out-of-tree > > > > .configs. I'm open to suggestions either way for both things. > > > > > > IMHO, I do think we should not worry about out-of-tree stuff at > > > all. > > > > For Kconfig I tend to agree. For DT I see some "bindings" in the > > staging tree are being deleted and published as official DT > > bindings with this patchset but I believe we still have to keep > > the compatible string backward compatibility regardless because > > there may be firmware out there using it. > > The bindings txt file removed in staging with this patchset was also > added by me three years ago[0], and has been changing until the YAML > bindings are reviewed by Rob and driver updated accordly in this > patchset. > > OpenWRT maintains its own file[1] which I don't know is updated or > not according to the one in staging which I am pretending to > properly mainline for 5.17. But yes, I agree there might be firmware > out there using current compatible string. > > [0]: Commit 5451e22618b8 ("staging: mt7621-pci: dt-bindings: add dt > bindings for mt7621 pcie controller") > [1]: https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621.dtsi OK, for now I left my rework as-is: - changed CONFIG_PCI_MT7621 to CONFIG_PCIE_MT7621 - renamed mediatek,mt7621-pci.yaml to mediatek,mt7621-pcie.yaml - renamed pci-mt7621.c to pcie-mt7621.c - kept DT compatible string "mediatek,mt7621-pci" in .yaml and .c I reason that the Kconfig and filename changes only affect people building kernels or DTs, but a compatible string change would force a DT update to be synchronized with a kernel update. Happy to change this if necessary. Bjorn
On Mon, Oct 25, 2021 at 11:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Oct 22, 2021 at 11:13:39AM +0200, Sergio Paracuellos wrote: > > On Fri, Oct 22, 2021 at 10:35 AM Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > On Thu, Oct 21, 2021 at 09:23:35PM +0200, Sergio Paracuellos wrote: > > > > On Thu, Oct 21, 2021 at 8:11 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > > > > > On Thu, Oct 21, 2021 at 07:27:21PM +0200, Sergio Paracuellos wrote: > > > > > > On Thu, Oct 21, 2021 at 5:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > > Since this is a PCIe (not conventional PCI) controller, I > > > > > > > vote for renaming these from: > > > > > > > > > > > > > > PCI_MT7621 > > > > > > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml > > > > > > > drivers/pci/controller/pci-mt7621.c > > > > > > > > > > > > > > to: > > > > > > > > > > > > > > PCIE_MT7621 > > > > > > > Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml > > > > > > > drivers/pci/controller/pcie-mt7621.c > > > > > > > > > > > > > > We have a mix of these, with many of the early PCIe > > > > > > > drivers being named "pci", but I think that was my mistake > > > > > > > and there's no reason to continue it. > > > > > > > > > > > > I see. > > > > > > > > > > > > > I can do this locally unless somebody objects. > > > > > > > > > > > > I have no problem at all. Only one question. Do you mean to > > > > > > change compatible string also, or only the name of the file? > > > > > > Let me know if I have to do anything. > > > > > > > > > > I didn't change the compatible string, to avoid a DT > > > > > incompatibility. But I *did* change the Kconfig symbol to > > > > > PCIE_MT7621, which could require changes to out-of-tree > > > > > .configs. I'm open to suggestions either way for both things. > > > > > > > > IMHO, I do think we should not worry about out-of-tree stuff at > > > > all. > > > > > > For Kconfig I tend to agree. For DT I see some "bindings" in the > > > staging tree are being deleted and published as official DT > > > bindings with this patchset but I believe we still have to keep > > > the compatible string backward compatibility regardless because > > > there may be firmware out there using it. > > > > The bindings txt file removed in staging with this patchset was also > > added by me three years ago[0], and has been changing until the YAML > > bindings are reviewed by Rob and driver updated accordly in this > > patchset. > > > > OpenWRT maintains its own file[1] which I don't know is updated or > > not according to the one in staging which I am pretending to > > properly mainline for 5.17. But yes, I agree there might be firmware > > out there using current compatible string. > > > > [0]: Commit 5451e22618b8 ("staging: mt7621-pci: dt-bindings: add dt > > bindings for mt7621 pcie controller") > > [1]: https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621.dtsi > > OK, for now I left my rework as-is: > > - changed CONFIG_PCI_MT7621 to CONFIG_PCIE_MT7621 > - renamed mediatek,mt7621-pci.yaml to mediatek,mt7621-pcie.yaml > - renamed pci-mt7621.c to pcie-mt7621.c > - kept DT compatible string "mediatek,mt7621-pci" in .yaml and .c > > I reason that the Kconfig and filename changes only affect people > building kernels or DTs, but a compatible string change would force a > DT update to be synchronized with a kernel update. This is all ok for me, Bjorn. Thanks for doing this. I guess even if we don't force people to a DT update to synchronize things, since bindings have been changed until they have been approved, I guess most people must upgrade from early not approved DT early versions in any case. But in any case I guess that maintaining the compatible string is the safest thing to do. > > Happy to change this if necessary. Best regards, Sergio Paracuellos > > Bjorn