Re: [PATCH V10 net-next 03/10] net: hibmcge: Add mdio and hardware configuration supported in this module

[Christophe JAILLET]

Le 12/09/2024 à 04:51, Jijie Shao a écrit :
> Implements the C22 read and write PHY registers interfaces.
>
> Some hardware interfaces related to the PHY are also implemented
> in this patch.
>
> Signed-off-by: Jijie Shao <shaojijie@xxxxxxxxxx>

Hi,

a few nitpick, should there be a v11.

> +static void hbg_hw_init_transmit_control(struct hbg_priv *priv)
> +{
> +	u32 control = 0;
> +
> +	control |= FIELD_PREP(HBG_REG_TRANSMIT_CONTROL_AN_EN_B, HBG_STATUS_ENABLE);
> +	control |= FIELD_PREP(HBG_REG_TRANSMIT_CONTROL_CRC_ADD_B, HBG_STATUS_ENABLE);
> +	control |= FIELD_PREP(HBG_REG_TRANSMIT_CONTROL_PAD_EN_B, HBG_STATUS_ENABLE);
> +
> +	hbg_reg_write(priv, HBG_REG_TRANSMIT_CONTROL_ADDR, control);
> +}
> +
> +static void hbg_hw_init_rx_ctrl(struct hbg_priv *priv)
> +{
> +	u32 ctrl = 0;

Nitpick: the same kind of variable is called 'control' just above.
Same in the function name.

Having the same pattern everywhere would look more consistent.

> +
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_RX_GET_ADDR_MODE_B, HBG_STATUS_ENABLE);
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_TIME_INF_EN_B, HBG_STATUS_DISABLE);
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_RXBUF_1ST_SKIP_SIZE_M, HBG_RX_SKIP1);
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_RXBUF_1ST_SKIP_SIZE2_M, HBG_RX_SKIP2);
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_RX_ALIGN_NUM_M, NET_IP_ALIGN);
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_PORT_NUM, priv->dev_specs.mac_id);
> +
> +	hbg_reg_write(priv, HBG_REG_RX_CTRL_ADDR, ctrl);
> +}

...

> +static int hbg_mdio_init_hw(struct hbg_priv *priv)

Nitpick: This could return void.

> +{
> +	u32 freq = priv->dev_specs.mdio_frequency;
> +	struct hbg_mac *mac = &priv->mac;
> +	u32 cmd = 0;
> +
> +	cmd |= FIELD_PREP(HBG_REG_MDIO_COMMAND_ST_M, HBG_MDIO_C22_MODE);
> +	cmd |= FIELD_PREP(HBG_REG_MDIO_COMMAND_AUTO_SCAN_B, HBG_STATUS_DISABLE);
> +
> +	/* freq use two bits, which are stored in clk_sel and clk_sel_exp */
> +	cmd |= FIELD_PREP(HBG_REG_MDIO_COMMAND_CLK_SEL_B, freq & 0x1);
> +	cmd |= FIELD_PREP(HBG_REG_MDIO_COMMAND_CLK_SEL_EXP_B, (freq >> 1) & 0x1);
> +
> +	hbg_mdio_set_command(mac, cmd);
> +	return 0;
> +}

...

CJ