[PATCH v2 0/3] Register at91 OHCI into DM and add SAMA7 USB PHY's

[PATCH v2 0/3] Register at91 OHCI into DM and add SAMA7 USB PHY's

[Sergiu Moga]

This patch series originates from a bigger patch series:
https://lists.denx.de/pipermail/u-boot/2022-December/502865.html

Register ohci-at91 driver into Driver Model. In order for the VBUS to
stay enabled, a `child_pre_probe` method has been added to overcome the
DM core disabling it in `usb_scan_device`: when the generic `device_probe`
method is called, the pinctrl is processed once again, undoing whatever
changes have been made in our driver's probe method.
In order to enable USB on SAMA7G5 the addition of the USB 2.0 PHY
drivers were required.

v1 -> v2:
- squashed previous 3/4 patch into 2/4 patch
- used *_bulk API's
- re-wrote at91_for_each_port
- use dev_read_u32_default for pdata->ports

Sergiu Moga (3):
  phy: at91: Add support for the USB 2.0 PHY's of SAMA7
  usb: ohci-at91: Enable OHCI functionality and register into DM
  usb: ohci-at91: Add USB PHY functionality

 drivers/phy/Kconfig              |  10 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-sama7-usb.c      |  90 ++++++++++++++
 drivers/phy/phy-sama7-utmi-clk.c | 202 +++++++++++++++++++++++++++++++
 drivers/usb/host/Kconfig         |   1 +
 drivers/usb/host/ohci-at91.c     | 185 ++++++++++++++++++++++++++++
 6 files changed, 489 insertions(+)
 create mode 100644 drivers/phy/phy-sama7-usb.c
 create mode 100644 drivers/phy/phy-sama7-utmi-clk.c

-- 
2.34.1

[PATCH v2 1/3] phy: at91: Add support for the USB 2.0 PHY's of SAMA7

[Sergiu Moga]

In order to have USB functionality, drivers for SAMA7's
USB 2.0 PHY's have been added. There is one driver
for UTMI clock's SFR and RESET required functionalities and
one for its three possible subclocks of the phy's themselves.
In order for this layout to properly work in conjunction with
CCF and DT, the former driver will also act as a clock provider
for the three phy's with the help of a custom hook into the
driver's of_xlate method.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
Tested-by: Mihai Sain <mihai.sain@microchip.com>
---


v1 -> v2:
- Nothing


 drivers/phy/Kconfig              |  10 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-sama7-usb.c      |  90 ++++++++++++++
 drivers/phy/phy-sama7-utmi-clk.c | 202 +++++++++++++++++++++++++++++++
 4 files changed, 303 insertions(+)
 create mode 100644 drivers/phy/phy-sama7-usb.c
 create mode 100644 drivers/phy/phy-sama7-utmi-clk.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index cf4d5908d7..9fbb956783 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -281,6 +281,16 @@ config PHY_XILINX_ZYNQMP
 	  Enable this to support ZynqMP High Speed Gigabit Transceiver
 	  that is part of ZynqMP SoC.
 
+config PHY_MICROCHIP_SAMA7_USB
+       tristate "Microchip SAMA7 USB 2.0 PHY"
+       depends on PHY && ARCH_AT91
+       help
+	 Enable this to support SAMA7 USB 2.0 PHY.
+
+	 The USB 2.0 PHY integrates high-speed, full-speed and low-speed
+	 termination and signal switching. With a single resistor, it
+	 requires minimal external components.
+
 source "drivers/phy/rockchip/Kconfig"
 source "drivers/phy/cadence/Kconfig"
 source "drivers/phy/ti/Kconfig"
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index a3b9f3c5b1..9d50affd47 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_PHY_MTK_TPHY) += phy-mtk-tphy.o
 obj-$(CONFIG_PHY_NPCM_USB) += phy-npcm-usb.o
 obj-$(CONFIG_PHY_IMX8MQ_USB) += phy-imx8mq-usb.o
 obj-$(CONFIG_PHY_XILINX_ZYNQMP) += phy-zynqmp.o
+obj-$(CONFIG_PHY_MICROCHIP_SAMA7_USB)  += phy-sama7-utmi-clk.o phy-sama7-usb.o
 obj-y += cadence/
 obj-y += ti/
 obj-y += qcom/
