Re: [PATCH net-next v3 04/12] net: ethernet: oa_tc6: implement software reset

[Andrew Lunn <andrew@xxxxxxx>]

> +/* Status Register #0 */
> +#define OA_TC6_REG_STATUS0			0x0008
> +#define STATUS0_RESETC				BIT(6)	/* Reset Complete */
> +
>  /* Control command header */
>  #define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL	BIT(31)
>  #define OA_TC6_CTRL_HEADER_WRITE		BIT(29)
> @@ -24,6 +34,8 @@
>  						(OA_TC6_CTRL_MAX_REGISTERS *\
>  						OA_TC6_CTRL_REG_VALUE_SIZE) +\
>  						OA_TC6_CTRL_IGNORED_SIZE)
> +#define STATUS0_RESETC_POLL_DELAY		5
> +#define STATUS0_RESETC_POLL_TIMEOUT		100
>  
>  /* Internal structure for MAC-PHY drivers */
>  struct oa_tc6 {
> @@ -279,6 +291,39 @@ int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value)
>  }
>  EXPORT_SYMBOL_GPL(oa_tc6_write_register);
>  
> +static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6)
> +{
> +	u32 regval;
> +	int ret;
> +
> +	ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, &regval);
> +	if (ret)
> +		return 0;
> +
> +	return regval;

The function name does not really fit what the function does. The
function returns OA_TC6_REG_STATUS0. I assume it has more bits in it
than just STATUS0_RESETC. So either this function should be called
oa_tc6_read_status0, or you should mask regval with STATUS0_RESETC, so
that it does actually return the sw reset status.

> +static int oa_tc6_sw_reset_macphy(struct oa_tc6 *tc6)
> +{
> +	u32 regval = RESET_SWRESET;
> +	int ret;
> +
> +	ret = oa_tc6_write_register(tc6, OA_TC6_REG_RESET, regval);
> +	if (ret)
> +		return ret;
> +
> +	/* Poll for soft reset complete for every 5us until 100us timeout */

Is this 100us defined in the standard? It is pretty short. If it is
not part of the standard, maybe set it to something much bigger?

Also, polling every 5us is pretty quick. I doubt most systems can even
sleep that short a time. So maybe 1ms between polls, and 1 second
before -ETIMEDOUT?

> +	ret = readx_poll_timeout(oa_tc6_read_sw_reset_status, tc6, regval,
> +				 regval & STATUS0_RESETC,
> +				 STATUS0_RESETC_POLL_DELAY,
> +				 STATUS0_RESETC_POLL_TIMEOUT);
> +	if (ret)
> +		return -ENODEV;
> +
> +	/* Clear the reset complete status */
> +	return oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, regval);

	Andrew