On Wed, 2017-08-16 at 10:17 +0200, Uwe Kleine-König wrote:
> Hallo Jan,
>
> On Tue, Aug 15, 2017 at 11:47:37AM +0200, Jan Luebbe wrote:
> > The PLL setup is needed to use the USB ports in Linux.
> >
> > This code is ported from mainline U-Boot arch/arm/mach-mvebu/cpu.c.
>
> Actually this is a work around because Linux doesn't do the necessary
> setup before making use of the hardware, right?
>
> If so, wouldn't it be better to teach Linux about the needed setup?
It would still be needed for barebox USB support.
> > +static void setup_usb_phys(void)
> > +{
> > + int dev;
> > +
> > + /*
> > + * USB PLL init
> > + */
> > +
> > + /* Setup PLL frequency */
> > + /* USB REF frequency = 25 MHz */
> > + clrsetbits_le32(MV_USB_PHY_PLL_REG(1), 0x3ff, 0x605);
> > +
> > + /* Power up PLL and PHY channel */
> > + setbits_le32(MV_USB_PHY_PLL_REG(2), BIT(9));
> > +
> > + /* Assert VCOCAL_START */
> > + setbits_le32(MV_USB_PHY_PLL_REG(1), BIT(21));
> > +
> > + mdelay(1);
> > +
> > + /*
> > + * USB PHY init (change from defaults) specific for 40nm
> > (78X30 78X60)
>
> I assume this comment also comes from U-Boot. Still I wonder if it
> makes sense to use the more common names instead of "78X30 78X60".
> That would be "Armada XP" but not "Armada 370", wouldn't it? (Armada
> 370 = 88F6710, 88F6707, and 88F6W11). If so, this code shouldn't be
> reached on Armada 370?
>
> Some other places in barebox suggest, that MV78xx0 is neither XP nor
> 370 (e.g.
> dts/Bindings/net/marvell-orion-mdio.txt
> dts/Bindings/arm/marvell/mvebu-system-controller.txt
> drivers/bus/Kconfig
> drivers/spi/mvebu_spi.c
> )
As far as I can see, there is no authoritative source on the naming. So
I'd like to avoid making changes without being sure that it is
actually correct.
> > + */
> > +
> > + for (dev = 0; dev < 3; dev++) {
> > + setbits_le32(MV_USB_X3_PHY_CHANNEL(dev, 3),
> > BIT(15));
> > +
> > + /* Assert REG_RCAL_START in channel REG 1 */
> > + setbits_le32(MV_USB_X3_PHY_CHANNEL(dev, 1),
> > BIT(12));
> > + udelay(40);
> > + clrbits_le32(MV_USB_X3_PHY_CHANNEL(dev, 1),
> > BIT(12));
> > + }
>
> Can this be made quicker using:
>
> for (dev = 0; dev < 3; dev++) {
> setbits_le32(MV_USB_X3_PHY_CHANNEL(dev, 3), BIT(15));
>
> /* Assert REG_RCAL_START in channel REG 1 */
> setbits_le32(MV_USB_X3_PHY_CHANNEL(dev, 1), BIT(12));
> }
>
> udelay(40;
>
> for (dev = 0; dev < 3; dev++)
> clrbits_le32(MV_USB_X3_PHY_CHANNEL(dev, 1), BIT(12));
>
> ?
Possibly. Nevertheless I'd prefer to keep the code identical to the one
from U-Boot, at least until someone is interested in refactoring this
on a larger scale.
> > +}
> > +
> > static int armada_370_xp_init_soc(void)
> > {
> > u32 reg;
> > @@ -109,6 +151,9 @@ static int armada_370_xp_init_soc(void)
> > reg = readl(ARMADA_XP_PUP_ENABLE);
> > reg |= GE0_PUP_EN | GE1_PUP_EN | LCD_PUP_EN |
> > NAND_PUP_EN | SPI_PUP_EN;
> > writel(reg, ARMADA_XP_PUP_ENABLE);
> > +
> > + /* Configure USB PLL and PHYs on AXP */
> > + setup_usb_phys();
> > }
> >
> > return 0;
> > diff --git a/arch/arm/mach-mvebu/include/mach/armada-370-xp-regs.h
> > b/arch/arm/mach-mvebu/include/mach/armada-370-xp-regs.h
> > index 1dad05317211..b972df151a74 100644
> > --- a/arch/arm/mach-mvebu/include/mach/armada-370-xp-regs.h
> > +++ b/arch/arm/mach-mvebu/include/mach/armada-370-xp-regs.h
> > @@ -72,4 +72,6 @@
> > (((port) % 4) * ARMADA_370_XP_PCIE_PORT_OFFSET))
> > #define PCIE_DEVICE_VENDOR_ID 0x000
> >
> > +#define ARMADA_370_XP_USB_BASE (ARMADA_370_XP_INT_R
> > EGS_BASE + 0x50000)
>
> Instead of hardcoding this address, this could be done depending on
> the device at the same address specified in the device tree.
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
