Re: [PATCH v3 5/6] PCI: cadence: Add callback functions for RP and EP controller

[Rob Herring]

On Wed, Apr 23, 2025 at 10:54 PM Manikandan Karunakaran Pillai
<mpillai@xxxxxxxxxxx> wrote:
>
> >> >What exactly is shared between these 2 implementations. Link handling,
> >> >config space accesses, address translation, and host init are all
> >> >different. What's left to share? MSIs (if not passed thru) and
> >> >interrupts? I think it's questionable that this be the same driver.
> >> >
> >> The address of both these have changed as the controller architecture has
> >> changed. In the event these driver have to be same driver, there will lot of
> >> sprinkled "if(is_hpa)" and that was already rejected in earlier version of
> >code.
> >
> >I'm saying they should *not* be the same driver because you don't
> >share hardly anything. Again, what is really common here?
>
> The architecture of the PCie controller is next generation but the software flow
> and functions are almost same. The addresses of the registers accessed for the
> newly added functions have changed and to ensure that we reduce "if(is_hpa)"
> checks, the ops method was adopted as in other existing drivers.

Please listen when I say we do not want the ops method used in other
drivers. That's called a midlayer and is an anti-pattern. Here's some
background reading for you:

https://lwn.net/Articles/708891/
https://blog.ffwll.ch/2016/12/midlayers-once-more-with-feeling.html

So what are you supposed to do with the common parts? Make them a
library that each driver can call into as I already suggested. If you
want an example, see SDHCI drivers and library (sdhci.c). Actually,
the current Cadence support is also an example of this. It's 2
different drivers (pcie-cadence-plat.c and pci-j721e.c) with a library
of functions (pcie-cadence.c). We probably had this same discussion
when all that was upstreamed. Sigh.

Now, where there should be more ops is in struct pci_host_bridge for
things like link state and PERST# control. Then the PCI core could
manage the link state and drivers only have to provide
start/stop/status.

Rob