ARM: tegra: Add PCIe support

From: Thierry Reding <[email protected]>

Given a device tree node, the fdt_n_addr_cells() function will walk up the device tree and search for an #address-cells property. It returns the number of cells required by the device tree node to represent an address.

Similarly the fdt_n_size_cells() function returns the number of cells required by the given device tree node to represent a size. It achieves that by walking up the device tree in seach for a #size-cells property.

If no #address-cells or #size-cells property can be found, both of the functions return 1.

Signed-off-by Thierry Reding <[email protected]>

---
 include/libfdt.h    | 28 ++++++++++++++++++++++++++++
 lib/libfdt/fdt_ro.c | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

include/libfdt.h +3 -0

index a1ef1e15df3d..e7f991b388cf 100644
@@ -1638,4 +1638,32 @@ int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,
                     struct fdt_region region[], int max_regions,
                     char *path, int path_len, int add_string_tab);
 
+/**
+ * fdt_n_addr_cells() - find the number of address cells required by a node
+ *

Hi Thierry,

Quoted 31 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Given a device tree node, the fdt_n_addr_cells() function will walk up the device tree and search for an #address-cells property. It returns the number of cells required by the device tree node to represent an address.

Similarly the fdt_n_size_cells() function returns the number of cells required by the given device tree node to represent a size. It achieves that by walking up the device tree in seach for a #size-cells property.

If no #address-cells or #size-cells property can be found, both of the functions return 1.

Signed-off-by Thierry Reding <[email protected]>

---
 include/libfdt.h    | 28 ++++++++++++++++++++++++++++
 lib/libfdt/fdt_ro.c | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

include/libfdt.h +3 -0

index a1ef1e15df3d..e7f991b388cf 100644
@@ -1638,4 +1638,32 @@ int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,
                     struct fdt_region region[], int max_regions,
                     char *path, int path_len, int add_string_tab);
 
+/**
+ * fdt_n_addr_cells() - find the number of address cells required by a node
+ *

include/libfdt.h int fdt_find_regions(const void *fdt, char * const inc[], int inc_count, · line 1653

Quoted 10 lines

include/libfdt.h +10 -0

+ * Looks up the #address-cells property of the node to examine. If that has
+ * no such property, walks up the device tree until it finds one in one of
+ * the device's parents. If no #address-cells property is found, it is
+ * assumed to be 1.
+ *
+ * @param fdt          FDT blob
+ * @param node         node to examine
+ * @return number of address cells
+ */
+int fdt_n_addr_cells(const void *fdt, int node);

There is a new fdt_address_cells() recently that looks suitable.

