Hi,
On Tuesday 17 July 2018 04:57 PM, Kunihiko Hayashi wrote:
> Hi Kishon,
>
> On Fri, 13 Jul 2018 12:45:06 +0530 <kis...@ti.com> wrote:
>
>> Hi,
>>
>> On Wednesday 11 July 2018 05:35 PM, Kunihiko Hayashi wrote:
>>> On Mon, 9 Jul 2018 20:23:19 +0900 <hayashi.kunih...@socionext.com> wrote:
>>>
>>>> Hi Kishon,
>>>> Thank you for your comments.
>>>>
>>>> On Mon, 9 Jul 2018 10:49:50 +0530 <kis...@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.kunih...@socionext.com>
>>>>>> Signed-off-by: Motoya Tanigawa <tanigawa.mot...@socionext.com>
>>>>>> Signed-off-by: Masami Hiramatsu <masami.hirama...@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.kunih...@socionext.com>
>>>>>> + * Contributors:
>>>>>> + * Motoya Tanigawa <tanigawa.mot...@socionext.com>
>>>>>> + * Masami Hiramatsu <masami.hirama...@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.
>
> These bitfields represent phy parameters only, not include clocks/mux etc.
> We can express the register (bitfield) name with macros.
>
> However, only recommended values are noted for the bitfields in the specsheet,
> because there are the trimmed values that aren't set as power-on values,
> and we should use the values for stable operation.
Calibration values are fine but at least the registers and bitfields has to be
described using names.
Thanks
Kishon
