On Wed, Sep 20, 2023 at 02:13:13PM +0300, Dmitry Baryshkov wrote: > On Wed, 20 Sept 2023 at 13:23, Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote: > > > > On Wed, Sep 20, 2023 at 01:27:59AM +0300, Dmitry Baryshkov wrote: > > > On Tue, 19 Sept 2023 at 15:08, Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote: > > > > > > > > On Fri, Sep 15, 2023 at 05:31:45AM +0300, Dmitry Baryshkov wrote: > > > > > On 11 September 2023 13:02:50 GMT+03:00, Can Guo <quic_cang@xxxxxxxxxxx> wrote: > > > > > > > > > > > >On 9/11/2023 5:46 PM, Konrad Dybcio wrote: > > > > > >> On 11.09.2023 11:42, Can Guo wrote: > > > > > >>> Hi Konrad, > > > > > >>> > > > > > >>> On 9/11/2023 5:17 PM, Konrad Dybcio wrote: > > > > > >>>> On 11.09.2023 07:59, Can Guo wrote: > > > > > >>>>> From: "Bao D. Nguyen" <quic_nguyenb@xxxxxxxxxxx> > > > > > >>>>> > > > > > >>>>> Retrieve UFS device version from UFS host controller's spare register > > > > > >>>>> which is populated by bootloader, and use the UFS device version together > > > > > >>>>> with host controller's HW version to decide the proper power modes which > > > > > >>>>> should be used to configure the UFS PHY. > > > > > >>>> That sounds a bit fishy.. is there no bootloader-independent > > > > > >>>> solution to that? Can't we bring in the code that the bootloader > > > > > >>>> uses to determine these values? > > > > > >>>> > > > > > >>>> Konrad > > > > > >>> > > > > > >>> Agree, it is. > > > > > >>> > > > > > >>> > > > > > >>> All these complexities come from one request from PHY design team - power saving. > > > > > >>> > > > > > >>> And to achieve power saving, Qualcomm UFS developers are requested to use the > > > > > >>> > > > > > >>> lowest hanging PHY settings which can sustain the Max agreed HS Gear (btw host > > > > > >>> > > > > > >>> and UFS device) during UFS's lifecycle in High Level OS, whereas the power saving > > > > > >>> > > > > > >>> request does not apply to bootloader, which works for only a few seconds during > > > > > >>> > > > > > >>> bootup. Hence, there is no such version detect code in bootloader - it just uses the > > > > > >>> > > > > > >>> highest PHY settings to configure PHY, boot up UFS and put UFS device version in this > > > > > >>> > > > > > >>> register. > > > > > >> First of all, your email client seems to be inserting 2 newlines > > > > > >> instead of 1. If you're using thunderbird, you may want to edit: > > > > > >> > > > > > >> mail.identity.(default or your mail identity idx).default.compose_html > > > > > >> > > > > > >> to `false` > > > > > >> > > > > > >> and add that to your internal wiki page, as I see many @quic folks having > > > > > >> this issue. > > > > > >> > > > > > >> > > > > > >> Going back to the main topic, I don't think we understood each other. > > > > > >> The commit message states: > > > > > >> > > > > > >> > > > > > >> "Retrieve UFS device version from UFS host controller's spare register > > > > > >> which is populated by bootloader" > > > > > >> > > > > > >> > > > > > >> Which means the bootloader is able to somehow determine the value > > > > > >> that's in the spare register and write it there. > > > > > >> > > > > > >> I'm asking whether we can take the logic behind this value and > > > > > >> move it to Linux so that we don't depend on the bootloader to > > > > > >> guarantee it (e.g. Chrome or some other devices with more exotic > > > > > >> fw may not work this way). > > > > > >> > > > > > >> > > > > > >> Konrad > > > > > > > > > > > > > > > > > >There is no logic behind this value at all in bootloader, as I explained, after bootloader > > > > > > > > > > > >initializes UFS, bootloader simply reads UFS's device version (the value you are referring) > > > > > > > > > > > >and write it to the register. But in Linux kernel, we need (or want to know) this value > > > > > > > > > > > >BEFORE we initialize UFS host controller (and UFS device). > > > > > > > > > > Depending on the bootloader behaviour is not an option. For example the kernel might be started via kexec. Or via u-boot. Or grub. Or any other bootloader. So please duplicate the logic to read the UFS version instead. > > > > > > > > > > > > > As Can said, there is no logic in the bootloader. What it does it, after doing > > > > the UFS initialization, it writes the agreed gear (between host and the device) > > > > to this register. And in linux, we use that value to initialize the device > > > > (i.e., not doing init based on the min gear). > > > > > > > > But the important factor here is that, we use this gear value to program the PHY > > > > init sequence. So if there is no hint from the bootloader, linux will program > > > > the min phy sequence (G3/G4) and then once the gear scaling happens, it will > > > > program the max phy sequence (G4/G5). > > > > > > > > Now on recent platforms, the init sequences are not compatible with each other > > > > i.e., once the min seq. is programmed, then before programming max seq. the > > > > registers not common to both seq. should be programmed to default value. In > > > > other words, min seq. specific registers should be reset to the default value. > > > > Otherwise, there will be stability issues in the PHY. > > > > > > I see nothing wrong with adding 'default' register programming to the > > > gear tables. If we have to reset them to the default values to switch > > > the PHY settings, these writes must be a part of the corresponding > > > tables. > > > > > > > Yep, that's what I initially proposed. But Qcom wanted to avoid the cost of > > programming the reset tables in the PHY driver. > > We should not be programming the whole reset table. Only those several > registers that are changed in the lowest settings. > I was referring to "several registers" as the reset table. I should've been more clear. - Mani > > > > Can, could you please check if programming the additional sequence doesn't cause > > any power/performance effect? > > > > - Mani > > > > > > > > > > So to avoid that, if we get the hint from bootloader (always the max supported > > > > gear between host and device), then only one seq. will be programmed. > > > > > > > > Other way to solve this issue is to reset the non common registers in the init > > > > seq. to default value. But that will be an additional overhead. > > > > > > > > But... if the bootloader doesn't populate this register (if the boot device is > > > > not UFS, like in compute platforms), then this whole logic won't work. This > > > > should also be taken into consideration. > > > > > > Yep, that's the dependency on the bootloader. Which we should avoid. > > > > > > > > > > > - Mani > > > > > > > > > > > > > > P.S. you have been asked to fix your email client. Please do so. Or, if you are inserting these linebreaks manually, please stop. > > > > > > > > > > >Thanks, > > > > > > > > > > > >Can Guo. > > > > > > > > > > > > > > > > > > > -- > > > > மணிவண்ணன் சதாசிவம் > > > > > > > > > > > > -- > > > With best wishes > > > Dmitry > > > > -- > > மணிவண்ணன் சதாசிவம் > > > > -- > With best wishes > Dmitry -- மணிவண்ணன் சதாசிவம்