include/libfdt.h int fdt_find_regions(const void *fdt, char * const inc[], int inc_count, · line 1656

Quoted 3 lines

include/libfdt.h +3 -0

+
+/**
+ * fdt_n_size_cells() - find the number of size cells required by a node

Also fdt_size_cells().

Quoted 3 lines · Simon Glass

Hi Thierry,

On 18 August 2014 01:16, Thierry Reding <[email protected]>

Quoted 20 lines · wrote:

Quoted 20 lines

From: Thierry Reding <[email protected]>

Given a device tree node, the fdt_n_addr_cells() function will walk up the device tree and search for an #address-cells property. It returns the number of cells required by the device tree node to represent an address.

Similarly the fdt_n_size_cells() function returns the number of cells required by the given device tree node to represent a size. It achieves that by walking up the device tree in seach for a #size-cells property.

If no #address-cells or #size-cells property can be found, both of the functions return 1.

Signed-off-by Thierry Reding <[email protected]>

---
 include/libfdt.h    | 28 ++++++++++++++++++++++++++++
 lib/libfdt/fdt_ro.c | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

Quoted 5 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char *

Quoted 5 lines

include/libfdt.h +0 -0

index a1ef1e15df3d..e7f991b388cf 100644
@@ -1638,4 +1638,32 @@ int fdt_find_regions(const void *fdt, char *

const inc[], int inc_count,

Quoted 5 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,

Quoted 5 lines

include/libfdt.h +2 -0

                     struct fdt_region region[], int max_regions,
                     char *path, int path_len, int add_string_tab);
 
+/**
+ * fdt_n_addr_cells() - find the number of address cells required by

a node

Quoted 2 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * · line 1644

Quoted 2 lines

include/libfdt.h +2 -0

+ *
+ * Looks up the #address-cells property of the node to examine. If

that has

Quoted 1 line

include/libfdt.h int fdt_find_regions(const void *fdt, char * · line 1645

Quoted 1 line

include/libfdt.h +1 -0

+ * no such property, walks up the device tree until it finds one in

one of

Quoted 9 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * const inc[], int inc_count, · line 1653

Quoted 8 lines

include/libfdt.h +8 -0

+ * the device's parents. If no #address-cells property is found, it is
+ * assumed to be 1.
+ *
+ * @param fdt          FDT blob
+ * @param node         node to examine
+ * @return number of address cells
+ */
+int fdt_n_addr_cells(const void *fdt, int node);

Quoted 5 lines

There is a new fdt_address_cells() recently that looks suitable.

include/libfdt.h int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,

Quoted 3 lines

include/libfdt.h +3 -0

+
+/**
+ * fdt_n_size_cells() - find the number of size cells required by a

node

Quoted 2 lines

Also fdt_size_cells().

Neither of those seem to walk up the tree, so inheriting #address-cells or #size-cells from a parent will not work.

According to the comments in the code they're also supposed to return the number of cells represented by a bus, not a device (which might explain why it doesn't walk up the tree).

I also can't reuse them to implement these fdt_n_addr_cells() and fdt_n_size_cells() functions because they don't return an accurate error if the #address-cells and #size-cells are not found, therefore it's impossible to decide when to climb further up the tree.

Hi Theirry,

Quoted 67 lines · Thierry Reding

Quoted 3 lines · Simon Glass

Hi Thierry,

On 18 August 2014 01:16, Thierry Reding <[email protected]>

Quoted 20 lines · wrote:

Quoted 20 lines

From: Thierry Reding <[email protected]>

Given a device tree node, the fdt_n_addr_cells() function will walk up the device tree and search for an #address-cells property. It returns the number of cells required by the device tree node to represent an address.

Similarly the fdt_n_size_cells() function returns the number of cells required by the given device tree node to represent a size. It achieves that by walking up the device tree in seach for a #size-cells property.

If no #address-cells or #size-cells property can be found, both of the functions return 1.

Signed-off-by Thierry Reding <[email protected]>

---
 include/libfdt.h    | 28 ++++++++++++++++++++++++++++
 lib/libfdt/fdt_ro.c | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

Quoted 5 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char *

Quoted 5 lines

include/libfdt.h +0 -0

index a1ef1e15df3d..e7f991b388cf 100644
@@ -1638,4 +1638,32 @@ int fdt_find_regions(const void *fdt, char *

const inc[], int inc_count,

Quoted 5 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,

Quoted 5 lines

include/libfdt.h +2 -0

                     struct fdt_region region[], int max_regions,
                     char *path, int path_len, int add_string_tab);
 
+/**
+ * fdt_n_addr_cells() - find the number of address cells required by

a node

Quoted 2 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * · line 1644

Quoted 2 lines

include/libfdt.h +2 -0

+ *
+ * Looks up the #address-cells property of the node to examine. If

that has

Quoted 1 line

include/libfdt.h int fdt_find_regions(const void *fdt, char * · line 1645

Quoted 1 line

include/libfdt.h +1 -0

+ * no such property, walks up the device tree until it finds one in

one of

Quoted 9 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * const inc[], int inc_count, · line 1653

Quoted 8 lines

include/libfdt.h +8 -0

+ * the device's parents. If no #address-cells property is found, it is
+ * assumed to be 1.
+ *
+ * @param fdt          FDT blob
+ * @param node         node to examine
+ * @return number of address cells
+ */
+int fdt_n_addr_cells(const void *fdt, int node);

Quoted 5 lines

There is a new fdt_address_cells() recently that looks suitable.

include/libfdt.h int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,

Quoted 3 lines

include/libfdt.h +3 -0

+
+/**
+ * fdt_n_size_cells() - find the number of size cells required by a

node

Quoted 2 lines

Also fdt_size_cells().

Neither of those seem to walk up the tree, so inheriting #address-cells or #size-cells from a parent will not work.

According to the comments in the code they're also supposed to return the number of cells represented by a bus, not a device (which might explain why it doesn't walk up the tree).

Quoted 5 lines

I also can't reuse them to implement these fdt_n_addr_cells() and fdt_n_size_cells() functions because they don't return an accurate error if the #address-cells and #size-cells are not found, therefore it's impossible to decide when to climb further up the tree.

OK but I have one more question. I thought that dtc gave a warning when you don't explicitly specify these values in the parent node?

Quoted 72 lines · Simon Glass

Hi Theirry,

Quoted 67 lines · Thierry Reding

Quoted 3 lines · Simon Glass

Hi Thierry,

On 18 August 2014 01:16, Thierry Reding <[email protected]>

Quoted 20 lines · wrote:

Quoted 20 lines

From: Thierry Reding <[email protected]>

Given a device tree node, the fdt_n_addr_cells() function will walk up the device tree and search for an #address-cells property. It returns the number of cells required by the device tree node to represent an address.

Similarly the fdt_n_size_cells() function returns the number of cells required by the given device tree node to represent a size. It achieves that by walking up the device tree in seach for a #size-cells property.

If no #address-cells or #size-cells property can be found, both of the functions return 1.

Signed-off-by Thierry Reding <[email protected]>

---
 include/libfdt.h    | 28 ++++++++++++++++++++++++++++
 lib/libfdt/fdt_ro.c | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

Quoted 5 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char *

Quoted 5 lines

include/libfdt.h +0 -0

index a1ef1e15df3d..e7f991b388cf 100644
@@ -1638,4 +1638,32 @@ int fdt_find_regions(const void *fdt, char *

const inc[], int inc_count,

Quoted 5 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,

Quoted 5 lines

include/libfdt.h +2 -0

                     struct fdt_region region[], int max_regions,
                     char *path, int path_len, int add_string_tab);
 
+/**
+ * fdt_n_addr_cells() - find the number of address cells required by

a node

Quoted 2 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * · line 1644

Quoted 2 lines

include/libfdt.h +2 -0

+ *
+ * Looks up the #address-cells property of the node to examine. If

that has

Quoted 1 line

include/libfdt.h int fdt_find_regions(const void *fdt, char * · line 1645

Quoted 1 line

include/libfdt.h +1 -0

+ * no such property, walks up the device tree until it finds one in

one of

Quoted 9 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * const inc[], int inc_count, · line 1653

Quoted 8 lines

include/libfdt.h +8 -0

+ * the device's parents. If no #address-cells property is found, it is
+ * assumed to be 1.
+ *
+ * @param fdt          FDT blob
+ * @param node         node to examine
+ * @return number of address cells
+ */
+int fdt_n_addr_cells(const void *fdt, int node);

Quoted 5 lines

There is a new fdt_address_cells() recently that looks suitable.

include/libfdt.h int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,

Quoted 3 lines

include/libfdt.h +3 -0

+
+/**
+ * fdt_n_size_cells() - find the number of size cells required by a

node

Quoted 2 lines

Also fdt_size_cells().

Neither of those seem to walk up the tree, so inheriting #address-cells or #size-cells from a parent will not work.

According to the comments in the code they're also supposed to return the number of cells represented by a bus, not a device (which might explain why it doesn't walk up the tree).

Quoted 8 lines

Quoted 5 lines

I also can't reuse them to implement these fdt_n_addr_cells() and fdt_n_size_cells() functions because they don't return an accurate error if the #address-cells and #size-cells are not found, therefore it's impossible to decide when to climb further up the tree.

OK but I have one more question. I thought that dtc gave a warning when you don't explicitly specify these values in the parent node?

It doesn't explicitly warn about the properties being absent. Rather it will fallback to the default (#address-cells = <2>, #size-cells = <1> in the version that I have) and warn if those don't match what's expected.

Interestingly as opposed to what the Linux kernel does, DTC doesn't seem to climb further up the tree if it can't find the properties in a device tree node. Instead, it seems to always use the default values instead.

Adding the [email protected] mailing list. Maybe somebody there can help iron out this inconsistency. ePAPR explicitly says that both
"... #address-cells and #size-cells properties are not inherited from
ancestors in the device tree. They shall be explicitly defined." (see
2.3.5 "#address-cells and #size-cells").

Hi Thierry,

Quoted 90 lines · Thierry Reding

Quoted 72 lines · Simon Glass

Hi Theirry,

Quoted 67 lines · Thierry Reding

Quoted 3 lines · Simon Glass

Hi Thierry,

On 18 August 2014 01:16, Thierry Reding <[email protected]>

Quoted 20 lines · wrote:

Quoted 20 lines

From: Thierry Reding <[email protected]>

Given a device tree node, the fdt_n_addr_cells() function will walk up the device tree and search for an #address-cells property. It returns the number of cells required by the device tree node to represent an address.

Similarly the fdt_n_size_cells() function returns the number of cells required by the given device tree node to represent a size. It achieves that by walking up the device tree in seach for a #size-cells property.

If no #address-cells or #size-cells property can be found, both of the functions return 1.

Signed-off-by Thierry Reding <[email protected]>

---
 include/libfdt.h    | 28 ++++++++++++++++++++++++++++
 lib/libfdt/fdt_ro.c | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

Quoted 5 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char *

Quoted 5 lines

include/libfdt.h +0 -0

index a1ef1e15df3d..e7f991b388cf 100644
@@ -1638,4 +1638,32 @@ int fdt_find_regions(const void *fdt, char *

const inc[], int inc_count,

Quoted 5 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * · line 1642

Quoted 5 lines

include/libfdt.h +2 -0

                     struct fdt_region region[], int max_regions,
                     char *path, int path_len, int add_string_tab);
 
+/**
+ * fdt_n_addr_cells() - find the number of address cells required by

a node

Quoted 2 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * · line 1644

Quoted 2 lines

include/libfdt.h +2 -0

+ *
+ * Looks up the #address-cells property of the node to examine. If

that has

Quoted 1 line

include/libfdt.h int fdt_find_regions(const void *fdt, char * · line 1645

Quoted 1 line

include/libfdt.h +1 -0

+ * no such property, walks up the device tree until it finds one in

one of

Quoted 9 lines

include/libfdt.h int fdt_find_regions(const void *fdt, char * · line 1653

Quoted 8 lines

include/libfdt.h +8 -0

+ * the device's parents. If no #address-cells property is found, it is
+ * assumed to be 1.
+ *
+ * @param fdt          FDT blob
+ * @param node         node to examine
+ * @return number of address cells
+ */
+int fdt_n_addr_cells(const void *fdt, int node);

Quoted 5 lines

There is a new fdt_address_cells() recently that looks suitable.

include/libfdt.h int fdt_find_regions(const void *fdt, char * · line 1656

Quoted 3 lines

include/libfdt.h +3 -0

+
+/**
+ * fdt_n_size_cells() - find the number of size cells required by a

node

Quoted 2 lines

Also fdt_size_cells().

Neither of those seem to walk up the tree, so inheriting #address-cells or #size-cells from a parent will not work.

According to the comments in the code they're also supposed to return the number of cells represented by a bus, not a device (which might explain why it doesn't walk up the tree).

Quoted 8 lines

Quoted 5 lines

I also can't reuse them to implement these fdt_n_addr_cells() and fdt_n_size_cells() functions because they don't return an accurate error if the #address-cells and #size-cells are not found, therefore it's impossible to decide when to climb further up the tree.

OK but I have one more question. I thought that dtc gave a warning when you don't explicitly specify these values in the parent node?

It doesn't explicitly warn about the properties being absent. Rather it will fallback to the default (#address-cells = <2>, #size-cells = <1> in the version that I have) and warn if those don't match what's expected.

Interestingly as opposed to what the Linux kernel does, DTC doesn't seem to climb further up the tree if it can't find the properties in a device tree node. Instead, it seems to always use the default values instead.

Quoted 5 lines

Adding the [email protected] mailing list. Maybe somebody there can help iron out this inconsistency. ePAPR explicitly says that both
"... #address-cells and #size-cells properties are not inherited from
ancestors in the device tree. They shall be explicitly defined." (see
2.3.5 "#address-cells and #size-cells").

That's my understanding too. So should we drop this patch?

From: Thierry Reding <[email protected]>

Given a device tree node and a property name, the fdt_get_string_index() function will look up a given string in the string list contained in the property's value and return its index.

Signed-off-by Thierry Reding <[email protected]>

---
 include/libfdt.h    | 11 +++++++++++
 lib/libfdt/fdt_ro.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

include/libfdt.h +11 -0

index e7f991b388cf..4d7fb2681669 100644
@@ -857,6 +857,17 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
  */
 int fdt_stringlist_contains(const char *strlist, int listlen, const char *str);
 
+/**
+ * fdt_get_string_index - get the index of a string in a string list
+ * @fdt: pointer to the device tree blob
+ * @node: offset of the node
+ * @property: name of the property containing the string list
+ * @string: string to look up in the string list
+ * @return the index of the string or negative on error
+ */
+int fdt_get_string_index(const void *fdt, int node, const char *property,
+                        const char *string);
+
 /**********************************************************************/
 /* Read-only functions (addressing related)                           */
 /**********************************************************************/

lib/libfdt/fdt_ro.c +6 -0

index 17cd11333c1e..f211c64ca7ca 100644
@@ -14,6 +14,8 @@
 
 #include "libfdt_internal.h"
 
+#define max(x, y) (((x) < (y)) ? (y) : (x))
+
 static int _fdt_nodename_eq(const void *fdt, int offset,
                            const char *s, int len)
 {

@@ -491,6 +493,32 @@ int fdt_stringlist_contains(const char *strlist, int listlen, const char *str)
        return 0;
 }
 
+int fdt_get_string_index(const void *fdt, int node, const char *property,
+                        const char *string)
+{
+       const char *list, *end;

lib/libfdt/fdt_ro.c +6 -1

+               index++;
+       }
+
+       return -FDT_ERR_NOTFOUND;
+}
+
 int fdt_node_check_compatible(const void *fdt, int nodeoffset,
                              const char *compatible)
 {
--

2.0.4

On 18 August 2014 01:16, Thierry Reding <[email protected]> wrote:

Quoted 8 lines

lib/libfdt/fdt_ro.c +8 -0

+int fdt_get_string_index(const void *fdt, int node, const char *property,
+                        const char *string)
+{
+       const char *list, *end;
+       int len, index = 0;
+
+       list = fdt_getprop(fdt, node, property, &len);
+       if (!list)

lib/libfdt/fdt_ro.c int fdt_stringlist_contains(const char *strlist, int listlen, const char *str) · line 509

Quoted 6 lines

lib/libfdt/fdt_ro.c +6 -0

+               return len;
+
+       end = list + len;
+
+       while (list < end) {
+               int n = strlen(string);

This can go outside the loop.

lib/libfdt/fdt_ro.c int fdt_stringlist_contains(const char *strlist, int listlen, const char *str) · line 515

Quoted 6 lines

lib/libfdt/fdt_ro.c +6 -0

+               int m = strlen(list);
+
+               if (n == m && memcmp(list, string, n) == 0)
+                       return index;
+
+               list += max(n, m) + 1;

I worry that if n > m this will end up in the middle of a the next string in the list. What is the intention here?

From: Thierry Reding <[email protected]>

Add the fdt_get_resource() and fdt_get_named_resource() functions which can be used to parse resources (memory regions) from an FDT. A helper to compute the size of a region is also provided.

Signed-off-by Thierry Reding <[email protected]>

---
 include/fdtdec.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/fdtdec.c     | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

include/fdtdec.h +0 -0

index 856e6cf766de..e9091eee6bae 100644

include/fdtdec.h +31 -0

+struct fdt_resource {
+       fdt_addr_t start;
+       fdt_addr_t end;
+};
+
+/**
+ * Compute the size of a resource.
+ *
+ * @param res  the resource to operate on
+ * @return the size of the resource
+ */
+static inline fdt_size_t fdt_resource_size(const struct fdt_resource *res)
+{
+       return res->end - res->start + 1;
+}
+
 /**
  * Compat types that we know about and for which we might have drivers.
  * Each is named COMPAT_<dir>_<filename> where <dir> is the directory

@@ -583,4 +600,35 @@ struct fmap_entry {
  */
 int fdtdec_read_fmap_entry(const void *blob, int node, const char *name,
                           struct fmap_entry *entry);
+
+/**
+ * Obtain an indexed resource from a device property.
+ *
+ * @param fdt          FDT blob
+ * @param node         node to examine
+ * @param property     name of the property to parse
+ * @param index                index of the resource to retrieve
+ * @param res          returns the resource
+ * @return 0 if ok, negative on error
+ */
+int fdt_get_resource(const void *fdt, int node, const char *property,
+                    unsigned int index, struct fdt_resource *res);
+
+/**

include/fdtdec.h +6 -0

+ * @param res          returns the resource
+ */
+int fdt_get_named_resource(const void *fdt, int node, const char *property,
+                          const char *names, const char *name,
+                          struct fdt_resource *res);
+
 #endif

lib/fdtdec.c +0 -0

index eb5aa20526fd..fbfae4a7cbaf 100644

lib/fdtdec.c +23 -1

+                       res->end = res->start + fdt_size_to_cpu(ptr[na]) - 1;
+                       return 0;
+               }
+
+               ptr += na + ns;
+               i++;
+       }
+
+       return -FDT_ERR_NOTFOUND;
+}
+
+int fdt_get_named_resource(const void *fdt, int node, const char *property,
+                          const char *names, const char *name,
+                          struct fdt_resource *res)
+{
+       int index;
+
+       index = fdt_get_string_index(fdt, node, names, name);
+       if (index < 0)
+               return index;
+
+       return fdt_get_resource(fdt, node, property, index, res);
+}
 #endif
--

2.0.4

Hi Thierry,

Quoted 20 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Add the fdt_get_resource() and fdt_get_named_resource() functions which can be used to parse resources (memory regions) from an FDT. A helper to compute the size of a region is also provided.

Signed-off-by Thierry Reding <[email protected]>

---
 include/fdtdec.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/fdtdec.c     | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

include/fdtdec.h +0 -0

index 856e6cf766de..e9091eee6bae 100644
@@ -40,6 +40,23 @@ struct fdt_memory {
        fdt_addr_t end;
 };
 

include/fdtdec.h struct fdt_memory { · line 43

Quoted 1 line

include/fdtdec.h +1 -0

+/* information about a resource */

Please add comments, e.g. that end is inclusive.

include/fdtdec.h struct fmap_entry { · line 627

Quoted 11 lines

include/fdtdec.h +11 -0

+/**
+ * Obtain a named resource from a device property.
+ *
+ * Look up the index of the name in a list of strings and return the resource
+ * at that index.
+ *
+ * @param fdt          FDT blob
+ * @param node         node to examine
+ * @param property     name of the property to parse
+ * @param names                name of the property to obtain the match the name to
+ * @param name         the name of the entry to look up

These two parameters are confusing. Perhaps rename names to something better?

include/fdtdec.h struct fdt_memory { · line 56

Quoted 2 lines

include/fdtdec.h +2 -0

+int fdt_get_resource(const void *fdt, int node, const char *property,
+                    unsigned int index, struct fdt_resource *res)

s/index/find_index/

include/fdtdec.h struct fdt_memory { · line 60

Quoted 4 lines

include/fdtdec.h +4 -0

+       if (!ptr)
+               return len;
+
+       end = ptr + len / 4;

sizeof(*ptr) might be better than 4.

include/fdtdec.h struct fdt_memory {

Quoted 4 lines

include/fdtdec.h +4 -0

+
+       while (ptr + na + ns <= end) {
+               if (i == index) {
+                       res->start = fdt_addr_to_cpu(*ptr);

This doesn't deal with 64-bit addresses. There is a half-hearted attempt with fdt_addr_t, but I wonder if we need a helper function like fdt_get_addr(ptr, num_cells)?

Quoted 23 lines · Simon Glass

Hi Thierry,

Quoted 20 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Add the fdt_get_resource() and fdt_get_named_resource() functions which can be used to parse resources (memory regions) from an FDT. A helper to compute the size of a region is also provided.

Signed-off-by Thierry Reding <[email protected]>

---
 include/fdtdec.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/fdtdec.c     | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

include/fdtdec.h +0 -0

index 856e6cf766de..e9091eee6bae 100644
@@ -40,6 +40,23 @@ struct fdt_memory {
        fdt_addr_t end;
 };
 

Quoted 3 lines

include/fdtdec.h struct fdt_memory { · line 43

Quoted 1 line

include/fdtdec.h +1 -0

+/* information about a resource */

Please add comments, e.g. that end is inclusive.

I've added this comment:

/*
* Information about a resource. start is the first address of the resource
* and end is the last address (inclusive). The length of the resource will
* be equal to: end - start + 1.
*/

Is that enough or do you want me to be more verbose?

Quoted 14 lines

Quoted 13 lines

include/fdtdec.h +11 -0

+/**
+ * Obtain a named resource from a device property.
+ *
+ * Look up the index of the name in a list of strings and return the resource
+ * at that index.
+ *
+ * @param fdt          FDT blob
+ * @param node         node to examine
+ * @param property     name of the property to parse
+ * @param names                name of the property to obtain the match the name to
+ * @param name         the name of the entry to look up

Quoted 1 line

These two parameters are confusing. Perhaps rename names to something better?

How about "prop_names" so that it indicates it is the name of a property? I've also adjusted the comment a bit:

* @param prop_names name of the property containing the list of names

Hopefully that will make it more obvious.

Quoted 6 lines

include/fdtdec.h struct fdt_memory { · line 56

Quoted 4 lines

include/fdtdec.h +2 -0

+int fdt_get_resource(const void *fdt, int node, const char *property,
+                    unsigned int index, struct fdt_resource *res)

s/index/find_index/

In my opinion the find_ prefix is redundant. After all the function name already indicates that we're looking for a resource inside a property. And index would be the offset in that property.

Quoted 8 lines

include/fdtdec.h struct fdt_memory { · line 60

Quoted 6 lines

include/fdtdec.h +4 -0

+       if (!ptr)
+               return len;
+
+       end = ptr + len / 4;

sizeof(*ptr) might be better than 4.

Device tree explicitly specifies that cells are 32-bit, so this can't ever be anything other than 4. But I'll change it anyway if you prefer.

Quoted 9 lines

include/fdtdec.h struct fdt_memory {

Quoted 5 lines

include/fdtdec.h +4 -0

+
+       while (ptr + na + ns <= end) {
+               if (i == index) {
+                       res->start = fdt_addr_to_cpu(*ptr);

This doesn't deal with 64-bit addresses. There is a half-hearted attempt with fdt_addr_t, but I wonder if we need a helper function like fdt_get_addr(ptr, num_cells)?

The Linux kernel handles this by wrapping it in an of_read_number() helper and always returning u64, like this:

        static inline u64 of_read_number(const __be32 *cell, int size)
        {
                u64 r = 0;

                while (size--)
                        r = (r << 32) | be32_to_cpu(*(cell++));

                return r;
        }

It obviously only works for size in { 0, 1, 2 }, but I can add a helper like that if you want.

On 19 August 2014 05:35, Thierry Reding <[email protected]> wrote:

Quoted 13 lines

The Linux kernel handles this by wrapping it in an of_read_number() helper and always returning u64, like this:

        static inline u64 of_read_number(const __be32 *cell, int size)
        {
                u64 r = 0;

                while (size--)
                        r = (r << 32) | be32_to_cpu(*(cell++));

                return r;
        }

Quoted 2 lines

It obviously only works for size in { 0, 1, 2 }, but I can add a helper like that if you want.

That looks good. It works for the cases we need and it's obvious later where the logic is if we want to extend it.

Quoted 1 line · Simon Glass

On 19 August 2014 05:35, Thierry Reding <[email protected]> wrote:

[...]

Quoted 13 lines

Quoted 13 lines

The Linux kernel handles this by wrapping it in an of_read_number() helper and always returning u64, like this:

        static inline u64 of_read_number(const __be32 *cell, int size)
        {
                u64 r = 0;

                while (size--)
                        r = (r << 32) | be32_to_cpu(*(cell++));

                return r;
        }

Quoted 6 lines

Quoted 2 lines

It obviously only works for size in { 0, 1, 2 }, but I can add a helper like that if you want.

That looks good. It works for the cases we need and it's obvious later where the logic is if we want to extend it.

This is what I have for U-Boot currently.

static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)

{
        fdt_addr_t addr = 0;

        while (cells--)
                /* do the shift in two steps to avoid warning on 32-bit */
                addr = (addr << 16) << 16 | fdt32_to_cpu(*ptr++);

        return addr;
}

I use the same function to parse the size field of reg entries, even though the types aren't technically the same. But making the types consistent would duplicate the above exactly, so I'm not sure it's worth it.

Alternatively perhaps something like this could be done:

        static u64 fdtdec_get_number(const fdt32_t *ptr, unsigned int cells)
        {
                /* the above */
        }

        static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)
        {
                return fdtdec_get_number(ptr, cells);
        }

        static fdt_size_t fdtdec_get_size(const fdt32_t *ptr, unsigned int cells)
        {
                return fdtdec_get_number(ptr, cells);
        }

Again, I'm not sure it's really worth the trouble since fdt_addr_t and fdt_size_t are both always either u32 or u64.

Hi Thierry,

Quoted 29 lines · Thierry Reding

Quoted 1 line · Simon Glass

On 19 August 2014 05:35, Thierry Reding <[email protected]> wrote:

[...]

Quoted 13 lines

Quoted 13 lines

The Linux kernel handles this by wrapping it in an of_read_number() helper and always returning u64, like this:

        static inline u64 of_read_number(const __be32 *cell, int size)
        {
                u64 r = 0;

                while (size--)
                        r = (r << 32) | be32_to_cpu(*(cell++));

                return r;
        }

Quoted 6 lines

Quoted 2 lines

It obviously only works for size in { 0, 1, 2 }, but I can add a helper like that if you want.

That looks good. It works for the cases we need and it's obvious later where the logic is if we want to extend it.

This is what I have for U-Boot currently.

static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)

