Re: [Qemu-devel] [PATCH v4 7/8] Add ENET/Gbps Ethernet support to FEC device

[Jason Wang <jasowang@redhat.com>]

On 2016年05月20日 02:14, Jean-Christophe DUBOIS wrote:
> Le 19/05/2016 05:48, Jason Wang a écrit :
>>
>>
>> On 2016年05月19日 06:23, Jean-Christophe Dubois wrote:
>>> The ENET device (present in i.MX6) is "derived" from FEC and backward
>>> compatible with it.
>>>
>>> This patch adds the necessary support of the added feature in the ENET
>>> device to allow Linux to use it (on supported processors).
>>>
>>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>>> ---
>>>
>>> Changes since v1:
>>>   * Not present on v1
>>>     Changes since v2:
>>>   * Not present on v2
>>>   Changes since v3:
>>>   * Separate and fix the 2 supported interrupts
>>>
>>>   hw/arm/fsl-imx25.c       |   3 +
>>>   hw/net/imx_fec.c         | 713 
>>> ++++++++++++++++++++++++++++++++++++++---------
>>>   include/hw/net/imx_fec.h | 195 ++++++++++---
>>>   3 files changed, 745 insertions(+), 166 deletions(-)
>>
>> [...]
>>
>>>   -static Property imx_fec_properties[] = {
>>> +static Property imx_eth_properties[] = {
>>>       DEFINE_NIC_PROPERTIES(IMXFECState, conf),
>>> +    DEFINE_PROP_BOOL("is-fec", IMXFECState, is_fec, false),
>>>       DEFINE_PROP_END_OF_LIST(),
>>>   };
>>
>> It's ok to decide with "is-fec", but is it better to use a new type 
>> for that?
>
> Well, there is a lot of common code between FEC and ENET because ENET 
> is basically backward compatible with FEC. So most/all of the FEC code 
> needs to go in the ENET device.
>
> I thought this way of doing thing was the best way to avoid 
> duplicating things.

You can still share almost all the codes. E.g you can have a look at 
e1000.c which have 3 types of rather similar devices.

Another question not relate to this patch, I saw version were bumped for 
vmstate version. Is it better to keep the vmstate for FEC and using a 
new vmstate for ENET (register array).

>
>>
>>>   -static void imx_fec_class_init(ObjectClass *klass, void *data)
>>> +static void imx_eth_class_init(ObjectClass *klass, void *data)
>>>   {
>>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>>   -    dc->vmsd = &vmstate_imx_fec;
>>> -    dc->reset = imx_fec_reset;
>>> -    dc->props = imx_fec_properties;
>>> -    dc->realize = imx_fec_realize;
>>> -    dc->desc = "i.MX FEC Ethernet Controller";
>>> +    dc->vmsd    = &vmstate_imx_eth;
>>> +    dc->reset   = imx_eth_reset;
>>> +    dc->props   = imx_eth_properties;
>>> +    dc->realize = imx_eth_realize;
>>> +    dc->desc    = "i.MX FEC/ENET Ethernet Controller";
>>>   }
>>>   -static const TypeInfo imx_fec_info = {
>>> -    .name = TYPE_IMX_FEC,
>>> -    .parent = TYPE_SYS_BUS_DEVICE,
>>> +static const TypeInfo imx_eth_info = {
>>> +    .name          = TYPE_IMX_FEC,
>>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>>>       .instance_size = sizeof(IMXFECState),
>>> -    .class_init = imx_fec_class_init,
>>> +    .class_init    = imx_eth_class_init,
>>>   };
>>>   -static void imx_fec_register_types(void)
>>> +static void imx_eth_register_types(void)
>>>   {
>>> -    type_register_static(&imx_fec_info);
>>> +    type_register_static(&imx_eth_info);
>>>   }
>>>   -type_init(imx_fec_register_types)
>>> +type_init(imx_eth_register_types)
>>> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
>>> index 709f8a0..a09b7d7 100644
>>> --- a/include/hw/net/imx_fec.h
>>> +++ b/include/hw/net/imx_fec.h
>>> @@ -1,5 +1,5 @@
>>>   /*
>>> - * i.MX Fast Ethernet Controller emulation.
>>> + * i.MX FEC/ENET Ethernet Controller emulation.
>>>    *
>>>    * Copyright (c) 2013 Jean-Christophe Dubois. <jcd@tribudubois.net>
>>>    *
>>> @@ -53,25 +53,64 @@
>>>   #define ENET_RDSR              96
>>>   #define ENET_TDSR              97
>>>   #define ENET_MRBR              98
>>> +#define ENET_RSFL              100
>>> +#define ENET_RSEM              101
>>> +#define ENET_RAEM              102
>>> +#define ENET_RAFL              103
>>> +#define ENET_TSEM              104
>>> +#define ENET_TAEM              105
>>> +#define ENET_TAFL              106
>>> +#define ENET_TIPG              107
>>> +#define ENET_FTRL              108
>>> +#define ENET_TACC              112
>>> +#define ENET_RACC              113
>>>   #define ENET_MIIGSK_CFGR       192
>>>   #define ENET_MIIGSK_ENR        194
>>> +#define ENET_ATCR              256
>>> +#define ENET_ATVR              257
>>> +#define ENET_ATOFF             258
>>> +#define ENET_ATPER             259
>>> +#define ENET_ATCOR             260
>>> +#define ENET_ATINC             261
>>> +#define ENET_ATSTMP            262
>>> +#define ENET_TGSR              385
>>> +#define ENET_TCSR0             386
>>> +#define ENET_TCCR0             387
>>> +#define ENET_TCSR1             388
>>> +#define ENET_TCCR1             389
>>> +#define ENET_TCSR2             390
>>> +#define ENET_TCCR2             391
>>> +#define ENET_TCSR3             392
>>> +#define ENET_TCCR3             393
>>>   #define ENET_MAX               400
>>>   -#define FEC_MAX_FRAME_SIZE 2032
>>> -
>>> -#define FEC_INT_HB      (1 << 31)
>>> -#define FEC_INT_BABR    (1 << 30)
>>> -#define FEC_INT_BABT    (1 << 29)
>>> -#define FEC_INT_GRA     (1 << 28)
>>> -#define FEC_INT_TXF     (1 << 27)
>>> -#define FEC_INT_TXB     (1 << 26)
>>> -#define FEC_INT_RXF     (1 << 25)
>>> -#define FEC_INT_RXB     (1 << 24)
>>> -#define FEC_INT_MII     (1 << 23)
>>> -#define FEC_INT_EBERR   (1 << 22)
>>> -#define FEC_INT_LC      (1 << 21)
>>> -#define FEC_INT_RL      (1 << 20)
>>> -#define FEC_INT_UN      (1 << 19)
>>> +#define ENET_MAX_FRAME_SIZE    2032
>>> +
>>> +/* EIR and EIMR */
>>> +#define ENET_INT_HB            (1 << 31)
>>> +#define ENET_INT_BABR          (1 << 30)
>>> +#define ENET_INT_BABT          (1 << 29)
>>> +#define ENET_INT_GRA           (1 << 28)
>>> +#define ENET_INT_TXF           (1 << 27)
>>> +#define ENET_INT_TXB           (1 << 26)
>>> +#define ENET_INT_RXF           (1 << 25)
>>> +#define ENET_INT_RXB           (1 << 24)
>>> +#define ENET_INT_MII           (1 << 23)
>>> +#define ENET_INT_EBERR         (1 << 22)
>>> +#define ENET_INT_LC            (1 << 21)
>>> +#define ENET_INT_RL            (1 << 20)
>>> +#define ENET_INT_UN            (1 << 19)
>>
>> The above renaming seems cause lots of unnecessary changes above 
>> which may brings issue when backporint patches to -stable. Can we 
>> just keep this, and all ENET_*** should be used after we're sure we 
>> are ENET?
>
> What do you mean by "after we're sure we are ENET"?

I mean "is_fec" is false here.

>
> ENET is the evolution of FEC to support Gb interface. FEC is more like 
> a legacy device these days (for ARM9 family). So it seems we are 
> better supporting ENET as default going forward.

Since Peter does not care about this, we're ok here.

Thanks

>
>>
>>> +#define ENET_INT_PLR           (1 << 18)
>>> +#define ENET_INT_WAKEUP        (1 << 17)
>>> +#define ENET_INT_TS_AVAIL      (1 << 16)
>>> +#define ENET_INT_TS_TIMER      (1 << 15)
>>> +
>>> +#define ENET_INT_MAC           (ENET_INT_HB | ENET_INT_BABR | 
>>> ENET_INT_BABT | \
>>> +                                ENET_INT_GRA | ENET_INT_TXF | 
>>> ENET_INT_TXB | \
>>> +                                ENET_INT_RXF | ENET_INT_RXB | 
>>> ENET_INT_MII | \
>>> +                                ENET_INT_EBERR | ENET_INT_LC | 
>>> ENET_INT_RL | \
>>> +                                ENET_INT_UN | ENET_INT_PLR | 
>>> ENET_INT_WAKEUP | \
>>> +                                ENET_INT_TS_AVAIL)
>>>     /* RDAR */
>>>   #define ENET_RDAR_RDAR         (1 << 24)
>>> @@ -79,8 +118,54 @@
>>>   /* TDAR */
>>>   #define ENET_TDAR_TDAR         (1 << 24)
>>>   -#define FEC_EN                 (1 << 1)
>>> -#define FEC_RESET              (1 << 0)
>>> +/* ECR */
>>> +#define ENET_ECR_RESET         (1 << 0)
>>> +#define ENET_ECR_ETHEREN       (1 << 1)
>>> +#define ENET_ECR_MAGICEN       (1 << 2)
>>> +#define ENET_ECR_SLEEP         (1 << 3)
>>> +#define ENET_ECR_EN1588        (1 << 4)
>>> +#define ENET_ECR_SPEED         (1 << 5)
>>> +#define ENET_ECR_DBGEN         (1 << 6)
>>> +#define ENET_ECR_STOPEN        (1 << 7)
>>> +#define ENET_ECR_DSBWP         (1 << 8)
>>> +
>>> +/* MIBC */
>>> +#define ENET_MIBC_MIB_DIS      (1 << 31)
>>> +#define ENET_MIBC_MIB_IDLE     (1 << 30)
>>> +#define ENET_MIBC_MIB_CLEAR    (1 << 29)
>>> +
>>> +/* RCR */
>>> +#define ENET_RCR_LOOP          (1 << 0)
>>> +#define ENET_RCR_DRT           (1 << 1)
>>> +#define ENET_RCR_MII_MODE      (1 << 2)
>>> +#define ENET_RCR_PROM          (1 << 3)
>>> +#define ENET_RCR_BC_REJ        (1 << 4)
>>> +#define ENET_RCR_FCE           (1 << 5)
>>> +#define ENET_RCR_RGMII_EN      (1 << 6)
>>> +#define ENET_RCR_RMII_MODE     (1 << 8)
>>> +#define ENET_RCR_RMII_10T      (1 << 9)
>>> +#define ENET_RCR_PADEN         (1 << 12)
>>> +#define ENET_RCR_PAUFWD        (1 << 13)
>>> +#define ENET_RCR_CRCFWD        (1 << 14)
>>> +#define ENET_RCR_CFEN          (1 << 15)
>>> +#define ENET_RCR_MAX_FL_SHIFT  (16)
>>> +#define ENET_RCR_MAX_FL_LENGTH (14)
>>> +#define ENET_RCR_NLC           (1 << 30)
>>> +#define ENET_RCR_GRS           (1 << 31)
>>> +
>>> +/* TCR */
>>> +#define ENET_TCR_GTS           (1 << 0)
>>> +#define ENET_TCR_FDEN          (1 << 2)
>>> +#define ENET_TCR_TFC_PAUSE     (1 << 3)
>>> +#define ENET_TCR_RFC_PAUSE     (1 << 4)
>>> +#define ENET_TCR_ADDSEL_SHIFT  (5)
>>> +#define ENET_TCR_ADDSEL_LENGTH (3)
>>> +#define ENET_TCR_CRCFWD        (1 << 9)
>>> +
>>> +/* RDSR */
>>> +#define ENET_TWFR_TFWR_SHIFT   (0)
>>> +#define ENET_TWFR_TFWR_LENGTH  (6)
>>> +#define ENET_TWFR_STRFWD       (1 << 8)
>>>     /* Buffer Descriptor.  */
>>>   typedef struct {
>>> @@ -89,22 +174,60 @@ typedef struct {
>>>       uint32_t data;
>>>   } IMXFECBufDesc;
>>>   -#define FEC_BD_R    (1 << 15)
>>> -#define FEC_BD_E    (1 << 15)
>>> -#define FEC_BD_O1   (1 << 14)
>>> -#define FEC_BD_W    (1 << 13)
>>> -#define FEC_BD_O2   (1 << 12)
>>> -#define FEC_BD_L    (1 << 11)
>>> -#define FEC_BD_TC   (1 << 10)
>>> -#define FEC_BD_ABC  (1 << 9)
>>> -#define FEC_BD_M    (1 << 8)
>>> -#define FEC_BD_BC   (1 << 7)
>>> -#define FEC_BD_MC   (1 << 6)
>>> -#define FEC_BD_LG   (1 << 5)
>>> -#define FEC_BD_NO   (1 << 4)
>>> -#define FEC_BD_CR   (1 << 2)
>>> -#define FEC_BD_OV   (1 << 1)
>>> -#define FEC_BD_TR   (1 << 0)
>>> +#define ENET_BD_R              (1 << 15)
>>> +#define ENET_BD_E              (1 << 15)
>>> +#define ENET_BD_O1             (1 << 14)
>>> +#define ENET_BD_W              (1 << 13)
>>> +#define ENET_BD_O2             (1 << 12)
>>> +#define ENET_BD_L              (1 << 11)
>>> +#define ENET_BD_TC             (1 << 10)
>>> +#define ENET_BD_ABC            (1 << 9)
>>> +#define ENET_BD_M              (1 << 8)
>>> +#define ENET_BD_BC             (1 << 7)
>>> +#define ENET_BD_MC             (1 << 6)
>>> +#define ENET_BD_LG             (1 << 5)
>>> +#define ENET_BD_NO             (1 << 4)
>>> +#define ENET_BD_CR             (1 << 2)
>>> +#define ENET_BD_OV             (1 << 1)
>>> +#define ENET_BD_TR             (1 << 0)
>>> +
>>> +typedef struct {
>>> +    uint16_t length;
>>> +    uint16_t flags;
>>> +    uint32_t data;
>>> +    uint16_t status;
>>> +    uint16_t option;
>>> +    uint16_t checksum;
>>> +    uint16_t head_proto;
>>> +    uint32_t last_buffer;
>>> +    uint32_t timestamp;
>>> +    uint32_t reserved[2];
>>> +} IMXENETBufDesc;
>>> +
>>> +#define ENET_BD_ME             (1 << 15)
>>> +#define ENET_BD_TX_INT         (1 << 14)
>>> +#define ENET_BD_TS             (1 << 13)
>>> +#define ENET_BD_PINS           (1 << 12)
>>> +#define ENET_BD_IINS           (1 << 11)
>>> +#define ENET_BD_PE             (1 << 10)
>>> +#define ENET_BD_CE             (1 << 9)
>>> +#define ENET_BD_UC             (1 << 8)
>>> +#define ENET_BD_RX_INT         (1 << 7)
>>> +
>>> +#define ENET_BD_TXE            (1 << 15)
>>> +#define ENET_BD_UE             (1 << 13)
>>> +#define ENET_BD_EE             (1 << 12)
>>> +#define ENET_BD_FE             (1 << 11)
>>> +#define ENET_BD_LCE            (1 << 10)
>>> +#define ENET_BD_OE             (1 << 9)
>>> +#define ENET_BD_TSE            (1 << 8)
>>> +#define ENET_BD_ICE            (1 << 5)
>>> +#define ENET_BD_PCR            (1 << 4)
>>> +#define ENET_BD_VLAN           (1 << 2)
>>> +#define ENET_BD_IPV6           (1 << 1)
>>> +#define ENET_BD_FRAG           (1 << 0)
>>> +
>>> +#define ENET_BD_BDU            (1 << 31)
>>>     typedef struct IMXFECState {
>>>       /*< private >*/
>>> @@ -113,7 +236,7 @@ typedef struct IMXFECState {
>>>       /*< public >*/
>>>       NICState *nic;
>>>       NICConf conf;
>>> -    qemu_irq irq;
>>> +    qemu_irq irq[2];
>>>       MemoryRegion iomem;
>>>         uint32_t regs[ENET_MAX];
>>> @@ -125,6 +248,8 @@ typedef struct IMXFECState {
>>>       uint32_t phy_advertise;
>>>       uint32_t phy_int;
>>>       uint32_t phy_int_mask;
>>> +
>>> +    bool is_fec;
>>>   } IMXFECState;
>>>     #endif
>>
>>
>