| Message ID | 20241004034131.1363813-2-CFSworks@gmail.com |
|---|---|
| State | Accepted |
| Commit | cb4c7df596a9048a6025e96e62fe698f15ec1992 |
| Headers | show |
| Series | phy: usb: Broadcom BCM4908 USB init fixes | expand |
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 */
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 */
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
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!
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 */
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(+)