Matthieu CASTET <matthieu.castet@xxxxxxxxxx> writes:
> Alexander Shishkin a crit :
>> Matthieu CASTET <matthieu.castet@xxxxxxxxxx> writes:
>>
>>> Alexander Shishkin a crit :
>>>> + ehci->regs = ci->hw_bank.cap +
>>>> + HC_LENGTH(ehci, ehci_readl(ehci, &ehci->caps->hc_capbase));
>>>> + ehci->hcs_params = ehci_readl(ehci, &ehci->caps->hcs_params);
>>>> + ehci->has_hostpc = 1;
>>>> + ehci->sbrn = 0x20;
>>> Why do you set has_hostpc = 1 ?
>>> This won't work with normal chipidea core ?
>>
>> ....because this way we get proper mode setting from ehci_reset on
>> lpm-capable chipideas, see below.
>>
>>>> +
>>>> + ret = hw_device_reset(ci, USBMODE_CM_HC);
>>>> + if (ret)
>>>> + goto rm_hcd;
>>> Why do you need to do that ? ehci_reset will do it (with tdi_reset).
>>
>> Actually, it will only do that for non lpm-capable controllers, which
>> have USBMODE register in the place where tdi_reset() expects it to
>> be. On lpm-capable controllers, USBMODE is in fact where USBMODE_EX
>> (from ehci-def.h) points to, so tdi_reset() sort of shoots at nothing in
>> particular. Writes to the USBMODE location for such devices are silently
>> ignored (maybe we need to tweak tdi_reset() to not do that anyway to be
>> no a safe side, but that's another matter).
>>
>> However, ehci_reset() will still do the right thing if has_hostpc is
>> set, so you're right, this reset call is actually not needed.
>>
>> Having said all that, it seems obvious that has_hostpc setting should
>> depend on wheather we are lpm-capable or not. What do you think?
>>
> If has_hostpc is really chipidea with lpm I think we should :
> - rename it to something understandable
> - autodetect it like we does in udc (using HCCPARAMS). May be it could be
> replaced with ehci_is_TDI(ehci) && HCC_LPM(hcc_params) ?
Well, that's pretty much how it ends up in the chipidea code, I set
has_hostpc from the lpm flag, which is read from HCCPARAMS in the
driver's probe. I'm about to resend the patchset with this change.
> - fix tdi_reset and tdi_in_host_mode to use the correct register (USBMODE or
> USBMODE_EX according to has_hostpc). May be we should move the code [1] in
> tdi_reset ?
Sounds sensible to me. At least it will make the code a bit more
understandable. But I'd like to do this outside of the chipidea patchset
(since the tdi_reset() currently does no harm).
> Is there a public datasheet to lpm chipidea controller ?
Unfortunately, I don't really know. I suspect not.
>
> Matthieu
>
> [1]
> if (ehci->has_hostpc) {
> ehci_writel(ehci, USBMODE_EX_HC | USBMODE_EX_VBPS,
> (u32 __iomem *)(((u8 *)ehci->regs) + USBMODE_EX));
> ehci_writel(ehci, TXFIFO_DEFAULT,
> (u32 __iomem *)(((u8 *)ehci->regs) + TXFILLTUNING));
> }
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
