Re: [RFC] AM35xx glue code for M-USB driver
Hi, Rolf Peukert writes: > Hi Felipe, > > On 07.10.2015 18:22, Felipe Balbi wrote: > [...] > b) The M-USB port on our board is wired as host only. If a device is > unplugged, the driver normally goes into some idle state and waits for > an ID signal change. But on our board that never happens. did you route ID pin anywhere ? I hope you tied it to ground. >>> >>> I think so. I'll double-check that. >> >> cool, thanks. > > The ID pin was not connected, it's tied to ground now, but still the > same behaviour. But I can keep it from entering the musb_try_idle > function by deactivating the vbus timeout. This can be done from > userspace, so we don't need the additional if-statements in the code. okay, eventually we want things to work without relying on userspace though. For now, it should be enough to do what you're doing so we focus on other issues. >>> [...] > + /* Set defaults */ > + config->num_eps = 16; > + config->ram_bits = 12; > + config->multipoint = true; all of these are board-specific. > + > + bdata->interface_type = MUSB_INTERFACE_UTMI; > + bdata->mode = MUSB_OTG; > + bdata->power = 500; > + bdata->extvbus = 1; so are these. > > OK, if everything is declared in the device tree, we don't need to set > default values here. right. > Regarding the four am35x_... functions from omap_phy_internal.c, it's > not easy to move them over to am35x.c. > While they just set a few bits in some system control module registers, > they call functions from control.c to access the SCM. The control.c > functions are not exported either (and use some static variables and > also local include files from mach-omap2). yeah, there's still quite a bit left to clean up for those devices and, unfortunately, I don't even have them available so I can't help much :-s >>> [...] > + phy_clk = clk_get(&pdev->dev, "hsotgusb_fck"); why did you change the clock name ? That shouldn't be necessary. >>> >>> I did get "failed to get PHY clock" and "failed to get clock" >>> messages. >> >> right, this a bug elsewhere. Drivers shouldn't care about the exact >> clock name ;-) >> > > The corresponding clock declaration seems to be in > drivers/clk/ti/clk-3xxx.c: > > DT_CLK(NULL, "hsotgusb_ick", "hsotgusb_ick_am35xx"), > DT_CLK(NULL, "hsotgusb_fck", "hsotgusb_fck_am35xx"), > > When I add two more lines there, > > DT_CLK("5c04.am35x_otg_hs", "ick", "hsotgusb_ick_am35xx"), > DT_CLK("5c04.am35x_otg_hs", "fck", "hsotgusb_fck_am35xx"), > > the "ick" and "fck" clocks are found. It doesn't work without the > address in the device name, and not with just "musb-am35x" either. right, you need to pass the correct device name. This alone is a bug fix worth sending. But send this two-line change as a single patch, a single bug fix with nothing else in it. > Still, all other device names in that file look simpler, and I'm > wondering if I forgot a proper name declaration somewhere else? no, you're correct. Those other devices are likely broken on DT boots too, dunno for sure. -- balbi signature.asc Description: PGP signature
Re: [RFC] AM35xx glue code for M-USB driver
Hi Felipe, On 07.10.2015 18:22, Felipe Balbi wrote: [...] b) The M-USB port on our board is wired as host only. If a device is unplugged, the driver normally goes into some idle state and waits for an ID signal change. But on our board that never happens. >>> >>> did you route ID pin anywhere ? I hope you tied it to ground. >> >> I think so. I'll double-check that. > > cool, thanks. The ID pin was not connected, it's tied to ground now, but still the same behaviour. But I can keep it from entering the musb_try_idle function by deactivating the vbus timeout. This can be done from userspace, so we don't need the additional if-statements in the code. >> [...] + /* Set defaults */ + config->num_eps = 16; + config->ram_bits = 12; + config->multipoint = true; >>> >>> all of these are board-specific. >>> + + bdata->interface_type = MUSB_INTERFACE_UTMI; + bdata->mode = MUSB_OTG; + bdata->power = 500; + bdata->extvbus = 1; >>> >>> so are these. OK, if everything is declared in the device tree, we don't need to set default values here. Regarding the four am35x_... functions from omap_phy_internal.c, it's not easy to move them over to am35x.c. While they just set a few bits in some system control module registers, they call functions from control.c to access the SCM. The control.c functions are not exported either (and use some static variables and also local include files from mach-omap2). >> [...] + phy_clk = clk_get(&pdev->dev, "hsotgusb_fck"); >>> >>> why did you change the clock name ? That shouldn't be necessary. >> >> I did get "failed to get PHY clock" and "failed to get clock" >> messages. > > right, this a bug elsewhere. Drivers shouldn't care about the exact > clock name ;-) > The corresponding clock declaration seems to be in drivers/clk/ti/clk-3xxx.c: DT_CLK(NULL, "hsotgusb_ick", "hsotgusb_ick_am35xx"), DT_CLK(NULL, "hsotgusb_fck", "hsotgusb_fck_am35xx"), When I add two more lines there, DT_CLK("5c04.am35x_otg_hs", "ick", "hsotgusb_ick_am35xx"), DT_CLK("5c04.am35x_otg_hs", "fck", "hsotgusb_fck_am35xx"), the "ick" and "fck" clocks are found. It doesn't work without the address in the device name, and not with just "musb-am35x" either. Still, all other device names in that file look simpler, and I'm wondering if I forgot a proper name declaration somewhere else? Best regards, Rolf -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] AM35xx glue code for M-USB driver
Hi, Rolf Peukert writes: > Hi Felipe, > > thank you for your reply. > > On 07.10.2015 15:59, Felipe Balbi wrote: >> >> Hi, >> >> (please sure you also Cc linux-...@vger.kernel.org for MUSB patches) >> >> Rolf Peukert writes: > [...] >>> b) The M-USB port on our board is wired as host only. If a device is >>> unplugged, the driver normally goes into some idle state and waits for >>> an ID signal change. But on our board that never happens. >> >> did you route ID pin anywhere ? I hope you tied it to ground. > > I think so. I'll double-check that. cool, thanks. > [...] >>> + /* Set defaults */ >>> + config->num_eps = 16; >>> + config->ram_bits = 12; >>> + config->multipoint = true; >> >> all of these are board-specific. >> >>> + >>> + bdata->interface_type = MUSB_INTERFACE_UTMI; >>> + bdata->mode = MUSB_OTG; >>> + bdata->power = 500; >>> + bdata->extvbus = 1; >> >> so are these. > > The AM3517/05 has an internal phy, so some of these are predetermined. > You're right about mode and power. and are you sure all AM35xx devices have the same number of endpoints, the same amount of ram and all ? Chances are, you're right :-p No point in making a new integration just for a variant. > [...] >>> + phy_clk = clk_get(&pdev->dev, "hsotgusb_fck"); >> >> why did you change the clock name ? That shouldn't be necessary. > > I did get "failed to get PHY clock" and "failed to get clock" > messages. right, this a bug elsewhere. Drivers shouldn't care about the exact clock name ;-) > I'll do some more testing and see if I can get by with less changes. cool. Also, have a read at Documentation/CodingStyle, Documentation/SubmittingPatches and Documentation/SubmitChecklist. In short, you want to make patches containing a single logical change. So your patch here needs to get broken. You also need to write proper commit logs and sign your patch. It's all described on the documentation I pointed out. -- balbi signature.asc Description: PGP signature
Re: [RFC] AM35xx glue code for M-USB driver
On 07.10.2015 14:15, kbuild test robot wrote: > Hi Rolf, > > [auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please > ignore] > > config: arm-omap2plus_defconfig (attached as .config) > reproduce: > wget > https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross > -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=arm > > All errors (new ones prefixed by >>): > >>> ERROR: "am35x_musb_reset" [drivers/usb/musb/am35x.ko] undefined! >>> ERROR: "am35x_set_mode" [drivers/usb/musb/am35x.ko] undefined! >>> ERROR: "am35x_musb_clear_irq" [drivers/usb/musb/am35x.ko] undefined! >>> ERROR: "am35x_musb_phy_power" [drivers/usb/musb/am35x.ko] undefined! Sorry, forgot to test it as a module. Guess I'll have to add some EXPORT_SYMBOL lines. Best regards, Rolf -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] AM35xx glue code for M-USB driver
Hi Felipe, thank you for your reply. On 07.10.2015 15:59, Felipe Balbi wrote: > > Hi, > > (please sure you also Cc linux-...@vger.kernel.org for MUSB patches) > > Rolf Peukert writes: [...] >> b) The M-USB port on our board is wired as host only. If a device is >> unplugged, the driver normally goes into some idle state and waits for >> an ID signal change. But on our board that never happens. > > did you route ID pin anywhere ? I hope you tied it to ground. I think so. I'll double-check that. [...] >> +/* Set defaults */ >> +config->num_eps = 16; >> +config->ram_bits = 12; >> +config->multipoint = true; > > all of these are board-specific. > >> + >> +bdata->interface_type = MUSB_INTERFACE_UTMI; >> +bdata->mode = MUSB_OTG; >> +bdata->power = 500; >> +bdata->extvbus = 1; > > so are these. The AM3517/05 has an internal phy, so some of these are predetermined. You're right about mode and power. [...] >> +phy_clk = clk_get(&pdev->dev, "hsotgusb_fck"); > > why did you change the clock name ? That shouldn't be necessary. I did get "failed to get PHY clock" and "failed to get clock" messages. I'll do some more testing and see if I can get by with less changes. Best regards, Rolf -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] AM35xx glue code for M-USB driver
Hi, (please sure you also Cc linux-...@vger.kernel.org for MUSB patches) Rolf Peukert writes: > Hi, > > we're running a 4.x kernel on a board with an AM3505 CPU and would like > to use device trees. The AM35xx glue code for the M-USB controller > didn't support configuration from a DT yet. I now got it working > somehow, but I'm not sure if I'm doing it the correct way. So this post > is not meant as a patch submission, but more as a request for comments > or help. > > In the older kernel we were using before (3.2.x), some data structures > were set up in the respective board file and then passed to the function > am35x_probe(). For the use with DT, I added the function > am35x_get_config, which sets up these data structures with default > values and then tries to read settings from the DT. > The device .compatible declaration is now "ti,am35x-musb", so it's > separate from "ti,omap3-musb" (as used in omap2430.c). > > Now the two things I'm not so sure about: > > a) We needed to set pointers to machine-specific functions like > am35x_musb_clear_irq etc. These functions are implemented in > arch/arm/mach-omap2/omap_phy_internal.c, and declared in > arch/arm/mach-omap2/usb.h. > As this header file can't be included easily from am35x.c, I moved the > declarations to include/linux/platform_data/usb-omap.h. I hope that's > OK, but would be happy about suggestions. Yeah, those functions should not be defined under arch/arm/mach-omap2, they need to be moved to drivers/usb/musb/am35x.c. That PHY power stuff also needs to move to some system controller driver of some sort. > b) The M-USB port on our board is wired as host only. If a device is > unplugged, the driver normally goes into some idle state and waits for > an ID signal change. But on our board that never happens. did you route ID pin anywhere ? I hope you tied it to ground. > So I added two checks for MUSB_OTG mode to support our hardware. Then if your HW is host-only, why are you messing around with OTG ? > I noticed similar code was removed from this file three years ago > (commit 032ec49f5351e9cb242b1a1c367d14415043ab95), and I don't know if > these lines might break something on other hardware. > > Best regards, > Rolf > > --- > Index: linux4/drivers/usb/musb/am35x.c > === > --- linux4.orig/drivers/usb/musb/am35x.c > +++ linux4/drivers/usb/musb/am35x.c > @@ -188,6 +188,10 @@ static void am35x_musb_try_idle(struct m > { > static unsigned long last_timer; > > + /* do not try if not in OTG mode */ > + if (musb->port_mode != MUSB_OTG) > + return; peripheral-only and host-only configurations are valid, you should not bail out unless OTG. > @@ -323,8 +327,9 @@ eoi: > musb_writel(reg_base, USB_END_OF_INTR_REG, 0); > } > > - /* Poll for ID change */ > - if (musb->xceiv->otg->state == OTG_STATE_B_IDLE) > + /* Poll for ID change (in OTG mode only) */ > + if (musb->port_mode == MUSB_OTG && > + musb->xceiv->otg->state == OTG_STATE_B_IDLE) no, this is wrong too. > @@ -458,6 +463,70 @@ static const struct platform_device_info > .dma_mask = DMA_BIT_MASK(32), > }; > > +static struct musb_hdrc_platform_data * > +am35x_get_config(struct platform_device *pdev) > +{ > + struct musb_hdrc_platform_data *pdata; > + struct omap_musb_board_data *bdata; > + struct musb_hdrc_config *config; > + struct device_node *np; > + int val, ret; > + > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + goto err_out; > + > + bdata = devm_kzalloc(&pdev->dev, sizeof(*bdata), GFP_KERNEL); > + if (!bdata) > + goto err_pdata; > + > + config = devm_kzalloc(&pdev->dev, sizeof(*config), GFP_KERNEL); > + if (!config) > + goto err_bdata; > + > + /* Set defaults */ > + config->num_eps = 16; > + config->ram_bits = 12; > + config->multipoint = true; all of these are board-specific. > + > + bdata->interface_type = MUSB_INTERFACE_UTMI; > + bdata->mode = MUSB_OTG; > + bdata->power = 500; > + bdata->extvbus = 1; so are these. > + bdata->set_phy_power = am35x_musb_phy_power; > + bdata->clear_irq = am35x_musb_clear_irq; > + bdata->set_mode = am35x_set_mode; > + bdata->reset = am35x_musb_reset; > + > + pdata->mode = MUSB_OTG; > + pdata->power = 250; also mode and power. > + pdata->board_data = bdata; > + pdata->config = config; > + > + /* Read settings from device tree */ > + np = pdev->dev.of_node; > + if (np) { > + of_property_read_u32(np, "mode", (u32 *)&pdata->mode); > + of_property_read_u32(np, "interface-type", > + (u32 *)&bdata->interface_type); > + of_property_read_u32(np, "num-eps", (u32 *)&config->num_eps); > + of_property_read_u32(np, "ram-bits", (u32 *)&config->ram_bit
Re: [RFC] AM35xx glue code for M-USB driver
Hi Rolf, [auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please ignore] config: arm-omap2plus_defconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm All errors (new ones prefixed by >>): >> ERROR: "am35x_musb_reset" [drivers/usb/musb/am35x.ko] undefined! >> ERROR: "am35x_set_mode" [drivers/usb/musb/am35x.ko] undefined! >> ERROR: "am35x_musb_clear_irq" [drivers/usb/musb/am35x.ko] undefined! >> ERROR: "am35x_musb_phy_power" [drivers/usb/musb/am35x.ko] undefined! --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
[RFC] AM35xx glue code for M-USB driver
Hi, we're running a 4.x kernel on a board with an AM3505 CPU and would like to use device trees. The AM35xx glue code for the M-USB controller didn't support configuration from a DT yet. I now got it working somehow, but I'm not sure if I'm doing it the correct way. So this post is not meant as a patch submission, but more as a request for comments or help. In the older kernel we were using before (3.2.x), some data structures were set up in the respective board file and then passed to the function am35x_probe(). For the use with DT, I added the function am35x_get_config, which sets up these data structures with default values and then tries to read settings from the DT. The device .compatible declaration is now "ti,am35x-musb", so it's separate from "ti,omap3-musb" (as used in omap2430.c). Now the two things I'm not so sure about: a) We needed to set pointers to machine-specific functions like am35x_musb_clear_irq etc. These functions are implemented in arch/arm/mach-omap2/omap_phy_internal.c, and declared in arch/arm/mach-omap2/usb.h. As this header file can't be included easily from am35x.c, I moved the declarations to include/linux/platform_data/usb-omap.h. I hope that's OK, but would be happy about suggestions. b) The M-USB port on our board is wired as host only. If a device is unplugged, the driver normally goes into some idle state and waits for an ID signal change. But on our board that never happens. So I added two checks for MUSB_OTG mode to support our hardware. Then I noticed similar code was removed from this file three years ago (commit 032ec49f5351e9cb242b1a1c367d14415043ab95), and I don't know if these lines might break something on other hardware. Best regards, Rolf --- Index: linux4/drivers/usb/musb/am35x.c === --- linux4.orig/drivers/usb/musb/am35x.c +++ linux4/drivers/usb/musb/am35x.c @@ -188,6 +188,10 @@ static void am35x_musb_try_idle(struct m { static unsigned long last_timer; + /* do not try if not in OTG mode */ + if (musb->port_mode != MUSB_OTG) + return; + if (timeout == 0) timeout = jiffies + msecs_to_jiffies(3); @@ -323,8 +327,9 @@ eoi: musb_writel(reg_base, USB_END_OF_INTR_REG, 0); } - /* Poll for ID change */ - if (musb->xceiv->otg->state == OTG_STATE_B_IDLE) + /* Poll for ID change (in OTG mode only) */ + if (musb->port_mode == MUSB_OTG && + musb->xceiv->otg->state == OTG_STATE_B_IDLE) mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ); spin_unlock_irqrestore(&musb->lock, flags); @@ -458,6 +463,70 @@ static const struct platform_device_info .dma_mask = DMA_BIT_MASK(32), }; +static struct musb_hdrc_platform_data * +am35x_get_config(struct platform_device *pdev) +{ + struct musb_hdrc_platform_data *pdata; + struct omap_musb_board_data *bdata; + struct musb_hdrc_config *config; + struct device_node *np; + int val, ret; + + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) + goto err_out; + + bdata = devm_kzalloc(&pdev->dev, sizeof(*bdata), GFP_KERNEL); + if (!bdata) + goto err_pdata; + + config = devm_kzalloc(&pdev->dev, sizeof(*config), GFP_KERNEL); + if (!config) + goto err_bdata; + + /* Set defaults */ + config->num_eps = 16; + config->ram_bits = 12; + config->multipoint = true; + + bdata->interface_type = MUSB_INTERFACE_UTMI; + bdata->mode = MUSB_OTG; + bdata->power = 500; + bdata->extvbus = 1; + bdata->set_phy_power = am35x_musb_phy_power; + bdata->clear_irq = am35x_musb_clear_irq; + bdata->set_mode = am35x_set_mode; + bdata->reset = am35x_musb_reset; + + pdata->mode = MUSB_OTG; + pdata->power = 250; + pdata->board_data = bdata; + pdata->config = config; + + /* Read settings from device tree */ + np = pdev->dev.of_node; + if (np) { + of_property_read_u32(np, "mode", (u32 *)&pdata->mode); + of_property_read_u32(np, "interface-type", + (u32 *)&bdata->interface_type); + of_property_read_u32(np, "num-eps", (u32 *)&config->num_eps); + of_property_read_u32(np, "ram-bits", (u32 *)&config->ram_bits); + of_property_read_u32(np, "power", (u32 *)&pdata->power); + + ret = of_property_read_u32(np, "multipoint", &val); + if (!ret && val) + config->multipoint = true; + } + return pdata; + +err_bdata: + kfree(bdata); +err_pdata: + kfree(pdata); +err_out: + return NULL; +} + static int am35x_probe(struct platform_device *pdev) { struct musb_hdrc_platform_data *pdata = dev_get_platdata(&pdev->dev); @@ -475,14 +544,20 @@ st