RE: [PATCH resend 2/3] musb: add musb support for AM35x

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,
> >>> +{
> >>> +	unsigned long timeout = jiffies + msecs_to_jiffies(100);
> >>> +	u32 devconf2;
> >>> +
> >>> +	/*
> >>> +	 * Start the on-chip PHY and its PLL.
> >>> +	 */
> >>> +	devconf2 = omap_ctrl_readl(AM35XX_CONTROL_DEVCONF2);
> >>> +
> >>> +	devconf2 &= ~(CONF2_RESET | CONF2_PHYPWRDN | CONF2_OTGPWRDN |
> >>> +			CONF2_PHY_GPIOMODE);
> 
> >>     BWT, what's thet GPIOMODE bit for?
> 
> > If GPIO Mode is set to '1' then UART3 Tx/Rx would come on DP and
> > DM pins of USBPHY.
> 
>     Hm, why GPIOMODE then I wonder... :-)
> 
> >>> +	devconf2 |= CONF2_PHY_PLLON | CONF2_DATPOL;
> 
> >>     So, AM35x always uses the reversed data polarity?
> 
> > It's getting set to '1' so its always normal polarity.
> 
>     Ah, I got it reversed: 1 means normal polarity indeed...

I think, this would again be dependent on boards and therefore
should be moved to board file.

> 
> >>> +static void otg_timer(unsigned long _musb)
> >>> +{
> >>> +	struct musb		*musb = (void *)_musb;
> >>> +	void __iomem		*mregs = musb->mregs;
> >>> +	u8			devctl;
> >>> +	unsigned long		flags;
> >>> +
> >>> +	/*
> >>> +	 * We poll because AM35x's won't expose several OTG-critical
> >>> +	 * status change events (from the transceiver) otherwise.
> >>> +	 */
> >>> +	devctl = musb_readb(mregs, MUSB_DEVCTL);
> >>> +	DBG(7, "Poll devctl %02x (%s)\n", devctl, otg_state_string(musb));
> >>> +
> >>> +	spin_lock_irqsave(&musb->lock, flags);
> >>> +	switch (musb->xceiv->state) {
> 
> >> [...]
> 
> >>> +	case OTG_STATE_A_WAIT_VFALL:
> 
> >>     So, are you sure there's no need to call mod_timer() here for RTL
> 1.8?
> 
> > What issue did you see on earlier RTLs ?
> 
>     I didn't test DaVinci much; I didn't even test DA8xx much in OTG mode.
> :-/
> And in the DA8xx host mode VBUS error would never occur anyway (as the
> VBUS
> comparator is overridden on the EVM board).
> 
> > As such I didn't see issue
> > In my normal testing.
> 
>     OK.
> 
> >>> +		musb->xceiv->state = OTG_STATE_A_WAIT_VRISE;
> >>> +		musb_writel(musb->ctrl_base, CORE_INTR_SRC_SET_REG,
> >>> +			    MUSB_INTR_VBUSERROR << USB_INTR_USB_SHIFT);
> >>> +		break;
> >>> +	case OTG_STATE_B_IDLE:
> >>> +		if (!is_peripheral_enabled(musb))
> >>> +			break;
> 
> >>     Hm, davinci.c sets DevCtl.Session here and I'm also doing it in
> >> da8xx.c --
> >> can you comment why you don't do that too?
> 
> > I remember this was causing some issue in OTG testing.
> 
>     Do you remember which issue?
>     Although I suspect that this code might be related to the comment
> below:
> "DEVCTL.BDEVICE is invalid without DEVCTL.SESSION set".

I will test again and then provide the details. I too feel it has something
to do with the comment you pointed out.

> 
> >>> +		devctl = musb_readb(mregs, MUSB_DEVCTL);
> >>> +		if (devctl & MUSB_DEVCTL_BDEVICE)
> >>> +			mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
> >>> +		else
> >>> +			musb->xceiv->state = OTG_STATE_A_IDLE;
> >>> +		break;
> 
> >> [...]
> 
> >>> +	musb->a_wait_bcon = A_WAIT_BCON_TIMEOUT;
> 
> >>     This field is set by musb_core.c, so I dropped this line from
> >> da8xx.c...
> 
>     This has also been dropped from omap2430.c by this commit:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-
> 2.6.git;a=commit;h=f7f9d63eac12b345d6243d1d608b7944a05be921
> 
> > Need to test again but I was facing a issue where OTG build image
> > Was either working in host only mode or device only. Role were
> > Not changing based on ID pin status.
> 
>     Hm, is this related?

Not really but let me test again and get the details.

> 
> >>> +int musb_platform_exit(struct musb *musb)
> >>> +{
> >>> +	struct clk *otg_fck = clk_get(musb->controller, "fck");
> >>> +
> >>> +	if (is_host_enabled(musb))
> >>> +		del_timer_sync(&otg_workaround);
> >>> +
> >>> +	/* Delay to avoid problems with module reload... */
> 
[..]
> 
> >>     musb_platfrom_init() suggests that otg_fck can't be NULL. Also,
> >> clk_get
> >> returns error code, not NULL, so should use IS_ERR() here.
> 
> > Correct, would clean it.
> 
>     Also, you call clk_get() on this clock twice, in musb_platfrom_init()
> and
> here, but clk_put() only once.

Ok fine.

Thanks,
Ajay
> 
> > Thanks,
> > Ajay
> 
> WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux