diff mbox

[09/14] hw/lan9118.c: Basic byte/word/long access support.

Message ID 1323251225-1072-10-git-send-email-e.voevodin@samsung.com
State New
Headers show

Commit Message

Evgeny Voevodin Dec. 7, 2011, 9:47 a.m. UTC 
We included this chip into s5pc210 platform because SMDK board holds
lan9215 chip. Difference is that 9215 access is 16-bit wide and some
registers differ. By addition basic 16-bit access to 9118 emulation we
achieved ethernet controller support by Linux lernel on SMDK boards.

Signed-off-by: Evgeny Voevodin <e.voevodin@samsung.com>
---
 hw/lan9118.c |   96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 90 insertions(+), 6 deletions(-)

Comments

Evgeny Voevodin Dec. 7, 2011, 10:58 a.m. UTC | #1 
On 12/07/2011 02:09 PM, Peter Maydell wrote:
> On 7 December 2011 09:47, Evgeny Voevodin<e.voevodin@samsung.com>  wrote:
>> We included this chip into s5pc210 platform because SMDK board holds
>> lan9215 chip. Difference is that 9215 access is 16-bit wide and some
>> registers differ. By addition basic 16-bit access to 9118 emulation we
>> achieved ethernet controller support by Linux lernel on SMDK boards.
>
> If it differs then shouldn't we add a new qdev device for 9215 ?
> (sharing most of the implementation code, obviously)
>

This patch could be interpreted as lan9118 emulation expansion since 
this chip supports 16-bit access too. These changes don't cover all the 
difference between 9118 and 9215, but it's enough to provide network 
support to Samsung boards. When 9215 support will be added we can easily 
switch to this chip.

>>   static const MemoryRegionOps lan9118_mem_ops = {
>> -    .read = lan9118_readl,
>> -    .write = lan9118_writel,
>> +    .old_mmio = {
>> +        .read = { lan9118_readb, lan9118_readw, lan9118_readl, },
>> +        .write = { lan9118_writeb, lan9118_writew, lan9118_writel, },
>> +    },
>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>   };
>
> This is going backwards -- the .old_mmio hooks are for backwards
> compatibility when converting old devices to MemoryRegions -- they
> shouldn't be added in new code.
>
> You need to make the lan9118_read/write functions look at their
> 'size' argument instead.
>
> -- PMM
>
>
Peter Maydell Dec. 7, 2011, 11:17 a.m. UTC | #2 
On 7 December 2011 10:58, Evgeny Voevodin <e.voevodin@samsung.com> wrote:
> On 12/07/2011 02:09 PM, Peter Maydell wrote:
>>
>> On 7 December 2011 09:47, Evgeny Voevodin<e.voevodin@samsung.com>  wrote:
>>>
>>> We included this chip into s5pc210 platform because SMDK board holds
>>> lan9215 chip. Difference is that 9215 access is 16-bit wide and some
>>> registers differ. By addition basic 16-bit access to 9118 emulation we
>>> achieved ethernet controller support by Linux lernel on SMDK boards.
>>
>>
>> If it differs then shouldn't we add a new qdev device for 9215 ?
>> (sharing most of the implementation code, obviously)
>>
>
> This patch could be interpreted as lan9118 emulation expansion since this
> chip supports 16-bit access too. These changes don't cover all the
> difference between 9118 and 9215, but it's enough to provide network support
> to Samsung boards. When 9215 support will be added we can easily switch to
> this chip.

If you're adding a legitimate missing feature (16 bit access support) to
lan9118 then your commit message should say so, not talk about 9215.
The right place to say "this should be a 9215 but the 9118 is close enough"
is in the patch adding the network chip to the board model, not in the
commit adding a feature to the network chip model.

We should probably make 16 vs 32 bit be a qdev property of lan9118
(corresponding to the way the hardware is configured by an input pin
on startup) and reflect this properly in the HW_CFG register.

Also, this todo comment is too obscure:
+    /* TODO: Implement fair word operation, because this implementation
+     * assumes that any register is accessed as two 16-bit operations. */

I have no idea what it means.

-- PMM
diff mbox

Patch

diff --git a/hw/lan9118.c b/hw/lan9118.c
index ee8b2ea..7aebabd 100644
--- a/hw/lan9118.c
+++ b/hw/lan9118.c
@@ -212,6 +212,14 @@  typedef struct {
     int rxp_offset;
     int rxp_size;
     int rxp_pad;
+
+    uint32_t write_word_prev_offset;
+    uint32_t write_word_n;
+    uint16_t write_word_l;
+    uint16_t write_word_h;
+    uint32_t read_word_prev_offset;
+    uint32_t read_word_n;
+    uint32_t read_long;
 } lan9118_state;
 
 static void lan9118_update(lan9118_state *s)
