Re: [PATCH net-next v13 4/6] net: dsa: mv88e6xxxx: Add RMU functionality.

[Andrew Lunn <andrew@lunn.ch>]

On Fri, Sep 16, 2022 at 02:18:15PM +0200, Mattias Forsblad wrote:
> The Marvell SOHO switches supports a secondary control
> channel for accessing data in the switch. Special crafted
> ethernet frames can access functions in the switch.
> These frames is handled by the Remote Management Unit (RMU)
> in the switch. Accessing data structures is specially
> efficient and lessens the access contention on the MDIO
> bus.

Thanks for continually reworking this. It is nearly there now.

I said earlier, you want lots of small patches which are easy to
review. I would separate the MIB stuff into a separate patch.  This
patch can provide the basic infrastructure to send a request to the
RMU, and do basic validation of the reply. Then have patches which
make use of that. One such patch is getting the ID. Another patch is
getting statistics.

> +int mv88e6xxx_state_get_stats_rmu(struct mv88e6xxx_chip *chip, int port, uint64_t *data)
> +{
> +	const u64 *stats = chip->ports[port].rmu_raw_stats;

...

> +static void mv88e6xxx_rmu_stats_get(struct mv88e6xxx_chip *chip, int port, uint64_t *data)
> +{
> +	u16 req[4] = { MV88E6XXX_RMU_REQ_FORMAT_SOHO,
> +		       MV88E6XXX_RMU_REQ_PAD,
> +		       MV88E6XXX_RMU_REQ_CODE_DUMP_MIB,
> +		       MV88E6XXX_RMU_REQ_DATA};
> +	struct mv88e6xxx_dump_mib_resp resp;
> +	struct mv88e6xxx_port *p;
> +	u8 resp_port;
> +	int resp_len;
> +	int num_mibs;
> +	int ret;
> +	int i;

...

> +	/* Copy MIB array for further processing according to chip type */
> +	num_mibs = (resp_len - offsetof(struct mv88e6xxx_dump_mib_resp, mib)) / sizeof(__be32);
> +	for (i = 0; i < num_mibs; i++)
> +		p->rmu_raw_stats[i] = be32_to_cpu(resp.mib[i]);
> +
> +	/* Update MIB for port */
> +	mv88e6xxx_state_get_stats_rmu(chip, port, data);

Why copy the stats into p->rmu_raw_stats[i] when you can just pass
resp to mv88e6xxx_state_get_stats_rmu().

This is left over from the way you used the old stats function to
decode the values. But now you have a dedicated function, you can
avoid this copy.

      Andrew