Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets

[Stephen Boyd <sboyd@codeaurora.org>]

On 12/20, Vivek Gautam wrote:
> Qualcomm SOCs have QMP phy controller that provides support
> to a number of controller, viz. PCIe, UFS, and USB.
> Add a new driver, based on generic phy framework, for this
> phy controller.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> 
> +
> +static struct phy *qcom_qmp_phy_xlate(struct device *dev,
> +					struct of_phandle_args *args)
> +{
> +	struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
> +	int i;
> +
> +	if (WARN_ON(args->args[0] >= qphy->cfg->nlanes))
> +		return ERR_PTR(-ENODEV);
> +
> +	for (i = 0; i < qphy->cfg->nlanes; i++)
> +		/* phys[i]->index */
> +		if (i == args->args[0])
> +			return qphy->phys[i]->phy;

What's the loop for? If args->arg[0] < qphy->cfg->nlanes then we
should be able to directly index the qphy->phys array with that
number and return it.
 
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +
[...]
> +
> +/*
> + * The <s>_pipe_clksrc generated by PHY goes to the GCC that gate
> + * controls it. The <s>_pipe_clk coming out of the GCC is requested
> + * by the PHY driver for its operations.
> + * We register the <s>_pipe_clksrc here. The gcc driver takes care
> + * of assigning this <s>_pipe_clksrc as parent to <s>_pipe_clk.
> + * Below picture shows this relationship.
> + *
> + *	   +--------------+
> + *	   |  PHY block   |<<---------------------------------------+
> + *	   |              |					    |
> + *	   |   +-------+  |		      +-----+		    |
> + *   I/P---^-->|  PLL  |--^--->pipe_clksrc--->| GCC |--->pipe_clk---+
> + *   clk   |   +-------+  |		      +-----+
> + *	   +--------------+

There are mixed tabs and spaces in this diagram causing
confusion in my editor. Please make it only spaces so the picture
comes out correctly.

> + *
> + */
> +static int phy_pipe_clk_register(struct qcom_qmp_phy *qphy, int id)
> +{
> +	char clk_name[MAX_PROP_NAME];

I'm not sure MAX_PROP_NAME is the same as some max clk name but
ok. We should be able to calculate that the maximum is length of
usb3_phy_pipe_clk_src for now though?

> +	struct clk *clk;
> +
> +	memset(&clk_name, 0, sizeof(clk_name));
> +	switch (qphy->cfg->type) {
> +	case PHY_TYPE_USB3:
> +		snprintf(clk_name, MAX_PROP_NAME, "usb3_phy_pipe_clk_src");
> +		break;
> +	case PHY_TYPE_PCIE:
> +		snprintf(clk_name, MAX_PROP_NAME, "pcie_%d_pipe_clk_src", id);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* controllers using QMP phys use 125MHz pipe clock interface */
> +	clk = clk_register_fixed_rate(qphy->dev, clk_name, NULL, 0, 125000000);

I was hoping you would be able to calculate the actual output
rate by reading hardware. This is ok too though. Just please use
clk_hw_register_fixed_rate() instead. And you'll probably need
some sort of devm() usage here to handle probe failure, so I
would probably roll my own and allocate a fixed_rate clk
structure and set the rate/name directly and then call
devm_clk_hw_register().

> +
> +	return PTR_ERR_OR_ZERO(clk);
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project