On Tue, Sep 09, 2025 at 03:37:19PM +0100, Vadim Fedorenko wrote:
> On 09/09/2025 13:09, Dong Yibo wrote:
> > Complete the network device (netdev) registration flow for Mucse Gbe
> > Ethernet chips, including:
> > 1. Hardware state initialization:
> > - Send powerup notification to firmware (via echo_fw_status)
> > - Sync with firmware
> > - Reset hardware
> > 2. MAC address handling:
> > - Retrieve permanent MAC from firmware (via mucse_mbx_get_macaddr)
> > - Fallback to random valid MAC (eth_random_addr) if not valid mac
> > from Fw
> >
> > Signed-off-by: Dong Yibo <dong100@xxxxxxxxx>
>
> [...]
>
> > +struct mucse_hw;
>
> why do you need this forward declaration ...> +
> > +struct mucse_hw_operations {
> > + int (*reset_hw)(struct mucse_hw *hw);
> > + int (*get_perm_mac)(struct mucse_hw *hw);
> > + int (*mbx_send_notify)(struct mucse_hw *hw, bool enable, int mode);
> > +};
> > +
> > +enum {
> > + mucse_fw_powerup,
> > +};
> > +
> > struct mucse_hw {
> > void __iomem *hw_addr;
> > + struct pci_dev *pdev;
> > + const struct mucse_hw_operations *ops;
> > + struct mucse_dma_info dma;
> > struct mucse_mbx_info mbx;
> > + int port;
> > + u8 perm_addr[ETH_ALEN];
> > u8 pfvfnum;
> > };
>
> ... if you can simply move mucse_hw_operations down here?
>
Got it, I will update it. At the beginning I defined
'struct mucse_hw_operations ops' in 'struct mucse_hw'. I missed to
consider this when it is changed to 'struct mucse_hw_operations *ops'.
> > @@ -54,4 +76,7 @@ int rnpgbe_init_hw(struct mucse_hw *hw, int board_type);
> > #define PCI_DEVICE_ID_N500_DUAL_PORT 0x8318
> > #define PCI_DEVICE_ID_N210 0x8208
> > #define PCI_DEVICE_ID_N210L 0x820a
> > +
> > +#define rnpgbe_dma_wr32(dma, reg, val) \
> > + writel((val), (dma)->dma_base_addr + (reg))
>
> [...]
>
> > @@ -48,8 +127,14 @@ static void rnpgbe_init_n210(struct mucse_hw *hw)
> > **/
> > int rnpgbe_init_hw(struct mucse_hw *hw, int board_type)
> > {
> > + struct mucse_dma_info *dma = &hw->dma;
> > struct mucse_mbx_info *mbx = &hw->mbx;
> > + hw->ops = &rnpgbe_hw_ops;
> > + hw->port = 0;
> > +
> > + dma->dma_base_addr = hw->hw_addr;
>
> not quite sure why do you need additional structure just to store the
> value that already exists in mucse_hw?
>
The original meaning was that all submodules have their own base, such
as dma, mbx... Just that 'dma_base_addr' is 'offset 0 from hw_addr'.
And maybe it is not good to do it like this?
...
#define MUCSE_GBE_DMA_BASE 0
dma->dma_base_addr = hw->hw_addr + MUCSE_GBE_DMA_BASE;
...
If so, I will remove dma_base_addr, and use hw_addr instead.
> > +
> > mbx->pf2fw_mbx_ctrl = MUCSE_GBE_PFFW_MBX_CTRL_OFFSET;
> > mbx->fwpf_mbx_mask = MUCSE_GBE_FWPF_MBX_MASK_OFFSET;
>
Thanks for your feedback.
