On 29-04-20, 17:39, Mathias Nyman wrote:
> 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 <vk...@kernel.org>
> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com>
> > Cc: Christian Lamparter <chunk...@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
Correct, this seems to be leftover, have cleaned it up now.
> > +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.
The registers are different, but that doesn't mean it can't be done. I
have added additional argument for rom and select the register to use
(ROM/RAM) and reused the rest of the fn
Thanks
--
~Vinod