@@ -345,6 +353,9 @@  static void lan9118_reset(DeviceState *d)
     s->mac_mii_data = 0;
     s->mac_flow = 0;
 
+    s->read_word_n = 0;
+    s->write_word_n = 0;
+
     phy_reset(s);
 
     s->eeprom_writable = 0;
@@ -896,11 +907,11 @@  static void lan9118_tick(void *opaque)
 }
 
 static void lan9118_writel(void *opaque, target_phys_addr_t offset,
-                           uint64_t val, unsigned size)
+                           uint32_t val)
 {
     lan9118_state *s = (lan9118_state *)opaque;
     offset &= 0xff;
-    
+
     //DPRINTF("Write reg 0x%02x = 0x%08x\n", (int)offset, val);
     if (offset >= 0x20 && offset < 0x40) {
         /* TX FIFO */
@@ -1029,8 +1040,43 @@  static void lan9118_writel(void *opaque, target_phys_addr_t offset,
     lan9118_update(s);
 }
 
-static uint64_t lan9118_readl(void *opaque, target_phys_addr_t offset,
-                              unsigned size)
+static void lan9118_writeb(void *opaque, target_phys_addr_t offset,
+                           uint32_t val)
+{
+    hw_error("lan9118_writeb: Bad reg 0x%x = %x\n", (int)offset, val);
+}
+
+static void lan9118_writew(void *opaque, target_phys_addr_t offset,
+                           uint32_t val)
+{
+    lan9118_state *s = (lan9118_state *)opaque;
+    offset &= 0xff;
+
+    /* TODO: Implement fair word operation, because this implementation
+     * assumes that any register is accessed as two 16-bit operations. */
+
+    if (s->write_word_prev_offset != (offset & ~0x3)) {
+        /* New offset, reset word counter */
+        s->write_word_n = 0;
+        s->write_word_prev_offset = offset & ~0x3;
+    }
+
+    if (offset & 0x2) {
+        s->write_word_h = val;
+    } else {
+        s->write_word_l = val;
+    }
+
+    //DPRINTF("Writew reg 0x%02x = 0x%08x\n", (int)offset, val);
+    s->write_word_n++;
+    if (s->write_word_n == 2) {
+        s->write_word_n = 0;
+        lan9118_writel(s, offset & ~3, s->write_word_l +
+                (s->write_word_h << 16));
+    }
+}
+
+static uint32_t lan9118_readl(void *opaque, target_phys_addr_t offset)
 {
     lan9118_state *s = (lan9118_state *)opaque;
 
@@ -1103,9 +1149,47 @@  static uint64_t lan9118_readl(void *opaque, target_phys_addr_t offset,
     return 0;
 }
 
+static uint32_t lan9118_readb(void *opaque, target_phys_addr_t offset)
+{
+    hw_error("lan9118_readb: Bad reg 0x%x\n", (int)offset);
+}
+
+static uint32_t lan9118_readw(void *opaque, target_phys_addr_t offset)
+{
+    lan9118_state *s = (lan9118_state *)opaque;
+    uint32_t val;
+
+    /* TODO: Implement fair word operation, because this implementation
+     * assumes that any register is accessed as two 16-bit operations. */
+
+    if (s->read_word_prev_offset != (offset & ~0x3)) {
+        /* New offset, reset word counter */
+        s->read_word_n = 0;
+        s->read_word_prev_offset = offset & ~0x3;
+    }
+
+    s->read_word_n++;
+    if (s->read_word_n == 1) {
+        s->read_long = lan9118_readl(s, offset & ~3);
+    } else {
+        s->read_word_n = 0;
+    }
+
+    if (offset & 2) {
+        val = s->read_long >> 16;
+    } else {
+        val = s->read_long & 0xFFFF;
+    }
+
+    //DPRINTF("Readw reg 0x%02x, val 0x%x\n", (int)offset, val);
+    return val;
+}
+
 static const MemoryRegionOps lan9118_mem_ops = {
-    .read = lan9118_readl,
-    .write = lan9118_writel,
+    .old_mmio = {
+        .read = { lan9118_readb, lan9118_readw, lan9118_readl, },
+        .write = { lan9118_writeb, lan9118_writew, lan9118_writel, },
+    },
     .endianness = DEVICE_NATIVE_ENDIAN,
 };