phy: usb: Broadcom BCM4908 USB init fixes

[PATCH v2 1/2] phy: usb: Fix missing elements in BCM4908 USB init array

Posted by Sam Edwards 1 month, 3 weeks ago

The Broadcom USB PHY driver contains a lookup table
(`reg_bits_map_tables`) to resolve register bitmaps unique to certain
versions of the USB PHY as found in various Broadcom chip families. A
recent commit (see 'fixes' tag) introduced two new elements to each chip
family in this table -- except for one: BCM4908. This resulted in the
xHCI controller not being initialized correctly, causing a panic on
boot.

The next patch will update this table to use designated initializers in
order to prevent this from happening again. For now, just add back the
missing array elements to resolve the regression.

Fixes: 4536fe9640b6 ("phy: usb: suppress OC condition for 7439b2")
Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 drivers/phy/broadcom/phy-brcm-usb-init.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/phy/broadcom/phy-brcm-usb-init.c b/drivers/phy/broadcom/phy-brcm-usb-init.c
index 39536b6d96a9..5ebb3a616115 100644
--- a/drivers/phy/broadcom/phy-brcm-usb-init.c
+++ b/drivers/phy/broadcom/phy-brcm-usb-init.c
@@ -220,6 +220,8 @@ usb_reg_bits_map_table[BRCM_FAMILY_COUNT][USB_CTRL_SELECTOR_COUNT] = {
 		0, /* USB_CTRL_SETUP_SCB2_EN_MASK */
 		0, /* USB_CTRL_SETUP_SS_EHCI64BIT_EN_MASK */
 		0, /* USB_CTRL_SETUP_STRAP_IPP_SEL_MASK */
+		0, /* USB_CTRL_SETUP_OC3_DISABLE_PORT0_MASK */
+		0, /* USB_CTRL_SETUP_OC3_DISABLE_PORT1_MASK */
 		0, /* USB_CTRL_SETUP_OC3_DISABLE_MASK */
 		0, /* USB_CTRL_PLL_CTL_PLL_IDDQ_PWRDN_MASK */
 		0, /* USB_CTRL_USB_PM_BDC_SOFT_RESETB_MASK */
-- 
2.44.2

Re: [PATCH v2 1/2] phy: usb: Fix missing elements in BCM4908 USB init array

Posted by Justin Chen 1 month, 3 weeks ago


On 10/3/24 8:41 PM, Sam Edwards wrote:
> The Broadcom USB PHY driver contains a lookup table
> (`reg_bits_map_tables`) to resolve register bitmaps unique to certain
> versions of the USB PHY as found in various Broadcom chip families. A
> recent commit (see 'fixes' tag) introduced two new elements to each chip
> family in this table -- except for one: BCM4908. This resulted in the
> xHCI controller not being initialized correctly, causing a panic on
> boot.
> 
> The next patch will update this table to use designated initializers in
> order to prevent this from happening again. For now, just add back the
> missing array elements to resolve the regression.
> 
> Fixes: 4536fe9640b6 ("phy: usb: suppress OC condition for 7439b2")
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>

Woops! Sorry I broke it.

Reviewed-by: Justin Chen <justin.chen@broadcom.com>

> ---
>   drivers/phy/broadcom/phy-brcm-usb-init.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/phy/broadcom/phy-brcm-usb-init.c b/drivers/phy/broadcom/phy-brcm-usb-init.c
> index 39536b6d96a9..5ebb3a616115 100644
> --- a/drivers/phy/broadcom/phy-brcm-usb-init.c
> +++ b/drivers/phy/broadcom/phy-brcm-usb-init.c
> @@ -220,6 +220,8 @@ usb_reg_bits_map_table[BRCM_FAMILY_COUNT][USB_CTRL_SELECTOR_COUNT] = {
>   		0, /* USB_CTRL_SETUP_SCB2_EN_MASK */
>   		0, /* USB_CTRL_SETUP_SS_EHCI64BIT_EN_MASK */
>   		0, /* USB_CTRL_SETUP_STRAP_IPP_SEL_MASK */
> +		0, /* USB_CTRL_SETUP_OC3_DISABLE_PORT0_MASK */
> +		0, /* USB_CTRL_SETUP_OC3_DISABLE_PORT1_MASK */
>   		0, /* USB_CTRL_SETUP_OC3_DISABLE_MASK */
>   		0, /* USB_CTRL_PLL_CTL_PLL_IDDQ_PWRDN_MASK */
>   		0, /* USB_CTRL_USB_PM_BDC_SOFT_RESETB_MASK */

Re: [PATCH v2 1/2] phy: usb: Fix missing elements in BCM4908 USB init array

Posted by Florian Fainelli 1 month, 3 weeks ago

On 10/3/24 20:41, Sam Edwards wrote:
> The Broadcom USB PHY driver contains a lookup table
> (`reg_bits_map_tables`) to resolve register bitmaps unique to certain
> versions of the USB PHY as found in various Broadcom chip families. A
> recent commit (see 'fixes' tag) introduced two new elements to each chip
> family in this table -- except for one: BCM4908. This resulted in the
> xHCI controller not being initialized correctly, causing a panic on
> boot.

Yes, I think I see what happened here, we took the patch in the "fixes" 
tag from the our downstream tree, and it applied just fine, we will keep 
a closer eye on other entries in the future.

> 
> The next patch will update this table to use designated initializers in
> order to prevent this from happening again. For now, just add back the
> missing array elements to resolve the regression.

Out of curiosity, can you check whether building with 
-Wmissing-field-initializers would have caught this?

> 
> Fixes: 4536fe9640b6 ("phy: usb: suppress OC condition for 7439b2")
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

> ---
>   drivers/phy/broadcom/phy-brcm-usb-init.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/phy/broadcom/phy-brcm-usb-init.c b/drivers/phy/broadcom/phy-brcm-usb-init.c
> index 39536b6d96a9..5ebb3a616115 100644
> --- a/drivers/phy/broadcom/phy-brcm-usb-init.c
> +++ b/drivers/phy/broadcom/phy-brcm-usb-init.c
> @@ -220,6 +220,8 @@ usb_reg_bits_map_table[BRCM_FAMILY_COUNT][USB_CTRL_SELECTOR_COUNT] = {
>   		0, /* USB_CTRL_SETUP_SCB2_EN_MASK */
>   		0, /* USB_CTRL_SETUP_SS_EHCI64BIT_EN_MASK */
>   		0, /* USB_CTRL_SETUP_STRAP_IPP_SEL_MASK */
> +		0, /* USB_CTRL_SETUP_OC3_DISABLE_PORT0_MASK */
> +		0, /* USB_CTRL_SETUP_OC3_DISABLE_PORT1_MASK */
>   		0, /* USB_CTRL_SETUP_OC3_DISABLE_MASK */
>   		0, /* USB_CTRL_PLL_CTL_PLL_IDDQ_PWRDN_MASK */
>   		0, /* USB_CTRL_USB_PM_BDC_SOFT_RESETB_MASK */


-- 
Florian

Re: [PATCH v2 1/2] phy: usb: Fix missing elements in BCM4908 USB init array

Posted by Sam Edwards 1 month, 3 weeks ago

On Fri, Oct 4, 2024 at 9:14 AM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> On 10/3/24 20:41, Sam Edwards wrote:
> > The Broadcom USB PHY driver contains a lookup table
> > (`reg_bits_map_tables`) to resolve register bitmaps unique to certain
> > versions of the USB PHY as found in various Broadcom chip families. A
> > recent commit (see 'fixes' tag) introduced two new elements to each chip
> > family in this table -- except for one: BCM4908. This resulted in the
> > xHCI controller not being initialized correctly, causing a panic on
> > boot.
>
> Yes, I think I see what happened here, we took the patch in the "fixes"
> tag from the our downstream tree, and it applied just fine, we will keep
> a closer eye on other entries in the future.
>
> >
> > The next patch will update this table to use designated initializers in
> > order to prevent this from happening again. For now, just add back the
> > missing array elements to resolve the regression.
>
> Out of curiosity, can you check whether building with
> -Wmissing-field-initializers would have caught this?

It appears that the answer is no, at least here on Clang. I also just
tried -Wextra to see if any warning would catch it and didn't see one.
My understanding is that -Wmissing-field-initializers is for struct
fields, and a construct like:
int array[3] = {1, 2};
...does not result in a warning because it's considered perfectly
valid standards-compliant C per C's default initialization rule.

Cheers,
Sam

>
> >
> > Fixes: 4536fe9640b6 ("phy: usb: suppress OC condition for 7439b2")
> > Signed-off-by: Sam Edwards <CFSworks@gmail.com>
>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
>
> > ---
> >   drivers/phy/broadcom/phy-brcm-usb-init.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/phy/broadcom/phy-brcm-usb-init.c b/drivers/phy/broadcom/phy-brcm-usb-init.c
> > index 39536b6d96a9..5ebb3a616115 100644
> > --- a/drivers/phy/broadcom/phy-brcm-usb-init.c
> > +++ b/drivers/phy/broadcom/phy-brcm-usb-init.c
> > @@ -220,6 +220,8 @@ usb_reg_bits_map_table[BRCM_FAMILY_COUNT][USB_CTRL_SELECTOR_COUNT] = {
> >               0, /* USB_CTRL_SETUP_SCB2_EN_MASK */
> >               0, /* USB_CTRL_SETUP_SS_EHCI64BIT_EN_MASK */
> >               0, /* USB_CTRL_SETUP_STRAP_IPP_SEL_MASK */
> > +             0, /* USB_CTRL_SETUP_OC3_DISABLE_PORT0_MASK */
> > +             0, /* USB_CTRL_SETUP_OC3_DISABLE_PORT1_MASK */
> >               0, /* USB_CTRL_SETUP_OC3_DISABLE_MASK */
> >               0, /* USB_CTRL_PLL_CTL_PLL_IDDQ_PWRDN_MASK */
> >               0, /* USB_CTRL_USB_PM_BDC_SOFT_RESETB_MASK */
>
>
> --
> Florian

Re: [PATCH v2 1/2] phy: usb: Fix missing elements in BCM4908 USB init array

Posted by Florian Fainelli 1 month, 3 weeks ago

On 10/4/24 16:35, Sam Edwards wrote:
> On Fri, Oct 4, 2024 at 9:14 AM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
>>
>> On 10/3/24 20:41, Sam Edwards wrote:
>>> The Broadcom USB PHY driver contains a lookup table
>>> (`reg_bits_map_tables`) to resolve register bitmaps unique to certain
>>> versions of the USB PHY as found in various Broadcom chip families. A
>>> recent commit (see 'fixes' tag) introduced two new elements to each chip
>>> family in this table -- except for one: BCM4908. This resulted in the
>>> xHCI controller not being initialized correctly, causing a panic on
>>> boot.
>>
>> Yes, I think I see what happened here, we took the patch in the "fixes"
>> tag from the our downstream tree, and it applied just fine, we will keep
>> a closer eye on other entries in the future.
>>
>>>
>>> The next patch will update this table to use designated initializers in
>>> order to prevent this from happening again. For now, just add back the
>>> missing array elements to resolve the regression.
>>
>> Out of curiosity, can you check whether building with
>> -Wmissing-field-initializers would have caught this?
> 
> It appears that the answer is no, at least here on Clang. I also just
> tried -Wextra to see if any warning would catch it and didn't see one.
> My understanding is that -Wmissing-field-initializers is for struct
> fields, and a construct like:
> int array[3] = {1, 2};
> ...does not result in a warning because it's considered perfectly
> valid standards-compliant C per C's default initialization rule.

I suspected that much, thanks for having checked!
-- 
Florian