| Message ID | 1694411968-14413-3-git-send-email-quic_cang@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] scsi: ufs: ufs-qcom: Setup host power mode during init | expand |
On 11 September 2023 13:02:50 GMT+03:00, Can Guo <quic_cang@quicinc.com> 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@quicinc.com> >>>>> >>>>> 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. 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. >
On 11.09.2023 12:02, Can Guo 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@quicinc.com> >>>>> >>>>> 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). Can't you just initialize the PHY at G4 or G5 unconditionally, read back the required info and then decide based on that? Konrad
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@quicinc.com> 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@quicinc.com> > >>>>> > >>>>> 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. 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. - 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. > > >
On Tue, 19 Sept 2023 at 15:08, Manivannan Sadhasivam <mani@kernel.org> 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@quicinc.com> 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@quicinc.com> > > >>>>> > > >>>>> 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. > > 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. > > > > > > > -- > மணிவண்ணன் சதாசிவம்
On Wed, Sep 20, 2023 at 01:27:59AM +0300, Dmitry Baryshkov wrote: > On Tue, 19 Sept 2023 at 15:08, Manivannan Sadhasivam <mani@kernel.org> 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@quicinc.com> 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@quicinc.com> > > > >>>>> > > > >>>>> 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. 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
On Wed, Sep 20, 2023 at 12:23:27PM +0200, Manivannan Sadhasivam 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@kernel.org> 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@quicinc.com> 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@quicinc.com> > > > > >>>>> > > > > >>>>> 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. > > Can, could you please check if programming the additional sequence doesn't cause > any power/performance effect? > I'd like to simplify this conversion as there has been some misunderstanding. First of all in linux, while probing the UFS device by the host controller, it needs to use _some_ gear. So far we were using HS_G2 as that gear and using the PHY init sequence of G3/G4 depending on the SoC. We do not need to use G2 init sequence because, there are only 2 init sequences available for any SoC and since the init sequences are backwards compatible, we mostly use the min init sequence, G3/G4. Even though this incurs slight power consumption during boot, the ufs host controller after probing the device will switch to max gear supported by both entities. If that max is G4/G5, then the respective init sequence will be programmed again. Now the issue is, for the automotive usecases, switching the gears 2 times during boot is affecting the boot KPI (Key Performance Inidicator). So the UFS team came with the idea of populating a spare register in the bootloader with the max gear info that the bootloader has already found out and using the same in the linux for first time itself. This helps linux in using a single gear during probe time. This is what this patch is doing. If for some reason, that register is not populated, then we default to the existing G2 gear and do init twice as the driver is doing currently. I hope this clarifies the intention of this patch. - Mani > - 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 > > -- > மணிவண்ணன் சதாசிவம்
On 11/09/2023 07:59, Can Guo wrote: > From: "Bao D. Nguyen" <quic_nguyenb@quicinc.com> > > 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. > > Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com> > Signed-off-by: Can Guo <quic_cang@quicinc.com> > --- > drivers/ufs/host/ufs-qcom.c | 30 +++++++++++++++++++++++------- > drivers/ufs/host/ufs-qcom.h | 2 ++ > 2 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 710f079..8a9d54f 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -1030,7 +1030,7 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) > | UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP); > } > > - if (host->hw_ver.major > 0x3) > + if (host->hw_ver.major > 0x3 && host->hw_ver.major < 0x5) > hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; > } > > @@ -1038,11 +1038,33 @@ static void ufs_qcom_set_pwr_mode_limits(struct ufs_hba *hba) > { > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > struct ufs_dev_params *host_pwr_cap = &host->host_pwr_cap; > + u32 val, dev_major = 0; > > ufshcd_init_pwr_dev_param(host_pwr_cap); > > /* This driver only supports symmetic gear setting i.e., hs_tx_gear == hs_rx_gear */ > host_pwr_cap->hs_tx_gear = host_pwr_cap->hs_rx_gear = ufs_qcom_get_hs_gear(hba); > + host->phy_gear = host_pwr_cap->hs_rx_gear; > + > + if (host->hw_ver.major < 0x5) { Here you set G2 for < 0x5 > + /* > + * Power up the PHY using the minimum supported gear (UFS_HS_G2). > + * Switching to max gear will be performed during reinit if supported. > + */ > + host->phy_gear = UFS_HS_G2; > + } else { So here is for >= 0x5 > + val = ufshcd_readl(host->hba, REG_UFS_DEBUG_SPARE_CFG); > + dev_major = FIELD_GET(GENMASK(7, 4), val); > + > + if (host->hw_ver.major == 0x5 && (dev_major >= 0x4 || > + dev_major == 0)) { > + /* For UFS 4.0 and newer, or dev version is not populated */ > + host_pwr_cap->hs_rate = PA_HS_MODE_A; > + } else if (dev_major < 0x4 && dev_major > 0) { > + /* For UFS 3.1 and older, apply HS-G4 PHY settings to save power */ > + host->phy_gear = UFS_HS_G4; > + } But behavior of > 0x5 is not clear here, could you clarify it in v2 ? Thanks, Neil > + } > } > > static void ufs_qcom_set_caps(struct ufs_hba *hba) > @@ -1287,12 +1309,6 @@ static int ufs_qcom_init(struct ufs_hba *hba) > dev_warn(dev, "%s: failed to configure the testbus %d\n", > __func__, err); > > - /* > - * Power up the PHY using the minimum supported gear (UFS_HS_G2). > - * Switching to max gear will be performed during reinit if supported. > - */ > - host->phy_gear = UFS_HS_G2; > - > return 0; > > out_variant_clear: > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h > index 4db64d9..e10889f 100644 > --- a/drivers/ufs/host/ufs-qcom.h > +++ b/drivers/ufs/host/ufs-qcom.h > @@ -56,6 +56,8 @@ enum { > UFS_AH8_CFG = 0xFC, > > REG_UFS_CFG3 = 0x271C, > + > + REG_UFS_DEBUG_SPARE_CFG = 0x284C, > }; > > /* QCOM UFS host controller vendor specific debug registers */
On 10/18/23 14:47, Manivannan Sadhasivam wrote: > On Wed, Sep 20, 2023 at 12:23:27PM +0200, Manivannan Sadhasivam 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@kernel.org> 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@quicinc.com> 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@quicinc.com> >>>>>>>>>> >>>>>>>>>> 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. >> >> Can, could you please check if programming the additional sequence doesn't cause >> any power/performance effect? >> > > I'd like to simplify this conversion as there has been some misunderstanding. > > First of all in linux, while probing the UFS device by the host controller, it > needs to use _some_ gear. So far we were using HS_G2 as that gear and using the > PHY init sequence of G3/G4 depending on the SoC. We do not need to use G2 init > sequence because, there are only 2 init sequences available for any SoC and > since the init sequences are backwards compatible, we mostly use the min init > sequence, G3/G4. Even though this incurs slight power consumption during boot, > the ufs host controller after probing the device will switch to max gear > supported by both entities. If that max is G4/G5, then the respective init > sequence will be programmed again. > > Now the issue is, for the automotive usecases, switching the gears 2 times > during boot is affecting the boot KPI (Key Performance Inidicator). So the UFS > team came with the idea of populating a spare register in the bootloader with > the max gear info that the bootloader has already found out and using the same > in the linux for first time itself. This helps linux in using a single gear > during probe time. > > This is what this patch is doing. If for some reason, that register is not > populated, then we default to the existing G2 gear and do init twice as the > driver is doing currently. > > I hope this clarifies the intention of this patch. Yes I understand this, but I am not sure if such tricks should make it upstream.. They depend on specific firmware (unrelated to the hw block itself) and only exist to improve boot times. If the firmware requirement was not at play, I would have no issues with this. Konrad
On Thu, Oct 26, 2023 at 09:31:31PM +0200, Konrad Dybcio wrote: > > > On 10/18/23 14:47, Manivannan Sadhasivam wrote: > > On Wed, Sep 20, 2023 at 12:23:27PM +0200, Manivannan Sadhasivam 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@kernel.org> 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@quicinc.com> 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@quicinc.com> > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > Can, could you please check if programming the additional sequence doesn't cause > > > any power/performance effect? > > > > > > > I'd like to simplify this conversion as there has been some misunderstanding. > > > > First of all in linux, while probing the UFS device by the host controller, it > > needs to use _some_ gear. So far we were using HS_G2 as that gear and using the > > PHY init sequence of G3/G4 depending on the SoC. We do not need to use G2 init > > sequence because, there are only 2 init sequences available for any SoC and > > since the init sequences are backwards compatible, we mostly use the min init > > sequence, G3/G4. Even though this incurs slight power consumption during boot, > > the ufs host controller after probing the device will switch to max gear > > supported by both entities. If that max is G4/G5, then the respective init > > sequence will be programmed again. > > > > Now the issue is, for the automotive usecases, switching the gears 2 times > > during boot is affecting the boot KPI (Key Performance Inidicator). So the UFS > > team came with the idea of populating a spare register in the bootloader with > > the max gear info that the bootloader has already found out and using the same > > in the linux for first time itself. This helps linux in using a single gear > > during probe time. > > > > This is what this patch is doing. If for some reason, that register is not > > populated, then we default to the existing G2 gear and do init twice as the > > driver is doing currently. > > > > I hope this clarifies the intention of this patch. > Yes I understand this, but I am not sure if such tricks should make > it upstream.. They depend on specific firmware (unrelated to the hw > block itself) and only exist to improve boot times. If the firmware > requirement was not at play, I would have no issues with this. > I do not see any issue with depending on firmware writes to spare registers. As I said, if that is not available, the patch will use the existing semantics. There is no rule that says that the driver should _only_ depend on register values populated by the hw. There are drivers in kernel that rely on firmware configuration of registers. For instance, BIOS may write to any PCIe config space registers and PCI core will happily honor them. I do not see any difference between that and what this patch does. - Mani > Konrad
Hi Neil, On 10/27/2023 8:51 PM, Neil Armstrong wrote: > On 18/10/2023 16:02, Neil Armstrong wrote: >> On 11/09/2023 07:59, Can Guo wrote: >>> From: "Bao D. Nguyen" <quic_nguyenb@quicinc.com> >>> >>> 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. >>> >>> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com> >>> Signed-off-by: Can Guo <quic_cang@quicinc.com> >>> --- >>> drivers/ufs/host/ufs-qcom.c | 30 +++++++++++++++++++++++------- >>> drivers/ufs/host/ufs-qcom.h | 2 ++ >>> 2 files changed, 25 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >>> index 710f079..8a9d54f 100644 >>> --- a/drivers/ufs/host/ufs-qcom.c >>> +++ b/drivers/ufs/host/ufs-qcom.c >>> @@ -1030,7 +1030,7 @@ static void ufs_qcom_advertise_quirks(struct >>> ufs_hba *hba) >>> | UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP); >>> } >>> - if (host->hw_ver.major > 0x3) >>> + if (host->hw_ver.major > 0x3 && host->hw_ver.major < 0x5) >>> hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; >>> } >>> @@ -1038,11 +1038,33 @@ static void >>> ufs_qcom_set_pwr_mode_limits(struct ufs_hba *hba) >>> { >>> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >>> struct ufs_dev_params *host_pwr_cap = &host->host_pwr_cap; >>> + u32 val, dev_major = 0; >>> ufshcd_init_pwr_dev_param(host_pwr_cap); >>> /* This driver only supports symmetic gear setting i.e., >>> hs_tx_gear == hs_rx_gear */ >>> host_pwr_cap->hs_tx_gear = host_pwr_cap->hs_rx_gear = >>> ufs_qcom_get_hs_gear(hba); >>> + host->phy_gear = host_pwr_cap->hs_rx_gear; >>> + >>> + if (host->hw_ver.major < 0x5) { >> >> Here you set G2 for < 0x5 >> >>> + /* >>> + * Power up the PHY using the minimum supported gear >>> (UFS_HS_G2). >>> + * Switching to max gear will be performed during reinit if >>> supported. >>> + */ >>> + host->phy_gear = UFS_HS_G2; >>> + } else { >> >> So here is for >= 0x5 >> >>> + val = ufshcd_readl(host->hba, REG_UFS_DEBUG_SPARE_CFG); >>> + dev_major = FIELD_GET(GENMASK(7, 4), val); >>> + >>> + if (host->hw_ver.major == 0x5 && (dev_major >= 0x4 || >>> + dev_major == 0)) { >>> + /* For UFS 4.0 and newer, or dev version is not >>> populated */ >>> + host_pwr_cap->hs_rate = PA_HS_MODE_A; >>> + } else if (dev_major < 0x4 && dev_major > 0) { >>> + /* For UFS 3.1 and older, apply HS-G4 PHY settings to >>> save power */ >>> + host->phy_gear = UFS_HS_G4; >>> + } >> >> But behavior of > 0x5 is not clear here, could you clarify it in v2 ? > > Now SM8650 is public, could you update it for v2 ? For HWs whose hw_ver.major is > 0x5, say SM8650, initially phy_gear == host_pwr_cap->hs_rx_gear, which is HS_G5. If a UFS3.x or older UFS device is connected, we overwrite phy_gear as HS_G4. I don't see an impact to SM8650. Please let me know if I misunderstand anything here. Thanks, Can Guo. > > Thanks, > Neil > >> >> Thanks, >> Neil >
Hi, On 31/10/2023 06:06, Can Guo wrote: > Hi Neil, > > On 10/27/2023 8:51 PM, Neil Armstrong wrote: >> On 18/10/2023 16:02, Neil Armstrong wrote: >>> On 11/09/2023 07:59, Can Guo wrote: >>>> From: "Bao D. Nguyen" <quic_nguyenb@quicinc.com> >>>> >>>> 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. >>>> >>>> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com> >>>> Signed-off-by: Can Guo <quic_cang@quicinc.com> >>>> --- >>>> drivers/ufs/host/ufs-qcom.c | 30 +++++++++++++++++++++++------- >>>> drivers/ufs/host/ufs-qcom.h | 2 ++ >>>> 2 files changed, 25 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >>>> index 710f079..8a9d54f 100644 >>>> --- a/drivers/ufs/host/ufs-qcom.c >>>> +++ b/drivers/ufs/host/ufs-qcom.c >>>> @@ -1030,7 +1030,7 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) >>>> | UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP); >>>> } >>>> - if (host->hw_ver.major > 0x3) >>>> + if (host->hw_ver.major > 0x3 && host->hw_ver.major < 0x5) >>>> hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; >>>> } >>>> @@ -1038,11 +1038,33 @@ static void ufs_qcom_set_pwr_mode_limits(struct ufs_hba *hba) >>>> { >>>> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >>>> struct ufs_dev_params *host_pwr_cap = &host->host_pwr_cap; >>>> + u32 val, dev_major = 0; >>>> ufshcd_init_pwr_dev_param(host_pwr_cap); >>>> /* This driver only supports symmetic gear setting i.e., hs_tx_gear == hs_rx_gear */ >>>> host_pwr_cap->hs_tx_gear = host_pwr_cap->hs_rx_gear = ufs_qcom_get_hs_gear(hba); >>>> + host->phy_gear = host_pwr_cap->hs_rx_gear; >>>> + >>>> + if (host->hw_ver.major < 0x5) { >>> >>> Here you set G2 for < 0x5 >>> >>>> + /* >>>> + * Power up the PHY using the minimum supported gear (UFS_HS_G2). >>>> + * Switching to max gear will be performed during reinit if supported. >>>> + */ >>>> + host->phy_gear = UFS_HS_G2; >>>> + } else { >>> >>> So here is for >= 0x5 >>> >>>> + val = ufshcd_readl(host->hba, REG_UFS_DEBUG_SPARE_CFG); >>>> + dev_major = FIELD_GET(GENMASK(7, 4), val); >>>> + >>>> + if (host->hw_ver.major == 0x5 && (dev_major >= 0x4 || >>>> + dev_major == 0)) { >>>> + /* For UFS 4.0 and newer, or dev version is not populated */ >>>> + host_pwr_cap->hs_rate = PA_HS_MODE_A; >>>> + } else if (dev_major < 0x4 && dev_major > 0) { >>>> + /* For UFS 3.1 and older, apply HS-G4 PHY settings to save power */ >>>> + host->phy_gear = UFS_HS_G4; >>>> + } >>> >>> But behavior of > 0x5 is not clear here, could you clarify it in v2 ? >> >> Now SM8650 is public, could you update it for v2 ? > > For HWs whose hw_ver.major is > 0x5, say SM8650, initially phy_gear == host_pwr_cap->hs_rx_gear, which is HS_G5. > > If a UFS3.x or older UFS device is connected, we overwrite phy_gear as HS_G4. I don't see an impact to SM8650. > > Please let me know if I misunderstand anything here. It's clear now, please add this in a comment when you send a v2, Thanks, Neil > > > Thanks, > > Can Guo. > >> >> Thanks, >> Neil >> >>> >>> Thanks, >>> Neil >>
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 710f079..8a9d54f 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -1030,7 +1030,7 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) | UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP); } - if (host->hw_ver.major > 0x3) + if (host->hw_ver.major > 0x3 && host->hw_ver.major < 0x5) hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; } @@ -1038,11 +1038,33 @@ static void ufs_qcom_set_pwr_mode_limits(struct ufs_hba *hba) { struct ufs_qcom_host *host = ufshcd_get_variant(hba); struct ufs_dev_params *host_pwr_cap = &host->host_pwr_cap; + u32 val, dev_major = 0; ufshcd_init_pwr_dev_param(host_pwr_cap); /* This driver only supports symmetic gear setting i.e., hs_tx_gear == hs_rx_gear */ host_pwr_cap->hs_tx_gear = host_pwr_cap->hs_rx_gear = ufs_qcom_get_hs_gear(hba); + host->phy_gear = host_pwr_cap->hs_rx_gear; + + if (host->hw_ver.major < 0x5) { + /* + * Power up the PHY using the minimum supported gear (UFS_HS_G2). + * Switching to max gear will be performed during reinit if supported. + */ + host->phy_gear = UFS_HS_G2; + } else { + val = ufshcd_readl(host->hba, REG_UFS_DEBUG_SPARE_CFG); + dev_major = FIELD_GET(GENMASK(7, 4), val); + + if (host->hw_ver.major == 0x5 && (dev_major >= 0x4 || + dev_major == 0)) { + /* For UFS 4.0 and newer, or dev version is not populated */ + host_pwr_cap->hs_rate = PA_HS_MODE_A; + } else if (dev_major < 0x4 && dev_major > 0) { + /* For UFS 3.1 and older, apply HS-G4 PHY settings to save power */ + host->phy_gear = UFS_HS_G4; + } + } } static void ufs_qcom_set_caps(struct ufs_hba *hba) @@ -1287,12 +1309,6 @@ static int ufs_qcom_init(struct ufs_hba *hba) dev_warn(dev, "%s: failed to configure the testbus %d\n", __func__, err); - /* - * Power up the PHY using the minimum supported gear (UFS_HS_G2). - * Switching to max gear will be performed during reinit if supported. - */ - host->phy_gear = UFS_HS_G2; - return 0; out_variant_clear: diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h index 4db64d9..e10889f 100644 --- a/drivers/ufs/host/ufs-qcom.h +++ b/drivers/ufs/host/ufs-qcom.h @@ -56,6 +56,8 @@ enum { UFS_AH8_CFG = 0xFC, REG_UFS_CFG3 = 0x271C, + + REG_UFS_DEBUG_SPARE_CFG = 0x284C, }; /* QCOM UFS host controller vendor specific debug registers */