From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Wed, 21 Oct 2015 14:46:12 -0600
Subject: [U-Boot] [PATCH 8/8] dm: tegra: pci: Convert tegra boards to
 driver model for PCI
In-Reply-To: <1445104205-4079-9-git-send-email-sjg@chromium.org>
References: <1445104205-4079-1-git-send-email-sjg@chromium.org>
 <1445104205-4079-9-git-send-email-sjg@chromium.org>
Message-ID: <5627F994.5030802@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 10/17/2015 11:50 AM, Simon Glass wrote:
> Adjust the Tegra PCI driver to support driver model and move all boards over
> at the same time. This can make use of some generic driver model code, such
> as the range decoding logic.

FYI, this patch has quite a few conflicts with my series to add Tegra210 
support. See the git branch I mentioned earlier for a quick-n-dirty 
resolution to the conflicts, and subsequent fixups.

> diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
> index a5b7e0d..aff4900 100644
> --- a/arch/arm/mach-tegra/Kconfig
> +++ b/arch/arm/mach-tegra/Kconfig
> @@ -12,6 +12,7 @@ config TEGRA_ARMV7_COMMON
>   	select DM_I2C
>   	select DM_SPI
>   	select DM_GPIO
> +	select DM_PCI

I wonder if we shouldn't create the TEGRA_COMMON config variable now, 
since many of the options are common to both TEGRA_ARMV7_COMMON and 
TEGRA210. Perhaps also TEGRA_ARMV8_COMMON too, although currently 
there's only support for a single ARMV8 Tegra chip in U-Boot.

> diff --git a/drivers/pci/pci_tegra.c b/drivers/pci/pci_tegra.c

> -#define DEBUG

I actually find the debug prints useful, but yes I suppose debug should 
be turned off!

> -static int tegra_pcie_read_conf(struct pci_controller *hose, pci_dev_t bdf,
> -				int where, u32 *value)
> +static int pci_tegra_read_config(struct udevice *bus, pci_dev_t bdf,
> +				 uint offset, ulong *valuep,
> +				 enum pci_size_t size)
>   {
> -	struct tegra_pcie *pcie = to_tegra_pcie(hose);
> -	unsigned long address;
> +	struct tegra_pcie *pcie = dev_get_priv(bus);
> +	unsigned long address, value;
>   	int err;
>
> -	err = tegra_pcie_conf_address(pcie, bdf, where, &address);
> +	err = tegra_pcie_conf_address(pcie, bdf, offset, &address);
>   	if (err < 0) {
> -		*value = 0xffffffff;
> -		return 1;
> +		value = 0xffffffff;
> +		goto done;
>   	}
>
> -	*value = readl(address);
> +	value = readl(address);

This function used to be used only for dword-sized reads. Presumably in 
that case, "where" was always dword-aligned. When used for 
word/byte-sized reads, the wrappers to do that (e.g. 
pci_hose_read_config_word_via_dword) used to force the address passed to 
this function to be dword-aligned. Now that this function handles all 
sizes of read, I think it needs to align the address itself, doesn't it; 
i.e. pass "offset & ~3" to tegra_pcie_conf_address() or "address & ~3" 
to readl()? Same for the write function below.

This seems to work in practice without generating any alignment faults 
though; I'm not sure why. Perhaps I'm missing something, or we're just 
getting lucky?

> +int pci_skip_dev(struct pci_controller *hose, pci_dev_t dev)

The diff might be slightly smaller/clearer if this function wasn't moved?