hw/net/eepro100: Implement read-only bits in MDI registers

Although we defined an eepro100_mdi_mask[] array indicating which bits
in the registers are read-only, we weren't actually doing anything with
it. Make the MDI register-read code use it rather than manually making
registers 2 and 3 totally read-only and the rest totally read-write.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This seemed a better fix for the unused variable than just deleting it...

 hw/net/eepro100.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

07.06.2014 20:52, Peter Maydell wrote:
> Although we defined an eepro100_mdi_mask[] array indicating which bits
> in the registers are read-only, we weren't actually doing anything with
> it. Make the MDI register-read code use it rather than manually making
> registers 2 and 3 totally read-only and the rest totally read-write.

s/register-read/register-write/ -- can be fixed when applying.

I'm not sure this is "trivial enough", because the side effect is
not obvious, at least not to someone not familiar with eepro100
registers and their usage.

Besides, the description does not seem to be very accurate too.
From the code I see that the original code makes register 0
"semi-writable", register 1 is unwritable and the rest fully
writable.

In this context, apparently we're losing the ability to write to
register 0 completely, since its mask is 0 but the original code
allows writing something to it.

Also, maybe updating the "missing()" calls according to the
bitmask is a good idea...

Thanks,

/mjt

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This seemed a better fix for the unused variable than just deleting it...
> 
>  hw/net/eepro100.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 3b891ca..9c70cce 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -1217,7 +1217,6 @@ static void eepro100_write_mdi(EEPRO100State *s)
>                  break;
>              case 1:            /* Status Register */
>                  missing("not writable");
> -                data = s->mdimem[reg];
>                  break;
>              case 2:            /* PHY Identification Register (Word 1) */
>              case 3:            /* PHY Identification Register (Word 2) */
> @@ -1230,7 +1229,8 @@ static void eepro100_write_mdi(EEPRO100State *s)
>              default:
>                  missing("not implemented");
>              }
> -            s->mdimem[reg] = data;
> +            s->mdimem[reg] &= eepro100_mdi_mask[reg];
> +            s->mdimem[reg] |= data & ~eepro100_mdi_mask[reg];
>          } else if (opcode == 2) {
>              /* MDI read */
>              switch (reg) {
>

On 8 June 2014 12:31, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 07.06.2014 20:52, Peter Maydell wrote:
>> Although we defined an eepro100_mdi_mask[] array indicating which bits
>> in the registers are read-only, we weren't actually doing anything with
>> it. Make the MDI register-read code use it rather than manually making
>> registers 2 and 3 totally read-only and the rest totally read-write.
>
> s/register-read/register-write/ -- can be fixed when applying.
>
> I'm not sure this is "trivial enough", because the side effect is
> not obvious, at least not to someone not familiar with eepro100
> registers and their usage.

Mmm, but do you want to suggest a better queue? I did check
that Linux could still boot and talk to the network via an eepro100.

> Besides, the description does not seem to be very accurate too.
> From the code I see that the original code makes register 0
> "semi-writable", register 1 is unwritable and the rest fully
> writable.

Yes, that was wrongly worded. You're correct that it's just
that 1 is unwritable in the original code.

> In this context, apparently we're losing the ability to write to
> register 0 completely, since its mask is 0 but the original code
> allows writing something to it.

Hmm? The mask is a mask of read-only bits, so if mask is zero
then ANDing the register with the mask will clear it, ANDing the
data with ~mask will do nothing, and then ORing the data into
the register means we set every bit in the register. (This
all happens after the register-specific case code, so the work
that does to have some of the bits have special effects by
changing the value of 'data' remains in place.)

> Also, maybe updating the "missing()" calls according to the
> bitmask is a good idea...

That seems like a separate thing; there's a lot of missing
behaviour here, I suspect.

thanks
-- PMM

08.06.2014 16:27, Peter Maydell wrote:
> On 8 June 2014 12:31, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> 07.06.2014 20:52, Peter Maydell wrote:
>>> Although we defined an eepro100_mdi_mask[] array indicating which bits
>>> in the registers are read-only, we weren't actually doing anything with
>>> it. Make the MDI register-read code use it rather than manually making
>>> registers 2 and 3 totally read-only and the rest totally read-write.
>>
>> s/register-read/register-write/ -- can be fixed when applying.
>>
>> I'm not sure this is "trivial enough", because the side effect is
>> not obvious, at least not to someone not familiar with eepro100
>> registers and their usage.
> 
> Mmm, but do you want to suggest a better queue? I did check
> that Linux could still boot and talk to the network via an eepro100.

[]

>> In this context, apparently we're losing the ability to write to
>> register 0 completely, since its mask is 0 but the original code
>> allows writing something to it.
> 
> Hmm? The mask is a mask of read-only bits, so if mask is zero
> then ANDing the register with the mask will clear it, ANDing the
> data with ~mask will do nothing, and then ORing the data into
> the register means we set every bit in the register. (This
> all happens after the register-specific case code, so the work
> that does to have some of the bits have special effects by
> changing the value of 'data' remains in place.)

That was inaccurate on my part.

Let's take a closer look.  Current code:

static const uint16_t eepro100_mdi_mask[] = {
    0x0000, 0xffff, 0xffff, 0xffff, 0xc01f, 0xffff, 0xffff, 0x0000,
    ...

static void eepro100_write_mdi() {
...
            switch (reg) {
            case 0:            /* Control Register */
                ..modify data...
                break;
            case 1:            /* Status Register */
                missing("not writable");
                data = s->mdimem[reg];
                break;
            case 2:            /* PHY Identification Register (Word 1) */
            case 3:            /* PHY Identification Register (Word 2) */
                missing("not implemented");
                break;
            case 4:            /* Auto-Negotiation Advertisement Register */
            case 5:            /* Auto-Negotiation Link Partner Ability Register */
                break;
            case 6:            /* Auto-Negotiation Expansion Register */
            default:
                missing("not implemented");
            }
            s->mdimem[reg] = data;

You're changing this last line with there (removing data= in case 1):

            s->mdimem[reg] &= eepro100_mdi_mask[reg];
            s->mdimem[reg] |= data & ~eepro100_mdi_mask[reg];

so eepro100_mdi_mask has bit set for UNwritable bits, and bit unset
for WRITABLE bits.  This is where I misunderstood it initially (because
it is sort of expected to be the opposite).  So, reg0 is fully writable
still (with the same mods which are applied initially).

But the the next 3 registers - 1, 2 and 3 - are becoming all UNwritable.
Original code has only register 1 unwritable, not registers 2 and 3 (and
all others).

Initially all registers but register 1 was fully writable, but now most
regs becomes UNwritable.  That's what I should have said in the first
email ;)

Maybe that's a non-issue anyway, maybe these regs aren't actually used
by the device emulation code, -- as I mentioned initially, this is not
exactly a trivial change, it isn't clear at all to someone who isn't
familiar with the registers and their usage -- in real hw and in there.

Thanks,

/mjt

On 8 June 2014 13:27, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 June 2014 12:31, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> 07.06.2014 20:52, Peter Maydell wrote:
>>> Although we defined an eepro100_mdi_mask[] array indicating which bits
>>> in the registers are read-only, we weren't actually doing anything with
>>> it. Make the MDI register-read code use it rather than manually making
>>> registers 2 and 3 totally read-only and the rest totally read-write.
>>
>> s/register-read/register-write/ -- can be fixed when applying.
>>
>> I'm not sure this is "trivial enough", because the side effect is
>> not obvious, at least not to someone not familiar with eepro100
>> registers and their usage.
>
> Mmm, but do you want to suggest a better queue? I did check
> that Linux could still boot and talk to the network via an eepro100.

Stefan has agreed to take this through the net queue;
I'll resend with the commit message errors you noted fixed.

thanks
-- PMM