On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote: > + Mayank (with whom I discussed this topic internally) > > On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote: >> >> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote: >>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote: >>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: >>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@xxxxxxxxxxx> wrote: >>>>>> From: Qiang Yu <quic_qianyu@xxxxxxxxxxx> >>>>>> >>>>>> Currently, BCR reset and PHY register setting are mandatory for every port >>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset. >>>>>> Different than BCR reset that is used to reset entire PHY including >>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will >>>>>> be reset but PHY registers will be retained, >>>>> I'm sorry, I can't parse this. >>>> The difference between no_csr reset and bcr reset is that no_csr reset >>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers >>>> are programed. After Linux boot up, the registers will not be reset but >>>> keep the value programmed by UEFI if we only do no_csr reset, so we can >>>> skip phy setting. >>> Please fix capitalization of the abbreviations (PHY, BCR) and add >>> similar text to the commit message. >>> >>>>>> which means PHY setting can >>>>>> be skipped during PHY init if PCIe link was enabled in booltloader and only >>>>>> no_csr is toggled after that. >>>>>> >>>>>> Hence, determine whether the PHY has been enabled in bootloader by >>>>>> verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is >>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be >>>>>> established with no_csr reset only. >>>>> This doesn't tell us why we want to do so. The general rule is not to >>>>> depend on the bootloaders at all. The reason is pretty simple: it is >>>>> hard to update bootloaders, while it is relatively easy to update the >>>>> kernel. If the hardware team issues any kind of changes to the >>>>> programming tables, the kernel will get them earlier than the >>>>> bootloader. We're assuming that if a product has shipped, the sequences used to power up the PHY in the bootloader (e.g. for NVMe) are already good. If some tragedy happens and an erratum is needed, we can always introduce a small override with the existing driver infrastructure (i.e. adding a new entry with a couple registers worth of programming sequence, leaving the other values in tact) While updates to the PHY init sequences have happened in the past, I believe they are mostly a result of oneOf: * someone upstreaming a pre-release revision / premature release * someone making a mistake when typing in the numbers Although sometimes there may be an update that isn't a result of any dev fault. It's worth to keep in mind certain values in there can be board specific, as they affect the analog interface. But we haven't seen that being used much if at all. >>>> With this change, we don't need to upstream phy setting for all phys >>>> support no_csr reset, this will save a great deal of efforts and simplify >>>> the phy driver. Our goal is to remove proprietary PCIe firmware operations >>>> from kernel. PHY is just the start and will be followed by controller, >>>> clocks, regulators, etc. If program table need to be changed, the place to >>>> do that will be UEFI. >>> Well, that sounds like a very bad idea. Please don't do that. Linux >>> kernel drivers should not depend on the UEFI or a bootloader. Unless >>> there is a good reason for that, Linux should continue to be able to >>> reset and program the PCIe PHY (as well as all other hw blocks). While I personally like being able to look into what's being programmed onto my hardware myself, this unfortunately does not scale, both from maintainability and reviewability perspectives with tables that host hundreds of essentially magic values. Plus this approach makes it easier to extend to other OSes, without polluting them with the same data. >> I'm wondering if it's really necessary for Linux to be able to program the >> PHY. Perhaps Linux should only care about common aspects defined by the >> PCIe spec like bus scanning, BAR space allocation, and functions provided >> by other PCIe capabilities. As for the specific operations that are >> different on various platforms, it might be more appropriate for the >> firmware to take care of them. This way, the responsibilities can be more >> clearly divided, and the driver could potentially be >> more streamlined. >> > > It is not necessary in an ideal world, but what we have seen is Qcom releasing > updated PHY init sequence after upstreaming the initial PHY driver support. In > that case, the devices with old firmware will become outdated unless the fw is > updated (which is not straightforward compared to updating the kernel). > > But, I do like this idea of reusing the PHY init sequence in the kernel. Though > we cannot just do it for all platforms. Maybe we can enable it on platforms like > compute starting from X1E and see how it goes? Just to minimize the impact if it > didn't go well. X1 is just the first of many here. We can possibly move some existing ones over after confirming that: a) the bootloader programs the PHYs at all b) the values programmed are reasonable on a case-by-case basis to ensure we're not regressing anything. Perhaps a piece of code and a modparam could be added that would read out the values present at boot and compare them to what the kernel has in store. >> On the other hand, since the no_csr reset can retain register values, >> maybe we should still make full use of it, even if we don't want to >> rely on UEFI. For example, during runtime suspend/resume >> (the D3cold -> D0 cycle) > > D3Cold during runtime suspend in bizarre. Very much so. But this extends to system suspend as well - we can still shave a couple ms off the wakeup times if we can skip all the programming. > >> , when re-initializing the PHY, same PHY >> settings will be programmed again. This is a bit redundant. >> > > Hmm, what would happen if the CX collapse happens during system suspend? Will > the PHY registers be retained? PHYs are always(asterisk) backed by some flavor of MX instead Konrad
