* [PATCH 0/1] USB: PHY: JZ4770: Next time, please test it
@ 2020-08-27 12:43 Paul Cercueil
2020-08-27 12:43 ` [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register Paul Cercueil
0 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2020-08-27 12:43 UTC (permalink / raw)
To: Felipe Balbi, 周琰杰, 周正,
漆鹏振
Cc: od, Greg Kroah-Hartman, linux-usb, linux-kernel, Paul Cercueil
Hi,
Here's a fix for the phy-jz4770 driver, which is broken in 5.9-rc2
starting from commit 2a6c0b82e651 ("USB: PHY: JZ4770: Add support for new Ingenic SoCs.").
I'm not amused by this, considering that this patch was obviously not
tested, yet it has a Tested-by tag. Next time, please test your patches
before sending them.
Thanks,
-Paul
Paul Cercueil (1):
USB: PHY: JZ4770: Fix uninitialized value written to HW register
drivers/usb/phy/phy-jz4770.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register
2020-08-27 12:43 [PATCH 0/1] USB: PHY: JZ4770: Next time, please test it Paul Cercueil
@ 2020-08-27 12:43 ` Paul Cercueil
2020-08-27 12:58 ` Felipe Balbi
0 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2020-08-27 12:43 UTC (permalink / raw)
To: Felipe Balbi, 周琰杰, 周正,
漆鹏振
Cc: od, Greg Kroah-Hartman, linux-usb, linux-kernel, Paul Cercueil
The 'reg' value was written to a hardware register in
ingenic_usb_phy_init(), while not being initialized anywhere.
Fixes: 2a6c0b82e651 ("USB: PHY: JZ4770: Add support for new Ingenic SoCs.")
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/usb/phy/phy-jz4770.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/phy/phy-jz4770.c b/drivers/usb/phy/phy-jz4770.c
index d4ee3cb721ea..58771a8688f2 100644
--- a/drivers/usb/phy/phy-jz4770.c
+++ b/drivers/usb/phy/phy-jz4770.c
@@ -97,7 +97,7 @@ enum ingenic_usb_phy_version {
struct ingenic_soc_info {
enum ingenic_usb_phy_version version;
- void (*usb_phy_init)(struct usb_phy *phy);
+ u32 (*usb_phy_init)(struct usb_phy *phy);
};
struct jz4770_phy {
@@ -172,7 +172,8 @@ static int ingenic_usb_phy_init(struct usb_phy *phy)
return err;
}
- priv->soc_info->usb_phy_init(phy);
+ reg = priv->soc_info->usb_phy_init(phy);
+ writel(reg, priv->base + REG_USBPCR_OFFSET);
/* Wait for PHY to reset */
usleep_range(30, 300);
@@ -195,19 +196,15 @@ static void ingenic_usb_phy_remove(void *phy)
usb_remove_phy(phy);
}
-static void jz4770_usb_phy_init(struct usb_phy *phy)
+static u32 jz4770_usb_phy_init(struct usb_phy *phy)
{
- struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
- u32 reg;
-
- reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS |
+ return USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS |
USBPCR_COMPDISTUNE_DFT | USBPCR_OTGTUNE_DFT | USBPCR_SQRXTUNE_DFT |
USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT | USBPCR_TXVREFTUNE_DFT |
USBPCR_POR;
- writel(reg, priv->base + REG_USBPCR_OFFSET);
}
-static void jz4780_usb_phy_init(struct usb_phy *phy)
+static u32 jz4780_usb_phy_init(struct usb_phy *phy)
{
struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
u32 reg;
@@ -216,11 +213,10 @@ static void jz4780_usb_phy_init(struct usb_phy *phy)
USBPCR1_WORD_IF_16BIT;
writel(reg, priv->base + REG_USBPCR1_OFFSET);
- reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
- writel(reg, priv->base + REG_USBPCR_OFFSET);
+ return USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
}
-static void x1000_usb_phy_init(struct usb_phy *phy)
+static u32 x1000_usb_phy_init(struct usb_phy *phy)
{
struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
u32 reg;
@@ -228,13 +224,12 @@ static void x1000_usb_phy_init(struct usb_phy *phy)
reg = readl(priv->base + REG_USBPCR1_OFFSET) | USBPCR1_WORD_IF_16BIT;
writel(reg, priv->base + REG_USBPCR1_OFFSET);
- reg = USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
+ return USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
USBPCR_COMMONONN | USBPCR_POR;
- writel(reg, priv->base + REG_USBPCR_OFFSET);
}
-static void x1830_usb_phy_init(struct usb_phy *phy)
+static u32 x1830_usb_phy_init(struct usb_phy *phy)
{
struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
u32 reg;
@@ -246,9 +241,8 @@ static void x1830_usb_phy_init(struct usb_phy *phy)
USBPCR1_DMPD | USBPCR1_DPPD;
writel(reg, priv->base + REG_USBPCR1_OFFSET);
- reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT | USBPCR_TXPREEMPHTUNE |
+ return USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT | USBPCR_TXPREEMPHTUNE |
USBPCR_COMMONONN | USBPCR_POR;
- writel(reg, priv->base + REG_USBPCR_OFFSET);
}
static const struct ingenic_soc_info jz4770_soc_info = {
--
2.28.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register
2020-08-27 12:43 ` [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register Paul Cercueil
@ 2020-08-27 12:58 ` Felipe Balbi
2020-08-27 13:11 ` Paul Cercueil
0 siblings, 1 reply; 9+ messages in thread
From: Felipe Balbi @ 2020-08-27 12:58 UTC (permalink / raw)
To: Paul Cercueil, 周琰杰, 周正,
漆鹏振
Cc: od, Greg Kroah-Hartman, linux-usb, linux-kernel, Paul Cercueil
[-- Attachment #1: Type: text/plain, Size: 3971 bytes --]
Hi,
Paul Cercueil <paul@crapouillou.net> writes:
> The 'reg' value was written to a hardware register in
> ingenic_usb_phy_init(), while not being initialized anywhere.
your patch does a lot more than fix the bug :-)
> Fixes: 2a6c0b82e651 ("USB: PHY: JZ4770: Add support for new Ingenic SoCs.")
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> drivers/usb/phy/phy-jz4770.c | 28 +++++++++++-----------------
> 1 file changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/phy/phy-jz4770.c b/drivers/usb/phy/phy-jz4770.c
> index d4ee3cb721ea..58771a8688f2 100644
> --- a/drivers/usb/phy/phy-jz4770.c
> +++ b/drivers/usb/phy/phy-jz4770.c
> @@ -97,7 +97,7 @@ enum ingenic_usb_phy_version {
> struct ingenic_soc_info {
> enum ingenic_usb_phy_version version;
>
> - void (*usb_phy_init)(struct usb_phy *phy);
> + u32 (*usb_phy_init)(struct usb_phy *phy);
this is not fixing any bug
> @@ -172,7 +172,8 @@ static int ingenic_usb_phy_init(struct usb_phy *phy)
> return err;
> }
>
> - priv->soc_info->usb_phy_init(phy);
> + reg = priv->soc_info->usb_phy_init(phy);
> + writel(reg, priv->base + REG_USBPCR_OFFSET);
not fixing any bug.
Looking at the code, the bug follows after this line. It would suffice
to read REG_USBPCR_OFFSET in order to initialize reg. This bug fix could
have been a one liner.
> @@ -195,19 +196,15 @@ static void ingenic_usb_phy_remove(void *phy)
> usb_remove_phy(phy);
> }
>
> -static void jz4770_usb_phy_init(struct usb_phy *phy)
> +static u32 jz4770_usb_phy_init(struct usb_phy *phy)
not a bug fix
> {
> - struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> - u32 reg;
> -
> - reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS |
> + return USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS |
> USBPCR_COMPDISTUNE_DFT | USBPCR_OTGTUNE_DFT | USBPCR_SQRXTUNE_DFT |
> USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT | USBPCR_TXVREFTUNE_DFT |
> USBPCR_POR;
> - writel(reg, priv->base + REG_USBPCR_OFFSET);
not a bug fix
> }
>
> -static void jz4780_usb_phy_init(struct usb_phy *phy)
> +static u32 jz4780_usb_phy_init(struct usb_phy *phy)
not a bug fix
> @@ -216,11 +213,10 @@ static void jz4780_usb_phy_init(struct usb_phy *phy)
> USBPCR1_WORD_IF_16BIT;
> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>
> - reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
> - writel(reg, priv->base + REG_USBPCR_OFFSET);
> + return USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
not a bug fix
> }
>
> -static void x1000_usb_phy_init(struct usb_phy *phy)
> +static u32 x1000_usb_phy_init(struct usb_phy *phy)
not a bug fix
> {
> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> u32 reg;
> @@ -228,13 +224,12 @@ static void x1000_usb_phy_init(struct usb_phy *phy)
> reg = readl(priv->base + REG_USBPCR1_OFFSET) | USBPCR1_WORD_IF_16BIT;
> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>
> - reg = USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
> + return USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
> USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
> USBPCR_COMMONONN | USBPCR_POR;
> - writel(reg, priv->base + REG_USBPCR_OFFSET);
not a bug fix
> }
>
> -static void x1830_usb_phy_init(struct usb_phy *phy)
> +static u32 x1830_usb_phy_init(struct usb_phy *phy)
not a bug fix
> {
> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> u32 reg;
> @@ -246,9 +241,8 @@ static void x1830_usb_phy_init(struct usb_phy *phy)
> USBPCR1_DMPD | USBPCR1_DPPD;
> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>
> - reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT | USBPCR_TXPREEMPHTUNE |
> + return USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT | USBPCR_TXPREEMPHTUNE |
> USBPCR_COMMONONN | USBPCR_POR;
> - writel(reg, priv->base + REG_USBPCR_OFFSET);
not a bug fix
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register
2020-08-27 12:58 ` Felipe Balbi
@ 2020-08-27 13:11 ` Paul Cercueil
2020-08-27 13:25 ` Felipe Balbi
0 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2020-08-27 13:11 UTC (permalink / raw)
To: Felipe Balbi
Cc: 周琰杰, 周正,
漆鹏振,
od, Greg Kroah-Hartman, linux-usb, linux-kernel
Hi Felipe,
Le jeu. 27 août 2020 à 15:58, Felipe Balbi <balbi@kernel.org> a
écrit :
>
> Hi,
>
> Paul Cercueil <paul@crapouillou.net> writes:
>> The 'reg' value was written to a hardware register in
>> ingenic_usb_phy_init(), while not being initialized anywhere.
>
> your patch does a lot more than fix the bug :-)
>
>> Fixes: 2a6c0b82e651 ("USB: PHY: JZ4770: Add support for new Ingenic
>> SoCs.")
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> ---
>> drivers/usb/phy/phy-jz4770.c | 28 +++++++++++-----------------
>> 1 file changed, 11 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/usb/phy/phy-jz4770.c
>> b/drivers/usb/phy/phy-jz4770.c
>> index d4ee3cb721ea..58771a8688f2 100644
>> --- a/drivers/usb/phy/phy-jz4770.c
>> +++ b/drivers/usb/phy/phy-jz4770.c
>> @@ -97,7 +97,7 @@ enum ingenic_usb_phy_version {
>> struct ingenic_soc_info {
>> enum ingenic_usb_phy_version version;
>>
>> - void (*usb_phy_init)(struct usb_phy *phy);
>> + u32 (*usb_phy_init)(struct usb_phy *phy);
>
> this is not fixing any bug
>
>> @@ -172,7 +172,8 @@ static int ingenic_usb_phy_init(struct usb_phy
>> *phy)
>> return err;
>> }
>>
>> - priv->soc_info->usb_phy_init(phy);
>> + reg = priv->soc_info->usb_phy_init(phy);
>> + writel(reg, priv->base + REG_USBPCR_OFFSET);
>
> not fixing any bug.
>
> Looking at the code, the bug follows after this line. It would suffice
> to read REG_USBPCR_OFFSET in order to initialize reg. This bug fix
> could
> have been a one liner.
There's no need to re-read a register when you have the value readily
available. It just needs to be returned from the usb_phy_init
callbacks. But yes, it's not a one-liner.
>
>> @@ -195,19 +196,15 @@ static void ingenic_usb_phy_remove(void *phy)
>> usb_remove_phy(phy);
>> }
>>
>> -static void jz4770_usb_phy_init(struct usb_phy *phy)
>> +static u32 jz4770_usb_phy_init(struct usb_phy *phy)
>
> not a bug fix
>
>> {
>> - struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>> - u32 reg;
>> -
>> - reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS
>> |
>> + return USBPCR_AVLD_REG | USBPCR_COMMONONN |
>> USBPCR_IDPULLUP_ALWAYS |
>> USBPCR_COMPDISTUNE_DFT | USBPCR_OTGTUNE_DFT |
>> USBPCR_SQRXTUNE_DFT |
>> USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT |
>> USBPCR_TXVREFTUNE_DFT |
>> USBPCR_POR;
>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>
> not a bug fix
>
>> }
>>
>> -static void jz4780_usb_phy_init(struct usb_phy *phy)
>> +static u32 jz4780_usb_phy_init(struct usb_phy *phy)
>
> not a bug fix
>
>> @@ -216,11 +213,10 @@ static void jz4780_usb_phy_init(struct
>> usb_phy *phy)
>> USBPCR1_WORD_IF_16BIT;
>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>
>> - reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>> + return USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>
> not a bug fix
>
>> }
>>
>> -static void x1000_usb_phy_init(struct usb_phy *phy)
>> +static u32 x1000_usb_phy_init(struct usb_phy *phy)
>
> not a bug fix
>
>> {
>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>> u32 reg;
>> @@ -228,13 +224,12 @@ static void x1000_usb_phy_init(struct usb_phy
>> *phy)
>> reg = readl(priv->base + REG_USBPCR1_OFFSET) |
>> USBPCR1_WORD_IF_16BIT;
>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>
>> - reg = USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>> + return USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>> USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
>> USBPCR_COMMONONN | USBPCR_POR;
>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>
> not a bug fix
>
>> }
>>
>> -static void x1830_usb_phy_init(struct usb_phy *phy)
>> +static u32 x1830_usb_phy_init(struct usb_phy *phy)
>
> not a bug fix
>
>> {
>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>> u32 reg;
>> @@ -246,9 +241,8 @@ static void x1830_usb_phy_init(struct usb_phy
>> *phy)
>> USBPCR1_DMPD | USBPCR1_DPPD;
>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>
>> - reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT
>> | USBPCR_TXPREEMPHTUNE |
>> + return USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT |
>> USBPCR_TXPREEMPHTUNE |
>> USBPCR_COMMONONN | USBPCR_POR;
>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>
> not a bug fix
Well, if you don't like my bug fix, next time wait for my Reviewed-by.
Cheers,
-Paul
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register
2020-08-27 13:11 ` Paul Cercueil
@ 2020-08-27 13:25 ` Felipe Balbi
2020-08-27 13:50 ` Paul Cercueil
2020-09-09 10:23 ` Pavel Machek
0 siblings, 2 replies; 9+ messages in thread
From: Felipe Balbi @ 2020-08-27 13:25 UTC (permalink / raw)
To: Paul Cercueil
Cc: 周琰杰, 周正,
漆鹏振,
od, Greg Kroah-Hartman, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4058 bytes --]
Hi,
Paul Cercueil <paul@crapouillou.net> writes:
>>> @@ -172,7 +172,8 @@ static int ingenic_usb_phy_init(struct usb_phy
>>> *phy)
>>> return err;
>>> }
>>>
>>> - priv->soc_info->usb_phy_init(phy);
>>> + reg = priv->soc_info->usb_phy_init(phy);
>>> + writel(reg, priv->base + REG_USBPCR_OFFSET);
>>
>> not fixing any bug.
>>
>> Looking at the code, the bug follows after this line. It would suffice
>> to read REG_USBPCR_OFFSET in order to initialize reg. This bug fix
>> could
>> have been a one liner.
>
> There's no need to re-read a register when you have the value readily
> available. It just needs to be returned from the usb_phy_init
> callbacks. But yes, it's not a one-liner.
there's a difference between making a bug fix and reworking the behavior
of the driver ;-)
>>> @@ -195,19 +196,15 @@ static void ingenic_usb_phy_remove(void *phy)
>>> usb_remove_phy(phy);
>>> }
>>>
>>> -static void jz4770_usb_phy_init(struct usb_phy *phy)
>>> +static u32 jz4770_usb_phy_init(struct usb_phy *phy)
>>
>> not a bug fix
>>
>>> {
>>> - struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> - u32 reg;
>>> -
>>> - reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS
>>> |
>>> + return USBPCR_AVLD_REG | USBPCR_COMMONONN |
>>> USBPCR_IDPULLUP_ALWAYS |
>>> USBPCR_COMPDISTUNE_DFT | USBPCR_OTGTUNE_DFT |
>>> USBPCR_SQRXTUNE_DFT |
>>> USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT |
>>> USBPCR_TXVREFTUNE_DFT |
>>> USBPCR_POR;
>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>
>> not a bug fix
>>
>>> }
>>>
>>> -static void jz4780_usb_phy_init(struct usb_phy *phy)
>>> +static u32 jz4780_usb_phy_init(struct usb_phy *phy)
>>
>> not a bug fix
>>
>>> @@ -216,11 +213,10 @@ static void jz4780_usb_phy_init(struct
>>> usb_phy *phy)
>>> USBPCR1_WORD_IF_16BIT;
>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>
>>> - reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>> + return USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>
>> not a bug fix
>>
>>> }
>>>
>>> -static void x1000_usb_phy_init(struct usb_phy *phy)
>>> +static u32 x1000_usb_phy_init(struct usb_phy *phy)
>>
>> not a bug fix
>>
>>> {
>>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> u32 reg;
>>> @@ -228,13 +224,12 @@ static void x1000_usb_phy_init(struct usb_phy
>>> *phy)
>>> reg = readl(priv->base + REG_USBPCR1_OFFSET) |
>>> USBPCR1_WORD_IF_16BIT;
>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>
>>> - reg = USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>>> + return USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>>> USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
>>> USBPCR_COMMONONN | USBPCR_POR;
>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>
>> not a bug fix
>>
>>> }
>>>
>>> -static void x1830_usb_phy_init(struct usb_phy *phy)
>>> +static u32 x1830_usb_phy_init(struct usb_phy *phy)
>>
>> not a bug fix
>>
>>> {
>>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> u32 reg;
>>> @@ -246,9 +241,8 @@ static void x1830_usb_phy_init(struct usb_phy
>>> *phy)
>>> USBPCR1_DMPD | USBPCR1_DPPD;
>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>
>>> - reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT
>>> | USBPCR_TXPREEMPHTUNE |
>>> + return USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT |
>>> USBPCR_TXPREEMPHTUNE |
>>> USBPCR_COMMONONN | USBPCR_POR;
>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>
>> not a bug fix
>
> Well, if you don't like my bug fix, next time wait for my Reviewed-by.
why so angry? Take a break every once in a while. Besides, someone else
already sent the oneliner before you ;-)
In any case, why should I wait for your Reviewed-by? Get maintainer
doesn't list you as the maintainer for it. Do you want to update
MAINTAINERS by any chance?
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register
2020-08-27 13:25 ` Felipe Balbi
@ 2020-08-27 13:50 ` Paul Cercueil
2020-08-27 14:26 ` Felipe Balbi
2020-08-27 15:35 ` Zhou Yanjie
2020-09-09 10:23 ` Pavel Machek
1 sibling, 2 replies; 9+ messages in thread
From: Paul Cercueil @ 2020-08-27 13:50 UTC (permalink / raw)
To: Felipe Balbi
Cc: 周琰杰, 周正,
漆鹏振,
od, Greg Kroah-Hartman, linux-usb, linux-kernel
Le jeu. 27 août 2020 à 16:25, Felipe Balbi <balbi@kernel.org> a
écrit :
>
> Hi,
>
> Paul Cercueil <paul@crapouillou.net> writes:
>>>> @@ -172,7 +172,8 @@ static int ingenic_usb_phy_init(struct
>>>> usb_phy
>>>> *phy)
>>>> return err;
>>>> }
>>>>
>>>> - priv->soc_info->usb_phy_init(phy);
>>>> + reg = priv->soc_info->usb_phy_init(phy);
>>>> + writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>
>>> not fixing any bug.
>>>
>>> Looking at the code, the bug follows after this line. It would
>>> suffice
>>> to read REG_USBPCR_OFFSET in order to initialize reg. This bug fix
>>> could
>>> have been a one liner.
>>
>> There's no need to re-read a register when you have the value
>> readily
>> available. It just needs to be returned from the usb_phy_init
>> callbacks. But yes, it's not a one-liner.
>
> there's a difference between making a bug fix and reworking the
> behavior
> of the driver ;-)
The one-liner is actually what changes the behavior of the driver,
since previously the code did not read back the register.
In this case I guess it's fine, because the register does not have
volatile bits.
>>>> @@ -195,19 +196,15 @@ static void ingenic_usb_phy_remove(void
>>>> *phy)
>>>> usb_remove_phy(phy);
>>>> }
>>>>
>>>> -static void jz4770_usb_phy_init(struct usb_phy *phy)
>>>> +static u32 jz4770_usb_phy_init(struct usb_phy *phy)
>>>
>>> not a bug fix
>>>
>>>> {
>>>> - struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>> - u32 reg;
>>>> -
>>>> - reg = USBPCR_AVLD_REG | USBPCR_COMMONONN |
>>>> USBPCR_IDPULLUP_ALWAYS
>>>> |
>>>> + return USBPCR_AVLD_REG | USBPCR_COMMONONN |
>>>> USBPCR_IDPULLUP_ALWAYS |
>>>> USBPCR_COMPDISTUNE_DFT | USBPCR_OTGTUNE_DFT |
>>>> USBPCR_SQRXTUNE_DFT |
>>>> USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT |
>>>> USBPCR_TXVREFTUNE_DFT |
>>>> USBPCR_POR;
>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>
>>> not a bug fix
>>>
>>>> }
>>>>
>>>> -static void jz4780_usb_phy_init(struct usb_phy *phy)
>>>> +static u32 jz4780_usb_phy_init(struct usb_phy *phy)
>>>
>>> not a bug fix
>>>
>>>> @@ -216,11 +213,10 @@ static void jz4780_usb_phy_init(struct
>>>> usb_phy *phy)
>>>> USBPCR1_WORD_IF_16BIT;
>>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>>
>>>> - reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>> + return USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>>
>>> not a bug fix
>>>
>>>> }
>>>>
>>>> -static void x1000_usb_phy_init(struct usb_phy *phy)
>>>> +static u32 x1000_usb_phy_init(struct usb_phy *phy)
>>>
>>> not a bug fix
>>>
>>>> {
>>>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>> u32 reg;
>>>> @@ -228,13 +224,12 @@ static void x1000_usb_phy_init(struct
>>>> usb_phy
>>>> *phy)
>>>> reg = readl(priv->base + REG_USBPCR1_OFFSET) |
>>>> USBPCR1_WORD_IF_16BIT;
>>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>>
>>>> - reg = USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>>>> + return USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>>>> USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
>>>> USBPCR_COMMONONN | USBPCR_POR;
>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>
>>> not a bug fix
>>>
>>>> }
>>>>
>>>> -static void x1830_usb_phy_init(struct usb_phy *phy)
>>>> +static u32 x1830_usb_phy_init(struct usb_phy *phy)
>>>
>>> not a bug fix
>>>
>>>> {
>>>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>> u32 reg;
>>>> @@ -246,9 +241,8 @@ static void x1830_usb_phy_init(struct usb_phy
>>>> *phy)
>>>> USBPCR1_DMPD | USBPCR1_DPPD;
>>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>>
>>>> - reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT
>>>> | USBPCR_TXPREEMPHTUNE |
>>>> + return USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT |
>>>> USBPCR_TXPREEMPHTUNE |
>>>> USBPCR_COMMONONN | USBPCR_POR;
>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>
>>> not a bug fix
>>
>> Well, if you don't like my bug fix, next time wait for my
>> Reviewed-by.
>
> why so angry? Take a break every once in a while. Besides, someone
> else
> already sent the oneliner before you ;-)
I'm just pissed that this patch has not been tested. I don't like
sloppy work.
> In any case, why should I wait for your Reviewed-by? Get maintainer
> doesn't list you as the maintainer for it. Do you want to update
> MAINTAINERS by any chance?
Yes, I thought I was (I'm maintainer of all Ingenic drivers), that also
explains why I wasn't Cc'd for the oneliner patch you mentioned...
IIRC Zhou has a patch to move the driver to drivers/phy/, I'll add
myself as maintainer once it's moved there.
-Paul
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register
2020-08-27 13:50 ` Paul Cercueil
@ 2020-08-27 14:26 ` Felipe Balbi
2020-08-27 15:35 ` Zhou Yanjie
1 sibling, 0 replies; 9+ messages in thread
From: Felipe Balbi @ 2020-08-27 14:26 UTC (permalink / raw)
To: Paul Cercueil
Cc: 周琰杰, 周正,
漆鹏振,
od, Greg Kroah-Hartman, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]
Hi,
Paul Cercueil <paul@crapouillou.net> writes:
>>>>> @@ -246,9 +241,8 @@ static void x1830_usb_phy_init(struct usb_phy
>>>>> *phy)
>>>>> USBPCR1_DMPD | USBPCR1_DPPD;
>>>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>>>
>>>>> - reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT
>>>>> | USBPCR_TXPREEMPHTUNE |
>>>>> + return USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT |
>>>>> USBPCR_TXPREEMPHTUNE |
>>>>> USBPCR_COMMONONN | USBPCR_POR;
>>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>>
>>>> not a bug fix
>>>
>>> Well, if you don't like my bug fix, next time wait for my
>>> Reviewed-by.
>>
>> why so angry? Take a break every once in a while. Besides, someone
>> else
>> already sent the oneliner before you ;-)
>
> I'm just pissed that this patch has not been tested. I don't like
> sloppy work.
yeah, s**t happens
>> In any case, why should I wait for your Reviewed-by? Get maintainer
>> doesn't list you as the maintainer for it. Do you want to update
>> MAINTAINERS by any chance?
>
> Yes, I thought I was (I'm maintainer of all Ingenic drivers), that also
> explains why I wasn't Cc'd for the oneliner patch you mentioned...
>
> IIRC Zhou has a patch to move the driver to drivers/phy/, I'll add
> myself as maintainer once it's moved there.
makes sense
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register
2020-08-27 13:50 ` Paul Cercueil
2020-08-27 14:26 ` Felipe Balbi
@ 2020-08-27 15:35 ` Zhou Yanjie
1 sibling, 0 replies; 9+ messages in thread
From: Zhou Yanjie @ 2020-08-27 15:35 UTC (permalink / raw)
To: Paul Cercueil, Felipe Balbi
Cc: 周正, 漆鹏振,
od, Greg Kroah-Hartman, linux-usb, linux-kernel
Hi
在 2020/8/27 下午9:50, Paul Cercueil 写道:
>
>
> Le jeu. 27 août 2020 à 16:25, Felipe Balbi <balbi@kernel.org> a écrit :
>>
>> Hi,
>>
>> Paul Cercueil <paul@crapouillou.net> writes:
>>>>> @@ -172,7 +172,8 @@ static int ingenic_usb_phy_init(struct usb_phy
>>>>> *phy)
>>>>> return err;
>>>>> }
>>>>>
>>>>> - priv->soc_info->usb_phy_init(phy);
>>>>> + reg = priv->soc_info->usb_phy_init(phy);
>>>>> + writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>>
>>>> not fixing any bug.
>>>>
>>>> Looking at the code, the bug follows after this line. It would
>>>> suffice
>>>> to read REG_USBPCR_OFFSET in order to initialize reg. This bug fix
>>>> could
>>>> have been a one liner.
>>>
>>> There's no need to re-read a register when you have the value readily
>>> available. It just needs to be returned from the usb_phy_init
>>> callbacks. But yes, it's not a one-liner.
>>
>> there's a difference between making a bug fix and reworking the behavior
>> of the driver ;-)
>
> The one-liner is actually what changes the behavior of the driver,
> since previously the code did not read back the register.
>
> In this case I guess it's fine, because the register does not have
> volatile bits.
>
>>>>> @@ -195,19 +196,15 @@ static void ingenic_usb_phy_remove(void *phy)
>>>>> usb_remove_phy(phy);
>>>>> }
>>>>>
>>>>> -static void jz4770_usb_phy_init(struct usb_phy *phy)
>>>>> +static u32 jz4770_usb_phy_init(struct usb_phy *phy)
>>>>
>>>> not a bug fix
>>>>
>>>>> {
>>>>> - struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>>> - u32 reg;
>>>>> -
>>>>> - reg = USBPCR_AVLD_REG | USBPCR_COMMONONN |
>>>>> USBPCR_IDPULLUP_ALWAYS
>>>>> |
>>>>> + return USBPCR_AVLD_REG | USBPCR_COMMONONN |
>>>>> USBPCR_IDPULLUP_ALWAYS |
>>>>> USBPCR_COMPDISTUNE_DFT | USBPCR_OTGTUNE_DFT |
>>>>> USBPCR_SQRXTUNE_DFT |
>>>>> USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT |
>>>>> USBPCR_TXVREFTUNE_DFT |
>>>>> USBPCR_POR;
>>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>>
>>>> not a bug fix
>>>>
>>>>> }
>>>>>
>>>>> -static void jz4780_usb_phy_init(struct usb_phy *phy)
>>>>> +static u32 jz4780_usb_phy_init(struct usb_phy *phy)
>>>>
>>>> not a bug fix
>>>>
>>>>> @@ -216,11 +213,10 @@ static void jz4780_usb_phy_init(struct
>>>>> usb_phy *phy)
>>>>> USBPCR1_WORD_IF_16BIT;
>>>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>>>
>>>>> - reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>>> + return USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>>>
>>>> not a bug fix
>>>>
>>>>> }
>>>>>
>>>>> -static void x1000_usb_phy_init(struct usb_phy *phy)
>>>>> +static u32 x1000_usb_phy_init(struct usb_phy *phy)
>>>>
>>>> not a bug fix
>>>>
>>>>> {
>>>>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>>> u32 reg;
>>>>> @@ -228,13 +224,12 @@ static void x1000_usb_phy_init(struct usb_phy
>>>>> *phy)
>>>>> reg = readl(priv->base + REG_USBPCR1_OFFSET) |
>>>>> USBPCR1_WORD_IF_16BIT;
>>>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>>>
>>>>> - reg = USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>>>>> + return USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>>>>> USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
>>>>> USBPCR_COMMONONN | USBPCR_POR;
>>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>>
>>>> not a bug fix
>>>>
>>>>> }
>>>>>
>>>>> -static void x1830_usb_phy_init(struct usb_phy *phy)
>>>>> +static u32 x1830_usb_phy_init(struct usb_phy *phy)
>>>>
>>>> not a bug fix
>>>>
>>>>> {
>>>>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>>> u32 reg;
>>>>> @@ -246,9 +241,8 @@ static void x1830_usb_phy_init(struct usb_phy
>>>>> *phy)
>>>>> USBPCR1_DMPD | USBPCR1_DPPD;
>>>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>>>
>>>>> - reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT
>>>>> | USBPCR_TXPREEMPHTUNE |
>>>>> + return USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT |
>>>>> USBPCR_TXPREEMPHTUNE |
>>>>> USBPCR_COMMONONN | USBPCR_POR;
>>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>>
>>>> not a bug fix
>>>
>>> Well, if you don't like my bug fix, next time wait for my Reviewed-by.
>>
>> why so angry? Take a break every once in a while. Besides, someone else
>> already sent the oneliner before you ;-)
>
> I'm just pissed that this patch has not been tested. I don't like
> sloppy work.
>
This is my fault. This error occurred in the v5 version, but was
corrected in the v6 version, but I don't know why v5 was merged into the
mainline and v6 was not merged, which caused this problem.
>> In any case, why should I wait for your Reviewed-by? Get maintainer
>> doesn't list you as the maintainer for it. Do you want to update
>> MAINTAINERS by any chance?
>
> Yes, I thought I was (I'm maintainer of all Ingenic drivers), that
> also explains why I wasn't Cc'd for the oneliner patch you mentioned...
>
I checked again, get_maintainer did not give the correct information,
which caused my script to not add Paul to the cc list.
> IIRC Zhou has a patch to move the driver to drivers/phy/, I'll add
> myself as maintainer once it's moved there.
>
I'll resend it soon.
Thanks and best regards!
> -Paul
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register
2020-08-27 13:25 ` Felipe Balbi
2020-08-27 13:50 ` Paul Cercueil
@ 2020-09-09 10:23 ` Pavel Machek
1 sibling, 0 replies; 9+ messages in thread
From: Pavel Machek @ 2020-09-09 10:23 UTC (permalink / raw)
To: Felipe Balbi
Cc: Paul Cercueil, 周琰杰, 周正,
漆鹏振,
od, Greg Kroah-Hartman, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 770 bytes --]
Hi!
> >>> - reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT
> >>> | USBPCR_TXPREEMPHTUNE |
> >>> + return USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT |
> >>> USBPCR_TXPREEMPHTUNE |
> >>> USBPCR_COMMONONN | USBPCR_POR;
> >>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
> >>
> >> not a bug fix
> >
> > Well, if you don't like my bug fix, next time wait for my Reviewed-by.
>
> why so angry? Take a break every once in a while. Besides, someone else
> already sent the oneliner before you ;-)
Your behaviour is pretty good explanation for angry people on the
mailing list.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-09-09 10:24 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-27 12:43 [PATCH 0/1] USB: PHY: JZ4770: Next time, please test it Paul Cercueil
2020-08-27 12:43 ` [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register Paul Cercueil
2020-08-27 12:58 ` Felipe Balbi
2020-08-27 13:11 ` Paul Cercueil
2020-08-27 13:25 ` Felipe Balbi
2020-08-27 13:50 ` Paul Cercueil
2020-08-27 14:26 ` Felipe Balbi
2020-08-27 15:35 ` Zhou Yanjie
2020-09-09 10:23 ` Pavel Machek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).