From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kishon Vijay Abraham I Subject: Re: [PATCH 2/4] phy: socionext: add USB3 PHY driver for UniPhier SoC Date: Tue, 24 Jul 2018 09:31:34 +0530 Message-ID: <419fd0cd-9df7-9581-124d-f7ca876eb8af@ti.com> References: <20180711210514.BB32.4A936039@socionext.com> <52b96200-2234-9017-a317-f929eac9b266@ti.com> <20180717202757.858B.4A936039@socionext.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20180717202757.858B.4A936039@socionext.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Kunihiko Hayashi Cc: Mark Rutland , devicetree@vger.kernel.org, Masami Hiramatsu , Jassi Brar , linux-kernel@vger.kernel.org, Masahiro Yamada , Rob Herring , linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org Hi, On Tuesday 17 July 2018 04:57 PM, Kunihiko Hayashi wrote: > Hi Kishon, > > On Fri, 13 Jul 2018 12:45:06 +0530 wrote: > >> Hi, >> >> On Wednesday 11 July 2018 05:35 PM, Kunihiko Hayashi wrote: >>> On Mon, 9 Jul 2018 20:23:19 +0900 wrote: >>> >>>> Hi Kishon, >>>> Thank you for your comments. >>>> >>>> On Mon, 9 Jul 2018 10:49:50 +0530 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 >>>>>> Signed-off-by: Motoya Tanigawa >>>>>> Signed-off-by: Masami Hiramatsu >>>>>> --- >>>>>> 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 >>>>>> + * Contributors: >>>>>> + * Motoya Tanigawa >>>>>> + * Masami Hiramatsu >>>>>> + */ >>>>>> + >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> + >>>>>> +#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