[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Etherne
From: |
Alexander Graf |
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC) |
Date: |
Wed, 17 Jul 2013 12:22:34 +0200 |
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
- Re: [Qemu-ppc] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), (continued)
- Re: [Qemu-ppc] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Scott Wood, 2013/07/15
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/16
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Alexander Graf, 2013/07/16
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/16
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Scott Wood, 2013/07/16
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/17
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Alexander Graf, 2013/07/17
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/17
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Scott Wood, 2013/07/16
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/17
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC),
Alexander Graf <=
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/17
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Scott Wood, 2013/07/17
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/18
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Scott Wood, 2013/07/18
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/19
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Scott Wood, 2013/07/19
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC), Fabien Chouteau, 2013/07/22