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