Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Vinod Koul <vkoul@kernel.org>
To: Christian Lamparter <chunkeey@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Mathias Nyman <mathias.nyman@intel.com>,
	linux-arm-msm@vger.kernel.org,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Christian Lamparter <chunkeey@googlemail.com>,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Subject: Re: [PATCH 1/5] usb: xhci: add firmware loader for uPD720201 and uPD720202 w/o ROM
Date: Fri, 21 Jun 2019 10:12:42 +0530
Message-ID: <20190621044242.GQ2962@vkoul-mobl> (raw)
In-Reply-To: <2465888.R7Jb3LzrEU@debian64>

On 20-06-19, 21:12, Christian Lamparter wrote:
> On Thursday, June 20, 2019 7:03:58 PM CEST Vinod Koul wrote:
> > On 20-06-19, 14:19, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 20, 2019 at 03:51:50PM +0530, Vinod Koul wrote:
> > > > From: Christian Lamparter <chunkeey@googlemail.com>
> > > > 
> > > > This patch adds a firmware loader for the uPD720201K8-711-BAC-A
> > > > and uPD720202K8-711-BAA-A variant. Both of these chips are listed
> > > > in Renesas' R19UH0078EJ0500 Rev.5.00 "User's Manual: Hardware" as
> > > > devices which need the firmware loader on page 2 in order to
> > > > work as they "do not support the External ROM".
> > > > 
> > > > The "Firmware Download Sequence" is describe in chapter
> > > > "7.1 FW Download Interface" R19UH0078EJ0500 Rev.5.00 page 131.
> > > > 
> > > > The firmware "K2013080.mem" is available from a USB3.0 Host to
> > > > PCIe Adapter (PP2U-E card) "Firmware download" archive. An
> > > > alternative version can be sourced from Netgear's WNDR4700 GPL
> > > > archives.
> > > > 
> > > > The release notes of the PP2U-E's "Firmware Download" ver 2.0.1.3
> > > > (2012-06-15) state that the firmware is for the following devices:
> > > >  - uPD720201 ES 2.0 sample whose revision ID is 2.
> > > >  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
> > > >  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
> > > > 
> > > > If someone from Renesas is listening: It would be great, if these
> > > > firmwares could be added to linux-firmware.git.
> > > 
> > > That paragraph does not need to be in the changelog :)
> > 
> > Sure will drop :)
> 
> ... those this mean that there is a firmware now? Do you have a link to it?

Unfortunately it is not public yet!

> > > >  #include <linux/slab.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/acpi.h>
> > > > +#include <linux/firmware.h>
> > > > +#include <asm/unaligned.h>
> > > 
> > > asm/ in a driver?  Are you sure???
> > 
> > Not sure :D, will check and remove
> 
> I think, as long as there is a "get_unaligned_le16" defined somewhere
> it should be fine.
> 
> This was a loong ago, the loader was developped on a PowerPC 464, but
> from what I remember it was checkpatch that didn't like the "unaligned"
> poking around in the firmware below.

It seems we have this in include/linux/unaligned/access_ok.h, so I will
add that instead

> > > > +static int renesas_fw_download_image(struct pci_dev *dev,
> > > > +				     const u32 *fw,
> > > > +				     size_t step)
> > > > +{
> > > > +	size_t i;
> > > > +	int err;
> > > > +	u8 fw_status;
> > > > +	bool data0_or_data1;
> > > > +
> > > > +	/*
> > > > +	 * 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;
> > > > +
> > > > +	/* step+1. Read "Set DATAX" and confirm it is cleared. */
> > > > +	for (i = 0; i < 10000; i++) {
> > > > +		err = pci_read_config_byte(dev, 0xF5, &fw_status);
> > > > +		if (err)
> > > > +			return pcibios_err_to_errno(err);
> > > > +		if (!(fw_status & BIT(data0_or_data1)))
> > > > +			break;
> > > > +
> > > > +		udelay(1);
> > > > +	}
> > > > +	if (i == 10000)
> > > > +		return -ETIMEDOUT;
> > > > +
> > > > +	/*
> > > > +	 * step+2. Write FW data to "DATAX".
> > > > +	 * "LSB is left" => force little endian
> > > > +	 */
> > > > +	err = pci_write_config_dword(dev, data0_or_data1 ? 0xFC : 0xF8,
> > > > +				     (__force u32) cpu_to_le32(fw[step]));
> > > > +	if (err)
> > > > +		return pcibios_err_to_errno(err);
> > > > +
> > > > +	udelay(100);
> > > > +
> > > > +	/* step+3. Set "Set DATAX". */
> > > > +	err = pci_write_config_byte(dev, 0xF5, BIT(data0_or_data1));
> > > > +	if (err)
> > > > +		return pcibios_err_to_errno(err);
> > > > +
> > > 
> > > Shouldn't you just do a read after the write to be sure the write
> > > actually went out on the wire?  Then you shouldn't have to do the
> > > udelay, right?
> > 
> > Well I am not sure that is how it works. The register is a DATA register
> > on the controller. We are writing to the memory of the controller here
> > and after writing DATA0 and DATA1 we check the Set DATA0 & Set DATA1
> > bits and write subsequenly only when controller is ready to accept more
> > data.
> > 
> > I do recall at least for ROM load (writing to NOR flash attached to
> > controller), we need to wait considerably more before the SetData0/1 was
> > set and ready for subsequent write
> 
> OffTopic: There's some leeway here. From what I remember you could just push
> the data through DATA0 and cut down on the logic. But this was slower than
> using both DATA0 and DATA1.
> 
> The udelay was placed because I vaguely remember that polling SET DATA0
> over and over slowed down the firmware download.
> So the intention was to have the 100µs as a baseline and then we don't
> slow down and waste more cycles in "step+1".

Yes I have seen that as well, lesser polling helps. I will reduce cycles
and add delay.

Btw would it be possible for you to test the series?

Thanks for quick reply, will post v2 shortly

-- 
~Vinod

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-20 10:21 [PATCH 0/5] usb: xhci: Add support for Renesas USB controllers Vinod Koul
2019-06-20 10:21 ` [PATCH 1/5] usb: xhci: add firmware loader for uPD720201 and uPD720202 w/o ROM Vinod Koul
2019-06-20 12:19   ` Greg Kroah-Hartman
2019-06-20 17:03     ` Vinod Koul
2019-06-20 19:12       ` Christian Lamparter
2019-06-21  4:42         ` Vinod Koul [this message]
2019-06-20 10:21 ` [PATCH 2/5] usb: xhci: handle " Vinod Koul
2019-06-20 10:21 ` [PATCH 3/5] usb: xhci: Use register defined and field names Vinod Koul
2019-06-20 10:21 ` [PATCH 4/5] usb: xhci: Add ROM loader for uPD720201 Vinod Koul
2019-06-20 10:21 ` [PATCH 5/5] usb: xhci: allow multiple firmware versions Vinod Koul
2019-06-20 12:20   ` Greg Kroah-Hartman
2019-06-20 17:07     ` Vinod Koul

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190621044242.GQ2962@vkoul-mobl \
    --to=vkoul@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=chunkeey@gmail.com \
    --cc=chunkeey@googlemail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@intel.com \
    --cc=yoshihiro.shimoda.uh@renesas.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox