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

[Elliot Berman]

On Mon, Jan 29, 2024 at 03:03:24PM +0200, 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@xxxxxxxxxx>

I forgot I've been having this on my device for past few days and USB's
been coming up consistently, so I'll count that as:

Tested-by: Elliot Berman <quic_eberman@xxxxxxxxxxx> # sm8650-qrd

> ---
> Changes in v3:
> - Reworked so that it uses base + reg-index.
> - Link to v2: https://lore.kernel.org/r/20240105-phy-qcom-eusb2-repeater-fixes-v2-0-775d98e7df05@xxxxxxxxxx
> 
> 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@xxxxxxxxxx
> ---
>  drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 166 +++++++++----------------
>  1 file changed, 62 insertions(+), 104 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> index a623f092b11f..a43e20abb10d 100644
> --- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> +++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> @@ -37,56 +37,28 @@
>  #define EUSB2_TUNE_EUSB_EQU		0x5A
>  #define EUSB2_TUNE_EUSB_HS_COMP_CUR	0x5B
>  
> -#define QCOM_EUSB2_REPEATER_INIT_CFG(r, v)	\
> -	{					\
> -		.reg = r,			\
> -		.val = v,			\
> -	}
> -
> -enum reg_fields {
> -	F_TUNE_EUSB_HS_COMP_CUR,
> -	F_TUNE_EUSB_EQU,
> -	F_TUNE_EUSB_SLEW,
> -	F_TUNE_USB2_HS_COMP_CUR,
> -	F_TUNE_USB2_PREEM,
> -	F_TUNE_USB2_EQU,
> -	F_TUNE_USB2_SLEW,
> -	F_TUNE_SQUELCH_U,
> -	F_TUNE_HSDISC,
> -	F_TUNE_RES_FSDIF,
> -	F_TUNE_IUSB2,
> -	F_TUNE_USB2_CROSSOVER,
> -	F_NUM_TUNE_FIELDS,
> -
> -	F_FORCE_VAL_5 = F_NUM_TUNE_FIELDS,
> -	F_FORCE_EN_5,
> -
> -	F_EN_CTL1,
> -
> -	F_RPTR_STATUS,
> -	F_NUM_FIELDS,
> -};
> -
> -static struct reg_field eusb2_repeater_tune_reg_fields[F_NUM_FIELDS] = {
> -	[F_TUNE_EUSB_HS_COMP_CUR] = REG_FIELD(EUSB2_TUNE_EUSB_HS_COMP_CUR, 0, 1),
> -	[F_TUNE_EUSB_EQU] = REG_FIELD(EUSB2_TUNE_EUSB_EQU, 0, 1),
> -	[F_TUNE_EUSB_SLEW] = REG_FIELD(EUSB2_TUNE_EUSB_SLEW, 0, 1),
> -	[F_TUNE_USB2_HS_COMP_CUR] = REG_FIELD(EUSB2_TUNE_USB2_HS_COMP_CUR, 0, 1),
> -	[F_TUNE_USB2_PREEM] = REG_FIELD(EUSB2_TUNE_USB2_PREEM, 0, 2),
> -	[F_TUNE_USB2_EQU] = REG_FIELD(EUSB2_TUNE_USB2_EQU, 0, 1),
> -	[F_TUNE_USB2_SLEW] = REG_FIELD(EUSB2_TUNE_USB2_SLEW, 0, 1),
> -	[F_TUNE_SQUELCH_U] = REG_FIELD(EUSB2_TUNE_SQUELCH_U, 0, 2),
> -	[F_TUNE_HSDISC] = REG_FIELD(EUSB2_TUNE_HSDISC, 0, 2),
> -	[F_TUNE_RES_FSDIF] = REG_FIELD(EUSB2_TUNE_RES_FSDIF, 0, 2),
> -	[F_TUNE_IUSB2] = REG_FIELD(EUSB2_TUNE_IUSB2, 0, 3),
> -	[F_TUNE_USB2_CROSSOVER] = REG_FIELD(EUSB2_TUNE_USB2_CROSSOVER, 0, 2),
> -
> -	[F_FORCE_VAL_5] = REG_FIELD(EUSB2_FORCE_VAL_5, 0, 7),
> -	[F_FORCE_EN_5] = REG_FIELD(EUSB2_FORCE_EN_5, 0, 7),
> -
> -	[F_EN_CTL1] = REG_FIELD(EUSB2_EN_CTL1, 0, 7),
> -
> -	[F_RPTR_STATUS] = REG_FIELD(EUSB2_RPTR_STATUS, 0, 7),
> +enum eusb2_reg_layout {
> +	TUNE_EUSB_HS_COMP_CUR,
> +	TUNE_EUSB_EQU,
> +	TUNE_EUSB_SLEW,
> +	TUNE_USB2_HS_COMP_CUR,
> +	TUNE_USB2_PREEM,
> +	TUNE_USB2_EQU,
> +	TUNE_USB2_SLEW,
> +	TUNE_SQUELCH_U,
> +	TUNE_HSDISC,
> +	TUNE_RES_FSDIF,
> +	TUNE_IUSB2,
> +	TUNE_USB2_CROSSOVER,
> +	NUM_TUNE_FIELDS,
> +
> +	FORCE_VAL_5 = NUM_TUNE_FIELDS,
> +	FORCE_EN_5,
> +
> +	EN_CTL1,
> +
> +	RPTR_STATUS,
> +	LAYOUT_SIZE,
>  };
>  
>  struct eusb2_repeater_cfg {
> @@ -98,10 +70,11 @@ struct eusb2_repeater_cfg {
>  
>  struct eusb2_repeater {
>  	struct device *dev;
> -	struct regmap_field *regs[F_NUM_FIELDS];
> +	struct regmap *regmap;
>  	struct phy *phy;
>  	struct regulator_bulk_data *vregs;
>  	const struct eusb2_repeater_cfg *cfg;
> +	u32 base;
>  	enum phy_mode mode;
>  };
>  
> @@ -109,10 +82,10 @@ static const char * const pm8550b_vreg_l[] = {
>  	"vdd18", "vdd3",
>  };
>  
> -static const u32 pm8550b_init_tbl[F_NUM_TUNE_FIELDS] = {
> -	[F_TUNE_IUSB2] = 0x8,
> -	[F_TUNE_SQUELCH_U] = 0x3,
> -	[F_TUNE_USB2_PREEM] = 0x5,
> +static const u32 pm8550b_init_tbl[NUM_TUNE_FIELDS] = {
> +	[TUNE_IUSB2] = 0x8,
> +	[TUNE_SQUELCH_U] = 0x3,
> +	[TUNE_USB2_PREEM] = 0x5,
>  };
>  
>  static const struct eusb2_repeater_cfg pm8550b_eusb2_cfg = {
> @@ -140,47 +113,42 @@ 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 device_node *np = rptr->dev->of_node;
> -	u32 init_tbl[F_NUM_TUNE_FIELDS] = { 0 };
> -	u8 override;
> +	struct regmap *regmap = rptr->regmap;
> +	const u32 *init_tbl = rptr->cfg->init_tbl;
> +	u8 tune_usb2_preem = init_tbl[TUNE_USB2_PREEM];
> +	u8 tune_hsdisc = init_tbl[TUNE_HSDISC];
> +	u8 tune_iusb2 = init_tbl[TUNE_IUSB2];
> +	u32 base = rptr->base;
>  	u32 val;
>  	int ret;
> -	int i;
> +
> +	of_property_read_u8(np, "qcom,tune-usb2-amplitude", &tune_iusb2);
> +	of_property_read_u8(np, "qcom,tune-usb2-disc-thres", &tune_hsdisc);
> +	of_property_read_u8(np, "qcom,tune-usb2-preem", &tune_usb2_preem);
>  
>  	ret = regulator_bulk_enable(rptr->cfg->num_vregs, rptr->vregs);
>  	if (ret)
>  		return ret;
>  
> -	regmap_field_update_bits(rptr->regs[F_EN_CTL1], EUSB2_RPTR_EN, EUSB2_RPTR_EN);
> +	regmap_write(regmap, base + EUSB2_EN_CTL1, 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));
> +	regmap_write(regmap, base + EUSB2_TUNE_EUSB_HS_COMP_CUR, init_tbl[TUNE_EUSB_HS_COMP_CUR]);
> +	regmap_write(regmap, base + EUSB2_TUNE_EUSB_EQU, init_tbl[TUNE_EUSB_EQU]);
> +	regmap_write(regmap, base + EUSB2_TUNE_EUSB_SLEW, init_tbl[TUNE_EUSB_SLEW]);
> +	regmap_write(regmap, base + EUSB2_TUNE_USB2_HS_COMP_CUR, init_tbl[TUNE_USB2_HS_COMP_CUR]);
> +	regmap_write(regmap, base + EUSB2_TUNE_USB2_EQU, init_tbl[TUNE_USB2_EQU]);
> +	regmap_write(regmap, base + EUSB2_TUNE_USB2_SLEW, init_tbl[TUNE_USB2_SLEW]);
> +	regmap_write(regmap, base + EUSB2_TUNE_SQUELCH_U, init_tbl[TUNE_SQUELCH_U]);
> +	regmap_write(regmap, base + EUSB2_TUNE_RES_FSDIF, init_tbl[TUNE_RES_FSDIF]);
> +	regmap_write(regmap, base + EUSB2_TUNE_USB2_CROSSOVER, init_tbl[TUNE_USB2_CROSSOVER]);
>  
> -	if (!of_property_read_u8(np, "qcom,tune-usb2-amplitude", &override))
> -		init_tbl[F_TUNE_IUSB2] = override;
> +	regmap_write(regmap, base + EUSB2_TUNE_USB2_PREEM, tune_usb2_preem);
> +	regmap_write(regmap, base + EUSB2_TUNE_HSDISC, tune_hsdisc);
> +	regmap_write(regmap, base + EUSB2_TUNE_IUSB2, tune_iusb2);
>  
> -	if (!of_property_read_u8(np, "qcom,tune-usb2-disc-thres", &override))
> -		init_tbl[F_TUNE_HSDISC] = override;
> -
> -	if (!of_property_read_u8(np, "qcom,tune-usb2-preem", &override))
> -		init_tbl[F_TUNE_USB2_PREEM] = override;
> -
> -	for (i = 0; i < F_NUM_TUNE_FIELDS; i++)
> -		regmap_field_update_bits(rptr->regs[i], init_tbl[i], init_tbl[i]);
> -
> -	ret = regmap_field_read_poll_timeout(rptr->regs[F_RPTR_STATUS],
> -					     val, val & RPTR_OK, 10, 5);
> +	ret = regmap_read_poll_timeout(regmap, base + EUSB2_RPTR_STATUS, val, val & RPTR_OK, 10, 5);
>  	if (ret)
>  		dev_err(rptr->dev, "initialization timed-out\n");
>  
> @@ -191,6 +159,8 @@ static int eusb2_repeater_set_mode(struct phy *phy,
>  				   enum phy_mode mode, int submode)
>  {
>  	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
> +	struct regmap *regmap = rptr->regmap;
> +	u32 base = rptr->base;
>  
>  	switch (mode) {
>  	case PHY_MODE_USB_HOST:
> @@ -199,10 +169,8 @@ static int eusb2_repeater_set_mode(struct phy *phy,
>  		 * per eUSB 1.2 Spec. Below implement software workaround until
>  		 * PHY and controller is fixing seen observation.
>  		 */
> -		regmap_field_update_bits(rptr->regs[F_FORCE_EN_5],
> -					 F_CLK_19P2M_EN, F_CLK_19P2M_EN);
> -		regmap_field_update_bits(rptr->regs[F_FORCE_VAL_5],
> -					 V_CLK_19P2M_EN, V_CLK_19P2M_EN);
> +		regmap_write(regmap, base + EUSB2_FORCE_EN_5, F_CLK_19P2M_EN);
> +		regmap_write(regmap, base + EUSB2_FORCE_VAL_5, V_CLK_19P2M_EN);
>  		break;
>  	case PHY_MODE_USB_DEVICE:
>  		/*
> @@ -211,10 +179,8 @@ static int eusb2_repeater_set_mode(struct phy *phy,
>  		 * repeater doesn't clear previous value due to shared
>  		 * regulators (say host <-> device mode switch).
>  		 */
> -		regmap_field_update_bits(rptr->regs[F_FORCE_EN_5],
> -					 F_CLK_19P2M_EN, 0);
> -		regmap_field_update_bits(rptr->regs[F_FORCE_VAL_5],
> -					 V_CLK_19P2M_EN, 0);
> +		regmap_write(regmap, base + EUSB2_FORCE_EN_5, 0);
> +		regmap_write(regmap, base + EUSB2_FORCE_VAL_5, 0);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -243,9 +209,8 @@ static int eusb2_repeater_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct phy_provider *phy_provider;
>  	struct device_node *np = dev->of_node;
> -	struct regmap *regmap;
> -	int i, ret;
>  	u32 res;
> +	int ret;
>  
>  	rptr = devm_kzalloc(dev, sizeof(*rptr), GFP_KERNEL);
>  	if (!rptr)
> @@ -258,22 +223,15 @@ static int eusb2_repeater_probe(struct platform_device *pdev)
>  	if (!rptr->cfg)
>  		return -EINVAL;
>  
> -	regmap = dev_get_regmap(dev->parent, NULL);
> -	if (!regmap)
> +	rptr->regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!rptr->regmap)
>  		return -ENODEV;
>  
>  	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;
> -
> -	ret = devm_regmap_field_bulk_alloc(dev, regmap, rptr->regs,
> -					   eusb2_repeater_tune_reg_fields,
> -					   F_NUM_FIELDS);
> -	if (ret)
> -		return ret;
> +	rptr->base = res;
>  
>  	ret = eusb2_repeater_init_vregs(rptr);
>  	if (ret < 0) {
> 
> ---
> base-commit: 01af33cc9894b4489fb68fa35c40e9fe85df63dc
> change-id: 20240104-phy-qcom-eusb2-repeater-fixes-c9201113032c
> 
> Best regards,
> -- 
> Abel Vesa <abel.vesa@xxxxxxxxxx>
>