| Message ID | 20201130091610.2752-1-stanley.chu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v1] scsi: ufs: Remove pre-defined initial VCC voltage values | expand |
On 11/30/2020 1:16 AM, Stanley Chu wrote: > UFS specficication allows different VCC configurations for UFS devices, > for example, > (1). 2.70V - 3.60V (By default) > (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in > device tree) > (3). 2.40V - 2.70V (Supported since UFS 3.x) > > With the introduction of UFS 3.x products, an issue is happening that > UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC > regulator on UFU 3.x products with VCC configuration (3) used. > > To solve this issue, we simply remove pre-defined initial VCC voltage > values in UFS driver with below reasons, > > 1. UFS specifications do not define how to detect the VCC configuration > supported by attached device. > > 2. Device tree already supports standard regulator properties. > > Therefore VCC voltage shall be defined correctly in device tree, and > shall not be changed by UFS driver. What UFS driver needs to do is simply > enabling or disabling the VCC regulator only. > > This is a RFC conceptional patch. Please help review this and feel > free to feedback any ideas. Once this concept is accepted, and then > I would post a more completed patch series to fix this issue. > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> > --- > drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- > 1 file changed, 1 insertion(+), 9 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c > index a6f76399b3ae..3965be03c136 100644 > --- a/drivers/scsi/ufs/ufshcd-pltfrm.c > +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c > @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, > vreg->max_uA = 0; > } > > - if (!strcmp(name, "vcc")) { > - if (of_property_read_bool(np, "vcc-supply-1p8")) { > - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; > - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; > - } else { > - vreg->min_uV = UFS_VREG_VCC_MIN_UV; > - vreg->max_uV = UFS_VREG_VCC_MAX_UV; > - } > - } else if (!strcmp(name, "vccq")) { > + if (!strcmp(name, "vccq")) { > vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; > vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; > } else if (!strcmp(name, "vccq2")) { > Hi Stanley Thanks for the patch. Bao (nguyenb) was also working towards something similar. Would it be possible for you to take into account the scenario in which the same platform supports both 2.x and 3.x UFS devices? These've different voltage requirements, 2.4v-3.6v. I'm not sure if standard dts regulator properties can support this. -asd
On Mon 30 Nov 03:16 CST 2020, Stanley Chu wrote: > UFS specficication allows different VCC configurations for UFS devices, > for example, > (1). 2.70V - 3.60V (By default) > (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in > device tree) > (3). 2.40V - 2.70V (Supported since UFS 3.x) > > With the introduction of UFS 3.x products, an issue is happening that > UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC > regulator on UFU 3.x products with VCC configuration (3) used. > > To solve this issue, we simply remove pre-defined initial VCC voltage > values in UFS driver with below reasons, > > 1. UFS specifications do not define how to detect the VCC configuration > supported by attached device. > > 2. Device tree already supports standard regulator properties. > > Therefore VCC voltage shall be defined correctly in device tree, and > shall not be changed by UFS driver. What UFS driver needs to do is simply > enabling or disabling the VCC regulator only. > > This is a RFC conceptional patch. Please help review this and feel > free to feedback any ideas. Once this concept is accepted, and then > I would post a more completed patch series to fix this issue. > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> This is the correct thing to do and I would prefer that we did the same for vccq and vccq2 as well - and thereby remove the min_uV and max_uV from ufs_vreg. Regards, Bjorn > --- > drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- > 1 file changed, 1 insertion(+), 9 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c > index a6f76399b3ae..3965be03c136 100644 > --- a/drivers/scsi/ufs/ufshcd-pltfrm.c > +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c > @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, > vreg->max_uA = 0; > } > > - if (!strcmp(name, "vcc")) { > - if (of_property_read_bool(np, "vcc-supply-1p8")) { > - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; > - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; > - } else { > - vreg->min_uV = UFS_VREG_VCC_MIN_UV; > - vreg->max_uV = UFS_VREG_VCC_MAX_UV; > - } > - } else if (!strcmp(name, "vccq")) { > + if (!strcmp(name, "vccq")) { > vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; > vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; > } else if (!strcmp(name, "vccq2")) { > -- > 2.18.0 >
On Mon 30 Nov 16:51 CST 2020, Asutosh Das (asd) wrote: > On 11/30/2020 1:16 AM, Stanley Chu wrote: > > UFS specficication allows different VCC configurations for UFS devices, > > for example, > > (1). 2.70V - 3.60V (By default) > > (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in > > device tree) > > (3). 2.40V - 2.70V (Supported since UFS 3.x) > > > > With the introduction of UFS 3.x products, an issue is happening that > > UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC > > regulator on UFU 3.x products with VCC configuration (3) used. > > > > To solve this issue, we simply remove pre-defined initial VCC voltage > > values in UFS driver with below reasons, > > > > 1. UFS specifications do not define how to detect the VCC configuration > > supported by attached device. > > > > 2. Device tree already supports standard regulator properties. > > > > Therefore VCC voltage shall be defined correctly in device tree, and > > shall not be changed by UFS driver. What UFS driver needs to do is simply > > enabling or disabling the VCC regulator only. > > > > This is a RFC conceptional patch. Please help review this and feel > > free to feedback any ideas. Once this concept is accepted, and then > > I would post a more completed patch series to fix this issue. > > > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> > > --- > > drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- > > 1 file changed, 1 insertion(+), 9 deletions(-) > > > > diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c > > index a6f76399b3ae..3965be03c136 100644 > > --- a/drivers/scsi/ufs/ufshcd-pltfrm.c > > +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c > > @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, > > vreg->max_uA = 0; > > } > > - if (!strcmp(name, "vcc")) { > > - if (of_property_read_bool(np, "vcc-supply-1p8")) { > > - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; > > - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; > > - } else { > > - vreg->min_uV = UFS_VREG_VCC_MIN_UV; > > - vreg->max_uV = UFS_VREG_VCC_MAX_UV; > > - } > > - } else if (!strcmp(name, "vccq")) { > > + if (!strcmp(name, "vccq")) { > > vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; > > vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; > > } else if (!strcmp(name, "vccq2")) { > > > > Hi Stanley > > Thanks for the patch. Bao (nguyenb) was also working towards something > similar. > Would it be possible for you to take into account the scenario in which the > same platform supports both 2.x and 3.x UFS devices? > > These've different voltage requirements, 2.4v-3.6v. > I'm not sure if standard dts regulator properties can support this. > What is the actual voltage requirement for these devices and how does the software know what voltage to pick in this range? Regards, Bjorn > -asd > > > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > Linux Foundation Collaborative Project
On 11/30/2020 3:14 PM, Bjorn Andersson wrote: > On Mon 30 Nov 16:51 CST 2020, Asutosh Das (asd) wrote: > >> On 11/30/2020 1:16 AM, Stanley Chu wrote: >>> UFS specficication allows different VCC configurations for UFS devices, >>> for example, >>> (1). 2.70V - 3.60V (By default) >>> (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in >>> device tree) >>> (3). 2.40V - 2.70V (Supported since UFS 3.x) >>> >>> With the introduction of UFS 3.x products, an issue is happening that >>> UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC >>> regulator on UFU 3.x products with VCC configuration (3) used. >>> >>> To solve this issue, we simply remove pre-defined initial VCC voltage >>> values in UFS driver with below reasons, >>> >>> 1. UFS specifications do not define how to detect the VCC configuration >>> supported by attached device. >>> >>> 2. Device tree already supports standard regulator properties. >>> >>> Therefore VCC voltage shall be defined correctly in device tree, and >>> shall not be changed by UFS driver. What UFS driver needs to do is simply >>> enabling or disabling the VCC regulator only. >>> >>> This is a RFC conceptional patch. Please help review this and feel >>> free to feedback any ideas. Once this concept is accepted, and then >>> I would post a more completed patch series to fix this issue. >>> >>> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> >>> --- >>> drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- >>> 1 file changed, 1 insertion(+), 9 deletions(-) >>> >>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c >>> index a6f76399b3ae..3965be03c136 100644 >>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c >>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c >>> @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, >>> vreg->max_uA = 0; >>> } >>> - if (!strcmp(name, "vcc")) { >>> - if (of_property_read_bool(np, "vcc-supply-1p8")) { >>> - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; >>> - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; >>> - } else { >>> - vreg->min_uV = UFS_VREG_VCC_MIN_UV; >>> - vreg->max_uV = UFS_VREG_VCC_MAX_UV; >>> - } >>> - } else if (!strcmp(name, "vccq")) { >>> + if (!strcmp(name, "vccq")) { >>> vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; >>> vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; >>> } else if (!strcmp(name, "vccq2")) { >>> >> >> Hi Stanley >> >> Thanks for the patch. Bao (nguyenb) was also working towards something >> similar. >> Would it be possible for you to take into account the scenario in which the >> same platform supports both 2.x and 3.x UFS devices? >> >> These've different voltage requirements, 2.4v-3.6v. >> I'm not sure if standard dts regulator properties can support this. >> > > What is the actual voltage requirement for these devices and how does > the software know what voltage to pick in this range? > > Regards, > Bjorn > >> -asd >> >> >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> Linux Foundation Collaborative Project For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), the voltage requirements (Vcc) are 2.4v-3.6v. The software initializes the ufs device at 2.95v & reads the version and if the device is 3.x, it may do the following: - Set the device power mode to SLEEP - Disable the Vcc - Enable the Vcc and set it to 2.5v - Set the device power mode to ACTIVE All of the above may be done at HS-G1 & moved to max supported gear based on the device version, perhaps? Am open to other ideas though. -asd
On Mon, 2020-11-30 at 15:54 -0800, Asutosh Das (asd) wrote: > On 11/30/2020 3:14 PM, Bjorn Andersson wrote: > > On Mon 30 Nov 16:51 CST 2020, Asutosh Das (asd) wrote: > > > >> On 11/30/2020 1:16 AM, Stanley Chu wrote: > >>> UFS specficication allows different VCC configurations for UFS devices, > >>> for example, > >>> (1). 2.70V - 3.60V (By default) > >>> (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in > >>> device tree) > >>> (3). 2.40V - 2.70V (Supported since UFS 3.x) > >>> > >>> With the introduction of UFS 3.x products, an issue is happening that > >>> UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC > >>> regulator on UFU 3.x products with VCC configuration (3) used. > >>> > >>> To solve this issue, we simply remove pre-defined initial VCC voltage > >>> values in UFS driver with below reasons, > >>> > >>> 1. UFS specifications do not define how to detect the VCC configuration > >>> supported by attached device. > >>> > >>> 2. Device tree already supports standard regulator properties. > >>> > >>> Therefore VCC voltage shall be defined correctly in device tree, and > >>> shall not be changed by UFS driver. What UFS driver needs to do is simply > >>> enabling or disabling the VCC regulator only. > >>> > >>> This is a RFC conceptional patch. Please help review this and feel > >>> free to feedback any ideas. Once this concept is accepted, and then > >>> I would post a more completed patch series to fix this issue. > >>> > >>> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> > >>> --- > >>> drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- > >>> 1 file changed, 1 insertion(+), 9 deletions(-) > >>> > >>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c > >>> index a6f76399b3ae..3965be03c136 100644 > >>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c > >>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c > >>> @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, > >>> vreg->max_uA = 0; > >>> } > >>> - if (!strcmp(name, "vcc")) { > >>> - if (of_property_read_bool(np, "vcc-supply-1p8")) { > >>> - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; > >>> - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; > >>> - } else { > >>> - vreg->min_uV = UFS_VREG_VCC_MIN_UV; > >>> - vreg->max_uV = UFS_VREG_VCC_MAX_UV; > >>> - } > >>> - } else if (!strcmp(name, "vccq")) { > >>> + if (!strcmp(name, "vccq")) { > >>> vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; > >>> vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; > >>> } else if (!strcmp(name, "vccq2")) { > >>> > >> > >> Hi Stanley > >> > >> Thanks for the patch. Bao (nguyenb) was also working towards something > >> similar. > >> Would it be possible for you to take into account the scenario in which the > >> same platform supports both 2.x and 3.x UFS devices? > >> > >> These've different voltage requirements, 2.4v-3.6v. > >> I'm not sure if standard dts regulator properties can support this. > >> > > > > What is the actual voltage requirement for these devices and how does > > the software know what voltage to pick in this range? > > > > Regards, > > Bjorn > > > >> -asd > >> > >> > >> -- > >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > >> Linux Foundation Collaborative Project > > For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), the > voltage requirements (Vcc) are 2.4v-3.6v. The software initializes the > ufs device at 2.95v & reads the version and if the device is 3.x, it may > do the following: > - Set the device power mode to SLEEP > - Disable the Vcc > - Enable the Vcc and set it to 2.5v > - Set the device power mode to ACTIVE > > All of the above may be done at HS-G1 & moved to max supported gear > based on the device version, perhaps? Hi Asutosh, Thanks for sharing this idea. 1. I did not see above flow defined in UFS specifications, please correct me if I was wrong. 2. For above flow, the concern is that I am not sure if all devices supporting VCC (2.4v - 2.7v) can accept higher voltage, say 2.95v, for version detection. 3. For version detection, another concern is that I am not sure if all 3.x devices support VCC (2.4v - 2.7v) only, or in other words, I am not sure if all 2.x devices support VCC (2.7v - 3.6v) only. The above rule will break any devices not obeying this "conventions". For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), It would be good for UFS drivers detecting the correct voltage if the protocol is well-defined in specifications. Until that day, any "non-standard" way may be better implemented in vendor's ops? If the vop concept works on your platform, we could still keep struct ufs_vreg and allow vendors to configure proper min_uV and max_uV to make regulator_set_voltage() works during VCC toggling flow. Without specific vendor configurations, min_uV and max_uV would be NULL by default and UFS core driver will only enable/disasble VCC regulator only without adjusting its voltage. Maybe one possible another idea is to decide the correct voltage and configure regulator properly before kernel? Thanks, Stanley Chu > > Am open to other ideas though. > > -asd >
On Mon 30 Nov 17:54 CST 2020, Asutosh Das (asd) wrote: > On 11/30/2020 3:14 PM, Bjorn Andersson wrote: > > On Mon 30 Nov 16:51 CST 2020, Asutosh Das (asd) wrote: > > > > > On 11/30/2020 1:16 AM, Stanley Chu wrote: > > > > UFS specficication allows different VCC configurations for UFS devices, > > > > for example, > > > > (1). 2.70V - 3.60V (By default) > > > > (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in > > > > device tree) > > > > (3). 2.40V - 2.70V (Supported since UFS 3.x) > > > > > > > > With the introduction of UFS 3.x products, an issue is happening that > > > > UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC > > > > regulator on UFU 3.x products with VCC configuration (3) used. > > > > > > > > To solve this issue, we simply remove pre-defined initial VCC voltage > > > > values in UFS driver with below reasons, > > > > > > > > 1. UFS specifications do not define how to detect the VCC configuration > > > > supported by attached device. > > > > > > > > 2. Device tree already supports standard regulator properties. > > > > > > > > Therefore VCC voltage shall be defined correctly in device tree, and > > > > shall not be changed by UFS driver. What UFS driver needs to do is simply > > > > enabling or disabling the VCC regulator only. > > > > > > > > This is a RFC conceptional patch. Please help review this and feel > > > > free to feedback any ideas. Once this concept is accepted, and then > > > > I would post a more completed patch series to fix this issue. > > > > > > > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> > > > > --- > > > > drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- > > > > 1 file changed, 1 insertion(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c > > > > index a6f76399b3ae..3965be03c136 100644 > > > > --- a/drivers/scsi/ufs/ufshcd-pltfrm.c > > > > +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c > > > > @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, > > > > vreg->max_uA = 0; > > > > } > > > > - if (!strcmp(name, "vcc")) { > > > > - if (of_property_read_bool(np, "vcc-supply-1p8")) { > > > > - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; > > > > - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; > > > > - } else { > > > > - vreg->min_uV = UFS_VREG_VCC_MIN_UV; > > > > - vreg->max_uV = UFS_VREG_VCC_MAX_UV; > > > > - } > > > > - } else if (!strcmp(name, "vccq")) { > > > > + if (!strcmp(name, "vccq")) { > > > > vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; > > > > vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; > > > > } else if (!strcmp(name, "vccq2")) { > > > > > > > > > > Hi Stanley > > > > > > Thanks for the patch. Bao (nguyenb) was also working towards something > > > similar. > > > Would it be possible for you to take into account the scenario in which the > > > same platform supports both 2.x and 3.x UFS devices? > > > > > > These've different voltage requirements, 2.4v-3.6v. > > > I'm not sure if standard dts regulator properties can support this. > > > > > > > What is the actual voltage requirement for these devices and how does > > the software know what voltage to pick in this range? > > > > Regards, > > Bjorn > > > > > -asd > > > > > > > > > -- > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > > > Linux Foundation Collaborative Project > > For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), the > voltage requirements (Vcc) are 2.4v-3.6v. The software initializes the ufs > device at 2.95v & reads the version and if the device is 3.x, it may do the > following: > - Set the device power mode to SLEEP > - Disable the Vcc > - Enable the Vcc and set it to 2.5v > - Set the device power mode to ACTIVE > > All of the above may be done at HS-G1 & moved to max supported gear based on > the device version, perhaps? > > Am open to other ideas though. > But that means that for a board where we don't know (don't want to know) if we have a 2.x or 3.x device we need to set: regulator-min-microvolt = <2.4V> regulator-max-microvolt = <3.6V> And the 2.5V and the two ranges should be hard coded into the ufshcd (in particular if they come from the specification). For devices with only 2.x or 3.x devices, regulator-{min,max}-microvolt should be adjusted accordingly. Note that driving the regulators outside these ranges will either damage the hardware or cause it to misbehave, so these values should be defined in the board.dts anyways. Also note that regulator_set_voltage(2.4V, 3.6V) won't give you "a voltage between 2.4V and 3.6V, it will most likely give either 2.4V or any more specific voltage that we've specified in the board file because the regulator happens to be shared with some other consumer and changing it in runtime would be bad. Regards, Bjorn
On 11/30/2020 5:25 PM, Stanley Chu wrote: > On Mon, 2020-11-30 at 15:54 -0800, Asutosh Das (asd) wrote: >> On 11/30/2020 3:14 PM, Bjorn Andersson wrote: >>> On Mon 30 Nov 16:51 CST 2020, Asutosh Das (asd) wrote: >>> >>>> On 11/30/2020 1:16 AM, Stanley Chu wrote: >>>>> UFS specficication allows different VCC configurations for UFS devices, >>>>> for example, >>>>> (1). 2.70V - 3.60V (By default) >>>>> (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in >>>>> device tree) >>>>> (3). 2.40V - 2.70V (Supported since UFS 3.x) >>>>> >>>>> With the introduction of UFS 3.x products, an issue is happening that >>>>> UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC >>>>> regulator on UFU 3.x products with VCC configuration (3) used. >>>>> >>>>> To solve this issue, we simply remove pre-defined initial VCC voltage >>>>> values in UFS driver with below reasons, >>>>> >>>>> 1. UFS specifications do not define how to detect the VCC configuration >>>>> supported by attached device. >>>>> >>>>> 2. Device tree already supports standard regulator properties. >>>>> >>>>> Therefore VCC voltage shall be defined correctly in device tree, and >>>>> shall not be changed by UFS driver. What UFS driver needs to do is simply >>>>> enabling or disabling the VCC regulator only. >>>>> >>>>> This is a RFC conceptional patch. Please help review this and feel >>>>> free to feedback any ideas. Once this concept is accepted, and then >>>>> I would post a more completed patch series to fix this issue. >>>>> >>>>> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> >>>>> --- >>>>> drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- >>>>> 1 file changed, 1 insertion(+), 9 deletions(-) >>>>> >>>>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c >>>>> index a6f76399b3ae..3965be03c136 100644 >>>>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c >>>>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c >>>>> @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, >>>>> vreg->max_uA = 0; >>>>> } >>>>> - if (!strcmp(name, "vcc")) { >>>>> - if (of_property_read_bool(np, "vcc-supply-1p8")) { >>>>> - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; >>>>> - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; >>>>> - } else { >>>>> - vreg->min_uV = UFS_VREG_VCC_MIN_UV; >>>>> - vreg->max_uV = UFS_VREG_VCC_MAX_UV; >>>>> - } >>>>> - } else if (!strcmp(name, "vccq")) { >>>>> + if (!strcmp(name, "vccq")) { >>>>> vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; >>>>> vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; >>>>> } else if (!strcmp(name, "vccq2")) { >>>>> >>>> >>>> Hi Stanley >>>> >>>> Thanks for the patch. Bao (nguyenb) was also working towards something >>>> similar. >>>> Would it be possible for you to take into account the scenario in which the >>>> same platform supports both 2.x and 3.x UFS devices? >>>> >>>> These've different voltage requirements, 2.4v-3.6v. >>>> I'm not sure if standard dts regulator properties can support this. >>>> >>> >>> What is the actual voltage requirement for these devices and how does >>> the software know what voltage to pick in this range? >>> >>> Regards, >>> Bjorn >>> >>>> -asd >>>> >>>> >>>> -- >>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >>>> Linux Foundation Collaborative Project >> >> For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), the >> voltage requirements (Vcc) are 2.4v-3.6v. The software initializes the >> ufs device at 2.95v & reads the version and if the device is 3.x, it may >> do the following: >> - Set the device power mode to SLEEP >> - Disable the Vcc >> - Enable the Vcc and set it to 2.5v >> - Set the device power mode to ACTIVE >> >> All of the above may be done at HS-G1 & moved to max supported gear >> based on the device version, perhaps? > > Hi Asutosh, > > Thanks for sharing this idea. > > 1. I did not see above flow defined in UFS specifications, please > correct me if I was wrong. > > 2. For above flow, the concern is that I am not sure if all devices > supporting VCC (2.4v - 2.7v) can accept higher voltage, say 2.95v, for > version detection. > > 3. For version detection, another concern is that I am not sure if all > 3.x devices support VCC (2.4v - 2.7v) only, or in other words, I am not > sure if all 2.x devices support VCC (2.7v - 3.6v) only. The above rule > will break any devices not obeying this "conventions". > > For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), > > It would be good for UFS drivers detecting the correct voltage if the > protocol is well-defined in specifications. Until that day, any > "non-standard" way may be better implemented in vendor's ops? > > If the vop concept works on your platform, we could still keep struct > ufs_vreg and allow vendors to configure proper min_uV and max_uV to make > regulator_set_voltage() works during VCC toggling flow. Without specific > vendor configurations, min_uV and max_uV would be NULL by default and > UFS core driver will only enable/disasble VCC regulator only without > adjusting its voltage. > I think this would work. Do you plan to implement this? If not, I can take this up. Please let me know. > Maybe one possible another idea is to decide the correct voltage and > configure regulator properly before kernel? > > Thanks, > Stanley Chu > >> >> Am open to other ideas though. >> >> -asd >> > -asd
On 11/30/2020 6:53 PM, Bjorn Andersson wrote: > On Mon 30 Nov 17:54 CST 2020, Asutosh Das (asd) wrote: > >> On 11/30/2020 3:14 PM, Bjorn Andersson wrote: >>> On Mon 30 Nov 16:51 CST 2020, Asutosh Das (asd) wrote: >>> >>>> On 11/30/2020 1:16 AM, Stanley Chu wrote: >>>>> UFS specficication allows different VCC configurations for UFS devices, >>>>> for example, >>>>> (1). 2.70V - 3.60V (By default) >>>>> (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in >>>>> device tree) >>>>> (3). 2.40V - 2.70V (Supported since UFS 3.x) >>>>> >>>>> With the introduction of UFS 3.x products, an issue is happening that >>>>> UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC >>>>> regulator on UFU 3.x products with VCC configuration (3) used. >>>>> >>>>> To solve this issue, we simply remove pre-defined initial VCC voltage >>>>> values in UFS driver with below reasons, >>>>> >>>>> 1. UFS specifications do not define how to detect the VCC configuration >>>>> supported by attached device. >>>>> >>>>> 2. Device tree already supports standard regulator properties. >>>>> >>>>> Therefore VCC voltage shall be defined correctly in device tree, and >>>>> shall not be changed by UFS driver. What UFS driver needs to do is simply >>>>> enabling or disabling the VCC regulator only. >>>>> >>>>> This is a RFC conceptional patch. Please help review this and feel >>>>> free to feedback any ideas. Once this concept is accepted, and then >>>>> I would post a more completed patch series to fix this issue. >>>>> >>>>> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> >>>>> --- >>>>> drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- >>>>> 1 file changed, 1 insertion(+), 9 deletions(-) >>>>> >>>>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c >>>>> index a6f76399b3ae..3965be03c136 100644 >>>>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c >>>>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c >>>>> @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, >>>>> vreg->max_uA = 0; >>>>> } >>>>> - if (!strcmp(name, "vcc")) { >>>>> - if (of_property_read_bool(np, "vcc-supply-1p8")) { >>>>> - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; >>>>> - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; >>>>> - } else { >>>>> - vreg->min_uV = UFS_VREG_VCC_MIN_UV; >>>>> - vreg->max_uV = UFS_VREG_VCC_MAX_UV; >>>>> - } >>>>> - } else if (!strcmp(name, "vccq")) { >>>>> + if (!strcmp(name, "vccq")) { >>>>> vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; >>>>> vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; >>>>> } else if (!strcmp(name, "vccq2")) { >>>>> >>>> >>>> Hi Stanley >>>> >>>> Thanks for the patch. Bao (nguyenb) was also working towards something >>>> similar. >>>> Would it be possible for you to take into account the scenario in which the >>>> same platform supports both 2.x and 3.x UFS devices? >>>> >>>> These've different voltage requirements, 2.4v-3.6v. >>>> I'm not sure if standard dts regulator properties can support this. >>>> >>> >>> What is the actual voltage requirement for these devices and how does >>> the software know what voltage to pick in this range? >>> >>> Regards, >>> Bjorn >>> >>>> -asd >>>> >>>> >>>> -- >>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >>>> Linux Foundation Collaborative Project >> >> For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), the >> voltage requirements (Vcc) are 2.4v-3.6v. The software initializes the ufs >> device at 2.95v & reads the version and if the device is 3.x, it may do the >> following: >> - Set the device power mode to SLEEP >> - Disable the Vcc >> - Enable the Vcc and set it to 2.5v >> - Set the device power mode to ACTIVE >> >> All of the above may be done at HS-G1 & moved to max supported gear based on >> the device version, perhaps? >> >> Am open to other ideas though. >> > > But that means that for a board where we don't know (don't want to know) > if we have a 2.x or 3.x device we need to set: > > regulator-min-microvolt = <2.4V> > regulator-max-microvolt = <3.6V> > > And the 2.5V and the two ranges should be hard coded into the ufshcd (in > particular if they come from the specification). > > For devices with only 2.x or 3.x devices, regulator-{min,max}-microvolt > should be adjusted accordingly. > > Note that driving the regulators outside these ranges will either damage > the hardware or cause it to misbehave, so these values should be defined > in the board.dts anyways. > > Also note that regulator_set_voltage(2.4V, 3.6V) won't give you "a > voltage between 2.4V and 3.6V, it will most likely give either 2.4V or > any more specific voltage that we've specified in the board file because > the regulator happens to be shared with some other consumer and changing > it in runtime would be bad. > > Regards, > Bjorn > Understood. I also understand that assumptions on the regulator limits in the driver is a bad idea. I'm not sure how it's designed, but I should think the power-grid design should take care of regulator sharing; if it's being shared and the platform supports both 2.x and 3.x. Perhaps, such platforms be identified using a dts flag - not sure if that's such a good idea though. I like Stanley's proposal of a vops and let vendors handle it, until specs or someone has a better suggestion. -asd
On 2020-12-01 11:19, Asutosh Das (asd) wrote: > On 11/30/2020 6:53 PM, Bjorn Andersson wrote: >> On Mon 30 Nov 17:54 CST 2020, Asutosh Das (asd) wrote: >> >>> On 11/30/2020 3:14 PM, Bjorn Andersson wrote: >>>> On Mon 30 Nov 16:51 CST 2020, Asutosh Das (asd) wrote: >>>> >>>>> On 11/30/2020 1:16 AM, Stanley Chu wrote: >>>>>> UFS specficication allows different VCC configurations for UFS >>>>>> devices, >>>>>> for example, >>>>>> (1). 2.70V - 3.60V (By default) >>>>>> (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in >>>>>> device tree) >>>>>> (3). 2.40V - 2.70V (Supported since UFS 3.x) >>>>>> >>>>>> With the introduction of UFS 3.x products, an issue is happening >>>>>> that >>>>>> UFS driver will use wrong "min_uV/max_uV" configuration to toggle >>>>>> VCC >>>>>> regulator on UFU 3.x products with VCC configuration (3) used. >>>>>> >>>>>> To solve this issue, we simply remove pre-defined initial VCC >>>>>> voltage >>>>>> values in UFS driver with below reasons, >>>>>> >>>>>> 1. UFS specifications do not define how to detect the VCC >>>>>> configuration >>>>>> supported by attached device. >>>>>> >>>>>> 2. Device tree already supports standard regulator properties. >>>>>> >>>>>> Therefore VCC voltage shall be defined correctly in device tree, >>>>>> and >>>>>> shall not be changed by UFS driver. What UFS driver needs to do is >>>>>> simply >>>>>> enabling or disabling the VCC regulator only. >>>>>> >>>>>> This is a RFC conceptional patch. Please help review this and feel >>>>>> free to feedback any ideas. Once this concept is accepted, and >>>>>> then >>>>>> I would post a more completed patch series to fix this issue. >>>>>> >>>>>> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> >>>>>> --- >>>>>> drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- >>>>>> 1 file changed, 1 insertion(+), 9 deletions(-) >>>>>> >>>>>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c >>>>>> b/drivers/scsi/ufs/ufshcd-pltfrm.c >>>>>> index a6f76399b3ae..3965be03c136 100644 >>>>>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c >>>>>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c >>>>>> @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device >>>>>> *dev, const char *name, >>>>>> vreg->max_uA = 0; >>>>>> } >>>>>> - if (!strcmp(name, "vcc")) { >>>>>> - if (of_property_read_bool(np, "vcc-supply-1p8")) { >>>>>> - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; >>>>>> - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; >>>>>> - } else { >>>>>> - vreg->min_uV = UFS_VREG_VCC_MIN_UV; >>>>>> - vreg->max_uV = UFS_VREG_VCC_MAX_UV; >>>>>> - } >>>>>> - } else if (!strcmp(name, "vccq")) { >>>>>> + if (!strcmp(name, "vccq")) { >>>>>> vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; >>>>>> vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; >>>>>> } else if (!strcmp(name, "vccq2")) { >>>>>> >>>>> >>>>> Hi Stanley >>>>> >>>>> Thanks for the patch. Bao (nguyenb) was also working towards >>>>> something >>>>> similar. >>>>> Would it be possible for you to take into account the scenario in >>>>> which the >>>>> same platform supports both 2.x and 3.x UFS devices? >>>>> >>>>> These've different voltage requirements, 2.4v-3.6v. >>>>> I'm not sure if standard dts regulator properties can support this. >>>>> >>>> >>>> What is the actual voltage requirement for these devices and how >>>> does >>>> the software know what voltage to pick in this range? >>>> >>>> Regards, >>>> Bjorn >>>> >>>>> -asd >>>>> >>>>> >>>>> -- The Qualcomm Innovation Center, Inc. is a member of the Code >>>>> Aurora Forum, >>>>> Linux Foundation Collaborative Project >>> >>> For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), >>> the >>> voltage requirements (Vcc) are 2.4v-3.6v. The software initializes >>> the ufs >>> device at 2.95v & reads the version and if the device is 3.x, it may >>> do the >>> following: >>> - Set the device power mode to SLEEP >>> - Disable the Vcc >>> - Enable the Vcc and set it to 2.5v >>> - Set the device power mode to ACTIVE >>> >>> All of the above may be done at HS-G1 & moved to max supported gear >>> based on >>> the device version, perhaps? >>> >>> Am open to other ideas though. >>> >> >> But that means that for a board where we don't know (don't want to >> know) >> if we have a 2.x or 3.x device we need to set: >> >> regulator-min-microvolt = <2.4V> >> regulator-max-microvolt = <3.6V> >> >> And the 2.5V and the two ranges should be hard coded into the ufshcd >> (in >> particular if they come from the specification). >> >> For devices with only 2.x or 3.x devices, >> regulator-{min,max}-microvolt >> should be adjusted accordingly. >> >> Note that driving the regulators outside these ranges will either >> damage >> the hardware or cause it to misbehave, so these values should be >> defined >> in the board.dts anyways. >> >> Also note that regulator_set_voltage(2.4V, 3.6V) won't give you "a >> voltage between 2.4V and 3.6V, it will most likely give either 2.4V or >> any more specific voltage that we've specified in the board file >> because >> the regulator happens to be shared with some other consumer and >> changing >> it in runtime would be bad. >> >> Regards, >> Bjorn >> > > Understood. > I also understand that assumptions on the regulator limits in the > driver is a bad idea. I'm not sure how it's designed, but I should > think the power-grid design should take care of regulator sharing; if > it's being shared and the platform supports both 2.x and 3.x. Perhaps, > such platforms be identified using a dts flag - not sure if that's > such a good idea though. > > I like Stanley's proposal of a vops and let vendors handle it, until > specs or someone has a better suggestion. Agree, vops is all we need as of now, please upload a change to add one properly. Thanks, Can Guo. > > -asd
On Mon 30 Nov 21:19 CST 2020, Asutosh Das (asd) wrote: > On 11/30/2020 6:53 PM, Bjorn Andersson wrote: > > On Mon 30 Nov 17:54 CST 2020, Asutosh Das (asd) wrote: > > > > > On 11/30/2020 3:14 PM, Bjorn Andersson wrote: > > > > On Mon 30 Nov 16:51 CST 2020, Asutosh Das (asd) wrote: > > > > > > > > > On 11/30/2020 1:16 AM, Stanley Chu wrote: > > > > > > UFS specficication allows different VCC configurations for UFS devices, > > > > > > for example, > > > > > > (1). 2.70V - 3.60V (By default) > > > > > > (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in > > > > > > device tree) > > > > > > (3). 2.40V - 2.70V (Supported since UFS 3.x) > > > > > > > > > > > > With the introduction of UFS 3.x products, an issue is happening that > > > > > > UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC > > > > > > regulator on UFU 3.x products with VCC configuration (3) used. > > > > > > > > > > > > To solve this issue, we simply remove pre-defined initial VCC voltage > > > > > > values in UFS driver with below reasons, > > > > > > > > > > > > 1. UFS specifications do not define how to detect the VCC configuration > > > > > > supported by attached device. > > > > > > > > > > > > 2. Device tree already supports standard regulator properties. > > > > > > > > > > > > Therefore VCC voltage shall be defined correctly in device tree, and > > > > > > shall not be changed by UFS driver. What UFS driver needs to do is simply > > > > > > enabling or disabling the VCC regulator only. > > > > > > > > > > > > This is a RFC conceptional patch. Please help review this and feel > > > > > > free to feedback any ideas. Once this concept is accepted, and then > > > > > > I would post a more completed patch series to fix this issue. > > > > > > > > > > > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> > > > > > > --- > > > > > > drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- > > > > > > 1 file changed, 1 insertion(+), 9 deletions(-) > > > > > > > > > > > > diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c > > > > > > index a6f76399b3ae..3965be03c136 100644 > > > > > > --- a/drivers/scsi/ufs/ufshcd-pltfrm.c > > > > > > +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c > > > > > > @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, > > > > > > vreg->max_uA = 0; > > > > > > } > > > > > > - if (!strcmp(name, "vcc")) { > > > > > > - if (of_property_read_bool(np, "vcc-supply-1p8")) { > > > > > > - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; > > > > > > - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; > > > > > > - } else { > > > > > > - vreg->min_uV = UFS_VREG_VCC_MIN_UV; > > > > > > - vreg->max_uV = UFS_VREG_VCC_MAX_UV; > > > > > > - } > > > > > > - } else if (!strcmp(name, "vccq")) { > > > > > > + if (!strcmp(name, "vccq")) { > > > > > > vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; > > > > > > vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; > > > > > > } else if (!strcmp(name, "vccq2")) { > > > > > > > > > > > > > > > > Hi Stanley > > > > > > > > > > Thanks for the patch. Bao (nguyenb) was also working towards something > > > > > similar. > > > > > Would it be possible for you to take into account the scenario in which the > > > > > same platform supports both 2.x and 3.x UFS devices? > > > > > > > > > > These've different voltage requirements, 2.4v-3.6v. > > > > > I'm not sure if standard dts regulator properties can support this. > > > > > > > > > > > > > What is the actual voltage requirement for these devices and how does > > > > the software know what voltage to pick in this range? > > > > > > > > Regards, > > > > Bjorn > > > > > > > > > -asd > > > > > > > > > > > > > > > -- > > > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > > > > > Linux Foundation Collaborative Project > > > > > > For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), the > > > voltage requirements (Vcc) are 2.4v-3.6v. The software initializes the ufs > > > device at 2.95v & reads the version and if the device is 3.x, it may do the > > > following: > > > - Set the device power mode to SLEEP > > > - Disable the Vcc > > > - Enable the Vcc and set it to 2.5v > > > - Set the device power mode to ACTIVE > > > > > > All of the above may be done at HS-G1 & moved to max supported gear based on > > > the device version, perhaps? > > > > > > Am open to other ideas though. > > > > > > > But that means that for a board where we don't know (don't want to know) > > if we have a 2.x or 3.x device we need to set: > > > > regulator-min-microvolt = <2.4V> > > regulator-max-microvolt = <3.6V> > > > > And the 2.5V and the two ranges should be hard coded into the ufshcd (in > > particular if they come from the specification). > > > > For devices with only 2.x or 3.x devices, regulator-{min,max}-microvolt > > should be adjusted accordingly. > > > > Note that driving the regulators outside these ranges will either damage > > the hardware or cause it to misbehave, so these values should be defined > > in the board.dts anyways. > > > > Also note that regulator_set_voltage(2.4V, 3.6V) won't give you "a > > voltage between 2.4V and 3.6V, it will most likely give either 2.4V or > > any more specific voltage that we've specified in the board file because > > the regulator happens to be shared with some other consumer and changing > > it in runtime would be bad. > > > > Regards, > > Bjorn > > > > Understood. > I also understand that assumptions on the regulator limits in the driver is > a bad idea. I'm not sure how it's designed, but I should think the > power-grid design should take care of regulator sharing; if it's being > shared and the platform supports both 2.x and 3.x. Perhaps, such platforms > be identified using a dts flag - not sure if that's such a good idea though. > Presumably you can't share vcc with other peripherals, given that the voltage levels might just change while the device is in use then. As you say the only way to avoid this is to think these problems through when designing the power grid. > I like Stanley's proposal of a vops and let vendors handle it, until specs > or someone has a better suggestion. > I too think it sounds quite reasonable. Regards, Bjorn
Hi Asutosh, On Mon, 2020-11-30 at 19:07 -0800, Asutosh Das (asd) wrote: > On 11/30/2020 5:25 PM, Stanley Chu wrote: > > On Mon, 2020-11-30 at 15:54 -0800, Asutosh Das (asd) wrote: > >> On 11/30/2020 3:14 PM, Bjorn Andersson wrote: > >>> On Mon 30 Nov 16:51 CST 2020, Asutosh Das (asd) wrote: > >>> > >>>> On 11/30/2020 1:16 AM, Stanley Chu wrote: > >>>>> UFS specficication allows different VCC configurations for UFS devices, > >>>>> for example, > >>>>> (1). 2.70V - 3.60V (By default) > >>>>> (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in > >>>>> device tree) > >>>>> (3). 2.40V - 2.70V (Supported since UFS 3.x) > >>>>> > >>>>> With the introduction of UFS 3.x products, an issue is happening that > >>>>> UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC > >>>>> regulator on UFU 3.x products with VCC configuration (3) used. > >>>>> > >>>>> To solve this issue, we simply remove pre-defined initial VCC voltage > >>>>> values in UFS driver with below reasons, > >>>>> > >>>>> 1. UFS specifications do not define how to detect the VCC configuration > >>>>> supported by attached device. > >>>>> > >>>>> 2. Device tree already supports standard regulator properties. > >>>>> > >>>>> Therefore VCC voltage shall be defined correctly in device tree, and > >>>>> shall not be changed by UFS driver. What UFS driver needs to do is simply > >>>>> enabling or disabling the VCC regulator only. > >>>>> > >>>>> This is a RFC conceptional patch. Please help review this and feel > >>>>> free to feedback any ideas. Once this concept is accepted, and then > >>>>> I would post a more completed patch series to fix this issue. > >>>>> > >>>>> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> > >>>>> --- > >>>>> drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- > >>>>> 1 file changed, 1 insertion(+), 9 deletions(-) > >>>>> > >>>>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c > >>>>> index a6f76399b3ae..3965be03c136 100644 > >>>>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c > >>>>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c > >>>>> @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, > >>>>> vreg->max_uA = 0; > >>>>> } > >>>>> - if (!strcmp(name, "vcc")) { > >>>>> - if (of_property_read_bool(np, "vcc-supply-1p8")) { > >>>>> - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; > >>>>> - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; > >>>>> - } else { > >>>>> - vreg->min_uV = UFS_VREG_VCC_MIN_UV; > >>>>> - vreg->max_uV = UFS_VREG_VCC_MAX_UV; > >>>>> - } > >>>>> - } else if (!strcmp(name, "vccq")) { > >>>>> + if (!strcmp(name, "vccq")) { > >>>>> vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; > >>>>> vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; > >>>>> } else if (!strcmp(name, "vccq2")) { > >>>>> > >>>> > >>>> Hi Stanley > >>>> > >>>> Thanks for the patch. Bao (nguyenb) was also working towards something > >>>> similar. > >>>> Would it be possible for you to take into account the scenario in which the > >>>> same platform supports both 2.x and 3.x UFS devices? > >>>> > >>>> These've different voltage requirements, 2.4v-3.6v. > >>>> I'm not sure if standard dts regulator properties can support this. > >>>> > >>> > >>> What is the actual voltage requirement for these devices and how does > >>> the software know what voltage to pick in this range? > >>> > >>> Regards, > >>> Bjorn > >>> > >>>> -asd > >>>> > >>>> > >>>> -- > >>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > >>>> Linux Foundation Collaborative Project > >> > >> For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), the > >> voltage requirements (Vcc) are 2.4v-3.6v. The software initializes the > >> ufs device at 2.95v & reads the version and if the device is 3.x, it may > >> do the following: > >> - Set the device power mode to SLEEP > >> - Disable the Vcc > >> - Enable the Vcc and set it to 2.5v > >> - Set the device power mode to ACTIVE > >> > >> All of the above may be done at HS-G1 & moved to max supported gear > >> based on the device version, perhaps? > > > > Hi Asutosh, > > > > Thanks for sharing this idea. > > > > 1. I did not see above flow defined in UFS specifications, please > > correct me if I was wrong. > > > > 2. For above flow, the concern is that I am not sure if all devices > > supporting VCC (2.4v - 2.7v) can accept higher voltage, say 2.95v, for > > version detection. > > > > 3. For version detection, another concern is that I am not sure if all > > 3.x devices support VCC (2.4v - 2.7v) only, or in other words, I am not > > sure if all 2.x devices support VCC (2.7v - 3.6v) only. The above rule > > will break any devices not obeying this "conventions". > > > > For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), > > > > It would be good for UFS drivers detecting the correct voltage if the > > protocol is well-defined in specifications. Until that day, any > > "non-standard" way may be better implemented in vendor's ops? > > > > If the vop concept works on your platform, we could still keep struct > > ufs_vreg and allow vendors to configure proper min_uV and max_uV to make > > regulator_set_voltage() works during VCC toggling flow. Without specific > > vendor configurations, min_uV and max_uV would be NULL by default and > > UFS core driver will only enable/disasble VCC regulator only without > > adjusting its voltage. > > > > I think this would work. Do you plan to implement this? > If not, I can take this up. Please let me know. Thanks for the understanding and support. I would like to re-post this patch to simply removing the pre-defined initial values of all device powers. For vop idea supporting the voltage detection way, could you please take it up since this would be better to fit what you need for fixing this issue? Thanks, Stanley Chu > > > Maybe one possible another idea is to decide the correct voltage and > > configure regulator properly before kernel? > > > > Thanks, > > Stanley Chu > > > >> > >> Am open to other ideas though. > >> > >> -asd > >> > > > > -asd > >
> > >>>> Hi Stanley > > >>>> > > >>>> Thanks for the patch. Bao (nguyenb) was also working towards > something > > >>>> similar. > > >>>> Would it be possible for you to take into account the scenario in > which the > > >>>> same platform supports both 2.x and 3.x UFS devices? > > >>>> > > >>>> These've different voltage requirements, 2.4v-3.6v. > > >>>> I'm not sure if standard dts regulator properties can support this. > > >>>> > > >>> > > >>> What is the actual voltage requirement for these devices and how > does > > >>> the software know what voltage to pick in this range? > > >>> > > >>> Regards, > > >>> Bjorn > > >>> > > >>>> -asd > > >>>> > > >>>> > > >>>> -- > > >>>> The Qualcomm Innovation Center, Inc. is a member of the Code > Aurora Forum, > > >>>> Linux Foundation Collaborative Project > > >> > > >> For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), the > > >> voltage requirements (Vcc) are 2.4v-3.6v. The software initializes the > > >> ufs device at 2.95v & reads the version and if the device is 3.x, it may > > >> do the following: > > >> - Set the device power mode to SLEEP > > >> - Disable the Vcc > > >> - Enable the Vcc and set it to 2.5v > > >> - Set the device power mode to ACTIVE > > >> > > >> All of the above may be done at HS-G1 & moved to max supported gear > > >> based on the device version, perhaps? > > > > > > Hi Asutosh, > > > > > > Thanks for sharing this idea. > > > > > > 1. I did not see above flow defined in UFS specifications, please > > > correct me if I was wrong. > > > > > > 2. For above flow, the concern is that I am not sure if all devices > > > supporting VCC (2.4v - 2.7v) can accept higher voltage, say 2.95v, for > > > version detection. > > > > > > 3. For version detection, another concern is that I am not sure if all > > > 3.x devices support VCC (2.4v - 2.7v) only, or in other words, I am not > > > sure if all 2.x devices support VCC (2.7v - 3.6v) only. The above rule > > > will break any devices not obeying this "conventions". > > > > > > For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), > > > > > > It would be good for UFS drivers detecting the correct voltage if the > > > protocol is well-defined in specifications. Until that day, any > > > "non-standard" way may be better implemented in vendor's ops? > > > > > > If the vop concept works on your platform, we could still keep struct > > > ufs_vreg and allow vendors to configure proper min_uV and max_uV to > make > > > regulator_set_voltage() works during VCC toggling flow. Without specific > > > vendor configurations, min_uV and max_uV would be NULL by default > and > > > UFS core driver will only enable/disasble VCC regulator only without > > > adjusting its voltage. > > > > > > > I think this would work. Do you plan to implement this? > > If not, I can take this up. Please let me know. > > Thanks for the understanding and support. > > I would like to re-post this patch to simply removing the pre-defined > initial values of all device powers. > > For vop idea supporting the voltage detection way, could you please take > it up since this would be better to fit what you need for fixing this > issue? Again - why vop and not a dts flag? The platform owner is aware of which device ships on which platform, isn't it? Thanks, Avri
On 2020-11-30 22:54, Stanley Chu wrote: > Hi Asutosh, > > On Mon, 2020-11-30 at 19:07 -0800, Asutosh Das (asd) wrote: >> On 11/30/2020 5:25 PM, Stanley Chu wrote: >> > On Mon, 2020-11-30 at 15:54 -0800, Asutosh Das (asd) wrote: >> >> On 11/30/2020 3:14 PM, Bjorn Andersson wrote: >> >>> On Mon 30 Nov 16:51 CST 2020, Asutosh Das (asd) wrote: >> >>> >> >>>> On 11/30/2020 1:16 AM, Stanley Chu wrote: >> >>>>> UFS specficication allows different VCC configurations for UFS devices, >> >>>>> for example, >> >>>>> (1). 2.70V - 3.60V (By default) >> >>>>> (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in >> >>>>> device tree) >> >>>>> (3). 2.40V - 2.70V (Supported since UFS 3.x) >> >>>>> >> >>>>> With the introduction of UFS 3.x products, an issue is happening that >> >>>>> UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC >> >>>>> regulator on UFU 3.x products with VCC configuration (3) used. >> >>>>> >> >>>>> To solve this issue, we simply remove pre-defined initial VCC voltage >> >>>>> values in UFS driver with below reasons, >> >>>>> >> >>>>> 1. UFS specifications do not define how to detect the VCC configuration >> >>>>> supported by attached device. >> >>>>> >> >>>>> 2. Device tree already supports standard regulator properties. >> >>>>> >> >>>>> Therefore VCC voltage shall be defined correctly in device tree, and >> >>>>> shall not be changed by UFS driver. What UFS driver needs to do is simply >> >>>>> enabling or disabling the VCC regulator only. >> >>>>> >> >>>>> This is a RFC conceptional patch. Please help review this and feel >> >>>>> free to feedback any ideas. Once this concept is accepted, and then >> >>>>> I would post a more completed patch series to fix this issue. >> >>>>> >> >>>>> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> >> >>>>> --- >> >>>>> drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- >> >>>>> 1 file changed, 1 insertion(+), 9 deletions(-) >> >>>>> >> >>>>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c >> >>>>> index a6f76399b3ae..3965be03c136 100644 >> >>>>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c >> >>>>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c >> >>>>> @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, >> >>>>> vreg->max_uA = 0; >> >>>>> } >> >>>>> - if (!strcmp(name, "vcc")) { >> >>>>> - if (of_property_read_bool(np, "vcc-supply-1p8")) { >> >>>>> - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; >> >>>>> - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; >> >>>>> - } else { >> >>>>> - vreg->min_uV = UFS_VREG_VCC_MIN_UV; >> >>>>> - vreg->max_uV = UFS_VREG_VCC_MAX_UV; >> >>>>> - } >> >>>>> - } else if (!strcmp(name, "vccq")) { >> >>>>> + if (!strcmp(name, "vccq")) { >> >>>>> vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; >> >>>>> vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; >> >>>>> } else if (!strcmp(name, "vccq2")) { >> >>>>> >> >>>> >> >>>> Hi Stanley >> >>>> >> >>>> Thanks for the patch. Bao (nguyenb) was also working towards something >> >>>> similar. >> >>>> Would it be possible for you to take into account the scenario in which the >> >>>> same platform supports both 2.x and 3.x UFS devices? >> >>>> >> >>>> These've different voltage requirements, 2.4v-3.6v. >> >>>> I'm not sure if standard dts regulator properties can support this. >> >>>> >> >>> >> >>> What is the actual voltage requirement for these devices and how does >> >>> the software know what voltage to pick in this range? >> >>> >> >>> Regards, >> >>> Bjorn >> >>> >> >>>> -asd >> >>>> >> >>>> >> >>>> -- >> >>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> >>>> Linux Foundation Collaborative Project >> >> >> >> For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), the >> >> voltage requirements (Vcc) are 2.4v-3.6v. The software initializes the >> >> ufs device at 2.95v & reads the version and if the device is 3.x, it may >> >> do the following: >> >> - Set the device power mode to SLEEP >> >> - Disable the Vcc >> >> - Enable the Vcc and set it to 2.5v >> >> - Set the device power mode to ACTIVE >> >> >> >> All of the above may be done at HS-G1 & moved to max supported gear >> >> based on the device version, perhaps? >> > >> > Hi Asutosh, >> > >> > Thanks for sharing this idea. >> > >> > 1. I did not see above flow defined in UFS specifications, please >> > correct me if I was wrong. >> > >> > 2. For above flow, the concern is that I am not sure if all devices >> > supporting VCC (2.4v - 2.7v) can accept higher voltage, say 2.95v, for >> > version detection. >> > >> > 3. For version detection, another concern is that I am not sure if all >> > 3.x devices support VCC (2.4v - 2.7v) only, or in other words, I am not >> > sure if all 2.x devices support VCC (2.7v - 3.6v) only. The above rule >> > will break any devices not obeying this "conventions". >> > >> > For platforms that support both 2.x (2.7v-3.6v) and 3.x (2.4v-2.7v), >> > >> > It would be good for UFS drivers detecting the correct voltage if the >> > protocol is well-defined in specifications. Until that day, any >> > "non-standard" way may be better implemented in vendor's ops? >> > >> > If the vop concept works on your platform, we could still keep struct >> > ufs_vreg and allow vendors to configure proper min_uV and max_uV to make >> > regulator_set_voltage() works during VCC toggling flow. Without specific >> > vendor configurations, min_uV and max_uV would be NULL by default and >> > UFS core driver will only enable/disasble VCC regulator only without >> > adjusting its voltage. >> > >> >> I think this would work. Do you plan to implement this? >> If not, I can take this up. Please let me know. > > Thanks for the understanding and support. > > I would like to re-post this patch to simply removing the pre-defined > initial values of all device powers. > > For vop idea supporting the voltage detection way, could you please > take > it up since this would be better to fit what you need for fixing this > issue? > > Thanks, > Stanley Chu While we are on this topic, another similar concern is how to set the UFS's regulators loading to Low Power Mode (LPM). Currently, the UFS_VREG_LPM_LOAD_UA is hardcoded to 1mA in the driver, and it is invoked by the ufshcd_config_vreg_lpm(). However, for some platforms, to put the regulators into LPM mode, it may be a different value than 1mA. Should we be using ufs_vreg's min_uA in the ufshcd_config_vreg_lpm() instead of using the hardcoded value? And the ufs_vreg's min_uA value would be parsed by the vendor's code? We can post a proposal if this sounds ok. Regards, Bao > > >> >> > Maybe one possible another idea is to decide the correct voltage and >> > configure regulator properly before kernel? >> > >> > Thanks, >> > Stanley Chu >> > >> >> >> >> Am open to other ideas though. >> >> >> >> -asd >> >> >> > >> >> -asd >> >>
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c index a6f76399b3ae..3965be03c136 100644 --- a/drivers/scsi/ufs/ufshcd-pltfrm.c +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c @@ -133,15 +133,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, vreg->max_uA = 0; } - if (!strcmp(name, "vcc")) { - if (of_property_read_bool(np, "vcc-supply-1p8")) { - vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV; - vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV; - } else { - vreg->min_uV = UFS_VREG_VCC_MIN_UV; - vreg->max_uV = UFS_VREG_VCC_MAX_UV; - } - } else if (!strcmp(name, "vccq")) { + if (!strcmp(name, "vccq")) { vreg->min_uV = UFS_VREG_VCCQ_MIN_UV; vreg->max_uV = UFS_VREG_VCCQ_MAX_UV; } else if (!strcmp(name, "vccq2")) {
UFS specficication allows different VCC configurations for UFS devices, for example, (1). 2.70V - 3.60V (By default) (2). 1.70V - 1.95V (Activated if "vcc-supply-1p8" is declared in device tree) (3). 2.40V - 2.70V (Supported since UFS 3.x) With the introduction of UFS 3.x products, an issue is happening that UFS driver will use wrong "min_uV/max_uV" configuration to toggle VCC regulator on UFU 3.x products with VCC configuration (3) used. To solve this issue, we simply remove pre-defined initial VCC voltage values in UFS driver with below reasons, 1. UFS specifications do not define how to detect the VCC configuration supported by attached device. 2. Device tree already supports standard regulator properties. Therefore VCC voltage shall be defined correctly in device tree, and shall not be changed by UFS driver. What UFS driver needs to do is simply enabling or disabling the VCC regulator only. This is a RFC conceptional patch. Please help review this and feel free to feedback any ideas. Once this concept is accepted, and then I would post a more completed patch series to fix this issue. Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> --- drivers/scsi/ufs/ufshcd-pltfrm.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)