Re: [RFC] AM35xx glue code for M-USB driver

[Felipe Balbi]

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(>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

[Rolf Peukert]

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(>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

[RFC] AM35xx glue code for M-USB driver

[Rolf Peukert]

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(_workaround, jiffies + POLL_SECONDS * HZ);

spin_unlock_irqrestore(>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(>dev, sizeof(*pdata), GFP_KERNEL);
+   if (!pdata)
+   goto err_out;
+
+   bdata = devm_kzalloc(>dev, sizeof(*bdata), GFP_KERNEL);
+   if (!bdata)
+   goto err_pdata;
+
+   config = devm_kzalloc(>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 *)>mode);
+   of_property_read_u32(np, "interface-type",
+   (u32 *)>interface_type);
+   of_property_read_u32(np, "num-eps", (u32 *)>num_eps);
+   of_property_read_u32(np, "ram-bits", (u32 *)>ram_bits);
+   of_property_read_u32(np, "power", (u32 *)>power);
+
+   ret = of_property_read_u32(np, "multipoint", );
+   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(>dev);
@@ -475,14 +544,20 @@ static int am35x_probe(struct platform_d
goto err0;

Re: [RFC] AM35xx glue code for M-USB driver

[Felipe Balbi]

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(>dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + goto err_out;
> +
> + bdata = devm_kzalloc(>dev, sizeof(*bdata), GFP_KERNEL);
> + if (!bdata)
> + goto err_pdata;
> +
> + config = devm_kzalloc(>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 *)>mode);
> + of_property_read_u32(np, "interface-type",
> + (u32 *)>interface_type);
> + of_property_read_u32(np, "num-eps", (u32 *)>num_eps);
> + of_property_read_u32(np, "ram-bits", (u32 *)>ram_bits);
> + 

Re: [RFC] AM35xx glue code for M-USB driver

[kbuild test robot]

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

Re: [RFC] AM35xx glue code for M-USB driver

[Rolf Peukert]

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(>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

[Rolf Peukert]

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