[PATCH v1 6/6] net: mv88e61xx: Reset switch PHYs when bootstrapped to !NO_CPU

[Marek Vasut]

On 6/1/23 14:13, Lukasz Majewski wrote:
> Hi Marek,
> 
>> On 6/1/23 12:00, Lukasz Majewski wrote:
>>> Some devices, when configured in bootstrap to 'no cpu' mode require
>>> PHY manual reset to get them operational and responding to reading
>>> their ID registers.
>>>
>>> Without this step - the PHYLIB probing will fail.
>>>
>>> In more details - the bootstrap configuration from switch must be
>>> read. The value of CONFIG Data1 (0x71) of Scratch and Misc register
>>> is read to check if 'no_cpu' and 'addr4' bits were set.
>>>
>>> Signed-off-by: Lukasz Majewski <lukma at denx.de>
>>> Reviewed-by: Ramon Fried <rfried.dev at gmail.com>
>>>
>>> ---
>>>
>>>    drivers/net/phy/mv88e61xx.c | 63
>>> +++++++++++++++++++++++++++++++++++-- 1 file changed, 61
>>> insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/mv88e61xx.c
>>> b/drivers/net/phy/mv88e61xx.c index 69a87bead469..cf8f5e833e82
>>> 100644 --- a/drivers/net/phy/mv88e61xx.c
>>> +++ b/drivers/net/phy/mv88e61xx.c
>>> @@ -194,6 +194,17 @@ struct mv88e61xx_phy_priv {
>>>    	u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */
>>>    	u8 phy_ctrl1_en_det_ctrl;  /* 'EDet' control value */
>>>    	u8 direct_access;          /* Access switch device
>>> directly */
>>> +	/*
>>> +	 * Bootstrap configuration:
>>> +	 *
>>> +	 * If addr4 = 1 device is accessible from 0x10 address on
>>> MDIO bus.
>>> +	 */
>>> +	u8 addr4;
>>> +	/*
>>> +	 * If no_cpu = 1 switch is automatically setup, otherwise
>>> PHY reset is
>>> +	 * required from CPU for normal operation.
>>> +	 */
>>> +	u8 no_cpu;
>>>    };
>>>    
>>>    static inline int smi_cmd(int cmd, int addr, int reg)
>>> @@ -1218,6 +1229,33 @@ U_BOOT_PHY_DRIVER(mv88e6071) = {
>>>    	.shutdown = &genphy_shutdown,
>>>    };
>>>    
>>> +static int mv88e61xx_read_bootstrap(struct phy_device *phydev)
>>> +{
>>> +	struct mv88e61xx_phy_priv *priv = phydev->priv;
>>> +	struct mii_dev *mdio_bus = priv->mdio_bus;
>>> +	int val;
>>> +
>>> +	/* mv88e6020 - ID = 0x0200 (REG 3 on non PHY port) */
>>> +	if (priv->id == PORT_SWITCH_ID_6020) {
>>> +		/* Prepare to read scratch and misc register */
>>> +		mdio_bus->write(mdio_bus, priv->global2, 0,
>>> +				0x1a /*MV_SCRATCH_MISC*/,
>>> +				(0x71 /*MV_CONFIG_DATA1*/ << 8));
>>
>> Introduce macros for these magic values.
> 
> Frankly speaking, this is more readable than macros as it is in sync
> with vendor's documentation.

Sorry, I am not buying this argument. Random constant 0x123 is NOT more 
readable than the standard that the rest of the U-Boot code base uses 
which is:

#define MV_SCRATCH_MISC 0x1a

and then call(MV_SCRATCH_MISC...) .

Hard-coding random magic register offsets into the code is just sloppy 
and leads to duplication. The mv88e61xx driver does not do it either, so 
this patch is violating its coding style, see the top 200 or so lines of 
the driver. Do not do it.

To make it clear, until this is fixed, NAK .

> In other words - it is easier to find this information in documentation
> when presented like above.

No