Re: [PATCH 2/6] scsi: ufs: ufs-qcom: Add support for UFS device version detection

[Manivannan Sadhasivam]

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

-- 
மணிவண்ணன் சதாசிவம்