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

[Stefan Roese]

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 at 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