{
        fdt_addr_t addr = 0;

Quoted 3 lines

        while (cells--)
                /* do the shift in two steps to avoid warning on 32-bit */
                addr = (addr << 16) << 16 | fdt32_to_cpu(*ptr++);

Odd warning! Does 32UL help?

Quoted 17 lines

Alternatively perhaps something like this could be done:

        static u64 fdtdec_get_number(const fdt32_t *ptr, unsigned int cells)
        {
                /* the above */
        }

        static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)
        {
                return fdtdec_get_number(ptr, cells);
        }

        static fdt_size_t fdtdec_get_size(const fdt32_t *ptr, unsigned int cells)
        {
                return fdtdec_get_number(ptr, cells);
        }

Quoted 2 lines

Again, I'm not sure it's really worth the trouble since fdt_addr_t and fdt_size_t are both always either u32 or u64.

Yes, although I suppose ultimately these might be 64-bit always, Perhaps we should merge the types?

Quoted 32 lines · Simon Glass

Hi Thierry,

Quoted 29 lines · Thierry Reding

Quoted 1 line · Simon Glass

On 19 August 2014 05:35, Thierry Reding <[email protected]> wrote:

[...]

Quoted 13 lines

Quoted 13 lines

The Linux kernel handles this by wrapping it in an of_read_number() helper and always returning u64, like this:

        static inline u64 of_read_number(const __be32 *cell, int size)
        {
                u64 r = 0;

                while (size--)
                        r = (r << 32) | be32_to_cpu(*(cell++));

                return r;
        }

