Re: [U-Boot] [PATCH v1 07/19] phy: marvell: a3700: Access USB3 register indirectly on lane 2

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.behun@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