[PATCH v2 0/2] phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework

[PATCH v2 0/2] phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework

[Abel Vesa]

Found the first issue (from first patch) while adding support
for X Elite (X1E80100) which comes with more than one repeaters.
The second fix is just bonus.

---
Changes in v2:
- The regfields is being dropped from the repeater init, but it's done
  in the second patch in order to not break bisectability, as it is
  still needed by the zero-out loop.
- Added Konrad's R-b tag to the first patch. Did not add Elliot's T-b
  tag as the second patch has been reworked massively.
- The zero-out loop is dropped now by holding a copy of the init_tlb in
  the container struct. This led to dropping the cfg from the container
  struct (see second patch commit message for more details).
- Link to v1: https://lore.kernel.org/r/20240104-phy-qcom-eusb2-repeater-fixes-v1-0-047b7b6b8333@linaro.org

---
Abel Vesa (2):
      phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances
      phy: qualcomm: eusb2-repeater: Rework init to drop redundant zero-out loop

 drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 53 ++++++++++++++------------
 1 file changed, 28 insertions(+), 25 deletions(-)
---
base-commit: e2425464bc87159274879ab30f9d4fe624b9fcd2
change-id: 20240104-phy-qcom-eusb2-repeater-fixes-c9201113032c

Best regards,
-- 
Abel Vesa <abel.vesa@linaro.org>


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 1/2] phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances

[Abel Vesa]

The global regmap fields offsets currently get incremented with the base
address of the repeater. This issue doesn't get noticed unless the probe
defers or there are multiple repeaters on that platform. So instead of
incrementing the global ones, copy them for each instance of the
repeater.

Fixes: 4ba2e52718c0 ("phy: qualcomm: phy-qcom-eusb2-repeater: Use regmap_fields")
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
index a623f092b11f..5f5862a68b73 100644
--- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
+++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
@@ -101,6 +101,7 @@ struct eusb2_repeater {
 	struct regmap_field *regs[F_NUM_FIELDS];
 	struct phy *phy;
 	struct regulator_bulk_data *vregs;
+	struct reg_field *regfields;
 	const struct eusb2_repeater_cfg *cfg;
 	enum phy_mode mode;
 };
@@ -140,8 +141,8 @@ static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr)
 
 static int eusb2_repeater_init(struct phy *phy)
 {
-	struct reg_field *regfields = eusb2_repeater_tune_reg_fields;
 	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
+	struct reg_field *regfields = rptr->regfields;
 	struct device_node *np = rptr->dev->of_node;
 	u32 init_tbl[F_NUM_TUNE_FIELDS] = { 0 };
 	u8 override;
@@ -262,15 +263,21 @@ static int eusb2_repeater_probe(struct platform_device *pdev)
 	if (!regmap)
 		return -ENODEV;
 
+	rptr->regfields = devm_kmemdup(dev, eusb2_repeater_tune_reg_fields,
+				       sizeof(eusb2_repeater_tune_reg_fields),
+				       GFP_KERNEL);
+	if (!rptr->regfields)
+		return -ENOMEM;
+
 	ret = of_property_read_u32(np, "reg", &res);
 	if (ret < 0)
 		return ret;
 
 	for (i = 0; i < F_NUM_FIELDS; i++)
-		eusb2_repeater_tune_reg_fields[i].reg += res;
+		rptr->regfields[i].reg += res;
 
 	ret = devm_regmap_field_bulk_alloc(dev, regmap, rptr->regs,
-					   eusb2_repeater_tune_reg_fields,
+					   rptr->regfields,
 					   F_NUM_FIELDS);
 	if (ret)
 		return ret;

-- 
2.34.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 2/2] phy: qualcomm: eusb2-repeater: Rework init to drop redundant zero-out loop

[Abel Vesa]

The device match config init table already has zero values, so rework
the container struct to hold a copy of the init table that can be
override be the DT specified values. By doing this, only the number of
vregs remain in the device match config that will be later needed, so
instead of holding the cfg after probe, store the number of vregs in the
container struct.

Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 42 ++++++++++++--------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
index 5f5862a68b73..d28106e71ce3 100644
--- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
+++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
@@ -102,7 +102,8 @@ struct eusb2_repeater {
 	struct phy *phy;
 	struct regulator_bulk_data *vregs;
 	struct reg_field *regfields;
-	const struct eusb2_repeater_cfg *cfg;
+	u32 *init_tbl;
+	int num_vregs;
 	enum phy_mode mode;
 };
 
@@ -123,9 +124,10 @@ static const struct eusb2_repeater_cfg pm8550b_eusb2_cfg = {
 	.num_vregs	= ARRAY_SIZE(pm8550b_vreg_l),
 };
 
-static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr)
+static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr,
+				     const struct eusb2_repeater_cfg *cfg)
 {
-	int num = rptr->cfg->num_vregs;
+	int num = cfg->num_vregs;
 	struct device *dev = rptr->dev;
 	int i;
 
@@ -134,7 +136,7 @@ static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr)
 		return -ENOMEM;
 
 	for (i = 0; i < num; i++)
-		rptr->vregs[i].supply = rptr->cfg->vreg_list[i];
+		rptr->vregs[i].supply = cfg->vreg_list[i];
 
 	return devm_regulator_bulk_get(dev, num, rptr->vregs);
 }
