lkml.org 
[lkml]   [2016]   [Dec]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets
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

\
 
 \ /
  Last update: 2016-12-29 00:17    [W:0.290 / U:0.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site