diff --git a/drivers/phy/phy-sama7-usb.c b/drivers/phy/phy-sama7-usb.c
new file mode 100644
index 0000000000..200324d812
--- /dev/null
+++ b/drivers/phy/phy-sama7-usb.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Support for Atmel/Microchip USB PHY's.
+ *
+ * Copyright (C) 2022 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Sergiu Moga <sergiu.moga@microchip.com>
+ */
+
+#include <clk.h>
+#include <dm.h>
+#include <generic-phy.h>
+#include <syscon.h>
+#include <regmap.h>
+#include <mach/sama7-sfr.h>
+
+struct sama7_usb_phy {
+	struct clk *uclk;
+	struct regmap *sfr;
+	int port;
+};
+
+static int sama7_usb_phy_init(struct phy *phy)
+{
+	struct sama7_usb_phy *sama7_phy = dev_get_priv(phy->dev);
+	int port = sama7_phy->port;
+
+	regmap_update_bits(sama7_phy->sfr, SAMA7_SFR_UTMI0R(port),
+			   SAMA7_SFR_UTMI_RX_TX_PREEM_AMP_TUNE_1X,
+			   SAMA7_SFR_UTMI_RX_TX_PREEM_AMP_TUNE_1X);
+
+	regmap_update_bits(sama7_phy->sfr, SAMA7_SFR_UTMI0R(port),
+			   SAMA7_SFR_UTMI_RX_VBUS,
+			   SAMA7_SFR_UTMI_RX_VBUS);
+
+	return 0;
+}
+
+static int sama7_phy_power_on(struct phy *phy)
+{
+	struct sama7_usb_phy *sama7_phy = dev_get_priv(phy->dev);
+
+	clk_prepare_enable(sama7_phy->uclk);
+
+	return 0;
+}
+
+static int sama7_phy_power_off(struct phy *phy)
+{
+	struct sama7_usb_phy *sama7_phy = dev_get_priv(phy->dev);
+
+	clk_disable_unprepare(sama7_phy->uclk);
+
+	return 0;
+}
+
+static int sama7_usb_phy_probe(struct udevice *dev)
+{
+	struct sama7_usb_phy *sama7_phy = dev_get_priv(dev);
+
+	sama7_phy->uclk = devm_clk_get(dev, "utmi_clk");
+	if (IS_ERR(sama7_phy->uclk))
+		return PTR_ERR(sama7_phy->uclk);
+
+	sama7_phy->sfr = syscon_regmap_lookup_by_phandle(dev, "sfr-phandle");
+	if (IS_ERR(sama7_phy->sfr))
+		return PTR_ERR(sama7_phy->sfr);
+
+	return dev_read_u32(dev, "reg", &sama7_phy->port);
+}
+
+static const struct phy_ops sama7_usb_phy_ops = {
+	.init = sama7_usb_phy_init,
+	.power_on = sama7_phy_power_on,
+	.power_off = sama7_phy_power_off,
+};
+
+static const struct udevice_id sama7_usb_phy_of_match[] = {
+	{ .compatible = "microchip,sama7g5-usb-phy", },
+	{ },
+};
+
+U_BOOT_DRIVER(sama7_usb_phy_driver) = {
+	.name = "sama7-usb-phy",
+	.id = UCLASS_PHY,
+	.of_match = sama7_usb_phy_of_match,
+	.ops = &sama7_usb_phy_ops,
+	.probe = sama7_usb_phy_probe,
+	.priv_auto = sizeof(struct sama7_usb_phy),
+};
diff --git a/drivers/phy/phy-sama7-utmi-clk.c b/drivers/phy/phy-sama7-utmi-clk.c
new file mode 100644
index 0000000000..ab9fddccf6
--- /dev/null
+++ b/drivers/phy/phy-sama7-utmi-clk.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Support for Atmel/Microchip USB PHY's.
+ *
+ * Copyright (C) 2022 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Sergiu Moga <sergiu.moga@microchip.com>
+ */
+
+#include <dm.h>
+#include <linux/clk-provider.h>
+#include <syscon.h>
+#include <regmap.h>
+#include <mach/sama7-sfr.h>
+#include <reset.h>
+#include <dt-bindings/clk/at91.h>
+
+struct sama7_utmi_clk {
+	struct clk		uclk;
+	struct regmap		*regmap_sfr;
+	struct reset_ctl	*reset;
+	u8 id;
+};
+
+#define to_sama7_utmi_clk(_c) container_of(_c, struct sama7_utmi_clk, uclk)
+
+#define UBOOT_DM_CLK_MICROCHIP_SAMA7G5_UTMI "sama7-utmi-clk"
+#define UBOOT_DM_MICROCHIP_SAMA7G5_UTMI "sama7-utmi"
+
+#define AT91_TO_CLK_ID(_t, _i)		(((_t) << 8) | ((_i) & 0xff))
+
+/*
+ * UTMI clock description
+ * @n:	clock name
+ * @p:	clock parent name
+ * @id: clock id in RSTC_GRSTR
+ */
+static struct {
+	const char *n;
+	const char *p;
+	u8 id;
+} sama7_utmick[] = {
+	{ .n = "utmi1",	.p = "utmick", .id = 0, },
+	{ .n = "utmi2",	.p = "utmi1",  .id = 1, },
+	{ .n = "utmi3",	.p = "utmi1",  .id = 2, },
+};
+
+static int sama7_utmi_clk_enable(struct clk *clk)
+{
+	int ret;
+
+	struct sama7_utmi_clk *utmi = to_sama7_utmi_clk(clk);
+	u8 id = utmi->id;
+
+	ret = reset_assert(utmi->reset);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(utmi->regmap_sfr, SAMA7_SFR_UTMI0R(id),
+				 SAMA7_SFR_UTMI_COMMONON, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = reset_deassert(utmi->reset);
+	if (ret)
+		return ret;
+
+	/* Datasheet states a minimum of 45 us before any USB operation */
+	udelay(50);
+
+	return 0;
+}
+
+static int sama7_utmi_clk_disable(struct clk *clk)
+{
+	int ret;
+	struct sama7_utmi_clk *utmi = to_sama7_utmi_clk(clk);
+	u8 id = utmi->id;
+
+	ret = reset_assert(utmi->reset);
+	if (ret)
+		return ret;
+
+	regmap_update_bits(utmi->regmap_sfr, SAMA7_SFR_UTMI0R(id),
+			   SAMA7_SFR_UTMI_COMMONON, SAMA7_SFR_UTMI_COMMONON);
+
+	return 0;
+}
+
+static ulong sama7_utmi_clk_get_rate(struct clk *clk)
+{
+	/* Return utmick's rate: 480MHz */
+	return clk_get_parent_rate(clk);
+}
+
+static const struct clk_ops sama7_utmi_clk_ops = {
+	.enable = sama7_utmi_clk_enable,
+	.disable = sama7_utmi_clk_disable,
+	.get_rate = sama7_utmi_clk_get_rate,
+};
+
+static struct clk*
+sama7_utmi_clk_register(struct regmap *regmap_sfr, struct reset_ctl *reset,
+			const char *name, const char *parent_name, u8 id)
+{
+	struct clk *clk;
+	struct sama7_utmi_clk *utmi_clk;
+	int ret;
+
+	if (!regmap_sfr || !reset || !name || !parent_name)
+		return ERR_PTR(-EINVAL);
+
+	utmi_clk = kzalloc(sizeof(*utmi_clk), GFP_KERNEL);
+	if (!utmi_clk)
+		return ERR_PTR(-ENOMEM);
+
+	utmi_clk->reset = reset;
+	utmi_clk->regmap_sfr = regmap_sfr;
+	utmi_clk->id = id;
+
+	clk = &utmi_clk->uclk;
+	ret = clk_register(clk, UBOOT_DM_CLK_MICROCHIP_SAMA7G5_UTMI,
+			   name, parent_name);
+	if (ret) {
+		kfree(utmi_clk);
+		clk = ERR_PTR(ret);
+	}
+
+	clk_dm(AT91_TO_CLK_ID(UTMI, utmi_clk->id), clk);
+
+	return clk;
+}
+
+static int sama7_utmi_probe(struct udevice *dev)
+{
+	struct clk *utmi_parent_clk, *utmi_clk;
+	struct regmap *regmap_sfr;
+	struct reset_ctl *phy_reset;
+	int i;
+	char name[16];
+
+	utmi_parent_clk = devm_clk_get(dev, "utmi_clk");
+	if (IS_ERR(utmi_parent_clk))
+		return PTR_ERR(utmi_parent_clk);
+
+	regmap_sfr = syscon_regmap_lookup_by_phandle(dev, "sfr-phandle");
+	if (IS_ERR(regmap_sfr))
+		return PTR_ERR(regmap_sfr);
+
+	for (i = 0; i < ARRAY_SIZE(sama7_utmick); i++) {
+		snprintf(name, sizeof(name), "usb%d_reset", i);
+		phy_reset = devm_reset_control_get(dev, name);
+		if (IS_ERR(phy_reset))
+			return PTR_ERR(phy_reset);
+
+		utmi_clk = sama7_utmi_clk_register(regmap_sfr, phy_reset,
+						   sama7_utmick[i].n,
+						   sama7_utmick[i].p,
+						   sama7_utmick[i].id);
+		if (IS_ERR(utmi_clk))
+			return PTR_ERR(utmi_clk);
+	}
+
+	return 0;
+};
+
+static const struct udevice_id sama7_utmi_clk_dt_ids[] = {
+	{ .compatible = "microchip,sama7g5-utmi-clk", },
+	{ /* sentinel */},
+};
+
+static int utmi_clk_of_xlate(struct clk *clk, struct ofnode_phandle_args *args)
+{
+	if (args->args_count != 1) {
+		debug("UTMI: clk: Invalid args_count: %d\n", args->args_count);
+		return -EINVAL;
+	}
+
+	clk->id = AT91_TO_CLK_ID(UTMI, args->args[0]);
+
+	return 0;
+}
+
+static const struct clk_ops sama7_utmi_ops = {
+	.of_xlate	= utmi_clk_of_xlate,
+	.enable		= ccf_clk_enable,
+	.disable	= ccf_clk_disable,
+};
+
+U_BOOT_DRIVER(microhip_sama7g5_utmi_clk) = {
+	.name = UBOOT_DM_CLK_MICROCHIP_SAMA7G5_UTMI,
+	.id = UCLASS_CLK,
+	.ops = &sama7_utmi_clk_ops,
+};
+
+U_BOOT_DRIVER(microhip_sama7g5_utmi) = {
+	.name = UBOOT_DM_MICROCHIP_SAMA7G5_UTMI,
+	.of_match = sama7_utmi_clk_dt_ids,
+	.id = UCLASS_CLK,
+	.ops = &sama7_utmi_ops,
+	.probe = sama7_utmi_probe,
+};
-- 
2.34.1

[PATCH v2 2/3] usb: ohci-at91: Enable OHCI functionality and register into DM

[Sergiu Moga]

Register the OHCI driver into DM by properly initializing the required
clocks and pins required by the DT node of OHCI. In order for the VBUS
to stay enabled, a `child_pre_probe` method has been added to overcome
the DM core disabling it in `usb_scan_device`: when the generic
`device_probe` method is called, the pinctrl is processed once again,
undoing whatever changes have been made in our driver's probe method.

Furthermore, enable CONFIG_DM_GPIO whenever this driver and CONFIG_DM_USB
are selected.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---


v1 -> v2:
- squashed 3/4 into this patch
- removed bool clocked
- use *_blk API's
- select DM_GPIO in Kconfig if DM_USB enabled
- use dev_read_u32_default


 drivers/usb/host/Kconfig     |   1 +
 drivers/usb/host/ohci-at91.c | 150 +++++++++++++++++++++++++++++++++++
 2 files changed, 151 insertions(+)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 6213b3c95f..bb1443a338 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -424,6 +424,7 @@ config USB_ATMEL
 	depends on ARCH_AT91
 	select SYS_USB_OHCI_CPU_INIT
 	select USB_OHCI_NEW
+	select DM_GPIO if DM_USB
 
 choice
 	prompt "Clock for OHCI"
diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index 9b955c1bd6..9de67df335 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -5,6 +5,9 @@
  */
 
 #include <common.h>
+
+#if !(CONFIG_IS_ENABLED(DM_USB))
+
 #include <asm/arch/clk.h>
 
 int usb_cpu_init(void)
@@ -62,3 +65,150 @@ int usb_cpu_init_fail(void)
 {
 	return usb_cpu_stop();
 }
+
+#else
+
+#include <clk.h>
+#include <dm.h>
+#include <asm/gpio.h>
+#include <usb.h>
+#include "ohci.h"
+
+#define AT91_MAX_USBH_PORTS        3
+
+#define at91_for_each_port(index, ports)					\
+		for ((index) = 0;						\
+		     (index) < min_t(u32, AT91_MAX_USBH_PORTS, (ports));	\
+		     (index)++)
+
+struct at91_usbh_data {
+	enum usb_init_type init_type;
+	struct gpio_desc vbus_pin[AT91_MAX_USBH_PORTS];
+	u32 ports;				/* number of ports on root hub */
+};
+
+struct ohci_at91_priv {
+	ohci_t ohci;
+	struct clk_bulk clks;
+};
+
+static int at91_start_clock(struct ohci_at91_priv *ohci_at91)
+{
+	return clk_enable_bulk(&ohci_at91->clks);
+}
+
+static int at91_stop_clock(struct ohci_at91_priv *ohci_at91)
+{
+	return clk_disable_bulk(&ohci_at91->clks);
+}
+
+static void ohci_at91_set_power(struct at91_usbh_data *pdata, int port,
+				bool enable)
+{
+	if (!dm_gpio_is_valid(&pdata->vbus_pin[port]))
+		return;
+
+	if (enable)
+		dm_gpio_set_dir_flags(&pdata->vbus_pin[port],
+				      GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
+	else
+		dm_gpio_set_dir_flags(&pdata->vbus_pin[port], 0);
+}
+
+static int at91_start_hc(struct udevice *dev)
+{
+	struct ohci_at91_priv *ohci_at91 = dev_get_priv(dev);
+
+	return at91_start_clock(ohci_at91);
+}
+
+static int at91_stop_hc(struct udevice *dev)
+{
+	struct ohci_at91_priv *ohci_at91 = dev_get_priv(dev);
+
+	return at91_stop_clock(ohci_at91);
+}
+
+static int ohci_atmel_deregister(struct udevice *dev)
+{
+	struct at91_usbh_data *pdata = dev_get_plat(dev);
+	int ret, i;
+
+	ret = at91_stop_hc(dev);
+	if (ret)
+		return ret;
+
+	at91_for_each_port(i, pdata->ports)
+		ohci_at91_set_power(pdata, i, false);
+
+	return ohci_deregister(dev);
+}
+
+static int ohci_atmel_child_pre_probe(struct udevice *dev)
+{
+	struct udevice *ohci_controller = dev_get_parent(dev);
+	struct at91_usbh_data *pdata = dev_get_plat(ohci_controller);
+	int i;
+
+	at91_for_each_port(i, pdata->ports)
+		ohci_at91_set_power(pdata, i, true);
+
+	return 0;
+}
+
+static int ohci_atmel_probe(struct udevice *dev)
+{
+	struct at91_usbh_data *pdata = dev_get_plat(dev);
+	struct ohci_at91_priv *ohci_at91 = dev_get_priv(dev);
+	u32		      i;
+	int		      ret;
+	struct ohci_regs      *regs = (struct ohci_regs *)dev_read_addr(dev);
+
+	pdata->ports = dev_read_u32_default(dev, "num-ports", 3);
+
+	at91_for_each_port(i, pdata->ports)
+		gpio_request_by_name(dev, "atmel,vbus-gpio", i,
+				     &pdata->vbus_pin[i], GPIOD_IS_OUT |
+				     GPIOD_IS_OUT_ACTIVE);
+
+	ret = clk_get_bulk(dev, &ohci_at91->clks);
+	if (ret)
+		goto fail;
+
+	ret = clk_enable_bulk(&ohci_at91->clks);
+	if (ret)
+		goto fail;
+
+	ret = at91_start_hc(dev);
+	if (ret)
+		goto fail;
+
+	return ohci_register(dev, regs);
+
+fail:
+	at91_for_each_port(i, pdata->ports)
+		if (dm_gpio_is_valid(&pdata->vbus_pin[i]))
+			gpio_free(pdata->vbus_pin[i].offset);
+
+	return ret;
+}
+
+static const struct udevice_id ohci_usb_ids[] = {
+	{ .compatible = "atmel,at91rm9200-ohci", },
+	{ }
+};
+
+U_BOOT_DRIVER(ohci_atmel) = {
+	.name		 = "ohci_atmel",
+	.id		 = UCLASS_USB,
+	.of_match	 = ohci_usb_ids,
+	.probe		 = ohci_atmel_probe,
+	.remove		 = ohci_atmel_deregister,
+	.child_pre_probe = ohci_atmel_child_pre_probe,
+	.ops		 = &ohci_usb_ops,
+	.plat_auto	 = sizeof(struct at91_usbh_data),
+	.priv_auto	 = sizeof(struct ohci_at91_priv),
+	.flags		 = DM_FLAG_ALLOC_PRIV_DMA,
+};
+
+#endif /* CONFIG_IS_ENABLED(DM_USB) */
-- 
2.34.1

[PATCH v2 3/3] usb: ohci-at91: Add USB PHY functionality

[Sergiu Moga]

Add the ability to enable/disable whatever USB PHY's are
passed to the AT91 OHCI driver through DT.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
Tested-by: Mihai Sain <mihai.sain@microchip.com>
---


v1 -> v2:
- use *_bulk API's


 drivers/usb/host/ohci-at91.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index 9de67df335..fece921d39 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -74,6 +74,10 @@ int usb_cpu_init_fail(void)
 #include <usb.h>
 #include "ohci.h"
 
+#if CONFIG_IS_ENABLED(PHY_MICROCHIP_SAMA7_USB)
+#include <generic-phy.h>
+#endif
+
 #define AT91_MAX_USBH_PORTS        3
 
 #define at91_for_each_port(index, ports)					\
@@ -90,15 +94,35 @@ struct at91_usbh_data {
 struct ohci_at91_priv {
 	ohci_t ohci;
 	struct clk_bulk clks;
+
+#if CONFIG_IS_ENABLED(PHY_MICROCHIP_SAMA7_USB)
+	struct phy_bulk phys;
+#endif
 };
 
 static int at91_start_clock(struct ohci_at91_priv *ohci_at91)
 {
+#if CONFIG_IS_ENABLED(PHY_MICROCHIP_SAMA7_USB)
+	int ret;
+
+	ret = generic_phy_power_on_bulk(&ohci_at91->phys);
+	if (ret)
+		return ret;
+#endif
+
 	return clk_enable_bulk(&ohci_at91->clks);
 }
 
 static int at91_stop_clock(struct ohci_at91_priv *ohci_at91)
 {
+#if CONFIG_IS_ENABLED(PHY_MICROCHIP_SAMA7_USB)
+	int ret;
+
+	ret = generic_phy_power_off_bulk(&ohci_at91->phys);
+	if (ret)
+		return ret;
+#endif
+
 	return clk_disable_bulk(&ohci_at91->clks);
 }
 
@@ -179,6 +203,16 @@ static int ohci_atmel_probe(struct udevice *dev)
 	if (ret)
 		goto fail;
 
+#if CONFIG_IS_ENABLED(PHY_MICROCHIP_SAMA7_USB)
+	ret = generic_phy_get_bulk(dev, &ohci_at91->phys);
+	if (ret)
+		goto fail;
+
+	ret = generic_phy_init_bulk(&ohci_at91->phys);
+	if (ret)
+		goto fail;
+#endif
+
 	ret = at91_start_hc(dev);
 	if (ret)
 		goto fail;
@@ -195,6 +229,7 @@ fail:
 
 static const struct udevice_id ohci_usb_ids[] = {
 	{ .compatible = "atmel,at91rm9200-ohci", },
+	{ .compatible = "microchip,sama7g5-ohci", },
 	{ }
 };
 
-- 
2.34.1

Re: [PATCH v2 1/3] phy: at91: Add support for the USB 2.0 PHY's of SAMA7

[Marek Vasut]

On 1/3/23 15:30, Sergiu Moga wrote:
> In order to have USB functionality, drivers for SAMA7's
> USB 2.0 PHY's have been added. There is one driver
> for UTMI clock's SFR and RESET required functionalities and
> one for its three possible subclocks of the phy's themselves.
> In order for this layout to properly work in conjunction with
> CCF and DT, the former driver will also act as a clock provider
> for the three phy's with the help of a custom hook into the
> driver's of_xlate method.

[...]

> +static int sama7_utmi_probe(struct udevice *dev)
> +{
> +	struct clk *utmi_parent_clk, *utmi_clk;
> +	struct regmap *regmap_sfr;
> +	struct reset_ctl *phy_reset;
> +	int i;
> +	char name[16];
> +
> +	utmi_parent_clk = devm_clk_get(dev, "utmi_clk");
> +	if (IS_ERR(utmi_parent_clk))
> +		return PTR_ERR(utmi_parent_clk);
> +
> +	regmap_sfr = syscon_regmap_lookup_by_phandle(dev, "sfr-phandle");
> +	if (IS_ERR(regmap_sfr))
> +		return PTR_ERR(regmap_sfr);
> +
> +	for (i = 0; i < ARRAY_SIZE(sama7_utmick); i++) {
> +		snprintf(name, sizeof(name), "usb%d_reset", i);
> +		phy_reset = devm_reset_control_get(dev, name);
> +		if (IS_ERR(phy_reset))
> +			return PTR_ERR(phy_reset);
> +
> +		utmi_clk = sama7_utmi_clk_register(regmap_sfr, phy_reset,
> +						   sama7_utmick[i].n,
> +						   sama7_utmick[i].p,
> +						   sama7_utmick[i].id);
> +		if (IS_ERR(utmi_clk))
> +			return PTR_ERR(utmi_clk);

Isn't this going to leak memory if i>0 and a failure occurs ? I think it 
will, since sama7_utmi_clk_register() calls kzalloc() .

Re: [PATCH v2 2/3] usb: ohci-at91: Enable OHCI functionality and register into DM

[Marek Vasut]

On 1/3/23 15:30, Sergiu Moga wrote:

[...]

> +static int ohci_atmel_probe(struct udevice *dev)
> +{
> +	struct at91_usbh_data *pdata = dev_get_plat(dev);
> +	struct ohci_at91_priv *ohci_at91 = dev_get_priv(dev);
> +	u32		      i;
> +	int		      ret;
> +	struct ohci_regs      *regs = (struct ohci_regs *)dev_read_addr(dev);

Use one space after type for indent.

Also, I think the dev_read_addr() can fail, so you want to check the 
return value.

> +	pdata->ports = dev_read_u32_default(dev, "num-ports", 3);
> +
> +	at91_for_each_port(i, pdata->ports)
> +		gpio_request_by_name(dev, "atmel,vbus-gpio", i,
> +				     &pdata->vbus_pin[i], GPIOD_IS_OUT |
> +				     GPIOD_IS_OUT_ACTIVE);
> +
> +	ret = clk_get_bulk(dev, &ohci_at91->clks);
> +	if (ret)
> +		goto fail;
> +
> +	ret = clk_enable_bulk(&ohci_at91->clks);
> +	if (ret)
> +		goto fail;
> +
> +	ret = at91_start_hc(dev);
> +	if (ret)
> +		goto fail;
> +
> +	return ohci_register(dev, regs);
> +
> +fail:
> +	at91_for_each_port(i, pdata->ports)
> +		if (dm_gpio_is_valid(&pdata->vbus_pin[i]))
> +			gpio_free(pdata->vbus_pin[i].offset);

Add clk_disable in case of failure after at91_start_hc , please check 
all the fail paths one more time.

[...]

Re: [PATCH v2 3/3] usb: ohci-at91: Add USB PHY functionality

[Marek Vasut]

On 1/3/23 15:30, Sergiu Moga wrote:
> Add the ability to enable/disable whatever USB PHY's are
> passed to the AT91 OHCI driver through DT.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> Tested-by: Mihai Sain <mihai.sain@microchip.com>
> ---
> 
> 
> v1 -> v2:
> - use *_bulk API's
> 
> 
>   drivers/usb/host/ohci-at91.c | 35 +++++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
> index 9de67df335..fece921d39 100644
> --- a/drivers/usb/host/ohci-at91.c
> +++ b/drivers/usb/host/ohci-at91.c
> @@ -74,6 +74,10 @@ int usb_cpu_init_fail(void)
>   #include <usb.h>
>   #include "ohci.h"
>   
> +#if CONFIG_IS_ENABLED(PHY_MICROCHIP_SAMA7_USB)
> +#include <generic-phy.h>
> +#endif
> +
>   #define AT91_MAX_USBH_PORTS        3
>   
>   #define at91_for_each_port(index, ports)					\
> @@ -90,15 +94,35 @@ struct at91_usbh_data {
>   struct ohci_at91_priv {
>   	ohci_t ohci;
>   	struct clk_bulk clks;
> +
> +#if CONFIG_IS_ENABLED(PHY_MICROCHIP_SAMA7_USB)
> +	struct phy_bulk phys;
> +#endif
>   };
>   
>   static int at91_start_clock(struct ohci_at91_priv *ohci_at91)
>   {
> +#if CONFIG_IS_ENABLED(PHY_MICROCHIP_SAMA7_USB)

if (CONFIG_IS_ENABLED(...)) please.

[...]

Re: [PATCH v2 1/3] phy: at91: Add support for the USB 2.0 PHY's of SAMA7

[Sergiu.Moga]

On 03.01.2023 17:31, Marek Vasut wrote:
> On 1/3/23 15:30, Sergiu Moga wrote:
>> In order to have USB functionality, drivers for SAMA7's
>> USB 2.0 PHY's have been added. There is one driver
>> for UTMI clock's SFR and RESET required functionalities and
>> one for its three possible subclocks of the phy's themselves.
>> In order for this layout to properly work in conjunction with
>> CCF and DT, the former driver will also act as a clock provider
>> for the three phy's with the help of a custom hook into the
>> driver's of_xlate method.
> 
> [...]
> 
>> +static int sama7_utmi_probe(struct udevice *dev)
>> +{
>> +     struct clk *utmi_parent_clk, *utmi_clk;
>> +     struct regmap *regmap_sfr;
>> +     struct reset_ctl *phy_reset;
>> +     int i;
>> +     char name[16];
>> +
>> +     utmi_parent_clk = devm_clk_get(dev, "utmi_clk");
>> +     if (IS_ERR(utmi_parent_clk))
>> +             return PTR_ERR(utmi_parent_clk);
>> +
>> +     regmap_sfr = syscon_regmap_lookup_by_phandle(dev, "sfr-phandle");
>> +     if (IS_ERR(regmap_sfr))
>> +             return PTR_ERR(regmap_sfr);
>> +
>> +     for (i = 0; i < ARRAY_SIZE(sama7_utmick); i++) {
>> +             snprintf(name, sizeof(name), "usb%d_reset", i);
>> +             phy_reset = devm_reset_control_get(dev, name);
>> +             if (IS_ERR(phy_reset))
>> +                     return PTR_ERR(phy_reset);
>> +
>> +             utmi_clk = sama7_utmi_clk_register(regmap_sfr, phy_reset,
>> +                                                sama7_utmick[i].n,
>> +                                                sama7_utmick[i].p,
>> +                                                sama7_utmick[i].id);
>> +             if (IS_ERR(utmi_clk))
>> +                     return PTR_ERR(utmi_clk);
> 
> Isn't this going to leak memory if i>0 and a failure occurs ? I think it
> will, since sama7_utmi_clk_register() calls kzalloc() .

Yes, you are right. Perhaps something like this should work better 
instead, wouldn't you agree?

  static int sama7_utmi_probe(struct udevice *dev)
  {
-	struct clk *utmi_parent_clk, *utmi_clk;
+	struct clk *utmi_parent_clk, *utmi_clk[ARRAY_SIZE(sama7_utmick)];
+	struct sama7_utmi_clk *uclk;
  	struct regmap *regmap_sfr;
  	struct reset_ctl *phy_reset;
-	int i;
+	int i, j;
  	char name[16];

  	utmi_parent_clk = devm_clk_get(dev, "utmi_clk");
@@ -153,12 +154,18 @@ static int sama7_utmi_probe(struct udevice *dev)
  		if (IS_ERR(phy_reset))
  			return PTR_ERR(phy_reset);

-		utmi_clk = sama7_utmi_clk_register(regmap_sfr, phy_reset,
+		utmi_clk[i] = sama7_utmi_clk_register(regmap_sfr, phy_reset,
  						   sama7_utmick[i].n,
  						   sama7_utmick[i].p,
  						   sama7_utmick[i].id);
-		if (IS_ERR(utmi_clk))
+		if (IS_ERR(utmi_clk)) {
+			for (j = 0; j < i; j++) {
+				uclk = to_sama7_utmi_clk(utmi_clk[j]);
+				kfree(uclk);
+			}
+
  			return PTR_ERR(utmi_clk);
+		}
  	}

  	return 0;

Re: [PATCH v2 1/3] phy: at91: Add support for the USB 2.0 PHY's of SAMA7

[Marek Vasut]

On 1/3/23 17:54, Sergiu.Moga@microchip.com wrote:
> On 03.01.2023 17:31, Marek Vasut wrote:
>> On 1/3/23 15:30, Sergiu Moga wrote:
>>> In order to have USB functionality, drivers for SAMA7's
>>> USB 2.0 PHY's have been added. There is one driver
>>> for UTMI clock's SFR and RESET required functionalities and
>>> one for its three possible subclocks of the phy's themselves.
>>> In order for this layout to properly work in conjunction with
>>> CCF and DT, the former driver will also act as a clock provider
>>> for the three phy's with the help of a custom hook into the
>>> driver's of_xlate method.
>>
>> [...]
>>
>>> +static int sama7_utmi_probe(struct udevice *dev)
>>> +{
>>> +     struct clk *utmi_parent_clk, *utmi_clk;
>>> +     struct regmap *regmap_sfr;
>>> +     struct reset_ctl *phy_reset;
>>> +     int i;
>>> +     char name[16];
>>> +
>>> +     utmi_parent_clk = devm_clk_get(dev, "utmi_clk");
>>> +     if (IS_ERR(utmi_parent_clk))
>>> +             return PTR_ERR(utmi_parent_clk);
>>> +
>>> +     regmap_sfr = syscon_regmap_lookup_by_phandle(dev, "sfr-phandle");
>>> +     if (IS_ERR(regmap_sfr))
>>> +             return PTR_ERR(regmap_sfr);
>>> +
>>> +     for (i = 0; i < ARRAY_SIZE(sama7_utmick); i++) {
>>> +             snprintf(name, sizeof(name), "usb%d_reset", i);
>>> +             phy_reset = devm_reset_control_get(dev, name);
>>> +             if (IS_ERR(phy_reset))
>>> +                     return PTR_ERR(phy_reset);
>>> +
>>> +             utmi_clk = sama7_utmi_clk_register(regmap_sfr, phy_reset,
>>> +                                                sama7_utmick[i].n,
>>> +                                                sama7_utmick[i].p,
>>> +                                                sama7_utmick[i].id);
>>> +             if (IS_ERR(utmi_clk))
>>> +                     return PTR_ERR(utmi_clk);
>>
>> Isn't this going to leak memory if i>0 and a failure occurs ? I think it
>> will, since sama7_utmi_clk_register() calls kzalloc() .
> 
> Yes, you are right. Perhaps something like this should work better
> instead, wouldn't you agree?
> 
>    static int sama7_utmi_probe(struct udevice *dev)
>    {
> -	struct clk *utmi_parent_clk, *utmi_clk;
> +	struct clk *utmi_parent_clk, *utmi_clk[ARRAY_SIZE(sama7_utmick)];
> +	struct sama7_utmi_clk *uclk;
>    	struct regmap *regmap_sfr;
>    	struct reset_ctl *phy_reset;
> -	int i;
> +	int i, j;
>    	char name[16];
> 
>    	utmi_parent_clk = devm_clk_get(dev, "utmi_clk");
> @@ -153,12 +154,18 @@ static int sama7_utmi_probe(struct udevice *dev)
>    		if (IS_ERR(phy_reset))
>    			return PTR_ERR(phy_reset);
> 
> -		utmi_clk = sama7_utmi_clk_register(regmap_sfr, phy_reset,
> +		utmi_clk[i] = sama7_utmi_clk_register(regmap_sfr, phy_reset,
>    						   sama7_utmick[i].n,
>    						   sama7_utmick[i].p,
>    						   sama7_utmick[i].id);
> -		if (IS_ERR(utmi_clk))
> +		if (IS_ERR(utmi_clk)) {
> +			for (j = 0; j < i; j++) {
> +				uclk = to_sama7_utmi_clk(utmi_clk[j]);
> +				kfree(uclk);

Do the usual error handling path using goto:

{
...
if (IS_ERR(...))
   goto error_clk_register;
...

error_clk_register:
   kfree();
error_something_else:
   clean_up_other_stuff();
   return ret;
}