re: stmmac: add ipq806x glue layer

re: stmmac: add ipq806x glue layer

[Dan Carpenter]

Hello Mathieu Olivari,

The patch b1c17215d718: "stmmac: add ipq806x glue layer" from May 27,
2015, leads to the following static checker warning:

	drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c:297 ipq806x_gmac_setup()
	warn: double left shift '1 << (1 << gmac->id)'

drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
   292          /* Configure the clock src according to the mode */
   293          regmap_read(gmac->nss_common, NSS_COMMON_CLK_SRC_CTRL, &val);
   294          val &= ~NSS_COMMON_CLK_SRC_CTRL_OFFSET(gmac->id);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We are clearing a bit here, but it's not the same bit we set later.

   295          switch (gmac->phy_mode) {
   296          case PHY_INTERFACE_MODE_RGMII:
   297                  val |= NSS_COMMON_CLK_SRC_CTRL_RGMII(gmac->id) <<
   298                          NSS_COMMON_CLK_SRC_CTRL_OFFSET(gmac->id);

NSS_COMMON_CLK_SRC_CTRL_RGMII(gmac->id) is always 1.
NSS_COMMON_CLK_SRC_CTRL_OFFSET is "(1 << gmac->id)".
gmac->id is a number in the 0-3 range so val is either 0x100, 0x10, 0x4
or 0x2, which is kind of weird.

Also the comments seem to imply that we read a number 1-4 for gmac->id
but we map it to 0-3 so that counting starts at zero.  But I don't see
that we subtract 1 from the number that we read in
ipq806x_gmac_of_parse().

   299                  break;
   300          case PHY_INTERFACE_MODE_SGMII:
   301                  val |= NSS_COMMON_CLK_SRC_CTRL_SGMII(gmac->id) <<
   302                          NSS_COMMON_CLK_SRC_CTRL_OFFSET(gmac->id);
   303                  break;
   304          default:
   305                  dev_err(&pdev->dev, "Unsupported PHY mode: \"%s\"\n",
   306                          phy_modes(gmac->phy_mode));
   307                  return NULL;
   308          }
   309          regmap_write(gmac->nss_common, NSS_COMMON_CLK_SRC_CTRL, val);



regards,
dan carpenter

re: stmmac: add ipq806x glue layer

[Dan Carpenter]

Hello Mathieu Olivari,

The patch b1c17215d718: "stmmac: add ipq806x glue layer" from May 27,
2015, leads to the following static checker warning:

	drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c:314 ipq806x_gmac_probe()
	warn: double left shift '1 << (1 << gmac->id)'

drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
   312          switch (gmac->phy_mode) {
   313          case PHY_INTERFACE_MODE_RGMII:
   314                  val |= NSS_COMMON_CLK_SRC_CTRL_RGMII(gmac->id) <<
   315                          NSS_COMMON_CLK_SRC_CTRL_OFFSET(gmac->id);

It's very strange to shift to do this.  My guess is we should delete the
NSS_COMMON_CLK_SRC_CTRL_RGMII macro and just use:

			val |= NSS_COMMON_CLK_SRC_CTRL_OFFSET(gmac->id);

   316                  break;
   317          case PHY_INTERFACE_MODE_SGMII:
   318                  val |= NSS_COMMON_CLK_SRC_CTRL_SGMII(gmac->id) <<
   319                          NSS_COMMON_CLK_SRC_CTRL_OFFSET(gmac->id);


Same here.


   320                  break;
   321          default:
   322                  dev_err(&pdev->dev, "Unsupported PHY mode: \"%s\"\n",
   323                          phy_modes(gmac->phy_mode));
   324                  return -EINVAL;
   325          }

regards,
dan carpenter

Re: stmmac: add ipq806x glue layer

[Dan Carpenter]

On Tue, Aug 04, 2015 at 03:42:51PM -0700, Mathieu Olivari wrote:
> Hi Dan,
> You're right, that's a bug. The offset should be (x), and not (1 << x).
> We could either redefine the macro to be (x), or we just use << gmac->id
> directly without a macro.
> The reason why we have NSS_COMMON_CLK_SRC_CTRL_RGMII &
> NSS_COMMON_CLK_SRC_CTRL_SGMII macros is that the bit takes different values
> depending on the port. I can post a patch today if you want.

Yes, please.  If you could give me a Reported-by: Dan Carpenter
<dan.carpenter@oracle.com> tag, I would be grateful.  Thanks!

regards,
dan carpenter