Re: [U-Boot] [PATCH v3 3/3] board:freescale:usb: Add device-tree fixup support for xhci controller

2016-03-03 Thread Sriram Dash 


>-Original Message-
>From: Marek Vasut [mailto:ma...@denx.de]
>Sent: Thursday, March 03, 2016 3:33 PM
>To: Sriram Dash ; u-boot@lists.denx.de
>Cc: york sun ; Ramneek Mehresh
>; Rajesh Bhagat 
>Subject: Re: [PATCH v3 3/3] board:freescale:usb: Add device-tree fixup support 
>for
>xhci controller
>
>On 03/03/2016 09:32 AM, Sriram Dash wrote:
>>> -Original Message-
>>> From: Marek Vasut [mailto:ma...@denx.de]
>>> Sent: Wednesday, March 02, 2016 3:55 AM
>>> To: Sriram Dash ; u-boot@lists.denx.de
>>> Cc: york sun ; Ramneek Mehresh
>>> ; Rajesh Bhagat 
>>> Subject: Re: [PATCH v3 3/3] board:freescale:usb: Add device-tree
>>> fixup support for xhci controller
>>>
>>> On 03/01/2016 08:03 AM, Sriram Dash wrote:
 Enables usb device-tree fixup code to incorporate xhci controller

 Signed-off-by: Ramneek Mehresh 
 Signed-off-by: Sriram Dash 
>>>
>>> Changelog ?
>>>
>>
>> Will include changelog for v2 and v3 in v4.
>>
 ---
  board/freescale/common/Makefile |  1 +
  board/freescale/common/usb.c| 30 +-
  include/fdt_support.h   |  4 ++--
  3 files changed, 16 insertions(+), 19 deletions(-)

 diff --git a/board/freescale/common/Makefile
 b/board/freescale/common/Makefile index 62de45c..c644896 100644
 --- a/board/freescale/common/Makefile
 +++ b/board/freescale/common/Makefile
 @@ -14,6 +14,7 @@ endif
  endif

  obj-$(CONFIG_USB_EHCI_FSL) += usb.o
 +obj-$(CONFIG_USB_XHCI_FSL) += usb.o

  ifdef MINIMAL
  # necessary to create built-in.o
 diff --git a/board/freescale/common/usb.c
 b/board/freescale/common/usb.c index d815dc1..8e423be 100644
 --- a/board/freescale/common/usb.c
 +++ b/board/freescale/common/usb.c
 @@ -18,29 +18,25 @@
  #define CONFIG_USB_MAX_CONTROLLER_COUNT 1  #endif

 +const char compat_usb_fsl[][16] = {"fsl-usb2-mph",
 + "fsl-usb2-dr",
 + "snps,dwc3"};