Quoted 6 lines

Quoted 2 lines

It obviously only works for size in { 0, 1, 2 }, but I can add a helper like that if you want.

That looks good. It works for the cases we need and it's obvious later where the logic is if we want to extend it.

This is what I have for U-Boot currently.

static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)

{
        fdt_addr_t addr = 0;

Quoted 5 lines

Quoted 3 lines

        while (cells--)
                /* do the shift in two steps to avoid warning on 32-bit */
                addr = (addr << 16) << 16 | fdt32_to_cpu(*ptr++);

Odd warning! Does 32UL help?

The exact warning that I get is this:

warning: left shift count >= width of type

So the problem is that since addr is of type fdt_addr_t which is 32-bit, we're effectively shifting all bits out of the variable. The compiler will generate the same assembly code whether or not I use the single 32- bit shift or two 16-bit shifts, but using the latter the warning is gone. That's on both 32-bit and 64-bit ARM.

Quoted 18 lines

Quoted 18 lines

Alternatively perhaps something like this could be done:

        static u64 fdtdec_get_number(const fdt32_t *ptr, unsigned int cells)
        {
                /* the above */
        }

        static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)
        {
                return fdtdec_get_number(ptr, cells);
        }

        static fdt_size_t fdtdec_get_size(const fdt32_t *ptr, unsigned int cells)
        {
                return fdtdec_get_number(ptr, cells);
        }

