Re: [Qemu-devel] [PATCH v4 6/8] i.MX: move FEC device to a register array structure.

[Jason Wang <jasowang@redhat.com>]

On 2016年05月19日 14:10, Jean-Christophe DUBOIS wrote:
> Le 19/05/2016 05:28, Jason Wang a écrit :
>>
>>
>> On 2016年05月19日 06:23, Jean-Christophe Dubois wrote:
>>> This is to prepare for the ENET Gb device of the i.MX6.
>>>
>>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>>> ---
>>>
>>> Changes since v1:
>>>   * Not present on v1.
>>>
>>> Changes since v2:
>>>   * The patch was split in 2 parts:
>>>     - a "port" to a reg array based device (this patch).
>>>     - the addition of the Gb support (next patch).
>>>   Changes since v3:
>>>   * Small fix patches were extracted from this patch (see previous 3 
>>> patches)
>>>   * Reset logic through ECR was fixed.
>>>   * TDAR/RDAR behavior was fixed.
>>>
>>>   hw/net/imx_fec.c         | 396 
>>> ++++++++++++++++++++++++++---------------------
>>>   include/hw/net/imx_fec.h |  55 ++++---
>>>   2 files changed, 258 insertions(+), 193 deletions(-)
>>>
>>>

[...]

>>> -    case 0x014: /* TDAR */
>>> -        if (s->ecr & FEC_EN) {
>>> +    case ENET_TDAR:
>>> +        if (s->regs[ENET_ECR] & FEC_EN) {
>>> +            s->regs[index] = ENET_TDAR_TDAR;
>>>               imx_fec_do_tx(s);
>>>           }
>>> +        s->regs[index] = 0;
>>>           break;
>>> -    case 0x024: /* ECR */
>>> -        s->ecr = value;
>>> +    case ENET_ECR:
>>>           if (value & FEC_RESET) {
>>> -            imx_fec_reset(DEVICE(s));
>>> +            return imx_fec_reset(DEVICE(s));
>>>           }
>>> -        if ((s->ecr & FEC_EN) == 0) {
>>> -            s->rx_enabled = 0;
>>> +        s->regs[index] = value;
>>> +        if ((s->regs[index] & FEC_EN) == 0) {
>>> +            s->regs[ENET_RDAR] = 0;
>>> +            s->rx_descriptor = s->regs[ENET_RDSR];
>>> +            s->regs[ENET_TDAR] = 0;
>>> +            s->tx_descriptor = s->regs[ENET_TDSR];
>>
>> This seems like a new behavior, is this a bug fix? If yes, better split.
>
> It is a more correct behavior I think. Note however that our guest 
> OSes (in particular Linux) do not trigger this bit. So it is mostly 
> untested/unused.
>

Is this the real hardware or documented behavior? If yes, need a 
separate patch for this. If not, we'd better not add this.

>>
>>>           }
>>>           break;
>>> -    case 0x040: /* MMFR */
>>> -        /* store the value */
>>> -        s->mmfr = value;
>>> +    case ENET_MMFR:
>>> +        s->regs[index] = value;
>>>           if (extract32(value, 29, 1)) {
>>> -            s->mmfr = do_phy_read(s, extract32(value, 18, 10));
>>> +            /* This is a read operation */
>>> +            s->regs[ENET_MMFR] = deposit32(s->regs[ENET_MMFR], 0, 16,
>>> +                                           do_phy_read(s,
>>> + extract32(value,
>>> + 18, 10)));
>>>           } else {
>>> +            /* This a write operation */
>>>               do_phy_write(s, extract32(value, 18, 10), 
>>> extract32(value, 0, 16));
>>>           }
>>>           /* raise the interrupt as the PHY operation is done */
>>> -        s->eir |= FEC_INT_MII;
>>> +        s->regs[ENET_EIR] |= FEC_INT_MII;
>>>           break;
>>> -    case 0x044: /* MSCR */
>>> -        s->mscr = value & 0xfe;
>>> +    case ENET_MSCR:
>>> +        s->regs[index] = value & 0xfe;
>>>           break;
>>> -    case 0x064: /* MIBC */
>>> +    case ENET_MIBC:
>>>           /* TODO: Implement MIB.  */
>>> -        s->mibc = (value & 0x80000000) ? 0xc0000000 : 0;
>>> +        s->regs[index] = (value & 0x80000000) ? 0xc0000000 : 0;
>>>           break;
>>> -    case 0x084: /* RCR */
>>> -        s->rcr = value & 0x07ff003f;
>>> +    case ENET_RCR:
>>> +        s->regs[index] = value & 0x07ff003f;
>>>           /* TODO: Implement LOOP mode.  */
>>>           break;
>>> -    case 0x0c4: /* TCR */
>>> +    case ENET_TCR:
>>>           /* We transmit immediately, so raise GRA immediately. */
>>> -        s->tcr = value;
>>> +        s->regs[index] = value;
>>>           if (value & 1) {
>>> -            s->eir |= FEC_INT_GRA;
>>> +            s->regs[ENET_EIR] |= FEC_INT_GRA;
>>>           }
>>>           break;
>>> -    case 0x0e4: /* PALR */
>>> +    case ENET_PALR:
>>> +        s->regs[index] = value;
>>>           s->conf.macaddr.a[0] = value >> 24;
>>>           s->conf.macaddr.a[1] = value >> 16;
>>>           s->conf.macaddr.a[2] = value >> 8;
>>>           s->conf.macaddr.a[3] = value;
>>
>> I believe we should use stl_be_p() here?
>
> I didn't change this bit ...
>

Sorry, you are right. I misread the patch.

>>
>>>           break;
>>> -    case 0x0e8: /* PAUR */
>>> +    case ENET_PAUR:
>>> +        s->regs[index] = (value | 0x0000ffff) & 0xffff8808;
>>>           s->conf.macaddr.a[4] = value >> 24;
>>>           s->conf.macaddr.a[5] = value >> 16;
>>>           break;
>>> -    case 0x0ec: /* OPDR */

[...]

>>> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
>>> index cbf8650..709f8a0 100644
>>> --- a/include/hw/net/imx_fec.h
>>> +++ b/include/hw/net/imx_fec.h
>>> @@ -30,6 +30,33 @@
>>>   #include "hw/sysbus.h"
>>>   #include "net/net.h"
>>>   +#define ENET_EIR               1
>>> +#define ENET_EIMR              2
>>> +#define ENET_RDAR              4
>>> +#define ENET_TDAR              5
>>> +#define ENET_ECR               9
>>> +#define ENET_MMFR              16
>>> +#define ENET_MSCR              17
>>> +#define ENET_MIBC              25
>>> +#define ENET_RCR               33
>>> +#define ENET_TCR               49
>>> +#define ENET_PALR              57
>>> +#define ENET_PAUR              58
>>> +#define ENET_OPD               59
>>> +#define ENET_IAUR              70
>>> +#define ENET_IALR              71
>>> +#define ENET_GAUR              72
>>> +#define ENET_GALR              73
>>> +#define ENET_TFWR              81
>>> +#define ENET_FRBR              83
>>> +#define ENET_FRSR              84
>>> +#define ENET_RDSR              96
>>> +#define ENET_TDSR              97
>>> +#define ENET_MRBR              98
>>> +#define ENET_MIIGSK_CFGR       192
>>> +#define ENET_MIIGSK_ENR        194
>>> +#define ENET_MAX               400
>>
>> Is this an arbitrary value or there's a plan to add more register 
>> whose index is greater than 192?
>
> More registers are coming with the ENET device.
>

Right, I see.

Thanks

>>
>>> +
>>>   #define FEC_MAX_FRAME_SIZE 2032
>>>     #define FEC_INT_HB      (1 << 31)
>>> @@ -46,8 +73,14 @@
>>>   #define FEC_INT_RL      (1 << 20)
>>>   #define FEC_INT_UN      (1 << 19)
>>>   -#define FEC_EN      2
>>> -#define FEC_RESET   1
>>> +/* RDAR */
>>> +#define ENET_RDAR_RDAR         (1 << 24)
>>> +
>>> +/* TDAR */
>>> +#define ENET_TDAR_TDAR         (1 << 24)
>>> +
>>> +#define FEC_EN                 (1 << 1)
>>> +#define FEC_RESET              (1 << 0)
>>>     /* Buffer Descriptor.  */
>>>   typedef struct {
>>> @@ -83,25 +116,9 @@ typedef struct IMXFECState {
>>>       qemu_irq irq;
>>>       MemoryRegion iomem;
>>>   -    uint32_t irq_state;
>>> -    uint32_t eir;
>>> -    uint32_t eimr;
>>> -    uint32_t rx_enabled;
>>> +    uint32_t regs[ENET_MAX];
>>>       uint32_t rx_descriptor;
>>>       uint32_t tx_descriptor;
>>> -    uint32_t ecr;
>>> -    uint32_t mmfr;
>>> -    uint32_t mscr;
>>> -    uint32_t mibc;
>>> -    uint32_t rcr;
>>> -    uint32_t tcr;
>>> -    uint32_t tfwr;
>>> -    uint32_t frsr;
>>> -    uint32_t erdsr;
>>> -    uint32_t etdsr;
>>> -    uint32_t emrbr;
>>> -    uint32_t miigsk_cfgr;
>>> -    uint32_t miigsk_enr;
>>>         uint32_t phy_status;
>>>       uint32_t phy_control;
>>
>>
>
>