Re: [PATCH v2 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters

[Pavan Kondeti]

Hi Vinod,

On Wed, Apr 13, 2022 at 03:12:09PM +0530, Vinod Koul wrote:
> On 03-03-22, 11:43, Sandeep Maheswaram wrote:
> > Added support for overriding x0,x1,x2,x3 params for SNPS PHY by reading
> > values from device tree.
> > 
> > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@xxxxxxxxxxx>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 192 ++++++++++++++++++++++++++
> >  1 file changed, 192 insertions(+)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > index 7e61202..b5aa06d 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > @@ -51,6 +51,48 @@
> >  #define USB2_SUSPEND_N				BIT(2)
> >  #define USB2_SUSPEND_N_SEL			BIT(3)
> >  
> > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0	(0x6c)
> > +
> > +/*USB_PHY_HS_PHY_OVERRIDE_X0 register bits*/
> 
> space after /* and before */ (checkpatch.pl --strict would warn you)
> Pls fix it everywhere
> 
> > +#define HS_DISCONNECT_MASK			GENMASK(2, 0)
> > +#define HS_DISCONNECT_SHIFT			0x0
> > +
> > +#define SQUELCH_DETECTOR_MASK			GENMASK(7, 5)
> > +#define SQUELCH_DETECTOR_SHIFT			0x5
> > +
> > +
> > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1	(0x70)
> > +
> > +/*USB_PHY_HS_PHY_OVERRIDE_X1 register bits*/
> > +#define HS_AMPLITUDE_MASK			GENMASK(3, 0)
> > +#define HS_AMPLITUDE_SHIFT			0x0
> > +
> > +#define PREEMPHASIS_DURATION_MASK		BIT(5)
> > +#define PREEMPHASIS_DURATION_SHIFT		0x5
> > +
> > +#define PREEMPHASIS_AMPLITUDE_MASK		GENMASK(7, 6)
> > +#define PREEMPHASIS_AMPLITUDE_SHIFT		0x6
> > +
> > +
> > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2	(0x74)
> > +
> > +/*USB_PHY_HS_PHY_OVERRIDE_X2 register bits*/
> > +#define HS_RISE_FALL_MASK			GENMASK(1, 0)
> > +#define HS_RISE_FALL_SHIFT			0x0
> > +
> > +#define HS_CROSSOVER_VOLTAGE_MASK		GENMASK(3, 2)
> > +#define HS_CROSSOVER_VOLTAGE_SHIFT		0x2
> > +
> > +#define HS_OUTPUT_IMPEDANCE_MASK		GENMASK(5, 4)
> > +#define HS_OUTPUT_IMPEDANCE_SHIFT		0x4
> > +
> > +
> > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3	(0x78)
> > +
> > +/*USB_PHY_HS_PHY_OVERRIDE_X3 register bits*/
> > +#define LS_FS_OUTPUT_IMPEDANCE_MASK		GENMASK(3, 0)
> > +#define LS_FS_OUTPUT_IMPEDANCE_SHIFT		0x0
> > +
> >  #define USB2_PHY_USB_PHY_CFG0			(0x94)
> >  #define UTMI_PHY_DATAPATH_CTRL_OVERRIDE_EN	BIT(0)
> >  #define UTMI_PHY_CMN_CTRL_OVERRIDE_EN		BIT(1)
> > @@ -65,6 +107,43 @@ static const char * const qcom_snps_hsphy_vreg_names[] = {
> >  
> >  #define SNPS_HS_NUM_VREGS		ARRAY_SIZE(qcom_snps_hsphy_vreg_names)
> >  
> > +/* struct override_param - structure holding snps phy overriding param
> > + * set override true if the  device tree property exists and read and assign
> > + * to value
> > + */
> > +struct override_param {
> > +	bool override;
> > +	u8 value;
> > +};
> > +
> > +/*struct override_params - structure holding snps phy overriding params
> > + * @hs_disconnect: disconnect threshold
> > + * @squelch_detector: threshold to detect valid high-speed data
> > + * @hs_amplitude: high-speed DC level voltage
> > + * @preemphasis_duration: duration for which the HS pre-emphasis current
> > + *  is sourced onto DP<#> or DM<#>
> > + * @preemphasis_amplitude: current sourced to DP<#> and DM<#> after
> > + *  a J-to-K or K-to-J transition.
> > + * @hs_rise_fall_time: rise/fall times of the high-speed waveform
> > + * @hs_crossover_voltage: voltage at which the DP<#> and DM<#>
> > + *  signals cross while transmitting in HS mode
> > + * @hs_output_impedance: driver source impedance to compensate for added series
> > + *  resistance on the USB
> > + * @ls_fs_output_impedance: low and full-speed single-ended source
> > + *  impedance while driving high
> > + */
> > +struct override_params {
> > +	struct override_param hs_disconnect;
> > +	struct override_param squelch_detector;
> > +	struct override_param hs_amplitude;
> > +	struct override_param preemphasis_duration;
> > +	struct override_param preemphasis_amplitude;
> > +	struct override_param hs_rise_fall_time;
> > +	struct override_param hs_crossover_voltage;
> > +	struct override_param hs_output_impedance;
> > +	struct override_param ls_fs_output_impedance;
> > +};
> > +
> >  /**
> >   * struct qcom_snps_hsphy - snps hs phy attributes
> >   *
> > @@ -87,6 +166,7 @@ struct qcom_snps_hsphy {
> >  	struct clk *ref_clk;
> >  	struct reset_control *phy_reset;
> >  	struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS];
> > +	struct override_params overrides;
> >  
> >  	bool phy_initialized;
> >  	enum phy_mode mode;
> > @@ -175,6 +255,7 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode,
> >  static int qcom_snps_hsphy_init(struct phy *phy)
> >  {
> >  	struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
> > +	struct override_params *or = &hsphy->overrides;
> >  	int ret;
> >  
> >  	dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__);
> > @@ -222,6 +303,60 @@ static int qcom_snps_hsphy_init(struct phy *phy)
> >  	qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL1,
> >  					VBUSVLDEXT0, VBUSVLDEXT0);
> >  
> > +	if (or->hs_disconnect.override)
> > +		qcom_snps_hsphy_write_mask(hsphy->base,
> > +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
> > +			HS_DISCONNECT_MASK,
> > +			or->hs_disconnect.value << HS_DISCONNECT_SHIFT);
> > +
> > +	if (or->squelch_detector.override)
> > +		qcom_snps_hsphy_write_mask(hsphy->base,
> > +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
> > +			SQUELCH_DETECTOR_MASK,
> > +			or->squelch_detector.value << SQUELCH_DETECTOR_SHIFT);
> > +
> > +	if (or->hs_amplitude.override)
> > +		qcom_snps_hsphy_write_mask(hsphy->base,
> > +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
> > +			HS_AMPLITUDE_MASK,
> > +			or->hs_amplitude.value << HS_AMPLITUDE_SHIFT);
> > +
> > +	if (or->preemphasis_duration.override)
> > +		qcom_snps_hsphy_write_mask(hsphy->base,
> > +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
> > +			PREEMPHASIS_DURATION_MASK,
> > +			or->preemphasis_duration.value << PREEMPHASIS_DURATION_SHIFT);
> > +
> > +	if (or->preemphasis_amplitude.override)
> > +		qcom_snps_hsphy_write_mask(hsphy->base,
> > +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
> > +			PREEMPHASIS_AMPLITUDE_MASK,
> > +			or->preemphasis_amplitude.value << PREEMPHASIS_AMPLITUDE_SHIFT);
> > +
> > +	if (or->hs_rise_fall_time.override)
> > +		qcom_snps_hsphy_write_mask(hsphy->base,
> > +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
> > +			HS_RISE_FALL_MASK,
> > +			or->hs_rise_fall_time.value << HS_RISE_FALL_SHIFT);
> > +
> > +	if (or->hs_crossover_voltage.override)
> > +		qcom_snps_hsphy_write_mask(hsphy->base,
> > +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
> > +			HS_CROSSOVER_VOLTAGE_MASK,
> > +			or->hs_crossover_voltage.value << HS_CROSSOVER_VOLTAGE_SHIFT);
> > +
> > +	if (or->hs_output_impedance.override)
> > +		qcom_snps_hsphy_write_mask(hsphy->base,
> > +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
> > +			HS_OUTPUT_IMPEDANCE_MASK,
> > +			or->hs_output_impedance.value << HS_OUTPUT_IMPEDANCE_SHIFT);
> > +
> > +	if (or->ls_fs_output_impedance.override)
> > +		qcom_snps_hsphy_write_mask(hsphy->base,
> > +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3,
> > +			LS_FS_OUTPUT_IMPEDANCE_MASK,
> > +			or->ls_fs_output_impedance.value << LS_FS_OUTPUT_IMPEDANCE_SHIFT);
> > +
> >  	qcom_snps_hsphy_write_mask(hsphy->base,
> >  					USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON2,
> >  					VREGBYPASS, VREGBYPASS);
> > @@ -292,12 +427,15 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
> >  	struct qcom_snps_hsphy *hsphy;
> >  	struct phy_provider *phy_provider;
> >  	struct phy *generic_phy;
> > +	struct override_params *or;
> >  	int ret, i;
> >  	int num;
> > +	u32 value;
> >  
> >  	hsphy = devm_kzalloc(dev, sizeof(*hsphy), GFP_KERNEL);
> >  	if (!hsphy)
> >  		return -ENOMEM;
> > +	or = &hsphy->overrides;
> >  
> >  	hsphy->base = devm_platform_ioremap_resource(pdev, 0);
> >  	if (IS_ERR(hsphy->base))
> > @@ -329,6 +467,60 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >  
> > +	if (!of_property_read_u32(dev->of_node, "qcom,hs-disconnect",
> > +				  &value)) {
> > +		or->hs_disconnect.value = (u8)value;
> > +		or->hs_disconnect.override = true;
> > +	}
> > +
> > +	if (!of_property_read_u32(dev->of_node, "qcom,squelch-detector",
> > +				  &value)) {
> > +		or->squelch_detector.value = (u8)value;
> > +		or->squelch_detector.override = true;
> > +	}
> > +
> > +	if (!of_property_read_u32(dev->of_node, "qcom,hs-amplitude",
> > +				  &value)) {
> > +		or->hs_amplitude.value = (u8)value;
> > +		or->hs_amplitude.override = true;
> > +	}
> > +
> > +	if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-duration",
> > +				  &value)) {
> > +		or->preemphasis_duration.value = (u8)value;
> > +		or->preemphasis_duration.override = true;
> > +	}
> > +
> > +	if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-amplitude",
> > +				  &value)) {
> > +		or->preemphasis_amplitude.value = (u8)value;
> > +		or->preemphasis_amplitude.override = true;
> > +	}
> > +
> > +	if (!of_property_read_u32(dev->of_node, "qcom,hs-rise-fall-time",
> > +				  &value)) {
> > +		or->hs_rise_fall_time.value = (u8)value;
> > +		or->hs_rise_fall_time.override = true;
> > +	}
> > +
> > +	if (!of_property_read_u32(dev->of_node, "qcom,hs-crossover-voltage",
> > +				  &value)) {
> > +		or->hs_crossover_voltage.value = (u8)value;
> > +		or->hs_crossover_voltage.override = true;
> > +	}
> > +
> > +	if (!of_property_read_u32(dev->of_node, "qcom,hs-output-impedance",
> > +				  &value)) {
> > +		or->hs_output_impedance.value = (u8)value;
> > +		or->hs_output_impedance.override = true;
> > +	}
> > +
> > +	if (!of_property_read_u32(dev->of_node, "qcom,ls-fs-output-impedance",
> > +				  &value)) {
> > +		or->ls_fs_output_impedance.value = (u8)value;
> > +		or->ls_fs_output_impedance.override = true;
> > +	}
> 
> Are all these values board specific or IP specific? Can we add these
> values as tables in driver? 

These are IP specific. The tuning is board specific. The plan is to add
tables in the driver which lookup the values passed by device tree and
program the override registers according. We are currently working on
this.

previous discussion:
https://lore.kernel.org/linux-usb/1a43277a-94b9-4f95-314a-876291227982@xxxxxxxxxxxxx/

Thanks,
Pavan
> -- 
> ~Vinod