Message ID | 1402159924-13853-1-git-send-email-peter.maydell@linaro.org |
---|---|
State | Superseded |
Headers | show |
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
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) {
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(-)