[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [PATCH] hw/net/eepro100: Implement read-only bits in
Re: [Qemu-trivial] [PATCH] hw/net/eepro100: Implement read-only bits in MDI registers
Sun, 8 Jun 2014 13:27:44 +0100
On 8 June 2014 12:31, Michael Tokarev <address@hidden> 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
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.