| Message ID | 1395257399-359545-7-git-send-email-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 19 March 2014 23:53:18 Sergei Shtylyov wrote: > On 03/19/2014 10:29 PM, Arnd Bergmann wrote: > > > The ohci-da8xx driver uses the DA8XX_SYSCFG0_VIRT macro to > > access the CFGCHIP2 register for controlling its PHY. > > > The macro in turn relies on the da8xx_syscfg0_base global > > variable. Since the OHCI driver can be a loadable module, > > this requires the symbol to be exported from platform code. > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Cc: Sekhar Nori <nsekhar@ti.com> > > Cc: Kevin Hilman <khilman@deeprootsystems.com> > > Cc: davinci-linux-open-source@linux.davincidsp.com > > --- > > arch/arm/mach-davinci/devices-da8xx.c | 1 + > > 1 file changed, 1 insertion(+) > > > diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c > > index 0486cdf..4da868a 100644 > > --- a/arch/arm/mach-davinci/devices-da8xx.c > > +++ b/arch/arm/mach-davinci/devices-da8xx.c > > @@ -66,6 +66,7 @@ > > #define DA850_DMA_MMCSD1_TX EDMA_CTLR_CHAN(1, 29) > > > > void __iomem *da8xx_syscfg0_base; > > +EXPORT_SYMBOL_GPL(da8xx_syscfg0_base); /* used by OHCI_HCD */ > > I have submitted such patch years ago and it was turned down. > The question is whether there is anyone who would do this properly. Both the OHCI and MUSB drivers use exactly one register (CFGCHIP2) to control the clock, phy and host/gadget mode switch. In the modern world, we'd probably want to have a clock driver and a phy driver for these, based on a syscon driver. In all honesty I don't see that happening on davinci. A somewhat better approach would be to export a set of exported functions to access the one register from the platform, e.g. u32 da8xx_cfgchip2_get(void); void da8xx_cfgchip2_set(u32); That interface would still be a bit ugly, but much better than what we have today, and easy to implement. Arnd
On 03/19/2014 10:29 PM, Arnd Bergmann wrote: > The ohci-da8xx driver uses the DA8XX_SYSCFG0_VIRT macro to > access the CFGCHIP2 register for controlling its PHY. > The macro in turn relies on the da8xx_syscfg0_base global > variable. Since the OHCI driver can be a loadable module, > this requires the symbol to be exported from platform code. > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Cc: Sekhar Nori <nsekhar@ti.com> > Cc: Kevin Hilman <khilman@deeprootsystems.com> > Cc: davinci-linux-open-source@linux.davincidsp.com > --- > arch/arm/mach-davinci/devices-da8xx.c | 1 + > 1 file changed, 1 insertion(+) > diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c > index 0486cdf..4da868a 100644 > --- a/arch/arm/mach-davinci/devices-da8xx.c > +++ b/arch/arm/mach-davinci/devices-da8xx.c > @@ -66,6 +66,7 @@ > #define DA850_DMA_MMCSD1_TX EDMA_CTLR_CHAN(1, 29) > > void __iomem *da8xx_syscfg0_base; > +EXPORT_SYMBOL_GPL(da8xx_syscfg0_base); /* used by OHCI_HCD */ I have submitted such patch years ago and it was turned down. :-) WBR, Sergei
On 03/19/2014 11:21 PM, Arnd Bergmann wrote: >>> The ohci-da8xx driver uses the DA8XX_SYSCFG0_VIRT macro to >>> access the CFGCHIP2 register for controlling its PHY. >>> The macro in turn relies on the da8xx_syscfg0_base global >>> variable. Since the OHCI driver can be a loadable module, >>> this requires the symbol to be exported from platform code. >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> Cc: Sekhar Nori <nsekhar@ti.com> >>> Cc: Kevin Hilman <khilman@deeprootsystems.com> >>> Cc: davinci-linux-open-source@linux.davincidsp.com >>> --- >>> arch/arm/mach-davinci/devices-da8xx.c | 1 + >>> 1 file changed, 1 insertion(+) >>> diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c >>> index 0486cdf..4da868a 100644 >>> --- a/arch/arm/mach-davinci/devices-da8xx.c >>> +++ b/arch/arm/mach-davinci/devices-da8xx.c >>> @@ -66,6 +66,7 @@ >>> #define DA850_DMA_MMCSD1_TX EDMA_CTLR_CHAN(1, 29) >>> >>> void __iomem *da8xx_syscfg0_base; >>> +EXPORT_SYMBOL_GPL(da8xx_syscfg0_base); /* used by OHCI_HCD */ >> I have submitted such patch years ago and it was turned down. I also had a pleasure of completing implementation of this driver and submitting it back in 2009 when I was working for MontaVista. :-) > The question is whether there is anyone who would do this properly. Nobody cares, it seems. As you can imagine, I stopped to care enough after being switched to other projects. > Both the OHCI and MUSB drivers use exactly one register (CFGCHIP2) The idea at the time was to just ioremap() this register but I was not very eager. There was no USB PHY driver subsystem yet. > to control the clock, phy Note that it only controls PHY clock (PLL) which could be covered by an USB PHY driver. > and host/gadget mode switch. The main mode the USB 2.0 PHY should function now is OTG. The host/gadged modes are forced overrides of the ID pin. Unfortunately, the board code has to force host mode when host-only driver config is selected (these MUSB's host/gadget only modes were removed at one point but got reintroduced again). > In the modern world, we'd probably want to have a clock driver and Not sure about the clock driver... > a phy driver for these, Yes, that's what the MUSB maintainer wants too. The issue is the driver should somehow differ which USB controller it's bound too and do different things depending on that (at least that's how it looks now). I'm not sure it's possible, so probably should be rethought. > based on a syscon driver. I don't know what syscon is... > In all honesty I don't see that happening on davinci. I don't think people use USB 1.1 controllers these days, especially when there is USB 2.0 in the same SoC. For MUSB, there were recent attempt to get the DA8xx driver out of the broken state but it was turned down as well, IIRC, since it didn't offer a PHY driver. > A somewhat better approach would be to export a set of exported > functions to access the one register from the platform, e.g. > u32 da8xx_cfgchip2_get(void); > void da8xx_cfgchip2_set(u32); > That interface would still be a bit ugly, but much better than > what we have today, and easy to implement. I'm leaving this idea to Sekhar... > Arnd WBR, Sergei
On Thursday 20 March 2014 01:36:13 Sergei Shtylyov wrote: > On 03/19/2014 11:21 PM, Arnd Bergmann wrote: > > The question is whether there is anyone who would do this properly. > > Nobody cares, it seems. As you can imagine, I stopped to care enough after > being switched to other projects. I only care about it because I have the intention to get all 'randconfig' kernels to build eventually. For stuff that is definitely 'legacy' and won't get cleaned up properly, I'm fine with a hack. > > Both the OHCI and MUSB drivers use exactly one register (CFGCHIP2) > > The idea at the time was to just ioremap() this register but I was not > very eager. Yes, that would work, too. However, that would cause problems later if we ever try to make the davinci platform "multiplatform", unless we also pass the physical address in a platform resource and make this a larger driver. I think we can reasonably have an exported set of functions declared in the platform data header file for these drivers, but passing the physical address through a header file wouldn't be much of an improvement over passing the virtual address. > There was no USB PHY driver subsystem yet. > > > to control the clock, phy > > Note that it only controls PHY clock (PLL) which could be covered by an > USB PHY driver. Good point. > > and host/gadget mode switch. > > The main mode the USB 2.0 PHY should function now is OTG. The host/gadged > modes are forced overrides of the ID pin. Unfortunately, the board code has to > force host mode when host-only driver config is selected (these MUSB's > host/gadget only modes were removed at one point but got reintroduced again). I think they are also required for 'randconfig' builds to some degree, because you can build a kernel without host mode for instance. > > In the modern world, we'd probably want to have a clock driver and > > Not sure about the clock driver... > > > a phy driver for these, > > Yes, that's what the MUSB maintainer wants too. The issue is the driver > should somehow differ which USB controller it's bound too and do different > things depending on that (at least that's how it looks now). I'm not sure it's > possible, so probably should be rethought. Yes, a phy driver seems the right approach if we can find someone to do it. Ideally that should let us use generic versions of the ohci and musb drivers even, if that's the only davinci specific part of these drivers. > > based on a syscon driver. > > I don't know what syscon is... The syscon (system controller) framework helps share a set of registers across multiple drivers. If all accesses to the CFGCHIP2 register are in a single PHY driver, we wouldn't need it. > > In all honesty I don't see that happening on davinci. > > I don't think people use USB 1.1 controllers these days, especially when > there is USB 2.0 in the same SoC. For MUSB, there were recent attempt to get > the DA8xx driver out of the broken state but it was turned down as well, IIRC, > since it didn't offer a PHY driver. For USB 2.0 compliance you actually need to provide something to handle the low speed modes. A lot of people use a hub to do that nowadays rather than an OHCI or UHCI, but I don't know if that works with OTG. Arnd
On Thursday 20 March 2014 12:12 PM, Arnd Bergmann wrote: > On Thursday 20 March 2014 01:36:13 Sergei Shtylyov wrote: >> On 03/19/2014 11:21 PM, Arnd Bergmann wrote: >>> The question is whether there is anyone who would do this properly. >> >> Nobody cares, it seems. As you can imagine, I stopped to care enough after >> being switched to other projects. > > I only care about it because I have the intention to get all 'randconfig' > kernels to build eventually. For stuff that is definitely 'legacy' > and won't get cleaned up properly, I'm fine with a hack. > >>> Both the OHCI and MUSB drivers use exactly one register (CFGCHIP2) >> >> The idea at the time was to just ioremap() this register but I was not >> very eager. > > Yes, that would work, too. However, that would cause problems later > if we ever try to make the davinci platform "multiplatform", unless we also > pass the physical address in a platform resource and make this a larger Some SATA driver work done by Bartlomiej Zolnierkiewicz needed driver access to syscfg registers too and I just asked him to pass the physical addresses using platform resource. I think thats the best bet we have in the absence of a modern interface. > The syscon (system controller) framework helps share a set of registers > across multiple drivers. If all accesses to the CFGCHIP2 register are > in a single PHY driver, we wouldn't need it. CFGCHIP2 has controls both for USB 1.1 (OHCI) and USB 2.0 (MUSB). Not sure if there can be a single PHY driver for both. Thanks, Sekhar
On Thursday 20 March 2014, Sekhar Nori wrote: > On Thursday 20 March 2014 12:12 PM, Arnd Bergmann wrote: > > On Thursday 20 March 2014 01:36:13 Sergei Shtylyov wrote: > >> On 03/19/2014 11:21 PM, Arnd Bergmann wrote: > >>> The question is whether there is anyone who would do this properly. > >> > >> Nobody cares, it seems. As you can imagine, I stopped to care enough after > >> being switched to other projects. > > > > I only care about it because I have the intention to get all 'randconfig' > > kernels to build eventually. For stuff that is definitely 'legacy' > > and won't get cleaned up properly, I'm fine with a hack. > > > >>> Both the OHCI and MUSB drivers use exactly one register (CFGCHIP2) > >> > >> The idea at the time was to just ioremap() this register but I was not > >> very eager. > > > > Yes, that would work, too. However, that would cause problems later > > if we ever try to make the davinci platform "multiplatform", unless we also > > pass the physical address in a platform resource and make this a larger > > Some SATA driver work done by Bartlomiej Zolnierkiewicz needed driver > access to syscfg registers too and I just asked him to pass the physical > addresses using platform resource. I think thats the best bet we have in > the absence of a modern interface. Ok. > > The syscon (system controller) framework helps share a set of registers > > across multiple drivers. If all accesses to the CFGCHIP2 register are > > in a single PHY driver, we wouldn't need it. > > CFGCHIP2 has controls both for USB 1.1 (OHCI) and USB 2.0 (MUSB). Not > sure if there can be a single PHY driver for both. The phy infrastructure explicitly supports multiple consumers for one phy, if I'm reading the code correctly. Arnd
Hello. On 20-03-2014 10:42, Arnd Bergmann wrote: >>> Both the OHCI and MUSB drivers use exactly one register (CFGCHIP2) >> The idea at the time was to just ioremap() this register but I was not >> very eager. ... also it was suggested to pass the CFGCHIP2 address as a resource. I certainly wasn't eager to do that since if done for both MUSB and OHCI, it would cause sort of a resource conflict (platform device resources are automatically registered in the resource tree, although without marking them exclusive), even if we don't call request_mem_region() on them (we can't do that since in this case the resource would be registered as exclusive, and the second call would fail). > Yes, that would work, too. However, that would cause problems later > if we ever try to make the davinci platform "multiplatform", unless we also > pass the physical address in a platform resource and make this a larger > driver. In my opinion, we don't have to pass CFGCHIP2 as a resource and could just ioremap() the raw physical address. > I think we can reasonably have an exported set of functions > declared in the platform data header file for these drivers, but passing > the physical address through a header file That's how it's passed today (however, thru <mach/da8xx.h>). > wouldn't be much of an improvement > over passing the virtual address. Not sure I understood about passing virtual address. >> There was no USB PHY driver subsystem yet. >>> to control the clock, phy >> Note that it only controls PHY clock (PLL) which could be covered by an >> USB PHY driver. > Good point. >>> and host/gadget mode switch. >> The main mode the USB 2.0 PHY should function now is OTG. The host/gadged >> modes are forced overrides of the ID pin. Unfortunately, the board code has to >> force host mode when host-only driver config is selected (these MUSB's >> host/gadget only modes were removed at one point but got reintroduced again). > I think they are also required for 'randconfig' builds to some degree, because > you can build a kernel without host mode for instance. Yes. >>> In the modern world, we'd probably want to have a clock driver and >> Not sure about the clock driver... >>> a phy driver for these, >> Yes, that's what the MUSB maintainer wants too. The issue is the driver >> should somehow differ which USB controller it's bound too and do different >> things depending on that (at least that's how it looks now). I'm not sure it's >> possible, so probably should be rethought. > Yes, a phy driver seems the right approach if we can find someone to do it. Perhaps a person that tried to unbreak the DA8xx MUSB driver could be interested (and competent) enough to do it... > Ideally that should let us use generic versions of the ohci and musb drivers > even, if that's the only davinci specific part of these drivers. No, not really. MUSB ceratinly has DaVinci specific register set mapped in its register space. OHCI has up to 2 clocks to enable since USB 1.1 PHY can be clocked from USB 2.0 PHY clock. >>> based on a syscon driver. >> I don't know what syscon is... > The syscon (system controller) framework helps share a set of registers > across multiple drivers. If all accesses to the CFGCHIP2 register are > in a single PHY driver, we wouldn't need it. Where can I find it in the kernel tree? >>> In all honesty I don't see that happening on davinci. >> I don't think people use USB 1.1 controllers these days, especially when >> there is USB 2.0 in the same SoC. For MUSB, there were recent attempt to get >> the DA8xx driver out of the broken state but it was turned down as well, IIRC, >> since it didn't offer a PHY driver. > For USB 2.0 compliance you actually need to provide something to handle the > low speed modes. A lot of people use a hub to do that nowadays rather than > an OHCI or UHCI, but I don't know if that works with OTG. MUSB handles all speeds. I think the reason TI also included USB 1.1 controller lies in the somewhat dubious reputation of both MUSB hardware and the Linux driver. > Arnd WBR, Sergei
On Thursday 20 March 2014 21:59:29 Sergei Shtylyov wrote: > > Yes, it does. The issue is that the PHY code is different in MUSB and OHCI > drivers. I don't think the PHY driver knows what device binds to it. At least in the DT case, it can get that information from DT, when you set #phy-cells=<1>. I don't know how it would be done for platform_data, but I assume one could find a way too. Arnd
Hello. On 03/20/2014 02:50 PM, Arnd Bergmann wrote: >>>>> The question is whether there is anyone who would do this properly. >>>> Nobody cares, it seems. As you can imagine, I stopped to care enough after >>>> being switched to other projects. >>> I only care about it because I have the intention to get all 'randconfig' >>> kernels to build eventually. For stuff that is definitely 'legacy' >>> and won't get cleaned up properly, I'm fine with a hack. >>>>> Both the OHCI and MUSB drivers use exactly one register (CFGCHIP2) >>>> The idea at the time was to just ioremap() this register but I was not >>>> very eager. >>> Yes, that would work, too. However, that would cause problems later >>> if we ever try to make the davinci platform "multiplatform", unless we also >>> pass the physical address in a platform resource and make this a larger >> Some SATA driver work done by Bartlomiej Zolnierkiewicz needed driver >> access to syscfg registers too and I just asked him to pass the physical >> addresses using platform resource. I think thats the best bet we have in >> the absence of a modern interface. > Ok. Depends on what SYSCFG register it uses. It wouldn't be good if that register range includes CFGCHIP2. >>> The syscon (system controller) framework helps share a set of registers >>> across multiple drivers. If all accesses to the CFGCHIP2 register are >>> in a single PHY driver, we wouldn't need it. >> CFGCHIP2 has controls both for USB 1.1 (OHCI) and USB 2.0 (MUSB). Not >> sure if there can be a single PHY driver for both. > The phy infrastructure explicitly supports multiple consumers for one > phy, if I'm reading the code correctly. Yes, it does. The issue is that the PHY code is different in MUSB and OHCI drivers. I don't think the PHY driver knows what device binds to it. > Arnd WBR, Sergei
Hello. On 03/20/2014 09:22 PM, Arnd Bergmann wrote: >> Yes, it does. The issue is that the PHY code is different in MUSB and OHCI >> drivers. I don't think the PHY driver knows what device binds to it. > At least in the DT case, it can get that information from DT, when you > set #phy-cells=<1>. I don't know how it would be done for platform_data, > but I assume one could find a way too. #phy-cells is only defined for drivers/phy/ bindings IIRC, and I was thinking a drivers/usb/phy/ driver so far. Probably you have a point and we should go for the generic PHY framework instead. I'm just not very familiar with it... > Arnd WBR, Sergei
On 03/20/2014 07:20 PM, Sergei Shtylyov wrote: >> The syscon (system controller) framework helps share a set of registers >> across multiple drivers. If all accesses to the CFGCHIP2 register are >> in a single PHY driver, we wouldn't need it. > Where can I find it in the kernel tree? Found it. WBR, Sergei
diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c index 0486cdf..4da868a 100644 --- a/arch/arm/mach-davinci/devices-da8xx.c +++ b/arch/arm/mach-davinci/devices-da8xx.c @@ -66,6 +66,7 @@ #define DA850_DMA_MMCSD1_TX EDMA_CTLR_CHAN(1, 29) void __iomem *da8xx_syscfg0_base; +EXPORT_SYMBOL_GPL(da8xx_syscfg0_base); /* used by OHCI_HCD */ void __iomem *da8xx_syscfg1_base; static struct plat_serial8250_port da8xx_serial0_pdata[] = {
The ohci-da8xx driver uses the DA8XX_SYSCFG0_VIRT macro to access the CFGCHIP2 register for controlling its PHY. The macro in turn relies on the da8xx_syscfg0_base global variable. Since the OHCI driver can be a loadable module, this requires the symbol to be exported from platform code. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Cc: Sekhar Nori <nsekhar@ti.com> Cc: Kevin Hilman <khilman@deeprootsystems.com> Cc: davinci-linux-open-source@linux.davincidsp.com --- arch/arm/mach-davinci/devices-da8xx.c | 1 + 1 file changed, 1 insertion(+)