Re: [PATCH v2 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan

["Sören Brinkmann" <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>]

On Tue, 2015-09-15 at 09:07AM +0800, Shawn Lin wrote:
> On 2015/9/14 23:07, Sören Brinkmann wrote:
> >Hi Shawn,
> >
> >overall, it looks good to me. I have some questions though.
> >
> >On Mon, 2015-09-14 at 02:29PM +0800, Shawn Lin wrote:
> 
> [...]
> 
> >>+err_phy_exit:
> >>+	phy_init(phy);
> >
> >Just to confirm, are these actions in the error path correct? E.g.
> >if the power_off() call fails, is it safe to call power_on()? Isn't
> >the phy still powered on? (this would apply to other error paths too)
> >
> 
> Cool question!
> 
> While writing this, I had read generic phy stuffs deliberately to find a
> solution for a case: how to deal with ping-pong fails? In another word, if
> power_off call fails, then we should call power_on, but unfortunately it
> fails again then we call power_off... so endless nested err handling... No
> answer yet.
> 
> So, I assumed two cases happened when power_off call fails:
> (1) *real power_off* is done, but some other stuffs in the calling path
> fail, so phy is really power_off in theory. We need to power_on it again,
> but if it fail, we don't know PHY is on or off since we don't know power_on
> fails for what? *real power on* ? some other stuffs?
> 
> (2) *real power_off* isn't completed, so indeed it's *still* in power_on
> state. The reason we never need to check the return value of power_on cross
> the err handling is that whether power_on call successfully or not, it's
> always make phy in power_on state.
> 
> Now, let's think about case(1).
> After reading dozens of sample codes(such as USB, UFS, MBUS) that adopt
> generic phy framework for PHY management, real thing should be like that:
> they NEVER deal with case(1).
> 
> It's a trick of sub-phy drivers themself. power_on/off calling path return
> err for two case:
> <1>  phy_runtime callback fails. It's after *real power on/off*, so
> definitely *real power on/off* is conpleted. That is the case(2) I mentioned
> above.
> <2>  sub-phy drivers return err for  phy->ops->power_off(phy); Look
> into all the sub-phy drivers twice, we find that they always return success
> for phy->ops->power_off hook. Why? Because all of them
> write registers to enable/disable something, always consider things going
> well. Actually if we write value into a register, we have to think that it's
> functional.
> 
> Anyway, back to this patch.
> Indeed we also write value into arasan phy's register to do
> phy_power_on/off/init/exit to make things work. Right, we return success
> state for all of these them just as all the other sub-phy drivers do.
> 
> Feel free to let me know if I make mistakes or misunderstanding above.
> 
> >>+	return ret;
> >>+}
> 
> [...]
> 
> >>+		}
> >>+	}
> >
> >I assume you looked at options for having the error paths in a
> >consolidated location? I guess this may be the nicest solution since all
> >of this is in this conditional block?
> >
> 
> yep, otherwise we should add some *if* statements to check sdhci_arasan->phy
> cross the err handles. And I intent to strictly limit
> the phy stuffs under the scope of arasan,sdhci-5.1 currently.
> 
> >Feel free to add my
> >Acked-by: Sören Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >
> 
> Thanks, Sören. :)

Makes all sense to me. Thanks for all the details.

	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html