On Thu, Sep 04, 2025 at 04:57:17PM GMT, zhangsenchuan wrote: > Dear Manivannan > > Thank you for your thorough review.Here are some of my clarifications and questions. > Looking forward to your answer, Thank you very much. > > > -----Original Messages----- > > From: "Manivannan Sadhasivam" <mani@xxxxxxxxxx> > > Send time:Monday, 01/09/2025 14:40:41 > > To: zhangsenchuan@xxxxxxxxxxxxxxxxxx > > Cc: bhelgaas@xxxxxxxxxx, lpieralisi@xxxxxxxxxx, kwilczynski@xxxxxxxxxx, robh@xxxxxxxxxx, krzk+dt@xxxxxxxxxx, conor+dt@xxxxxxxxxx, linux-pci@xxxxxxxxxxxxxxx, devicetree@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, p.zabel@xxxxxxxxxxxxxx, johan+linaro@xxxxxxxxxx, quic_schintav@xxxxxxxxxxx, shradha.t@xxxxxxxxxxx, cassel@xxxxxxxxxx, thippeswamy.havalige@xxxxxxx, mayank.rana@xxxxxxxxxxxxxxxx, inochiama@xxxxxxxxx, ningyu@xxxxxxxxxxxxxxxxxx, linmin@xxxxxxxxxxxxxxxxxx, pinkesh.vaghela@xxxxxxxxxxxxxx > > Subject: Re: [PATCH v2 2/2] PCI: eic7700: Add Eswin eic7700 PCIe host controller driver > > > > > + /* config eswin vendor id and eic7700 device id */ > > > + dw_pcie_writel_dbi(pci, PCIE_TYPE_DEV_VEND_ID, 0x20301fe1); > > > > Does it need to be configured all the time? > > Clarification： > Our hardware initialization did not configure the device Id and vendor Id. > Now, we can only rewrite the device Id and vendor Id in the code. > Ok. Then mention it in the comment itself. Like, /* * Configure ESWIN VID:DID for Root Port as the default values are * invalid. */ > > > > > + > > > + /* lane fix config, real driver NOT need, default x4 */ > > > > What do you mean by 'readl driver NOT need'? > > > > Clarification： > Sorry, this was added during the compatibility platform test. It is not needed for real devices. > I will remove it later. > > > > + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); > > > + val &= 0xffffff80; > > > + val |= 0x44; > > > + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val); > > > + > > > + val = dw_pcie_readl_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS); > > > + val &= ~(0x7 << 5); > > > + val |= (0x2 << 5); > > > + dw_pcie_writel_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS, val); > > > + > > > + /* config support 32 msi vectors */ > > > + val = dw_pcie_readl_dbi(pci, PCIE_DSP_PF0_MSI_CAP); > > > + val &= ~PCIE_MSI_MULTIPLE_MSG_MASK; > > > + val |= PCIE_MSI_MULTIPLE_MSG_32; > > > + dw_pcie_writel_dbi(pci, PCIE_DSP_PF0_MSI_CAP, val); > > > + > > > + /* disable msix cap */ > > > > Why? Hw doesn't support MSI-X but it advertises MSI-X capability? > > > > I'm not quite sure what this comment means? Indeed, our hardware doesn't support MSI-X. So it advertises MSI-X in capability by mistake then. If so, do you think it is going to be applicable for future revisions of the controller also? I believe this is a kind of hw bug. Usually, these kind of issues are fixed in future revisions of the SoC. So I was checking if you intend to clear it for all SoCs in the future or not. Otherwise, you may set a flag and clear it conditionally. > We can't disable the MSI-X capability using the PCIE_NEXT_CAP_PTR register? Then which > register is needed to disable the MSI-X capability? > No, my question was not about *how to clear MSI cap*. - Mani -- மணிவண்ணன் சதாசிவம்