@@ -142,32 +144,19 @@ static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr)
 static int eusb2_repeater_init(struct phy *phy)
 {
 	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
-	struct reg_field *regfields = rptr->regfields;
 	struct device_node *np = rptr->dev->of_node;
-	u32 init_tbl[F_NUM_TUNE_FIELDS] = { 0 };
+	u32 *init_tbl = rptr->init_tbl;
 	u8 override;
 	u32 val;
 	int ret;
 	int i;
 
-	ret = regulator_bulk_enable(rptr->cfg->num_vregs, rptr->vregs);
+	ret = regulator_bulk_enable(rptr->num_vregs, rptr->vregs);
 	if (ret)
 		return ret;
 
 	regmap_field_update_bits(rptr->regs[F_EN_CTL1], EUSB2_RPTR_EN, EUSB2_RPTR_EN);
 
-	for (i = 0; i < F_NUM_TUNE_FIELDS; i++) {
-		if (init_tbl[i]) {
-			regmap_field_update_bits(rptr->regs[i], init_tbl[i], init_tbl[i]);
-		} else {
-			/* Write 0 if there's no value set */
-			u32 mask = GENMASK(regfields[i].msb, regfields[i].lsb);
-
-			regmap_field_update_bits(rptr->regs[i], mask, 0);
-		}
-	}
-	memcpy(init_tbl, rptr->cfg->init_tbl, sizeof(init_tbl));
-
 	if (!of_property_read_u8(np, "qcom,tune-usb2-amplitude", &override))
 		init_tbl[F_TUNE_IUSB2] = override;
 
@@ -228,7 +217,7 @@ static int eusb2_repeater_exit(struct phy *phy)
 {
 	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
 
-	return regulator_bulk_disable(rptr->cfg->num_vregs, rptr->vregs);
+	return regulator_bulk_disable(rptr->num_vregs, rptr->vregs);
 }
 
 static const struct phy_ops eusb2_repeater_ops = {
@@ -240,6 +229,7 @@ static const struct phy_ops eusb2_repeater_ops = {
 
 static int eusb2_repeater_probe(struct platform_device *pdev)
 {
+	const struct eusb2_repeater_cfg *cfg;
 	struct eusb2_repeater *rptr;
 	struct device *dev = &pdev->dev;
 	struct phy_provider *phy_provider;
@@ -255,8 +245,8 @@ static int eusb2_repeater_probe(struct platform_device *pdev)
 	rptr->dev = dev;
 	dev_set_drvdata(dev, rptr);
 
-	rptr->cfg = of_device_get_match_data(dev);
-	if (!rptr->cfg)
+	cfg = of_device_get_match_data(dev);
+	if (!cfg)
 		return -EINVAL;
 
 	regmap = dev_get_regmap(dev->parent, NULL);
@@ -269,6 +259,12 @@ static int eusb2_repeater_probe(struct platform_device *pdev)
 	if (!rptr->regfields)
 		return -ENOMEM;
 
+	rptr->init_tbl = devm_kmemdup(dev, cfg->init_tbl,
+				       sizeof(cfg->init_tbl),
+				       GFP_KERNEL);
+	if (!rptr->init_tbl)
+		return -ENOMEM;
+
 	ret = of_property_read_u32(np, "reg", &res);
 	if (ret < 0)
 		return ret;
@@ -282,7 +278,7 @@ static int eusb2_repeater_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = eusb2_repeater_init_vregs(rptr);
+	ret = eusb2_repeater_init_vregs(rptr, cfg);
 	if (ret < 0) {
 		dev_err(dev, "unable to get supplies\n");
 		return ret;

-- 
2.34.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: qualcomm: eusb2-repeater: Rework init to drop redundant zero-out loop

[Konrad Dybcio]

On 5.01.2024 11:16, Abel Vesa wrote:
> The device match config init table already has zero values, so rework
> the container struct to hold a copy of the init table that can be
> override be the DT specified values. By doing this, only the number of
> vregs remain in the device match config that will be later needed, so
> instead of holding the cfg after probe, store the number of vregs in the
> container struct.
> 
> Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---

This looks good as-is, though I think my proposal of storing the
peripheral base reg instead is still better, as it'd require less
memory (no kmemdup as the regs wouldn't be modified).

Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: qualcomm: eusb2-repeater: Rework init to drop redundant zero-out loop

[Dmitry Baryshkov]

On Fri, 5 Jan 2024 at 13:44, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 5.01.2024 11:16, Abel Vesa wrote:
> > The device match config init table already has zero values, so rework
> > the container struct to hold a copy of the init table that can be
> > override be the DT specified values. By doing this, only the number of
> > vregs remain in the device match config that will be later needed, so
> > instead of holding the cfg after probe, store the number of vregs in the
> > container struct.
> >
> > Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
>
> This looks good as-is, though I think my proposal of storing the
> peripheral base reg instead is still better, as it'd require less
> memory (no kmemdup as the regs wouldn't be modified).

I'd second this. We usually handle such cases via the base + offset
rather than patching the data. If regfields can not handle this, then
the regfield should be fixed.

-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: qualcomm: eusb2-repeater: Rework init to drop redundant zero-out loop

[Abel Vesa]

On 24-01-05 14:22:50, Dmitry Baryshkov wrote:
> On Fri, 5 Jan 2024 at 13:44, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >
> > On 5.01.2024 11:16, Abel Vesa wrote:
> > > The device match config init table already has zero values, so rework
> > > the container struct to hold a copy of the init table that can be
> > > override be the DT specified values. By doing this, only the number of
> > > vregs remain in the device match config that will be later needed, so
> > > instead of holding the cfg after probe, store the number of vregs in the
> > > container struct.
> > >
> > > Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > ---
> >
> > This looks good as-is, though I think my proposal of storing the
> > peripheral base reg instead is still better, as it'd require less
> > memory (no kmemdup as the regs wouldn't be modified).
> 
> I'd second this. We usually handle such cases via the base + offset
> rather than patching the data. If regfields can not handle this, then
> the regfield should be fixed.
> 

Sure. Will do that instead.

> -- 
> With best wishes
> Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy