| Message ID | 1362049024-11383-4-git-send-email-mkl@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, Feb 28, 2013 at 11:57:02AM +0100, Marc Kleine-Budde wrote: > @@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device *pdev) > return -ENODEV; > } > > + /* For now we treat dual-role as otg */ > + dr_mode = ci->platdata->dr_mode; > + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE) > + dr_mode = USB_DR_MODE_OTG; > + > /* initialize role(s) before the interrupt is requested */ > - ret = ci_hdrc_host_init(ci); > - if (ret) > - dev_info(dev, "doesn't support host\n"); > + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { this is not something you should be passing via pdata; chipidea core should know how to read this data by itself. Meaning that chipidea core should be taught about devicetree. But make it optional since now all users use DT.
On 8 March 2013 18:33, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Thu, Feb 28, 2013 at 11:57:02AM +0100, Marc Kleine-Budde wrote: >> @@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device *pdev) >> return -ENODEV; >> } >> >> + /* For now we treat dual-role as otg */ >> + dr_mode = ci->platdata->dr_mode; >> + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE) >> + dr_mode = USB_DR_MODE_OTG; >> + >> /* initialize role(s) before the interrupt is requested */ >> - ret = ci_hdrc_host_init(ci); >> - if (ret) >> - dev_info(dev, "doesn't support host\n"); >> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { > > this is not something you should be passing via pdata; chipidea core > should know how to read this data by itself. Meaning that chipidea core > should be taught about devicetree. But make it optional since now all > users use DT. And I don't think I like the idea of chipidea core calling into device tree code directly. Regards, -- Alex
On 03/08/2013 05:46 PM, Alexander Shishkin wrote: > On 8 March 2013 18:33, Felipe Balbi <balbi@ti.com> wrote: >> Hi, >> >> On Thu, Feb 28, 2013 at 11:57:02AM +0100, Marc Kleine-Budde wrote: >>> @@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device *pdev) >>> return -ENODEV; >>> } >>> >>> + /* For now we treat dual-role as otg */ >>> + dr_mode = ci->platdata->dr_mode; >>> + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE) >>> + dr_mode = USB_DR_MODE_OTG; >>> + >>> /* initialize role(s) before the interrupt is requested */ >>> - ret = ci_hdrc_host_init(ci); >>> - if (ret) >>> - dev_info(dev, "doesn't support host\n"); >>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { >> >> this is not something you should be passing via pdata; chipidea core >> should know how to read this data by itself. Meaning that chipidea core >> should be taught about devicetree. But make it optional since now all >> users use DT. > > And I don't think I like the idea of chipidea core calling into device > tree code directly. Hmmm....this means draw :) Marc
On 8 March 2013 18:52, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > On 03/08/2013 05:46 PM, Alexander Shishkin wrote: >> On 8 March 2013 18:33, Felipe Balbi <balbi@ti.com> wrote: >>> Hi, >>> >>> On Thu, Feb 28, 2013 at 11:57:02AM +0100, Marc Kleine-Budde wrote: >>>> @@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device *pdev) >>>> return -ENODEV; >>>> } >>>> >>>> + /* For now we treat dual-role as otg */ Btw, if we do this, Peter's otg code will try to access OTGSC, which is not what we want on non-otg devices, so we'll need a clear distinction between the two. >>>> + dr_mode = ci->platdata->dr_mode; >>>> + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE) >>>> + dr_mode = USB_DR_MODE_OTG; >>>> + >>>> /* initialize role(s) before the interrupt is requested */ >>>> - ret = ci_hdrc_host_init(ci); >>>> - if (ret) >>>> - dev_info(dev, "doesn't support host\n"); >>>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { >>> >>> this is not something you should be passing via pdata; chipidea core >>> should know how to read this data by itself. Meaning that chipidea core >>> should be taught about devicetree. But make it optional since now all >>> users use DT. >> >> And I don't think I like the idea of chipidea core calling into device >> tree code directly. > > Hmmm....this means draw :) Well, we could go for something like ci_hdrc-$(CONFIG_OF) += of.o and try to contain the damage there, maybe? Ideas? I would very much like to keep the clutter away from the core probe if possible.
Hi, On Fri, Mar 08, 2013 at 10:55:46PM +0200, Alexander Shishkin wrote: > >>>> + dr_mode = ci->platdata->dr_mode; > >>>> + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE) > >>>> + dr_mode = USB_DR_MODE_OTG; > >>>> + > >>>> /* initialize role(s) before the interrupt is requested */ > >>>> - ret = ci_hdrc_host_init(ci); > >>>> - if (ret) > >>>> - dev_info(dev, "doesn't support host\n"); > >>>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { > >>> > >>> this is not something you should be passing via pdata; chipidea core > >>> should know how to read this data by itself. Meaning that chipidea core > >>> should be taught about devicetree. But make it optional since now all > >>> users use DT. > >> > >> And I don't think I like the idea of chipidea core calling into device > >> tree code directly. > > > > Hmmm....this means draw :) > > Well, we could go for something like > > ci_hdrc-$(CONFIG_OF) += of.o > > and try to contain the damage there, maybe? Ideas? I would very much > like to keep the clutter away from the core probe if possible. damage, what damage ? DeviceTree is quite real and drivers need to cope with it. If not all platforms support devicetree, make it optional. It's easy enough to make the choice based on device.of_node being valid or not. At the end of the day, it's the chipidea IP which needs dr_mode, not the glue. Passing the responsability of decoding dr_mode to the glue is moronic. It's just like asking the glue to control chipidea's clocks.
Felipe Balbi <balbi@ti.com> writes: > Hi, > > On Fri, Mar 08, 2013 at 10:55:46PM +0200, Alexander Shishkin wrote: >> >>>> + dr_mode = ci->platdata->dr_mode; >> >>>> + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE) >> >>>> + dr_mode = USB_DR_MODE_OTG; >> >>>> + >> >>>> /* initialize role(s) before the interrupt is requested */ >> >>>> - ret = ci_hdrc_host_init(ci); >> >>>> - if (ret) >> >>>> - dev_info(dev, "doesn't support host\n"); >> >>>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { >> >>> >> >>> this is not something you should be passing via pdata; chipidea core >> >>> should know how to read this data by itself. Meaning that chipidea core >> >>> should be taught about devicetree. But make it optional since now all >> >>> users use DT. >> >> >> >> And I don't think I like the idea of chipidea core calling into device >> >> tree code directly. >> > >> > Hmmm....this means draw :) >> >> Well, we could go for something like >> >> ci_hdrc-$(CONFIG_OF) += of.o >> >> and try to contain the damage there, maybe? Ideas? I would very much >> like to keep the clutter away from the core probe if possible. > > damage, what damage ? DeviceTree is quite real and drivers need to cope > with it. If not all platforms support devicetree, make it optional. It's > easy enough to make the choice based on device.of_node being valid or > not. We have dr_mode and phy_mode (so far). The latter is simple, but the former one needs to see some special cases, based on its setting. Now, if we're a pci device, for example, we don't have phandles and stuff and we will still get this information via platform data. So, what we'll end up with is some glue drivers (that don't have device tree) passing all sorts of stuff via platform data and others just expecting the chipidea to take care of it. That's inconsistent at best. > At the end of the day, it's the chipidea IP which needs dr_mode, not the > glue. Passing the responsability of decoding dr_mode to the glue is > moronic. It's just like asking the glue to control chipidea's clocks. Now, now. There's something to be said about stuffing core drivers with support for all sorts of resource management protocols du jour, but we'll leave that for another day. As for the clocks, if they are external to chipidea controller, the latter has no business messing with them. It's like asking chipidea to do power management on your SoC for you. :) Regards, -- Alex
Hi, On Thu, Mar 28, 2013 at 01:13:00PM +0200, Alexander Shishkin wrote: > >> >>>> + dr_mode = ci->platdata->dr_mode; > >> >>>> + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE) > >> >>>> + dr_mode = USB_DR_MODE_OTG; > >> >>>> + > >> >>>> /* initialize role(s) before the interrupt is requested */ > >> >>>> - ret = ci_hdrc_host_init(ci); > >> >>>> - if (ret) > >> >>>> - dev_info(dev, "doesn't support host\n"); > >> >>>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { > >> >>> > >> >>> this is not something you should be passing via pdata; chipidea core > >> >>> should know how to read this data by itself. Meaning that chipidea core > >> >>> should be taught about devicetree. But make it optional since now all > >> >>> users use DT. > >> >> > >> >> And I don't think I like the idea of chipidea core calling into device > >> >> tree code directly. > >> > > >> > Hmmm....this means draw :) > >> > >> Well, we could go for something like > >> > >> ci_hdrc-$(CONFIG_OF) += of.o > >> > >> and try to contain the damage there, maybe? Ideas? I would very much > >> like to keep the clutter away from the core probe if possible. > > > > damage, what damage ? DeviceTree is quite real and drivers need to cope > > with it. If not all platforms support devicetree, make it optional. It's > > easy enough to make the choice based on device.of_node being valid or > > not. > > We have dr_mode and phy_mode (so far). The latter is simple, but the > former one needs to see some special cases, based on its setting. Now, > if we're a pci device, for example, we don't have phandles and stuff and > we will still get this information via platform data. fair enough: if (pdev->dev.of_node) chipidea_init_from_dt(ci); else chipidea_init_from_pdata(ci); > So, what we'll end up with is some glue drivers (that don't have device > tree) passing all sorts of stuff via platform data and others just > expecting the chipidea to take care of it. That's inconsistent at best. it's not inconsistent at all. Some drivers pass data through DT and some pass data through pdata. Regardless of which driver type you have, chipidea core still needs to fetch the data, either by of_property_*() calls or by reading pdata->$field. I wouldn't call it inconsistency, it's just coping with both ways of receiving data. > > At the end of the day, it's the chipidea IP which needs dr_mode, not the > > glue. Passing the responsability of decoding dr_mode to the glue is > > moronic. It's just like asking the glue to control chipidea's clocks. > > Now, now. There's something to be said about stuffing core drivers with > support for all sorts of resource management protocols du jour, but > we'll leave that for another day. > > As for the clocks, if they are external to chipidea controller, the > latter has no business messing with them. It's like asking chipidea to heh, that's not what I said... > do power management on your SoC for you. :) right, asking other layers to do your work is stupid, that's exactly what I said. Shuving DT knowledge in glue layer just so chipidea core only understands pdata is stupid. You end up allocating memory twice to hold the same data (once for DT and once for the pdata copies of it).
Felipe Balbi <balbi@ti.com> writes: > Hi, Hi, > On Thu, Mar 28, 2013 at 01:13:00PM +0200, Alexander Shishkin wrote: >> >> >>>> + dr_mode = ci->platdata->dr_mode; >> >> >>>> + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE) >> >> >>>> + dr_mode = USB_DR_MODE_OTG; >> >> >>>> + >> >> >>>> /* initialize role(s) before the interrupt is requested */ >> >> >>>> - ret = ci_hdrc_host_init(ci); >> >> >>>> - if (ret) >> >> >>>> - dev_info(dev, "doesn't support host\n"); >> >> >>>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { >> >> >>> >> >> >>> this is not something you should be passing via pdata; chipidea core >> >> >>> should know how to read this data by itself. Meaning that chipidea core >> >> >>> should be taught about devicetree. But make it optional since now all >> >> >>> users use DT. >> >> >> >> >> >> And I don't think I like the idea of chipidea core calling into device >> >> >> tree code directly. >> >> > >> >> > Hmmm....this means draw :) >> >> >> >> Well, we could go for something like >> >> >> >> ci_hdrc-$(CONFIG_OF) += of.o >> >> >> >> and try to contain the damage there, maybe? Ideas? I would very much >> >> like to keep the clutter away from the core probe if possible. >> > >> > damage, what damage ? DeviceTree is quite real and drivers need to cope >> > with it. If not all platforms support devicetree, make it optional. It's >> > easy enough to make the choice based on device.of_node being valid or >> > not. >> >> We have dr_mode and phy_mode (so far). The latter is simple, but the >> former one needs to see some special cases, based on its setting. Now, >> if we're a pci device, for example, we don't have phandles and stuff and >> we will still get this information via platform data. > > fair enough: > > if (pdev->dev.of_node) > chipidea_init_from_dt(ci); > else > chipidea_init_from_pdata(ci); You mean, you want to have two instances of the similar logic? Don't forget that they might fail to fetch certain phandles and still continue, but failing to fetch other phandles will be fatal for probe(). The above snipped can also be shortened to chipidea_just_do_the_right_thing(ci); /* I'd like that, btw */ The devil is in the details. Then, I hate to bring it up, but what do you do for acpi devices? PnP devices? PCMCIA devices? Right now, the core is a platform driver. It gets all the information from platform data. That's all it needs for its purpose, and all the platform specific details are abstracted away. It's the purpose of the glue layer's existance to fetch all the relevant bits from the glue driver knows where and supply it in a *consistent* manner to the core. Note, it's totally different for regulators or clocks or phys. It is totally unacceptable to pass objects around between glue and core and glue shouldn't have to deal with those. And, of course, you can request all those in the core code in a platform-agnostic manner. >> So, what we'll end up with is some glue drivers (that don't have device >> tree) passing all sorts of stuff via platform data and others just >> expecting the chipidea to take care of it. That's inconsistent at best. > > it's not inconsistent at all. > > Some drivers pass data through DT and some pass data through pdata. > > Regardless of which driver type you have, chipidea core still needs to > fetch the data, either by of_property_*() calls or by reading > pdata->$field. > > I wouldn't call it inconsistency, it's just coping with both ways of > receiving data. > >> > At the end of the day, it's the chipidea IP which needs dr_mode, not the >> > glue. Passing the responsability of decoding dr_mode to the glue is >> > moronic. It's just like asking the glue to control chipidea's clocks. >> >> Now, now. There's something to be said about stuffing core drivers with >> support for all sorts of resource management protocols du jour, but >> we'll leave that for another day. >> >> As for the clocks, if they are external to chipidea controller, the >> latter has no business messing with them. It's like asking chipidea to > > heh, that's not what I said... > >> do power management on your SoC for you. :) > > right, asking other layers to do your work is stupid, that's exactly > what I said. Shuving DT knowledge in glue layer just so chipidea core > only understands pdata is stupid. Why? Especially if the glue drivers have to fetch stuff for their own needs from DT anyway. Might easily happen. > You end up allocating memory twice to > hold the same data (once for DT and once for the pdata copies of it). It would have been one valid reason for teaching chipidea core about DT, if we could duplicate *all* of the pdata fields in DT. Otherwise we still need pdata. And supposing that we can (which we can't) do that, and supposing that extra 32 bytes of memory actually matter, it still doesn't justify the extra code in the core to deal with DT. I'm still not convinced. Regards, -- Alex
Hi, On Thu, Mar 28, 2013 at 03:18:14PM +0200, Alexander Shishkin wrote: > >> >> >>>> + dr_mode = ci->platdata->dr_mode; > >> >> >>>> + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE) > >> >> >>>> + dr_mode = USB_DR_MODE_OTG; > >> >> >>>> + > >> >> >>>> /* initialize role(s) before the interrupt is requested */ > >> >> >>>> - ret = ci_hdrc_host_init(ci); > >> >> >>>> - if (ret) > >> >> >>>> - dev_info(dev, "doesn't support host\n"); > >> >> >>>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { > >> >> >>> > >> >> >>> this is not something you should be passing via pdata; chipidea core > >> >> >>> should know how to read this data by itself. Meaning that chipidea core > >> >> >>> should be taught about devicetree. But make it optional since now all > >> >> >>> users use DT. > >> >> >> > >> >> >> And I don't think I like the idea of chipidea core calling into device > >> >> >> tree code directly. > >> >> > > >> >> > Hmmm....this means draw :) > >> >> > >> >> Well, we could go for something like > >> >> > >> >> ci_hdrc-$(CONFIG_OF) += of.o > >> >> > >> >> and try to contain the damage there, maybe? Ideas? I would very much > >> >> like to keep the clutter away from the core probe if possible. > >> > > >> > damage, what damage ? DeviceTree is quite real and drivers need to cope > >> > with it. If not all platforms support devicetree, make it optional. It's > >> > easy enough to make the choice based on device.of_node being valid or > >> > not. > >> > >> We have dr_mode and phy_mode (so far). The latter is simple, but the > >> former one needs to see some special cases, based on its setting. Now, > >> if we're a pci device, for example, we don't have phandles and stuff and > >> we will still get this information via platform data. > > > > fair enough: > > > > if (pdev->dev.of_node) > > chipidea_init_from_dt(ci); > > else > > chipidea_init_from_pdata(ci); > > You mean, you want to have two instances of the similar logic? Don't > forget that they might fail to fetch certain phandles and still > continue, but failing to fetch other phandles will be fatal for > probe(). The above snipped can also be shortened to > > chipidea_just_do_the_right_thing(ci); /* I'd like that, btw */ > > The devil is in the details. > > Then, I hate to bring it up, but what do you do for acpi devices? PnP > devices? PCMCIA devices? those look like PCI devices right ? What's the problem with them ? > Right now, the core is a platform driver. It gets all the information > from platform data. That's all it needs for its purpose, and all the > platform specific details are abstracted away. It's the purpose of the > glue layer's existance to fetch all the relevant bits from the glue > driver knows where and supply it in a *consistent* manner to the core. I still that e.g. requesting regulators in glue and passing a regulator pointer through platform_data is really, really wrong. > Note, it's totally different for regulators or clocks or phys. It is > totally unacceptable to pass objects around between glue and core and > glue shouldn't have to deal with those. And, of course, you can request > all those in the core code in a platform-agnostic manner. how ? If your regulator is bound to the glue, how will you regulator_get() from core driver ? > >> So, what we'll end up with is some glue drivers (that don't have device > >> tree) passing all sorts of stuff via platform data and others just > >> expecting the chipidea to take care of it. That's inconsistent at best. > > > > it's not inconsistent at all. > > > > Some drivers pass data through DT and some pass data through pdata. > > > > Regardless of which driver type you have, chipidea core still needs to > > fetch the data, either by of_property_*() calls or by reading > > pdata->$field. > > > > I wouldn't call it inconsistency, it's just coping with both ways of > > receiving data. > > > >> > At the end of the day, it's the chipidea IP which needs dr_mode, not the > >> > glue. Passing the responsability of decoding dr_mode to the glue is > >> > moronic. It's just like asking the glue to control chipidea's clocks. > >> > >> Now, now. There's something to be said about stuffing core drivers with > >> support for all sorts of resource management protocols du jour, but > >> we'll leave that for another day. > >> > >> As for the clocks, if they are external to chipidea controller, the > >> latter has no business messing with them. It's like asking chipidea to > > > > heh, that's not what I said... > > > >> do power management on your SoC for you. :) > > > > right, asking other layers to do your work is stupid, that's exactly > > what I said. Shuving DT knowledge in glue layer just so chipidea core > > only understands pdata is stupid. > > Why? Especially if the glue drivers have to fetch stuff for their own > needs from DT anyway. Might easily happen. why ? Because it's poor encapsulation. Why would you give another entity knowledge about yourself ? > > You end up allocating memory twice to > > hold the same data (once for DT and once for the pdata copies of it). > > It would have been one valid reason for teaching chipidea core about DT, > if we could duplicate *all* of the pdata fields in DT. Otherwise we > still need pdata. And supposing that we can (which we can't) do that, > and supposing that extra 32 bytes of memory actually matter, it still > doesn't justify the extra code in the core to deal with DT. I'm still > not convinced. fair enough, it's your headache in the end of the day anyway. When you get bug reports such as one we saw recently of clocks being left on even though probe failed, you'll understand. You have worked with MUSB before and should already know that giving too much knowledge to your glue layers is a recipe for disaster.
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 04d68cb..ec27060 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -435,6 +435,7 @@ static int ci_hdrc_probe(struct platform_device *pdev) struct resource *res; void __iomem *base; int ret; + enum usb_dr_mode dr_mode; if (!dev->platform_data) { dev_err(dev, "platform data missing\n"); @@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device *pdev) return -ENODEV; } + /* For now we treat dual-role as otg */ + dr_mode = ci->platdata->dr_mode; + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE) + dr_mode = USB_DR_MODE_OTG; + /* initialize role(s) before the interrupt is requested */ - ret = ci_hdrc_host_init(ci); - if (ret) - dev_info(dev, "doesn't support host\n"); + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) { + ret = ci_hdrc_host_init(ci); + if (ret) + dev_info(dev, "doesn't support host\n"); + } - ret = ci_hdrc_gadget_init(ci); - if (ret) - dev_info(dev, "doesn't support gadget\n"); + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) { + ret = ci_hdrc_gadget_init(ci); + if (ret) + dev_info(dev, "doesn't support gadget\n"); + } if (!ci->roles[CI_ROLE_HOST] && !ci->roles[CI_ROLE_GADGET]) { dev_err(dev, "no supported roles\n"); diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h index 1a2aa18..b314647 100644 --- a/include/linux/usb/chipidea.h +++ b/include/linux/usb/chipidea.h @@ -20,7 +20,7 @@ struct ci13xxx_platform_data { #define CI13XXX_REQUIRE_TRANSCEIVER BIT(1) #define CI13XXX_PULLUP_ON_VBUS BIT(2) #define CI13XXX_DISABLE_STREAMING BIT(3) - + enum usb_dr_mode dr_mode; #define CI13XXX_CONTROLLER_RESET_EVENT 0 #define CI13XXX_CONTROLLER_STOPPED_EVENT 1 void (*notify_event) (struct ci13xxx *ci, unsigned event);