Hi,
On 01/19/2014 08:56 PM, Russell King - ARM Linux wrote:
On Sun, Jan 19, 2014 at 08:07:46PM +0100, Hans de Goede wrote:
Hi,
On 01/19/2014 01:22 PM, Russell King - ARM Linux wrote:
On Sun, Jan 19, 2014 at 12:48:51AM +0100, Hans de Goede wrote:
+ timeout = 0x100000;
+ do {
+ reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
+ } while (--timeout && (reg_val != 0x2));
+ if (!timeout) {
+ dev_err(dev, "PHY power up failed.\n");
+ return -EIO;
+ }
This is not a good way to detect failure - there's several things wrong
here.
First, how long does sunxi_getbits() take? What does that depend on?
Therefore, how long does it take to time out?
You're interpreting the timeout in the above code as an actual timeout, but
that is not what it is, it is a means to avoid looping forever if something
is seriously amiss. The only time I've ever seen the timeout trigger is when
I forgot to enable some clks iirc.
I can rename the variable from timeout to max_tries to make this more clear.
Secondly, what if the success condition becomes true at the same time that
a timeout occurs?
We should never get anywhere near timeout becoming 0, so if both happen at
the same time, then something is pretty seriously broken and the returning of
an error as the code does now is the right thing to do.
Yes... and if we look back in history, there's been lots of stuff just
like this where the loop has had to have the number of iterations
increased as CPUs have become faster and compilers become better?
So... my question stands: but let me put it a different way in two parts:
1. What is the maximum expected time for the success condition to be
satisfied?
TBH, I don't have a clue this code comes from the android driver (never an
excuse, I know) and we don't have any documentation other then the android
driver.
2. How long does it actually take for the loop to time out in existing
CPUs/compilers?
I don't know either. I understand where you're coming from, and I believe
that the best way to solve this is to take your suggested implementation, start
with a way too high timeout, and add a debug print to see how much time is left
after a successfull phy_init, then do a couple of test runs to get a ballpark
figure for how long we need to wait, multiply that by 5 to be on the safe side,
and use that.
Does that sound like a plan ?
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
