On 02/05/2024 15:16, Sam Day wrote:

The newer "register map for simple gate clocks" support added for qcom clocks is used. As a result gcc_apq8016 now has a mixture of the old and new styles. I didn't (and still don't!) feel comfortable enough in this area to update the existing code.

Some groundwork was also laid to dump the state of these gate clocks with the recently-added qcom dump callbacks.

I'm not sure which part of the patch you're referring to, but the dump callbacks aren't in upstream yet.

Signed-off-by: Sam Day me@samcday.com

drivers/clk/qcom/clock-apq8016.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)

diff --git a/drivers/clk/qcom/clock-apq8016.c b/drivers/clk/qcom/clock-apq8016.c index d3b63b9c1a..745371a289 100644 --- a/drivers/clk/qcom/clock-apq8016.c +++ b/drivers/clk/qcom/clock-apq8016.c @@ -17,6 +17,8 @@

#include "clock-qcom.h"

+#define USB_HS_SYSTEM_CLK_CMD_RCGR 0x41010

• /* Clocks: (from CLK_CTL_BASE) */ #define GPLL0_STATUS (0x2101C) #define APCS_GPLL_ENA_VOTE (0x45000)

@@ -39,6 +41,11 @@ /* GPLL0 clock control registers */ #define GPLL0_STATUS_ACTIVE BIT(17)

+static const struct freq_tbl ftbl_gcc_usb_hs_system_clk[] = {

• F(80000000, CFG_CLK_SRC_GPLL0, 10, 0, 0),

Since this is just one entry, it probably makes more sense to just put the literal values in the clk_rcg_set_rate_mnd(). This makes it clear that there's only one actual frequency, and we have the kernel for reference anyway.

• { }

+};

• static struct pll_vote_clk gpll0_vote_clk = { .status = GPLL0_STATUS, .status_bit = GPLL0_STATUS_ACTIVE,

@@ -52,6 +59,11 @@ static struct vote_clk gcc_blsp1_ahb_clk = { .vote_bit = BIT(10), };

+static const struct gate_clk apq8016_clks[] = {

Please define the size of this array, or add a bounds check in apq8016_clk_enable() (I know this is missing for some of the other drivers).

• GATE_CLK(GCC_USB_HS_AHB_CLK, 0x41008, 0x00000001),
• GATE_CLK(GCC_USB_HS_SYSTEM_CLK, 0x41004, 0x00000001),

+};

• /* SDHCI */ static int apq8016_clk_init_sdc(struct msm_clk_priv *priv, int slot, uint rate) {

@@ -106,6 +118,7 @@ int apq8016_clk_init_uart(phys_addr_t base, unsigned long id)

static ulong apq8016_clk_set_rate(struct clk *clk, ulong rate) {

• const struct freq_tbl *freq; struct msm_clk_priv *priv = dev_get_priv(clk->dev);

  switch (clk->id) {

@@ -117,13 +130,31 @@ static ulong apq8016_clk_set_rate(struct clk *clk, ulong rate) case GCC_BLSP1_UART2_APPS_CLK: /* UART2 */ apq8016_clk_init_uart(priv->base, clk->id); return 7372800;

• case GCC_USB_HS_SYSTEM_CLK:

• freq = qcom_find_freq(ftbl_gcc_usb_hs_system_clk, rate);
  

As there is only one valid "rate" for this clock, I would be in favour of a log_warn() if the rate is wrong (it should be 80000000).

Thanks and regards,

• clk_rcg_set_rate_mnd(priv->base, USB_HS_SYSTEM_CLK_CMD_RCGR,
  

•                      freq->pre_div, freq->m, freq->n, freq->src, 0);
  

• return freq->freq;
  

  default: return 0; } }

+static int apq8016_clk_enable(struct clk *clk) +{

• struct msm_clk_priv *priv = dev_get_priv(clk->dev);
• debug("%s: clk %s\n", __func__, apq8016_clks[clk->id].name);
• qcom_gate_clk_en(priv, clk->id);
• return 0;

+}

• static struct msm_clk_data apq8016_clk_data = { .set_rate = apq8016_clk_set_rate,

• .clks = apq8016_clks,

• .num_clks = ARRAY_SIZE(apq8016_clks),

• .enable = apq8016_clk_enable, };

  static const struct udevice_id gcc_apq8016_of_match[] = {

On 02/05/2024 15:16, Sam Day wrote:

This seems to be necessary on my samsung-a5. Without this patch, the first access of EHCI registers causes a bus stall and subsequent reset.

I am unsure why this wasn't already necessary for db410c, perhaps those clocks are already enabled on boot.

Signed-off-by: Sam Day me@samcday.com

drivers/usb/host/ehci-msm.c | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c index 98fe7bc3bc..b2e294dd64 100644 --- a/drivers/usb/host/ehci-msm.c +++ b/drivers/usb/host/ehci-msm.c @@ -7,8 +7,10 @@

• Based on Linux driver

*/

+#include <clk.h> #include <common.h> #include <dm.h> +#include <dm/device_compat.h> #include <dm/lists.h> #include <errno.h> #include <usb.h> @@ -25,6 +27,8 @@ struct msm_ehci_priv { struct usb_ehci *ehci; /* Start of IP core*/ struct ulpi_viewport ulpi_vp; /* ULPI Viewport */ struct phy phy;

• struct clk iface_clk;
• struct clk core_clk;

You could simplify this with the bulk clock API, but I'm easy either way.

Reviewed-by: Caleb Connolly caleb.connolly@linaro.org

};

static int msm_init_after_reset(struct ehci_ctrl *dev) @@ -53,20 +57,46 @@ static int ehci_usb_probe(struct udevice *dev) struct ehci_hcor *hcor; int ret;

• ret = clk_get_by_name(dev, "core", &p->core_clk);

• if (ret) {

• dev_err(dev, "Failed to get core clock: %d\n", ret);
  

• return ret;
  

• }

• ret = clk_get_by_name(dev, "iface", &p->iface_clk);

• if (ret) {

• dev_err(dev, "Failed to get iface clock: %d\n", ret);
  

• return ret;
  

• }

• ret = clk_prepare_enable(&p->core_clk);

• if (ret)

• return ret;
  

• ret = clk_prepare_enable(&p->iface_clk);

• if (ret)

• goto cleanup_core;
  

• hccr = (struct ehci_hccr *)((phys_addr_t)&ehci->caplength); hcor = (struct ehci_hcor *)((phys_addr_t)hccr + HC_LENGTH(ehci_readl(&(hccr)->cr_capbase)));

  ret = generic_setup_phy(dev, &p->phy, 0); if (ret)

• return ret;
  

• goto cleanup_iface;
  
  

  ret = board_usb_init(0, plat->init_type); if (ret < 0)

• return ret;
  

• goto cleanup_iface;
  
  

  return ehci_register(dev, hccr, hcor, &msm_ehci_ops, 0, plat->init_type);

+cleanup_iface:

• clk_disable_unprepare(&p->iface_clk);

+cleanup_core:

• clk_disable_unprepare(&p->core_clk);

• return ret; }

  static int ehci_usb_remove(struct udevice *dev)

@@ -82,6 +112,9 @@ static int ehci_usb_remove(struct udevice *dev) /* Stop controller. */ clrbits_le32(&ehci->usbcmd, CMD_RUN);

• clk_disable_unprepare(&p->iface_clk);
• clk_disable_unprepare(&p->core_clk);
• ret = generic_shutdown_phy(&p->phy); if (ret) return ret;