[PATCH 2/4] phy: socionext: add USB3 PHY driver for UniPhier SoC

Kishon Vijay Abraham I
2018-07-13T07:15:06Z

Hi,

On Wednesday 11 July 2018 05:35 PM, Kunihiko Hayashi wrote:
> On Mon, 9 Jul 2018 20:23:19 +0900 <hayashi.kunihiko@socionext.com> wrote:
>
>> 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.
>>
>
> This way might be misunderstood.
> These HS-PHY and SS-PHY have "internal" registers, which are not memory-mapped.
>
> And to access these internal registers, we need to specify the number
> corresponding to the register.
>
> The "addr" in "uniphier_u3hsphy_param" is just the number of the register.
> The "mask" shows a bitfield of the register, that means one of PHY parameters.
> The "value" shows a parameter value to set to the bitfield.

What does each of these bitfields represent? Which PHY parameter does it
configure. Are they really PHY parameters or they also configure clocks/mux
etc? I would like the configurations to be more descriptive so that we get to
know at least what's getting configured here.

Thanks
Kishon