| Message ID | 20191107204645.13739-1-rikard.falkeborn@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 96b4ea324ae92386db2b0c73ace597c80cde1ecb |
| Headers | show |
| Series | phy: allwinner: Fix GENMASK misuse | expand |
Hello Rikard, On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote: > Arguments are supposed to be ordered high then low. > > Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com> > --- > Spotted while trying to add compile time checks of GENMASK arguments. > Patch has only been compile tested. thank you! Tested-by: Ondrej Jirman <megous@megous.com> regards, o. > drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c b/drivers/phy/allwinner/phy-sun50i-usb3.c > index 1169f3e83a6f..b1c04f71a31d 100644 > --- a/drivers/phy/allwinner/phy-sun50i-usb3.c > +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c > @@ -49,7 +49,7 @@ > #define SUNXI_LOS_BIAS(n) ((n) << 3) > #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3) > #define SUNXI_TXVBOOSTLVL(n) ((n) << 0) > -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2) > +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0) > > struct sun50i_usb3_phy { > struct phy *phy; > -- > 2.24.0 >
On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote: > Arguments are supposed to be ordered high then low. > > Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com> > --- > Spotted while trying to add compile time checks of GENMASK arguments. > Patch has only been compile tested. My feeling, personally, is that GENMASK() really isn't worth the pain it causes. Can we instead get rid of this thing and just use easier to understand and less error-prone hex masks please? I don't care what anyone else says, personally I'm going to stick with using hex masks as I find them way easier to get right first time than a problematical opaque macro - and I really don't want the effort of finding out that I've got the arguments wrong when I build it. It's just _way_ easier and less error prone to use a hex mask straight off.
On Thu, 2019-11-07 at 23:39 +0000, Russell King - ARM Linux admin wrote: > On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote: > > Arguments are supposed to be ordered high then low. > > > > Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com> > > --- > > Spotted while trying to add compile time checks of GENMASK arguments. > > Patch has only been compile tested. > > My feeling, personally, is that GENMASK() really isn't worth the pain > it causes. Can we instead get rid of this thing and just use easier > to understand and less error-prone hex masks please? > > I don't care what anyone else says, personally I'm going to stick with > using hex masks as I find them way easier to get right first time than > a problematical opaque macro - and I really don't want the effort of > finding out that I've got the arguments wrong when I build it. It's > just _way_ easier and less error prone to use a hex mask straight off. I agree, but there are already more than 8000 uses of this rather silly (and perhaps backwards argument order) macro in the kernel.
Hi, On Thu 07 Nov 19, 23:39, Russell King - ARM Linux admin wrote: > On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote: > > Arguments are supposed to be ordered high then low. > > > > Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com> > > --- > > Spotted while trying to add compile time checks of GENMASK arguments. > > Patch has only been compile tested. > > My feeling, personally, is that GENMASK() really isn't worth the pain > it causes. Can we instead get rid of this thing and just use easier > to understand and less error-prone hex masks please? One advantage it has is that is matches the order in which bit fields are usually given in datasheets, so I personally found that it makes verification of fields much more straightforward and immediate. My 2 cents are that it makes sense for hardware registers. Note that I have recently introduced a SHIFT_AND_MASK_BITS macro[0] for a V4L2 driver, that I (and Mauro) would like to move to linux/bits.h eventually. > I don't care what anyone else says, personally I'm going to stick with > using hex masks as I find them way easier to get right first time than > a problematical opaque macro - and I really don't want the effort of > finding out that I've got the arguments wrong when I build it. It's > just _way_ easier and less error prone to use a hex mask straight off. I guess it's a matter of personal habit. [0]: https://git.linuxtv.org/media_tree.git/commit/?id=06eff2150d4db991ca236f3d05a9dc0101475aea Cheers, Paul
于 2019年11月8日 GMT+08:00 上午5:45:14, "Ondřej Jirman" <megous@megous.com> 写到: >Hello Rikard, > >On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote: >> Arguments are supposed to be ordered high then low. >> >> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com> >> --- >> Spotted while trying to add compile time checks of GENMASK arguments. >> Patch has only been compile tested. > >thank you! > >Tested-by: Ondrej Jirman <megous@megous.com> Does it affect or fix the performance? > >regards, > o. > >> drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c >b/drivers/phy/allwinner/phy-sun50i-usb3.c >> index 1169f3e83a6f..b1c04f71a31d 100644 >> --- a/drivers/phy/allwinner/phy-sun50i-usb3.c >> +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c >> @@ -49,7 +49,7 @@ >> #define SUNXI_LOS_BIAS(n) ((n) << 3) >> #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3) >> #define SUNXI_TXVBOOSTLVL(n) ((n) << 0) >> -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2) >> +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0) >> >> struct sun50i_usb3_phy { >> struct phy *phy; >> -- >> 2.24.0 >>
On Fri, Nov 08, 2019 at 07:29:21PM +0800, Icenowy Zheng wrote: > > > 于 2019年11月8日 GMT+08:00 上午5:45:14, "Ondřej Jirman" <megous@megous.com> 写到: > >Hello Rikard, > > > >On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote: > >> Arguments are supposed to be ordered high then low. > >> > >> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com> > >> --- > >> Spotted while trying to add compile time checks of GENMASK arguments. > >> Patch has only been compile tested. > > > >thank you! > > > >Tested-by: Ondrej Jirman <megous@megous.com> > > Does it affect or fix the performance? See here: https://forum.armbian.com/topic/10131-orange-pi-lite2-usb3-now-working/?do=findComment&comment=88904 Quote: > It may or may not help. On Opi3 I see no change, probably because HUB is > really close to the SoC, but on boards without a HUB, SoC's USB3 phy will > have to drive the signal over the longer cable and this patch might benefit > those boards. Maybe someone with boards without PHY will test it more. regards, o. > > > >regards, > > o. > > > >> drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c > >b/drivers/phy/allwinner/phy-sun50i-usb3.c > >> index 1169f3e83a6f..b1c04f71a31d 100644 > >> --- a/drivers/phy/allwinner/phy-sun50i-usb3.c > >> +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c > >> @@ -49,7 +49,7 @@ > >> #define SUNXI_LOS_BIAS(n) ((n) << 3) > >> #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3) > >> #define SUNXI_TXVBOOSTLVL(n) ((n) << 0) > >> -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2) > >> +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0) > >> > >> struct sun50i_usb3_phy { > >> struct phy *phy; > >> -- > >> 2.24.0 > >>
On Fri, Nov 08, 2019 at 12:41:39PM +0100, megous hlavni wrote: > On Fri, Nov 08, 2019 at 07:29:21PM +0800, Icenowy Zheng wrote: > > > > > > 于 2019年11月8日 GMT+08:00 上午5:45:14, "Ondřej Jirman" <megous@megous.com> 写到: > > >Hello Rikard, > > > > > >On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote: > > >> Arguments are supposed to be ordered high then low. > > >> > > >> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com> > > >> --- > > >> Spotted while trying to add compile time checks of GENMASK arguments. > > >> Patch has only been compile tested. > > > > > >thank you! > > > > > >Tested-by: Ondrej Jirman <megous@megous.com> > > > > Does it affect or fix the performance? > > See here: https://forum.armbian.com/topic/10131-orange-pi-lite2-usb3-now-working/?do=findComment&comment=88904 > > Quote: > > > It may or may not help. On Opi3 I see no change, probably because HUB is > > really close to the SoC, but on boards without a HUB, SoC's USB3 phy will > > have to drive the signal over the longer cable and this patch might benefit > > those boards. > > Maybe someone with boards without PHY will test it more. Eh, on boards without a USB3 HUB. > regards, > o. > > > > > > >regards, > > > o. > > > > > >> drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c > > >b/drivers/phy/allwinner/phy-sun50i-usb3.c > > >> index 1169f3e83a6f..b1c04f71a31d 100644 > > >> --- a/drivers/phy/allwinner/phy-sun50i-usb3.c > > >> +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c > > >> @@ -49,7 +49,7 @@ > > >> #define SUNXI_LOS_BIAS(n) ((n) << 3) > > >> #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3) > > >> #define SUNXI_TXVBOOSTLVL(n) ((n) << 0) > > >> -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2) > > >> +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0) > > >> > > >> struct sun50i_usb3_phy { > > >> struct phy *phy; > > >> -- > > >> 2.24.0 > > >>
diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c b/drivers/phy/allwinner/phy-sun50i-usb3.c index 1169f3e83a6f..b1c04f71a31d 100644 --- a/drivers/phy/allwinner/phy-sun50i-usb3.c +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c @@ -49,7 +49,7 @@ #define SUNXI_LOS_BIAS(n) ((n) << 3) #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3) #define SUNXI_TXVBOOSTLVL(n) ((n) << 0) -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2) +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0) struct sun50i_usb3_phy { struct phy *phy;
Arguments are supposed to be ordered high then low. Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com> --- Spotted while trying to add compile time checks of GENMASK arguments. Patch has only been compile tested. drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)