Quoted 5 lines

Quoted 2 lines

Again, I'm not sure it's really worth the trouble since fdt_addr_t and fdt_size_t are both always either u32 or u64.

Yes, although I suppose ultimately these might be 64-bit always, Perhaps we should merge the types?

That's one other possibility. On Linux there's one common type for these:

        typedef phys_addr_t resource_size_t;

where phys_addr_t is defined as follows:

        #ifdef CONFIG_PHYS_ADDR_T_64BIT
        typedef u64 phys_addr_t;
        #else
        typedef u32 phys_addr_t;
        #endif

Perhaps we should simply copy that. I take it CONFIG_PHYS_64BIT is U-Boot's equivalent of CONFIG_PHYS_ADDR_T_64BIT? It doesn't seem to be documented anywhere but its usage would indicate that it is. I don't think U-Boot supports things like LPAE, so there's probably no need to control this more fine-grainedly than with a single CONFIG_PHYS_64BIT setting.

From: Thierry Reding <[email protected]>

The fdtdec_pci_get_bdf() function returns the bus, device, function triplet of a PCI device by parsing the "reg" property according to the PCI device tree binding.

Signed-off-by Thierry Reding <[email protected]>

From: Thierry Reding <[email protected]>

The fdt_for_each_subnode() iterator macro provided by this patch can be used to iterate over a device tree node's subnodes. At each iteration a loop variable will be set to the next subnode.

Signed-off-by Thierry Reding <[email protected]>

---
 include/libfdt.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

include/libfdt.h +4 -0

index 4d7fb2681669..0330d7a29a58 100644
@@ -163,6 +163,26 @@ int fdt_first_subnode(const void *fdt, int offset);
  */
 int fdt_next_subnode(const void *fdt, int offset);
 
+/**
+ * fdt_for_each_subnode - iterate over all subnodes of a parent
+ *
+ * This is actually a wrapper around a for loop and would be used like so:

include/libfdt.h +6 -1

+ */
+#define fdt_for_each_subnode(fdt, node, parent)                \
+       for (node = fdt_first_subnode(fdt, parent);     \
+            node >= 0;                                 \
+            node = fdt_next_subnode(fdt, node))
+
 /**********************************************************************/
 /* General functions                                                  */
 /**********************************************************************/
