| Message ID | 1386570664-6713-10-git-send-email-peter.chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/09/2013 07:30 AM, Peter Chen wrote: > Some PHY bugs are fixed by IC logic, but these bits are not > enabled by default, so we enable them at driver. Which bugs are fixed by enabling this bit? Is it only suspend related? Can you document them or better add a pointer to the documentation. Further I don't like the idea of adding code, or enabling a feature on certain hardware, that is broken in the first place and fixing it in a later patch. Think about squashing it into the correct patch. > Signed-off-by: Peter Chen <peter.chen@freescale.com> > --- > drivers/usb/phy/phy-mxs-usb.c | 28 ++++++++++++++++++++++++++-- > 1 files changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c > index e3df53f..d1c319b 100644 > --- a/drivers/usb/phy/phy-mxs-usb.c > +++ b/drivers/usb/phy/phy-mxs-usb.c > @@ -31,6 +31,10 @@ > #define HW_USBPHY_CTRL_SET 0x34 > #define HW_USBPHY_CTRL_CLR 0x38 > > +#define HW_USBPHY_IP 0x90 > +#define HW_USBPHY_IP_SET 0x94 > +#define HW_USBPHY_IP_CLR 0x98 > + > #define BM_USBPHY_CTRL_SFTRST BIT(31) > #define BM_USBPHY_CTRL_CLKGATE BIT(30) > #define BM_USBPHY_CTRL_ENAUTOSET_USBCLKS BIT(26) > @@ -42,6 +46,8 @@ > #define BM_USBPHY_CTRL_ENUTMILEVEL2 BIT(14) > #define BM_USBPHY_CTRL_ENHOSTDISCONDETECT BIT(1) > > +#define BM_USBPHY_IP_FIX (BIT(17) | BIT(18)) > + > #define to_mxs_phy(p) container_of((p), struct mxs_phy, phy) > > /* Do disconnection between PHY and controller without vbus */ > @@ -63,6 +69,9 @@ > /* The SoCs who have anatop module */ > #define MXS_PHY_HAS_ANATOP BIT(3) > > +/* IC has bug fixes logic */ > +#define MXS_PHY_NEED_IP_FIX BIT(4) > + > struct mxs_phy_data { > unsigned int flags; > }; > @@ -74,12 +83,14 @@ static const struct mxs_phy_data imx23_phy_data = { > static const struct mxs_phy_data imx6q_phy_data = { > .flags = MXS_PHY_SENDING_SOF_TOO_FAST | > MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS | > - MXS_PHY_HAS_ANATOP, > + MXS_PHY_HAS_ANATOP | > + MXS_PHY_NEED_IP_FIX, > }; > > static const struct mxs_phy_data imx6sl_phy_data = { > .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS | > - MXS_PHY_HAS_ANATOP, > + MXS_PHY_HAS_ANATOP | > + MXS_PHY_NEED_IP_FIX, > }; > > static const struct of_device_id mxs_phy_dt_ids[] = { > @@ -97,6 +108,16 @@ struct mxs_phy { > struct regmap *regmap_anatop; > }; > > +static inline bool is_imx6q_phy(struct mxs_phy *mxs_phy) > +{ > + return mxs_phy->data == &imx6q_phy_data; > +} > + > +static inline bool is_imx6sl_phy(struct mxs_phy *mxs_phy) > +{ > + return mxs_phy->data == &imx6sl_phy_data; > +} Are the two is_imx6* functions still needed? > + > static int mxs_phy_hw_init(struct mxs_phy *mxs_phy) > { > int ret; > @@ -123,6 +144,9 @@ static int mxs_phy_hw_init(struct mxs_phy *mxs_phy) > BM_USBPHY_CTRL_ENUTMILEVEL3, > base + HW_USBPHY_CTRL_SET); > > + if (mxs_phy->data->flags & MXS_PHY_NEED_IP_FIX) > + writel(BM_USBPHY_IP_FIX, base + HW_USBPHY_IP_SET); > + > return 0; > } Marc
On Mon, Dec 09, 2013 at 09:38:17AM +0100, Marc Kleine-Budde wrote: > On 12/09/2013 07:30 AM, Peter Chen wrote: > > Some PHY bugs are fixed by IC logic, but these bits are not > > enabled by default, so we enable them at driver. > > Which bugs are fixed by enabling this bit? Is it only suspend related? > Can you document them or better add a pointer to the documentation. I will add more, in fact, it fixes the bug which flag BIT(1) and BIT(2) stands for. > > Further I don't like the idea of adding code, or enabling a feature on > certain hardware, that is broken in the first place and fixing it in a > later patch. Think about squashing it into the correct patch. No fixes are related with this patch, you can see there is no "-" at this patch. > > > > +static inline bool is_imx6q_phy(struct mxs_phy *mxs_phy) > > +{ > > + return mxs_phy->data == &imx6q_phy_data; > > +} > > + > > +static inline bool is_imx6sl_phy(struct mxs_phy *mxs_phy) > > +{ > > + return mxs_phy->data == &imx6sl_phy_data; > > +} > > Are the two is_imx6* functions still needed? Will more it to [14/15], they are needed there. Peter > > > + > > static int mxs_phy_hw_init(struct mxs_phy *mxs_phy) > > { > > int ret; > > @@ -123,6 +144,9 @@ static int mxs_phy_hw_init(struct mxs_phy *mxs_phy) > > BM_USBPHY_CTRL_ENUTMILEVEL3, > > base + HW_USBPHY_CTRL_SET); > > > > + if (mxs_phy->data->flags & MXS_PHY_NEED_IP_FIX) > > + writel(BM_USBPHY_IP_FIX, base + HW_USBPHY_IP_SET); > > + > > return 0; > > } > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions | Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | >
On 12/09/2013 10:07 AM, Peter Chen wrote: > On Mon, Dec 09, 2013 at 09:38:17AM +0100, Marc Kleine-Budde wrote: >> On 12/09/2013 07:30 AM, Peter Chen wrote: >>> Some PHY bugs are fixed by IC logic, but these bits are not >>> enabled by default, so we enable them at driver. >> >> Which bugs are fixed by enabling this bit? Is it only suspend related? >> Can you document them or better add a pointer to the documentation. > > I will add more, in fact, it fixes the bug which flag BIT(1) and BIT(2) > stands for. > >> >> Further I don't like the idea of adding code, or enabling a feature on >> certain hardware, that is broken in the first place and fixing it in a >> later patch. Think about squashing it into the correct patch. > > No fixes are related with this patch, you can see there is no "-" > at this patch. Yes, there isn't any broken code (thus no "-"), but you first enable a feature in the hardware and in a later patch (this one) make it work properly. Marc
On Mon, Dec 09, 2013 at 10:52:33AM +0100, Marc Kleine-Budde wrote: > On 12/09/2013 10:07 AM, Peter Chen wrote: > > On Mon, Dec 09, 2013 at 09:38:17AM +0100, Marc Kleine-Budde wrote: > >> On 12/09/2013 07:30 AM, Peter Chen wrote: > >>> Some PHY bugs are fixed by IC logic, but these bits are not > >>> enabled by default, so we enable them at driver. > >> > >> Which bugs are fixed by enabling this bit? Is it only suspend related? > >> Can you document them or better add a pointer to the documentation. > > > > I will add more, in fact, it fixes the bug which flag BIT(1) and BIT(2) > > stands for. > > > >> > >> Further I don't like the idea of adding code, or enabling a feature on > >> certain hardware, that is broken in the first place and fixing it in a > >> later patch. Think about squashing it into the correct patch. > > > > No fixes are related with this patch, you can see there is no "-" > > at this patch. > > Yes, there isn't any broken code (thus no "-"), but you first enable a > feature in the hardware and in a later patch (this one) make it work > properly. > Sorry? I haven't enabled related hardware feature at previous patches. These two bits are enable bit, we just need to enable it.
diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c index e3df53f..d1c319b 100644 --- a/drivers/usb/phy/phy-mxs-usb.c +++ b/drivers/usb/phy/phy-mxs-usb.c @@ -31,6 +31,10 @@ #define HW_USBPHY_CTRL_SET 0x34 #define HW_USBPHY_CTRL_CLR 0x38 +#define HW_USBPHY_IP 0x90 +#define HW_USBPHY_IP_SET 0x94 +#define HW_USBPHY_IP_CLR 0x98 + #define BM_USBPHY_CTRL_SFTRST BIT(31) #define BM_USBPHY_CTRL_CLKGATE BIT(30) #define BM_USBPHY_CTRL_ENAUTOSET_USBCLKS BIT(26) @@ -42,6 +46,8 @@ #define BM_USBPHY_CTRL_ENUTMILEVEL2 BIT(14) #define BM_USBPHY_CTRL_ENHOSTDISCONDETECT BIT(1) +#define BM_USBPHY_IP_FIX (BIT(17) | BIT(18)) + #define to_mxs_phy(p) container_of((p), struct mxs_phy, phy) /* Do disconnection between PHY and controller without vbus */ @@ -63,6 +69,9 @@ /* The SoCs who have anatop module */ #define MXS_PHY_HAS_ANATOP BIT(3) +/* IC has bug fixes logic */ +#define MXS_PHY_NEED_IP_FIX BIT(4) + struct mxs_phy_data { unsigned int flags; }; @@ -74,12 +83,14 @@ static const struct mxs_phy_data imx23_phy_data = { static const struct mxs_phy_data imx6q_phy_data = { .flags = MXS_PHY_SENDING_SOF_TOO_FAST | MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS | - MXS_PHY_HAS_ANATOP, + MXS_PHY_HAS_ANATOP | + MXS_PHY_NEED_IP_FIX, }; static const struct mxs_phy_data imx6sl_phy_data = { .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS | - MXS_PHY_HAS_ANATOP, + MXS_PHY_HAS_ANATOP | + MXS_PHY_NEED_IP_FIX, }; static const struct of_device_id mxs_phy_dt_ids[] = { @@ -97,6 +108,16 @@ struct mxs_phy { struct regmap *regmap_anatop; }; +static inline bool is_imx6q_phy(struct mxs_phy *mxs_phy) +{ + return mxs_phy->data == &imx6q_phy_data; +} + +static inline bool is_imx6sl_phy(struct mxs_phy *mxs_phy) +{ + return mxs_phy->data == &imx6sl_phy_data; +} + static int mxs_phy_hw_init(struct mxs_phy *mxs_phy) { int ret; @@ -123,6 +144,9 @@ static int mxs_phy_hw_init(struct mxs_phy *mxs_phy) BM_USBPHY_CTRL_ENUTMILEVEL3, base + HW_USBPHY_CTRL_SET); + if (mxs_phy->data->flags & MXS_PHY_NEED_IP_FIX) + writel(BM_USBPHY_IP_FIX, base + HW_USBPHY_IP_SET); + return 0; }
Some PHY bugs are fixed by IC logic, but these bits are not enabled by default, so we enable them at driver. Signed-off-by: Peter Chen <peter.chen@freescale.com> --- drivers/usb/phy/phy-mxs-usb.c | 28 ++++++++++++++++++++++++++-- 1 files changed, 26 insertions(+), 2 deletions(-)