Re: [PATCH v4 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver

[Rob Herring <robh+dt@kernel.org>]

On Thu, Jul 21, 2022 at 2:58 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Jul 21, 2022 at 11:04:00AM +0200, Krzysztof Kozlowski wrote:
> > On 20/07/2022 08:01, Wangseok Lee wrote:
> > > Add support Axis, ARTPEC-8 SoC. ARTPEC-8 is the SoC platform of Axis
> > > Communications. This is based on arm64 and support GEN4 & 2lane. This
> > > PCIe controller is based on DesignWare Hardware core and uses DesignWare
> > > core functions to implement the driver. "pcie-artpec6. c" supports artpec6
> > > and artpec7 H/W. artpec8 can not be expanded because H/W configuration is
> > > completely different from artpec6/7. PHY and sub controller are different.
> > >
> > > Signed-off-by: Wangseok Lee <wangseok.lee@samsung.com>
> > > Signed-off-by: Jaeho Cho <jaeho79.cho@samsung.com>
> > > ---
> > > v3->v4 :
> > > -Remove unnecessary enum type
> > > -Fix indentation
> > >
> >
> > Thanks for the changes. This starts to look good, however I am not going
> > to ack it. This is also not a strong NAK, as I would respect Bjorn and
> > other maintainers decision.
> >
> > I don't like the approach of creating only Artpec-8 specific driver.
> > Samsung heavily reuses its block in all Exynos devices. Now it re-uses
> > them for other designs as well. Therefore, even if merging with existing
> > Exynos PCIe driver is not feasible (we had such discussions), I expect
> > this to cover all Samsung Foundry PCIe devices. From all current designs
> > up to future licensed blocks, including some new Samsung Exynos SoC. Or
> > at least be ready for it.
>
> I would certainly prefer fewer drivers but I don't know enough about
> the underlying IP and the places it's integrated to to know what's
> practical.  The only way I could figure that out would be by manually
> comparing the drivers for similarity.  I assume/expect all driver
> authors are doing that.

Sadly, I would not assume that. :(

Just looking at the ELBI registers between Exynos and this one, the
registers look completely different with nothing shared between the 2.
Maybe sharing is possible with some new, yet to be upstreamed Samsung
PCIe IP, but not the existing one. Same vendor is not always a good
reason to share a driver.

I think the way to further shrink the vendor DWC drivers is getting
the DWC core and PCI core to do more in terms of clocks, resets, and
phys management, handling of PERST, and link state management. We
should also get rid of vendor drivers doing their own abstraction
layers (ops structs) (looking at you QCom).

Rob