On Mon, 9 Jul 2018 20:23:19 +0900 [off-list ref] wrote:

Hi Kishon,
Thank you for your comments.

On Mon, 9 Jul 2018 10:49:50 +0530 [off-list ref] wrote:

quoted

Hi,

On Friday 29 June 2018 02:08 PM, Kunihiko Hayashi wrote:

quoted

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 <redacted>
Signed-off-by: Masami Hiramatsu <redacted>
---
 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...)

quoted

quoted

--- /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.

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.

Then, I think it's sufficient to define a macro with a meaningful label for
each bitfield, and PHY parameters are expressed like that.

  { 10, FOO_BAR_MODE, 0x2 }, /* set 2 to FOO_BAR_MODE in register 10 */

In this case, we need to shift "value" according to "mask".

Thank you,

---
Best Regards,
Kunihiko Hayashi