> > > > You can't really compare a bus like i2c, which can't enumerate devices
> > > > natively, to ULPI which can.
> > >
> > > why not ? The BIOS might not need to use the PHY (or USB) at all, it can
> > > very well decide to never turn it on, right ?
> >
> > If ULPI was seen as a bus, then no. BIOS would have definitely left
> > the PHY on. In fact, if we would have just asked the BIOS writers to
> > leave it on, they would not have any problem with that, even without
> > the bus.
>
> That's a really wrong assumption. ULPI bus depends on dwc3 to be
> functional and dwc3 depends on phy to be functional to complete its
> power on sequence. We can't ask BIOS to let both up and running all the
> time.
>
> FWIW we *cannot* rely on ULPI bus enumeration to probe ULPI devices,
> because it requires the ULPI device to be previously functional which
> can't happen before the enumeration. Even if we ask BIOS to let phy
> functional after boot, what happens when we remove modules and load it
> again? Should we ask BIOS to power on the components again in order to
> probe and power it on? It's a circular dependency you're creating.
>
> >
> > > > I don't think the other boards we have which use TUSB1210, like the
> > > > BYT-I ones and I think some Merrifield based boards, expect any less
> > > > from PM efficiency then BYT-CR, but we don't need to do any tricks
> > > > with them in order to use ULPI bus. That is what I mean when I say
> > > > this is BYT-CR specific problem.
> > >
> > > perhaps because firmware on those other boards are powering up the PHY ?
> >
> > Yes.
>
> And that's wrong assumption. Not all fw will power on PHY. Maybe we
> should stop all of other discussions and concentrate on this one:
> BIOS will not guarantee phy will be functional after boot. And if we
> remove modules and load again, it won't be functional regardless what
> BIOS did during boot.
I was wrong when I talked about BIOS powering the PHY before bootup. I
admit it. I'm saying this again as a reminder to myself: We can't mix
BIOS (or any FW) and ACPI. I mix them constantly. And I definitely
need to stop talking about bootup.
How this is handled is by letting the ACPI Power State methods of the
dwc3 device take care of the toggling of the gpios. It is done with
the help of something called GPIO OperationRegion. In case you are not
familiar with ACPI, then if you send me ACPI dump of one of those
devices (or just copy /sys/firmware/acpi/tables/DSDT), I can try to
modify the DSDT for you so you can use to test it, and provide you
with the ASL snippet.
You will see that the PHY is indeed in reset after bootup like it is
now (BIOS does nothing differently), but the gpios are automatically
toggled by the DSDT code. So every time you load dwc3 module, the PHY
will be operational when we need it, and when you unload dwc3, it will
be left in reset again. The OS does not have to do anything.
I really think that this BYT-CR case will be one of really rare
exception we will see, especially if we manage to put together the
ACPI table review that has been though about.
> > > > I don't agree with PM arguments if it means that we should be ready to
> > > > accept loosing possibility for a generic solution in OS with a single
> > > > device like our PHY. I seriously doubt it would prevent the products
> > > > using these boards of achieving their PM requirements. But this
> > > > conversation is outside our topic.
> > >
> > > we're not loosing anything. We're just considering what's the best way
> > > to tackle that ulpi_read() inside probe(). TUSB1210 driver _has_ to cope
> > > with situations where reset_gpio/cs_gpio are in unexpected state. Saying
> > > we will just "fix the firmware", as if that was a simple feat, is
> > > counter-productive.
> >
> > You know guys, we shouldn't always just lay down and say, "we just
> > have to accept it can be anything" or "we just have to try to prepare
> > for everything". We can influence these things, and we should. We can
> > influence these things inside our own companies before any products is
> > launched using our SoCs, and since more and more companies are
> > releasing their code into the public before their product are
> > launched, we even have a change to influence others. Lack of standards
> > does not mean we should not try to achieve consistency.
> >
> > For example, now I should probable write to Documentation that "ULPI
> > PHY needs to be in condition where it's register can be accessed
> > before the interface is registered.", and I'm pretty sure it would be
> > enough to have an effect on many of the new platforms that use ULPI
> > PHYs.
>
> In order for phy to be functional, it does not depend only on toggling
> GPIOs. It depends on DWC3 going to reset state, then phy executes power
> on sequence, then DWC3 going out of reset state to sync clocks with phy.
> You're saying we should tell BIOS is concurrently mess with dwc3
> together with dwc3 driver?
I don't understand what you are saying here?
> > > > Because of the need to write to the ULPI registers, I don't think we
> > > > should try anything else except to use ULPI bus straight away. We'll
> > >
> > > I'll agree with this.
> > >
> > > > start by making use of ULPI bus possible by adding the quirk for BYT
> > > > (attached), which to me is perfectly OK solution. I would appreciate
> > > > if you gave it a review.
> > >
> > > it's not perfectly ok for dwc3 to toggle PHY's GPIOs. Have the PHY
> > > driver to that.
> >
> > Oh, I agree with that..
> >
> > > > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> > > > index 8d95056..53902ea 100644
> > > > --- a/drivers/usb/dwc3/dwc3-pci.c
> > > > +++ b/drivers/usb/dwc3/dwc3-pci.c
> > > > @@ -21,6 +21,7 @@
> > > > #include <linux/slab.h>
> > > > #include <linux/pci.h>
> > > > #include <linux/platform_device.h>
> > > > +#include <linux/gpio/consumer.h>
> > > >
> > > > #include "platform_data.h"
> > > >
> > > > @@ -35,6 +36,24 @@
> > > >
> > > > static int dwc3_pci_quirks(struct pci_dev *pdev)
> > > > {
> > > > + if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > > + pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > > + struct gpio_desc *gpio;
> > > > +
> > > > + gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > > > + if (!IS_ERR(gpio)) {
> > > > + gpiod_direction_output(gpio, 0);
> > > > + gpiod_set_value_cansleep(gpio, 1);
> > > > + gpiod_put(gpio);
> > > > + }
> > > > + gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > > > + if (!IS_ERR(gpio)) {
> > > > + gpiod_direction_output(gpio, 0);
> > > > + gpiod_set_value_cansleep(gpio, 1);
> > > > + gpiod_put(gpio);
> > > > + }
> > > > + }
> > >
> > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > > very good.
> >
> > ..but unfortunately we can't use the bus without it :(. We depend on
> > being able to read the vendor and product id's in the bus driver.
>
> Doesn't the ugly platform device case look less ugly than this?
The platform device would in any case need to be created only for this
case. We can't any more just create the phy device unconditionally for
every PCI platform like it was created before, as it's clear now the
PHY device can be be created from other sources. It was a risk always.
But the big problem is that since the PHY on your board is ULPI PHY,
it will make it really difficult to add the ULPI bus support. And like
I said in my previous mail, we would really need it for the boards
that expect the PHY driver to provide functions like charger
detection. We don't need it only for BYT based boards.
So on top of the above, since the GPIO resources are given to dwc3, I
actually think that my hack is better then the platform device.
Thanks,
--
heikki
Hi David, Felipe,
> > > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > > > very good.
> > >
> > > ..but unfortunately we can't use the bus without it :(. We depend on
> > > being able to read the vendor and product id's in the bus driver.
> >
> > Doesn't the ugly platform device case look less ugly than this?
>
> The platform device would in any case need to be created only for this
> case. We can't any more just create the phy device unconditionally for
> every PCI platform like it was created before, as it's clear now the
> PHY device can be be created from other sources. It was a risk always.
>
> But the big problem is that since the PHY on your board is ULPI PHY,
> it will make it really difficult to add the ULPI bus support. And like
> I said in my previous mail, we would really need it for the boards
> that expect the PHY driver to provide functions like charger
> detection. We don't need it only for BYT based boards.
>
> So on top of the above, since the GPIO resources are given to dwc3, I
> actually think that my hack is better then the platform device.
So what do you guys think we should do? I can't figure out how would
we make the platform device work with ulpi bus. If we think about
handling this in ulpi bus driver by setting setting the gpios before
attempting to access the phy, which I'm not completely against, we
have couple of problems.
Firstly, the gpio resources are given to the dwc3 in this case,
while normally they will be given to separate device object for the
phy.
Secondly, these gpios were not labeled in DSDT, but apparently
requesting the gpios with index will be deprecated and not acceptable
any more. With the remaining platforms that have not labeled the gpios
we have to bind the gpios to labels separately in the drivers. With
ACPI platforms the labels are introduced in struct acpi_gpio_mapping
which needs to be registered with acpi_dev_add_driver_gpios(). Check
net/rfkill/rfkill-gpio.c as an example how to use it.
I think those points would make this too platform specific case to be
handled in the ulpi bus driver.
Suggestions? I still think the correct thing to do is to handle this
in the quirk in dwc3-pci.c.
Cheers,
--
heikki
On Tue, Feb 03, 2015 at 01:37:39PM +0200, Heikki Krogerus wrote:
> Hi David, Felipe,
Hi Heikki,
>
> > > > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > > > > very good.
> > > >
> > > > ..but unfortunately we can't use the bus without it :(. We depend on
> > > > being able to read the vendor and product id's in the bus driver.
> > >
> > > Doesn't the ugly platform device case look less ugly than this?
> >
> > The platform device would in any case need to be created only for this
> > case. We can't any more just create the phy device unconditionally for
> > every PCI platform like it was created before, as it's clear now the
> > PHY device can be be created from other sources. It was a risk always.
> >
> > But the big problem is that since the PHY on your board is ULPI PHY,
> > it will make it really difficult to add the ULPI bus support. And like
> > I said in my previous mail, we would really need it for the boards
> > that expect the PHY driver to provide functions like charger
> > detection. We don't need it only for BYT based boards.
> >
> > So on top of the above, since the GPIO resources are given to dwc3, I
> > actually think that my hack is better then the platform device.
>
> So what do you guys think we should do? I can't figure out how would
> we make the platform device work with ulpi bus. If we think about
> handling this in ulpi bus driver by setting setting the gpios before
> attempting to access the phy, which I'm not completely against, we
> have couple of problems.
It's still not enough :/
In order to make the phy functional (in BYT case) you need to bring DWC3
to reset state, then toggle these gpios to reset and power on phy, then
remove DWC3 from reset state so both can sync clocks.
You can still reset and power on phy before DWC3 goes to and from reset,
but then there's a chance phy may become unstable. It's then expected to
fail long stability tests.
>
> Firstly, the gpio resources are given to the dwc3 in this case,
> while normally they will be given to separate device object for the
> phy.
Yup. You're correct. We can influence new products to not repeat this
issue, but we still need to support legacy ones.
>
> Secondly, these gpios were not labeled in DSDT, but apparently
> requesting the gpios with index will be deprecated and not acceptable
> any more. With the remaining platforms that have not labeled the gpios
> we have to bind the gpios to labels separately in the drivers. With
> ACPI platforms the labels are introduced in struct acpi_gpio_mapping
> which needs to be registered with acpi_dev_add_driver_gpios(). Check
> net/rfkill/rfkill-gpio.c as an example how to use it.
For new products, certanly.
>
> I think those points would make this too platform specific case to be
> handled in the ulpi bus driver.
>
> Suggestions? I still think the correct thing to do is to handle this
> in the quirk in dwc3-pci.c.
IMO it would make things uglier than it was before.
BR, David
>
>
> Cheers,
>
> --
> heikki
On Mon, Feb 02, 2015 at 02:59:59PM +0200, Heikki Krogerus wrote:
> > > > > You can't really compare a bus like i2c, which can't enumerate devices
> > > > > natively, to ULPI which can.
> > > >
> > > > why not ? The BIOS might not need to use the PHY (or USB) at all, it can
> > > > very well decide to never turn it on, right ?
> > >
> > > If ULPI was seen as a bus, then no. BIOS would have definitely left
> > > the PHY on. In fact, if we would have just asked the BIOS writers to
> > > leave it on, they would not have any problem with that, even without
> > > the bus.
> >
> > That's a really wrong assumption. ULPI bus depends on dwc3 to be
> > functional and dwc3 depends on phy to be functional to complete its
> > power on sequence. We can't ask BIOS to let both up and running all the
> > time.
> >
> > FWIW we *cannot* rely on ULPI bus enumeration to probe ULPI devices,
> > because it requires the ULPI device to be previously functional which
> > can't happen before the enumeration. Even if we ask BIOS to let phy
> > functional after boot, what happens when we remove modules and load it
> > again? Should we ask BIOS to power on the components again in order to
> > probe and power it on? It's a circular dependency you're creating.
> >
> > >
> > > > > I don't think the other boards we have which use TUSB1210, like the
> > > > > BYT-I ones and I think some Merrifield based boards, expect any less
> > > > > from PM efficiency then BYT-CR, but we don't need to do any tricks
> > > > > with them in order to use ULPI bus. That is what I mean when I say
> > > > > this is BYT-CR specific problem.
> > > >
> > > > perhaps because firmware on those other boards are powering up the PHY ?
> > >
> > > Yes.
> >
> > And that's wrong assumption. Not all fw will power on PHY. Maybe we
> > should stop all of other discussions and concentrate on this one:
> > BIOS will not guarantee phy will be functional after boot. And if we
> > remove modules and load again, it won't be functional regardless what
> > BIOS did during boot.
>
> I was wrong when I talked about BIOS powering the PHY before bootup. I
> admit it. I'm saying this again as a reminder to myself: We can't mix
> BIOS (or any FW) and ACPI. I mix them constantly. And I definitely
> need to stop talking about bootup.
>
> How this is handled is by letting the ACPI Power State methods of the
> dwc3 device take care of the toggling of the gpios. It is done with
> the help of something called GPIO OperationRegion. In case you are not
> familiar with ACPI, then if you send me ACPI dump of one of those
> devices (or just copy /sys/firmware/acpi/tables/DSDT), I can try to
> modify the DSDT for you so you can use to test it, and provide you
> with the ASL snippet.
>
> You will see that the PHY is indeed in reset after bootup like it is
> now (BIOS does nothing differently), but the gpios are automatically
> toggled by the DSDT code. So every time you load dwc3 module, the PHY
> will be operational when we need it, and when you unload dwc3, it will
> be left in reset again. The OS does not have to do anything.
Hm. That changes everything :)
It's a feasible direction to push vendors.
>
> I really think that this BYT-CR case will be one of really rare
> exception we will see, especially if we manage to put together the
> ACPI table review that has been though about.
I hope it will.
>
> > > > > I don't agree with PM arguments if it means that we should be ready to
> > > > > accept loosing possibility for a generic solution in OS with a single
> > > > > device like our PHY. I seriously doubt it would prevent the products
> > > > > using these boards of achieving their PM requirements. But this
> > > > > conversation is outside our topic.
> > > >
> > > > we're not loosing anything. We're just considering what's the best way
> > > > to tackle that ulpi_read() inside probe(). TUSB1210 driver _has_ to cope
> > > > with situations where reset_gpio/cs_gpio are in unexpected state. Saying
> > > > we will just "fix the firmware", as if that was a simple feat, is
> > > > counter-productive.
> > >
> > > You know guys, we shouldn't always just lay down and say, "we just
> > > have to accept it can be anything" or "we just have to try to prepare
> > > for everything". We can influence these things, and we should. We can
> > > influence these things inside our own companies before any products is
> > > launched using our SoCs, and since more and more companies are
> > > releasing their code into the public before their product are
> > > launched, we even have a change to influence others. Lack of standards
> > > does not mean we should not try to achieve consistency.
> > >
> > > For example, now I should probable write to Documentation that "ULPI
> > > PHY needs to be in condition where it's register can be accessed
> > > before the interface is registered.", and I'm pretty sure it would be
> > > enough to have an effect on many of the new platforms that use ULPI
> > > PHYs.
> >
> > In order for phy to be functional, it does not depend only on toggling
> > GPIOs. It depends on DWC3 going to reset state, then phy executes power
> > on sequence, then DWC3 going out of reset state to sync clocks with phy.
> > You're saying we should tell BIOS is concurrently mess with dwc3
> > together with dwc3 driver?
>
> I don't understand what you are saying here?
TUSB1210 needs to come out of reset only when DWC3 is in reset state.
This is how current code works in dwc3_core_soft_reset():
- dwc3 goes to reset
- phy goes to reset
- phy gets out of reset
- dwc3 gets out of reset
This is how you're proposing:
- phy goes to reset (DSDT code, when loading module)
- phy gets out of reset (DSDT code, when loading module)
- dwc3 goes to reset (dwc3_core_soft_reset())
- dwc3 gets our of reset (dwc3_core_soft_reset())
Felipe, do you see a problem with this new context? If not, I'm
satisfied with Heikki's ULPI bus proposal considering my comment below.
>
> > > > > Because of the need to write to the ULPI registers, I don't think we
> > > > > should try anything else except to use ULPI bus straight away. We'll
> > > >
> > > > I'll agree with this.
> > > >
> > > > > start by making use of ULPI bus possible by adding the quirk for BYT
> > > > > (attached), which to me is perfectly OK solution. I would appreciate
> > > > > if you gave it a review.
> > > >
> > > > it's not perfectly ok for dwc3 to toggle PHY's GPIOs. Have the PHY
> > > > driver to that.
> > >
> > > Oh, I agree with that..
> > >
> > > > > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> > > > > index 8d95056..53902ea 100644
> > > > > --- a/drivers/usb/dwc3/dwc3-pci.c
> > > > > +++ b/drivers/usb/dwc3/dwc3-pci.c
> > > > > @@ -21,6 +21,7 @@
> > > > > #include <linux/slab.h>
> > > > > #include <linux/pci.h>
> > > > > #include <linux/platform_device.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > >
> > > > > #include "platform_data.h"
> > > > >
> > > > > @@ -35,6 +36,24 @@
> > > > >
> > > > > static int dwc3_pci_quirks(struct pci_dev *pdev)
> > > > > {
> > > > > + if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > > > + pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > > > + struct gpio_desc *gpio;
> > > > > +
> > > > > + gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > > > > + if (!IS_ERR(gpio)) {
> > > > > + gpiod_direction_output(gpio, 0);
> > > > > + gpiod_set_value_cansleep(gpio, 1);
> > > > > + gpiod_put(gpio);
> > > > > + }
> > > > > + gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > > > > + if (!IS_ERR(gpio)) {
> > > > > + gpiod_direction_output(gpio, 0);
> > > > > + gpiod_set_value_cansleep(gpio, 1);
> > > > > + gpiod_put(gpio);
> > > > > + }
> > > > > + }
> > > >
> > > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > > > very good.
> > >
> > > ..but unfortunately we can't use the bus without it :(. We depend on
> > > being able to read the vendor and product id's in the bus driver.
> >
> > Doesn't the ugly platform device case look less ugly than this?
>
> The platform device would in any case need to be created only for this
> case. We can't any more just create the phy device unconditionally for
> every PCI platform like it was created before, as it's clear now the
> PHY device can be be created from other sources. It was a risk always.
>
> But the big problem is that since the PHY on your board is ULPI PHY,
> it will make it really difficult to add the ULPI bus support. And like
> I said in my previous mail, we would really need it for the boards
> that expect the PHY driver to provide functions like charger
> detection. We don't need it only for BYT based boards.
>
> So on top of the above, since the GPIO resources are given to dwc3, I
> actually think that my hack is better then the platform device.
This would bring support to current BYT-CR products, but it's a lot ugly :/
But current DSDT is a lot ugly too.
Felipe, we do need ULPI bus working for BYT-CR due to 2 scenarios:
- we need eye optimization
- we need to pull down D+/- on phy after cable disconnection so charging
detection module can detect CDP connection during next cable plug in.
I see no way to make current BYT-CR platforms work with Heikki's ULPI
bus proposal without crippling it if we don't toggle these GPIOs outside
ULPI's device probe.
>From my side, the quirk is acceptable as a trade off to not damage his
design due to legacy platforms.
Br, David
>
>
> Thanks,
>
> --
> heikki
On Tue, Feb 10, 2015 at 11:05:31AM -0800, David Cohen wrote:
> On Mon, Feb 02, 2015 at 02:59:59PM +0200, Heikki Krogerus wrote:
> > > > > > You can't really compare a bus like i2c, which can't enumerate devices
> > > > > > natively, to ULPI which can.
> > > > >
> > > > > why not ? The BIOS might not need to use the PHY (or USB) at all, it can
> > > > > very well decide to never turn it on, right ?
> > > >
> > > > If ULPI was seen as a bus, then no. BIOS would have definitely left
> > > > the PHY on. In fact, if we would have just asked the BIOS writers to
> > > > leave it on, they would not have any problem with that, even without
> > > > the bus.
> > >
> > > That's a really wrong assumption. ULPI bus depends on dwc3 to be
> > > functional and dwc3 depends on phy to be functional to complete its
> > > power on sequence. We can't ask BIOS to let both up and running all the
> > > time.
> > >
> > > FWIW we *cannot* rely on ULPI bus enumeration to probe ULPI devices,
> > > because it requires the ULPI device to be previously functional which
> > > can't happen before the enumeration. Even if we ask BIOS to let phy
> > > functional after boot, what happens when we remove modules and load it
> > > again? Should we ask BIOS to power on the components again in order to
> > > probe and power it on? It's a circular dependency you're creating.
> > >
> > > >
> > > > > > I don't think the other boards we have which use TUSB1210, like the
> > > > > > BYT-I ones and I think some Merrifield based boards, expect any less
> > > > > > from PM efficiency then BYT-CR, but we don't need to do any tricks
> > > > > > with them in order to use ULPI bus. That is what I mean when I say
> > > > > > this is BYT-CR specific problem.
> > > > >
> > > > > perhaps because firmware on those other boards are powering up the PHY ?
> > > >
> > > > Yes.
> > >
> > > And that's wrong assumption. Not all fw will power on PHY. Maybe we
> > > should stop all of other discussions and concentrate on this one:
> > > BIOS will not guarantee phy will be functional after boot. And if we
> > > remove modules and load again, it won't be functional regardless what
> > > BIOS did during boot.
> >
> > I was wrong when I talked about BIOS powering the PHY before bootup. I
> > admit it. I'm saying this again as a reminder to myself: We can't mix
> > BIOS (or any FW) and ACPI. I mix them constantly. And I definitely
> > need to stop talking about bootup.
> >
> > How this is handled is by letting the ACPI Power State methods of the
> > dwc3 device take care of the toggling of the gpios. It is done with
> > the help of something called GPIO OperationRegion. In case you are not
> > familiar with ACPI, then if you send me ACPI dump of one of those
> > devices (or just copy /sys/firmware/acpi/tables/DSDT), I can try to
> > modify the DSDT for you so you can use to test it, and provide you
> > with the ASL snippet.
> >
> > You will see that the PHY is indeed in reset after bootup like it is
> > now (BIOS does nothing differently), but the gpios are automatically
> > toggled by the DSDT code. So every time you load dwc3 module, the PHY
> > will be operational when we need it, and when you unload dwc3, it will
> > be left in reset again. The OS does not have to do anything.
>
> Hm. That changes everything :)
> It's a feasible direction to push vendors.
>
> >
> > I really think that this BYT-CR case will be one of really rare
> > exception we will see, especially if we manage to put together the
> > ACPI table review that has been though about.
>
> I hope it will.
>
> >
> > > > > > I don't agree with PM arguments if it means that we should be ready to
> > > > > > accept loosing possibility for a generic solution in OS with a single
> > > > > > device like our PHY. I seriously doubt it would prevent the products
> > > > > > using these boards of achieving their PM requirements. But this
> > > > > > conversation is outside our topic.
> > > > >
> > > > > we're not loosing anything. We're just considering what's the best way
> > > > > to tackle that ulpi_read() inside probe(). TUSB1210 driver _has_ to cope
> > > > > with situations where reset_gpio/cs_gpio are in unexpected state. Saying
> > > > > we will just "fix the firmware", as if that was a simple feat, is
> > > > > counter-productive.
> > > >
> > > > You know guys, we shouldn't always just lay down and say, "we just
> > > > have to accept it can be anything" or "we just have to try to prepare
> > > > for everything". We can influence these things, and we should. We can
> > > > influence these things inside our own companies before any products is
> > > > launched using our SoCs, and since more and more companies are
> > > > releasing their code into the public before their product are
> > > > launched, we even have a change to influence others. Lack of standards
> > > > does not mean we should not try to achieve consistency.
> > > >
> > > > For example, now I should probable write to Documentation that "ULPI
> > > > PHY needs to be in condition where it's register can be accessed
> > > > before the interface is registered.", and I'm pretty sure it would be
> > > > enough to have an effect on many of the new platforms that use ULPI
> > > > PHYs.
> > >
> > > In order for phy to be functional, it does not depend only on toggling
> > > GPIOs. It depends on DWC3 going to reset state, then phy executes power
> > > on sequence, then DWC3 going out of reset state to sync clocks with phy.
> > > You're saying we should tell BIOS is concurrently mess with dwc3
> > > together with dwc3 driver?
> >
> > I don't understand what you are saying here?
>
> TUSB1210 needs to come out of reset only when DWC3 is in reset state.
> This is how current code works in dwc3_core_soft_reset():
> - dwc3 goes to reset
> - phy goes to reset
> - phy gets out of reset
> - dwc3 gets out of reset
>
> This is how you're proposing:
> - phy goes to reset (DSDT code, when loading module)
> - phy gets out of reset (DSDT code, when loading module)
>
> - dwc3 goes to reset (dwc3_core_soft_reset())
> - dwc3 gets our of reset (dwc3_core_soft_reset())
>
> Felipe, do you see a problem with this new context? If not, I'm
> satisfied with Heikki's ULPI bus proposal considering my comment below.
Sorry, guess I spoke too soon :/
I am satisfied with the phy case, but I forgot about the chicken/egg
problem I reported earlier:
DWC3 will not be functional when reloading the module after it went to
reset state. Then ULPI enumeration can't happen regardless DSDT code
powered on phy.
Heikki, do you have a proposal for that? IMHO that's the main missing
point if we forget about BYT-CR legacy.
Br, David
>
> >
> > > > > > Because of the need to write to the ULPI registers, I don't think we
> > > > > > should try anything else except to use ULPI bus straight away. We'll
> > > > >
> > > > > I'll agree with this.
> > > > >
> > > > > > start by making use of ULPI bus possible by adding the quirk for BYT
> > > > > > (attached), which to me is perfectly OK solution. I would appreciate
> > > > > > if you gave it a review.
> > > > >
> > > > > it's not perfectly ok for dwc3 to toggle PHY's GPIOs. Have the PHY
> > > > > driver to that.
> > > >
> > > > Oh, I agree with that..
> > > >
> > > > > > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> > > > > > index 8d95056..53902ea 100644
> > > > > > --- a/drivers/usb/dwc3/dwc3-pci.c
> > > > > > +++ b/drivers/usb/dwc3/dwc3-pci.c
> > > > > > @@ -21,6 +21,7 @@
> > > > > > #include <linux/slab.h>
> > > > > > #include <linux/pci.h>
> > > > > > #include <linux/platform_device.h>
> > > > > > +#include <linux/gpio/consumer.h>
> > > > > >
> > > > > > #include "platform_data.h"
> > > > > >
> > > > > > @@ -35,6 +36,24 @@
> > > > > >
> > > > > > static int dwc3_pci_quirks(struct pci_dev *pdev)
> > > > > > {
> > > > > > + if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > > > > + pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > > > > + struct gpio_desc *gpio;
> > > > > > +
> > > > > > + gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > > > > > + if (!IS_ERR(gpio)) {
> > > > > > + gpiod_direction_output(gpio, 0);
> > > > > > + gpiod_set_value_cansleep(gpio, 1);
> > > > > > + gpiod_put(gpio);
> > > > > > + }
> > > > > > + gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > > > > > + if (!IS_ERR(gpio)) {
> > > > > > + gpiod_direction_output(gpio, 0);
> > > > > > + gpiod_set_value_cansleep(gpio, 1);
> > > > > > + gpiod_put(gpio);
> > > > > > + }
> > > > > > + }
> > > > >
> > > > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > > > > very good.
> > > >
> > > > ..but unfortunately we can't use the bus without it :(. We depend on
> > > > being able to read the vendor and product id's in the bus driver.
> > >
> > > Doesn't the ugly platform device case look less ugly than this?
> >
> > The platform device would in any case need to be created only for this
> > case. We can't any more just create the phy device unconditionally for
> > every PCI platform like it was created before, as it's clear now the
> > PHY device can be be created from other sources. It was a risk always.
> >
> > But the big problem is that since the PHY on your board is ULPI PHY,
> > it will make it really difficult to add the ULPI bus support. And like
> > I said in my previous mail, we would really need it for the boards
> > that expect the PHY driver to provide functions like charger
> > detection. We don't need it only for BYT based boards.
> >
> > So on top of the above, since the GPIO resources are given to dwc3, I
> > actually think that my hack is better then the platform device.
>
> This would bring support to current BYT-CR products, but it's a lot ugly :/
> But current DSDT is a lot ugly too.
>
> Felipe, we do need ULPI bus working for BYT-CR due to 2 scenarios:
> - we need eye optimization
> - we need to pull down D+/- on phy after cable disconnection so charging
> detection module can detect CDP connection during next cable plug in.
>
> I see no way to make current BYT-CR platforms work with Heikki's ULPI
> bus proposal without crippling it if we don't toggle these GPIOs outside
> ULPI's device probe.
>
> From my side, the quirk is acceptable as a trade off to not damage his
> design due to legacy platforms.
>
> Br, David
>
> >
> >
> > Thanks,
> >
> > --
> > heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi David,
> > > > In order for phy to be functional, it does not depend only on toggling
> > > > GPIOs. It depends on DWC3 going to reset state, then phy executes power
> > > > on sequence, then DWC3 going out of reset state to sync clocks with phy.
> > > > You're saying we should tell BIOS is concurrently mess with dwc3
> > > > together with dwc3 driver?
> > >
> > > I don't understand what you are saying here?
> >
> > TUSB1210 needs to come out of reset only when DWC3 is in reset state.
> > This is how current code works in dwc3_core_soft_reset():
> > - dwc3 goes to reset
> > - phy goes to reset
> > - phy gets out of reset
> > - dwc3 gets out of reset
> >
> > This is how you're proposing:
> > - phy goes to reset (DSDT code, when loading module)
> > - phy gets out of reset (DSDT code, when loading module)
> >
> > - dwc3 goes to reset (dwc3_core_soft_reset())
> > - dwc3 gets our of reset (dwc3_core_soft_reset())
> >
> > Felipe, do you see a problem with this new context? If not, I'm
> > satisfied with Heikki's ULPI bus proposal considering my comment below.
>
> Sorry, guess I spoke too soon :/
> I am satisfied with the phy case, but I forgot about the chicken/egg
> problem I reported earlier:
> DWC3 will not be functional when reloading the module after it went to
> reset state. Then ULPI enumeration can't happen regardless DSDT code
> powered on phy.
One point here. If we have DSDT handling the gpios with the operation
region, those gpio resources don't need to be given to any device
(actually I think they really shouldn't be given to anything in that
case).
> Heikki, do you have a proposal for that? IMHO that's the main missing
> point if we forget about BYT-CR legacy.
I'm sorry but I'm still not sure about the scenario you are talking
about.
When we load dwc3, we end up autoloading phy-tusb1210 in this case and
increasing the phy devices ref count i.e. preventing phy-tusb1210
module from being unloaded before dwc3 is unloaded.
If we unload dwc3 we can also unload phy-tusb1210 if we like but if
after that we load dwc3 again, the ULPI will be accessible the moment
we register the ulpi interface as it was before.
That I believe is actually a must in case of ULPI. When dwc3 is reset
with GCTL or DCTL SoftReset, it will first write to the ULPI
FunctionControl register's reset bit in order to but the PHY to reset
(PHYSoftRst has no effect in case of ULPI), so ULPI really has to be
accessible before the core is soft reset.
Thanks,
--
heikki
On Wed, Feb 11, 2015 at 03:12:55PM +0200, Heikki Krogerus wrote:
> Hi David,
Hi Heikki,
>
> > > > > In order for phy to be functional, it does not depend only on toggling
> > > > > GPIOs. It depends on DWC3 going to reset state, then phy executes power
> > > > > on sequence, then DWC3 going out of reset state to sync clocks with phy.
> > > > > You're saying we should tell BIOS is concurrently mess with dwc3
> > > > > together with dwc3 driver?
> > > >
> > > > I don't understand what you are saying here?
> > >
> > > TUSB1210 needs to come out of reset only when DWC3 is in reset state.
> > > This is how current code works in dwc3_core_soft_reset():
> > > - dwc3 goes to reset
> > > - phy goes to reset
> > > - phy gets out of reset
> > > - dwc3 gets out of reset
> > >
> > > This is how you're proposing:
> > > - phy goes to reset (DSDT code, when loading module)
> > > - phy gets out of reset (DSDT code, when loading module)
> > >
> > > - dwc3 goes to reset (dwc3_core_soft_reset())
> > > - dwc3 gets our of reset (dwc3_core_soft_reset())
> > >
> > > Felipe, do you see a problem with this new context? If not, I'm
> > > satisfied with Heikki's ULPI bus proposal considering my comment below.
> >
> > Sorry, guess I spoke too soon :/
> > I am satisfied with the phy case, but I forgot about the chicken/egg
> > problem I reported earlier:
> > DWC3 will not be functional when reloading the module after it went to
> > reset state. Then ULPI enumeration can't happen regardless DSDT code
> > powered on phy.
>
> One point here. If we have DSDT handling the gpios with the operation
> region, those gpio resources don't need to be given to any device
> (actually I think they really shouldn't be given to anything in that
> case).
Agree with that.
>
> > Heikki, do you have a proposal for that? IMHO that's the main missing
> > point if we forget about BYT-CR legacy.
>
> I'm sorry but I'm still not sure about the scenario you are talking
> about.
Probably because I'm asking this question in the wrong place.
It is regarding patch 6/8. I resent the question there.
Br, David