[PATCH 0/8] Add support for MediaTek xHCI host controller

[PATCH 0/8] Add support for MediaTek xHCI host controller

[Chunfeng Yun]

The series of patches are used to support xHCI host controller on
MediaTek SoCs which has a glue layer IPPC (IP Port Control), and
add USB function on T-PHY including T-PHY V1 and V2;
Finally add USB related nodes for MT7629 platform.

Chunfeng Yun (8):
  phy: phy-mtk-tphy: add support USB phys
  phy: phy-mtk-tphy: add support new version
  phy: phy-mtk-tphy: add a new reference clock
  dm: core: Add function to get child count of ofnode
  xhci: mediatek: Add support for MTK xHCI host controller
  arm: dts: mt7629: add support usb related nodes
  dt-bindings: phy-mtk-tphy: add properties of address mapping and
    clocks
  dt-bindings: usb: mtk-xhci: Add binding for MediaTek xHCI host
    controller

 arch/arm/dts/mt7629-rfb.dts                   |   8 +
 arch/arm/dts/mt7629.dtsi                      |  41 ++
 doc/device-tree-bindings/phy/phy-mtk-tphy.txt |  78 ++-
 .../usb/mediatek,mtk-xhci.txt                 |  40 ++
 drivers/phy/phy-mtk-tphy.c                    | 313 ++++++++++-
 drivers/usb/host/Kconfig                      |   6 +
 drivers/usb/host/Makefile                     |   1 +
 drivers/usb/host/xhci-mtk.c                   | 508 ++++++++++++++++++
 include/dm/ofnode.h                           |  17 +
 9 files changed, 987 insertions(+), 25 deletions(-)
 create mode 100644 doc/device-tree-bindings/usb/mediatek,mtk-xhci.txt
 create mode 100644 drivers/usb/host/xhci-mtk.c

-- 
2.25.1

[PATCH 1/8] phy: phy-mtk-tphy: add support USB phys

[Chunfeng Yun]

Support USB2 and USB3 PHY with shared banks when support multi-phys

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/phy/phy-mtk-tphy.c | 224 +++++++++++++++++++++++++++++++++++--
 1 file changed, 217 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/phy-mtk-tphy.c b/drivers/phy/phy-mtk-tphy.c
index bd089b7a43..fc74098c84 100644
--- a/drivers/phy/phy-mtk-tphy.c
+++ b/drivers/phy/phy-mtk-tphy.c
@@ -20,11 +20,66 @@
 /* version V1 sub-banks offset base address */
 /* banks shared by multiple phys */
 #define SSUSB_SIFSLV_V1_SPLLC		0x000	/* shared by u3 phys */
+#define SSUSB_SIFSLV_V1_U2FREQ		0x100	/* shared by u2 phys */
 #define SSUSB_SIFSLV_V1_CHIP		0x300	/* shared by u3 phys */
+/* u2 phy bank */
+#define SSUSB_SIFSLV_V1_U2PHY_COM	0x000
 /* u3/pcie/sata phy banks */
 #define SSUSB_SIFSLV_V1_U3PHYD		0x000
 #define SSUSB_SIFSLV_V1_U3PHYA		0x200
 
+#define U3P_USBPHYACR0			0x000
+#define PA0_RG_U2PLL_FORCE_ON		BIT(15)
+#define PA0_RG_USB20_INTR_EN		BIT(5)
+
+#define U3P_USBPHYACR5			0x014
+#define PA5_RG_U2_HSTX_SRCAL_EN		BIT(15)
+#define PA5_RG_U2_HSTX_SRCTRL		GENMASK(14, 12)
+#define PA5_RG_U2_HSTX_SRCTRL_VAL(x)	((0x7 & (x)) << 12)
+#define PA5_RG_U2_HS_100U_U3_EN		BIT(11)
+
+#define U3P_USBPHYACR6			0x018
+#define PA6_RG_U2_BC11_SW_EN		BIT(23)
+#define PA6_RG_U2_OTG_VBUSCMP_EN	BIT(20)
+#define PA6_RG_U2_SQTH			GENMASK(3, 0)
+#define PA6_RG_U2_SQTH_VAL(x)		(0xf & (x))
+
+#define U3P_U2PHYACR4			0x020
+#define P2C_RG_USB20_GPIO_CTL		BIT(9)
+#define P2C_USB20_GPIO_MODE		BIT(8)
+#define P2C_U2_GPIO_CTR_MSK	\
+		(P2C_RG_USB20_GPIO_CTL | P2C_USB20_GPIO_MODE)
+
+#define U3P_U2PHYDTM0			0x068
+#define P2C_FORCE_UART_EN		BIT(26)
+#define P2C_FORCE_DATAIN		BIT(23)
+#define P2C_FORCE_DM_PULLDOWN		BIT(21)
+#define P2C_FORCE_DP_PULLDOWN		BIT(20)
+#define P2C_FORCE_XCVRSEL		BIT(19)
+#define P2C_FORCE_SUSPENDM		BIT(18)
+#define P2C_FORCE_TERMSEL		BIT(17)
+#define P2C_RG_DATAIN			GENMASK(13, 10)
+#define P2C_RG_DATAIN_VAL(x)		((0xf & (x)) << 10)
+#define P2C_RG_DMPULLDOWN		BIT(7)
+#define P2C_RG_DPPULLDOWN		BIT(6)
+#define P2C_RG_XCVRSEL			GENMASK(5, 4)
+#define P2C_RG_XCVRSEL_VAL(x)		((0x3 & (x)) << 4)
+#define P2C_RG_SUSPENDM			BIT(3)
+#define P2C_RG_TERMSEL			BIT(2)
+#define P2C_DTM0_PART_MASK	\
+		(P2C_FORCE_DATAIN | P2C_FORCE_DM_PULLDOWN | \
+		P2C_FORCE_DP_PULLDOWN | P2C_FORCE_XCVRSEL | \
+		P2C_FORCE_TERMSEL | P2C_RG_DMPULLDOWN | \
+		P2C_RG_DPPULLDOWN | P2C_RG_TERMSEL)
+
+#define U3P_U2PHYDTM1			0x06C
+#define P2C_RG_UART_EN			BIT(16)
+#define P2C_FORCE_IDDIG			BIT(9)
+#define P2C_RG_VBUSVALID		BIT(5)
+#define P2C_RG_SESSEND			BIT(4)
+#define P2C_RG_AVALID			BIT(2)
+#define P2C_RG_IDDIG			BIT(1)
+
 #define U3P_U3_CHIP_GPIO_CTLD		0x0c
 #define P3C_REG_IP_SW_RST		BIT(31)
 #define P3C_MCU_BUS_CK_GATE_EN		BIT(30)
@@ -42,6 +97,14 @@
 #define P3A_RG_CLKDRV_AMP		GENMASK(31, 29)
 #define P3A_RG_CLKDRV_AMP_VAL(x)	((0x7 & (x)) << 29)
 
+#define U3P_U3_PHYA_REG6		0x018
+#define P3A_RG_TX_EIDLE_CM		GENMASK(31, 28)
+#define P3A_RG_TX_EIDLE_CM_VAL(x)	((0xf & (x)) << 28)
+
+#define U3P_U3_PHYA_REG9		0x024
+#define P3A_RG_RX_DAC_MUX		GENMASK(5, 1)
+#define P3A_RG_RX_DAC_MUX_VAL(x)	((0x1f & (x)) << 1)
+
 #define U3P_U3_PHYA_DA_REG0		0x100
 #define P3A_RG_XTAL_EXT_PE2H		GENMASK(17, 16)
 #define P3A_RG_XTAL_EXT_PE2H_VAL(x)	((0x3 & (x)) << 16)
@@ -77,6 +140,16 @@
 #define P3A_RG_PLL_DELTA_PE2H		GENMASK(15, 0)
 #define P3A_RG_PLL_DELTA_PE2H_VAL(x)	(0xffff & (x))
 
+#define U3P_U3_PHYD_LFPS1		0x00c
+#define P3D_RG_FWAKE_TH			GENMASK(21, 16)
+#define P3D_RG_FWAKE_TH_VAL(x)		((0x3f & (x)) << 16)
+
+#define U3P_U3_PHYD_CDR1		0x05c
+#define P3D_RG_CDR_BIR_LTD1		GENMASK(28, 24)
+#define P3D_RG_CDR_BIR_LTD1_VAL(x)	((0x1f & (x)) << 24)
+#define P3D_RG_CDR_BIR_LTD0		GENMASK(12, 8)
+#define P3D_RG_CDR_BIR_LTD0_VAL(x)	((0x1f & (x)) << 8)
+
 #define U3P_U3_PHYD_RXDET1		0x128
 #define P3D_RG_RXDET_STB2_SET		GENMASK(17, 9)
 #define P3D_RG_RXDET_STB2_SET_VAL(x)	((0x1ff & (x)) << 9)
@@ -85,6 +158,16 @@
 #define P3D_RG_RXDET_STB2_SET_P3	GENMASK(8, 0)
 #define P3D_RG_RXDET_STB2_SET_P3_VAL(x)	(0x1ff & (x))
 
