Add generic QUSB2 V2 PHY table so the respective phys
can use the same table.
Signed-off-by: Sandeep Maheswaram <[email protected]>
Reviewed-by: Matthias Kaehlcke <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
drivers/phy/qualcomm/phy-qcom-qusb2.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index bf94a52..70c9da6 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * Copyright (c) 2017, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2017, 2019, The Linux Foundation. All rights reserved.
*/
#include <linux/clk.h>
@@ -177,7 +177,7 @@ static const struct qusb2_phy_init_tbl msm8998_init_tbl[] = {
QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_DIGITAL_TIMERS_TWO, 0x19),
};
-static const unsigned int sdm845_regs_layout[] = {
+static const unsigned int qusb2_v2_regs_layout[] = {
[QUSB2PHY_PLL_CORE_INPUT_OVERRIDE] = 0xa8,
[QUSB2PHY_PLL_STATUS] = 0x1a0,
[QUSB2PHY_PORT_TUNE1] = 0x240,
@@ -191,7 +191,7 @@ static const unsigned int sdm845_regs_layout[] = {
[QUSB2PHY_INTR_CTRL] = 0x230,
};
-static const struct qusb2_phy_init_tbl sdm845_init_tbl[] = {
+static const struct qusb2_phy_init_tbl qusb2_v2_init_tbl[] = {
QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_ANALOG_CONTROLS_TWO, 0x03),
QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_CLOCK_INVERTERS, 0x7c),
QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_CMODE, 0x80),
@@ -258,10 +258,10 @@ static const struct qusb2_phy_cfg msm8998_phy_cfg = {
.update_tune1_with_efuse = true,
};
-static const struct qusb2_phy_cfg sdm845_phy_cfg = {
- .tbl = sdm845_init_tbl,
- .tbl_num = ARRAY_SIZE(sdm845_init_tbl),
- .regs = sdm845_regs_layout,
+static const struct qusb2_phy_cfg qusb2_v2_phy_cfg = {
+ .tbl = qusb2_v2_init_tbl,
+ .tbl_num = ARRAY_SIZE(qusb2_v2_init_tbl),
+ .regs = qusb2_v2_regs_layout,
.disable_ctrl = (PWR_CTRL1_VREF_SUPPLY_TRIM | PWR_CTRL1_CLAMP_N_EN |
POWER_DOWN),
@@ -774,8 +774,8 @@ static const struct of_device_id qusb2_phy_of_match_table[] = {
.compatible = "qcom,msm8998-qusb2-phy",
.data = &msm8998_phy_cfg,
}, {
- .compatible = "qcom,sdm845-qusb2-phy",
- .data = &sdm845_phy_cfg,
+ .compatible = "qcom,qusb2-v2-phy",
+ .data = &qusb2_v2_phy_cfg,
},
{ },
};
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
On Mon, Mar 9, 2020 at 2:54 AM Sandeep Maheswaram <[email protected]> wrote:
> @@ -774,8 +774,8 @@ static const struct of_device_id qusb2_phy_of_match_table[] = {
> .compatible = "qcom,msm8998-qusb2-phy",
> .data = &msm8998_phy_cfg,
> }, {
> - .compatible = "qcom,sdm845-qusb2-phy",
> - .data = &sdm845_phy_cfg,
> + .compatible = "qcom,qusb2-v2-phy",
> + .data = &qusb2_v2_phy_cfg,
> },
> { },
> };
Just as a heads up, Yongqin (cc'ed) reported this patch (now upstream)
seems to be causing trouble on the db845c.
It seems like its removing support for the "qcom,sdm845-qusb2-phy"
compatible string, which is documented:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml#n23
and already in use:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845.dtsi#n2389
Should this instead have been an addition of the "qcom,qusb2-v2-phy",
line instead of replacing "qcom,sdm845-qusb2-phy"?
thanks
-john
In commit 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2
PHY support"), the change was made to add "qcom,qusb2-v2-phy"
as a generic compat string. However the change also removed
the "qcom,sdm845-qusb2-phy" compat string, which is documented
in the binding and already in use.
This patch re-adds the "qcom,sdm845-qusb2-phy" compat string
which allows the driver to continue to work with existing dts
entries such as found on the db845c.
Cc: Andy Gross <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Doug Anderson <[email protected]>
Cc: Manu Gautam <[email protected]>
Cc: Sandeep Maheswaram <[email protected]>
Cc: Matthias Kaehlcke <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: Kishon Vijay Abraham I <[email protected]>
Cc: [email protected]
Cc: [email protected]
Fixes: 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2 PHY support")
Reported-by: YongQin Liu <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
drivers/phy/qualcomm/phy-qcom-qusb2.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index 3708d43b7508..ab7941ce5d3a 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -815,6 +815,9 @@ static const struct of_device_id qusb2_phy_of_match_table[] = {
}, {
.compatible = "qcom,msm8998-qusb2-phy",
.data = &msm8998_phy_cfg,
+ }, {
+ .compatible = "qcom,sdm845-qusb2-phy",
+ .data = &qusb2_v2_phy_cfg,
}, {
.compatible = "qcom,qusb2-v2-phy",
.data = &qusb2_v2_phy_cfg,
--
2.17.1
Hi,
On Thu, Apr 2, 2020 at 2:39 PM John Stultz <[email protected]> wrote:
>
> On Mon, Mar 9, 2020 at 2:54 AM Sandeep Maheswaram <[email protected]> wrote:
> > @@ -774,8 +774,8 @@ static const struct of_device_id qusb2_phy_of_match_table[] = {
> > .compatible = "qcom,msm8998-qusb2-phy",
> > .data = &msm8998_phy_cfg,
> > }, {
> > - .compatible = "qcom,sdm845-qusb2-phy",
> > - .data = &sdm845_phy_cfg,
> > + .compatible = "qcom,qusb2-v2-phy",
> > + .data = &qusb2_v2_phy_cfg,
> > },
> > { },
> > };
>
> Just as a heads up, Yongqin (cc'ed) reported this patch (now upstream)
> seems to be causing trouble on the db845c.
>
> It seems like its removing support for the "qcom,sdm845-qusb2-phy"
> compatible string, which is documented:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml#n23
>
> and already in use:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845.dtsi#n2389
>
> Should this instead have been an addition of the "qcom,qusb2-v2-phy",
> line instead of replacing "qcom,sdm845-qusb2-phy"?
To avoid forking the discussion into two threads, I'll suggest that we
focus on keeping the discussion in reply to your newly proposed patch.
I've already replied there:
https://lore.kernel.org/r/CAD=FV=VGT75c4_ErQAJgNtcCd2Jzv0A2KpfEkS637GqOhamj9Q@mail.gmail.com
-Doug
Hi,
On Thu, Apr 2, 2020 at 3:37 PM John Stultz <[email protected]> wrote:
>
> In commit 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2
> PHY support"), the change was made to add "qcom,qusb2-v2-phy"
> as a generic compat string. However the change also removed
> the "qcom,sdm845-qusb2-phy" compat string, which is documented
> in the binding and already in use.
>
> This patch re-adds the "qcom,sdm845-qusb2-phy" compat string
> which allows the driver to continue to work with existing dts
> entries such as found on the db845c.
>
> Cc: Andy Gross <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Manu Gautam <[email protected]>
> Cc: Sandeep Maheswaram <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: Kishon Vijay Abraham I <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Fixes: 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2 PHY support")
> Reported-by: YongQin Liu <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qusb2.c | 3 +++
> 1 file changed, 3 insertions(+)
Do you have an out-of-tree dts file? If not, I'd prefer that the fix
was for this patch to land instead:
https://lore.kernel.org/r/[email protected]
While we can land your patch if someone needs it for supporting an
out-of-tree dts, it gives people supporting future SoCs the idea that
they need to add themselves to this table too. That's now discouraged
unless there's a specific quirk that needs to be handled just for this
SoC.
-Doug
On Thu 02 Apr 15:37 PDT 2020, John Stultz wrote:
> In commit 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2
> PHY support"), the change was made to add "qcom,qusb2-v2-phy"
> as a generic compat string. However the change also removed
> the "qcom,sdm845-qusb2-phy" compat string, which is documented
> in the binding and already in use.
>
> This patch re-adds the "qcom,sdm845-qusb2-phy" compat string
> which allows the driver to continue to work with existing dts
> entries such as found on the db845c.
>
> Cc: Andy Gross <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Manu Gautam <[email protected]>
> Cc: Sandeep Maheswaram <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: Kishon Vijay Abraham I <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Fixes: 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2 PHY support")
> Reported-by: YongQin Liu <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> ---
> drivers/phy/qualcomm/phy-qcom-qusb2.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> index 3708d43b7508..ab7941ce5d3a 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> @@ -815,6 +815,9 @@ static const struct of_device_id qusb2_phy_of_match_table[] = {
> }, {
> .compatible = "qcom,msm8998-qusb2-phy",
> .data = &msm8998_phy_cfg,
> + }, {
> + .compatible = "qcom,sdm845-qusb2-phy",
> + .data = &qusb2_v2_phy_cfg,
> }, {
> .compatible = "qcom,qusb2-v2-phy",
> .data = &qusb2_v2_phy_cfg,
> --
> 2.17.1
>
On Thu, Apr 2, 2020 at 3:56 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Thu, Apr 2, 2020 at 3:37 PM John Stultz <[email protected]> wrote:
> >
> > In commit 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2
> > PHY support"), the change was made to add "qcom,qusb2-v2-phy"
> > as a generic compat string. However the change also removed
> > the "qcom,sdm845-qusb2-phy" compat string, which is documented
> > in the binding and already in use.
> >
> > This patch re-adds the "qcom,sdm845-qusb2-phy" compat string
> > which allows the driver to continue to work with existing dts
> > entries such as found on the db845c.
> >
> > Cc: Andy Gross <[email protected]>
> > Cc: Bjorn Andersson <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: Doug Anderson <[email protected]>
> > Cc: Manu Gautam <[email protected]>
> > Cc: Sandeep Maheswaram <[email protected]>
> > Cc: Matthias Kaehlcke <[email protected]>
> > Cc: Stephen Boyd <[email protected]>
> > Cc: Kishon Vijay Abraham I <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Fixes: 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2 PHY support")
> > Reported-by: YongQin Liu <[email protected]>
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > drivers/phy/qualcomm/phy-qcom-qusb2.c | 3 +++
> > 1 file changed, 3 insertions(+)
>
> Do you have an out-of-tree dts file? If not, I'd prefer that the fix
> was for this patch to land instead:
>
> https://lore.kernel.org/r/[email protected]
No, no out of tree dts. The usage is already in tree:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845.dtsi#n2389
> While we can land your patch if someone needs it for supporting an
> out-of-tree dts, it gives people supporting future SoCs the idea that
> they need to add themselves to this table too. That's now discouraged
> unless there's a specific quirk that needs to be handled just for this
> SoC.
My understanding with dts bindings is that they are effectively an
ABI. While maybe it makes sense to deprecate the
"qcom,sdm845-qusb2-phy" string in the Documentation to avoid new
users, I'd think we'd want to keep the support in the driver as we
aren't supposed to have tight coupling between the DTB and kernel (at
least for official bindings).
Granted, I've not gotten much experience with boards that were fully
upstream and thus didn't have an eternally evolving dts file that had
to be kept in sync with the kernel, so in practice either solution
does work for me, but in theory it seems like we should at least
pretend these things are stable. :)
thanks
-john
Hi,
On Thu, Apr 2, 2020 at 4:08 PM John Stultz <[email protected]> wrote:
>
> On Thu, Apr 2, 2020 at 3:56 PM Doug Anderson <[email protected]> wrote:
> >
> > Hi,
> >
> > On Thu, Apr 2, 2020 at 3:37 PM John Stultz <[email protected]> wrote:
> > >
> > > In commit 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2
> > > PHY support"), the change was made to add "qcom,qusb2-v2-phy"
> > > as a generic compat string. However the change also removed
> > > the "qcom,sdm845-qusb2-phy" compat string, which is documented
> > > in the binding and already in use.
> > >
> > > This patch re-adds the "qcom,sdm845-qusb2-phy" compat string
> > > which allows the driver to continue to work with existing dts
> > > entries such as found on the db845c.
> > >
> > > Cc: Andy Gross <[email protected]>
> > > Cc: Bjorn Andersson <[email protected]>
> > > Cc: Rob Herring <[email protected]>
> > > Cc: Mark Rutland <[email protected]>
> > > Cc: Doug Anderson <[email protected]>
> > > Cc: Manu Gautam <[email protected]>
> > > Cc: Sandeep Maheswaram <[email protected]>
> > > Cc: Matthias Kaehlcke <[email protected]>
> > > Cc: Stephen Boyd <[email protected]>
> > > Cc: Kishon Vijay Abraham I <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Fixes: 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2 PHY support")
> > > Reported-by: YongQin Liu <[email protected]>
> > > Signed-off-by: John Stultz <[email protected]>
> > > ---
> > > drivers/phy/qualcomm/phy-qcom-qusb2.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> >
> > Do you have an out-of-tree dts file? If not, I'd prefer that the fix
> > was for this patch to land instead:
> >
> > https://lore.kernel.org/r/[email protected]
>
> No, no out of tree dts. The usage is already in tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845.dtsi#n2389
>
>
> > While we can land your patch if someone needs it for supporting an
> > out-of-tree dts, it gives people supporting future SoCs the idea that
> > they need to add themselves to this table too. That's now discouraged
> > unless there's a specific quirk that needs to be handled just for this
> > SoC.
>
> My understanding with dts bindings is that they are effectively an
> ABI. While maybe it makes sense to deprecate the
> "qcom,sdm845-qusb2-phy" string in the Documentation to avoid new
> users, I'd think we'd want to keep the support in the driver as we
> aren't supposed to have tight coupling between the DTB and kernel (at
> least for official bindings).
If nothing else if we're going to land your patch, can you at least
put a comment in there that says "only needed to support legacy device
trees that didn't include "qcom,qusb2-v2-phy" in the compatible
string. Then the person who adds the next Qualcomm SoC will know not
to add themselves to the table too.
> Granted, I've not gotten much experience with boards that were fully
> upstream and thus didn't have an eternally evolving dts file that had
> to be kept in sync with the kernel, so in practice either solution
> does work for me, but in theory it seems like we should at least
> pretend these things are stable. :)
Yeah, I don't want to get into the whole stable ABI argument, but what
you say is the official word. The bindings are supposed to be a
stable ABI and it's a good goal to strive for.
...but in reality most people are OK with it not being quite so stable
as long as it's not hurting anyone. What should have happened here is
that the bindings and dts should have landed in one Linux version and
the driver change landed in the next Linux version. Now we're stuck
with the breakage, though. :( In general for "new" architectures
it's considered more OK to break compatibility, though I guess you can
argue whether sdm845 is really new enough. I guess to get at the meat
of the issue though: if you need a patch to fix your problem anyway,
why not land the patch that doesn't end up chewing extra up extra code
space and providing a bad example for someone to copy?
Now certainly if changing your DTS was an undue burden (like you've
already baked device trees into firmware) there's no question we
should land your patch. I'm just not sure the lofty goal of "it's
supposed to be a stable ABI so let's add an entry to the table that
nobody will ever care about after the dts change lands" is enough of a
reason to land it now.
OK, I'll duck from the tomatoes now.
-Doug
In commit 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2
PHY support"), the change was made to add "qcom,qusb2-v2-phy"
as a generic compat string. However the change also removed
the "qcom,sdm845-qusb2-phy" compat string, which is documented
in the binding and already in use.
This patch re-adds the "qcom,sdm845-qusb2-phy" compat string
which allows the driver to continue to work with existing dts
entries such as found on the db845c.
Cc: Andy Gross <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Doug Anderson <[email protected]>
Cc: Manu Gautam <[email protected]>
Cc: Sandeep Maheswaram <[email protected]>
Cc: Matthias Kaehlcke <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: Kishon Vijay Abraham I <[email protected]>
Cc: [email protected]
Cc: [email protected]
Reviewed-by: Bjorn Andersson <[email protected]>
Fixes: 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2 PHY support")
Reported-by: YongQin Liu <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
v2: Add deprecation note on "qcom,sdm845-qusb2-phy" string
as suggested by Doug.
---
drivers/phy/qualcomm/phy-qcom-qusb2.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index 3708d43b7508..393011a05b48 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -815,6 +815,13 @@ static const struct of_device_id qusb2_phy_of_match_table[] = {
}, {
.compatible = "qcom,msm8998-qusb2-phy",
.data = &msm8998_phy_cfg,
+ }, {
+ /*
+ * Deprecated. Only here to support legacy device
+ * trees that didn't include "qcom,qusb2-v2-phy"
+ */
+ .compatible = "qcom,sdm845-qusb2-phy",
+ .data = &qusb2_v2_phy_cfg,
}, {
.compatible = "qcom,qusb2-v2-phy",
.data = &qusb2_v2_phy_cfg,
--
2.17.1
Hi,
On Thu, Apr 2, 2020 at 4:45 PM John Stultz <[email protected]> wrote:
>
> In commit 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2
> PHY support"), the change was made to add "qcom,qusb2-v2-phy"
> as a generic compat string. However the change also removed
> the "qcom,sdm845-qusb2-phy" compat string, which is documented
> in the binding and already in use.
>
> This patch re-adds the "qcom,sdm845-qusb2-phy" compat string
> which allows the driver to continue to work with existing dts
> entries such as found on the db845c.
>
> Cc: Andy Gross <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Manu Gautam <[email protected]>
> Cc: Sandeep Maheswaram <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: Kishon Vijay Abraham I <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Reviewed-by: Bjorn Andersson <[email protected]>
> Fixes: 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2 PHY support")
> Reported-by: YongQin Liu <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> v2: Add deprecation note on "qcom,sdm845-qusb2-phy" string
> as suggested by Doug.
> ---
> drivers/phy/qualcomm/phy-qcom-qusb2.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> index 3708d43b7508..393011a05b48 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> @@ -815,6 +815,13 @@ static const struct of_device_id qusb2_phy_of_match_table[] = {
> }, {
> .compatible = "qcom,msm8998-qusb2-phy",
> .data = &msm8998_phy_cfg,
> + }, {
> + /*
> + * Deprecated. Only here to support legacy device
> + * trees that didn't include "qcom,qusb2-v2-phy"
> + */
> + .compatible = "qcom,sdm845-qusb2-phy",
> + .data = &qusb2_v2_phy_cfg,
Thanks for adding the comment. With that, I'll still grumble but I'm
OK with this if people really want it.
Reviewed-by: Douglas Anderson <[email protected]>
-Doug
On Thu, Apr 2, 2020 at 4:19 PM Doug Anderson <[email protected]> wrote:
> On Thu, Apr 2, 2020 at 4:08 PM John Stultz <[email protected]> wrote:
> > My understanding with dts bindings is that they are effectively an
> > ABI. While maybe it makes sense to deprecate the
> > "qcom,sdm845-qusb2-phy" string in the Documentation to avoid new
> > users, I'd think we'd want to keep the support in the driver as we
> > aren't supposed to have tight coupling between the DTB and kernel (at
> > least for official bindings).
>
> If nothing else if we're going to land your patch, can you at least
> put a comment in there that says "only needed to support legacy device
> trees that didn't include "qcom,qusb2-v2-phy" in the compatible
> string. Then the person who adds the next Qualcomm SoC will know not
> to add themselves to the table too.
Done.
> > Granted, I've not gotten much experience with boards that were fully
> > upstream and thus didn't have an eternally evolving dts file that had
> > to be kept in sync with the kernel, so in practice either solution
> > does work for me, but in theory it seems like we should at least
> > pretend these things are stable. :)
>
> Yeah, I don't want to get into the whole stable ABI argument, but what
> you say is the official word. The bindings are supposed to be a
> stable ABI and it's a good goal to strive for.
>
> ...but in reality most people are OK with it not being quite so stable
> as long as it's not hurting anyone. What should have happened here is
> that the bindings and dts should have landed in one Linux version and
> the driver change landed in the next Linux version. Now we're stuck
> with the breakage, though. :( In general for "new" architectures
> it's considered more OK to break compatibility, though I guess you can
> argue whether sdm845 is really new enough. I guess to get at the meat
> of the issue though: if you need a patch to fix your problem anyway,
> why not land the patch that doesn't end up chewing extra up extra code
> space and providing a bad example for someone to copy?
>
> Now certainly if changing your DTS was an undue burden (like you've
> already baked device trees into firmware) there's no question we
> should land your patch. I'm just not sure the lofty goal of "it's
> supposed to be a stable ABI so let's add an entry to the table that
> nobody will ever care about after the dts change lands" is enough of a
> reason to land it now.
Personally, I'm fine with either solution (as there's still dts
changes for db845c pending that we're carrying), but I also want to
make sure we're setting a good standard for future changes (as these
sorts of things seem to bite me far too frequently on the db845c,
sometimes even resulting in forced userland changes that we've so far
been able to adapt to, but are not ideal).
So I've resubmitted my version to let the maintainers decide. :)
thanks
-john
On Thu 02 Apr 15:56 PDT 2020, Doug Anderson wrote:
> Hi,
>
> On Thu, Apr 2, 2020 at 3:37 PM John Stultz <[email protected]> wrote:
> >
> > In commit 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2
> > PHY support"), the change was made to add "qcom,qusb2-v2-phy"
> > as a generic compat string. However the change also removed
> > the "qcom,sdm845-qusb2-phy" compat string, which is documented
> > in the binding and already in use.
> >
> > This patch re-adds the "qcom,sdm845-qusb2-phy" compat string
> > which allows the driver to continue to work with existing dts
> > entries such as found on the db845c.
> >
> > Cc: Andy Gross <[email protected]>
> > Cc: Bjorn Andersson <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: Doug Anderson <[email protected]>
> > Cc: Manu Gautam <[email protected]>
> > Cc: Sandeep Maheswaram <[email protected]>
> > Cc: Matthias Kaehlcke <[email protected]>
> > Cc: Stephen Boyd <[email protected]>
> > Cc: Kishon Vijay Abraham I <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Fixes: 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2 PHY support")
> > Reported-by: YongQin Liu <[email protected]>
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > drivers/phy/qualcomm/phy-qcom-qusb2.c | 3 +++
> > 1 file changed, 3 insertions(+)
>
> Do you have an out-of-tree dts file? If not, I'd prefer that the fix
> was for this patch to land instead:
>
> https://lore.kernel.org/r/[email protected]
>
> While we can land your patch if someone needs it for supporting an
> out-of-tree dts, it gives people supporting future SoCs the idea that
> they need to add themselves to this table too. That's now discouraged
> unless there's a specific quirk that needs to be handled just for this
> SoC.
>
Afaict the compatible has been in use in the upstream sdm845.dtsi since
v4.20 and we do have an outspoken rule that we don't break backwards
compatibility with existing DTBs.
There are cases where it makes sense to break this rule, e.g. to fix
something that's clearly broken, but I don't see that this is such a
case.
Regards,
Bjorn
Quoting John Stultz (2020-04-02 16:44:55)
> In commit 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2
> PHY support"), the change was made to add "qcom,qusb2-v2-phy"
> as a generic compat string. However the change also removed
> the "qcom,sdm845-qusb2-phy" compat string, which is documented
> in the binding and already in use.
>
> This patch re-adds the "qcom,sdm845-qusb2-phy" compat string
> which allows the driver to continue to work with existing dts
> entries such as found on the db845c.
>
> Cc: Andy Gross <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Manu Gautam <[email protected]>
> Cc: Sandeep Maheswaram <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: Kishon Vijay Abraham I <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Reviewed-by: Bjorn Andersson <[email protected]>
> Fixes: 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2 PHY support")
> Reported-by: YongQin Liu <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
Reviewed-by: Stephen Boyd <[email protected]>
Might be worth it sending it outside of this thread in case it's missed.