| Message ID | 20240910-topic-ufs-enhancements-v1-12-3ee0bffacc64@linaro.org |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | ufs: enhancements to support Qualcomm UFS controllers | expand |
Hi Neil On 10/09/24 14:50, Neil Armstrong wrote: > The link_startup_again logic was added in Linux to handle device > that were set in LinkDown state, which should not be the case since U-boot > doesn't set LinkDown state are init, and Linux sets the device active > in ufshcd_init() for the first link startup. > > While it worked to far, it breaks link startup for Qualcomm Controllers v5, > let's just remove the logic. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> I had sent a review comment here https://patchwork.ozlabs.org/project/uboot/patch/20240528-topic-sm8x50-ufs-core-link-startup-again-v1-1-146ca43e80b6@linaro.org/ Would like Bhupesh to also have a look at this. > --- > drivers/ufs/ufs.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/ufs/ufs.c b/drivers/ufs/ufs.c > index d2df5c26f76..e34e4586224 100644 > --- a/drivers/ufs/ufs.c > +++ b/drivers/ufs/ufs.c > @@ -462,9 +462,7 @@ static int ufshcd_link_startup(struct ufs_hba *hba) > { > int ret; > int retries = DME_LINKSTARTUP_RETRIES; > - bool link_startup_again = true; > > -link_startup: > do { > ufshcd_ops_link_startup_notify(hba, PRE_CHANGE); > > @@ -490,12 +488,6 @@ link_startup: > /* failed to get the link up... retire */ > goto out; > > - if (link_startup_again) { > - link_startup_again = false; > - retries = DME_LINKSTARTUP_RETRIES; > - goto link_startup; > - } > - > /* Mark that link is up in PWM-G1, 1-lane, SLOW-AUTO mode */ > ufshcd_init_pwr_info(hba); > >
Hi, On 10/09/2024 13:36, Neha Malcom Francis wrote: > Hi Neil > > On 10/09/24 14:50, Neil Armstrong wrote: >> The link_startup_again logic was added in Linux to handle device >> that were set in LinkDown state, which should not be the case since U-boot >> doesn't set LinkDown state are init, and Linux sets the device active >> in ufshcd_init() for the first link startup. >> >> While it worked to far, it breaks link startup for Qualcomm Controllers v5, >> let's just remove the logic. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > > I had sent a review comment here https://patchwork.ozlabs.org/project/uboot/patch/20240528-topic-sm8x50-ufs-core-link-startup-again-v1-1-146ca43e80b6@linaro.org/ Indeed I missed it, I'll fix the commit msg. Nevertheless, as I explain in the cover letter the double link startup is not done in Linux for the initial power-up: https://elixir.bootlin.com/linux/v6.10.9/source/drivers/ufs/core/ufshcd.c#L10548 ufshcd_set_ufs_dev_active(hba) is called at ufshcd_init() right before scheduling an ufshcd_async_scan that will call ufshcd_device_init() then ufshcd_link_startup(). The comment in probe says: /* * We are assuming that device wasn't put in sleep/power-down * state exclusively during the boot stage before kernel. * This assumption helps avoid doing link startup twice during * ufshcd_probe_hba(). */ we can assume the same from U-boot. Neil > > Would like Bhupesh to also have a look at this. > >> --- >> drivers/ufs/ufs.c | 8 -------- >> 1 file changed, 8 deletions(-) >> >> diff --git a/drivers/ufs/ufs.c b/drivers/ufs/ufs.c >> index d2df5c26f76..e34e4586224 100644 >> --- a/drivers/ufs/ufs.c >> +++ b/drivers/ufs/ufs.c >> @@ -462,9 +462,7 @@ static int ufshcd_link_startup(struct ufs_hba *hba) >> { >> int ret; >> int retries = DME_LINKSTARTUP_RETRIES; >> - bool link_startup_again = true; >> -link_startup: >> do { >> ufshcd_ops_link_startup_notify(hba, PRE_CHANGE); >> @@ -490,12 +488,6 @@ link_startup: >> /* failed to get the link up... retire */ >> goto out; >> - if (link_startup_again) { >> - link_startup_again = false; >> - retries = DME_LINKSTARTUP_RETRIES; >> - goto link_startup; >> - } >> - >> /* Mark that link is up in PWM-G1, 1-lane, SLOW-AUTO mode */ >> ufshcd_init_pwr_info(hba); >> >
diff --git a/drivers/ufs/ufs.c b/drivers/ufs/ufs.c index d2df5c26f76..e34e4586224 100644 --- a/drivers/ufs/ufs.c +++ b/drivers/ufs/ufs.c @@ -462,9 +462,7 @@ static int ufshcd_link_startup(struct ufs_hba *hba) { int ret; int retries = DME_LINKSTARTUP_RETRIES; - bool link_startup_again = true; -link_startup: do { ufshcd_ops_link_startup_notify(hba, PRE_CHANGE); @@ -490,12 +488,6 @@ link_startup: /* failed to get the link up... retire */ goto out; - if (link_startup_again) { - link_startup_again = false; - retries = DME_LINKSTARTUP_RETRIES; - goto link_startup; - } - /* Mark that link is up in PWM-G1, 1-lane, SLOW-AUTO mode */ ufshcd_init_pwr_info(hba);
The link_startup_again logic was added in Linux to handle device that were set in LinkDown state, which should not be the case since U-boot doesn't set LinkDown state are init, and Linux sets the device active in ufshcd_init() for the first link startup. While it worked to far, it breaks link startup for Qualcomm Controllers v5, let's just remove the logic. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/ufs/ufs.c | 8 -------- 1 file changed, 8 deletions(-)