From: Manivannan Sadhasivam <mani@kernel.org> To: Can Guo <quic_cang@quicinc.com> Cc: Manivannan Sadhasivam <mani@kernel.org>, bvanassche@acm.org, adrian.hunter@intel.com, cmd4@qualcomm.com, beanhuo@micron.com, avri.altman@wdc.com, junwoo80.lee@samsung.com, martin.petersen@oracle.com, linux-scsi@vger.kernel.org, linux-arm-msm@vger.kernel.org, Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Vinod Koul <vkoul@kernel.org>, Kishon Vijay Abraham I <kishon@kernel.org>, "open list:GENERIC PHY FRAMEWORK" <linux-phy@lists.infradead.org>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v6 08/10] phy: qualcomm: phy-qcom-qmp-ufs: Add High Speed Gear 5 support for SM8550 Date: Thu, 30 Nov 2023 15:04:23 +0530 [thread overview] Message-ID: <20231130093423.GO3043@thinkpad> (raw) In-Reply-To: <c112b051-05ba-45f0-a9f3-f0da5afad4f7@quicinc.com> On Thu, Nov 30, 2023 at 04:49:12PM +0800, Can Guo wrote: > > > On 11/30/2023 4:38 PM, Manivannan Sadhasivam wrote: > > On Thu, Nov 30, 2023 at 04:14:25PM +0800, Can Guo wrote: [...] > > > > > +static int qmp_ufs_get_gear_overlay(struct qmp_ufs *qmp, const struct qmp_phy_cfg *cfg) > > > > > +{ > > > > > + u32 max_gear, floor_max_gear = cfg->max_supported_gear; > > > > > + int i = NUM_OVERLAY - 1; > > > > > > > > Just use i directly in the for loop. Also, please rename "i" with "idx" to make > > > > it clear. > > > > > > OK > > > > > > > > > > > > + int ret = -EINVAL; > > > > > + > > > > > + for (; i >= 0; i --) { > > > > > > > > i-- > > > > > > > > > + max_gear = cfg->tbls_hs_overlay[i].max_gear; > > > > > + > > > > > + if (max_gear == 0) > > > > > + continue; > > > > > > > > You are setting max_gear even for targets with a single overlay. How can this > > > > become 0? > > > > > > Say 8550 has two overlays, 8450 has one overlay. We are sweeping all > > > overlays as NUM_OVERLAY == 2, so for 8450, there is one overlay initialized, > > > another one not initialized (max_gear == 0), we are skipping the one which > > > is not initialized. > > > > > > > This is confusing at its best :) Please check for the existence of the actual > > table instead. Like, > > > > for (idx = NUM_OVERLAY - 1; i >= 0, i--) { > > > > /* Skip if the table is not available */ > > if (!cfg->tbls_hs_overlay[i].serdes) > > continue; > > > > ... > > } > > We cannot expect overlay must has its own serdes, or tx/rx/pcs, hence I am > checking max_gear intead of any specific member. > Hmm, then please add the comment as I suggested above. > > > > > > > > > > > + > > > > > + /* Direct matching, bail */ > > > > > + if (qmp->submode == max_gear) > > > > > + return i; > > > > > + > > > > > + /* If no direct matching, the lowest gear is the best matching */ > > > > > + if (max_gear < floor_max_gear) { > > > > > + ret = i; > > > > > + floor_max_gear = max_gear; > > > > > + } > > > > > + } > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg *cfg) > > > > > { > > > > > + int i; > > > > > + bool apply_overlay = false; > > > > > + > > > > > + i = qmp_ufs_get_gear_overlay(qmp, cfg); > > > > > + if (i >= 0) > > > > > + apply_overlay = true; > > > > > > > > How about? > > > > > > > > ``` > > > > int idx; > > > > > > > > idx = qmp_ufs_get_gear_overlay(qmp, cfg); > > > > > > > > qmp_ufs_serdes_init(qmp, &cfg->tbls); > > > > qmp_ufs_lanes_init(qmp, &cfg->tbls); > > > > ... > > > > > > > > if (idx >= 0) { > > > > qmp_ufs_serdes_init(qmp, &cfg->tbls_hs_overlay[idx]); > > > > qmp_ufs_lanes_init(qmp, &cfg->tbls_hs_overlay[idx]); > > > > ... > > > > } > > > > ``` > > > > > > > > Since the ordering doesn't matter for init sequence, you can program the overlay > > > > tables under a single condition. > > > > > > We can do that, but we need to be careful. When I say (in my previous reply) > > > the ordering does not matter, that saying is from the UFS PHY HPG doc. > > > However, in SW implementation, the 'tbls_hs_b' is actually overwriting one > > > COM_VCO_TUNE_MAP register, the same register is also programmed by common > > > table or overlay table. So qmp_ufs_serdes_init(qmp, &cfg->tbls_hs_b) should > > > come after overlays. > > > > > > > Then you can program tbls_hs_b after overlay tables. Wouldn't that work? > > I am programming tbls_hs_b after overlay tables, just a heads up in case you > are surprised :). > Cool! - Mani -- மணிவண்ணன் சதாசிவம்
WARNING: multiple messages have this Message-ID (diff)
From: Manivannan Sadhasivam <mani@kernel.org> To: Can Guo <quic_cang@quicinc.com> Cc: Manivannan Sadhasivam <mani@kernel.org>, bvanassche@acm.org, adrian.hunter@intel.com, cmd4@qualcomm.com, beanhuo@micron.com, avri.altman@wdc.com, junwoo80.lee@samsung.com, martin.petersen@oracle.com, linux-scsi@vger.kernel.org, linux-arm-msm@vger.kernel.org, Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Vinod Koul <vkoul@kernel.org>, Kishon Vijay Abraham I <kishon@kernel.org>, "open list:GENERIC PHY FRAMEWORK" <linux-phy@lists.infradead.org>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v6 08/10] phy: qualcomm: phy-qcom-qmp-ufs: Add High Speed Gear 5 support for SM8550 Date: Thu, 30 Nov 2023 15:04:23 +0530 [thread overview] Message-ID: <20231130093423.GO3043@thinkpad> (raw) In-Reply-To: <c112b051-05ba-45f0-a9f3-f0da5afad4f7@quicinc.com> On Thu, Nov 30, 2023 at 04:49:12PM +0800, Can Guo wrote: > > > On 11/30/2023 4:38 PM, Manivannan Sadhasivam wrote: > > On Thu, Nov 30, 2023 at 04:14:25PM +0800, Can Guo wrote: [...] > > > > > +static int qmp_ufs_get_gear_overlay(struct qmp_ufs *qmp, const struct qmp_phy_cfg *cfg) > > > > > +{ > > > > > + u32 max_gear, floor_max_gear = cfg->max_supported_gear; > > > > > + int i = NUM_OVERLAY - 1; > > > > > > > > Just use i directly in the for loop. Also, please rename "i" with "idx" to make > > > > it clear. > > > > > > OK > > > > > > > > > > > > + int ret = -EINVAL; > > > > > + > > > > > + for (; i >= 0; i --) { > > > > > > > > i-- > > > > > > > > > + max_gear = cfg->tbls_hs_overlay[i].max_gear; > > > > > + > > > > > + if (max_gear == 0) > > > > > + continue; > > > > > > > > You are setting max_gear even for targets with a single overlay. How can this > > > > become 0? > > > > > > Say 8550 has two overlays, 8450 has one overlay. We are sweeping all > > > overlays as NUM_OVERLAY == 2, so for 8450, there is one overlay initialized, > > > another one not initialized (max_gear == 0), we are skipping the one which > > > is not initialized. > > > > > > > This is confusing at its best :) Please check for the existence of the actual > > table instead. Like, > > > > for (idx = NUM_OVERLAY - 1; i >= 0, i--) { > > > > /* Skip if the table is not available */ > > if (!cfg->tbls_hs_overlay[i].serdes) > > continue; > > > > ... > > } > > We cannot expect overlay must has its own serdes, or tx/rx/pcs, hence I am > checking max_gear intead of any specific member. > Hmm, then please add the comment as I suggested above. > > > > > > > > > > > + > > > > > + /* Direct matching, bail */ > > > > > + if (qmp->submode == max_gear) > > > > > + return i; > > > > > + > > > > > + /* If no direct matching, the lowest gear is the best matching */ > > > > > + if (max_gear < floor_max_gear) { > > > > > + ret = i; > > > > > + floor_max_gear = max_gear; > > > > > + } > > > > > + } > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg *cfg) > > > > > { > > > > > + int i; > > > > > + bool apply_overlay = false; > > > > > + > > > > > + i = qmp_ufs_get_gear_overlay(qmp, cfg); > > > > > + if (i >= 0) > > > > > + apply_overlay = true; > > > > > > > > How about? > > > > > > > > ``` > > > > int idx; > > > > > > > > idx = qmp_ufs_get_gear_overlay(qmp, cfg); > > > > > > > > qmp_ufs_serdes_init(qmp, &cfg->tbls); > > > > qmp_ufs_lanes_init(qmp, &cfg->tbls); > > > > ... > > > > > > > > if (idx >= 0) { > > > > qmp_ufs_serdes_init(qmp, &cfg->tbls_hs_overlay[idx]); > > > > qmp_ufs_lanes_init(qmp, &cfg->tbls_hs_overlay[idx]); > > > > ... > > > > } > > > > ``` > > > > > > > > Since the ordering doesn't matter for init sequence, you can program the overlay > > > > tables under a single condition. > > > > > > We can do that, but we need to be careful. When I say (in my previous reply) > > > the ordering does not matter, that saying is from the UFS PHY HPG doc. > > > However, in SW implementation, the 'tbls_hs_b' is actually overwriting one > > > COM_VCO_TUNE_MAP register, the same register is also programmed by common > > > table or overlay table. So qmp_ufs_serdes_init(qmp, &cfg->tbls_hs_b) should > > > come after overlays. > > > > > > > Then you can program tbls_hs_b after overlay tables. Wouldn't that work? > > I am programming tbls_hs_b after overlay tables, just a heads up in case you > are surprised :). > Cool! - Mani -- மணிவண்ணன் சதாசிவம் -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy
next prev parent reply other threads:[~2023-11-30 9:34 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-11-29 8:28 [PATCH v6 00/10] Enable HS-G5 support on SM8550 Can Guo 2023-11-29 8:28 ` Can Guo 2023-11-29 8:28 ` [PATCH v6 01/10] scsi: ufs: host: Rename structure ufs_dev_params to ufs_host_params Can Guo 2023-11-29 8:28 ` Can Guo 2023-11-29 18:11 ` Bart Van Assche 2023-11-29 18:11 ` Bart Van Assche 2023-11-29 8:28 ` [PATCH v6 02/10] scsi: ufs: ufs-qcom: No need to set hs_rate after ufshcd_init_host_param() Can Guo 2023-11-29 20:48 ` Andrew Halaney 2023-11-29 8:28 ` [PATCH v6 03/10] scsi: ufs: ufs-qcom: Setup host power mode during init Can Guo 2023-11-29 8:28 ` [PATCH v6 04/10] scsi: ufs: ufs-qcom: Allow the first init start with the maximum supported gear Can Guo 2023-11-30 6:42 ` Manivannan Sadhasivam 2023-11-30 7:44 ` Can Guo 2023-11-29 8:28 ` [PATCH v6 05/10] scsi: ufs: ufs-qcom: Limit HS-G5 Rate-A to hosts with HW version 5 Can Guo 2023-11-29 8:28 ` [PATCH v6 06/10] scsi: ufs: ufs-qcom: Set initial PHY gear to max HS gear for HW ver 4 and newer Can Guo 2023-11-30 5:52 ` Nitin Rawat 2023-11-30 6:47 ` Manivannan Sadhasivam 2023-11-30 7:49 ` Can Guo 2023-11-29 8:28 ` [PATCH v6 07/10] phy: qualcomm: phy-qcom-qmp-ufs: Rectify SM8550 UFS HS-G4 PHY Settings Can Guo 2023-11-29 8:28 ` Can Guo 2023-11-29 8:28 ` [PATCH v6 08/10] phy: qualcomm: phy-qcom-qmp-ufs: Add High Speed Gear 5 support for SM8550 Can Guo 2023-11-29 8:28 ` Can Guo 2023-11-30 7:12 ` Manivannan Sadhasivam 2023-11-30 7:12 ` Manivannan Sadhasivam 2023-11-30 8:14 ` Can Guo 2023-11-30 8:14 ` Can Guo 2023-11-30 8:38 ` Manivannan Sadhasivam 2023-11-30 8:38 ` Manivannan Sadhasivam 2023-11-30 8:49 ` Can Guo 2023-11-30 8:49 ` Can Guo 2023-11-30 9:34 ` Manivannan Sadhasivam [this message] 2023-11-30 9:34 ` Manivannan Sadhasivam 2023-11-30 9:35 ` Can Guo 2023-11-30 9:35 ` Can Guo 2023-11-29 8:28 ` [PATCH v6 09/10] scsi: ufs: ufs-qcom: Check return value of phy_set_mode_ext() Can Guo 2023-11-29 8:38 ` Nitin Rawat 2023-11-30 7:16 ` Manivannan Sadhasivam 2023-11-30 8:19 ` Can Guo 2023-11-29 8:28 ` [PATCH v6 10/10] scsi: ufs: ufs-qcom: Add support for UFS device version detection Can Guo 2023-11-29 19:04 ` Eric Chanudet 2023-11-30 7:28 ` Manivannan Sadhasivam 2023-11-30 7:25 ` Manivannan Sadhasivam 2023-11-30 8:21 ` Can Guo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20231130093423.GO3043@thinkpad \ --to=mani@kernel.org \ --cc=adrian.hunter@intel.com \ --cc=agross@kernel.org \ --cc=andersson@kernel.org \ --cc=avri.altman@wdc.com \ --cc=beanhuo@micron.com \ --cc=bvanassche@acm.org \ --cc=cmd4@qualcomm.com \ --cc=junwoo80.lee@samsung.com \ --cc=kishon@kernel.org \ --cc=konrad.dybcio@linaro.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-phy@lists.infradead.org \ --cc=linux-scsi@vger.kernel.org \ --cc=martin.petersen@oracle.com \ --cc=quic_cang@quicinc.com \ --cc=vkoul@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.