On 17.07.2013, at 12:17, Fabien Chouteau wrote: > On 07/16/2013 07:50 PM, Scott Wood wrote: >> On 07/16/2013 10:28:28 AM, Fabien Chouteau wrote: >>> On 07/16/2013 04:06 AM, Scott Wood wrote: >>>> On 07/10/2013 12:10:02 PM, Fabien Chouteau wrote: >>>>> +obj-$(CONFIG_ETSEC) += etsec.o etsec_registers.o etsec_rings.o >>>>> etsec_miim.o >>>> >>>> Maybe an fsl_etsec directory? >>>> >>> >>> Is it really necessary? >> >> Not strictly necessary, but it would help keep things tidy. >> > > I will try :) > >>>>> +DeviceState *etsec_create(hwaddr base, >>>>> + MemoryRegion * mr, >>>>> + NICInfo * nd, >>>>> + qemu_irq tx_irq, >>>>> + qemu_irq rx_irq, >>>>> + qemu_irq err_irq) >>>>> >>>> Do you plan to update hw/ppc/e500.c (or maybe some other platform?) to >>>> call this? >>>> >>> >>> No I don't, not for the moment. I use it in one of our machine (that is not >>> in mainstream). >> >> Someone should, so we can test this without your out-of-tree code. >> >>> e500.c would require PCI support I think. >> >> e500.c has PCI support, but how is that relevant to eTSEC? > > I guess it's not. We would have to remove: > > if (pci_bus) { > /* Register network interfaces. */ > for (i = 0; i < nb_nics; i++) { > pci_nic_init_nofail(&nd_table[i], pci_bus, "virtio", NULL); > } > }
Well, this code is pretty broken anyways. It should honor the IF type that we request from it. Then we can also add eTSEC as one. > >> >>>>> + if (*size == etsec->rx_padding) { >>>>> + /* The remaining bytes are for padding which is not actually >>>>> allocated >>>>> + in the buffer */ >>>>> + >>>>> + rem = MIN(etsec->regs[MRBLR].value - bd->length, >>>>> etsec->rx_padding); >>>>> + >>>>> + if (rem > 0) { >>>>> + memset(padd, 0x0, sizeof(padd)); >>>>> + etsec->rx_padding -= rem; >>>>> + *size -= rem; >>>>> + bd->length += rem; >>>>> + cpu_physical_memory_write(bufptr, padd, rem); >>>>> + } >>>>> + } >>>> >>>> What if *size > 0 && *size < etsec->rx_padding? >>> >>> I don't think it's possible... >> >> Maybe throw in an assertion, then? >> >> I can see how it might not be possible if rx_padding is being used for >> padding a short frame, since MRBLR must be a multiple of 64, but what if >> it's 4 bytes for CRC? >> > > Can you explain a possible error scenario? > >>>>> +static void rx_init_frame(eTSEC *etsec, const uint8_t *buf, size_t size) >>>>> +{ >>>>> + uint32_t fcb_size = 0; >>>>> + uint8_t prsdep = (etsec->regs[RCTRL].value >> RCTRL_PRSDEP_OFFSET) >>>>> + & RCTRL_PRSDEP_MASK; >>>>> + >>>>> + if (prsdep != 0) { >>>>> + /* Prepend FCB */ >>>>> + fcb_size = 8 + 2; /* FCB size + align */ >>>>> + /* I can't find this 2 bytes alignement in fsl documentation but >>>>> VxWorks >>>>> + expects them */ >>>> >>>> Could these 2 bytes be from RCTRL[PAD], which you seem to ignore? >>> >>> Did you mean RCTRL[PAL]? It could be that. >> >> Yes, sorry. >> > > Thanks for pointing it ;) > >>>>> + etsec->rx_padding = 4; >>> >>> CRC. >>> >>>>> + if (size < 60) { >>>>> + etsec->rx_padding += 60 - size; >>>>> + } >>>> >>>> Could you explain what you're doing with rx_padding? >>> >>> Avoiding short frames. I'll add a comments. >> >> This is for when RCTRL[RSF] is set, to accept short frames? Is it >> documented anywhere that these frames are zero-padded to 64 bytes on rx? If >> not, is this the observed behavior of real hardware? >> >> In any case, please add some comments. >> > > Actually, this might be obsolete. At the time QEMU was sending short > frames, but vxWorks drop them, so I made this change to always have 64B > frames. Now I have another patch to fix this and I will remove this hack > and implement RCTRL[RSF]. > > >>>>> + /* ring_base = (etsec->regs[RBASEH].value & 0xF) << 32; */ >>>>> + ring_base += etsec->regs[RBASE0 + ring_nbr].value & ~0x7; >>>>> + start_bd_addr = bd_addr = etsec->regs[RBPTR0 + ring_nbr].value & >>>>> ~0x7; >>>> >>>> What about RBDBPH (upper bits of physical address)? Likewise for TX. >>>> >>> >>> I'm only interested in 32bits address spaces, so RBASEH, TBASEH, RBDBPH or >>> TBDBPH. >> >> When adding code to mainline, it's about more than what you're personally >> interested in... >> > > If I'm not "personally" interested, I will not have time to develop or > test a feature. If someone wants to do 36bit addressing, feel free to do > it, that's why I send the patch on QEMU-devel, but I won't implement > full support of eTSEC myself... > >> Could you at least have a way to diagnose when the guest OS tries to >> use some functionality that you don't support, rather than silently >> doing the wrong thing? >> > > This device is so complex, detecting unsupported features would take too > much work. In this case a simple if(<upper bits of addresses>) hw_error(...) would be good enough :). It's just to make sure that all this valuable knowledge doesn't get lost any someone who eventually does run into 36bit addressing doesn't have to debug for a week to figure out who randomly overwrites his kernel ;). Alex