[PATCH v3 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2

[PATCH v3 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

Start by reading the content of the VENDOR_SPECIFIC2 register and update
each bit field based on device properties when defined.

The use of bit masks prevents fields from overriding each other and
enables users to clear bits which are set by default, like datapolarity
in this instance.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
Changes since v1:
- use set_mask_bits

Changes since v2:
- fix missing bit shift dropped in v2
- rebase on 5.9-rc1

 drivers/phy/ti/phy-tusb1210.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
index d8d0cc11d187..358842b5790f 100644
--- a/drivers/phy/ti/phy-tusb1210.c
+++ b/drivers/phy/ti/phy-tusb1210.c
@@ -14,8 +14,11 @@
 
 #define TUSB1210_VENDOR_SPECIFIC2		0x80
 #define TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT	0
+#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK	GENMASK(3, 0)
 #define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT	4
+#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK	GENMASK(5, 4)
 #define TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT	6
+#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK	BIT(6)
 
 struct tusb1210 {
 	struct ulpi *ulpi;
@@ -118,23 +121,27 @@ static int tusb1210_probe(struct ulpi *ulpi)
 	 * diagram optimization and DP/DM swap.
 	 */
 
+	reg = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
+
 	/* High speed output drive strength configuration */
-	device_property_read_u8(&ulpi->dev, "ihstx", &val);
-	reg = val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT;
+	if (!device_property_read_u8(&ulpi->dev, "ihstx", &val))
+		reg = set_mask_bits(&reg, TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK,
+				    val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT);
 
 	/* High speed output impedance configuration */
-	device_property_read_u8(&ulpi->dev, "zhsdrv", &val);
-	reg |= val << TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT;
+	if (!device_property_read_u8(&ulpi->dev, "zhsdrv", &val))
+		reg = set_mask_bits(&reg, TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK,
+				    val << TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT);
 
 	/* DP/DM swap control */
-	device_property_read_u8(&ulpi->dev, "datapolarity", &val);
-	reg |= val << TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT;
-
-	if (reg) {
-		ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
-		tusb->vendor_specific2 = reg;
+	if (!device_property_read_u8(&ulpi->dev, "datapolarity", &val)) {
+		reg = set_mask_bits(&reg, TUSB1210_VENDOR_SPECIFIC2_DP_MASK,
+				    val << TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT);
 	}
 
+	ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
+	tusb->vendor_specific2 = reg;
+
 	tusb->phy = ulpi_phy_create(ulpi, &phy_ops);
 	if (IS_ERR(tusb->phy))
 		return PTR_ERR(tusb->phy);

base-commit: 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
-- 
2.27.0

Re: [PATCH v3 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

Hi Liam,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5]

url:    https://github.com/0day-ci/linux/commits/Liam-Beguin/phy-tusb1210-use-bitmasks-to-set-VENDOR_SPECIFIC2/20200818-013719
base:    9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

ERROR: modpost: "__delay" [drivers/net/phy/mdio-cavium.ko] undefined!
>> ERROR: modpost: "__cmpxchg_called_with_bad_pointer" [drivers/phy/ti/phy-tusb1210.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52700 bytes --]

Re: [PATCH v3 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2

[Geert Uytterhoeven]

Hi Liam,


On Mon, Aug 17, 2020 at 7:38 PM Liam Beguin <liambeguin@gmail.com> wrote:
> From: Liam Beguin <lvb@xiphos.com>
>
> Start by reading the content of the VENDOR_SPECIFIC2 register and update
> each bit field based on device properties when defined.
>
> The use of bit masks prevents fields from overriding each other and
> enables users to clear bits which are set by default, like datapolarity
> in this instance.
>
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
> Changes since v1:
> - use set_mask_bits
>
> Changes since v2:
> - fix missing bit shift dropped in v2
> - rebase on 5.9-rc1
>
>  drivers/phy/ti/phy-tusb1210.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> index d8d0cc11d187..358842b5790f 100644
> --- a/drivers/phy/ti/phy-tusb1210.c
> +++ b/drivers/phy/ti/phy-tusb1210.c
> @@ -14,8 +14,11 @@
>
>  #define TUSB1210_VENDOR_SPECIFIC2              0x80
>  #define TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT  0
> +#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK   GENMASK(3, 0)
>  #define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT 4
> +#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK  GENMASK(5, 4)
>  #define TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT     6
> +#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK      BIT(6)
>
>  struct tusb1210 {
>         struct ulpi *ulpi;
> @@ -118,23 +121,27 @@ static int tusb1210_probe(struct ulpi *ulpi)
>          * diagram optimization and DP/DM swap.
>          */
>
> +       reg = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> +
>         /* High speed output drive strength configuration */
> -       device_property_read_u8(&ulpi->dev, "ihstx", &val);
> -       reg = val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT;
> +       if (!device_property_read_u8(&ulpi->dev, "ihstx", &val))
> +               reg = set_mask_bits(&reg, TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK,
> +                                   val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT);

Triggered by your patches to add support for cmpxchg on u8 pointers to
various architectures (which is a valuable goal in itself ;-), I decided
to have a look at the underlying problem you were facing.

IMHO, using set_mask_bits() is overkill here.  That helper is meant to
update individual bits in a variable that may be accessed concurrently,
hence its use of cmpxchg().

In this driver, you just want to modify a local variable, by clearing a
field, and setting some bits. No concurrency is involved.
Perhaps using FIELD_PREP() from include/linux/bitfield.h is more
appropriate?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2

[Vinod Koul]

On 19-08-20, 15:57, Geert Uytterhoeven wrote:
> Hi Liam,
> 
> 
> On Mon, Aug 17, 2020 at 7:38 PM Liam Beguin <liambeguin@gmail.com> wrote:
> > From: Liam Beguin <lvb@xiphos.com>
> >
> > Start by reading the content of the VENDOR_SPECIFIC2 register and update
> > each bit field based on device properties when defined.
> >
> > The use of bit masks prevents fields from overriding each other and
> > enables users to clear bits which are set by default, like datapolarity
> > in this instance.
> >
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> > Changes since v1:
> > - use set_mask_bits
> >
> > Changes since v2:
> > - fix missing bit shift dropped in v2
> > - rebase on 5.9-rc1
> >
> >  drivers/phy/ti/phy-tusb1210.c | 27 +++++++++++++++++----------
> >  1 file changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> > index d8d0cc11d187..358842b5790f 100644
> > --- a/drivers/phy/ti/phy-tusb1210.c
> > +++ b/drivers/phy/ti/phy-tusb1210.c
> > @@ -14,8 +14,11 @@
> >
> >  #define TUSB1210_VENDOR_SPECIFIC2              0x80
> >  #define TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT  0
> > +#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK   GENMASK(3, 0)
> >  #define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT 4
> > +#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK  GENMASK(5, 4)
> >  #define TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT     6
> > +#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK      BIT(6)
> >
> >  struct tusb1210 {
> >         struct ulpi *ulpi;
> > @@ -118,23 +121,27 @@ static int tusb1210_probe(struct ulpi *ulpi)
> >          * diagram optimization and DP/DM swap.
> >          */
> >
> > +       reg = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> > +
> >         /* High speed output drive strength configuration */
> > -       device_property_read_u8(&ulpi->dev, "ihstx", &val);
> > -       reg = val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT;
> > +       if (!device_property_read_u8(&ulpi->dev, "ihstx", &val))
> > +               reg = set_mask_bits(&reg, TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK,
> > +                                   val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT);
> 
> Triggered by your patches to add support for cmpxchg on u8 pointers to
> various architectures (which is a valuable goal in itself ;-), I decided
> to have a look at the underlying problem you were facing.
> 
> IMHO, using set_mask_bits() is overkill here.  That helper is meant to
> update individual bits in a variable that may be accessed concurrently,
> hence its use of cmpxchg().
> 
> In this driver, you just want to modify a local variable, by clearing a
> field, and setting some bits. No concurrency is involved.
> Perhaps using FIELD_PREP() from include/linux/bitfield.h is more
> appropriate?

Yeah i discovered the include/linux/bitfield.h and yes that looks more
apt here. u32_encode_bits() would make sense here and get rid of mask
and shift stuff too.

-- 
~Vinod

Re: [PATCH v3 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2

[Liam Beguin]

Hi Geert,

On Wed Aug 19, 2020 at 9:57 AM EDT, Geert Uytterhoeven wrote:
> Hi Liam,
>
>
> On Mon, Aug 17, 2020 at 7:38 PM Liam Beguin <liambeguin@gmail.com>
> wrote:
> > From: Liam Beguin <lvb@xiphos.com>
> >
> > Start by reading the content of the VENDOR_SPECIFIC2 register and update
> > each bit field based on device properties when defined.
> >
> > The use of bit masks prevents fields from overriding each other and
> > enables users to clear bits which are set by default, like datapolarity
> > in this instance.
> >
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> > Changes since v1:
> > - use set_mask_bits
> >
> > Changes since v2:
> > - fix missing bit shift dropped in v2
> > - rebase on 5.9-rc1
> >
> >  drivers/phy/ti/phy-tusb1210.c | 27 +++++++++++++++++----------
> >  1 file changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> > index d8d0cc11d187..358842b5790f 100644
> > --- a/drivers/phy/ti/phy-tusb1210.c
> > +++ b/drivers/phy/ti/phy-tusb1210.c
> > @@ -14,8 +14,11 @@
> >
> >  #define TUSB1210_VENDOR_SPECIFIC2              0x80
> >  #define TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT  0
> > +#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK   GENMASK(3, 0)
> >  #define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT 4
> > +#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK  GENMASK(5, 4)
> >  #define TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT     6
> > +#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK      BIT(6)
> >
> >  struct tusb1210 {
> >         struct ulpi *ulpi;
> > @@ -118,23 +121,27 @@ static int tusb1210_probe(struct ulpi *ulpi)
> >          * diagram optimization and DP/DM swap.
> >          */
> >
> > +       reg = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> > +
> >         /* High speed output drive strength configuration */
> > -       device_property_read_u8(&ulpi->dev, "ihstx", &val);
> > -       reg = val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT;
> > +       if (!device_property_read_u8(&ulpi->dev, "ihstx", &val))
> > +               reg = set_mask_bits(&reg, TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK,
> > +                                   val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT);
>
> Triggered by your patches to add support for cmpxchg on u8 pointers to
> various architectures (which is a valuable goal in itself ;-), I decided
> to have a look at the underlying problem you were facing.
>

Thanks for taking the time to look at this, I'll still give the SuperH
patch a try if I can test it properly :-).

> IMHO, using set_mask_bits() is overkill here. That helper is meant to
> update individual bits in a variable that may be accessed concurrently,
> hence its use of cmpxchg().
>
> In this driver, you just want to modify a local variable, by clearing a
> field, and setting some bits. No concurrency is involved.
> Perhaps using FIELD_PREP() from include/linux/bitfield.h is more
> appropriate?
>

Using set_mask_bits() does seem overkill seeing that it triggers all
these kbot warnings...
I'll prepare a patch using functions from bitfield.h instead.
Thanks again,

Liam

> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker.
> But
> when I'm talking to journalists I just say "programmer" or something
> like that.
> -- Linus Torvalds