[U-Boot] [PATCH 8/8] dm: tegra: pci: Convert tegra boards to driver model for PCI

[Simon Glass]

Hi Stephen,

On 21 October 2015 at 14:46, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 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.

I've rebased to mainline so hopefully we are OK now. I'd like to get
this in as I have a PCI series on top of it.

>
>> 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.

Sounds like a good idea to me.

>
>> 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?

tegra_pcie_conf_address() should be doing that correctly.

>
>> +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?

OK, I'll leave it. I do like the U_BOOT_DRIVER() thing to be at the
end of the file, but it is still very close to the end.

Regards,
Simon