[PATCH 2/2] net/ibm/emac: wrong bit is used for STA control register write

[PATCH 2/2] net/ibm/emac: wrong bit is used for STA control register write

[Ivan Mikhaylov]

STA control register has areas of mode and opcodes for opeations. 18 bit is
using for mode selection, where 0 is old MIO/MDIO access method and 1 is
indirect access mode. 19-20 bits are using for setting up read/write
operation(STA opcodes). In current state 'read' is set into old MIO/MDIO mode
with 19 bit and write operation is set into 18 bit which is mode selection,
not a write operation. To correlate write with read we set it into 20 bit.

Signed-off-by: Ivan Mikhaylov <ivan@de.ibm.com>
---
 drivers/net/ethernet/ibm/emac/emac.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/emac.h b/drivers/net/ethernet/ibm/emac/emac.h
index d0a0e3b..c26d263 100644
--- a/drivers/net/ethernet/ibm/emac/emac.h
+++ b/drivers/net/ethernet/ibm/emac/emac.h
@@ -244,7 +244,7 @@ struct emac_regs {
 #define EMAC_STACR_PHYE			0x00004000
 #define EMAC_STACR_STAC_MASK		0x00003000
 #define EMAC_STACR_STAC_READ		0x00001000
-#define EMAC_STACR_STAC_WRITE		0x00002000
+#define EMAC_STACR_STAC_WRITE		0x00000800
 #define EMAC_STACR_OPBC_MASK		0x00000C00
 #define EMAC_STACR_OPBC_50		0x00000000
 #define EMAC_STACR_OPBC_66		0x00000400
-- 
1.7.1

Re: [PATCH 2/2] net/ibm/emac: wrong bit is used for STA control register write

[Christian Lamparter]

On Monday, January 22, 2018 5:00:38 PM CET Ivan Mikhaylov wrote:
> STA control register has areas of mode and opcodes for opeations. 18 bit is
> using for mode selection, where 0 is old MIO/MDIO access method and 1 is
> indirect access mode. 19-20 bits are using for setting up read/write
> operation(STA opcodes). In current state 'read' is set into old MIO/MDIO mode
> with 19 bit and write operation is set into 18 bit which is mode selection,
> not a write operation. To correlate write with read we set it into 20 bit.
> 
> Signed-off-by: Ivan Mikhaylov <ivan@de.ibm.com>
> ---
>  drivers/net/ethernet/ibm/emac/emac.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/emac.h b/drivers/net/ethernet/ibm/emac/emac.h
> index d0a0e3b..c26d263 100644
> --- a/drivers/net/ethernet/ibm/emac/emac.h
> +++ b/drivers/net/ethernet/ibm/emac/emac.h
> @@ -244,7 +244,7 @@ struct emac_regs {
>  #define EMAC_STACR_PHYE			0x00004000
>  #define EMAC_STACR_STAC_MASK		0x00003000
>  #define EMAC_STACR_STAC_READ		0x00001000
> -#define EMAC_STACR_STAC_WRITE		0x00002000
> +#define EMAC_STACR_STAC_WRITE		0x00000800
>  #define EMAC_STACR_OPBC_MASK		0x00000C00
>  #define EMAC_STACR_OPBC_50		0x00000000
>  #define EMAC_STACR_OPBC_66		0x00000400
> 

Something looks wrong here?! The commit message talks about bit 18, 19 and 20.
However, 0x0800, 0x1000, 0x2000 and are like bit 11, 12 and 13? Furthermore,
what about the EMAC_STACR_STAC_MASK? shouldn't it be 0x1800 now (or delete it
since it doesn't look like it's used anywhere?).

Re: [PATCH 2/2] net/ibm/emac: wrong bit is used for STA control register write

[Ivan Mikhaylov]

>Something looks wrong here?! The commit message talks about bit 18, 19 and 20.
>However, 0x0800, 0x1000, 0x2000 and are like bit 11, 12 and 13? Furthermore,
>what about the EMAC_STACR_STAC_MASK? shouldn't it be 0x1800 now (or delete it
>since it doesn't look like it's used anywhere?).
Christian, nope, it's all fine there, it's big endian. This commit related only
to write operation, I'll check MASK and see what I can do with that.

Re: [PATCH 2/2] net/ibm/emac: wrong bit is used for STA control register write

[Christian Lamparter]

On Monday, January 22, 2018 8:01:46 PM CET Ivan Mikhaylov wrote:
> >Something looks wrong here?! The commit message talks about bit 18, 19 and 20.
> >However, 0x0800, 0x1000, 0x2000 and are like bit 11, 12 and 13? Furthermore,
> >what about the EMAC_STACR_STAC_MASK? shouldn't it be 0x1800 now (or delete it
> >since it doesn't look like it's used anywhere?).
> Christian, nope, it's all fine there, it's big endian.
Ok Thanks. I think I found the relevant info on Wikipedia:
<https://en.wikipedia.org/wiki/Bit_numbering#Usage>

"Little-endian CPUs usually employ "LSB 0" bit numbering, however both bit
numbering conventions can be seen in big-endian machines. Some architectures
like SPARC and Motorola 68000 use "LSB 0" bit numbering, while S/390, PowerPC
and PA-RISC use "MSB 0"."

But as far as the kernel (code) is concerned, all architectures (big or
little endian) have the same BIT marco:
<https://elixir.free-electrons.com/linux/v4.15-rc9/source/include/linux/bitops.h>
|#define BIT(nr)			(1UL << (nr))
So if someone tries to #define EMAC_STACR_STAC_WRITE BIT(18) it would be
0x40000 instead. This is where the confusion is coming from. Can you please
at least mention this somewhere that all the bits in the commit message are
in "MSB 0" format? It's confusing enough as it is ;).

> This commit related only
> to write operation, I'll check MASK and see what I can do with that.
Well, the MASK is not used and it now looks odd. So you might as well
delete it. Maybe as well as all the unused EMACX_STACR_STAC_IND_* macros?

Thanks,
Christian

Re: [PATCH 2/2] net/ibm/emac: wrong bit is used for STA control register write

[Ivan Mikhaylov]

> So if someone tries to #define EMAC_STACR_STAC_WRITE BIT(18) it would be
> 0x40000 instead. This is where the confusion is coming from. Can you please
> at least mention this somewhere that all the bits in the commit message are
> in "MSB 0" format? It's confusing enough as it is ;).
Yeap, sure, will do.

> Well, the MASK is not used and it now looks odd. So you might as well
> delete it. Maybe as well as all the unused EMACX_STACR_STAC_IND_* macros?
I'll check and delete if it's not used anywhere. Thanks for finding :)