--

2.0.4

Quoted 6 lines · Thierry Reding

From: Thierry Reding <[email protected]>

The fdt_for_each_subnode() iterator macro provided by this patch can be used to iterate over a device tree node's subnodes. At each iteration a loop variable will be set to the next subnode.

Quoted 1 line

Signed-off-by Thierry Reding <[email protected]>

Acked-by Simon Glass <[email protected]>

Will this go upstream to dtc at some point?

Quoted 21 lines

---
 include/libfdt.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

include/libfdt.h +9 -0

index 4d7fb2681669..0330d7a29a58 100644
@@ -163,6 +163,26 @@ int fdt_first_subnode(const void *fdt, int offset);
  */
 int fdt_next_subnode(const void *fdt, int offset);
 
+/**
+ * fdt_for_each_subnode - iterate over all subnodes of a parent
+ *
+ * This is actually a wrapper around a for loop and would be used like so:
+ *
+ *     fdt_for_each_subnode(fdt, node, parent) {
+ *             ...
+ *             use node
+ *             ...

include/libfdt.h int fdt_first_subnode(const void *fdt, int offset); · line 179

Quoted 5 lines

include/libfdt.h +5 -0

+ *     }
+ *
+ * @fdt:       FDT blob
+ * @node:      child node
+ * @parent:    parent node

It might be worth mentioning the type of each of these since it is not in the macro.

Quoted 7 lines · Simon Glass

Quoted 6 lines · Thierry Reding

From: Thierry Reding <[email protected]>

The fdt_for_each_subnode() iterator macro provided by this patch can be used to iterate over a device tree node's subnodes. At each iteration a loop variable will be set to the next subnode.

Quoted 5 lines

Quoted 1 line

Signed-off-by Thierry Reding <[email protected]>

Acked-by Simon Glass <[email protected]>

Will this go upstream to dtc at some point?

I suppose it could. I don't know anything about how code flows back and forth between dtc and U-Boot.

Hi Thierry,

Quoted 14 lines · Thierry Reding

Quoted 12 lines · Simon Glass

Quoted 7 lines · Thierry Reding

From: Thierry Reding <[email protected]>

The fdt_for_each_subnode() iterator macro provided by this patch can be used to iterate over a device tree node's subnodes. At each iteration a loop variable will be set to the next subnode.

Signed-off-by Thierry Reding <[email protected]>

Acked-by Simon Glass <[email protected]>

Will this go upstream to dtc at some point?

Quoted 2 lines

I suppose it could. I don't know anything about how code flows back and forth between dtc and U-Boot.

Well normally we try to send changes upstream to minimise the diff between libfdt and U-Boot. You can join devicetree-compiler at vger.kernel.org and send patches. If they get modified upstream then I can apply a diff to U-Boot when they are applied.

From: Thierry Reding <[email protected]>

This macro can be overridden in source files (before including common.h) and can be used to specify a prefix for debug and error messages. An example of how to use this is shown below:

        #define pr_fmt(fmt) "foo: " fmt

        #include <common.h>

        ...
        debug("bar");

The resulting message will read:

foo: bar

Signed-off-by Thierry Reding <[email protected]>

---
 include/common.h | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

include/common.h +9 -6

index 1d6cb48ff078..65db04b452d7 100644
@@ -99,15 +99,19 @@ typedef volatile unsigned char      vu_char;
 #define _DEBUG 0
 #endif
 
+#ifndef pr_fmt
+#define pr_fmt(fmt) fmt
+#endif
+
 /*
  * Output a debug text when condition "cond" is met. The "cond" should be
  * computed by a preprocessor in the best case, allowing for the best
  * optimization.
  */