+#define U3P_SPLLC_XTALCTL3		0x018
+#define XC3_RG_U3_XTAL_RX_PWD		BIT(9)
+#define XC3_RG_U3_FRC_XTAL_RX_PWD	BIT(8)
+
+struct u2phy_banks {
+	void __iomem *misc;
+	void __iomem *fmreg;
+	void __iomem *com;
+};
+
 struct u3phy_banks {
 	void __iomem *spllc;
 	void __iomem *chip;
@@ -95,21 +178,127 @@ struct u3phy_banks {
 struct mtk_phy_instance {
 	void __iomem *port_base;
 	const struct device_node *np;
-
-	struct u3phy_banks u3_banks;
+	union {
+		struct u2phy_banks u2_banks;
+		struct u3phy_banks u3_banks;
+	};
 
 	/* reference clock of anolog phy */
 	struct clk ref_clk;
 	u32 index;
-	u8 type;
+	u32 type;
 };
 
 struct mtk_tphy {
+	struct udevice *dev;
 	void __iomem *sif_base;
 	struct mtk_phy_instance **phys;
 	int nphys;
 };
 
+static void u2_phy_instance_init(struct mtk_tphy *tphy,
+				 struct mtk_phy_instance *instance)
+{
+	struct u2phy_banks *u2_banks = &instance->u2_banks;
+
+	/* switch to USB function, and enable usb pll */
+	clrsetbits_le32(u2_banks->com + U3P_U2PHYDTM0,
+			P2C_FORCE_UART_EN | P2C_FORCE_SUSPENDM,
+			P2C_RG_XCVRSEL_VAL(1) | P2C_RG_DATAIN_VAL(0));
+
+	clrbits_le32(u2_banks->com + U3P_U2PHYDTM1, P2C_RG_UART_EN);
+	setbits_le32(u2_banks->com + U3P_USBPHYACR0, PA0_RG_USB20_INTR_EN);
+
+	/* disable switch 100uA current to SSUSB */
+	clrbits_le32(u2_banks->com + U3P_USBPHYACR5, PA5_RG_U2_HS_100U_U3_EN);
+
+	clrbits_le32(u2_banks->com + U3P_U2PHYACR4, P2C_U2_GPIO_CTR_MSK);
+
+	/* DP/DM BC1.1 path Disable */
+	clrsetbits_le32(u2_banks->com + U3P_USBPHYACR6,
+			PA6_RG_U2_BC11_SW_EN | PA6_RG_U2_SQTH,
+			PA6_RG_U2_SQTH_VAL(2));
+
+	/* set HS slew rate */
+	clrsetbits_le32(u2_banks->com + U3P_USBPHYACR5,
+			PA5_RG_U2_HSTX_SRCTRL, PA5_RG_U2_HSTX_SRCTRL_VAL(4));
+
+	dev_dbg(tphy->dev, "%s(%d)\n", __func__, instance->index);
+}
+
+static void u2_phy_instance_power_on(struct mtk_tphy *tphy,
+				     struct mtk_phy_instance *instance)
+{
+	struct u2phy_banks *u2_banks = &instance->u2_banks;
+
+	clrbits_le32(u2_banks->com + U3P_U2PHYDTM0,
+		     P2C_RG_XCVRSEL | P2C_RG_DATAIN | P2C_DTM0_PART_MASK);
+
+	/* OTG Enable */
+	setbits_le32(u2_banks->com + U3P_USBPHYACR6,
+		     PA6_RG_U2_OTG_VBUSCMP_EN);
+
+	clrsetbits_le32(u2_banks->com + U3P_U2PHYDTM1,
+			P2C_RG_SESSEND, P2C_RG_VBUSVALID | P2C_RG_AVALID);
+
+	dev_dbg(tphy->dev, "%s(%d)\n", __func__, instance->index);
+}
+
+static void u2_phy_instance_power_off(struct mtk_tphy *tphy,
+				      struct mtk_phy_instance *instance)
+{
+	struct u2phy_banks *u2_banks = &instance->u2_banks;
+
+	clrbits_le32(u2_banks->com + U3P_U2PHYDTM0,
+		     P2C_RG_XCVRSEL | P2C_RG_DATAIN);
+
+	/* OTG Disable */
+	clrbits_le32(u2_banks->com + U3P_USBPHYACR6,
+		     PA6_RG_U2_OTG_VBUSCMP_EN);
+
+	clrsetbits_le32(u2_banks->com + U3P_U2PHYDTM1,
+			P2C_RG_VBUSVALID | P2C_RG_AVALID, P2C_RG_SESSEND);
+
+	dev_dbg(tphy->dev, "%s(%d)\n", __func__, instance->index);
+}
+
+static void u3_phy_instance_init(struct mtk_tphy *tphy,
+				 struct mtk_phy_instance *instance)
+{
+	struct u3phy_banks *u3_banks = &instance->u3_banks;
+
+	/* gating PCIe Analog XTAL clock */
+	setbits_le32(u3_banks->spllc + U3P_SPLLC_XTALCTL3,
+		     XC3_RG_U3_XTAL_RX_PWD | XC3_RG_U3_FRC_XTAL_RX_PWD);
+
+	/* gating XSQ */
+	clrsetbits_le32(u3_banks->phya + U3P_U3_PHYA_DA_REG0,
+			P3A_RG_XTAL_EXT_EN_U3, P3A_RG_XTAL_EXT_EN_U3_VAL(2));
+
+	clrsetbits_le32(u3_banks->phya + U3P_U3_PHYA_REG9,
+			P3A_RG_RX_DAC_MUX, P3A_RG_RX_DAC_MUX_VAL(4));
+
+	clrsetbits_le32(u3_banks->phya + U3P_U3_PHYA_REG6,
+			P3A_RG_TX_EIDLE_CM, P3A_RG_TX_EIDLE_CM_VAL(0xe));
+
+	clrsetbits_le32(u3_banks->phyd + U3P_U3_PHYD_CDR1,
+			P3D_RG_CDR_BIR_LTD0 | P3D_RG_CDR_BIR_LTD1,
+			P3D_RG_CDR_BIR_LTD0_VAL(0xc) |
+			P3D_RG_CDR_BIR_LTD1_VAL(0x3));
+
+	clrsetbits_le32(u3_banks->phyd + U3P_U3_PHYD_LFPS1,
+			P3D_RG_FWAKE_TH, P3D_RG_FWAKE_TH_VAL(0x34));
+
+	clrsetbits_le32(u3_banks->phyd + U3P_U3_PHYD_RXDET1,
+			P3D_RG_RXDET_STB2_SET, P3D_RG_RXDET_STB2_SET_VAL(0x10));
+
+	clrsetbits_le32(u3_banks->phyd + U3P_U3_PHYD_RXDET2,
+			P3D_RG_RXDET_STB2_SET_P3,
+			P3D_RG_RXDET_STB2_SET_P3_VAL(0x10));
+
+	dev_dbg(tphy->dev, "%s(%d)\n", __func__, instance->index);
+}
+
 static void pcie_phy_instance_init(struct mtk_tphy *tphy,
 				   struct mtk_phy_instance *instance)
 {
@@ -187,9 +376,16 @@ static void pcie_phy_instance_power_off(struct mtk_tphy *tphy,
 static void phy_v1_banks_init(struct mtk_tphy *tphy,
 			      struct mtk_phy_instance *instance)
 {
+	struct u2phy_banks *u2_banks = &instance->u2_banks;
 	struct u3phy_banks *u3_banks = &instance->u3_banks;
 
 	switch (instance->type) {
+	case PHY_TYPE_USB2:
+		u2_banks->misc = NULL;
+		u2_banks->fmreg = tphy->sif_base + SSUSB_SIFSLV_V1_U2FREQ;
+		u2_banks->com = instance->port_base + SSUSB_SIFSLV_V1_U2PHY_COM;
+		break;
+	case PHY_TYPE_USB3:
 	case PHY_TYPE_PCIE:
 		u3_banks->spllc = tphy->sif_base + SSUSB_SIFSLV_V1_SPLLC;
 		u3_banks->chip = tphy->sif_base + SSUSB_SIFSLV_V1_CHIP;
@@ -197,6 +393,7 @@ static void phy_v1_banks_init(struct mtk_tphy *tphy,
 		u3_banks->phya = instance->port_base + SSUSB_SIFSLV_V1_U3PHYA;
 		break;
 	default:
+		dev_err(tphy->dev, "incompatible PHY type\n");
 		return;
 	}
 }
@@ -212,10 +409,17 @@ static int mtk_phy_init(struct phy *phy)
 		return ret;
 
 	switch (instance->type) {
+	case PHY_TYPE_USB2:
+		u2_phy_instance_init(tphy, instance);
+		break;
+	case PHY_TYPE_USB3:
+		u3_phy_instance_init(tphy, instance);
+		break;
 	case PHY_TYPE_PCIE:
 		pcie_phy_instance_init(tphy, instance);
 		break;
 	default:
+		dev_err(tphy->dev, "incompatible PHY type\n");
 		return -EINVAL;
 	}
 
@@ -227,7 +431,10 @@ static int mtk_phy_power_on(struct phy *phy)
 	struct mtk_tphy *tphy = dev_get_priv(phy->dev);
 	struct mtk_phy_instance *instance = tphy->phys[phy->id];
 
-	pcie_phy_instance_power_on(tphy, instance);
+	if (instance->type == PHY_TYPE_USB2)
+		u2_phy_instance_power_on(tphy, instance);
+	else if (instance->type == PHY_TYPE_PCIE)
+		pcie_phy_instance_power_on(tphy, instance);
 
 	return 0;
 }
@@ -237,7 +444,10 @@ static int mtk_phy_power_off(struct phy *phy)
 	struct mtk_tphy *tphy = dev_get_priv(phy->dev);
 	struct mtk_phy_instance *instance = tphy->phys[phy->id];
 
-	pcie_phy_instance_power_off(tphy, instance);
+	if (instance->type == PHY_TYPE_USB2)
+		u2_phy_instance_power_off(tphy, instance);
+	else if (instance->type == PHY_TYPE_PCIE)
+		pcie_phy_instance_power_off(tphy, instance);
 
 	return 0;
 }
@@ -285,8 +495,7 @@ static int mtk_phy_xlate(struct phy *phy,
 	instance->type = args->args[1];
 	if (!(instance->type == PHY_TYPE_USB2 ||
 	      instance->type == PHY_TYPE_USB3 ||
-	      instance->type == PHY_TYPE_PCIE ||
-	      instance->type == PHY_TYPE_SATA)) {
+	      instance->type == PHY_TYPE_PCIE)) {
 		dev_err(phy->dev, "unsupported device type\n");
 		return -EINVAL;
 	}
@@ -318,6 +527,7 @@ static int mtk_tphy_probe(struct udevice *dev)
 	if (!tphy->phys)
 		return -ENOMEM;
 
+	tphy->dev = dev;
 	tphy->sif_base = dev_read_addr_ptr(dev);
 	if (!tphy->sif_base)
 		return -ENOENT;
-- 
2.25.1

[PATCH 2/8] phy: phy-mtk-tphy: add support new version

[Chunfeng Yun]

The new version removes all shared banks between multi-phys

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/phy/phy-mtk-tphy.c | 68 +++++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/phy-mtk-tphy.c b/drivers/phy/phy-mtk-tphy.c
index fc74098c84..dd59f9a13f 100644
--- a/drivers/phy/phy-mtk-tphy.c
+++ b/drivers/phy/phy-mtk-tphy.c
@@ -28,6 +28,17 @@
 #define SSUSB_SIFSLV_V1_U3PHYD		0x000
 #define SSUSB_SIFSLV_V1_U3PHYA		0x200
 
+/* version V2 sub-banks offset base address */
+/* u2 phy banks */
+#define SSUSB_SIFSLV_V2_MISC		0x000
+#define SSUSB_SIFSLV_V2_U2FREQ		0x100
+#define SSUSB_SIFSLV_V2_U2PHY_COM	0x300
+/* u3/pcie/sata phy banks */
+#define SSUSB_SIFSLV_V2_SPLLC		0x000
+#define SSUSB_SIFSLV_V2_CHIP		0x100
+#define SSUSB_SIFSLV_V2_U3PHYD		0x200
+#define SSUSB_SIFSLV_V2_U3PHYA		0x400
+
 #define U3P_USBPHYACR0			0x000
 #define PA0_RG_U2PLL_FORCE_ON		BIT(15)
 #define PA0_RG_USB20_INTR_EN		BIT(5)
@@ -162,6 +173,11 @@
 #define XC3_RG_U3_XTAL_RX_PWD		BIT(9)
 #define XC3_RG_U3_FRC_XTAL_RX_PWD	BIT(8)
 
+enum mtk_phy_version {
+	MTK_TPHY_V1 = 1,
+	MTK_TPHY_V2,
+};
+
 struct u2phy_banks {
 	void __iomem *misc;
 	void __iomem *fmreg;
@@ -192,6 +208,7 @@ struct mtk_phy_instance {
 struct mtk_tphy {
 	struct udevice *dev;
 	void __iomem *sif_base;
+	enum mtk_phy_version version;
 	struct mtk_phy_instance **phys;
 	int nphys;
 };
@@ -304,6 +321,9 @@ static void pcie_phy_instance_init(struct mtk_tphy *tphy,
 {
 	struct u3phy_banks *u3_banks = &instance->u3_banks;
 
+	if (tphy->version != MTK_TPHY_V1)
+		return;
+
 	clrsetbits_le32(u3_banks->phya + U3P_U3_PHYA_DA_REG0,
 			P3A_RG_XTAL_EXT_PE1H | P3A_RG_XTAL_EXT_PE2H,
 			P3A_RG_XTAL_EXT_PE1H_VAL(0x2) |
@@ -398,6 +418,31 @@ static void phy_v1_banks_init(struct mtk_tphy *tphy,
 	}
 }
 
+static void phy_v2_banks_init(struct mtk_tphy *tphy,
+			      struct mtk_phy_instance *instance)
+{
+	struct u2phy_banks *u2_banks = &instance->u2_banks;
+	struct u3phy_banks *u3_banks = &instance->u3_banks;
+
+	switch (instance->type) {
+	case PHY_TYPE_USB2:
+		u2_banks->misc = instance->port_base + SSUSB_SIFSLV_V2_MISC;
+		u2_banks->fmreg = instance->port_base + SSUSB_SIFSLV_V2_U2FREQ;
+		u2_banks->com = instance->port_base + SSUSB_SIFSLV_V2_U2PHY_COM;
+		break;
+	case PHY_TYPE_USB3:
+	case PHY_TYPE_PCIE:
+		u3_banks->spllc = instance->port_base + SSUSB_SIFSLV_V2_SPLLC;
+		u3_banks->chip = instance->port_base + SSUSB_SIFSLV_V2_CHIP;
+		u3_banks->phyd = instance->port_base + SSUSB_SIFSLV_V2_U3PHYD;
+		u3_banks->phya = instance->port_base + SSUSB_SIFSLV_V2_U3PHYA;
+		break;
+	default:
+		dev_err(tphy->dev, "incompatible PHY type\n");
+		return;
+	}
+}
+
 static int mtk_phy_init(struct phy *phy)
 {
 	struct mtk_tphy *tphy = dev_get_priv(phy->dev);
@@ -500,7 +545,14 @@ static int mtk_phy_xlate(struct phy *phy,
 		return -EINVAL;
 	}
 
-	phy_v1_banks_init(tphy, instance);
+	if (tphy->version == MTK_TPHY_V1) {
+		phy_v1_banks_init(tphy, instance);
+	} else if (tphy->version == MTK_TPHY_V2) {
+		phy_v2_banks_init(tphy, instance);
+	} else {
+		dev_err(phy->dev, "phy version is not supported\n");
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -528,9 +580,14 @@ static int mtk_tphy_probe(struct udevice *dev)
 		return -ENOMEM;
 
 	tphy->dev = dev;
-	tphy->sif_base = dev_read_addr_ptr(dev);
-	if (!tphy->sif_base)
-		return -ENOENT;
+	tphy->version = dev_get_driver_data(dev);
+
+	/* v1 has shared banks */
+	if (tphy->version == MTK_TPHY_V1) {
+		tphy->sif_base = dev_read_addr_ptr(dev);
+		if (!tphy->sif_base)
+			return -ENOENT;
+	}
 
 	dev_for_each_subnode(subnode, dev) {
 		struct mtk_phy_instance *instance;
@@ -561,7 +618,8 @@ static int mtk_tphy_probe(struct udevice *dev)
 }
 
 static const struct udevice_id mtk_tphy_id_table[] = {
-	{ .compatible = "mediatek,generic-tphy-v1", },
+	{ .compatible = "mediatek,generic-tphy-v1", .data = MTK_TPHY_V1, },
+	{ .compatible = "mediatek,generic-tphy-v2", .data = MTK_TPHY_V2, },
 	{ }
 };
 
-- 
2.25.1

[PATCH 3/8] phy: phy-mtk-tphy: add a new reference clock

[Chunfeng Yun]

Usually the digital and analog phys use the same reference clock,
but some platforms have two separate reference clocks for each of
them, so add another optional clock to support them.
In order to keep the clock names consistent with PHY IP's, change
the da_ref for analog phy and ref clock for digital phy.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/phy/phy-mtk-tphy.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/phy-mtk-tphy.c b/drivers/phy/phy-mtk-tphy.c
index dd59f9a13f..86cefaa0c5 100644
--- a/drivers/phy/phy-mtk-tphy.c
+++ b/drivers/phy/phy-mtk-tphy.c
@@ -199,8 +199,8 @@ struct mtk_phy_instance {
 		struct u3phy_banks u3_banks;
 	};
 
-	/* reference clock of anolog phy */
-	struct clk ref_clk;
+	struct clk ref_clk;	/* reference clock of (digital) phy */
+	struct clk da_ref_clk;	/* reference clock of analog phy */
 	u32 index;
 	u32 type;
 };
@@ -450,8 +450,17 @@ static int mtk_phy_init(struct phy *phy)
 	int ret;
 
 	ret = clk_enable(&instance->ref_clk);
-	if (ret)
+	if (ret < 0) {
+		dev_err(tphy->dev, "failed to enable ref_clk\n");
 		return ret;
+	}
+
+	ret = clk_enable(&instance->da_ref_clk);
+	if (ret < 0) {
+		dev_err(tphy->dev, "failed to enable da_ref_clk %d\n", ret);
+		clk_disable(&instance->ref_clk);
+		return ret;
+	}
 
 	switch (instance->type) {
 	case PHY_TYPE_USB2:
@@ -502,6 +511,7 @@ static int mtk_phy_exit(struct phy *phy)
 	struct mtk_tphy *tphy = dev_get_priv(phy->dev);
 	struct mtk_phy_instance *instance = tphy->phys[phy->id];
 
+	clk_disable(&instance->da_ref_clk);
 	clk_disable(&instance->ref_clk);
 
 	return 0;
@@ -612,6 +622,11 @@ static int mtk_tphy_probe(struct udevice *dev)
 					     &instance->ref_clk);
 		if (err)
 			return err;
+
+		err = clk_get_optional_nodev(subnode, "da_ref",
+					     &instance->da_ref_clk);
+		if (err)
+			return err;
 	}
 
 	return 0;
-- 
2.25.1

[PATCH 4/8] dm: core: Add function to get child count of ofnode

[Chunfeng Yun]

This patch add a function used to get the child count of
a ofnode

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 include/dm/ofnode.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
index b5a50e8849..b2c0118a36 100644
--- a/include/dm/ofnode.h
+++ b/include/dm/ofnode.h
@@ -793,6 +793,23 @@ ofnode ofnode_by_prop_value(ofnode from, const char *propname,
 	     ofnode_valid(node); \
 	     node = ofnode_next_subnode(node))
 
+/**
+ * ofnode_get_child_count() - get the child count of a ofnode
+ *
+ * @node:	valid node ot get its child count
+ * @return the count of child subnode
+ */
+static inline int ofnode_get_child_count(ofnode parent)
+{
+	ofnode child;
+	int num = 0;
+
+	ofnode_for_each_subnode(child, parent)
+		num++;
+
+	return num;
+}
+
 /**
  * ofnode_translate_address() - Translate a device-tree address
  *
-- 
2.25.1

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Chunfeng Yun]

This patch is used to support the on-chip xHCI controller on
MediaTek SoCs, currently only control/bulk transfers are
supported.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/host/Kconfig    |   6 +
 drivers/usb/host/Makefile   |   1 +
 drivers/usb/host/xhci-mtk.c | 508 ++++++++++++++++++++++++++++++++++++
 3 files changed, 515 insertions(+)
 create mode 100644 drivers/usb/host/xhci-mtk.c

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 0987ff25b1..931af268f4 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -30,6 +30,12 @@ config USB_XHCI_DWC3_OF_SIMPLE
 	  Support USB2/3 functionality in simple SoC integrations with
 	  USB controller based on the DesignWare USB3 IP Core.
 
+config USB_XHCI_MTK
+	bool "Support for MediaTek on-chip xHCI USB controller"
+	depends on ARCH_MEDIATEK
+	help
+	  Enables support for the on-chip xHCI controller on MediaTek SoCs.
+
 config USB_XHCI_MVEBU
 	bool "MVEBU USB 3.0 support"
 	default y
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 7feeff679c..104821f188 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_USB_XHCI_DWC3_OF_SIMPLE) += dwc3-of-simple.o
 obj-$(CONFIG_USB_XHCI_ROCKCHIP) += xhci-rockchip.o
 obj-$(CONFIG_USB_XHCI_EXYNOS) += xhci-exynos5.o
 obj-$(CONFIG_USB_XHCI_FSL) += xhci-fsl.o
+obj-$(CONFIG_USB_XHCI_MTK) += xhci-mtk.o
 obj-$(CONFIG_USB_XHCI_MVEBU) += xhci-mvebu.o
 obj-$(CONFIG_USB_XHCI_OMAP) += xhci-omap.o
 obj-$(CONFIG_USB_XHCI_PCI) += xhci-pci.o
diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
new file mode 100644
index 0000000000..a2a042b07c
--- /dev/null
+++ b/drivers/usb/host/xhci-mtk.c
@@ -0,0 +1,508 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2019 MediaTek, Inc.
+ * Authors: Chunfeng Yun <chunfeng.yun@mediatek.com>
+ */
+
+#include <clk.h>
+#include <common.h>
+#include <dm.h>
+#include <dm/devres.h>
+#include <generic-phy.h>
+#include <malloc.h>
+#include <usb.h>
+#include <linux/errno.h>
+#include <linux/compat.h>
+#include <power/regulator.h>
+#include <linux/iopoll.h>
+
+#include <usb/xhci.h>
+
+#define MU3C_U3_PORT_MAX 4
+#define MU3C_U2_PORT_MAX 5
+
+/**
+ * struct mtk_ippc_regs: MTK ssusb ip port control registers
+ * @ip_pw_ctr0~3: ip power and clock control registers
+ * @ip_pw_sts1~2: ip power and clock status registers
+ * @ip_xhci_cap: ip xHCI capability register
+ * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
+ * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
+ * @u2_phy_pll: usb2 phy pll control register
+ */
+struct mtk_ippc_regs {
+	__le32 ip_pw_ctr0;
+	__le32 ip_pw_ctr1;
+	__le32 ip_pw_ctr2;
+	__le32 ip_pw_ctr3;
+	__le32 ip_pw_sts1;
+	__le32 ip_pw_sts2;
+	__le32 reserved0[3];
+	__le32 ip_xhci_cap;
+	__le32 reserved1[2];
+	__le64 u3_ctrl_p[MU3C_U3_PORT_MAX];
+	__le64 u2_ctrl_p[MU3C_U2_PORT_MAX];
+	__le32 reserved2;
+	__le32 u2_phy_pll;
+	__le32 reserved3[33]; /* 0x80 ~ 0xff */
+};
+
+/* ip_pw_ctrl0 register */
+#define CTRL0_IP_SW_RST	BIT(0)
+
+/* ip_pw_ctrl1 register */
+#define CTRL1_IP_HOST_PDN	BIT(0)
+
+/* ip_pw_ctrl2 register */
+#define CTRL2_IP_DEV_PDN	BIT(0)
+
+/* ip_pw_sts1 register */
+#define STS1_IP_SLEEP_STS	BIT(30)
+#define STS1_U3_MAC_RST	BIT(16)
+#define STS1_XHCI_RST		BIT(11)
+#define STS1_SYS125_RST	BIT(10)
+#define STS1_REF_RST		BIT(8)
+#define STS1_SYSPLL_STABLE	BIT(0)
+
+/* ip_xhci_cap register */
+#define CAP_U3_PORT_NUM(p)	((p) & 0xff)
+#define CAP_U2_PORT_NUM(p)	(((p) >> 8) & 0xff)
+
+/* u3_ctrl_p register */
+#define CTRL_U3_PORT_HOST_SEL	BIT(2)
+#define CTRL_U3_PORT_PDN	BIT(1)
+#define CTRL_U3_PORT_DIS	BIT(0)
+
+/* u2_ctrl_p register */
+#define CTRL_U2_PORT_HOST_SEL	BIT(2)
+#define CTRL_U2_PORT_PDN	BIT(1)
+#define CTRL_U2_PORT_DIS	BIT(0)
+
+struct mtk_xhci {
+	struct xhci_ctrl ctrl;	/* Needs to come first in this struct! */
+	struct xhci_hccr *hcd;
+	struct mtk_ippc_regs *ippc;
+	struct udevice *dev;
+	struct udevice *vusb33_supply;
+	struct udevice *vbus_supply;
+	struct clk *sys_clk;
+	struct clk *xhci_clk;
+	struct clk *ref_clk;
+	struct clk *mcu_clk;
+	struct clk *dma_clk;
+	int num_u2_ports;
+	int num_u3_ports;
+	struct phy *phys;
+	int num_phys;
+};
+
+static int xhci_mtk_host_enable(struct mtk_xhci *mtk)
+{
+	struct mtk_ippc_regs *ippc = mtk->ippc;
+	u32 value, check_val;
+	int ret;
+	int i;
+
+	/* power on host ip */
+	value = readl(&ippc->ip_pw_ctr1);
+	value &= ~CTRL1_IP_HOST_PDN;
+	writel(value, &ippc->ip_pw_ctr1);
+
+	/* power on and enable all u3 ports */
+	for (i = 0; i < mtk->num_u3_ports; i++) {
+		value = readl(&ippc->u3_ctrl_p[i]);
+		value &= ~(CTRL_U3_PORT_PDN | CTRL_U3_PORT_DIS);
+		value |= CTRL_U3_PORT_HOST_SEL;
+		writel(value, &ippc->u3_ctrl_p[i]);
+	}
+
+	/* power on and enable all u2 ports */
+	for (i = 0; i < mtk->num_u2_ports; i++) {
+		value = readl(&ippc->u2_ctrl_p[i]);
+		value &= ~(CTRL_U2_PORT_PDN | CTRL_U2_PORT_DIS);
+		value |= CTRL_U2_PORT_HOST_SEL;
+		writel(value, &ippc->u2_ctrl_p[i]);
+	}
+
+	/*
+	 * wait for clocks to be stable, and clock domains reset to
+	 * be inactive after power on and enable ports
+	 */
+	check_val = STS1_SYSPLL_STABLE | STS1_REF_RST |
+			STS1_SYS125_RST | STS1_XHCI_RST;
+
+	if (mtk->num_u3_ports)
+		check_val |= STS1_U3_MAC_RST;
+
+	ret = readl_poll_timeout(&ippc->ip_pw_sts1, value,
+				 (check_val == (value & check_val)), 20000);
+	if (ret) {
+		dev_err(mtk->dev, "clocks are not stable (0x%x)\n", value);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int xhci_mtk_host_disable(struct mtk_xhci *mtk)
+{
+	struct mtk_ippc_regs *ippc = mtk->ippc;
+	u32 value;
+	int i;
+
+	/* power down all u3 ports */
+	for (i = 0; i < mtk->num_u3_ports; i++) {
+		value = readl(&ippc->u3_ctrl_p[i]);
+		value |= CTRL_U3_PORT_PDN;
+		writel(value, &ippc->u3_ctrl_p[i]);
+	}
+
+	/* power down all u2 ports */
+	for (i = 0; i < mtk->num_u2_ports; i++) {
+		value = readl(&ippc->u2_ctrl_p[i]);
+		value |= CTRL_U2_PORT_PDN;
+		writel(value, &ippc->u2_ctrl_p[i]);
+	}
+
+	/* power down host ip */
+	value = readl(&ippc->ip_pw_ctr1);
+	value |= CTRL1_IP_HOST_PDN;
+	writel(value, &ippc->ip_pw_ctr1);
+
+	return 0;
+}
+
+static int xhci_mtk_ssusb_init(struct mtk_xhci *mtk)
+{
+	struct mtk_ippc_regs *ippc = mtk->ippc;
+	u32 value;
+
+	/* reset whole ip */
+	value = readl(&ippc->ip_pw_ctr0);
+	value |= CTRL0_IP_SW_RST;
+	writel(value, &ippc->ip_pw_ctr0);
+	udelay(1);
+	value = readl(&ippc->ip_pw_ctr0);
+	value &= ~CTRL0_IP_SW_RST;
+	writel(value, &ippc->ip_pw_ctr0);
+
+	value = readl(&ippc->ip_xhci_cap);
+	mtk->num_u3_ports = CAP_U3_PORT_NUM(value);
+	mtk->num_u2_ports = CAP_U2_PORT_NUM(value);
+	dev_info(mtk->dev, "%s u2p:%d, u3p:%d\n", __func__,
+		 mtk->num_u2_ports, mtk->num_u3_ports);
+
+	return xhci_mtk_host_enable(mtk);
+}
+
+static int xhci_mtk_clks_enable(struct mtk_xhci *mtk)
+{
+	int ret;
+
+	ret = clk_enable(mtk->sys_clk);
+	if (ret) {
+		dev_err(mtk->dev, "failed to enable sys_clk\n");
+		goto sys_clk_err;
+	}
+
+	ret = clk_enable(mtk->ref_clk);
+	if (ret) {
+		dev_err(mtk->dev, "failed to enable ref_clk\n");
+		goto ref_clk_err;
+	}
+
+	ret = clk_enable(mtk->xhci_clk);
+	if (ret) {
+		dev_err(mtk->dev, "failed to enable xhci_clk\n");
+		goto xhci_clk_err;
+	}
+
+	ret = clk_enable(mtk->mcu_clk);
+	if (ret) {
+		dev_err(mtk->dev, "failed to enable mcu_clk\n");
+		goto mcu_clk_err;
+	}
+
+	ret = clk_enable(mtk->dma_clk);
+	if (ret) {
+		dev_err(mtk->dev, "failed to enable dma_clk\n");
+		goto dma_clk_err;
+	}
+
+	return 0;
+
+dma_clk_err:
+	clk_disable(mtk->mcu_clk);
+mcu_clk_err:
+	clk_disable(mtk->xhci_clk);
+xhci_clk_err:
+	clk_disable(mtk->sys_clk);
+sys_clk_err:
+	clk_disable(mtk->ref_clk);
+ref_clk_err:
+	return ret;
+}
+
+static void xhci_mtk_clks_disable(struct mtk_xhci *mtk)
+{
+	clk_disable(mtk->dma_clk);
+	clk_disable(mtk->mcu_clk);
+	clk_disable(mtk->xhci_clk);
+	clk_disable(mtk->ref_clk);
+	clk_disable(mtk->sys_clk);
+}
+
+static int xhci_mtk_ofdata_get(struct mtk_xhci *mtk)
+{
+	struct udevice *dev = mtk->dev;
+	fdt_addr_t reg_base;
+	int ret = 0;
+
+	reg_base = devfdt_get_addr_name(dev, "mac");
+	if (reg_base == FDT_ADDR_T_NONE) {
+		pr_err("Failed to get xHCI base address\n");
+		return -ENXIO;
+	}
+
+	mtk->hcd = (struct xhci_hccr *)reg_base;
+
+	reg_base = devfdt_get_addr_name(dev, "ippc");
+	if (reg_base == FDT_ADDR_T_NONE) {
+		pr_err("Failed to get IPPC base address\n");
+		return -ENXIO;
+	}
+
+	mtk->ippc = (struct mtk_ippc_regs *)reg_base;
+
+	dev_info(dev, "hcd: 0x%x, ippc: 0x%x\n",
+		 (int)mtk->hcd, (int)mtk->ippc);
+
+	mtk->sys_clk = devm_clk_get(dev, "sys_ck");
+	if (IS_ERR(mtk->sys_clk)) {
+		dev_err(dev, "Failed to get sys clock\n");
+		goto clk_err;
+	}
+
+	mtk->ref_clk = devm_clk_get_optional(dev, "ref_ck");
+	if (IS_ERR(mtk->ref_clk)) {
+		dev_err(dev, "Failed to get ref clock\n");
+		goto clk_err;
+	}
+
+	mtk->xhci_clk = devm_clk_get_optional(dev, "xhci_ck");
+	if (IS_ERR(mtk->xhci_clk)) {
+		dev_err(dev, "Failed to get xhci clock\n");
+		goto clk_err;
+	}
+
+	mtk->mcu_clk = devm_clk_get_optional(dev, "mcu_ck");
+	if (IS_ERR(mtk->mcu_clk)) {
+		dev_err(dev, "Failed to get mcu clock\n");
+		goto clk_err;
+	}
+
+	mtk->dma_clk = devm_clk_get_optional(dev, "dma_ck");
+	if (IS_ERR(mtk->dma_clk)) {
+		dev_err(dev, "Failed to get dma clock\n");
+		goto clk_err;
+	}
+
+	ret = device_get_supply_regulator(dev, "vusb33-supply",
+					  &mtk->vusb33_supply);
+	if (ret)
+		debug("Can't get vusb33 regulator! %d\n", ret);
+
+	ret = device_get_supply_regulator(dev, "vbus-supply",
+					  &mtk->vbus_supply);
+	if (ret)
+		debug("Can't get vbus regulator! %d\n", ret);
+
+	return 0;
+
+clk_err:
+	return ret;
+}
+
+static int xhci_mtk_ldos_enable(struct mtk_xhci *mtk)
+{
+	int ret;
+
+	ret = regulator_set_enable(mtk->vusb33_supply, true);
+	if (ret < 0 && ret != -ENOSYS) {
+		dev_err(mtk->dev, "failed to enable vusb33\n");
+		return ret;
+	}
+
+	ret = regulator_set_enable(mtk->vbus_supply, true);
+	if (ret < 0 && ret != -ENOSYS) {
+		dev_err(mtk->dev, "failed to enable vbus\n");
+		regulator_set_enable(mtk->vusb33_supply, false);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void xhci_mtk_ldos_disable(struct mtk_xhci *mtk)
+{
+	regulator_set_enable(mtk->vbus_supply, false);
+	regulator_set_enable(mtk->vusb33_supply, false);
+}
+
+int xhci_mtk_phy_setup(struct mtk_xhci *mtk)
+{
+	struct udevice *dev = mtk->dev;
+	struct phy *usb_phys;
+	int i, ret, count;
+
+	/* Return if no phy declared */
+	if (!dev_read_prop(dev, "phys", NULL))
+		return 0;
+
+	count = dev_count_phandle_with_args(dev, "phys", "#phy-cells");
+	if (count <= 0)
+		return count;
+
+	usb_phys = devm_kcalloc(dev, count, sizeof(*usb_phys),
+				GFP_KERNEL);
+	if (!usb_phys)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		ret = generic_phy_get_by_index(dev, i, &usb_phys[i]);
+		if (ret && ret != -ENOENT) {
+			dev_err(dev, "Failed to get USB PHY%d for %s\n",
+				i, dev->name);
+			return ret;
+		}
+	}
+
+	for (i = 0; i < count; i++) {
+		ret = generic_phy_init(&usb_phys[i]);
+		if (ret) {
+			dev_err(dev, "Can't init USB PHY%d for %s\n",
+				i, dev->name);
+			goto phys_init_err;
+		}
+	}
+
+	for (i = 0; i < count; i++) {
+		ret = generic_phy_power_on(&usb_phys[i]);
+		if (ret) {
+			dev_err(dev, "Can't power USB PHY%d for %s\n",
+				i, dev->name);
+			goto phys_poweron_err;
+		}
+	}
+
+	mtk->phys = usb_phys;
+	mtk->num_phys =  count;
+
+	return 0;
+
+phys_poweron_err:
+	for (i = count - 1; i >= 0; i--)
+		generic_phy_power_off(&usb_phys[i]);
+
+	for (i = 0; i < count; i++)
+		generic_phy_exit(&usb_phys[i]);
+
+	return ret;
+
+phys_init_err:
+	for (; i >= 0; i--)
+		generic_phy_exit(&usb_phys[i]);
+
+	return ret;
+}
+
+void xhci_mtk_phy_shutdown(struct mtk_xhci *mtk)
+{
+	struct udevice *dev = mtk->dev;
+	struct phy *usb_phys;
+	int i, ret;
+
+	usb_phys = mtk->phys;
+	for (i = 0; i < mtk->num_phys; i++) {
+		if (!generic_phy_valid(&usb_phys[i]))
+			continue;
+
+		ret = generic_phy_power_off(&usb_phys[i]);
+		ret |= generic_phy_exit(&usb_phys[i]);
+		if (ret) {
+			dev_err(dev, "Can't shutdown USB PHY%d for %s\n",
+				i, dev->name);
+		}
+	}
+}
+
+static int xhci_mtk_probe(struct udevice *dev)
+{
+	struct mtk_xhci *mtk = dev_get_priv(dev);
+	struct xhci_hcor *hcor;
+	int ret;
+
+	mtk->dev = dev;
+	ret = xhci_mtk_ofdata_get(mtk);
+	if (ret)
+		return ret;
+
+	ret = xhci_mtk_ldos_enable(mtk);
+	if (ret)
+		goto ldos_err;
+
+	ret = xhci_mtk_clks_enable(mtk);
+	if (ret)
+		goto clks_err;
+
+	ret = xhci_mtk_phy_setup(mtk);
+	if (ret)
+		goto phys_err;
+
+	ret = xhci_mtk_ssusb_init(mtk);
+	if (ret)
+		goto ssusb_init_err;
+
+	hcor = (struct xhci_hcor *)((uintptr_t)mtk->hcd +
+			HC_LENGTH(xhci_readl(&mtk->hcd->cr_capbase)));
+
+	return xhci_register(dev, mtk->hcd, hcor);
+
+ssusb_init_err:
+	xhci_mtk_phy_shutdown(mtk);
+phys_err:
+	xhci_mtk_clks_disable(mtk);
+clks_err:
+	xhci_mtk_ldos_disable(mtk);
+ldos_err:
+	return ret;
+}
+
+static int xhci_mtk_remove(struct udevice *dev)
+{
+	struct mtk_xhci *mtk = dev_get_priv(dev);
+
+	xhci_deregister(dev);
+	xhci_mtk_host_disable(mtk);
+	xhci_mtk_ldos_disable(mtk);
+	xhci_mtk_clks_disable(mtk);
+
+	return 0;
+}
+
+static const struct udevice_id xhci_mtk_ids[] = {
+	{ .compatible = "mediatek,mtk-xhci" },
+	{ }
+};
+
+U_BOOT_DRIVER(usb_xhci) = {
+	.name	= "xhci-mtk",
+	.id	= UCLASS_USB,
+	.of_match = xhci_mtk_ids,
+	.probe = xhci_mtk_probe,
+	.remove = xhci_mtk_remove,
+	.ops	= &xhci_usb_ops,
+	.bind	= dm_scan_fdt_dev,
+	.priv_auto_alloc_size = sizeof(struct mtk_xhci),
+	.flags	= DM_FLAG_ALLOC_PRIV_DMA,
+};
-- 
2.25.1

[PATCH 6/8] arm: dts: mt7629: add support usb related nodes

[Chunfeng Yun]

Add usb, phy and clock nodes

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 arch/arm/dts/mt7629-rfb.dts |  8 ++++++++
 arch/arm/dts/mt7629.dtsi    | 41 +++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/arch/arm/dts/mt7629-rfb.dts b/arch/arm/dts/mt7629-rfb.dts
index 687fe1c029..bf84f76344 100644
--- a/arch/arm/dts/mt7629-rfb.dts
+++ b/arch/arm/dts/mt7629-rfb.dts
@@ -82,6 +82,14 @@
 	status = "okay";
 };
 
+&xhci {
+	status = "okay";
+};
+
+&u3phy {
+	status = "okay";
+};
+
 &watchdog {
 	pinctrl-names = "default";
 	pinctrl-0 = <&watchdog_pins>;
diff --git a/arch/arm/dts/mt7629.dtsi b/arch/arm/dts/mt7629.dtsi
index a33a74a556..3da02c830e 100644
--- a/arch/arm/dts/mt7629.dtsi
+++ b/arch/arm/dts/mt7629.dtsi
@@ -11,6 +11,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/power/mt7629-power.h>
 #include <dt-bindings/reset/mt7629-reset.h>
+#include <dt-bindings/phy/phy.h>
 #include "skeleton.dtsi"
 
 / {
@@ -222,6 +223,46 @@
 		#size-cells = <0>;
 	};
 
+	ssusbsys: ssusbsys at 1a000000 {
+		compatible = "mediatek,mt7629-ssusbsys", "syscon";
+		reg = <0x1a000000 0x1000>;
+		#clock-cells = <1>;
+	};
+
+	xhci: usb at 1a0c0000 {
+		compatible = "mediatek,mt7629-xhci", "mediatek,mtk-xhci";
+		reg = <0x1a0c0000 0x1000>, <0x1a0c3e00 0x0100>;
+		reg-names = "mac", "ippc";
+		power-domains = <&scpsys MT7629_POWER_DOMAIN_HIF1>;
+		clocks = <&ssusbsys CLK_SSUSB_SYS_EN>, <&ssusbsys CLK_SSUSB_REF_EN>,
+			 <&ssusbsys CLK_SSUSB_MCU_EN>, <&ssusbsys CLK_SSUSB_DMA_EN>;
+		clock-names = "sys_ck", "ref_ck", "mcu_ck", "dma_ck";
+		phys = <&u2port0 PHY_TYPE_USB2>, <&u3port0 PHY_TYPE_USB3>;
+		status = "disabled";
+	};
+
+	u3phy: usb-phy at 1a0c4000 {
+		compatible = "mediatek,mt7629-tphy", "mediatek,generic-tphy-v2";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 0x1a0c4000 0x1000>;
+		status = "disabled";
+
+		u2port0: usb-phy at 0 {
+			reg = <0x0 0x0700>;
+			#phy-cells = <1>;
+			clocks = <&ssusbsys CLK_SSUSB_U2_PHY_EN>, <&clk20m>;
+			clock-names = "ref", "da_ref";
+		};
+
+		u3port0: usb-phy at 700 {
+			reg = <0x0700 0x0700>;
+			#phy-cells = <1>;
+			clocks = <&clk20m>, <&clk20m>;
+			clock-names = "ref", "da_ref";
+		};
+	};
+
 	ethsys: syscon at 1b000000 {
 		compatible = "mediatek,mt7629-ethsys", "syscon";
 		reg = <0x1b000000 0x1000>;
-- 
2.25.1

[PATCH 7/8] dt-bindings: phy-mtk-tphy: add properties of address mapping and clocks

[Chunfeng Yun]

1. add the address mapping related properties;
2. make "ref" clock optional, and add optional clock "da_ref";
3. add the banks layout of TPHY V1 and V2;

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 doc/device-tree-bindings/phy/phy-mtk-tphy.txt | 78 ++++++++++++++++---
 1 file changed, 68 insertions(+), 10 deletions(-)

diff --git a/doc/device-tree-bindings/phy/phy-mtk-tphy.txt b/doc/device-tree-bindings/phy/phy-mtk-tphy.txt
index 037c5a4be5..8cd23d8c0b 100644
--- a/doc/device-tree-bindings/phy/phy-mtk-tphy.txt
+++ b/doc/device-tree-bindings/phy/phy-mtk-tphy.txt
@@ -7,10 +7,17 @@ controllers on MediaTek SoCs, such as, USB2.0, USB3.0, PCIe, and SATA.
 Required properties (controller (parent) node):
  - compatible	: should be one of
 		  "mediatek,generic-tphy-v1"
- - clocks	: (deprecated, use port's clocks instead) a list of phandle +
-		  clock-specifier pairs, one for each entry in clock-names
- - clock-names	: (deprecated, use port's one instead) must contain
-		  "u3phya_ref": for reference clock of usb3.0 analog phy.
+		  "mediatek,generic-tphy-v2"
+
+- #address-cells:	the number of cells used to represent physical
+		base addresses.
+- #size-cells:	the number of cells used to represent the size of an address.
+- ranges:	the address mapping relationship to the parent, defined with
+		- empty value: if optional 'reg' is used.
+		- non-empty value: if optional 'reg' is not used. should set
+			the child's base address to 0, the physical address
+			within parent's address space, and the length of
+			the address map.
 
 Required nodes	: a sub-node is required for each port the controller
 		  provides. Address range information including the usual
@@ -27,12 +34,6 @@ Optional properties (controller (parent) node):
 
 Required properties (port (child) node):
 - reg		: address and length of the register set for the port.
-- clocks	: a list of phandle + clock-specifier pairs, one for each
-		  entry in clock-names
-- clock-names	: must contain
-		  "ref": 48M reference clock for HighSpeed analog phy; and 26M
-			reference clock for SuperSpeed analog phy, sometimes is
-			24M, 25M or 27M, depended on platform.
 - #phy-cells	: should be 1 (See second example)
 		  cell after port phandle is phy type from:
 			- PHY_TYPE_USB2
@@ -40,6 +41,17 @@ Required properties (port (child) node):
 			- PHY_TYPE_PCIE
 			- PHY_TYPE_SATA
 
+Optional properties (port (child) node):
+- clocks	: a list of phandle + clock-specifier pairs, one for each
+		  entry in clock-names
+- clock-names	: may contain
+		  "ref": 48M reference clock for HighSpeed (digital) phy; and 26M
+			reference clock for SuperSpeed (digital) phy, sometimes is
+			24M, 25M or 27M, depended on platform.
+		  "da_ref": the reference clock of analog phy, used if the clocks
+			of analog and digital phys are separated, otherwise uses
+			"ref" clock only if needed.
+
 Example:
 
 	u3phy2: usb-phy at 1a244000 {
@@ -84,3 +96,49 @@ usb30: usb at 11270000 {
 	phy-names = "usb2-0", "usb3-0";
 	...
 };
+
+Layout differences of banks between TPHY V1 and V2
+-------------------------------------------------------------
+IP V1:
+port        offset    bank
+shared      0x0000    SPLLC
+            0x0100    FMREG
+u2 port0    0x0800    U2PHY_COM
+u3 port0    0x0900    U3PHYD
+            0x0a00    U3PHYD_BANK2
+            0x0b00    U3PHYA
+            0x0c00    U3PHYA_DA
+u2 port1    0x1000    U2PHY_COM
+u3 port1    0x1100    U3PHYD
+            0x1200    U3PHYD_BANK2
+            0x1300    U3PHYA
+            0x1400    U3PHYA_DA
+u2 port2    0x1800    U2PHY_COM
+            ...
+
+IP V2:
+port        offset    bank
+u2 port0    0x0000    MISC
+            0x0100    FMREG
+            0x0300    U2PHY_COM
+u3 port0    0x0700    SPLLC
+            0x0800    CHIP
+            0x0900    U3PHYD
+            0x0a00    U3PHYD_BANK2
+            0x0b00    U3PHYA
+            0x0c00    U3PHYA_DA
+u2 port1    0x1000    MISC
+            0x1100    FMREG
+            0x1300    U2PHY_COM
+u3 port1    0x1700    SPLLC
+            0x1800    CHIP
+            0x1900    U3PHYD
+            0x1a00    U3PHYD_BANK2
+            0x1b00    U3PHYA
+            0x1c00    U3PHYA_DA
+u2 port2    0x2000    MISC
+            ...
+
+    SPLLC shared by u3 ports and FMREG shared by u2 ports on
+TPHY V1 are put back into each port; a new bank MISC for
+u2 ports and CHIP for u3 ports are added on TPHY V2.
-- 
2.25.1

[PATCH 8/8] dt-bindings: usb: mtk-xhci: Add binding for MediaTek xHCI host controller

[Chunfeng Yun]

Add dt-binding for MediaTek xHCI host controller

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 .../usb/mediatek,mtk-xhci.txt                 | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 doc/device-tree-bindings/usb/mediatek,mtk-xhci.txt

diff --git a/doc/device-tree-bindings/usb/mediatek,mtk-xhci.txt b/doc/device-tree-bindings/usb/mediatek,mtk-xhci.txt
new file mode 100644
index 0000000000..0447468a2d
--- /dev/null
+++ b/doc/device-tree-bindings/usb/mediatek,mtk-xhci.txt
@@ -0,0 +1,40 @@
+MediaTek xHCI
+
+The device node for USB3 host controller on MediaTek SoCs.
+
+Required properties:
+ - compatible : should be "mediatek,mtk-xhci"
+ - reg : specifies physical base address and size of the registers
+ - reg-names: should be "mac" for xHCI MAC and "ippc" for IP port control
+ - power-domains : a phandle to USB power domain node to control USB's
+	MTCMOS
+ - vusb33-supply : regulator of USB avdd3.3v
+
+ - clocks : a list of phandle + clock-specifier pairs, one for each
+	entry in clock-names
+ - clock-names : must contain
+	"sys_ck": controller clock used by normal mode,
+	the following ones are optional:
+	"ref_ck": reference clock used by low power mode etc,
+	"mcu_ck": mcu_bus clock for register access,
+	"dma_ck": dma_bus clock for data transfer by DMA,
+	"xhci_ck": controller clock
+
+ - phys : list of all the USB PHYs on this HCD
+ - phy-names: name specifier for the USB PHY
+
+Optional properties:
+ - vbus-supply : reference to the VBUS regulator;
+
+Example:
+xhci: usb at 1a0c0000 {
+	compatible = "mediatek,mt7629-xhci", "mediatek,mtk-xhci";
+	reg = <0x1a0c0000 0x1000>, <0x1a0c3e00 0x0100>;
+	reg-names = "mac", "ippc";
+	power-domains = <&scpsys MT7629_POWER_DOMAIN_HIF1>;
+	clocks = <&ssusbsys CLK_SSUSB_SYS_EN>, <&ssusbsys CLK_SSUSB_REF_EN>,
+		 <&ssusbsys CLK_SSUSB_MCU_EN>, <&ssusbsys CLK_SSUSB_DMA_EN>;
+	clock-names = "sys_ck", "ref_ck", "mcu_ck", "dma_ck";
+	phys = <&u2port0 PHY_TYPE_USB2>, <&u3port0 PHY_TYPE_USB3>;
+	status = "disabled";
+};
-- 
2.25.1

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/11/20 7:50 AM, Chunfeng Yun wrote:
[...]
> + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
> + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
> + * @u2_phy_pll: usb2 phy pll control register
> + */
> +struct mtk_ippc_regs {
> +	__le32 ip_pw_ctr0;
> +	__le32 ip_pw_ctr1;
> +	__le32 ip_pw_ctr2;

Please define the registers with #define macros , this struct-based
approach doesn't scale.

[..]

> +static int xhci_mtk_host_enable(struct mtk_xhci *mtk)
> +{
> +	struct mtk_ippc_regs *ippc = mtk->ippc;
> +	u32 value, check_val;
> +	int ret;
> +	int i;
> +
> +	/* power on host ip */
> +	value = readl(&ippc->ip_pw_ctr1);
> +	value &= ~CTRL1_IP_HOST_PDN;
> +	writel(value, &ippc->ip_pw_ctr1);
> +
> +	/* power on and enable all u3 ports */
> +	for (i = 0; i < mtk->num_u3_ports; i++) {
> +		value = readl(&ippc->u3_ctrl_p[i]);
> +		value &= ~(CTRL_U3_PORT_PDN | CTRL_U3_PORT_DIS);
> +		value |= CTRL_U3_PORT_HOST_SEL;
> +		writel(value, &ippc->u3_ctrl_p[i]);
> +	}

Use clrsetbits_le32() above and below and where applicable.

> +	/* power on and enable all u2 ports */
> +	for (i = 0; i < mtk->num_u2_ports; i++) {
> +		value = readl(&ippc->u2_ctrl_p[i]);
> +		value &= ~(CTRL_U2_PORT_PDN | CTRL_U2_PORT_DIS);
> +		value |= CTRL_U2_PORT_HOST_SEL;
> +		writel(value, &ippc->u2_ctrl_p[i]);
> +	}

[...]

> +static int xhci_mtk_clks_enable(struct mtk_xhci *mtk)
> +{
> +	int ret;
> +
> +	ret = clk_enable(mtk->sys_clk);
> +	if (ret) {
> +		dev_err(mtk->dev, "failed to enable sys_clk\n");
> +		goto sys_clk_err;
> +	}

Can you use clk_enable_bulk() and other clk.*bulk functions ?
[...]

-- 
Best regards,
Marek Vasut

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Chunfeng Yun]

On Wed, 2020-03-11 at 08:11 +0100, Marek Vasut wrote:
> On 3/11/20 7:50 AM, Chunfeng Yun wrote:
> [...]
> > + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
> > + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
> > + * @u2_phy_pll: usb2 phy pll control register
> > + */
> > +struct mtk_ippc_regs {
> > +	__le32 ip_pw_ctr0;
> > +	__le32 ip_pw_ctr1;
> > +	__le32 ip_pw_ctr2;
> 
> Please define the registers with #define macros , this struct-based
> approach doesn't scale.
Ok
> 
> [..]
> 
> > +static int xhci_mtk_host_enable(struct mtk_xhci *mtk)
> > +{
> > +	struct mtk_ippc_regs *ippc = mtk->ippc;
> > +	u32 value, check_val;
> > +	int ret;
> > +	int i;
> > +
> > +	/* power on host ip */
> > +	value = readl(&ippc->ip_pw_ctr1);
> > +	value &= ~CTRL1_IP_HOST_PDN;
> > +	writel(value, &ippc->ip_pw_ctr1);
> > +
> > +	/* power on and enable all u3 ports */
> > +	for (i = 0; i < mtk->num_u3_ports; i++) {
> > +		value = readl(&ippc->u3_ctrl_p[i]);
> > +		value &= ~(CTRL_U3_PORT_PDN | CTRL_U3_PORT_DIS);
> > +		value |= CTRL_U3_PORT_HOST_SEL;
> > +		writel(value, &ippc->u3_ctrl_p[i]);
> > +	}
> 
> Use clrsetbits_le32() above and below and where applicable.
Ok
> 
> > +	/* power on and enable all u2 ports */
> > +	for (i = 0; i < mtk->num_u2_ports; i++) {
> > +		value = readl(&ippc->u2_ctrl_p[i]);
> > +		value &= ~(CTRL_U2_PORT_PDN | CTRL_U2_PORT_DIS);
> > +		value |= CTRL_U2_PORT_HOST_SEL;
> > +		writel(value, &ippc->u2_ctrl_p[i]);
> > +	}
> 
> [...]
> 
> > +static int xhci_mtk_clks_enable(struct mtk_xhci *mtk)
> > +{
> > +	int ret;
> > +
> > +	ret = clk_enable(mtk->sys_clk);
> > +	if (ret) {
> > +		dev_err(mtk->dev, "failed to enable sys_clk\n");
> > +		goto sys_clk_err;
> > +	}
> 
> Can you use clk_enable_bulk() and other clk.*bulk functions ?
Will try it, thank you
> [...]
> 

[PATCH 4/8] dm: core: Add function to get child count of ofnode

[Simon Glass]

Hi Chunfeng,

On Wed, 11 Mar 2020 at 01:01, Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> This patch add a function used to get the child count of
> a ofnode
>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>  include/dm/ofnode.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
> index b5a50e8849..b2c0118a36 100644
> --- a/include/dm/ofnode.h
> +++ b/include/dm/ofnode.h
> @@ -793,6 +793,23 @@ ofnode ofnode_by_prop_value(ofnode from, const char *propname,
>              ofnode_valid(node); \
>              node = ofnode_next_subnode(node))
>
> +/**
> + * ofnode_get_child_count() - get the child count of a ofnode
> + *
> + * @node:      valid node ot get its child count
> + * @return the count of child subnode
> + */
> +static inline int ofnode_get_child_count(ofnode parent)

Please put this in a C file. There is not really any benefit to be being inline.

> +{
> +       ofnode child;
> +       int num = 0;
> +
> +       ofnode_for_each_subnode(child, parent)
> +               num++;
> +
> +       return num;
> +}
> +
>  /**
>   * ofnode_translate_address() - Translate a device-tree address
>   *
> --
> 2.25.1

Also please add a simple test for your new function to test/dm/ofnode.c

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

Hi Chunfeng,

On Wed, 11 Mar 2020 at 01:00, Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> This patch is used to support the on-chip xHCI controller on
> MediaTek SoCs, currently only control/bulk transfers are
> supported.
>

Are interrupt transfers planned for later?

> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>  drivers/usb/host/Kconfig    |   6 +
>  drivers/usb/host/Makefile   |   1 +
>  drivers/usb/host/xhci-mtk.c | 508 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 515 insertions(+)
>  create mode 100644 drivers/usb/host/xhci-mtk.c

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Chunfeng Yun]

On Wed, 2020-03-11 at 06:18 -0600, Simon Glass wrote:
> Hi Chunfeng,
> 
> On Wed, 11 Mar 2020 at 01:00, Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> >
> > This patch is used to support the on-chip xHCI controller on
> > MediaTek SoCs, currently only control/bulk transfers are
> > supported.
> >
> 
> Are interrupt transfers planned for later?
Yes, will support it in the later version

> 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >  drivers/usb/host/Kconfig    |   6 +
> >  drivers/usb/host/Makefile   |   1 +
> >  drivers/usb/host/xhci-mtk.c | 508 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 515 insertions(+)
> >  create mode 100644 drivers/usb/host/xhci-mtk.c
> 
> Regards,
> Simon

[PATCH 4/8] dm: core: Add function to get child count of ofnode

[Chunfeng Yun]

On Wed, 2020-03-11 at 06:17 -0600, Simon Glass wrote:
> Hi Chunfeng,
> 
> On Wed, 11 Mar 2020 at 01:01, Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> >
> > This patch add a function used to get the child count of
> > a ofnode
> >
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >  include/dm/ofnode.h | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
> > index b5a50e8849..b2c0118a36 100644
> > --- a/include/dm/ofnode.h
> > +++ b/include/dm/ofnode.h
> > @@ -793,6 +793,23 @@ ofnode ofnode_by_prop_value(ofnode from, const char *propname,
> >              ofnode_valid(node); \
> >              node = ofnode_next_subnode(node))
> >
> > +/**
> > + * ofnode_get_child_count() - get the child count of a ofnode
> > + *
> > + * @node:      valid node ot get its child count
> > + * @return the count of child subnode
> > + */
> > +static inline int ofnode_get_child_count(ofnode parent)
> 
> Please put this in a C file. There is not really any benefit to be being inline.
Ok
> 
> > +{
> > +       ofnode child;
> > +       int num = 0;
> > +
> > +       ofnode_for_each_subnode(child, parent)
> > +               num++;
> > +
> > +       return num;
> > +}
> > +
> >  /**
> >   * ofnode_translate_address() - Translate a device-tree address
> >   *
> > --
> > 2.25.1
> 
> Also please add a simple test for your new function to test/dm/ofnode.c
Will add it.

Thanks 

> 
> Regards,
> Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Chunfeng Yun]

On Wed, 2020-03-11 at 08:11 +0100, Marek Vasut wrote:
> On 3/11/20 7:50 AM, Chunfeng Yun wrote:
> [...]
> > + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
> > + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
> > + * @u2_phy_pll: usb2 phy pll control register
> > + */
> > +struct mtk_ippc_regs {
> > +	__le32 ip_pw_ctr0;
> > +	__le32 ip_pw_ctr1;
> > +	__le32 ip_pw_ctr2;
> 
> Please define the registers with #define macros , this struct-based
> approach doesn't scale.

When I prepare for v2, and find that if define registers as macros, it do not keep
the same style with the xhci core, so I leave it unchanged in v2, if you
still suggest to avoid struct-based approach, I will change it in v3
version, thanks

> [..]
> 

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

Hi Marek,

On Wed, 11 Mar 2020 at 01:11, Marek Vasut <marex@denx.de> wrote:
>
> On 3/11/20 7:50 AM, Chunfeng Yun wrote:
> [...]
> > + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
> > + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
> > + * @u2_phy_pll: usb2 phy pll control register
> > + */
> > +struct mtk_ippc_regs {
> > +     __le32 ip_pw_ctr0;
> > +     __le32 ip_pw_ctr1;
> > +     __le32 ip_pw_ctr2;
>
> Please define the registers with #define macros , this struct-based
> approach doesn't scale.
>

What does this mean? I much prefer the struct approach, unless the
registers move around in future generations.

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/21/20 3:12 PM, Simon Glass wrote:
> Hi Marek,
> 
> On Wed, 11 Mar 2020 at 01:11, Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/11/20 7:50 AM, Chunfeng Yun wrote:
>> [...]
>>> + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
>>> + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
>>> + * @u2_phy_pll: usb2 phy pll control register
>>> + */
>>> +struct mtk_ippc_regs {
>>> +     __le32 ip_pw_ctr0;
>>> +     __le32 ip_pw_ctr1;
>>> +     __le32 ip_pw_ctr2;
>>
>> Please define the registers with #define macros , this struct-based
>> approach doesn't scale.
>>
> 
> What does this mean? I much prefer the struct approach, unless the
> registers move around in future generations.

This means I want to see #define MTK_XHCI_<register name> 0xf00

The struct based approach doesn't scale and on e.g. altera is causing us
a massive amount of problems now. It looked like a good idea back then,
but now it's a massive burden, so I don't want that to proliferate. And
here I would expect the registers to move around, just like everywhere else.

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

Hi Marek,

On Sat, 21 Mar 2020 at 09:09, Marek Vasut <marex@denx.de> wrote:
>
> On 3/21/20 3:12 PM, Simon Glass wrote:
> > Hi Marek,
> >
> > On Wed, 11 Mar 2020 at 01:11, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 3/11/20 7:50 AM, Chunfeng Yun wrote:
> >> [...]
> >>> + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
> >>> + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
> >>> + * @u2_phy_pll: usb2 phy pll control register
> >>> + */
> >>> +struct mtk_ippc_regs {
> >>> +     __le32 ip_pw_ctr0;
> >>> +     __le32 ip_pw_ctr1;
> >>> +     __le32 ip_pw_ctr2;
> >>
> >> Please define the registers with #define macros , this struct-based
> >> approach doesn't scale.
> >>
> >
> > What does this mean? I much prefer the struct approach, unless the
> > registers move around in future generations.
>
> This means I want to see #define MTK_XHCI_<register name> 0xf00
>
> The struct based approach doesn't scale and on e.g. altera is causing us
> a massive amount of problems now. It looked like a good idea back then,
> but now it's a massive burden, so I don't want that to proliferate. And
> here I would expect the registers to move around, just like everywhere else.

That sounds like something that is easily avoided. For example,
Designware doesn't seem to move things around.

Perhaps MediaTek has a policy of not doing this either?

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/21/20 5:42 PM, Simon Glass wrote:
> Hi Marek,

Hi,

> On Sat, 21 Mar 2020 at 09:09, Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/21/20 3:12 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On Wed, 11 Mar 2020 at 01:11, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 3/11/20 7:50 AM, Chunfeng Yun wrote:
>>>> [...]
>>>>> + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
>>>>> + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
>>>>> + * @u2_phy_pll: usb2 phy pll control register
>>>>> + */
>>>>> +struct mtk_ippc_regs {
>>>>> +     __le32 ip_pw_ctr0;
>>>>> +     __le32 ip_pw_ctr1;
>>>>> +     __le32 ip_pw_ctr2;
>>>>
>>>> Please define the registers with #define macros , this struct-based
>>>> approach doesn't scale.
>>>>
>>>
>>> What does this mean? I much prefer the struct approach, unless the
>>> registers move around in future generations.
>>
>> This means I want to see #define MTK_XHCI_<register name> 0xf00
>>
>> The struct based approach doesn't scale and on e.g. altera is causing us
>> a massive amount of problems now. It looked like a good idea back then,
>> but now it's a massive burden, so I don't want that to proliferate. And
>> here I would expect the registers to move around, just like everywhere else.
> 
> That sounds like something that is easily avoided. For example,
> Designware doesn't seem to move things around.
> 
> Perhaps MediaTek has a policy of not doing this either?

Such policies change as the hardware evolves. I got burned by this
struct-based approach more often then it was beneficial, if it ever
really was beneficial, hence I don't want to see it used.

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

Hi Marek,

On Sat, 21 Mar 2020 at 11:00, Marek Vasut <marex@denx.de> wrote:
>
> On 3/21/20 5:42 PM, Simon Glass wrote:
> > Hi Marek,
>
> Hi,
>
> > On Sat, 21 Mar 2020 at 09:09, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 3/21/20 3:12 PM, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On Wed, 11 Mar 2020 at 01:11, Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 3/11/20 7:50 AM, Chunfeng Yun wrote:
> >>>> [...]
> >>>>> + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
> >>>>> + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
> >>>>> + * @u2_phy_pll: usb2 phy pll control register
> >>>>> + */
> >>>>> +struct mtk_ippc_regs {
> >>>>> +     __le32 ip_pw_ctr0;
> >>>>> +     __le32 ip_pw_ctr1;
> >>>>> +     __le32 ip_pw_ctr2;
> >>>>
> >>>> Please define the registers with #define macros , this struct-based
> >>>> approach doesn't scale.
> >>>>
> >>>
> >>> What does this mean? I much prefer the struct approach, unless the
> >>> registers move around in future generations.
> >>
> >> This means I want to see #define MTK_XHCI_<register name> 0xf00
> >>
> >> The struct based approach doesn't scale and on e.g. altera is causing us
> >> a massive amount of problems now. It looked like a good idea back then,
> >> but now it's a massive burden, so I don't want that to proliferate. And
> >> here I would expect the registers to move around, just like everywhere else.
> >
> > That sounds like something that is easily avoided. For example,
> > Designware doesn't seem to move things around.
> >
> > Perhaps MediaTek has a policy of not doing this either?
>
> Such policies change as the hardware evolves. I got burned by this
> struct-based approach more often then it was beneficial, if it ever
> really was beneficial, hence I don't want to see it used.

Some benefits:
- using C struct instead of 'assembler-like' addition is less error-prone
- you can pass the regs struct around between functions in the driver,
particularly helpful in large drivers
- lower-case letters are reasier to read
- you can specify the data size and endianness in the struct using C types

If the hardware changes enough, then you need a new driver anyway, or
perhaps separate C implementations of the low-level register access if
the high-level flow is similar. In general you can't always determine
this sort of thing at compile time since the version of the hardware
may not be apparent until run-time. You end up with variables holding
the offset of each register.

Sometimes is it better to have an umbrella driver with a common core.

So I would push back against a move in this direction in general. It
depends on the circumstances, and anyway, converting from struct to
offsets if desirable later is not that hard.

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/21/20 7:41 PM, Simon Glass wrote:
> Hi Marek,

Hi,

> On Sat, 21 Mar 2020 at 11:00, Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/21/20 5:42 PM, Simon Glass wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Sat, 21 Mar 2020 at 09:09, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 3/21/20 3:12 PM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On Wed, 11 Mar 2020 at 01:11, Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 3/11/20 7:50 AM, Chunfeng Yun wrote:
>>>>>> [...]
>>>>>>> + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
>>>>>>> + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
>>>>>>> + * @u2_phy_pll: usb2 phy pll control register
>>>>>>> + */
>>>>>>> +struct mtk_ippc_regs {
>>>>>>> +     __le32 ip_pw_ctr0;
>>>>>>> +     __le32 ip_pw_ctr1;
>>>>>>> +     __le32 ip_pw_ctr2;
>>>>>>
>>>>>> Please define the registers with #define macros , this struct-based
>>>>>> approach doesn't scale.
>>>>>>
>>>>>
>>>>> What does this mean? I much prefer the struct approach, unless the
>>>>> registers move around in future generations.
>>>>
>>>> This means I want to see #define MTK_XHCI_<register name> 0xf00
>>>>
>>>> The struct based approach doesn't scale and on e.g. altera is causing us
>>>> a massive amount of problems now. It looked like a good idea back then,
>>>> but now it's a massive burden, so I don't want that to proliferate. And
>>>> here I would expect the registers to move around, just like everywhere else.
>>>
>>> That sounds like something that is easily avoided. For example,
>>> Designware doesn't seem to move things around.
>>>
>>> Perhaps MediaTek has a policy of not doing this either?
>>
>> Such policies change as the hardware evolves. I got burned by this
>> struct-based approach more often then it was beneficial, if it ever
>> really was beneficial, hence I don't want to see it used.
> 
> Some benefits:
> - using C struct instead of 'assembler-like' addition is less error-prone

In my experience, this is exactly the opposite. If I have a long
structure describing some register set and I have to count lines to
figure out which register it is at offset e.g. 0x124 , then I am very
likely to make a mistake. However, if I have a macro which looks like
#define REG_FOO 0x124 , I see it right away.

And I am not even talking about modifying such structures, where one has
to over and over recount and verify that nothing got padded the wrong
way. Worse yet, that the structure might just be embedded in some other
super-structure, in which case the superstructure gets padded
differently if the sub-structure changes in size.

Let alone the problematic fact that you might have multiple versions of
the same IP in the system, with just slightly different register
layouts, at which point the structures become ridden with unions {} and
it becomes a total hell.

So no, this argument I am not buying, sorry.

Also, how is C preprocessor macro assembler like ?

> - you can pass the regs struct around between functions in the driver,
> particularly helpful in large drivers

You can pass base addresses around too.

> - lower-case letters are reasier to read

That's a matter of taste.

I find it much easier to identify register offsets (and other macros)
among functions if the offsets are written in uppercase while functions
in lowercase.

> - you can specify the data size and endianness in the struct using C types

Yes, you can, which is not a benefit but another problem, esp. if that
structure gets used on systems where CPU and bus endianness can differ.
(take a look e.g. at the macro monstrosity that is NS16550)

> If the hardware changes enough, then you need a new driver anyway, or
> perhaps separate C implementations of the low-level register access if
> the high-level flow is similar.

Not really, there is a lot of hardware where registers just move around,
but existing driver can very well handle all such instances.

> In general you can't always determine
> this sort of thing at compile time since the version of the hardware
> may not be apparent until run-time. You end up with variables holding
> the offset of each register.

You can use some/any sort of register abstraction to do the remapping.
This is completely independent of how you represent the register offsets.

> Sometimes is it better to have an umbrella driver with a common core.
> 
> So I would push back against a move in this direction in general. It
> depends on the circumstances, and anyway, converting from struct to
> offsets if desirable later is not that hard.

NAK, I am opposed to the struct based access, sorry.

We had massive problems moving toward it in multiple areas (socfpga was
hit very bad and I regret it to this day), only to move back from it
these days because it is borderline impossible to accommodate newly
emerging hardware and we suffered problems outlined above often.

If you want one more argument, then Linux is not using this anywhere and
it has much larger driver base. One might expect Linux to be somewhat
ahead of U-Boot in adopting such techniques.

-- 
Best regards,
Marek Vasut

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

Hi Marek,

On Sat, 21 Mar 2020 at 13:01, Marek Vasut <marex@denx.de> wrote:
>
> On 3/21/20 7:41 PM, Simon Glass wrote:
> > Hi Marek,
>
> Hi,
>
> > On Sat, 21 Mar 2020 at 11:00, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 3/21/20 5:42 PM, Simon Glass wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> On Sat, 21 Mar 2020 at 09:09, Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 3/21/20 3:12 PM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On Wed, 11 Mar 2020 at 01:11, Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> On 3/11/20 7:50 AM, Chunfeng Yun wrote:
> >>>>>> [...]
> >>>>>>> + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
> >>>>>>> + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
> >>>>>>> + * @u2_phy_pll: usb2 phy pll control register
> >>>>>>> + */
> >>>>>>> +struct mtk_ippc_regs {
> >>>>>>> +     __le32 ip_pw_ctr0;
> >>>>>>> +     __le32 ip_pw_ctr1;
> >>>>>>> +     __le32 ip_pw_ctr2;
> >>>>>>
> >>>>>> Please define the registers with #define macros , this struct-based
> >>>>>> approach doesn't scale.
> >>>>>>
> >>>>>
> >>>>> What does this mean? I much prefer the struct approach, unless the
> >>>>> registers move around in future generations.
> >>>>
> >>>> This means I want to see #define MTK_XHCI_<register name> 0xf00
> >>>>
> >>>> The struct based approach doesn't scale and on e.g. altera is causing us
> >>>> a massive amount of problems now. It looked like a good idea back then,
> >>>> but now it's a massive burden, so I don't want that to proliferate. And
> >>>> here I would expect the registers to move around, just like everywhere else.
> >>>
> >>> That sounds like something that is easily avoided. For example,
> >>> Designware doesn't seem to move things around.
> >>>
> >>> Perhaps MediaTek has a policy of not doing this either?
> >>
> >> Such policies change as the hardware evolves. I got burned by this
> >> struct-based approach more often then it was beneficial, if it ever
> >> really was beneficial, hence I don't want to see it used.
> >
> > Some benefits:
> > - using C struct instead of 'assembler-like' addition is less error-prone
>
> In my experience, this is exactly the opposite. If I have a long
> structure describing some register set and I have to count lines to
> figure out which register it is at offset e.g. 0x124 , then I am very
> likely to make a mistake. However, if I have a macro which looks like
> #define REG_FOO 0x124 , I see it right away.

See check_member() though. How do you know it is as offset 0x124?

>
> And I am not even talking about modifying such structures, where one has
> to over and over recount and verify that nothing got padded the wrong
> way. Worse yet, that the structure might just be embedded in some other
> super-structure, in which case the superstructure gets padded
> differently if the sub-structure changes in size.

You always have the choice. You are over-dramatising the difference by
coming up with cases where 'struct' doesn't work. I'm not suggesting
struct should be used always, but to me it has clear benefits in many
drivers in terms of readability and maintainability. We have both
written and maintained a lot of drivers. Also see
ich_init_controller() for a hybrid approach.

>
> Let alone the problematic fact that you might have multiple versions of
> the same IP in the system, with just slightly different register
> layouts, at which point the structures become ridden with unions {} and
> it becomes a total hell.

I don't think structs should be used in that situation though, unless
it is just for a few fields.

>
> So no, this argument I am not buying, sorry.
>
> Also, how is C preprocessor macro assembler like ?

Well in assembler we used to have [BASE + REG_TX], [BASE + REG_RX].
That is what structs are for, so not using them is ignoring a useful
feature of the C language.

>
> > - you can pass the regs struct around between functions in the driver,
> > particularly helpful in large drivers
>
> You can pass base addresses around too.

Untyped though. Then you need casts and defeat the type system, etc.
For example in one driver there are two different register sets and it
is nice to pass just the pointer you need without any ambiguity.

>
> > - lower-case letters are reasier to read
>
> That's a matter of taste.
>
> I find it much easier to identify register offsets (and other macros)
> among functions if the offsets are written in uppercase while functions
> in lowercase.

OK

>
> > - you can specify the data size and endianness in the struct using C types
>
> Yes, you can, which is not a benefit but another problem, esp. if that
> structure gets used on systems where CPU and bus endianness can differ.
> (take a look e.g. at the macro monstrosity that is NS16550)

This is a good example of a driver which should be split out, rather
than cramming all the the little tweaks into ifdefs, etc. Also see
serial_out_dynamic() which heads in that direction. Anyway ns16550 has
to deal with registers being of different sizes. I agree it is a bad
candidate for struct, unless the driver is split.

>
> > If the hardware changes enough, then you need a new driver anyway, or
> > perhaps separate C implementations of the low-level register access if
> > the high-level flow is similar.
>
> Not really, there is a lot of hardware where registers just move around,
> but existing driver can very well handle all such instances.

In my experience things seldom move around and never within the same
SoC generation. In other words there are loads of drivers where it
makes sense to use struct.

>
> > In general you can't always determine
> > this sort of thing at compile time since the version of the hardware
> > may not be apparent until run-time. You end up with variables holding
> > the offset of each register.
>
> You can use some/any sort of register abstraction to do the remapping.
> This is completely independent of how you represent the register offsets.

Yes my point is that you go too far with trying to use the same drive
for different hardware.

>
> > Sometimes is it better to have an umbrella driver with a common core.
> >
> > So I would push back against a move in this direction in general. It
> > depends on the circumstances, and anyway, converting from struct to
> > offsets if desirable later is not that hard.
>
> NAK, I am opposed to the struct based access, sorry.

NAK to what exactly? If you are NAKing using struct when it has the
problems you mention above, I agree with you. If you are NAKing struct
when it has none of these problems, I disagree.

>
> We had massive problems moving toward it in multiple areas (socfpga was
> hit very bad and I regret it to this day), only to move back from it
> these days because it is borderline impossible to accommodate newly
> emerging hardware and we suffered problems outlined above often.

I'm sorry about your socfpga problems. But then, just use offsets, right?

>
> If you want one more argument, then Linux is not using this anywhere and
> it has much larger driver base. One might expect Linux to be somewhat
> ahead of U-Boot in adopting such techniques.

Yes I remember when I first moved from Linux to U-Boot I saw the
struct approach and was very impressed. Has there been any discussion
of moving Linux in that direction? When there is little to no
variability in the register offsets, it is far superior in the right
circumstances I think.

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/21/20 8:34 PM, Simon Glass wrote:
> Hi Marek,

Hi,

> On Sat, 21 Mar 2020 at 13:01, Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/21/20 7:41 PM, Simon Glass wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Sat, 21 Mar 2020 at 11:00, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 3/21/20 5:42 PM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> On Sat, 21 Mar 2020 at 09:09, Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 3/21/20 3:12 PM, Simon Glass wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On Wed, 11 Mar 2020 at 01:11, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 3/11/20 7:50 AM, Chunfeng Yun wrote:
>>>>>>>> [...]
>>>>>>>>> + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
>>>>>>>>> + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
>>>>>>>>> + * @u2_phy_pll: usb2 phy pll control register
>>>>>>>>> + */
>>>>>>>>> +struct mtk_ippc_regs {
>>>>>>>>> +     __le32 ip_pw_ctr0;
>>>>>>>>> +     __le32 ip_pw_ctr1;
>>>>>>>>> +     __le32 ip_pw_ctr2;
>>>>>>>>
>>>>>>>> Please define the registers with #define macros , this struct-based
>>>>>>>> approach doesn't scale.
>>>>>>>>
>>>>>>>
>>>>>>> What does this mean? I much prefer the struct approach, unless the
>>>>>>> registers move around in future generations.
>>>>>>
>>>>>> This means I want to see #define MTK_XHCI_<register name> 0xf00
>>>>>>
>>>>>> The struct based approach doesn't scale and on e.g. altera is causing us
>>>>>> a massive amount of problems now. It looked like a good idea back then,
>>>>>> but now it's a massive burden, so I don't want that to proliferate. And
>>>>>> here I would expect the registers to move around, just like everywhere else.
>>>>>
>>>>> That sounds like something that is easily avoided. For example,
>>>>> Designware doesn't seem to move things around.
>>>>>
>>>>> Perhaps MediaTek has a policy of not doing this either?
>>>>
>>>> Such policies change as the hardware evolves. I got burned by this
>>>> struct-based approach more often then it was beneficial, if it ever
>>>> really was beneficial, hence I don't want to see it used.
>>>
>>> Some benefits:
>>> - using C struct instead of 'assembler-like' addition is less error-prone
>>
>> In my experience, this is exactly the opposite. If I have a long
>> structure describing some register set and I have to count lines to
>> figure out which register it is at offset e.g. 0x124 , then I am very
>> likely to make a mistake. However, if I have a macro which looks like
>> #define REG_FOO 0x124 , I see it right away.
> 
> See check_member() though.

This only adds duplication and more complexity. With this, not only do
you have a structure which is difficult and error-prone to work with,
but you also have a macro which defines the offset, but only for the
purpose of preventing errors originally induced by the complexity of
working with the structure.

So this is complexity on top of complexity.

> How do you know it is as offset 0x124?

I see this offset in the datasheet of the IP/SoC/whatever and/or I can
simply git grep for this value in the sources. And since this value is
there verbatim, I see #define REG_FOO 0x124, simple.

With the complexity presented above, this no longer works. I have to
comb through structures (possibly embedded in other structures)
manually, I cannot directly grep for the register offsets, and it's
complicated, cumbersome and massively error prone.

>> And I am not even talking about modifying such structures, where one has
>> to over and over recount and verify that nothing got padded the wrong
>> way. Worse yet, that the structure might just be embedded in some other
>> super-structure, in which case the superstructure gets padded
>> differently if the sub-structure changes in size.
> 
> You always have the choice. You are over-dramatising the difference by
> coming up with cases where 'struct' doesn't work.

I am coming up with real world experience I have, which tells me that
this does not work in the long run in vast majority of cases and/or is a
massive pain to work with.

Obviously, I come up with cases where this struct based approach fails
and/or displays it's shortcomings, where else would I demonstrate all
the problems with this ?

> I'm not suggesting
> struct should be used always, but to me it has clear benefits in many
> drivers in terms of readability and maintainability. We have both
> written and maintained a lot of drivers. Also see
> ich_init_controller() for a hybrid approach.

Sorry, I just don't see the clear benefit.

Originally I thought this struct-based approach was a good idea too,
which is why I was pushing for it a lot back then, but now I think it
was a big mistake.

>> Let alone the problematic fact that you might have multiple versions of
>> the same IP in the system, with just slightly different register
>> layouts, at which point the structures become ridden with unions {} and
>> it becomes a total hell.
> 
> I don't think structs should be used in that situation though, unless
> it is just for a few fields.

So this would imply you would have drivers which have struct based
access, but then suddenly when the hardware complexity increases, you
would convert them to plain macros ? In that case, we can very well just
stick with plain macros right from the beginning.

>> So no, this argument I am not buying, sorry.
>>
>> Also, how is C preprocessor macro assembler like ?
> 
> Well in assembler we used to have [BASE + REG_TX], [BASE + REG_RX].
> That is what structs are for, so not using them is ignoring a useful
> feature of the C language.

In my experience, this is not helpful at all, it only is in the way and
is cumbersome to deal with.

The base + offset is easy to calculate, which makes it easy and straight
forward to work with and to debug. But trying to find out what the type
of some structure is, and then calculate at what offset in &foo->bar the
BAR register really is, awful and error prone.

>>> - you can pass the regs struct around between functions in the driver,
>>> particularly helpful in large drivers
>>
>> You can pass base addresses around too.
> 
> Untyped though. Then you need casts and defeat the type system, etc.
> For example in one driver there are two different register sets and it
> is nice to pass just the pointer you need without any ambiguity.

Or, you have a driver which handles two different version of the same
IP, with different register layouts. Suddenly you have two structures
and massive amount of casts.

>>> - lower-case letters are reasier to read
>>
>> That's a matter of taste.
>>
>> I find it much easier to identify register offsets (and other macros)
>> among functions if the offsets are written in uppercase while functions
>> in lowercase.
> 
> OK
> 
>>
>>> - you can specify the data size and endianness in the struct using C types
>>
>> Yes, you can, which is not a benefit but another problem, esp. if that
>> structure gets used on systems where CPU and bus endianness can differ.
>> (take a look e.g. at the macro monstrosity that is NS16550)
> 
> This is a good example of a driver which should be split out, rather
> than cramming all the the little tweaks into ifdefs, etc. Also see
> serial_out_dynamic() which heads in that direction. Anyway ns16550 has
> to deal with registers being of different sizes. I agree it is a bad
> candidate for struct, unless the driver is split.

It's a great driver for using macros though.

>>> If the hardware changes enough, then you need a new driver anyway, or
>>> perhaps separate C implementations of the low-level register access if
>>> the high-level flow is similar.
>>
>> Not really, there is a lot of hardware where registers just move around,
>> but existing driver can very well handle all such instances.
> 
> In my experience things seldom move around and never within the same
> SoC generation. In other words there are loads of drivers where it
> makes sense to use struct.

My experience is the exact opposite.

>>> In general you can't always determine
>>> this sort of thing at compile time since the version of the hardware
>>> may not be apparent until run-time. You end up with variables holding
>>> the offset of each register.
>>
>> You can use some/any sort of register abstraction to do the remapping.
>> This is completely independent of how you represent the register offsets.
> 
> Yes my point is that you go too far with trying to use the same drive
> for different hardware.

No. If the hardware is basically the same , except registers move
slightly , then it only makes sense to reuse the same driver.

>>> Sometimes is it better to have an umbrella driver with a common core.
>>>
>>> So I would push back against a move in this direction in general. It
>>> depends on the circumstances, and anyway, converting from struct to
>>> offsets if desirable later is not that hard.
>>
>> NAK, I am opposed to the struct based access, sorry.
> 
> NAK to what exactly? If you are NAKing using struct when it has the
> problems you mention above, I agree with you. If you are NAKing struct
> when it has none of these problems, I disagree.

I am NAKing the use of structs here, because in my experience this will
come back to bite us later on and because I don't see any benefits from
using them.

We can revisit this discussion a few years down the road and see who was
right if you want.

>> We had massive problems moving toward it in multiple areas (socfpga was
>> hit very bad and I regret it to this day), only to move back from it
>> these days because it is borderline impossible to accommodate newly
>> emerging hardware and we suffered problems outlined above often.
> 
> I'm sorry about your socfpga problems. But then, just use offsets, right?

offsets ?

>> If you want one more argument, then Linux is not using this anywhere and
>> it has much larger driver base. One might expect Linux to be somewhat
>> ahead of U-Boot in adopting such techniques.
> 
> Yes I remember when I first moved from Linux to U-Boot I saw the
> struct approach and was very impressed. Has there been any discussion
> of moving Linux in that direction? When there is little to no
> variability in the register offsets, it is far superior in the right
> circumstances I think.

Most embedded hardware has this variability it seems.
Feel free to start debating this on the Linux side.

-- 
Best regards,
Marek Vasut

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

Hi Marek,

I think at this point we've covered all the ground and mentioned the
pros and cons of each method, so I'll leave the discussion where it
is.

[..]

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/22/20 3:08 AM, Simon Glass wrote:
> Hi Marek,

Hi,

> I think at this point we've covered all the ground and mentioned the
> pros and cons of each method, so I'll leave the discussion where it
> is.

Great, so let's remove the struct-based access from the driver and use
regular #define REGISTER 0xoffset.

Thank you.

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

Hi Marek,

On Sat, 21 Mar 2020 at 20:15, Marek Vasut <marex@denx.de> wrote:
>
> On 3/22/20 3:08 AM, Simon Glass wrote:
> > Hi Marek,
>
> Hi,
>
> > I think at this point we've covered all the ground and mentioned the
> > pros and cons of each method, so I'll leave the discussion where it
> > is.
>
> Great, so let's remove the struct-based access from the driver and use
> regular #define REGISTER 0xoffset.

I think any individual decision depends on the pros and cons we
outlined in our discussion. I don't have any information to suggest
that the Mediatek XHCI driver has any of the variations you talked
about in your worst-case scenarios, so I can't comment on that. I am
more concerned about this as a general rule as I feel that the
struct-based approach is generally best for U-Boot, except for the
cases you highlighted:

- where the registers appear at different offsets in different
hardware revisions served by the same driver
- where the driver only uses a small subset of the registers and it is
not worth defining a struct to cover them all, with associated empty
regions, etc.

Anything else?

This is a USB driver and you are the USB maintainer, so your decision
is OK with me. For driver model in general I feel that struct access
should be the default, but individual maintainers with strong views on
their subsystem need to have preference.

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/22/20 4:17 PM, Simon Glass wrote:
> Hi Marek,

Hi,

> On Sat, 21 Mar 2020 at 20:15, Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/22/20 3:08 AM, Simon Glass wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> I think at this point we've covered all the ground and mentioned the
>>> pros and cons of each method, so I'll leave the discussion where it
>>> is.
>>
>> Great, so let's remove the struct-based access from the driver and use
>> regular #define REGISTER 0xoffset.
> 
> I think any individual decision depends on the pros and cons we
> outlined in our discussion. I don't have any information to suggest
> that the Mediatek XHCI driver has any of the variations you talked
> about in your worst-case scenarios, so I can't comment on that. I am
> more concerned about this as a general rule as I feel that the
> struct-based approach is generally best for U-Boot, except for the
> cases you highlighted:
> 
> - where the registers appear at different offsets in different
> hardware revisions served by the same driver
> - where the driver only uses a small subset of the registers and it is
> not worth defining a struct to cover them all, with associated empty
> regions, etc.
> 
> Anything else?

It's also very difficult to easily figure out the address of a register
that's buried somewhere down in a long structure, possibly with embedded
sub-structures.

> This is a USB driver and you are the USB maintainer, so your decision
> is OK with me. For driver model in general I feel that struct access
> should be the default, but individual maintainers with strong views on
> their subsystem need to have preference.

Well, like I said, my experience tells me the struct approach was a big
mistake in multiple places, so I would prefer macros here.

-- 
Best regards,
Marek Vasut

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Chunfeng Yun]

Hi Marek & Simon,


Firstly, thanks for your suggestion and discussion.

As Simon guess, MediaTek indeed has some policies to avoid to move
registers around for USB IP, I think we will not encounter the
worst-case scenarios as Marek mentioned. Due to there is only a little
registers, both struct and macros approach are OK for me.

And in a word, I tend to agree with Simon's opinion, choose struct or
macro approach, case by case, after weigh the pros and cons. But Marek
is the maintainer of USB subsystem, so I'd better to make him happy, and
should follow his suggetions:)

Again, thank you guys


On Sun, 2020-03-22 at 16:34 +0100, Marek Vasut wrote:
> On 3/22/20 4:17 PM, Simon Glass wrote:
> > Hi Marek,
> 
> Hi,
> 
> > On Sat, 21 Mar 2020 at 20:15, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 3/22/20 3:08 AM, Simon Glass wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> I think at this point we've covered all the ground and mentioned the
> >>> pros and cons of each method, so I'll leave the discussion where it
> >>> is.
> >>
> >> Great, so let's remove the struct-based access from the driver and use
> >> regular #define REGISTER 0xoffset.
> > 
> > I think any individual decision depends on the pros and cons we
> > outlined in our discussion. I don't have any information to suggest
> > that the Mediatek XHCI driver has any of the variations you talked
> > about in your worst-case scenarios, so I can't comment on that. I am
> > more concerned about this as a general rule as I feel that the
> > struct-based approach is generally best for U-Boot, except for the
> > cases you highlighted:
> > 
> > - where the registers appear at different offsets in different
> > hardware revisions served by the same driver
> > - where the driver only uses a small subset of the registers and it is
> > not worth defining a struct to cover them all, with associated empty
> > regions, etc.
> > 
> > Anything else?
> 
> It's also very difficult to easily figure out the address of a register
> that's buried somewhere down in a long structure, possibly with embedded
> sub-structures.
> 
> > This is a USB driver and you are the USB maintainer, so your decision
> > is OK with me. For driver model in general I feel that struct access
> > should be the default, but individual maintainers with strong views on
> > their subsystem need to have preference.
> 
> Well, like I said, my experience tells me the struct approach was a big
> mistake in multiple places, so I would prefer macros here.
> 

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

Hi Marek,

On Sun, 22 Mar 2020 at 09:34, Marek Vasut <marex@denx.de> wrote:
>
> On 3/22/20 4:17 PM, Simon Glass wrote:
> > Hi Marek,
>
> Hi,
>
> > On Sat, 21 Mar 2020 at 20:15, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 3/22/20 3:08 AM, Simon Glass wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> I think at this point we've covered all the ground and mentioned the
> >>> pros and cons of each method, so I'll leave the discussion where it
> >>> is.
> >>
> >> Great, so let's remove the struct-based access from the driver and use
> >> regular #define REGISTER 0xoffset.
> >
> > I think any individual decision depends on the pros and cons we
> > outlined in our discussion. I don't have any information to suggest
> > that the Mediatek XHCI driver has any of the variations you talked
> > about in your worst-case scenarios, so I can't comment on that. I am
> > more concerned about this as a general rule as I feel that the
> > struct-based approach is generally best for U-Boot, except for the
> > cases you highlighted:
> >
> > - where the registers appear at different offsets in different
> > hardware revisions served by the same driver
> > - where the driver only uses a small subset of the registers and it is
> > not worth defining a struct to cover them all, with associated empty
> > regions, etc.
> >
> > Anything else?
>
> It's also very difficult to easily figure out the address of a register
> that's buried somewhere down in a long structure, possibly with embedded
> sub-structures.

OK I have updated the coding style page with all of this.

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/31/20 1:31 AM, Simon Glass wrote:
> Hi Marek,

Hi,

> On Sun, 22 Mar 2020 at 09:34, Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/22/20 4:17 PM, Simon Glass wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Sat, 21 Mar 2020 at 20:15, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 3/22/20 3:08 AM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> I think at this point we've covered all the ground and mentioned the
>>>>> pros and cons of each method, so I'll leave the discussion where it
>>>>> is.
>>>>
>>>> Great, so let's remove the struct-based access from the driver and use
>>>> regular #define REGISTER 0xoffset.
>>>
>>> I think any individual decision depends on the pros and cons we
>>> outlined in our discussion. I don't have any information to suggest
>>> that the Mediatek XHCI driver has any of the variations you talked
>>> about in your worst-case scenarios, so I can't comment on that. I am
>>> more concerned about this as a general rule as I feel that the
>>> struct-based approach is generally best for U-Boot, except for the
>>> cases you highlighted:
>>>
>>> - where the registers appear at different offsets in different
>>> hardware revisions served by the same driver
>>> - where the driver only uses a small subset of the registers and it is
>>> not worth defining a struct to cover them all, with associated empty
>>> regions, etc.
>>>
>>> Anything else?
>>
>> It's also very difficult to easily figure out the address of a register
>> that's buried somewhere down in a long structure, possibly with embedded
>> sub-structures.
> 
> OK I have updated the coding style page with all of this.

Which page ?

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

Hi Marek,

On Mon, 30 Mar 2020 at 18:39, Marek Vasut <marex@denx.de> wrote:
>
> On 3/31/20 1:31 AM, Simon Glass wrote:
> > Hi Marek,
>
> Hi,
>
> > On Sun, 22 Mar 2020 at 09:34, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 3/22/20 4:17 PM, Simon Glass wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> On Sat, 21 Mar 2020 at 20:15, Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 3/22/20 3:08 AM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>
> >>>> Hi,
> >>>>
> >>>>> I think at this point we've covered all the ground and mentioned the
> >>>>> pros and cons of each method, so I'll leave the discussion where it
> >>>>> is.
> >>>>
> >>>> Great, so let's remove the struct-based access from the driver and use
> >>>> regular #define REGISTER 0xoffset.
> >>>
> >>> I think any individual decision depends on the pros and cons we
> >>> outlined in our discussion. I don't have any information to suggest
> >>> that the Mediatek XHCI driver has any of the variations you talked
> >>> about in your worst-case scenarios, so I can't comment on that. I am
> >>> more concerned about this as a general rule as I feel that the
> >>> struct-based approach is generally best for U-Boot, except for the
> >>> cases you highlighted:
> >>>
> >>> - where the registers appear at different offsets in different
> >>> hardware revisions served by the same driver
> >>> - where the driver only uses a small subset of the registers and it is
> >>> not worth defining a struct to cover them all, with associated empty
> >>> regions, etc.
> >>>
> >>> Anything else?
> >>
> >> It's also very difficult to easily figure out the address of a register
> >> that's buried somewhere down in a long structure, possibly with embedded
> >> sub-structures.
> >
> > OK I have updated the coding style page with all of this.
>
> Which page ?

https://www.denx.de/wiki/U-Boot/CodingStyle

Separately, I sent a patch a while back which updated checkpatch for
U-Boot. I got some pushback, but I think that was wrong and we should
do it. For example I am saying many of the same things in code reviews
and many of them could be caught by the script. Examples include using
if() instead of #if where possible.

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/31/20 3:24 PM, Simon Glass wrote:
> Hi Marek,
> 
> On Mon, 30 Mar 2020 at 18:39, Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/31/20 1:31 AM, Simon Glass wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Sun, 22 Mar 2020 at 09:34, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 3/22/20 4:17 PM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> On Sat, 21 Mar 2020 at 20:15, Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 3/22/20 3:08 AM, Simon Glass wrote:
>>>>>>> Hi Marek,
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> I think at this point we've covered all the ground and mentioned the
>>>>>>> pros and cons of each method, so I'll leave the discussion where it
>>>>>>> is.
>>>>>>
>>>>>> Great, so let's remove the struct-based access from the driver and use
>>>>>> regular #define REGISTER 0xoffset.
>>>>>
>>>>> I think any individual decision depends on the pros and cons we
>>>>> outlined in our discussion. I don't have any information to suggest
>>>>> that the Mediatek XHCI driver has any of the variations you talked
>>>>> about in your worst-case scenarios, so I can't comment on that. I am
>>>>> more concerned about this as a general rule as I feel that the
>>>>> struct-based approach is generally best for U-Boot, except for the
>>>>> cases you highlighted:
>>>>>
>>>>> - where the registers appear at different offsets in different
>>>>> hardware revisions served by the same driver
>>>>> - where the driver only uses a small subset of the registers and it is
>>>>> not worth defining a struct to cover them all, with associated empty
>>>>> regions, etc.
>>>>>
>>>>> Anything else?
>>>>
>>>> It's also very difficult to easily figure out the address of a register
>>>> that's buried somewhere down in a long structure, possibly with embedded
>>>> sub-structures.
>>>
>>> OK I have updated the coding style page with all of this.
>>
>> Which page ?
> 
> https://www.denx.de/wiki/U-Boot/CodingStyle

" U-Boot typically uses a C structure to map out the registers in an I/O
region, rather than offsets. The reasons for this are: " is misleading
and suggests that using structures is the best practice. This should be
reworded to make it clear both options are equally valid.

> Separately, I sent a patch a while back which updated checkpatch for
> U-Boot. I got some pushback

Link ?

I am very much opposed to this struct-based approach, so there is my
pushback too. I think we should NOT do it.

>, but I think that was wrong and we should
> do it. For example I am saying many of the same things in code reviews
> and many of them could be caught by the script. Examples include using
> if() instead of #if where possible.
> 
> Regards,
> Simon
> 


-- 
Best regards,
Marek Vasut

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

HI Marek,

On Tue, 31 Mar 2020 at 07:29, Marek Vasut <marex@denx.de> wrote:
>
> On 3/31/20 3:24 PM, Simon Glass wrote:
> > Hi Marek,
> >
> > On Mon, 30 Mar 2020 at 18:39, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 3/31/20 1:31 AM, Simon Glass wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> On Sun, 22 Mar 2020 at 09:34, Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 3/22/20 4:17 PM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>
> >>>> Hi,
> >>>>
> >>>>> On Sat, 21 Mar 2020 at 20:15, Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> On 3/22/20 3:08 AM, Simon Glass wrote:
> >>>>>>> Hi Marek,
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>>> I think at this point we've covered all the ground and mentioned the
> >>>>>>> pros and cons of each method, so I'll leave the discussion where it
> >>>>>>> is.
> >>>>>>
> >>>>>> Great, so let's remove the struct-based access from the driver and use
> >>>>>> regular #define REGISTER 0xoffset.
> >>>>>
> >>>>> I think any individual decision depends on the pros and cons we
> >>>>> outlined in our discussion. I don't have any information to suggest
> >>>>> that the Mediatek XHCI driver has any of the variations you talked
> >>>>> about in your worst-case scenarios, so I can't comment on that. I am
> >>>>> more concerned about this as a general rule as I feel that the
> >>>>> struct-based approach is generally best for U-Boot, except for the
> >>>>> cases you highlighted:
> >>>>>
> >>>>> - where the registers appear at different offsets in different
> >>>>> hardware revisions served by the same driver
> >>>>> - where the driver only uses a small subset of the registers and it is
> >>>>> not worth defining a struct to cover them all, with associated empty
> >>>>> regions, etc.
> >>>>>
> >>>>> Anything else?
> >>>>
> >>>> It's also very difficult to easily figure out the address of a register
> >>>> that's buried somewhere down in a long structure, possibly with embedded
> >>>> sub-structures.
> >>>
> >>> OK I have updated the coding style page with all of this.
> >>
> >> Which page ?
> >
> > https://www.denx.de/wiki/U-Boot/CodingStyle
>
> " U-Boot typically uses a C structure to map out the registers in an I/O
> region, rather than offsets. The reasons for this are: " is misleading
> and suggests that using structures is the best practice. This should be
> reworded to make it clear both options are equally valid.

I'd like to see a preference to use struct where it makes sense and
not use it when it doesn't, with the different tradeoffs clearly
written. Are asking that we say nothing about which is better in each
situation?

>
> > Separately, I sent a patch a while back which updated checkpatch for
> > U-Boot. I got some pushback
>
> Link ?
>

http://patchwork.ozlabs.org/patch/999510/

> I am very much opposed to this struct-based approach, so there is my
> pushback too. I think we should NOT do it.

Right, but there are arguments on both sides. I strongly prefer it
where it makes sense, for reasons that we've already discussed.

>
> >, but I think that was wrong and we should
> > do it. For example I am saying many of the same things in code reviews
> > and many of them could be caught by the script. Examples include using
> > if() instead of #if where possible.
> >
> > Regards,
> > Simon
> >

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/31/20 4:16 PM, Simon Glass wrote:
> HI Marek,

Hi,

[...]

>>>>> OK I have updated the coding style page with all of this.
>>>>
>>>> Which page ?
>>>
>>> https://www.denx.de/wiki/U-Boot/CodingStyle
>>
>> " U-Boot typically uses a C structure to map out the registers in an I/O
>> region, rather than offsets. The reasons for this are: " is misleading
>> and suggests that using structures is the best practice. This should be
>> reworded to make it clear both options are equally valid.
> 
> I'd like to see a preference to use struct where it makes sense and
> not use it when it doesn't, with the different tradeoffs clearly
> written. Are asking that we say nothing about which is better in each
> situation?

Correct, because I don't see a clear agreement on which one is better
and should be preferred.

>>> Separately, I sent a patch a while back which updated checkpatch for
>>> U-Boot. I got some pushback
>>
>> Link ?
>>
> 
> http://patchwork.ozlabs.org/patch/999510/

OK, this has nothing to do with structs, right ?

>> I am very much opposed to this struct-based approach, so there is my
>> pushback too. I think we should NOT do it.
> 
> Right, but there are arguments on both sides. I strongly prefer it
> where it makes sense, for reasons that we've already discussed.

Your preference does not mean it is also the project preference though.
If you want to decide on that, make sure there is some agreement first.

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/31/20 6:05 PM, Marek Vasut wrote:
> On 3/31/20 4:16 PM, Simon Glass wrote:
>> HI Marek,
> 
> Hi,
> 
> [...]
> 
>>>>>> OK I have updated the coding style page with all of this.
>>>>>
>>>>> Which page ?
>>>>
>>>> https://www.denx.de/wiki/U-Boot/CodingStyle
>>>
>>> " U-Boot typically uses a C structure to map out the registers in an I/O
>>> region, rather than offsets. The reasons for this are: " is misleading
>>> and suggests that using structures is the best practice. This should be
>>> reworded to make it clear both options are equally valid.
>>
>> I'd like to see a preference to use struct where it makes sense and
>> not use it when it doesn't, with the different tradeoffs clearly
>> written. Are asking that we say nothing about which is better in each
>> situation?
> 
> Correct, because I don't see a clear agreement on which one is better
> and should be preferred.

I see the wiki has still not been correctly reworded, can you please
repair it ?

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

Hi Marek,

On Fri, 10 Apr 2020 at 12:45, Marek Vasut <marex@denx.de> wrote:
>
> On 3/31/20 6:05 PM, Marek Vasut wrote:
> > On 3/31/20 4:16 PM, Simon Glass wrote:
> >> HI Marek,
> >
> > Hi,
> >
> > [...]
> >
> >>>>>> OK I have updated the coding style page with all of this.
> >>>>>
> >>>>> Which page ?
> >>>>
> >>>> https://www.denx.de/wiki/U-Boot/CodingStyle
> >>>
> >>> " U-Boot typically uses a C structure to map out the registers in an I/O
> >>> region, rather than offsets. The reasons for this are: " is misleading
> >>> and suggests that using structures is the best practice. This should be
> >>> reworded to make it clear both options are equally valid.
> >>
> >> I'd like to see a preference to use struct where it makes sense and
> >> not use it when it doesn't, with the different tradeoffs clearly
> >> written. Are asking that we say nothing about which is better in each
> >> situation?
> >
> > Correct, because I don't see a clear agreement on which one is better
> > and should be preferred.
>
> I see the wiki has still not been correctly reworded, can you please
> repair it ?

What sort of 'repairs' are you thinking of?

I did add your counterpoints but did not change the text in the first
part, which has been long-standing. I think to do that we really would
need a wider discussion than just the two of us, particularly as we
seem not to agree on the 'default' position.

Regards,
Simon

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 4/10/20 9:14 PM, Simon Glass wrote:
> Hi Marek,

Hi,

> On Fri, 10 Apr 2020 at 12:45, Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/31/20 6:05 PM, Marek Vasut wrote:
>>> On 3/31/20 4:16 PM, Simon Glass wrote:
>>>> HI Marek,
>>>
>>> Hi,
>>>
>>> [...]
>>>
>>>>>>>> OK I have updated the coding style page with all of this.
>>>>>>>
>>>>>>> Which page ?
>>>>>>
>>>>>> https://www.denx.de/wiki/U-Boot/CodingStyle
>>>>>
>>>>> " U-Boot typically uses a C structure to map out the registers in an I/O
>>>>> region, rather than offsets. The reasons for this are: " is misleading
>>>>> and suggests that using structures is the best practice. This should be
>>>>> reworded to make it clear both options are equally valid.
>>>>
>>>> I'd like to see a preference to use struct where it makes sense and
>>>> not use it when it doesn't, with the different tradeoffs clearly
>>>> written. Are asking that we say nothing about which is better in each
>>>> situation?
>>>
>>> Correct, because I don't see a clear agreement on which one is better
>>> and should be preferred.
>>
>> I see the wiki has still not been correctly reworded, can you please
>> repair it ?
> 
> What sort of 'repairs' are you thinking of?

"
 This may need to change to the kernel model if we allow for more
run-time detection of what drivers are appropriate for what we're
running on.
"

This was removed. I believe this is quite important to leave there.

> I did add your counterpoints but did not change the text in the first
> part, which has been long-standing. I think to do that we really would
> need a wider discussion than just the two of us, particularly as we
> seem not to agree on the 'default' position.

ACK.

[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

Hi Marek,

On Fri, 10 Apr 2020 at 13:57, Marek Vasut <marex@denx.de> wrote:
>
> On 4/10/20 9:14 PM, Simon Glass wrote:
> > Hi Marek,
>
> Hi,
>
> > On Fri, 10 Apr 2020 at 12:45, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 3/31/20 6:05 PM, Marek Vasut wrote:
> >>> On 3/31/20 4:16 PM, Simon Glass wrote:
> >>>> HI Marek,
> >>>
> >>> Hi,
> >>>
> >>> [...]
> >>>
> >>>>>>>> OK I have updated the coding style page with all of this.
> >>>>>>>
> >>>>>>> Which page ?
> >>>>>>
> >>>>>> https://www.denx.de/wiki/U-Boot/CodingStyle
> >>>>>
> >>>>> " U-Boot typically uses a C structure to map out the registers in an I/O
> >>>>> region, rather than offsets. The reasons for this are: " is misleading
> >>>>> and suggests that using structures is the best practice. This should be
> >>>>> reworded to make it clear both options are equally valid.
> >>>>
> >>>> I'd like to see a preference to use struct where it makes sense and
> >>>> not use it when it doesn't, with the different tradeoffs clearly
> >>>> written. Are asking that we say nothing about which is better in each
> >>>> situation?
> >>>
> >>> Correct, because I don't see a clear agreement on which one is better
> >>> and should be preferred.
> >>
> >> I see the wiki has still not been correctly reworded, can you please
> >> repair it ?
> >
> > What sort of 'repairs' are you thinking of?
>
> "
>  This may need to change to the kernel model if we allow for more
> run-time detection of what drivers are appropriate for what we're
> running on.
> "
>
> This was removed. I believe this is quite important to leave there.

OK it seemed to repeat the first point, but I suppose it is saying it
a different way. I updated it.

>> > I did add your counterpoints but did not change the text in the first
> > part, which has been long-standing. I think to do that we really would
> > need a wider discussion than just the two of us, particularly as we
> > seem not to agree on the 'default' position.
>
> ACK.

Regards,
Simon