>>>
>>> This is const char *foo[].
>>>
>>
>> Reference from "char compat[][16] = { "cfi-flash", "jedec-flash" };"
>> I will change to const char *compat_usb_fsl[] in v4.
>>
  static const char *fdt_usb_get_node_type(void *blob, int start_offset,
 int *node_offset)
  {
 -  const char *compat_dr = "fsl-usb2-dr";
 -  const char *compat_mph = "fsl-usb2-mph";
const char *node_type = NULL;
 -
 -  *node_offset = fdt_node_offset_by_compatible(blob, start_offset,
 -   compat_mph);
 -  if (*node_offset < 0) {
 -  *node_offset = fdt_node_offset_by_compatible(blob,
 -   start_offset,
 -   compat_dr);
 -  if (*node_offset < 0) {
 -  printf("ERROR: could not find compatible node: %s\n",
 - fdt_strerror(*node_offset));
 -  } else {
 -  node_type = compat_dr;
 +  int size = sizeof(compat_usb_fsl)/sizeof(compat_usb_fsl[0]);
>>>
>>> Oh the art of counting. Firstly, what you did here is
>>> reimplementation of ARRAY_SIZE(), but that's wrong in this context.
>>> Each one of the array elements is differently sized, so to avoid
>>> problems with this crap, the code hard-codes random constant defining
>>> the element size, which is another crap workaround as it will break once a 
>>> longer
>element is added.
>>> And it wastes space. No, instead, use a terminating entry in the array.
>>>
>>
>> Will change compat_usb_fsl[][16] into const char *compat_usb_fsl[],
>> and use ARRAY_SIZE(compat_usb_fsl) . What is your opinion?
>
>My opinion is to use a terminating NULL entry and iterate over the array until 
>you
>reach it.
>

Accepted. Will do it in v4.

>--
>Best regards,
>Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/3] board:freescale:usb: Add device-tree fixup support for xhci controller

2016-03-03 Thread Marek Vasut 
On 03/03/2016 09:32 AM, Sriram Dash wrote:
>> -Original Message-
>> From: Marek Vasut [mailto:ma...@denx.de]
>> Sent: Wednesday, March 02, 2016 3:55 AM
>> To: Sriram Dash ; u-boot@lists.denx.de
>> Cc: york sun ; Ramneek Mehresh
>> ; Rajesh Bhagat 
>> Subject: Re: [PATCH v3 3/3] board:freescale:usb: Add device-tree fixup 
>> support for
>> xhci controller
>>
>> On 03/01/2016 08:03 AM, Sriram Dash wrote:
>>> Enables usb device-tree fixup code to incorporate xhci controller
>>>
>>> Signed-off-by: Ramneek Mehresh 
>>> Signed-off-by: Sriram Dash 
>>
>> Changelog ?
>>
> 
> Will include changelog for v2 and v3 in v4.
> 
>>> ---
>>>  board/freescale/common/Makefile |  1 +
>>>  board/freescale/common/usb.c| 30 +-
>>>  include/fdt_support.h   |  4 ++--
>>>  3 files changed, 16 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/board/freescale/common/Makefile
>>> b/board/freescale/common/Makefile index 62de45c..c644896 100644
>>> --- a/board/freescale/common/Makefile
>>> +++ b/board/freescale/common/Makefile
>>> @@ -14,6 +14,7 @@ endif
>>>  endif
>>>
>>>  obj-$(CONFIG_USB_EHCI_FSL) += usb.o
>>> +obj-$(CONFIG_USB_XHCI_FSL) += usb.o
>>>
>>>  ifdef MINIMAL
>>>  # necessary to create built-in.o
>>> diff --git a/board/freescale/common/usb.c
>>> b/board/freescale/common/usb.c index d815dc1..8e423be 100644
>>> --- a/board/freescale/common/usb.c
>>> +++ b/board/freescale/common/usb.c
>>> @@ -18,29 +18,25 @@
>>>  #define CONFIG_USB_MAX_CONTROLLER_COUNT 1  #endif
>>>
>>> +const char compat_usb_fsl[][16] = {"fsl-usb2-mph",
>>> +  "fsl-usb2-dr",
>>> +  "snps,dwc3"};
>>
>> This is const char *foo[].
>>
> 
> Reference from "char compat[][16] = { "cfi-flash", "jedec-flash" };"
> I will change to const char *compat_usb_fsl[] in v4. 
> 
>>>  static const char *fdt_usb_get_node_type(void *blob, int start_offset,
>>>  int *node_offset)
>>>  {
>>> -   const char *compat_dr = "fsl-usb2-dr";
>>> -   const char *compat_mph = "fsl-usb2-mph";
>>> const char *node_type = NULL;
>>> -
>>> -   *node_offset = fdt_node_offset_by_compatible(blob, start_offset,
>>> -compat_mph);
>>> -   if (*node_offset < 0) {
>>> -   *node_offset = fdt_node_offset_by_compatible(blob,
>>> -start_offset,
>>> -compat_dr);
>>> -   if (*node_offset < 0) {
>>> -   printf("ERROR: could not find compatible node: %s\n",
>>> -  fdt_strerror(*node_offset));
>>> -   } else {
>>> -   node_type = compat_dr;
>>> +   int size = sizeof(compat_usb_fsl)/sizeof(compat_usb_fsl[0]);
>>
>> Oh the art of counting. Firstly, what you did here is reimplementation of
>> ARRAY_SIZE(), but that's wrong in this context. Each one of the array 
>> elements is
>> differently sized, so to avoid problems with this crap, the code hard-codes 
>> random
>> constant defining the element size, which is another crap workaround as it 
>> will
>> break once a longer element is added.
>> And it wastes space. No, instead, use a terminating entry in the array.
>>
> 
> Will change compat_usb_fsl[][16] into const char *compat_usb_fsl[], 
> and use ARRAY_SIZE(compat_usb_fsl) . What is your opinion?

My opinion is to use a terminating NULL entry and iterate over the array
until you reach it.

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/3] board:freescale:usb: Add device-tree fixup support for xhci controller

2016-03-03 Thread Sriram Dash 
>-Original Message-
>From: Marek Vasut [mailto:ma...@denx.de]
>Sent: Wednesday, March 02, 2016 3:55 AM
>To: Sriram Dash ; u-boot@lists.denx.de
>Cc: york sun ; Ramneek Mehresh
>; Rajesh Bhagat 
>Subject: Re: [PATCH v3 3/3] board:freescale:usb: Add device-tree fixup support 
>for
>xhci controller
>
>On 03/01/2016 08:03 AM, Sriram Dash wrote:
>> Enables usb device-tree fixup code to incorporate xhci controller
>>
>> Signed-off-by: Ramneek Mehresh 
>> Signed-off-by: Sriram Dash 
>
>Changelog ?
>

Will include changelog for v2 and v3 in v4.

>> ---
>>  board/freescale/common/Makefile |  1 +
>>  board/freescale/common/usb.c| 30 +-
>>  include/fdt_support.h   |  4 ++--
>>  3 files changed, 16 insertions(+), 19 deletions(-)
>>
>> diff --git a/board/freescale/common/Makefile
>> b/board/freescale/common/Makefile index 62de45c..c644896 100644
>> --- a/board/freescale/common/Makefile
>> +++ b/board/freescale/common/Makefile
>> @@ -14,6 +14,7 @@ endif
>>  endif
>>
>>  obj-$(CONFIG_USB_EHCI_FSL) += usb.o
>> +obj-$(CONFIG_USB_XHCI_FSL) += usb.o
>>
>>  ifdef MINIMAL
>>  # necessary to create built-in.o
>> diff --git a/board/freescale/common/usb.c
>> b/board/freescale/common/usb.c index d815dc1..8e423be 100644
>> --- a/board/freescale/common/usb.c
>> +++ b/board/freescale/common/usb.c
>> @@ -18,29 +18,25 @@
>>  #define CONFIG_USB_MAX_CONTROLLER_COUNT 1  #endif
>>
>> +const char compat_usb_fsl[][16] = {"fsl-usb2-mph",
>> +   "fsl-usb2-dr",
>> +   "snps,dwc3"};
>
>This is const char *foo[].
>

Reference from "char compat[][16] = { "cfi-flash", "jedec-flash" };"
I will change to const char *compat_usb_fsl[] in v4. 

>>  static const char *fdt_usb_get_node_type(void *blob, int start_offset,
>>   int *node_offset)
>>  {
>> -const char *compat_dr = "fsl-usb2-dr";
>> -const char *compat_mph = "fsl-usb2-mph";
>>  const char *node_type = NULL;
>> -
>> -*node_offset = fdt_node_offset_by_compatible(blob, start_offset,
>> - compat_mph);
>> -if (*node_offset < 0) {
>> -*node_offset = fdt_node_offset_by_compatible(blob,
>> - start_offset,
>> - compat_dr);
>> -if (*node_offset < 0) {
>> -printf("ERROR: could not find compatible node: %s\n",
>> -   fdt_strerror(*node_offset));
>> -} else {
>> -node_type = compat_dr;
>> +int size = sizeof(compat_usb_fsl)/sizeof(compat_usb_fsl[0]);
>
>Oh the art of counting. Firstly, what you did here is reimplementation of
>ARRAY_SIZE(), but that's wrong in this context. Each one of the array elements 
>is
>differently sized, so to avoid problems with this crap, the code hard-codes 
>random
>constant defining the element size, which is another crap workaround as it will
>break once a longer element is added.
>And it wastes space. No, instead, use a terminating entry in the array.
>

Will change compat_usb_fsl[][16] into const char *compat_usb_fsl[], 
and use ARRAY_SIZE(compat_usb_fsl) . What is your opinion?

>btw use checkpatch before your next submission.
>
>> +int i;
>> +
>> +for (i = 0; i < size ; i++) {
>> +*node_offset = fdt_node_offset_by_compatible
>> +(blob, start_offset, compat_usb_fsl[i]);
>> +if (*node_offset >= 0) {
>> +node_type = compat_usb_fsl[i];
>> +break;
>>  }
>> -} else {
>> -node_type = compat_mph;
>>  }
>> -
>>  return node_type;
>>  }
>>
>> diff --git a/include/fdt_support.h b/include/fdt_support.h index
>> 296add0..d34e959 100644
>> --- a/include/fdt_support.h
>> +++ b/include/fdt_support.h
>> @@ -113,11 +113,11 @@ void fdt_fixup_qe_firmware(void *fdt);
>>   */
>>  int fdt_fixup_display(void *blob, const char *path, const char
>> *display);
>>
>> -#if defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB)
>> +#if defined(CONFIG_USB_EHCI_FSL) || defined(CONFIG_USB_XHCI_FSL)
>>  void fdt_fixup_dr_usb(void *blob, bd_t *bd);  #else  static inline
>> void fdt_fixup_dr_usb(void *blob, bd_t *bd) {} -#endif /*
>> defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB) */
>> +#endif /* defined(CONFIG_USB_EHCI_FSL) ||
>> +defined(CONFIG_USB_XHCI_FSL) */
>>
>>  #if defined(CONFIG_SYS_FSL_SEC_COMPAT)
>>  void fdt_fixup_crypto_node(void *blob, int sec_rev);
>>
>
>
>--
>Best regards,
>Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/3] board:freescale:usb: Add device-tree fixup support for xhci controller

2016-03-01 Thread Marek Vasut 
On 03/01/2016 08:03 AM, Sriram Dash wrote:
> Enables usb device-tree fixup code to incorporate xhci controller
> 
> Signed-off-by: Ramneek Mehresh 
> Signed-off-by: Sriram Dash 

Changelog ?

> ---
>  board/freescale/common/Makefile |  1 +
>  board/freescale/common/usb.c| 30 +-
>  include/fdt_support.h   |  4 ++--
>  3 files changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/board/freescale/common/Makefile b/board/freescale/common/Makefile
> index 62de45c..c644896 100644
> --- a/board/freescale/common/Makefile
> +++ b/board/freescale/common/Makefile
> @@ -14,6 +14,7 @@ endif
>  endif
>  
>  obj-$(CONFIG_USB_EHCI_FSL) += usb.o
> +obj-$(CONFIG_USB_XHCI_FSL) += usb.o
>  
>  ifdef MINIMAL
>  # necessary to create built-in.o
> diff --git a/board/freescale/common/usb.c b/board/freescale/common/usb.c
> index d815dc1..8e423be 100644
> --- a/board/freescale/common/usb.c
> +++ b/board/freescale/common/usb.c
> @@ -18,29 +18,25 @@
>  #define CONFIG_USB_MAX_CONTROLLER_COUNT 1
>  #endif
>  
> +const char compat_usb_fsl[][16] = {"fsl-usb2-mph",
> +"fsl-usb2-dr",
> +"snps,dwc3"};

This is const char *foo[].

>  static const char *fdt_usb_get_node_type(void *blob, int start_offset,
>int *node_offset)
>  {
> - const char *compat_dr = "fsl-usb2-dr";
> - const char *compat_mph = "fsl-usb2-mph";
>   const char *node_type = NULL;
> -
> - *node_offset = fdt_node_offset_by_compatible(blob, start_offset,
> -  compat_mph);
> - if (*node_offset < 0) {
> - *node_offset = fdt_node_offset_by_compatible(blob,
> -  start_offset,
> -  compat_dr);
> - if (*node_offset < 0) {
> - printf("ERROR: could not find compatible node: %s\n",
> -fdt_strerror(*node_offset));
> - } else {
> - node_type = compat_dr;
> + int size = sizeof(compat_usb_fsl)/sizeof(compat_usb_fsl[0]);

Oh the art of counting. Firstly, what you did here is reimplementation
of ARRAY_SIZE(), but that's wrong in this context. Each one of the array
elements is differently sized, so to avoid problems with this crap, the
code hard-codes random constant defining the element size, which is
another crap workaround as it will break once a longer element is added.
And it wastes space. No, instead, use a terminating entry in
the array.

btw use checkpatch before your next submission.

> + int i;
> +
> + for (i = 0; i < size ; i++) {
> + *node_offset = fdt_node_offset_by_compatible
> + (blob, start_offset, compat_usb_fsl[i]);
> + if (*node_offset >= 0) {
> + node_type = compat_usb_fsl[i];
> + break;
>   }
> - } else {
> - node_type = compat_mph;
>   }
> -
>   return node_type;
>  }
>  
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 296add0..d34e959 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -113,11 +113,11 @@ void fdt_fixup_qe_firmware(void *fdt);
>   */
>  int fdt_fixup_display(void *blob, const char *path, const char *display);
>  
> -#if defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB)
> +#if defined(CONFIG_USB_EHCI_FSL) || defined(CONFIG_USB_XHCI_FSL)
>  void fdt_fixup_dr_usb(void *blob, bd_t *bd);
>  #else
>  static inline void fdt_fixup_dr_usb(void *blob, bd_t *bd) {}
> -#endif /* defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB) 
> */
> +#endif /* defined(CONFIG_USB_EHCI_FSL) || defined(CONFIG_USB_XHCI_FSL) */
>  
>  #if defined(CONFIG_SYS_FSL_SEC_COMPAT)
>  void fdt_fixup_crypto_node(void *blob, int sec_rev);
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v3 3/3] board:freescale:usb: Add device-tree fixup support for xhci controller

2016-02-29 Thread Sriram Dash 
Enables usb device-tree fixup code to incorporate xhci controller

Signed-off-by: Ramneek Mehresh 
Signed-off-by: Sriram Dash 
---
 board/freescale/common/Makefile |  1 +
 board/freescale/common/usb.c| 30 +-
 include/fdt_support.h   |  4 ++--
 3 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/board/freescale/common/Makefile b/board/freescale/common/Makefile
index 62de45c..c644896 100644
--- a/board/freescale/common/Makefile
+++ b/board/freescale/common/Makefile
@@ -14,6 +14,7 @@ endif
 endif
 
 obj-$(CONFIG_USB_EHCI_FSL) += usb.o
+obj-$(CONFIG_USB_XHCI_FSL) += usb.o
 
 ifdef MINIMAL
 # necessary to create built-in.o
diff --git a/board/freescale/common/usb.c b/board/freescale/common/usb.c
index d815dc1..8e423be 100644
--- a/board/freescale/common/usb.c
+++ b/board/freescale/common/usb.c
@@ -18,29 +18,25 @@
 #define CONFIG_USB_MAX_CONTROLLER_COUNT 1
 #endif
 
+const char compat_usb_fsl[][16] = {"fsl-usb2-mph",
+  "fsl-usb2-dr",
+  "snps,dwc3"};
+
 static const char *fdt_usb_get_node_type(void *blob, int start_offset,
 int *node_offset)
 {
-   const char *compat_dr = "fsl-usb2-dr";
-   const char *compat_mph = "fsl-usb2-mph";
const char *node_type = NULL;
-
-   *node_offset = fdt_node_offset_by_compatible(blob, start_offset,
-compat_mph);
-   if (*node_offset < 0) {
-   *node_offset = fdt_node_offset_by_compatible(blob,
-start_offset,
-compat_dr);
-   if (*node_offset < 0) {
-   printf("ERROR: could not find compatible node: %s\n",
-  fdt_strerror(*node_offset));
-   } else {
-   node_type = compat_dr;
+   int size = sizeof(compat_usb_fsl)/sizeof(compat_usb_fsl[0]);
+   int i;
+
+   for (i = 0; i < size ; i++) {
+   *node_offset = fdt_node_offset_by_compatible
+   (blob, start_offset, compat_usb_fsl[i]);
+   if (*node_offset >= 0) {
+   node_type = compat_usb_fsl[i];
+   break;
}
-   } else {
-   node_type = compat_mph;
}
-
return node_type;
 }
 
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 296add0..d34e959 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -113,11 +113,11 @@ void fdt_fixup_qe_firmware(void *fdt);
  */
 int fdt_fixup_display(void *blob, const char *path, const char *display);
 
-#if defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB)
+#if defined(CONFIG_USB_EHCI_FSL) || defined(CONFIG_USB_XHCI_FSL)
 void fdt_fixup_dr_usb(void *blob, bd_t *bd);
 #else
 static inline void fdt_fixup_dr_usb(void *blob, bd_t *bd) {}
-#endif /* defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB) */
+#endif /* defined(CONFIG_USB_EHCI_FSL) || defined(CONFIG_USB_XHCI_FSL) */
 
 #if defined(CONFIG_SYS_FSL_SEC_COMPAT)
 void fdt_fixup_crypto_node(void *blob, int sec_rev);
-- 
2.1.0

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot