Re: [PATCH 05/15] net: rnpgbe: Add download firmware for n210 chip

[Yibo Dong]

On Fri, Jul 04, 2025 at 08:33:14PM +0200, Andrew Lunn wrote:
> >  static int init_firmware_for_n210(struct mucse_hw *hw)
> >  {
> > -	return 0;
> > +	char *filename = "n210_driver_update.bin";
> > +	const struct firmware *fw;
> > +	struct pci_dev *pdev = hw->pdev;
> > +	int rc = 0;
> > +	int err = 0;
> > +	struct mucse *mucse = (struct mucse *)hw->back;
> > +
> > +	rc = request_firmware(&fw, filename, &pdev->dev);
> > +
> > +	if (rc != 0) {
> > +		dev_err(&pdev->dev, "requesting firmware file failed\n");
> > +		return rc;
> > +	}
> > +
> > +	if (rnpgbe_check_fw_from_flash(hw, fw->data)) {
> > +		dev_info(&pdev->dev, "firmware type error\n");
> 
> Why dev_info()? If this is an error then you should use dev_err().
> 
Yes, it should be dev_err().
> > +	dev_info(&pdev->dev, "init firmware successfully.");
> > +	dev_info(&pdev->dev, "Please reboot.");
> 
> Don't spam the lock with status messages.
> 
> Reboot? Humm, maybe this should be devlink flash command.
> 
> request_firmware() is normally used for download into SRAM which is
> then used immediately. If you need to reboot the machine, devlink is
> more appropriate.
> 
Yes, this is used to download flash to the chip, and then reboot to run.
I will change it to devlink.
> > +static inline void mucse_sfc_command(u8 __iomem *hw_addr, u32 cmd)
> > +{
> > +	iowrite32(cmd, (hw_addr + 0x8));
> > +	iowrite32(1, (hw_addr + 0x0));
> > +	while (ioread32(hw_addr) != 0)
> > +		;
> 
> 
> Never do endless loops waiting for hardware. It might never give what
> you want, and there is no escape.
> 
Got it, I will update this.
> > +static int32_t mucse_sfc_flash_wait_idle(u8 __iomem *hw_addr)
> > +{
> > +	int time = 0;
> > +	int ret = HAL_OK;
> > +
> > +	iowrite32(CMD_CYCLE(8), (hw_addr + 0x10));
> > +	iowrite32(RD_DATA_CYCLE(8), (hw_addr + 0x14));
> > +
> > +	while (1) {
> > +		mucse_sfc_command(hw_addr, CMD_READ_STATUS);
> > +		if ((ioread32(hw_addr + 0x4) & 0x1) == 0)
> > +			break;
> > +		time++;
> > +		if (time > 1000)
> > +			ret = HAL_FAIL;
> > +	}
> 
> iopoll.h 
> 
Got it, I will use method in iopoll.h.
> > +static int mucse_sfc_flash_erase_sector(u8 __iomem *hw_addr,
> > +					u32 address)
> > +{
> > +	int ret = HAL_OK;
> > +
> > +	if (address >= RSP_FLASH_HIGH_16M_OFFSET)
> > +		return HAL_EINVAL;
> 
> Use linux error codes, EINVAL.
> 
Got it.
> > +
> > +	if (address % 4096)
> > +		return HAL_EINVAL;
> 
> EINVAL
> 
Got it.
> > +
> > +	mucse_sfc_flash_write_enable(hw_addr);
> > +
> > +	iowrite32((CMD_CYCLE(8) | ADDR_CYCLE(24)), (hw_addr + 0x10));
> > +	iowrite32((RD_DATA_CYCLE(0) | WR_DATA_CYCLE(0)), (hw_addr + 0x14));
> > +	iowrite32(SFCADDR(address), (hw_addr + 0xc));
> > +	mucse_sfc_command(hw_addr, CMD_SECTOR_ERASE);
> > +	if (mucse_sfc_flash_wait_idle(hw_addr)) {
> > +		ret = HAL_FAIL;
> > +		goto failed;
> 
> mucse_sfc_flash_wait_idle() should return -ETIMEDOUT, so return that.
> 
> 	Andrew
> 
Got it, I will return -ETIMEDOUT.
Thanks for your feedback.