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
.-------.
| |
| CPU |
| |
'-------'
|
|
|
v
.------------------.
.-----------| HOST/PCI Bridge |------------.
| '------------------' | Type1
BUS0 | | | Access
v v v On Bus0
.-------------. .-------------. .-------------.
| VIRTUAL P2P | | VIRTUAL P2P | | VIRTUAL P2P |
| BUS0 | | BUS0 | | BUS0 |
| DEV0 | | DEV1 | | DEV2 |
'-------------' '-------------' '-------------'
Type0 | Type0 | Type0 |
Access BUS1 | Access BUS2| Access BUS3|
On Bus1 v On Bus2 v On Bus3 v
.----------. .----------. .----------.
| Device 0 | | Device 0 | | Device 0 |
| Func 0 | | Func 0 | | Func 0 |
'----------' '----------' '----------'
This driver has been very long time in staging and I have been cleaning
it from its first versions where there was code kaos and PCI_LEGACY support.
Original code came probably from openWRT based on mediatek's SDK code. There
is no documentation at all about the mt7621 PCI subsystem.
I have been cleaning it targeting mt7621 SoC which is the one I use in
my GNUBee PC1 board and HiLink HLK-MT7621A evaluation board.
Now I think is clean enough to be moved into 'arch/mips/pci'.
This driver also uses already mainlined pci phy driver located in
'drivers/phy/ralink/phy-mt7621-pci.c'. There are two instances of
the phy being the first one dual ported for pci0 and pci1, and the
second one not dual ported dedicated to pci2. Because of writing twice
some phy registers of the dual-ported one sometimes become in not
confident boot cycles we have to take care of this when device link
is checked here in controller driver. We power on the dual ported-phy
if there is something connected in pcie0 or pcie1. In the same manner
we have to properly disable it only if nothing is connected in of both
pcie0 and pci1 slots.
Another thing that must be mentioned is that this driver uses IO
in physical address 0x001e160000. IO_SPACE_LIMIT for MIPS is 0xffff
so some generic PCI functions (like of_pci_range_to_resource) won't
work and the resource ranges part for IO is set manually.
I had already sent binding documentation to be reviewed but I am
include also here with the driver itself and this cover letter
to make easy review process.
Best regards,
Sergio Paracuellos
Sergio Paracuellos (4):
dt-bindings: mt7621-pci: PCIe binding documentation for MT7621 SoCs
MIPS: pci: Add driver for MT7621 PCIe controller
staging: mt7621-pci: remove driver from staging
MAINTAINERS: add myself as maintainer of the MT7621 PCI controller
driver
.../bindings/pci/mediatek,mt7621-pci.yaml | 149 ++++++++++++++++++
MAINTAINERS | 6 +
arch/mips/pci/Makefile | 1 +
.../mt7621-pci => arch/mips/pci}/pci-mt7621.c | 0
arch/mips/ralink/Kconfig | 9 +-
drivers/staging/Kconfig | 2 -
drivers/staging/Makefile | 1 -
drivers/staging/mt7621-pci/Kconfig | 8 -
drivers/staging/mt7621-pci/Makefile | 2 -
drivers/staging/mt7621-pci/TODO | 4 -
.../mt7621-pci/mediatek,mt7621-pci.txt | 104 ------------
11 files changed, 164 insertions(+), 122 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml
rename {drivers/staging/mt7621-pci => arch/mips/pci}/pci-mt7621.c (100%)
delete mode 100644 drivers/staging/mt7621-pci/Kconfig
delete mode 100644 drivers/staging/mt7621-pci/Makefile
delete mode 100644 drivers/staging/mt7621-pci/TODO
delete mode 100644 drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
--
2.25.1
On Sat, May 15, 2021 at 02:40:51PM +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
>
> .-------.
> | |
> | CPU |
> | |
> '-------'
> |
> |
> |
> v
> .------------------.
> .-----------| HOST/PCI Bridge |------------.
> | '------------------' | Type1
> BUS0 | | | Access
> v v v On Bus0
> .-------------. .-------------. .-------------.
> | VIRTUAL P2P | | VIRTUAL P2P | | VIRTUAL P2P |
> | BUS0 | | BUS0 | | BUS0 |
> | DEV0 | | DEV1 | | DEV2 |
> '-------------' '-------------' '-------------'
> Type0 | Type0 | Type0 |
> Access BUS1 | Access BUS2| Access BUS3|
> On Bus1 v On Bus2 v On Bus3 v
> .----------. .----------. .----------.
> | Device 0 | | Device 0 | | Device 0 |
> | Func 0 | | Func 0 | | Func 0 |
> '----------' '----------' '----------'
>
> This driver has been very long time in staging and I have been cleaning
> it from its first versions where there was code kaos and PCI_LEGACY support.
> Original code came probably from openWRT based on mediatek's SDK code. There
> is no documentation at all about the mt7621 PCI subsystem.
> I have been cleaning it targeting mt7621 SoC which is the one I use in
> my GNUBee PC1 board and HiLink HLK-MT7621A evaluation board.
>
> Now I think is clean enough to be moved into 'arch/mips/pci'.
>
> This driver also uses already mainlined pci phy driver located in
> 'drivers/phy/ralink/phy-mt7621-pci.c'. There are two instances of
> the phy being the first one dual ported for pci0 and pci1, and the
> second one not dual ported dedicated to pci2. Because of writing twice
> some phy registers of the dual-ported one sometimes become in not
> confident boot cycles we have to take care of this when device link
> is checked here in controller driver. We power on the dual ported-phy
> if there is something connected in pcie0 or pcie1. In the same manner
> we have to properly disable it only if nothing is connected in of both
> pcie0 and pci1 slots.
>
> Another thing that must be mentioned is that this driver uses IO
> in physical address 0x001e160000. IO_SPACE_LIMIT for MIPS is 0xffff
> so some generic PCI functions (like of_pci_range_to_resource) won't
> work and the resource ranges part for IO is set manually.
>
> I had already sent binding documentation to be reviewed but I am
> include also here with the driver itself and this cover letter
> to make easy review process.
>
> Best regards,
> Sergio Paracuellos
>
> Sergio Paracuellos (4):
> dt-bindings: mt7621-pci: PCIe binding documentation for MT7621 SoCs
> MIPS: pci: Add driver for MT7621 PCIe controller
> staging: mt7621-pci: remove driver from staging
Generally it's better if the move can be done in one commit instead of
an add followed by a remove.
I see there are a bunch of MIPS PCI controller drivers in
arch/mips/pci/, so I see the argument for putting this one there as
well.
But most of the similar drivers are in drivers/pci/controller/, where
I think it's easier to keep them up to date with changes in the PCI
core. Have you considered putting this one there?
> MAINTAINERS: add myself as maintainer of the MT7621 PCI controller
> driver
>
> .../bindings/pci/mediatek,mt7621-pci.yaml | 149 ++++++++++++++++++
> MAINTAINERS | 6 +
> arch/mips/pci/Makefile | 1 +
> .../mt7621-pci => arch/mips/pci}/pci-mt7621.c | 0
> arch/mips/ralink/Kconfig | 9 +-
> drivers/staging/Kconfig | 2 -
> drivers/staging/Makefile | 1 -
> drivers/staging/mt7621-pci/Kconfig | 8 -
> drivers/staging/mt7621-pci/Makefile | 2 -
> drivers/staging/mt7621-pci/TODO | 4 -
> .../mt7621-pci/mediatek,mt7621-pci.txt | 104 ------------
> 11 files changed, 164 insertions(+), 122 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml
> rename {drivers/staging/mt7621-pci => arch/mips/pci}/pci-mt7621.c (100%)
> delete mode 100644 drivers/staging/mt7621-pci/Kconfig
> delete mode 100644 drivers/staging/mt7621-pci/Makefile
> delete mode 100644 drivers/staging/mt7621-pci/TODO
> delete mode 100644 drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
>
> --
> 2.25.1
Hi Bjorn,
On Wed, May 19, 2021 at 10:36 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Sat, May 15, 2021 at 02:40:51PM +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
> >
> > .-------.
> > | |
> > | CPU |
> > | |
> > '-------'
> > |
> > |
> > |
> > v
> > .------------------.
> > .-----------| HOST/PCI Bridge |------------.
> > | '------------------' | Type1
> > BUS0 | | | Access
> > v v v On Bus0
> > .-------------. .-------------. .-------------.
> > | VIRTUAL P2P | | VIRTUAL P2P | | VIRTUAL P2P |
> > | BUS0 | | BUS0 | | BUS0 |
> > | DEV0 | | DEV1 | | DEV2 |
> > '-------------' '-------------' '-------------'
> > Type0 | Type0 | Type0 |
> > Access BUS1 | Access BUS2| Access BUS3|
> > On Bus1 v On Bus2 v On Bus3 v
> > .----------. .----------. .----------.
> > | Device 0 | | Device 0 | | Device 0 |
> > | Func 0 | | Func 0 | | Func 0 |
> > '----------' '----------' '----------'
> >
> > This driver has been very long time in staging and I have been cleaning
> > it from its first versions where there was code kaos and PCI_LEGACY support.
> > Original code came probably from openWRT based on mediatek's SDK code. There
> > is no documentation at all about the mt7621 PCI subsystem.
> > I have been cleaning it targeting mt7621 SoC which is the one I use in
> > my GNUBee PC1 board and HiLink HLK-MT7621A evaluation board.
> >
> > Now I think is clean enough to be moved into 'arch/mips/pci'.
> >
> > This driver also uses already mainlined pci phy driver located in
> > 'drivers/phy/ralink/phy-mt7621-pci.c'. There are two instances of
> > the phy being the first one dual ported for pci0 and pci1, and the
> > second one not dual ported dedicated to pci2. Because of writing twice
> > some phy registers of the dual-ported one sometimes become in not
> > confident boot cycles we have to take care of this when device link
> > is checked here in controller driver. We power on the dual ported-phy
> > if there is something connected in pcie0 or pcie1. In the same manner
> > we have to properly disable it only if nothing is connected in of both
> > pcie0 and pci1 slots.
> >
> > Another thing that must be mentioned is that this driver uses IO
> > in physical address 0x001e160000. IO_SPACE_LIMIT for MIPS is 0xffff
> > so some generic PCI functions (like of_pci_range_to_resource) won't
> > work and the resource ranges part for IO is set manually.
> >
> > I had already sent binding documentation to be reviewed but I am
> > include also here with the driver itself and this cover letter
> > to make easy review process.
> >
> > Best regards,
> > Sergio Paracuellos
> >
> > Sergio Paracuellos (4):
> > dt-bindings: mt7621-pci: PCIe binding documentation for MT7621 SoCs
> > MIPS: pci: Add driver for MT7621 PCIe controller
> > staging: mt7621-pci: remove driver from staging
>
> Generally it's better if the move can be done in one commit instead of
> an add followed by a remove.
I have no problem at all to just move the driver instead of add and
remove. But it is easier to review initially in this way. No other
reason but this one. So, if after review is ok, we can just move this
in the way you are pointing out here.
>
> I see there are a bunch of MIPS PCI controller drivers in
> arch/mips/pci/, so I see the argument for putting this one there as
> well.
>
> But most of the similar drivers are in drivers/pci/controller/, where
> I think it's easier to keep them up to date with changes in the PCI
> core. Have you considered putting this one there?
Most pci drivers in 'arch/mips/pci' are still using PCI_LEGACY stuff.
In contrast mt7621-pci is using current pci generic apis but even most
of the code is generic enough, there is one remaining thing which
depends on mips architecture which is the iocu region configuration
which must be done in the driver itself. This is the only reason to
move this driver into 'arch/mips/pci' instead of
'drivers/pci/controller/'. So... I am all ears to listen to
suggestions for the proper place for this driver. Thomas, do you have
any thoughts on this?
Thanks,
Sergio Paracuellos
>
> > MAINTAINERS: add myself as maintainer of the MT7621 PCI controller
> > driver
> >
> > .../bindings/pci/mediatek,mt7621-pci.yaml | 149 ++++++++++++++++++
> > MAINTAINERS | 6 +
> > arch/mips/pci/Makefile | 1 +
> > .../mt7621-pci => arch/mips/pci}/pci-mt7621.c | 0
> > arch/mips/ralink/Kconfig | 9 +-
> > drivers/staging/Kconfig | 2 -
> > drivers/staging/Makefile | 1 -
> > drivers/staging/mt7621-pci/Kconfig | 8 -
> > drivers/staging/mt7621-pci/Makefile | 2 -
> > drivers/staging/mt7621-pci/TODO | 4 -
> > .../mt7621-pci/mediatek,mt7621-pci.txt | 104 ------------
> > 11 files changed, 164 insertions(+), 122 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml
> > rename {drivers/staging/mt7621-pci => arch/mips/pci}/pci-mt7621.c (100%)
> > delete mode 100644 drivers/staging/mt7621-pci/Kconfig
> > delete mode 100644 drivers/staging/mt7621-pci/Makefile
> > delete mode 100644 drivers/staging/mt7621-pci/TODO
> > delete mode 100644 drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
> >
> > --
> > 2.25.1
On Wed, May 19, 2021 at 11:18:36PM +0200, Sergio Paracuellos wrote:
> On Wed, May 19, 2021 at 10:36 PM Bjorn Helgaas <[email protected]> wrote:
> > But most of the similar drivers are in drivers/pci/controller/, where
> > I think it's easier to keep them up to date with changes in the PCI
> > core. Have you considered putting this one there?
>
> Most pci drivers in 'arch/mips/pci' are still using PCI_LEGACY stuff.
> In contrast mt7621-pci is using current pci generic apis but even most
> of the code is generic enough, there is one remaining thing which
> depends on mips architecture which is the iocu region configuration
> which must be done in the driver itself. This is the only reason to
> move this driver into 'arch/mips/pci' instead of
> 'drivers/pci/controller/'. So... I am all ears to listen to
> suggestions for the proper place for this driver. Thomas, do you have
> any thoughts on this?
I tried to put a pci-xtalk driver into drivers/pci/controller, but
Lorenzo didn't want it there for being MIPS and not DT based. So this
one is DT based, but still MIPS. I'm perfectly fine putting this
driver into drivers/pci/controller/
Lorenzo, what's your opinion ?
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
On Friday 21 May 2021 12:23:38 Thomas Bogendoerfer wrote:
> On Wed, May 19, 2021 at 11:18:36PM +0200, Sergio Paracuellos wrote:
> > On Wed, May 19, 2021 at 10:36 PM Bjorn Helgaas <[email protected]> wrote:
> > > But most of the similar drivers are in drivers/pci/controller/, where
> > > I think it's easier to keep them up to date with changes in the PCI
> > > core. Have you considered putting this one there?
> >
> > Most pci drivers in 'arch/mips/pci' are still using PCI_LEGACY stuff.
> > In contrast mt7621-pci is using current pci generic apis but even most
> > of the code is generic enough, there is one remaining thing which
> > depends on mips architecture which is the iocu region configuration
> > which must be done in the driver itself. This is the only reason to
> > move this driver into 'arch/mips/pci' instead of
> > 'drivers/pci/controller/'. So... I am all ears to listen to
> > suggestions for the proper place for this driver. Thomas, do you have
> > any thoughts on this?
>
> I tried to put a pci-xtalk driver into drivers/pci/controller, but
> Lorenzo didn't want it there for being MIPS and not DT based. So this
> one is DT based, but still MIPS. I'm perfectly fine putting this
> driver into drivers/pci/controller/
In my personal opinion this driver could go into drivers/pci/controller/
On Mon, May 31, 2021 at 03:18:45PM +0200, Pali Roh?r wrote:
> On Friday 21 May 2021 12:23:38 Thomas Bogendoerfer wrote:
> > On Wed, May 19, 2021 at 11:18:36PM +0200, Sergio Paracuellos wrote:
> > > On Wed, May 19, 2021 at 10:36 PM Bjorn Helgaas <[email protected]> wrote:
> > > > But most of the similar drivers are in drivers/pci/controller/, where
> > > > I think it's easier to keep them up to date with changes in the PCI
> > > > core. Have you considered putting this one there?
> > >
> > > Most pci drivers in 'arch/mips/pci' are still using PCI_LEGACY stuff.
> > > In contrast mt7621-pci is using current pci generic apis but even most
> > > of the code is generic enough, there is one remaining thing which
> > > depends on mips architecture which is the iocu region configuration
> > > which must be done in the driver itself. This is the only reason to
> > > move this driver into 'arch/mips/pci' instead of
> > > 'drivers/pci/controller/'. So... I am all ears to listen to
> > > suggestions for the proper place for this driver. Thomas, do you have
> > > any thoughts on this?
> >
> > I tried to put a pci-xtalk driver into drivers/pci/controller, but
> > Lorenzo didn't want it there for being MIPS and not DT based. So this
> > one is DT based, but still MIPS. I'm perfectly fine putting this
> > driver into drivers/pci/controller/
>
> In my personal opinion this driver could go into drivers/pci/controller/
I'm not sure exactly what "PCI_LEGACY" above refers to.
I don't see any direct #includes of arch/mips in the driver. I do see
that it uses mips_cps_numiocu(), which is certainly MIPS-specific.
But we do have some things in drivers/pci/controller/ that only build
on certain arches, enforced mostly by Kconfig rules, so I think you
could do that. We try to make so things can at least be *compiled* on
any arch, but I know that's not always possible.
So I think it would be useful to put this in drivers/pci/controller/
somewhere because it will make it easier to see common patterns and
refactoring opportunities.
Bjorn
Hi Bjorn,
On Fri, Jun 4, 2021 at 9:43 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Mon, May 31, 2021 at 03:18:45PM +0200, Pali Rohár wrote:
> > On Friday 21 May 2021 12:23:38 Thomas Bogendoerfer wrote:
> > > On Wed, May 19, 2021 at 11:18:36PM +0200, Sergio Paracuellos wrote:
> > > > On Wed, May 19, 2021 at 10:36 PM Bjorn Helgaas <[email protected]> wrote:
> > > > > But most of the similar drivers are in drivers/pci/controller/, where
> > > > > I think it's easier to keep them up to date with changes in the PCI
> > > > > core. Have you considered putting this one there?
> > > >
> > > > Most pci drivers in 'arch/mips/pci' are still using PCI_LEGACY stuff.
> > > > In contrast mt7621-pci is using current pci generic apis but even most
> > > > of the code is generic enough, there is one remaining thing which
> > > > depends on mips architecture which is the iocu region configuration
> > > > which must be done in the driver itself. This is the only reason to
> > > > move this driver into 'arch/mips/pci' instead of
> > > > 'drivers/pci/controller/'. So... I am all ears to listen to
> > > > suggestions for the proper place for this driver. Thomas, do you have
> > > > any thoughts on this?
> > >
> > > I tried to put a pci-xtalk driver into drivers/pci/controller, but
> > > Lorenzo didn't want it there for being MIPS and not DT based. So this
> > > one is DT based, but still MIPS. I'm perfectly fine putting this
> > > driver into drivers/pci/controller/
> >
> > In my personal opinion this driver could go into drivers/pci/controller/
>
> I'm not sure exactly what "PCI_LEGACY" above refers to.
I meant most of the drivers there are not using current generic pci
apis but still using pci legacy ones.
>
> I don't see any direct #includes of arch/mips in the driver. I do see
> that it uses mips_cps_numiocu(), which is certainly MIPS-specific.
Yes, mips_cps_numiocu is the only stuff needed and arch specific used
by this driver.
>
> But we do have some things in drivers/pci/controller/ that only build
> on certain arches, enforced mostly by Kconfig rules, so I think you
> could do that. We try to make so things can at least be *compiled* on
> any arch, but I know that's not always possible.
>
> So I think it would be useful to put this in drivers/pci/controller/
> somewhere because it will make it easier to see common patterns and
> refactoring opportunities.
Ok, so I will move the driver into 'drivers/pci/controller/' in v2.
Thanks,
Sergio Paracuellos
>
> Bjorn