dm: pci: tegra: Convert Tegra PCI to driver model

Move this option to Kconig and fix up all users.

Quoted 2 lines · Simon Glass

Move this option to Kconig and fix up all users.

What are your thoughts on how/when to merge this? The series (mainly the final patch) conflicts with my series to add Tegra210 support, plus one of them needs rebasing onto the other so that either the board support my series adds gets adjusted by this series, or my series is adjusted for all the changes in this series.

Obviously I'd hope my series gets merged first since I sent it much earlier:-)

FWIW, I rebased this series on top of my series to validate it, and pushed it to:

git://github.com/swarren/u-boot.git pci-dm-conversion-rebase

That seems to work OK, but the final patch I added there needs integration into earlier patches and/or cleanup work.

This is not supported with driver model, so print a message instead of generating a build error. Rescanning PCI is not yet implemented.

Quoted 9 lines · Simon Glass

This is not supported with driver model, so print a message instead of generating a build error. Rescanning PCI is not yet implemented.

common/cmd_pci.c static int do_pci(cmd_tbl_t *cmdtp, int flag, int · line 466

Quoted 10 lines

common/cmd_pci.c +4 -0

@@ -457,7 +457,11 @@ static int do_pci(cmd_tbl_t *cmdtp, int flag, int
 argc, char * const argv[])
                return pci_cfg_display(bdf, addr, size, value);
  #ifdef CONFIG_CMD_PCI_ENUM
        case 'e':
+# ifdef CONFIG_DM_PCI
+               printf("This command is not yet supported with driver
 model\n");
+# else
                pci_init();
+# endif

That feature is enabled on most/all Tegra boards with PCI support. Hopefully nobody will miss it; I guess I haven't used it so I don't object to this change.

However, wouldn't it be better to remove CONFIG_CMD_PCI_ENUM from the config header rather than leaving the command enabled yet non-functional? Or are you planning on implementing this path in the near future?

Hi Stephen,

Quoted 26 lines · Stephen Warren

Quoted 9 lines · Simon Glass

This is not supported with driver model, so print a message instead of generating a build error. Rescanning PCI is not yet implemented.

common/cmd_pci.c static int do_pci(cmd_tbl_t *cmdtp, int flag, int · line 466

Quoted 10 lines

common/cmd_pci.c +4 -0

@@ -457,7 +457,11 @@ static int do_pci(cmd_tbl_t *cmdtp, int flag, int
 argc, char * const argv[])
                 return pci_cfg_display(bdf, addr, size, value);
   #ifdef CONFIG_CMD_PCI_ENUM
         case 'e':
+# ifdef CONFIG_DM_PCI
+               printf("This command is not yet supported with driver
 model\n");
+# else
                 pci_init();
+# endif

That feature is enabled on most/all Tegra boards with PCI support. Hopefully nobody will miss it; I guess I haven't used it so I don't object to this change.

Quoted 3 lines

However, wouldn't it be better to remove CONFIG_CMD_PCI_ENUM from the config header rather than leaving the command enabled yet non-functional? Or are you planning on implementing this path in the near future?

I was looking for feedback on why anyone would use this option. It's not clear to me what it is for.

We can implement it for driver model if it is needed.

Quoted 29 lines · Simon Glass

Hi Stephen,

Quoted 26 lines · Stephen Warren

Quoted 9 lines · Simon Glass

This is not supported with driver model, so print a message instead of generating a build error. Rescanning PCI is not yet implemented.

common/cmd_pci.c static int do_pci(cmd_tbl_t *cmdtp, int flag, int · line 466

Quoted 10 lines

common/cmd_pci.c +4 -0

@@ -457,7 +457,11 @@ static int do_pci(cmd_tbl_t *cmdtp, int flag, int
 argc, char * const argv[])
                return pci_cfg_display(bdf, addr, size, value);
  #ifdef CONFIG_CMD_PCI_ENUM
        case 'e':
+# ifdef CONFIG_DM_PCI
+               printf("This command is not yet supported with driver
 model\n");
+# else
                pci_init();
+# endif

That feature is enabled on most/all Tegra boards with PCI support. Hopefully nobody will miss it; I guess I haven't used it so I don't object to this change.

Quoted 6 lines

Quoted 3 lines

However, wouldn't it be better to remove CONFIG_CMD_PCI_ENUM from the config header rather than leaving the command enabled yet non-functional? Or are you planning on implementing this path in the near future?

I was looking for feedback on why anyone would use this option. It's not clear to me what it is for.

I assume it's to support hot-pluggable PCI connectors? Or perhaps it's just useful during debug of PCI drivers?

Quoted 1 line

We can implement it for driver model if it is needed.

I'd be happy disabling it in the configs myself. Thierry, you enabled the option when you first implemented the Tegra PCIe controller driver. Do you have a preference?

SDRAM doesn't always start at 0. Adjust the region mapping so that it works on platforms where SDRAM is somewhere else.

This needs testing on other platforms.

At present we add a new resource entry for every range entry. But some range entries refer to configuration regions. To make this work, avoid adding two regions of the same time. The later ranges will overwrite the earlier (configuration) ones.

There does not seem to be a way to distinguish the configuration ranges other than by ordering.

drivers/pci/pci-uclass.c +5 -0

+		}
+		if (pos == -1)
+			pos = hose->region_count++;
+		debug(" - type=%d, pos=%d\n", type, pos);
+		pci_set_region(hose->regions + pos, pci_addr, addr, size, type);
  	}

Quoted 7 lines · Simon Glass

At present we add a new resource entry for every range entry. But some range
entries refer to configuration regions. To make this work, avoid adding two
regions of the same time. The later ranges will overwrite the earlier (configuration) ones.

s/time/type/ in the last-but-one line.

What's wrong with having two regions of the same type? Equally, if we can "get away" with not storing some of the regions that happen to have a duplicate type, why not recast the function so that it only stores regions of specific (useful/desired) types, and simply dropping all of the other regions. That'd be a lot more consistent than only storing a somewhat arbitrary subset of the regions.

Quoted 3 lines

There does not seem to be a way to distinguish the configuration ranges other than by ordering.

Well, they do have different addresses too. But yes, the DT binding is written so that the entries in ranges must appear in a specific order, so order is the correct way to index the entries.

Quoted 1 line

drivers/pci/pci-uclass.c +0 -0

drivers/pci/pci-uclass.c static int decode_regions(struct pci_controller · line 729

Quoted 13 lines

drivers/pci/pci-uclass.c +4 -3

@@ -720,9 +721,15 @@ static int decode_regions(struct pci_controller
 *hose, const void *blob,
                } else {
                        continue;
                }
-               debug(" - type=%d\n", type);
-               pci_set_region(hose->regions + hose->region_count++,
 pci_addr,
-                              addr, size, type);
+               pos = -1;
+               for (i = 0; i < hose->region_count; i++) {
+                       if (hose->regions[i].flags == type)
+                               pos = i;

and break too?

Quoted 9 lines

drivers/pci/pci-uclass.c +5 -0

+               }
+               if (pos == -1)
+                       pos = hose->region_count++;
+               debug(" - type=%d, pos=%d\n", type, pos);
+               pci_set_region(hose->regions + pos, pci_addr, addr, size,
 type);

        }

Provide a few functions to support using 32-bit access to emulate 8- and 16-bit access.

A PCI bus may be a bridge device where the controller is the bridge's parent. Add a function to return the controller device, given a PCI device.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci-uclass.c | 8 ++++++++
 include/pci.h            | 8 ++++++++
 2 files changed, 16 insertions(+)

drivers/pci/pci-uclass.c +0 -0

index 1c6df0f..527d3bb 100644

drivers/pci/pci-uclass.c +5 -0

+               dev = dev->parent;
+
+       return dev;
+}
+
 pci_dev_t pci_get_bdf(struct udevice *dev)
 {
        struct pci_child_platdata *pplat = dev_get_parent_platdata(dev);

include/pci.h +2 -0

index f40a83d..8b471ab 100644
@@ -1123,6 +1123,14 @@ ulong pci_conv_size_to_32(ulong old, ulong value, uint offset,
                          enum pci_size_t size);
 
 /**
+ * pci_get_controller() - obtain the controller to use for a bus
+ *

This function looks up the controller and returns a pointer to each region type.

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.

drivers/pci/pci_tegra.c +5 -5

-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);

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

Quoted 6 lines · Simon Glass

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.

arch/arm/mach-tegra/Kconfig config TEGRA_ARMV7_COMMON · line 15

Quoted 10 lines

arch/arm/mach-tegra/Kconfig +1 -0

index a5b7e0d..aff4900 100644
@@ -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.

drivers/pci/pci_tegra.c line 13

Quoted 5 lines

drivers/pci/pci_tegra.c +0 -1

 
 
-#define DEBUG

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

Quoted 14 lines

-static int tegra_pcie_read_conf(struct pci_controller *hose, pci_dev_t bdf,

drivers/pci/pci_tegra.c +5 -4

-                               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);

drivers/pci/pci_tegra.c static int tegra_pcie_conf_address(struct tegra_pcie *pcie, pci_dev_t bdf, · line 295

Quoted 10 lines

drivers/pci/pci_tegra.c +4 -3

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

Quoted 2 lines

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