On Wed, Jun 07, 2023 at 03:29:18PM +0300, Dmitry Baryshkov wrote:

Two minor nits on top of the review:

On 07/06/2023 14:54, Konrad Dybcio wrote:

quoted

On 7.06.2023 12:56, Varadarajan Narayanan wrote:

quoted

Add the M31 USB2 phy driver

Signed-off-by: Varadarajan Narayanan <redacted>
---
 drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
 1 file changed, 360 insertions(+)
 create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c

diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
new file mode 100644
index 0000000..d29a91e
--- /dev/null
+++ b/drivers/phy/qualcomm/phy-qcom-m31.c

@@ -0,0 +1,360 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/usb/phy.h>
+#include <linux/reset.h>
+#include <linux/of_device.h>

Please sort these

quoted

+
+enum clk_reset_action {
+	CLK_RESET_DEASSERT	= 0,
+	CLK_RESET_ASSERT	= 1
+};
+
+#define USB2PHY_PORT_POWERDOWN		0xA4
+#define POWER_UP			BIT(0)
+#define POWER_DOWN			0
+
+#define USB2PHY_PORT_UTMI_CTRL1	0x40
+
+#define USB2PHY_PORT_UTMI_CTRL2	0x44
+#define UTMI_ULPI_SEL			BIT(7)
+#define UTMI_TEST_MUX_SEL		BIT(6)
+
+#define HS_PHY_CTRL_REG			0x10
+#define UTMI_OTG_VBUS_VALID             BIT(20)
+#define SW_SESSVLD_SEL                  BIT(28)
+
+#define USB_PHY_CFG0			0x94
+#define USB_PHY_UTMI_CTRL5		0x50
+#define USB_PHY_FSEL_SEL		0xB8
+#define USB_PHY_HS_PHY_CTRL_COMMON0	0x54
+#define USB_PHY_REFCLK_CTRL		0xA0
+#define USB_PHY_HS_PHY_CTRL2		0x64
+#define USB_PHY_UTMI_CTRL0		0x3c
+#define USB2PHY_USB_PHY_M31_XCFGI_1	0xBC
+#define USB2PHY_USB_PHY_M31_XCFGI_4	0xC8
+#define USB2PHY_USB_PHY_M31_XCFGI_5	0xCC
+#define USB2PHY_USB_PHY_M31_XCFGI_11	0xE4

Could you sort them address-wise?

... and lowercase the hex values, please.

Ok.

quoted

quoted

+
+#define USB2_0_TX_ENABLE		BIT(2)
+#define HSTX_SLEW_RATE_565PS		3
+#define PLL_CHARGING_PUMP_CURRENT_35UA	(3 << 3)
+#define ODT_VALUE_38_02_OHM		(3 << 6)
+#define ODT_VALUE_45_02_OHM		BIT(2)
+#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA	(1)

Weird mix of values, bits, bitfields.. perhaps BIT(n) and
GENMASK() (+ FIELD_PREP) would be more suitable?

quoted

+
+#define UTMI_PHY_OVERRIDE_EN		BIT(1)
+#define POR_EN				BIT(1)

Please associate these with their registers, like

#define FOO_REG		0xf00
 #define POR_EN		BIT(1)

quoted

+#define FREQ_SEL			BIT(0)
+#define COMMONONN			BIT(7)
+#define FSEL				BIT(4)
+#define RETENABLEN			BIT(3)
+#define USB2_SUSPEND_N_SEL		BIT(3)
+#define USB2_SUSPEND_N			BIT(2)
+#define USB2_UTMI_CLK_EN		BIT(1)
+#define CLKCORE				BIT(1)
+#define ATERESET			~BIT(0)
+#define FREQ_24MHZ			(5 << 4)
+#define XCFG_COARSE_TUNE_NUM		(2 << 0)
+#define XCFG_FINE_TUNE_NUM		(1 << 3)

same comment

quoted

+
+static void m31usb_write_readback(void *base, u32 offset,
+					const u32 mask, u32 val);

We don't need this forward-definition, just move the function up.

quoted

+
+struct m31usb_phy {
+	struct usb_phy		phy;
+	void __iomem		*base;
+	void __iomem		*qscratch_base;
+
+	struct reset_control	*phy_reset;
+
+	bool			cable_connected;
+	bool			suspended;
+	bool			ulpi_mode;
+};
+
+static void m31usb_reset(struct m31usb_phy *qphy, u32 action)
+{
+	if (action == CLK_RESET_ASSERT)
+		reset_control_assert(qphy->phy_reset);
+	else
+		reset_control_deassert(qphy->phy_reset);
+	wmb(); /* ensure data is written to hw register */

Please move the comment above the call.

quoted

+}

Or even better just inline the function. I was never a fan of such
multiplexers.

Also does wmb() make sense here? Doesn't regmap (which is used by reset
controller) remove the need for it?

Will inline and remove the wmb.

Thanks
Varada

quoted

quoted

+
+static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
+{
+	/* Enable override ctrl */
+	writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);

Some of the comments are missing a space before '*/'

Also, please consider adding some newlines to logically split the
actions.

--
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel