Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Marc Gonzalez <marc.w.gonzalez@free.fr>
To: Vivek Gautam <vivek.gautam@codeaurora.org>,
	Kishon Vijay Abraham <kishon@ti.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Douglas Anderson <dianders@chromium.org>
Cc: MSM <linux-arm-msm@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1] phy: qcom-qmp: Raise qcom_qmp_phy_enable() polling delay
Date: Fri, 14 Jun 2019 14:38:02 +0200
Message-ID: <134f4648-682e-5fed-60e7-bc25985dd7e9@free.fr> (raw)
In-Reply-To: <a3a50cf5-083a-5aa8-e77c-6feb2f2fd866@codeaurora.org>

+ Doug (who is familiar with usleep_range quirks)

On 14/06/2019 11:50, Vivek Gautam wrote:

> On 6/13/2019 5:02 PM, Marc Gonzalez wrote:
> 
>> readl_poll_timeout() calls usleep_range() to sleep between reads.
>> usleep_range() doesn't work efficiently for tiny values.
>>
>> Raise the polling delay in qcom_qmp_phy_enable() to bring it in line
>> with the delay in qcom_qmp_phy_com_init().
>>
>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
>> ---
>> Vivek, do you remember why you didn't use the same delay value in
>> qcom_qmp_phy_enable) and qcom_qmp_phy_com_init() ?
> 
> phy_qcom_init() thingy came from the PCIE phy driver from downstream
> msm-3.18 PCIE did something as below:

FWIW and IMO, drivers/pci/host/pci-msm.c is a good example of how not to write
a device driver. It's huge (7000+ lines) because it handles multiple platforms
via ifdefs, and lumps everything together (phy, core IP, SoC specific glue)
in a single file.

> -----
> do {
>          if (pcie_phy_is_ready(dev))
>                  break;
>          retries++;
>          usleep_range(REFCLK_STABILIZATION_DELAY_US_MIN,
>                                   REFCLK_STABILIZATION_DELAY_US_MAX);
> } while (retries < PHY_READY_TIMEOUT_COUNT);
> 
> REFCLK_STABILIZATION_DELAY_US_MIN/MAX ==> 1000/1005
> PHY_READY_TIMEOUT_COUNT ==> 10
> -----

https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/pci/host/pci-msm.c?h=LE.UM.1.3.r3.25#n4624

https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/pci/host/pci-msm.c?h=LE.UM.1.3.r3.25#n1721

readl_relaxed(dev->phy + PCIE_N_PCS_STATUS(dev->rc_idx, dev->common_phy)) & BIT(6)
is equivalent to:
the check in qcom_qmp_phy_enable()

readl_relaxed(dev->phy + PCIE_COM_PCS_READY_STATUS) & 0x1
is equivalent to:
the check in qcom_qmp_phy_com_init()

I'll take a closer look, using some printks, to narrow down the run-time
execution path.

> phy_enable() from the usb phy driver from downstream.
>   /* Wait for PHY initialization to be done */
>   do {
>           if (readl_relaxed(phy->base +
>                   phy->phy_reg[USB3_PHY_PCS_STATUS]) & PHYSTATUS)
>                   usleep_range(1, 2);
> else
> break;
>   } while (--init_timeout_usec);
> 
> init_timeout_usec ==> 1000
> -----
> USB never had a COM_PHY status bit.
> 
> So clearly the resolutions were different.
> 
> Does this change solve an issue at hand?

The issue is usleep_range() being misused ^_^

Although usleep_range() takes unsigned longs as parameters, it is
not appropriate over the entire 0-2^64 range.

a) It should not be used with tiny values, because the cost of programming
the timer interrupt, and processing the resulting IRQ would dominate.

b) It should not be used with large values (above 2000000/HZ) because
msleep() is more efficient, and is acceptable for these ranges.

Regards.

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-13 11:32 Marc Gonzalez
2019-06-14  9:50 ` Vivek Gautam
2019-06-14 12:38   ` Marc Gonzalez [this message]
2019-06-20  6:25     ` Kishon Vijay Abraham I
2019-06-24 11:55       ` Marc Gonzalez
2019-06-24 15:52 ` Doug Anderson

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=134f4648-682e-5fed-60e7-bc25985dd7e9@free.fr \
    --to=marc.w.gonzalez@free.fr \
    --cc=bjorn.andersson@linaro.org \
    --cc=dianders@chromium.org \
    --cc=kishon@ti.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=vivek.gautam@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git