-#define debug_cond(cond, fmt, args...)         \
-       do {                                    \
-               if (cond)                       \
-                       printf(fmt, ##args);    \
+#define debug_cond(cond, fmt, args...)                 \
+       do {                                            \
+               if (cond)                               \
+                       printf(pr_fmt(fmt), ##args);    \
        } while (0)
 
 #define debug(fmt, args...)                    \

@@ -129,7 +133,7 @@ void __assert_fail(const char *assertion, const char *file, unsigned line,
                __assert_fail(#x, __FILE__, __LINE__, __func__); })
 
 #define error(fmt, args...) do {                                       \
-               printf("ERROR: " fmt "\nat %s:%d/%s()\n",               \
+               printf("ERROR: " pr_fmt(fmt) "\nat %s:%d/%s()\n",       \
                        ##args, __FILE__, __LINE__, __func__);          \
 } while (0)
 
--

2.0.4

Quoted 13 lines · Thierry Reding

From: Thierry Reding <[email protected]>

This macro can be overridden in source files (before including common.h) and can be used to specify a prefix for debug and error messages. An example of how to use this is shown below:

        #define pr_fmt(fmt) "foo: " fmt

        #include <common.h>

        ...
        debug("bar");

Quoted 5 lines

The resulting message will read:

foo: bar

Signed-off-by Thierry Reding <[email protected]>

Acked-by Simon Glass <[email protected]>

Seems OK. But I wonder if a string might be simpler?

#define DEBUG_PREFIX "foo: "

Quoted 26 lines · Simon Glass

Quoted 13 lines · Thierry Reding

From: Thierry Reding <[email protected]>

This macro can be overridden in source files (before including common.h) and can be used to specify a prefix for debug and error messages. An example of how to use this is shown below:

        #define pr_fmt(fmt) "foo: " fmt

        #include <common.h>

        ...
        debug("bar");

Quoted 5 lines

The resulting message will read:

foo: bar

Signed-off-by Thierry Reding <[email protected]>

Acked-by Simon Glass <[email protected]>

Seems OK. But I wonder if a string might be simpler?

#define DEBUG_PREFIX "foo: "

Review

Quoted 3 lines

(quoted diff) +3 -0

+#ifndef DEBUG_PREFIX
+#define DEBUG_PREFIX ""
+#endif

The advantage of using the macro is that it can perform more complicated transformations on the format string, like this for example:

        #define pr_fmt(fmt) "foo: %s():" fmt, __func__

Thierry

From: Thierry Reding <[email protected]>

This function is required by PCIe and SATA. This patch implements it on Tegra20, Tegra30 and Tegra124. It isn't implemented for Tegra114 because it doesn't support PCIe or SATA.

From: Thierry Reding <[email protected]>

This reset is required for PCIe and the corresponding ID therefore needs to be defined.

Quoted 4 lines · Thierry Reding

From: Thierry Reding <[email protected]>

This reset is required for PCIe and the corresponding ID therefore needs to be defined.

It might be worth mentioning in the commit description why this patch does different things for each SoC; namely that some SoCs already define this ID, and the code already contains the parts you're adding here.

Aside from that,

Acked-by Stephen Warren <[email protected]>

arch/arm/include/asm/arch-tegra/powergate.h +6 -0

 
+enum tegra_powergate {
+	TEGRA_POWERGATE_CPU,
+	TEGRA_POWERGATE_3D,
+	TEGRA_POWERGATE_VENC,
+	TEGRA_POWERGATE_PCIE,
+	TEGRA_POWERGATE_VDEC,

Quoted 4 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Implement the powergate API that allows various power partitions to be power up and down.

arch/arm/cpu/tegra-common/powergate.c line 25

Quoted 5 lines

arch/arm/cpu/tegra-common/powergate.c +2 -0

 
+static int tegra_powergate_set(enum tegra_powergate id, bool state)
 
+	writel(PWRGATE_TOGGLE_START | id, NV_PA_PMC_BASE + PWRGATE_TOGGLE);

Since the power-down/up is an asynchronous operation, don't you need to wait for it to complete here?

arch/arm/cpu/tegra-common/powergate.c line 49

Quoted 13 lines

arch/arm/cpu/tegra-common/powergate.c +12 -0

+	return 0;
+}
 
+static int tegra_powergate_remove_clamping(enum tegra_powergate id)
+{
+	unsigned long value;
+
+	if (id == TEGRA_POWERGATE_VDEC)
+		value = 1 << TEGRA_POWERGATE_PCIE;
+	else if (id == TEGRA_POWERGATE_PCIE)
+		value = 1 << TEGRA_POWERGATE_VDEC;
+	else
+		value = 1 << id;

A comment indicating why there's a special case here would be useful.

Isn't the special-case (HW design bug) restricted to Tegra20, or did it carry over into later chips in order to maintain HW register compatibility?

Quoted 12 lines

arch/arm/include/asm/arch-tegra/powergate.h +10 -0

 
+enum tegra_powergate {
+	TEGRA_POWERGATE_CPU,
+	TEGRA_POWERGATE_3D,
+	TEGRA_POWERGATE_VENC,
+	TEGRA_POWERGATE_PCIE,
+	TEGRA_POWERGATE_VDEC,
+	TEGRA_POWERGATE_L2,
+	TEGRA_POWERGATE_MPE,
+	TEGRA_POWERGATE_HEG,
+	TEGRA_POWERGATE_SATA,

arch/arm/include/asm/arch-tegra/powergate.h line 21

Quoted 6 lines

arch/arm/include/asm/arch-tegra/powergate.h +6 -0

+	TEGRA_POWERGATE_CPU1,
+	TEGRA_POWERGATE_CPU2,
+	TEGRA_POWERGATE_CPU3,
+	TEGRA_POWERGATE_CELP,
+	TEGRA_POWERGATE_3D1,
+};

I thought the list of partitions varied a bit by chip?

From: Thierry Reding <[email protected]>

This controller was introduced on Tegra114 to handle XUSB pads. On Tegra124 it is also used for PCIe and SATA pin muxing and PHY control. Only the Tegra124 PCIe and SATA functionality is currently implemented, with weak symbols on Tegra114.

Tegra20 and Tegra30 also provide weak symbols for these functions so that drivers can use the same API irrespective of which SoC they're being built for.

  arch/arm/include/asm/arch-tegra/xusb-padctl.h    |  14 +
  arch/arm/include/asm/arch-tegra114/xusb-padctl.h |   6 +
  arch/arm/include/asm/arch-tegra124/xusb-padctl.h |   6 +
  arch/arm/include/asm/arch-tegra20/xusb-padctl.h  |   6 +
  arch/arm/include/asm/arch-tegra30/xusb-padctl.h  |   6 +

Quoted 11 lines · Thierry Reding

From: Thierry Reding <[email protected]>

This controller was introduced on Tegra114 to handle XUSB pads. On Tegra124 it is also used for PCIe and SATA pin muxing and PHY control. Only the Tegra124 PCIe and SATA functionality is currently implemented, with weak symbols on Tegra114.

Tegra20 and Tegra30 also provide weak symbols for these functions so that drivers can use the same API irrespective of which SoC they're being built for.

Quoted 5 lines

 arch/arm/include/asm/arch-tegra/xusb-padctl.h    |  14 +
 arch/arm/include/asm/arch-tegra114/xusb-padctl.h |   6 +
 arch/arm/include/asm/arch-tegra124/xusb-padctl.h |   6 +
 arch/arm/include/asm/arch-tegra20/xusb-padctl.h  |   6 +
 arch/arm/include/asm/arch-tegra30/xusb-padctl.h  |   6 +

These per-SoC headers do nothing but include the common header. Why not just have the code include the common header directly? IIRC, that's what we do now for some other modules that are identical. We only need SoC-specific headers if we have SoC-specific information to represent.

arch/arm/cpu/tegra124-common/Makefile line 11

Quoted 6 lines

arch/arm/cpu/tegra124-common/Makefile +1 -0

 
 obj-y	+= clock.o
 obj-y	+= funcmux.o
 obj-y	+= pinmux.o
+obj-y	+= xusb-padctl.o

It looks like there's indentation inconsistency there?

arch/arm/cpu/tegra124-common/xusb-padctl.c line 438

Quoted 8 lines

arch/arm/cpu/tegra124-common/xusb-padctl.c +4 -0

 
+int fdtdec_count_strings(const void *fdt, int node, const char *prop_name)
 
+int fdtdec_get_string_index(const void *fdt, int node, const char *prop_name,
+			    int index, const char **output)
 
+int fdtdec_get_string(const void *fdt, int node, const char *prop_name,

Shouldn't those be in libfdt or similar? I thought I saw a patch to do that.

arch/arm/include/asm/arch-tegra/xusb-padctl.h

Quoted 3 lines

arch/arm/include/asm/arch-tegra/xusb-padctl.h +1 -0

 
+struct tegra_xusb_phy *tegra_xusb_phy_get(unsigned int type);

What is "type"? A comment would be useful, or using an enum type instead.

On 08/18/2014 01:16 AM, Thierry Reding wrote:

arch/arm/cpu/tegra124-common/xusb-padctl.c line 438

Quoted 8 lines

arch/arm/cpu/tegra124-common/xusb-padctl.c +4 -0

 
+int fdtdec_count_strings(const void *fdt, int node, const char *prop_name)
 
+int fdtdec_get_string_index(const void *fdt, int node, const char *prop_name,
+			    int index, const char **output)
 
+int fdtdec_get_string(const void *fdt, int node, const char *prop_name,

Shouldn't those be in libfdt or similar? I thought I saw a patch to do that.

From: Thierry Reding <[email protected]>

The XUSB pad controller is used for pinmuxing of the XUSB, PCIe and SATA lanes.

From: Thierry Reding <[email protected]>

Add the PCIe and SATA lane configuration to the Jetson TK1 device tree, so that the XUSB pad controller can be appropriately configured.

From: Thierry Reding <[email protected]>

Add support for the PCIe controller found on some generations of Tegra. Tegra20 has 2 root ports with a total of 4 lanes, Tegra30 has 3 root ports with a total of 6 lanes and Tegra124 has 2 root ports with a total of 5 lanes.

drivers/pci/pci_tegra.c +7 -0

+static bool tegra_pcie_port_check_link(struct tegra_pcie_port *port)
+{
+	unsigned int retries = 3;
+	unsigned long value;
+
+	value = rp_readl(port, RP_PRIV_MISC);
+	value &= ~RP_PRIV_MISC_PRSNT_MAP_EP_ABSNT;

Quoted 8 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Add support for the PCIe controller found on some generations of Tegra. Tegra20 has 2 root ports with a total of 4 lanes, Tegra30 has 3 root ports with a total of 6 lanes and Tegra124 has 2 root ports with a total of 5 lanes.

drivers/pci/pci_tegra.c +0 -0

Quoted 5 lines

+ * Copyright (c) 2010, CompuLab, Ltd.
+ * Author: Mike Rapoport <[email protected]>
+ *
+ * Based on NVIDIA PCIe driver
+ * Copyright (c) 2008-2009, NVIDIA Corporation.

Presumably also at least (c) 2013-2014 NVIDIA too?

Do you have Mike's S-o-b for the initial work?

Quoted 9 lines

drivers/pci/pci_tegra.c +9 -0

+static int tegra_pcie_read_conf(struct pci_controller *hose, pci_dev_t bdf,
+				int where, u32 *value)
+{
+	struct tegra_pcie *pcie = to_tegra_pcie(hose);
+	unsigned long address;
+	int err;
+
+	err = tegra_pcie_conf_address(pcie, bdf, where, &address);
+	if (err < 0) {

drivers/pci/pci_tegra.c line 295

Quoted 3 lines

drivers/pci/pci_tegra.c +3 -0

+		*value = 0xffffffff;
+		return 1;
+	}

In the kernel, don't we have to play games to dynamically map/unmap memory regions and/or map pages in a non-linear fashion since the config space is so large, and bus/device ID -> address mapping is odd? I don't see that here.

Quoted 15 lines

drivers/pci/pci_tegra.c +15 -0

+static bool tegra_pcie_port_check_link(struct tegra_pcie_port *port)
+{
+	unsigned int retries = 3;
+	unsigned long value;
+
+	value = rp_readl(port, RP_PRIV_MISC);
+	value &= ~RP_PRIV_MISC_PRSNT_MAP_EP_ABSNT;
+	value |= RP_PRIV_MISC_PRSNT_MAP_EP_PRSNT;
+	rp_writel(port, value, RP_PRIV_MISC);
+
+	do {
+		unsigned int timeout = 200;
+
+		do {
+			value = rp_readl(port, RP_VEND_XP);

drivers/pci/pci_tegra.c line 886

Quoted 2 lines

drivers/pci/pci_tegra.c +2 -0

+			if (value & RP_VEND_XP_DL_UP)
+				break;

On my board, this almost never succeeds for Jetson TK1's built-in NIC, although it works fine for the mini-PCIe slot. On the other two boards I borrowed, this succeeds almost always. I'd prefer if we could get to the bottom of this before we actually apply this, although the bug is proving quite perplexing. The mainline kernel PCIe patches have the same issue.

drivers/pci/pci_tegra.c line 1097

Quoted 10 lines

drivers/pci/pci_tegra.c +9 -0

+static int process_nodes(const void *fdt, int nodes[], unsigned int count)
 
+#ifdef CONFIG_PCI_SCAN_SHOW
+		printf("PCI: Enumerating devices...\n");
+		printf("---------------------------------------\n");
+		printf("  Device        ID          Description\n");
+		printf("  ------        --          -----------\n");
+#endif
+
+		pcie->hose.last_busno = pci_hose_scan(&pcie->hose);

Shouldn't that ifdef'd block be part of pci_hose_scan()?

Quoted 25 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Add support for the PCIe controller found on some generations of Tegra. Tegra20 has 2 root ports with a total of 4 lanes, Tegra30 has 3 root ports with a total of 6 lanes and Tegra124 has 2 root ports with a total of 5 lanes.

drivers/pci/pci_tegra.c +16 -0

 
+static bool tegra_pcie_port_check_link(struct tegra_pcie_port *port)
+{
+    unsigned int retries = 3;
+    unsigned long value;
+
+    value = rp_readl(port, RP_PRIV_MISC);
+    value &= ~RP_PRIV_MISC_PRSNT_MAP_EP_ABSNT;
+    value |= RP_PRIV_MISC_PRSNT_MAP_EP_PRSNT;
+    rp_writel(port, value, RP_PRIV_MISC);
+
+    do {
+        unsigned int timeout = 200;
+
+        do {
+            value = rp_readl(port, RP_VEND_XP);
+            if (value & RP_VEND_XP_DL_UP)

Quoted 1 line

+ break;

On my board, this almost never succeeds for Jetson TK1's built-in NIC, although it works fine for the mini-PCIe slot. On the other two boards I borrowed, this succeeds almost always. I'd prefer if we could get to the bottom of this before we actually apply this, although the bug is proving quite perplexing. The mainline kernel PCIe patches have the same issue.

Quoted 26 lines · Stephen Warren

Quoted 25 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Add support for the PCIe controller found on some generations of Tegra. Tegra20 has 2 root ports with a total of 4 lanes, Tegra30 has 3 root ports with a total of 6 lanes and Tegra124 has 2 root ports with a total of 5 lanes.

drivers/pci/pci_tegra.c +16 -0

 
+static bool tegra_pcie_port_check_link(struct tegra_pcie_port *port)
+{
+    unsigned int retries = 3;
+    unsigned long value;
+
+    value = rp_readl(port, RP_PRIV_MISC);
+    value &= ~RP_PRIV_MISC_PRSNT_MAP_EP_ABSNT;
+    value |= RP_PRIV_MISC_PRSNT_MAP_EP_PRSNT;
+    rp_writel(port, value, RP_PRIV_MISC);
+
+    do {
+        unsigned int timeout = 200;
+
+        do {
+            value = rp_readl(port, RP_VEND_XP);
+            if (value & RP_VEND_XP_DL_UP)

Quoted 8 lines

Quoted 1 line

+ break;

On my board, this almost never succeeds for Jetson TK1's built-in NIC, although it works fine for the mini-PCIe slot. On the other two boards I borrowed, this succeeds almost always. I'd prefer if we could get to the bottom of this before we actually apply this, although the bug is proving quite perplexing. The mainline kernel PCIe patches have the same issue.

I tracked the issue down to some missing pinmux configuration for the PCIe reset lane. With that in place, both U-Boot and the mainline Linux kernel PCIe support appear to work reliably. I'll work on patches to resolve that.

So, I think we can go ahead and merge these patches without issue ASAP, as soon as any review comments are addressed.

From: Thierry Reding <[email protected]>

Add the device tree node for the PCIe controller found on Tegra20 SoCs.

From: Thierry Reding <[email protected]>

The TrimSlice has an ethernet NIC connected to the PCIe bus. Enable the PCIe controller and the network driver so that the device can boot over the network.

Quoted 7 lines · Thierry Reding

From: Thierry Reding <[email protected]>

The TrimSlice has an ethernet NIC connected to the PCIe bus. Enable the PCIe controller and the network driver so that the device can boot over the network.

arch/arm/dts/tegra20-trimslice.dts +0 -0

arch/arm/dts/tegra20-trimslice.dts line 53

Quoted 9 lines

arch/arm/dts/tegra20-trimslice.dts +9 -0

+	pcie-controller {
+		status = "okay";
+		pex-clk-supply = <&pci_clk_reg>;
+		vdd-supply = <&pci_vdd_reg>;
+
+		pci at 1,0 {
+			status = "okay";
+		};
+	};

There are many more regulators here in the kernel's DT. Is that just the legacy properties in order to support the old incorrect binding?