lkml.org 
[lkml]   [2018]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/4] phy: socionext: add USB3 PHY driver for UniPhier SoC
    Hi Kishon,
    Thank you for your comments.

    On Mon, 9 Jul 2018 10:49:50 +0530 <kishon@ti.com> wrote:

    > Hi,
    >
    > On Friday 29 June 2018 02:08 PM, Kunihiko Hayashi wrote:
    > > Add a driver for PHY interface built into USB3 controller
    > > implemented in UniPhier SoCs.
    > > This driver supports High-Speed PHY and Super-Speed PHY.
    > >
    > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
    > > Signed-off-by: Motoya Tanigawa <tanigawa.motoya@socionext.com>
    > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
    > > ---
    > > drivers/phy/Kconfig | 1 +
    > > drivers/phy/Makefile | 1 +
    > > drivers/phy/socionext/Kconfig | 12 +
    > > drivers/phy/socionext/Makefile | 6 +
    > > drivers/phy/socionext/phy-uniphier-usb3hs.c | 422 ++++++++++++++++++++++++++++
    > > drivers/phy/socionext/phy-uniphier-usb3ss.c | 369 ++++++++++++++++++++++++
    > > 6 files changed, 811 insertions(+)
    > > create mode 100644 drivers/phy/socionext/Kconfig
    > > create mode 100644 drivers/phy/socionext/Makefile
    > > create mode 100644 drivers/phy/socionext/phy-uniphier-usb3hs.c
    > > create mode 100644 drivers/phy/socionext/phy-uniphier-usb3ss.c

    (snip...)

    > > --- /dev/null
    > > +++ b/drivers/phy/socionext/phy-uniphier-usb3hs.c
    > > @@ -0,0 +1,422 @@
    > > +// SPDX-License-Identifier: GPL-2.0
    > > +/*
    > > + * phy-uniphier-usb3hs.c - HS-PHY driver for Socionext UniPhier USB3 controller
    > > + * Copyright 2015-2018 Socionext Inc.
    > > + * Author:
    > > + * Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
    > > + * Contributors:
    > > + * Motoya Tanigawa <tanigawa.motoya@socionext.com>
    > > + * Masami Hiramatsu <masami.hiramatsu@linaro.org>
    > > + */
    > > +
    > > +#include <linux/bitfield.h>
    > > +#include <linux/bitops.h>
    > > +#include <linux/clk.h>
    > > +#include <linux/io.h>
    > > +#include <linux/mfd/syscon.h>
    > > +#include <linux/module.h>
    > > +#include <linux/nvmem-consumer.h>
    > > +#include <linux/of.h>
    > > +#include <linux/of_platform.h>
    > > +#include <linux/phy/phy.h>
    > > +#include <linux/platform_device.h>
    > > +#include <linux/reset.h>
    > > +#include <linux/slab.h>
    > > +
    > > +#define HSPHY_CFG0 0x0
    > > +#define HSPHY_CFG0_HS_I_MASK GENMASK(31, 28)
    > > +#define HSPHY_CFG0_HSDISC_MASK GENMASK(27, 26)
    > > +#define HSPHY_CFG0_SWING_MASK GENMASK(17, 16)
    > > +#define HSPHY_CFG0_SEL_T_MASK GENMASK(15, 12)
    > > +#define HSPHY_CFG0_RTERM_MASK GENMASK(7, 6)
    > > +#define HSPHY_CFG0_TRIMMASK (HSPHY_CFG0_HS_I_MASK \
    > > + | HSPHY_CFG0_SEL_T_MASK \
    > > + | HSPHY_CFG0_RTERM_MASK)
    > > +
    > > +#define HSPHY_CFG1 0x4
    > > +#define HSPHY_CFG1_DAT_EN BIT(29)
    > > +#define HSPHY_CFG1_ADR_EN BIT(28)
    > > +#define HSPHY_CFG1_ADR_MASK GENMASK(27, 16)
    > > +#define HSPHY_CFG1_DAT_MASK GENMASK(23, 16)
    > > +
    > > +#define MAX_CLKS 3
    > > +#define MAX_RSTS 2
    > > +#define MAX_PHY_PARAMS 1
    > > +
    > > +struct uniphier_u3hsphy_param {
    > > + u32 addr;
    > > + u32 mask;
    > > + u32 val;
    > > +};
    >
    > I'd like to avoid configure the PHY this way, since it's impossible to know
    > which register is being configured.

    I see.
    In order to know which register is set, I'll add definitions for address
    and mask.
    And I think the driver might have functions for each SoC to configure
    the registers instead of uniphier_u3hsphy_param.

    Furthermore, I'll omit the register values that are already set
    at power on if the configurations are not affected.


    > > +
    > > +struct uniphier_u3hsphy_trim_param {
    > > + unsigned int rterm;
    > > + unsigned int sel_t;
    > > + unsigned int hs_i;
    > > +};
    > > +
    > > +#define trim_param_is_valid(p) ((p)->rterm || (p)->sel_t || (p)->hs_i)

    (snip...)

    > > +static int uniphier_u3hsphy_probe(struct platform_device *pdev)
    > > +{
    > > + struct device *dev = &pdev->dev;
    > > + struct uniphier_u3hsphy_priv *priv;
    > > + struct phy_provider *phy_provider;
    > > + struct resource *res;
    > > + struct phy *phy;
    > > + struct clk *clk;
    > > + struct reset_control *rst;
    > > + const char *name;
    > > + int i, ret, nc, nr;
    > > +
    > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
    > > + if (!priv)
    > > + return -ENOMEM;
    > > +
    > > + priv->dev = dev;
    > > + priv->data = of_device_get_match_data(dev);
    > > + if (WARN_ON(!priv->data ||
    > > + priv->data->nparams > MAX_PHY_PARAMS))
    > > + return -EINVAL;
    > > +
    > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    > > + priv->base = devm_ioremap_resource(dev, res);
    > > + if (IS_ERR(priv->base))
    > > + return PTR_ERR(priv->base);
    > > +
    > > + for (i = 0; i < MAX_CLKS; i++) {
    > > + name = priv->data->clock_names[i];
    > > + if (!name)
    > > + break;
    > > + clk = devm_clk_get(dev, name);
    > > + /* "phy-ext" is optional */
    > > + if (!strcmp(name, "phy-ext")) {
    > > + if (PTR_ERR(clk) == -ENOENT)
    > > + clk = NULL;
    > > + priv->clk_phy_ext = clk;
    > > + } else if (!strcmp(name, "phy")) {
    > > + priv->clk_phy = clk;
    > > + } else {
    > > + priv->clk[priv->nclks++] = clk;
    >
    > Only "link" clock will be here? Do we need an array for this?
    >
    > I think the above can be replaced with 3 separate clk_get calls instead of
    > storing clock names in uniphier_u3hsphy_soc_data.

    Indeed, in case of HS, we don't need such an array.
    I'll rewrite it.

    Thank you,

    ---
    Best Regards,
    Kunihiko Hayashi


    \
     
     \ /
      Last update: 2018-07-09 13:24    [W:2.272 / U:0.264 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site