| Message ID | 1500293043-1887-4-git-send-email-varada@codeaurora.org |
|---|---|
| State | Superseded |
| Headers | show |
On Mon 17 Jul 05:03 PDT 2017, Varadarajan Narayanan wrote: > Presently, the phy pipe clock's name is assumed to be either > usb3_phy_pipe_clk_src or pcie_XX_pipe_clk_src (where XX is the > phy lane's number). However, this will not work if an SoC has > more than one instance of the phy. Hence, instead of assuming > the name of the clock, fetch it from the DT. > Adding the support to fetch this from DT looks reasonable, but you should make sure to fall back to the old logic in case you don't find a "clock-output-names" property. [..] > @@ -1110,6 +1103,8 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > > id = 0; > for_each_available_child_of_node(dev->of_node, child) { > + const char *clk_name; > + > /* Create per-lane phy */ > ret = qcom_qmp_phy_create(dev, child, id); > if (ret) { > @@ -1118,11 +1113,20 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > return ret; > } > > + ret = of_property_read_string(child, "clock-output-names", > + &clk_name); > + if (ret) { This would be the case for any existing dts files, so you're not allowed to treat this as an error. > + dev_err(dev, > + "failed to get clock-output-names for lane%d phy, %d\n", > + id, ret); > + return ret; > + } > + Regards, Bjorn
Bjorn, On Mon, Jul 17, 2017 at 03:30:47PM -0700, Bjorn Andersson wrote: > On Mon 17 Jul 05:03 PDT 2017, Varadarajan Narayanan wrote: > > > Presently, the phy pipe clock's name is assumed to be either > > usb3_phy_pipe_clk_src or pcie_XX_pipe_clk_src (where XX is the > > phy lane's number). However, this will not work if an SoC has > > more than one instance of the phy. Hence, instead of assuming > > the name of the clock, fetch it from the DT. > > > > Adding the support to fetch this from DT looks reasonable, but you > should make sure to fall back to the old logic in case you don't find a > "clock-output-names" property. If more than one instance of the IP block is instantiated in the SoC, it will try to register the same clock (i.e. pcie_0_pipe_clk_src or usb3_phy_pipe_clk_src) more than once and devm_clk_hw_register() will fail for the second and subsequent instances of the IP, resulting in the pipe_clk not being able to find its parent. Additionally, the clock name itself differs between SoCs. For example, in MSM8996 it is usb3_phy_pipe_clk_src for USB and pcie_X_pipe_clk_src (where X is lane number 0, 1 or 2). In IPQ8074 it is usb3phy_X_cc_pipe_clk or pcie20_phyX_pipe_clk (where X is IP instance number 0 or 1). MSM8996, has one instance of the IP with 3 different lanes, whereas IPQ8074 has two instances with one lane in each instance. A future SoC might have multiple instances with each instance having different number of lanes and the clock's name might have X & Y to indicate instance number and lane numbers. Hence, if the "clock-output-names" property is not available there is no logic that can correctly "guess" the clock's name. The old logic incorrectly assumes the format of the clock name, resulting in devm_clk_hw_register() failures and pipe_clk not being linked to its proper parent. > [..] > > @@ -1110,6 +1103,8 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > > > > id = 0; > > for_each_available_child_of_node(dev->of_node, child) { > > + const char *clk_name; > > + > > /* Create per-lane phy */ > > ret = qcom_qmp_phy_create(dev, child, id); > > if (ret) { > > @@ -1118,11 +1113,20 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > > return ret; > > } > > > > + ret = of_property_read_string(child, "clock-output-names", > > + &clk_name); > > + if (ret) { > > This would be the case for any existing dts files, so you're not allowed > to treat this as an error. Since, there are no dts files that presently enable this driver, wouldn't it be better to flag this as an error now itself instead of having to fall back to old handling (which as mentioned above is incomplete). Please let me know. Thanks Varada > > + dev_err(dev, > > + "failed to get clock-output-names for lane%d phy, %d\n", > > + id, ret); > > + return ret; > > + } > > + > > Regards, > Bjorn -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On Tue 18 Jul 01:54 PDT 2017, Varadarajan Narayanan wrote: > On Mon, Jul 17, 2017 at 03:30:47PM -0700, Bjorn Andersson wrote: [..] > > > > This would be the case for any existing dts files, so you're not allowed > > to treat this as an error. > > Since, there are no dts files that presently enable this driver, > wouldn't it be better to flag this as an error now itself instead > of having to fall back to old handling (which as mentioned above > is incomplete). Please let me know. > You're right, hopefully we can introduce it in the upstream dts for v4.14... So it does make sense to fix this up now, rather than live with the fallback forever. Can you ensure we get an "ack" from Vivek on this? Regards, Bjorn
On Tue, Jul 18, 2017 at 10:26 PM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Tue 18 Jul 01:54 PDT 2017, Varadarajan Narayanan wrote: >> On Mon, Jul 17, 2017 at 03:30:47PM -0700, Bjorn Andersson wrote: > [..] >> > >> > This would be the case for any existing dts files, so you're not allowed >> > to treat this as an error. >> >> Since, there are no dts files that presently enable this driver, >> wouldn't it be better to flag this as an error now itself instead >> of having to fall back to old handling (which as mentioned above >> is incomplete). Please let me know. >> > > You're right, hopefully we can introduce it in the upstream dts for > v4.14... > > So it does make sense to fix this up now, rather than live with the > fallback forever. Can you ensure we get an "ack" from Vivek on this? Right, it makes sense to get this name from the dt (since same will be also available in the clock entry), and not assume the name based on some logic. And since we don't have any already existing bindings, it's good that we fix it now. > > Regards, > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Jul 17, 2017 at 5:33 PM, Varadarajan Narayanan <varada@codeaurora.org> wrote: > Presently, the phy pipe clock's name is assumed to be either > usb3_phy_pipe_clk_src or pcie_XX_pipe_clk_src (where XX is the > phy lane's number). However, this will not work if an SoC has > more than one instance of the phy. Hence, instead of assuming > the name of the clock, fetch it from the DT. > > Signed-off-by: Varadarajan Narayanan <varada@codeaurora.org> > --- Looks good. Acked-by: Vivek Gautam <vivek.gautam@codeaurora.org> > drivers/phy/qualcomm/phy-qcom-qmp.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c > index 78ca628..97020ec 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > @@ -925,20 +925,13 @@ static int qcom_qmp_phy_clk_init(struct device *dev) > * clk | +-------+ | +-----+ > * +---------------+ > */ > -static int phy_pipe_clk_register(struct qcom_qmp *qmp, int id) > +static int phy_pipe_clk_register(struct qcom_qmp *qmp, const char *clk_name) > { > - char name[24]; > struct clk_fixed_rate *fixed; > struct clk_init_data init = { }; > > - switch (qmp->cfg->type) { > - case PHY_TYPE_USB3: > - snprintf(name, sizeof(name), "usb3_phy_pipe_clk_src"); > - break; > - case PHY_TYPE_PCIE: > - snprintf(name, sizeof(name), "pcie_%d_pipe_clk_src", id); > - break; > - default: > + if ((qmp->cfg->type != PHY_TYPE_USB3) && > + (qmp->cfg->type != PHY_TYPE_PCIE)) { > /* not all phys register pipe clocks, so return success */ > return 0; > } > @@ -947,7 +940,7 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, int id) > if (!fixed) > return -ENOMEM; > > - init.name = name; > + init.name = clk_name; > init.ops = &clk_fixed_rate_ops; > > /* controllers using QMP phys use 125MHz pipe clock interface */ > @@ -1110,6 +1103,8 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > > id = 0; > for_each_available_child_of_node(dev->of_node, child) { > + const char *clk_name; > + > /* Create per-lane phy */ > ret = qcom_qmp_phy_create(dev, child, id); > if (ret) { > @@ -1118,11 +1113,20 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > return ret; > } > > + ret = of_property_read_string(child, "clock-output-names", > + &clk_name); > + if (ret) { > + dev_err(dev, > + "failed to get clock-output-names for lane%d phy, %d\n", > + id, ret); > + return ret; > + } > + > /* > * Register the pipe clock provided by phy. > * See function description to see details of this pipe clock. > */ > - ret = phy_pipe_clk_register(qmp, id); > + ret = phy_pipe_clk_register(qmp, clk_name); > if (ret) { > dev_err(qmp->dev, > "failed to register pipe clock source\n"); > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c index 78ca628..97020ec 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c @@ -925,20 +925,13 @@ static int qcom_qmp_phy_clk_init(struct device *dev) * clk | +-------+ | +-----+ * +---------------+ */ -static int phy_pipe_clk_register(struct qcom_qmp *qmp, int id) +static int phy_pipe_clk_register(struct qcom_qmp *qmp, const char *clk_name) { - char name[24]; struct clk_fixed_rate *fixed; struct clk_init_data init = { }; - switch (qmp->cfg->type) { - case PHY_TYPE_USB3: - snprintf(name, sizeof(name), "usb3_phy_pipe_clk_src"); - break; - case PHY_TYPE_PCIE: - snprintf(name, sizeof(name), "pcie_%d_pipe_clk_src", id); - break; - default: + if ((qmp->cfg->type != PHY_TYPE_USB3) && + (qmp->cfg->type != PHY_TYPE_PCIE)) { /* not all phys register pipe clocks, so return success */ return 0; } @@ -947,7 +940,7 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, int id) if (!fixed) return -ENOMEM; - init.name = name; + init.name = clk_name; init.ops = &clk_fixed_rate_ops; /* controllers using QMP phys use 125MHz pipe clock interface */ @@ -1110,6 +1103,8 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) id = 0; for_each_available_child_of_node(dev->of_node, child) { + const char *clk_name; + /* Create per-lane phy */ ret = qcom_qmp_phy_create(dev, child, id); if (ret) { @@ -1118,11 +1113,20 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) return ret; } + ret = of_property_read_string(child, "clock-output-names", + &clk_name); + if (ret) { + dev_err(dev, + "failed to get clock-output-names for lane%d phy, %d\n", + id, ret); + return ret; + } + /* * Register the pipe clock provided by phy. * See function description to see details of this pipe clock. */ - ret = phy_pipe_clk_register(qmp, id); + ret = phy_pipe_clk_register(qmp, clk_name); if (ret) { dev_err(qmp->dev, "failed to register pipe clock source\n");
Presently, the phy pipe clock's name is assumed to be either usb3_phy_pipe_clk_src or pcie_XX_pipe_clk_src (where XX is the phy lane's number). However, this will not work if an SoC has more than one instance of the phy. Hence, instead of assuming the name of the clock, fetch it from the DT. Signed-off-by: Varadarajan Narayanan <varada@codeaurora.org> --- drivers/phy/qualcomm/phy-qcom-qmp.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)