Received-SPF: pass (google.com: domain of linux-kernel+bounces-117169-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 25 Mar 2024 14:45:09 +0100
From: Niklas Cassel <cassel@kernel.org>
To: Siddharth Vadapalli <s-vadapalli@ti.com>
Cc: lpieralisi@kernel.org, kw@linux.com, robh@kernel.org,
	bhelgaas@google.com, manivannan.sadhasivam@linaro.org,
	fancer.lancer@gmail.com, u.kleine-koenig@pengutronix.de,
	dlemoal@kernel.org, yoshihiro.shimoda.uh@renesas.com,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, srk@ti.com
Subject: Re: [PATCH v4] PCI: keystone: Fix pci_ops for AM654x SoC
Message-ID: <ZgF_5fYsI5lOFjOv@ryzen>
References: <20240325053722.1955433-1-s-vadapalli@ti.com>
 <ZgFemQ8gHpB8yMef@ryzen>
 <ea0294d4-85d1-4784-acd7-dd247165f69b@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ea0294d4-85d1-4784-acd7-dd247165f69b@ti.com>

Hello Siddharth,

On Mon, Mar 25, 2024 at 05:52:28PM +0530, Siddharth Vadapalli wrote:
> On Mon, Mar 25, 2024 at 12:23:05PM +0100, Niklas Cassel wrote:
> > On Mon, Mar 25, 2024 at 11:07:22AM +0530, Siddharth Vadapalli wrote:
> > > @@ -822,6 +788,23 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
> > >  	if (ret < 0)
> > >  		return ret;
> > >
> > 
> > > +	if (!ks_pcie->is_am6) {
> > 
> > Perhaps add a comment here stating WHY this is needed for v3.65a (!is_am6).
> > 
> > From reading the old threads, it appears that v3.65a:
> > -Has no support for iATUs. iATU-specific resource handling code is to be
> >  bypassed for v3.65 h/w. Thus v3.65a has it's own .child_ops implementation,
> >  so that pcie-designware-host.c does not configure the iATUs.
> > -v3.65a has it's own .msi_init implementation, so that pcie-designware-host.c
> >  does not call dw_pcie_msi_host_init() to configure the MSI controller.
> > 
> > While 4.90a:
> > -Does have iATU support.
> > -Does use the generic dw_pcie_msi_host_init().
> > 
> > Considering the major differences (with v3.65a being the outlier) here,
> > I think it would have been a much wiser idea to have two different glue
> > drivers for these two compatibles (ti,keystone-pcie and ti,am654-pcie-rc).
> > 
> > Right now the driver is quite hard to read, most of the functions in this
> > driver exist because v3.65a does not have an iATU and does not use the
> > generic DWC way to handle MSIs. Additionally, you have "if (!ks_pcie->is_am6)"
> > spread out all over the driver, to control quite major things, like if you
> > should overload .child_ops, or if you should set up inbound translation without
> > an iATU. This makes is even harder to see which code is actually used for
> > am654... like the fact that it actually uses the generic way to handle MSIs...
> > 
> > The driver for am654 would be much nicer since many of the functions in
> > this driver would not be needed (and the fact that you have only implemented
> > EP support for am654 and not for v3.65a). All EP related stuff would be in
> > the am654 file/driver.
> > You could keep the quirky stuff for v3.65a in the existing pci-keystone.c
> > driver.
> > 
> > (I guess if there is a function that is identical between the twos, you could
> > have a pci-keystone-common.{c,h}  that can be used by both drivers, but from
> > the looks of it, they seem to share very little code.
> 
> Thank you for reviewing the patch. I agree that two drivers will be
> better considering the !ks_pcie->is_am6 present throughout the driver.
> However, I hope you notice the fact that commit:
> 6ab15b5e7057 PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus
> introduced a regression in a driver which was working prior to that
> commit for AM654. While there are flaws in the driver and it needs to be
> split to handle v3.65a and other versions in a cleaner manner, I am
> unable to understand why that is a precursor to fixing the regression.
> 
> If splitting the driver is the only way to fix this regression, please
> let me know and I will work on that instead, though it will take up more
> time.

I think you are misunderstanding me.

I think this patch is fine, except for the comment that I gave:
"Perhaps add a comment here stating WHY this is needed for v3.65a (!is_am6)."

Like:

/*
 * This is only needed for !am654 since it has its own msi_irq_chip
 * implementation. (am654 uses the generic msi_irq_chip implementation.)
 */
if (!ks_pcie->is_am6) {
	...
}


In fact, if you move this code to ks_pcie_msi_host_init(), instead of
ks_pcie_host_init(), you would not need a comment (or a if (!ks_pcie->is_am6)),
since ks_pcie_msi_host_init() is only executed by !am654.


My suggestion to split this driver to two different drivers is just because
I noticed how different they are (am654 has iATUs, uses generic msi_irq_chip
implementation and has EP-mode support. !am654 has no iATUs, its own MSI
implementation and no EP-mode support.)

So the am654 driver would look like most other DWC glue drivers.
The non-am654 driver would look mostly like it looks today, except you would
remove the EP-mode support.

However, this suggestion can of course be implemented sometime in the future
and should not be a blocker for the patch in $subject.


Kind regards,
Niklas