On Mon, Mar 10, 2025 at 11:09:54PM +0800, Hans Zhang wrote: > > > On 2025/3/9 18:02, Siddharth Vadapalli wrote: > > > Hi Siddharth, > > > > > > Prior to this patch, I don't think hard-coded is that reasonable. Because > > > the SOC design of each company does not guarantee that the offset of each > > > capability is the same. This parameter can be configured when selecting PCIe > > > configuration options. The current code that just happens to hit the offset > > > address is the same. > > > > 1. You are modifying the driver for the Cadence PCIe Controller IP and > > not the one for your SoC (a.k.a the application/glue/wrapper driver). > > 2. The offsets are tied to the Controller IP and not to your SoC. Any > > differences that arise due to IP Integration into your SoC should be > > handled in the Glue driver (the one which you haven't upstreamed yet). > > 3. If the offsets in the Controller IP itself have changed, this > > indicates a different IP version which has nothing to do with the SoC > > that you are using. > > > > Please clarify whether you are facing problems with the offsets due to a > > difference in the IP Controller Version, or due to the way in which the IP > > was integrated into your SoC. > > > > Hi Siddharth, > > I have consulted several PCIe RTL designers in the past two days. They told > me that the controller IP of Synopsys or Cadence can be configured with the > offset address of the capability. I don't think it has anything to do with > SOC, it's just a feature of PCIe controller IP. In particular, the number of > extended capability is relatively large. When RTL is generated, one more > configuration may cause the offset addresses of extended capability to be > different. Therefore, it is unreasonable to assign all the offset addresses > in advance. > > Here, I want to make Cadence PCIe common driver more general. When we keep > developing new SoCs, the capability or extended capability offset address > may eventually be inconsistent. > > Thank you very much Siddharth for discussing this patch with me. I would > like to know what other maintainers have to say about this. > Even though this patch is mostly for an out of tree controller driver which is not going to be upstreamed, the patch itself is serving some purpose. I really like to avoid the hardcoded offsets wherever possible. So I'm in favor of this patch. However, these newly introduced functions are a duplicated version of DWC functions. So we will end up with duplicated functions in multiple places. I'd like them to be moved (both this and DWC) to drivers/pci/pci.c if possible. The generic function *_find_capability() can accept the controller specific readl/ readw APIs and the controller specific private data. - Mani -- மணிவண்ணன் சதாசிவம்
