Re: [PATCH v10 4/5] usb: renesas-xhci: Add ROM loader for uPD720201

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 24.4.2020 13.14, Vinod Koul wrote:
> uPD720201 supports ROM and allows software to program the ROM and boot
> from it. Add support for detecting if ROM is present, if so load the ROM
> if not programmed earlier.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: Christian Lamparter <chunkeey@googlemail.com>
> ---
>  drivers/usb/host/xhci-pci-renesas.c | 342 +++++++++++++++++++++++++++-
>  1 file changed, 341 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-pci-renesas.c b/drivers/usb/host/xhci-pci-renesas.c
> index 402e86912c9f..6bb537999754 100644
> --- a/drivers/usb/host/xhci-pci-renesas.c
> +++ b/drivers/usb/host/xhci-pci-renesas.c
> @@ -50,6 +50,22 @@
>  #define RENESAS_RETRY	10000
>  #define RENESAS_DELAY	10
>  
> +#define ROM_VALID_01 0x2013
> +#define ROM_VALID_02 0x2026
> +
> +static int renesas_verify_fw_version(struct pci_dev *pdev, u32 version)
> +{
> +	switch (version) {
> +	case ROM_VALID_01:
> +	case ROM_VALID_02:
> +		return 0;
> +	default:
> +		dev_err(&pdev->dev, "FW has invalid version :%d\n", version);
> +		return 1;
> +	}
> +	return -EINVAL;

This never returns -EINVAL
Maybe just get rid of the default case and print
the error message before returning

> +}
> +
>  static int renesas_fw_download_image(struct pci_dev *dev,
>  				     const u32 *fw,
>  				     size_t step)
> @@ -144,10 +160,84 @@ static int renesas_fw_verify(const void *fw_data,
>  	return 0;
>  }
>  
> -static int renesas_fw_check_running(struct pci_dev *pdev)
> +static bool renesas_check_rom(struct pci_dev *pdev)
>  {
> +	u16 rom_status;
> +	int retval;
> +
> +	/* Check if external ROM exists */
> +	retval = pci_read_config_word(pdev, RENESAS_ROM_STATUS, &rom_status);
> +	if (retval)
> +		return false;
> +
> +	rom_status &= RENESAS_ROM_STATUS_ROM_EXISTS;
> +	if (rom_status) {
> +		dev_dbg(&pdev->dev, "External ROM exists\n");
> +		return true; /* External ROM exists */
> +	}
> +
> +	return false;
> +}
> +
> +static int renesas_check_rom_state(struct pci_dev *pdev)
> +{
> +	u16 rom_state;
> +	u32 version;
>  	int err;
> +
> +	/* check FW version */
> +	err = pci_read_config_dword(pdev, RENESAS_FW_VERSION, &version);
> +	if (err)
> +		return pcibios_err_to_errno(err);
> +
> +	version &= RENESAS_FW_VERSION_FIELD;
> +	version = version >> RENESAS_FW_VERSION_OFFSET;
> +
> +	err = renesas_verify_fw_version(pdev, version);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * Test if ROM is present and loaded, if so we can skip everything
> +	 */
> +	err = pci_read_config_word(pdev, RENESAS_ROM_STATUS, &rom_state);
> +	if (err)
> +		return pcibios_err_to_errno(err);
> +
> +	if (rom_state & BIT(15)) {
> +		/* ROM exists */
> +		dev_dbg(&pdev->dev, "ROM exists\n");
> +
> +		/* Check the "Result Code" Bits (6:4) and act accordingly */
> +		switch (rom_state & RENESAS_ROM_STATUS_RESULT) {
> +		case RENESAS_ROM_STATUS_SUCCESS:
> +			return 0;
> +
> +		case RENESAS_ROM_STATUS_NO_RESULT: /* No result yet */
> +			return 0;
> +
> +		case RENESAS_ROM_STATUS_ERROR: /* Error State */
> +		default: /* All other states are marked as "Reserved states" */
> +			dev_err(&pdev->dev, "Invalid ROM..");
> +			break;
> +		}
> +	}
> +
> +	return -EIO;
> +}
> +
> +static int renesas_fw_check_running(struct pci_dev *pdev)
> +{
>  	u8 fw_state;
> +	int err;
> +
> +	/* Check if device has ROM and loaded, if so skip everything */
> +	err = renesas_check_rom(pdev);
> +	if (err) { /* we have rom */
> +		err = renesas_check_rom_state(pdev);
> +		if (!err)
> +			return err;
> +	}
>  
>  	/*
>  	 * Test if the device is actually needing the firmware. As most
> @@ -303,11 +393,261 @@ static int renesas_fw_download(struct pci_dev *pdev,
>  	return 0;
>  }
>  
> +static void renesas_rom_erase(struct pci_dev *pdev)
> +{
> +	int retval, i;
> +	u8 status;
> +
> +	dev_dbg(&pdev->dev, "Performing ROM Erase...\n");
> +	retval = pci_write_config_dword(pdev, RENESAS_DATA0,
> +					RENESAS_ROM_ERASE_MAGIC);
> +	if (retval) {
> +		dev_err(&pdev->dev, "ROM erase, magic word write failed: %d\n",
> +			pcibios_err_to_errno(retval));
> +		return;
> +	}
> +
> +	retval = pci_read_config_byte(pdev, RENESAS_ROM_STATUS, &status);
> +	if (retval) {
> +		dev_err(&pdev->dev, "ROM status read failed: %d\n",
> +			pcibios_err_to_errno(retval));
> +		return;
> +	}
> +	status |= RENESAS_ROM_STATUS_ERASE;
> +	retval = pci_write_config_byte(pdev, RENESAS_ROM_STATUS, status);
> +	if (retval) {
> +		dev_err(&pdev->dev, "ROM erase set word write failed\n");
> +		return;
> +	}
> +
> +	/* sleep a bit while ROM is erased */
> +	msleep(20);
> +
> +	for (i = 0; i < RENESAS_RETRY; i++) {
> +		retval = pci_read_config_byte(pdev, RENESAS_ROM_STATUS,
> +					      &status);
> +		status &= RENESAS_ROM_STATUS_ERASE;
> +		if (!status)
> +			break;
> +
> +		mdelay(RENESAS_DELAY);
> +	}
> +
> +	if (i == RENESAS_RETRY)
> +		dev_dbg(&pdev->dev, "Chip erase timedout: %x\n", status);
> +
> +	dev_dbg(&pdev->dev, "ROM Erase... Done success\n");
> +}
> +
> +static bool renesas_download_rom(struct pci_dev *pdev,
> +				 const u32 *fw, size_t step)
> +{
> +	bool data0_or_data1;
> +	u8 fw_status;
> +	size_t i;
> +	int err;
> +
> +	/*
> +	 * The hardware does alternate between two 32-bit pages.
> +	 * (This is because each row of the firmware is 8 bytes).
> +	 *
> +	 * for even steps we use DATA0, for odd steps DATA1.
> +	 */
> +	data0_or_data1 = (step & 1) == 1;
> +
> +	/* Read "Set DATAX" and confirm it is cleared. */
> +	for (i = 0; i < RENESAS_RETRY; i++) {
> +		err = pci_read_config_byte(pdev, RENESAS_ROM_STATUS_MSB,
> +					   &fw_status);
> +		if (err) {
> +			dev_err(&pdev->dev, "Read ROM Status failed: %d\n",
> +				pcibios_err_to_errno(err));
> +			return false;
> +		}
> +		if (!(fw_status & BIT(data0_or_data1)))
> +			break;
> +
> +		udelay(RENESAS_DELAY);
> +	}
> +	if (i == RENESAS_RETRY) {
> +		dev_err(&pdev->dev, "Timeout for Set DATAX step: %zd\n", step);
> +		return false;
> +	}
> +
> +	/*
> +	 * Write FW data to "DATAX".
> +	 * "LSB is left" => force little endian
> +	 */
> +	err = pci_write_config_dword(pdev, data0_or_data1 ?
> +				     RENESAS_DATA1 : RENESAS_DATA0,
> +				     (__force u32)cpu_to_le32(fw[step]));
> +	if (err) {
> +		dev_err(&pdev->dev, "Write to DATAX failed: %d\n",
> +			pcibios_err_to_errno(err));
> +		return false;
> +	}
> +
> +	udelay(100);
> +
> +	/* Set "Set DATAX". */
> +	err = pci_write_config_byte(pdev, RENESAS_ROM_STATUS_MSB,
> +				    BIT(data0_or_data1));
> +	if (err) {
> +		dev_err(&pdev->dev, "Write config for DATAX failed: %d\n",
> +			pcibios_err_to_errno(err));
> +		return false;
> +	}
> +
> +	return true;
> +}

The above function is almost identical to renesas_fw_download_image() added in a 
previous patch.
To avoid code duplication I'm sure one function that handles both cases would be possible. 

-Mathias