On 07.03.2018 22:52, Marek Behún wrote: > When USB3 is on comphy lane 2 on the Armada 37xx, the registers > have to be accessed indirectly via SATA indirect access. > > Signed-off-by: Marek Behun <marek.be...@nic.cz>
Could you please describe, which problem this patch fixes? Is this on an already available A37xx board or a new one? Please find more comments below... > --- > drivers/phy/marvell/comphy_a3700.c | 111 > +++++++++++++++++++++++++------------ > drivers/phy/marvell/comphy_a3700.h | 1 + > 2 files changed, 78 insertions(+), 34 deletions(-) > > diff --git a/drivers/phy/marvell/comphy_a3700.c > b/drivers/phy/marvell/comphy_a3700.c > index 81d24a5b61..b5f2013bbb 100644 > --- a/drivers/phy/marvell/comphy_a3700.c > +++ b/drivers/phy/marvell/comphy_a3700.c > @@ -133,7 +133,7 @@ static u32 comphy_poll_reg(void *addr, u32 val, u32 mask, > u8 op_type) > */ > static int comphy_pcie_power_up(u32 speed, u32 invert) > { > - int ret; > + int ret; > > debug_enter(); > > @@ -300,17 +300,50 @@ static int comphy_sata_power_up(void) > return ret; > } > > +/* > + * usb3_reg_set16_indirect > + * > + * return: void > + */ > +static void usb3_reg_set16_indirect(u32 reg, u16 data, u16 mask) > +{ > + reg_set_indirect(USB3PHY_LANE2_REG_BASE_OFFSET + reg, data, mask); > +} > + > +/* > + * usb3_reg_set16_direct > + * > + * return: void > + */ > +static void usb3_reg_set16_direct(u32 reg, u16 data, u16 mask) > +{ > + reg_set16(PHY_ADDR(USB3, reg), data, mask); > +} > + > /* > * comphy_usb3_power_up > * > * return: 1 if PLL locked (OK), 0 otherwise (FAIL) > */ > -static int comphy_usb3_power_up(u32 type, u32 speed, u32 invert) > +static int comphy_usb3_power_up(u32 lane, u32 type, u32 speed, u32 invert) > { > - int ret; > + int ret; > + void (*usb3_reg_set16)(u32, u16, u16); Wouldn't it be cleared / easier to read, if you would encapsulate the Hmmm. This does not seem to be needed here, right? static void usb3_reg_set16_direct(u32 reg, u16 data, u16 mask, int lane) { if (lane reg_set16(PHY_ADDR(USB3, reg), data, mask); } > > debug_enter(); > > + /* > + * When Lane 2 PHY is for USB3, access the PHY registers > + * through indirect Address and Data registers INDIR_ACC_PHY_ADDR > + * (RD00E0178h [31:0]) and INDIR_ACC_PHY_DATA (RD00E017Ch [31:0]) > + * within the SATA Host Controller registers, Lane 2 base register > + * offset is 0x200 > + */ > + if (lane == 2) > + usb3_reg_set16 = usb3_reg_set16_indirect; > + else > + usb3_reg_set16 = usb3_reg_set16_direct; > + Wouldn't it be cleared / easier to read, if you would encapsulate the lane -> function usage in the itself? Something like this: static void usb3_reg_set16(u32 reg, u16 data, u16 mask, int lane) { if (lane == 2) reg_set_indirect(USB3PHY_LANE2_REG_BASE_OFFSET + reg, data, mask); else reg_set16(PHY_ADDR(USB3, reg), data, mask); } What do you think? Thanks, Stefan _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot