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

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

 



Hi,
> Ajay Kumar Gupta <ajay.gupta@xxxxxx> wrote:
> > AM35x is based on OMAP35x but has an updated musb interface which
> > uses CPPI4.1 DMA engine.
> >
> > Current patch supports only PIO mode transfers.
> >
> > Signed-off-by: Ajay Kumar Gupta <ajay.gupta@xxxxxx>
> > ---
> > Changes from v1: (Based on Sergei's comment)
> >        - Moved PHY clock and OTGMODE settings to board files
> >        - Added clk_disable/put in exit path
> >        - Removed unwanted code related to vbus
> >        - Removed unhandled interrupt check
> >        - Added timeout in phy_on()
> > I had to use MACH_OMAP3517EVM as there is no ARCH_xxx config specific to
> > AM3517 platforms other than ARCH_OMAP3.
> >
> >  drivers/usb/musb/Kconfig  |    4 +-
> >  drivers/usb/musb/Makefile |    4 +
> >  drivers/usb/musb/am3517.c |  517
[..]
> > +
> > +static inline void phy_on(void)
> > +{
> > +       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);
> > +       devconf2 |= CONF2_PHY_PLLON | CONF2_DATPOL;
> > +
> > +       omap_ctrl_writel(devconf2, AM35XX_CONTROL_DEVCONF2);
> > +
> > +       DBG(1, "Waiting for PHY clock good...\n");
> > +       while (!(omap_ctrl_readl(AM35XX_CONTROL_DEVCONF2)
> > +                       & CONF2_PHYCLKGD)) {
> > +               cpu_relax();
> > +
> > +               if (time_after(jiffies, timeout)) {
> > +                       DBG(1, "musb PHY clock good timed out\n");
> > +                       return;
> A 'break' would have been more appropriate.

Why?
'break' would bring in additional instruction and finally it would
return as the loop is already at the bottom of the function.

> 
> > +               }
> > +       }
> > +}
> > +
[..]
> > +int musb_platform_set_mode(struct musb *musb, u8 musb_mode)
> > +{
> > +       u32 devconf2 = omap_ctrl_readl(AM35XX_CONTROL_DEVCONF2);
> > +
> > +       devconf2 &= ~CONF2_OTGMODE;
> > +       switch (musb_mode) {
> > +#ifdef CONFIG_USB_MUSB_HDRC_HCD
> > +       case MUSB_HOST:         /* Force VBUS valid, ID = 0 */
> > +               devconf2 |= CONF2_FORCE_HOST;
> > +               break;
> > +#endif
> > +#ifdef CONFIG_USB_GADGET_MUSB_HDRC
> > +       case MUSB_PERIPHERAL:   /* Force VBUS valid, ID = 1 */
> > +               devconf2 |= CONF2_FORCE_DEVICE;
> > +               break;
> > +#endif
> > +#ifdef CONFIG_USB_MUSB_OTG
> > +       case MUSB_OTG:          /* Don't override the VBUS/ID
> comparators */
> > +               devconf2 |= CONF2_NO_OVERRIDE;
> > +               break;
> > +#endif
> > +       default:
> > +               DBG(2, "Trying to set unsupported mode %u\n",
> musb_mode);
> > +       }
> Either the switch case or the compile time flag is redundant.
No. both has to be there.

'musb_mode' is an entry from user via sysfs for changing the
operating mode and if some modes are not even configured in
kernel then we should not switch the mode and also warn the
user for this.

Ajay
> 
> > +
> > +       omap_ctrl_writel(devconf2, AM35XX_CONTROL_DEVCONF2);
> > +       return 0;
[..]
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux