Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Fri, Feb 13, 2015 at 02:03:45PM -0800, David Cohen wrote: > Hi Heikki, > > On Fri, Feb 13, 2015 at 03:16:40PM +0200, Heikki Krogerus wrote: > > On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote: > > > On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote: > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > > > index a8c9062..66cbf38 100644 > > > > > > --- a/drivers/usb/dwc3/core.c > > > > > > +++ b/drivers/usb/dwc3/core.c > > > > > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device > > > > > > *pdev) > > > > > > platform_set_drvdata(pdev, dwc); > > > > > > dwc3_cache_hwparams(dwc); > > > > > > > > > > > > + ret = dwc3_ulpi_init(dwc); > > > > > > > > > > If I understood correctly, this call will result in enumerating the > > > > > phy > > > > > via ULPI bus, then registering the correct ULPI device. > > > > > Can you guarantee ULPI will be always accessible at this point if we > > > > > remove dwc3 module and load it again? > > > > > > > > OK, got it. So yes, I can guarantee that ULPI will be acessible at > > > > this point. If we are in a state where we could soft reset dwc3, we > > > > know we can access ULPI. The fact that dwc3 itself expects to be able > > > > to write to the ULPI registers at that point guarantees it for us. > > > > > > I just double checked DWC3 TRM. > > > You are correct, by the time we're executing dwc3_core_soft_reset() ULPI > > > bus is already accessible. But the TRM also specifies an ULPI phy would > > > be reset by DCTL's core soft reset, which is executed by dwc3_core_init() > > > before calling dwc3_core_soft_reset(). It does mention DWC3 writes data > > > to an ULPI phy register during reset, but it also mentions the clock > > > sync happens during that time. > > > > > > That makes no even OK, but more correct IMO to power on phy before > > > core's soft reset (i.e. by ACPI methods). But it lets us in a grey area > > > whether ULPI is reliably accessible before core's soft reset. > > > > > > I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after > > > core's soft reset we've got no more grey area. > > > > Well, we have already requested the phys in dwc3_probe before that so > > some refactoring will be needed. It's probable no a problem. > > Sounds good :) > > > > > Btw, I'm sorry about telling this so late, but I'm going to be on > > vacation for the next two weeks. > > Thanks for the notice. you'll be back right around merge window closing time :-) Not to worry, we'll have time to get this into the next merge window. -- balbi signature.asc Description: Digital signature
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
Hi Heikki, On Fri, Feb 13, 2015 at 03:16:40PM +0200, Heikki Krogerus wrote: > On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote: > > On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote: > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > > index a8c9062..66cbf38 100644 > > > > > --- a/drivers/usb/dwc3/core.c > > > > > +++ b/drivers/usb/dwc3/core.c > > > > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device > > > > > *pdev) > > > > > platform_set_drvdata(pdev, dwc); > > > > > dwc3_cache_hwparams(dwc); > > > > > > > > > > + ret = dwc3_ulpi_init(dwc); > > > > > > > > If I understood correctly, this call will result in enumerating the phy > > > > via ULPI bus, then registering the correct ULPI device. > > > > Can you guarantee ULPI will be always accessible at this point if we > > > > remove dwc3 module and load it again? > > > > > > OK, got it. So yes, I can guarantee that ULPI will be acessible at > > > this point. If we are in a state where we could soft reset dwc3, we > > > know we can access ULPI. The fact that dwc3 itself expects to be able > > > to write to the ULPI registers at that point guarantees it for us. > > > > I just double checked DWC3 TRM. > > You are correct, by the time we're executing dwc3_core_soft_reset() ULPI > > bus is already accessible. But the TRM also specifies an ULPI phy would > > be reset by DCTL's core soft reset, which is executed by dwc3_core_init() > > before calling dwc3_core_soft_reset(). It does mention DWC3 writes data > > to an ULPI phy register during reset, but it also mentions the clock > > sync happens during that time. > > > > That makes no even OK, but more correct IMO to power on phy before > > core's soft reset (i.e. by ACPI methods). But it lets us in a grey area > > whether ULPI is reliably accessible before core's soft reset. > > > > I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after > > core's soft reset we've got no more grey area. > > Well, we have already requested the phys in dwc3_probe before that so > some refactoring will be needed. It's probable no a problem. Sounds good :) > > Btw, I'm sorry about telling this so late, but I'm going to be on > vacation for the next two weeks. Thanks for the notice. Br, David > > > Cheers, > > -- > heikki -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote: > On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote: > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > index a8c9062..66cbf38 100644 > > > > --- a/drivers/usb/dwc3/core.c > > > > +++ b/drivers/usb/dwc3/core.c > > > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) > > > > platform_set_drvdata(pdev, dwc); > > > > dwc3_cache_hwparams(dwc); > > > > > > > > + ret = dwc3_ulpi_init(dwc); > > > > > > If I understood correctly, this call will result in enumerating the phy > > > via ULPI bus, then registering the correct ULPI device. > > > Can you guarantee ULPI will be always accessible at this point if we > > > remove dwc3 module and load it again? > > > > OK, got it. So yes, I can guarantee that ULPI will be acessible at > > this point. If we are in a state where we could soft reset dwc3, we > > know we can access ULPI. The fact that dwc3 itself expects to be able > > to write to the ULPI registers at that point guarantees it for us. > > I just double checked DWC3 TRM. > You are correct, by the time we're executing dwc3_core_soft_reset() ULPI > bus is already accessible. But the TRM also specifies an ULPI phy would > be reset by DCTL's core soft reset, which is executed by dwc3_core_init() > before calling dwc3_core_soft_reset(). It does mention DWC3 writes data > to an ULPI phy register during reset, but it also mentions the clock > sync happens during that time. > > That makes no even OK, but more correct IMO to power on phy before > core's soft reset (i.e. by ACPI methods). But it lets us in a grey area > whether ULPI is reliably accessible before core's soft reset. > > I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after > core's soft reset we've got no more grey area. Well, we have already requested the phys in dwc3_probe before that so some refactoring will be needed. It's probable no a problem. Btw, I'm sorry about telling this so late, but I'm going to be on vacation for the next two weeks. Cheers, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote: On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote: diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index a8c9062..66cbf38 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) platform_set_drvdata(pdev, dwc); dwc3_cache_hwparams(dwc); + ret = dwc3_ulpi_init(dwc); If I understood correctly, this call will result in enumerating the phy via ULPI bus, then registering the correct ULPI device. Can you guarantee ULPI will be always accessible at this point if we remove dwc3 module and load it again? OK, got it. So yes, I can guarantee that ULPI will be acessible at this point. If we are in a state where we could soft reset dwc3, we know we can access ULPI. The fact that dwc3 itself expects to be able to write to the ULPI registers at that point guarantees it for us. I just double checked DWC3 TRM. You are correct, by the time we're executing dwc3_core_soft_reset() ULPI bus is already accessible. But the TRM also specifies an ULPI phy would be reset by DCTL's core soft reset, which is executed by dwc3_core_init() before calling dwc3_core_soft_reset(). It does mention DWC3 writes data to an ULPI phy register during reset, but it also mentions the clock sync happens during that time. That makes no even OK, but more correct IMO to power on phy before core's soft reset (i.e. by ACPI methods). But it lets us in a grey area whether ULPI is reliably accessible before core's soft reset. I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after core's soft reset we've got no more grey area. Well, we have already requested the phys in dwc3_probe before that so some refactoring will be needed. It's probable no a problem. Btw, I'm sorry about telling this so late, but I'm going to be on vacation for the next two weeks. Cheers, -- heikki -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Fri, Feb 13, 2015 at 02:03:45PM -0800, David Cohen wrote: Hi Heikki, On Fri, Feb 13, 2015 at 03:16:40PM +0200, Heikki Krogerus wrote: On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote: On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote: diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index a8c9062..66cbf38 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) platform_set_drvdata(pdev, dwc); dwc3_cache_hwparams(dwc); + ret = dwc3_ulpi_init(dwc); If I understood correctly, this call will result in enumerating the phy via ULPI bus, then registering the correct ULPI device. Can you guarantee ULPI will be always accessible at this point if we remove dwc3 module and load it again? OK, got it. So yes, I can guarantee that ULPI will be acessible at this point. If we are in a state where we could soft reset dwc3, we know we can access ULPI. The fact that dwc3 itself expects to be able to write to the ULPI registers at that point guarantees it for us. I just double checked DWC3 TRM. You are correct, by the time we're executing dwc3_core_soft_reset() ULPI bus is already accessible. But the TRM also specifies an ULPI phy would be reset by DCTL's core soft reset, which is executed by dwc3_core_init() before calling dwc3_core_soft_reset(). It does mention DWC3 writes data to an ULPI phy register during reset, but it also mentions the clock sync happens during that time. That makes no even OK, but more correct IMO to power on phy before core's soft reset (i.e. by ACPI methods). But it lets us in a grey area whether ULPI is reliably accessible before core's soft reset. I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after core's soft reset we've got no more grey area. Well, we have already requested the phys in dwc3_probe before that so some refactoring will be needed. It's probable no a problem. Sounds good :) Btw, I'm sorry about telling this so late, but I'm going to be on vacation for the next two weeks. Thanks for the notice. you'll be back right around merge window closing time :-) Not to worry, we'll have time to get this into the next merge window. -- balbi signature.asc Description: Digital signature
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
Hi Heikki, On Fri, Feb 13, 2015 at 03:16:40PM +0200, Heikki Krogerus wrote: On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote: On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote: diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index a8c9062..66cbf38 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) platform_set_drvdata(pdev, dwc); dwc3_cache_hwparams(dwc); + ret = dwc3_ulpi_init(dwc); If I understood correctly, this call will result in enumerating the phy via ULPI bus, then registering the correct ULPI device. Can you guarantee ULPI will be always accessible at this point if we remove dwc3 module and load it again? OK, got it. So yes, I can guarantee that ULPI will be acessible at this point. If we are in a state where we could soft reset dwc3, we know we can access ULPI. The fact that dwc3 itself expects to be able to write to the ULPI registers at that point guarantees it for us. I just double checked DWC3 TRM. You are correct, by the time we're executing dwc3_core_soft_reset() ULPI bus is already accessible. But the TRM also specifies an ULPI phy would be reset by DCTL's core soft reset, which is executed by dwc3_core_init() before calling dwc3_core_soft_reset(). It does mention DWC3 writes data to an ULPI phy register during reset, but it also mentions the clock sync happens during that time. That makes no even OK, but more correct IMO to power on phy before core's soft reset (i.e. by ACPI methods). But it lets us in a grey area whether ULPI is reliably accessible before core's soft reset. I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after core's soft reset we've got no more grey area. Well, we have already requested the phys in dwc3_probe before that so some refactoring will be needed. It's probable no a problem. Sounds good :) Btw, I'm sorry about telling this so late, but I'm going to be on vacation for the next two weeks. Thanks for the notice. Br, David Cheers, -- heikki -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote: > On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote: > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > index a8c9062..66cbf38 100644 > > > > --- a/drivers/usb/dwc3/core.c > > > > +++ b/drivers/usb/dwc3/core.c > > > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) > > > > platform_set_drvdata(pdev, dwc); > > > > dwc3_cache_hwparams(dwc); > > > > > > > > + ret = dwc3_ulpi_init(dwc); > > > > > > If I understood correctly, this call will result in enumerating the phy > > > via ULPI bus, then registering the correct ULPI device. > > > Can you guarantee ULPI will be always accessible at this point if we > > > remove dwc3 module and load it again? > > > > OK, got it. So yes, I can guarantee that ULPI will be acessible at > > this point. If we are in a state where we could soft reset dwc3, we > > know we can access ULPI. The fact that dwc3 itself expects to be able > > to write to the ULPI registers at that point guarantees it for us. > > I just double checked DWC3 TRM. > You are correct, by the time we're executing dwc3_core_soft_reset() ULPI > bus is already accessible. But the TRM also specifies an ULPI phy would > be reset by DCTL's core soft reset, which is executed by dwc3_core_init() > before calling dwc3_core_soft_reset(). It does mention DWC3 writes data > to an ULPI phy register during reset, but it also mentions the clock > sync happens during that time. > > That makes no even OK, but more correct IMO to power on phy before Sorry for the typo. I meant: That makes not only OK, but more correct... > core's soft reset (i.e. by ACPI methods). But it lets us in a grey area > whether ULPI is reliably accessible before core's soft reset. > > I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after > core's soft reset we've got no more grey area. > > Br, David > > > > > So in practice when ever dwc3 is powered we will be able to access > > ULPI for as long as the USB2 PHY interface is not suspended separately > > with GUSB2PHYCFG SusPHY bit. And even then we would only need to > > resume it with the same bit and ULPI is accessible again. > > > > > > Cheers, > > > > -- > > heikki -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote: > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index a8c9062..66cbf38 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) > > > platform_set_drvdata(pdev, dwc); > > > dwc3_cache_hwparams(dwc); > > > > > > + ret = dwc3_ulpi_init(dwc); > > > > If I understood correctly, this call will result in enumerating the phy > > via ULPI bus, then registering the correct ULPI device. > > Can you guarantee ULPI will be always accessible at this point if we > > remove dwc3 module and load it again? > > OK, got it. So yes, I can guarantee that ULPI will be acessible at > this point. If we are in a state where we could soft reset dwc3, we > know we can access ULPI. The fact that dwc3 itself expects to be able > to write to the ULPI registers at that point guarantees it for us. I just double checked DWC3 TRM. You are correct, by the time we're executing dwc3_core_soft_reset() ULPI bus is already accessible. But the TRM also specifies an ULPI phy would be reset by DCTL's core soft reset, which is executed by dwc3_core_init() before calling dwc3_core_soft_reset(). It does mention DWC3 writes data to an ULPI phy register during reset, but it also mentions the clock sync happens during that time. That makes no even OK, but more correct IMO to power on phy before core's soft reset (i.e. by ACPI methods). But it lets us in a grey area whether ULPI is reliably accessible before core's soft reset. I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after core's soft reset we've got no more grey area. Br, David > > So in practice when ever dwc3 is powered we will be able to access > ULPI for as long as the USB2 PHY interface is not suspended separately > with GUSB2PHYCFG SusPHY bit. And even then we would only need to > resume it with the same bit and ULPI is accessible again. > > > Cheers, > > -- > heikki -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index a8c9062..66cbf38 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, dwc); > > dwc3_cache_hwparams(dwc); > > > > + ret = dwc3_ulpi_init(dwc); > > If I understood correctly, this call will result in enumerating the phy > via ULPI bus, then registering the correct ULPI device. > Can you guarantee ULPI will be always accessible at this point if we > remove dwc3 module and load it again? OK, got it. So yes, I can guarantee that ULPI will be acessible at this point. If we are in a state where we could soft reset dwc3, we know we can access ULPI. The fact that dwc3 itself expects to be able to write to the ULPI registers at that point guarantees it for us. So in practice when ever dwc3 is powered we will be able to access ULPI for as long as the USB2 PHY interface is not suspended separately with GUSB2PHYCFG SusPHY bit. And even then we would only need to resume it with the same bit and ULPI is accessible again. Cheers, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index a8c9062..66cbf38 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) platform_set_drvdata(pdev, dwc); dwc3_cache_hwparams(dwc); + ret = dwc3_ulpi_init(dwc); If I understood correctly, this call will result in enumerating the phy via ULPI bus, then registering the correct ULPI device. Can you guarantee ULPI will be always accessible at this point if we remove dwc3 module and load it again? OK, got it. So yes, I can guarantee that ULPI will be acessible at this point. If we are in a state where we could soft reset dwc3, we know we can access ULPI. The fact that dwc3 itself expects to be able to write to the ULPI registers at that point guarantees it for us. So in practice when ever dwc3 is powered we will be able to access ULPI for as long as the USB2 PHY interface is not suspended separately with GUSB2PHYCFG SusPHY bit. And even then we would only need to resume it with the same bit and ULPI is accessible again. Cheers, -- heikki -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote: diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index a8c9062..66cbf38 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) platform_set_drvdata(pdev, dwc); dwc3_cache_hwparams(dwc); + ret = dwc3_ulpi_init(dwc); If I understood correctly, this call will result in enumerating the phy via ULPI bus, then registering the correct ULPI device. Can you guarantee ULPI will be always accessible at this point if we remove dwc3 module and load it again? OK, got it. So yes, I can guarantee that ULPI will be acessible at this point. If we are in a state where we could soft reset dwc3, we know we can access ULPI. The fact that dwc3 itself expects to be able to write to the ULPI registers at that point guarantees it for us. I just double checked DWC3 TRM. You are correct, by the time we're executing dwc3_core_soft_reset() ULPI bus is already accessible. But the TRM also specifies an ULPI phy would be reset by DCTL's core soft reset, which is executed by dwc3_core_init() before calling dwc3_core_soft_reset(). It does mention DWC3 writes data to an ULPI phy register during reset, but it also mentions the clock sync happens during that time. That makes no even OK, but more correct IMO to power on phy before core's soft reset (i.e. by ACPI methods). But it lets us in a grey area whether ULPI is reliably accessible before core's soft reset. I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after core's soft reset we've got no more grey area. Br, David So in practice when ever dwc3 is powered we will be able to access ULPI for as long as the USB2 PHY interface is not suspended separately with GUSB2PHYCFG SusPHY bit. And even then we would only need to resume it with the same bit and ULPI is accessible again. Cheers, -- heikki -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote: On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote: diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index a8c9062..66cbf38 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) platform_set_drvdata(pdev, dwc); dwc3_cache_hwparams(dwc); + ret = dwc3_ulpi_init(dwc); If I understood correctly, this call will result in enumerating the phy via ULPI bus, then registering the correct ULPI device. Can you guarantee ULPI will be always accessible at this point if we remove dwc3 module and load it again? OK, got it. So yes, I can guarantee that ULPI will be acessible at this point. If we are in a state where we could soft reset dwc3, we know we can access ULPI. The fact that dwc3 itself expects to be able to write to the ULPI registers at that point guarantees it for us. I just double checked DWC3 TRM. You are correct, by the time we're executing dwc3_core_soft_reset() ULPI bus is already accessible. But the TRM also specifies an ULPI phy would be reset by DCTL's core soft reset, which is executed by dwc3_core_init() before calling dwc3_core_soft_reset(). It does mention DWC3 writes data to an ULPI phy register during reset, but it also mentions the clock sync happens during that time. That makes no even OK, but more correct IMO to power on phy before Sorry for the typo. I meant: That makes not only OK, but more correct... core's soft reset (i.e. by ACPI methods). But it lets us in a grey area whether ULPI is reliably accessible before core's soft reset. I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after core's soft reset we've got no more grey area. Br, David So in practice when ever dwc3 is powered we will be able to access ULPI for as long as the USB2 PHY interface is not suspended separately with GUSB2PHYCFG SusPHY bit. And even then we would only need to resume it with the same bit and ULPI is accessible again. Cheers, -- heikki -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
Hi Heikki, On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote: > Registers DWC3's ULPI interface with the ULPI bus when it's > available. > > Signed-off-by: Heikki Krogerus > --- > drivers/usb/dwc3/Kconfig | 7 +++ > drivers/usb/dwc3/Makefile | 4 ++ > drivers/usb/dwc3/core.c | 6 +++ > drivers/usb/dwc3/core.h | 13 ++ > drivers/usb/dwc3/ulpi.c | 112 > ++ > 5 files changed, 142 insertions(+) > create mode 100644 drivers/usb/dwc3/ulpi.c > > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig > index 58b5b2c..cda82ad 100644 > --- a/drivers/usb/dwc3/Kconfig > +++ b/drivers/usb/dwc3/Kconfig > @@ -11,6 +11,13 @@ config USB_DWC3 > > if USB_DWC3 > > +config USB_DWC3_ULPI > + bool "Register ULPI PHY Interface" > + depends on USB_ULPI_BUS=y > + help > + Select this if you have ULPI type PHY attached to your DWC3 > + controller. > + > choice > bool "DWC3 Mode Selection" > default USB_DWC3_DUAL_ROLE if (USB && USB_GADGET) > diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile > index bb34fbc..2fc44e0 100644 > --- a/drivers/usb/dwc3/Makefile > +++ b/drivers/usb/dwc3/Makefile > @@ -16,6 +16,10 @@ ifneq ($(filter y,$(CONFIG_USB_DWC3_GADGET) > $(CONFIG_USB_DWC3_DUAL_ROLE)),) > dwc3-y += gadget.o ep0.o > endif > > +ifneq ($(CONFIG_USB_DWC3_ULPI),) > + dwc3-y += ulpi.o > +endif > + > ifneq ($(CONFIG_DEBUG_FS),) > dwc3-y += debugfs.o > endif > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index a8c9062..66cbf38 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, dwc); > dwc3_cache_hwparams(dwc); > > + ret = dwc3_ulpi_init(dwc); If I understood correctly, this call will result in enumerating the phy via ULPI bus, then registering the correct ULPI device. Can you guarantee ULPI will be always accessible at this point if we remove dwc3 module and load it again? Br, David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
Hi Heikki, On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote: Registers DWC3's ULPI interface with the ULPI bus when it's available. Signed-off-by: Heikki Krogerus heikki.kroge...@linux.intel.com --- drivers/usb/dwc3/Kconfig | 7 +++ drivers/usb/dwc3/Makefile | 4 ++ drivers/usb/dwc3/core.c | 6 +++ drivers/usb/dwc3/core.h | 13 ++ drivers/usb/dwc3/ulpi.c | 112 ++ 5 files changed, 142 insertions(+) create mode 100644 drivers/usb/dwc3/ulpi.c diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index 58b5b2c..cda82ad 100644 --- a/drivers/usb/dwc3/Kconfig +++ b/drivers/usb/dwc3/Kconfig @@ -11,6 +11,13 @@ config USB_DWC3 if USB_DWC3 +config USB_DWC3_ULPI + bool Register ULPI PHY Interface + depends on USB_ULPI_BUS=y + help + Select this if you have ULPI type PHY attached to your DWC3 + controller. + choice bool DWC3 Mode Selection default USB_DWC3_DUAL_ROLE if (USB USB_GADGET) diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile index bb34fbc..2fc44e0 100644 --- a/drivers/usb/dwc3/Makefile +++ b/drivers/usb/dwc3/Makefile @@ -16,6 +16,10 @@ ifneq ($(filter y,$(CONFIG_USB_DWC3_GADGET) $(CONFIG_USB_DWC3_DUAL_ROLE)),) dwc3-y += gadget.o ep0.o endif +ifneq ($(CONFIG_USB_DWC3_ULPI),) + dwc3-y += ulpi.o +endif + ifneq ($(CONFIG_DEBUG_FS),) dwc3-y += debugfs.o endif diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index a8c9062..66cbf38 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev) platform_set_drvdata(pdev, dwc); dwc3_cache_hwparams(dwc); + ret = dwc3_ulpi_init(dwc); If I understood correctly, this call will result in enumerating the phy via ULPI bus, then registering the correct ULPI device. Can you guarantee ULPI will be always accessible at this point if we remove dwc3 module and load it again? Br, David -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Tue, Jan 27, 2015 at 01:09:25PM +0200, Heikki Krogerus wrote: > > look at your patch again. In case DWC3_ULPI isn't enabled, this file > > won't be linked at all. You're using stubs, so taht's fine. > > > > In case it _is_ enabled, you're breaking out early if you can't register > > ulpi interface, meaning you're exitting probe() (which, in fact, seems > > wrong as I want to be able to run dwc3 with ULPI enabled on a platform > > that was configured with ULPI+UTMI, but uses UTMI PHY). > > > > I still think this patch won't work in all cases. > > OK. So in case of ULPI+UTMI we'll try the ulpi interface, and if it > fails, fall back to UTMI. Or can we use some other method to determine > to which interface the PHY is really attached to in that case? I think we would need a phy_interface_sel pdata/DT binding for those cases. It should be optional and probably only needed for a few odd devices. -- balbi signature.asc Description: Digital signature
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
> look at your patch again. In case DWC3_ULPI isn't enabled, this file > won't be linked at all. You're using stubs, so taht's fine. > > In case it _is_ enabled, you're breaking out early if you can't register > ulpi interface, meaning you're exitting probe() (which, in fact, seems > wrong as I want to be able to run dwc3 with ULPI enabled on a platform > that was configured with ULPI+UTMI, but uses UTMI PHY). > > I still think this patch won't work in all cases. OK. So in case of ULPI+UTMI we'll try the ulpi interface, and if it fails, fall back to UTMI. Or can we use some other method to determine to which interface the PHY is really attached to in that case? Thanks, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
look at your patch again. In case DWC3_ULPI isn't enabled, this file won't be linked at all. You're using stubs, so taht's fine. In case it _is_ enabled, you're breaking out early if you can't register ulpi interface, meaning you're exitting probe() (which, in fact, seems wrong as I want to be able to run dwc3 with ULPI enabled on a platform that was configured with ULPI+UTMI, but uses UTMI PHY). I still think this patch won't work in all cases. OK. So in case of ULPI+UTMI we'll try the ulpi interface, and if it fails, fall back to UTMI. Or can we use some other method to determine to which interface the PHY is really attached to in that case? Thanks, -- heikki -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Tue, Jan 27, 2015 at 01:09:25PM +0200, Heikki Krogerus wrote: look at your patch again. In case DWC3_ULPI isn't enabled, this file won't be linked at all. You're using stubs, so taht's fine. In case it _is_ enabled, you're breaking out early if you can't register ulpi interface, meaning you're exitting probe() (which, in fact, seems wrong as I want to be able to run dwc3 with ULPI enabled on a platform that was configured with ULPI+UTMI, but uses UTMI PHY). I still think this patch won't work in all cases. OK. So in case of ULPI+UTMI we'll try the ulpi interface, and if it fails, fall back to UTMI. Or can we use some other method to determine to which interface the PHY is really attached to in that case? I think we would need a phy_interface_sel pdata/DT binding for those cases. It should be optional and probably only needed for a few odd devices. -- balbi signature.asc Description: Digital signature
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Mon, Jan 26, 2015 at 01:46:10PM +0200, Heikki Krogerus wrote: > On Fri, Jan 23, 2015 at 10:24:43AM -0600, Felipe Balbi wrote: > > On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote: > > > +int dwc3_ulpi_init(struct dwc3 *dwc) > > > +{ > > > + u32 reg; > > > + > > > + /* First check USB2 PHY interface type */ > > > + switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) { > > > + case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI: > > > + /* Select ULPI Interface */ > > > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > > > + reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI; > > > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > > > + /* FALLTHROUGH */ > > > + case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI: > > > + break; > > > + default: > > > + return 0; > > > + } > > > + > > > + /* Register the interface */ > > > + dwc->ulpi = ulpi_register_interface(dwc->dev, _ulpi); > > > + if (IS_ERR(dwc->ulpi)) { > > > > so, this will only build and link if DWC3_ULPI is enabled, in case of > > error you return early... > > > > > + dev_err(dwc->dev, "failed to register ULPI interface"); > > > + return PTR_ERR(dwc->ulpi); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +void dwc3_ulpi_exit(struct dwc3 *dwc) > > > +{ > > > + if (dwc->ulpi) { > > > > ... looks like this branch is unnecessary. > > We can't do that, or distros that select DWC3_ULPI option can only use > dwc3 with hardware that really has ULPI PHY. So I guess we'll drop the > DWC3_ULPI option and build the dwc3 ulpi support always if ULPI bus is > enabled. OK? look at your patch again. In case DWC3_ULPI isn't enabled, this file won't be linked at all. You're using stubs, so taht's fine. In case it _is_ enabled, you're breaking out early if you can't register ulpi interface, meaning you're exitting probe() (which, in fact, seems wrong as I want to be able to run dwc3 with ULPI enabled on a platform that was configured with ULPI+UTMI, but uses UTMI PHY). I still think this patch won't work in all cases. -- balbi signature.asc Description: Digital signature
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Fri, Jan 23, 2015 at 10:24:43AM -0600, Felipe Balbi wrote: > On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote: > > +int dwc3_ulpi_init(struct dwc3 *dwc) > > +{ > > + u32 reg; > > + > > + /* First check USB2 PHY interface type */ > > + switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) { > > + case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI: > > + /* Select ULPI Interface */ > > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > > + reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI; > > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > > + /* FALLTHROUGH */ > > + case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI: > > + break; > > + default: > > + return 0; > > + } > > + > > + /* Register the interface */ > > + dwc->ulpi = ulpi_register_interface(dwc->dev, _ulpi); > > + if (IS_ERR(dwc->ulpi)) { > > so, this will only build and link if DWC3_ULPI is enabled, in case of > error you return early... > > > + dev_err(dwc->dev, "failed to register ULPI interface"); > > + return PTR_ERR(dwc->ulpi); > > + } > > + > > + return 0; > > +} > > + > > +void dwc3_ulpi_exit(struct dwc3 *dwc) > > +{ > > + if (dwc->ulpi) { > > ... looks like this branch is unnecessary. We can't do that, or distros that select DWC3_ULPI option can only use dwc3 with hardware that really has ULPI PHY. So I guess we'll drop the DWC3_ULPI option and build the dwc3 ulpi support always if ULPI bus is enabled. OK? Thanks, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Mon, Jan 26, 2015 at 01:46:10PM +0200, Heikki Krogerus wrote: On Fri, Jan 23, 2015 at 10:24:43AM -0600, Felipe Balbi wrote: On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote: +int dwc3_ulpi_init(struct dwc3 *dwc) +{ + u32 reg; + + /* First check USB2 PHY interface type */ + switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc-hwparams.hwparams3)) { + case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI: + /* Select ULPI Interface */ + reg = dwc3_readl(dwc-regs, DWC3_GUSB2PHYCFG(0)); + reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI; + dwc3_writel(dwc-regs, DWC3_GUSB2PHYCFG(0), reg); + /* FALLTHROUGH */ + case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI: + break; + default: + return 0; + } + + /* Register the interface */ + dwc-ulpi = ulpi_register_interface(dwc-dev, dwc3_ulpi); + if (IS_ERR(dwc-ulpi)) { so, this will only build and link if DWC3_ULPI is enabled, in case of error you return early... + dev_err(dwc-dev, failed to register ULPI interface); + return PTR_ERR(dwc-ulpi); + } + + return 0; +} + +void dwc3_ulpi_exit(struct dwc3 *dwc) +{ + if (dwc-ulpi) { ... looks like this branch is unnecessary. We can't do that, or distros that select DWC3_ULPI option can only use dwc3 with hardware that really has ULPI PHY. So I guess we'll drop the DWC3_ULPI option and build the dwc3 ulpi support always if ULPI bus is enabled. OK? look at your patch again. In case DWC3_ULPI isn't enabled, this file won't be linked at all. You're using stubs, so taht's fine. In case it _is_ enabled, you're breaking out early if you can't register ulpi interface, meaning you're exitting probe() (which, in fact, seems wrong as I want to be able to run dwc3 with ULPI enabled on a platform that was configured with ULPI+UTMI, but uses UTMI PHY). I still think this patch won't work in all cases. -- balbi signature.asc Description: Digital signature
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
On Fri, Jan 23, 2015 at 10:24:43AM -0600, Felipe Balbi wrote: On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote: +int dwc3_ulpi_init(struct dwc3 *dwc) +{ + u32 reg; + + /* First check USB2 PHY interface type */ + switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc-hwparams.hwparams3)) { + case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI: + /* Select ULPI Interface */ + reg = dwc3_readl(dwc-regs, DWC3_GUSB2PHYCFG(0)); + reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI; + dwc3_writel(dwc-regs, DWC3_GUSB2PHYCFG(0), reg); + /* FALLTHROUGH */ + case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI: + break; + default: + return 0; + } + + /* Register the interface */ + dwc-ulpi = ulpi_register_interface(dwc-dev, dwc3_ulpi); + if (IS_ERR(dwc-ulpi)) { so, this will only build and link if DWC3_ULPI is enabled, in case of error you return early... + dev_err(dwc-dev, failed to register ULPI interface); + return PTR_ERR(dwc-ulpi); + } + + return 0; +} + +void dwc3_ulpi_exit(struct dwc3 *dwc) +{ + if (dwc-ulpi) { ... looks like this branch is unnecessary. We can't do that, or distros that select DWC3_ULPI option can only use dwc3 with hardware that really has ULPI PHY. So I guess we'll drop the DWC3_ULPI option and build the dwc3 ulpi support always if ULPI bus is enabled. OK? Thanks, -- heikki -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
Hi, On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote: > +int dwc3_ulpi_init(struct dwc3 *dwc) > +{ > + u32 reg; > + > + /* First check USB2 PHY interface type */ > + switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) { > + case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI: > + /* Select ULPI Interface */ > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > + reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI; > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > + /* FALLTHROUGH */ > + case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI: > + break; > + default: > + return 0; > + } > + > + /* Register the interface */ > + dwc->ulpi = ulpi_register_interface(dwc->dev, _ulpi); > + if (IS_ERR(dwc->ulpi)) { so, this will only build and link if DWC3_ULPI is enabled, in case of error you return early... > + dev_err(dwc->dev, "failed to register ULPI interface"); > + return PTR_ERR(dwc->ulpi); > + } > + > + return 0; > +} > + > +void dwc3_ulpi_exit(struct dwc3 *dwc) > +{ > + if (dwc->ulpi) { ... looks like this branch is unnecessary. -- balbi signature.asc Description: Digital signature
Re: [PATCH 6/8] usb: dwc3: add ULPI interface support
Hi, On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote: +int dwc3_ulpi_init(struct dwc3 *dwc) +{ + u32 reg; + + /* First check USB2 PHY interface type */ + switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc-hwparams.hwparams3)) { + case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI: + /* Select ULPI Interface */ + reg = dwc3_readl(dwc-regs, DWC3_GUSB2PHYCFG(0)); + reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI; + dwc3_writel(dwc-regs, DWC3_GUSB2PHYCFG(0), reg); + /* FALLTHROUGH */ + case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI: + break; + default: + return 0; + } + + /* Register the interface */ + dwc-ulpi = ulpi_register_interface(dwc-dev, dwc3_ulpi); + if (IS_ERR(dwc-ulpi)) { so, this will only build and link if DWC3_ULPI is enabled, in case of error you return early... + dev_err(dwc-dev, failed to register ULPI interface); + return PTR_ERR(dwc-ulpi); + } + + return 0; +} + +void dwc3_ulpi_exit(struct dwc3 *dwc) +{ + if (dwc-ulpi) { ... looks like this branch is unnecessary. -- balbi signature.asc Description: Digital signature