[dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Ravi Kerur]

Hi,

We have a Gigabyte H97N motherboard which has I217 Intel chipset which uses
e100e drivers. I looked into lib/librte_pmd_e1000 directory and I do see
that e1000e code is integrated but missing some support for read/write from
flash_address and other minor things. I have made changes shown below and
have done some testing with testpmd utility and now have following questions

1. What amount of testing is required to qualify patch as successfully
tested on new chipsets

2. FreeBSD testing, currently we have Ubuntu 14.04 installed on existing
H97N motherboard and testing is done solely on Linux. We plan to get
another motherboard which will have I218 chipset and still deciding whether
to go with FreeBSD or Ubuntu. So the question I have is what amount of
testing should be done on FreeBSD? I don't think setup.sh/dpdk_nic_bind.py
works on FreeBSD yet hence the question on testing.

Thanks,
Ravi



On Sun, Jan 4, 2015 at 3:15 PM, Ravi Kerur <rkerur@gmail.com> wrote:

> This patch adds support for I217 and I218 Intel chipsets.
> Gigabyte H97N motherboard has I217 Intel chipsets and changes
> include
>
> 1. Add relevant device-ids via RTE_PCI_DEV_ID_DECL_EM
> 2. Add support for memory mapped flash address read/write
>
> Basic testing on Ubuntu with testpmd utility.
>
> Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> ---
>  lib/librte_eal/common/include/rte_pci_dev_ids.h |  4 ++++
>  lib/librte_pmd_e1000/e1000/e1000_api.c          | 21 +++++++++++++++++++++
>  lib/librte_pmd_e1000/e1000/e1000_api.h          |  1 +
>  lib/librte_pmd_e1000/e1000/e1000_osdep.h        | 24
> +++++++++++++++++++-----
>  lib/librte_pmd_e1000/em_ethdev.c                |  7 +++++++
>  5 files changed, 52 insertions(+), 5 deletions(-)
>
> diff --git a/lib/librte_eal/common/include/rte_pci_dev_ids.h
> b/lib/librte_eal/common/include/rte_pci_dev_ids.h
> index c922de9..712793a 100644
> --- a/lib/librte_eal/common/include/rte_pci_dev_ids.h
> +++ b/lib/librte_eal/common/include/rte_pci_dev_ids.h
> @@ -274,6 +274,10 @@ RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> E1000_DEV_ID_82572EI)
>  RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL, E1000_DEV_ID_82573L)
>  RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL, E1000_DEV_ID_82574L)
>  RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL, E1000_DEV_ID_82574LA)
> +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL, E1000_DEV_ID_PCH_LPT_I217_LM)
> +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL, E1000_DEV_ID_PCH_LPT_I217_V)
> +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> E1000_DEV_ID_PCH_LPTLP_I218_LM)
> +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL, E1000_DEV_ID_PCH_LPTLP_I218_V)
>
>  /******************** Physical IGB devices from e1000_hw.h
> ********************/
>
> diff --git a/lib/librte_pmd_e1000/e1000/e1000_api.c
> b/lib/librte_pmd_e1000/e1000/e1000_api.c
> index a064565..30ed1f3 100644
> --- a/lib/librte_pmd_e1000/e1000/e1000_api.c
> +++ b/lib/librte_pmd_e1000/e1000/e1000_api.c
> @@ -1355,3 +1355,24 @@ void e1000_shutdown_fiber_serdes_link(struct
> e1000_hw *hw)
>                 hw->mac.ops.shutdown_serdes(hw);
>  }
>
> +/**
> + *  e1000_device_is_ich8 - Check for ICH8 device
> + *  @hw: pointer to the HW structure
> + *
> + *  return TRUE for ICH8, otherwise FALSE
> + **/
> +bool e1000_device_is_ich8(struct e1000_hw *hw)
> +{
> +       DEBUGFUNC("e1000_device_is_ich8");
> +
> +       switch (hw->device_id) {
> +       case E1000_DEV_ID_PCH_LPT_I217_LM:
> +       case E1000_DEV_ID_PCH_LPT_I217_V:
> +       case E1000_DEV_ID_PCH_LPTLP_I218_LM:
> +       case E1000_DEV_ID_PCH_LPTLP_I218_V:
> +               return 1;
> +
> +       default:
> +               return 0;
> +       }
> +}
> diff --git a/lib/librte_pmd_e1000/e1000/e1000_api.h
> b/lib/librte_pmd_e1000/e1000/e1000_api.h
> index 02b16da..f96a674 100644
> --- a/lib/librte_pmd_e1000/e1000/e1000_api.h
> +++ b/lib/librte_pmd_e1000/e1000/e1000_api.h
> @@ -49,6 +49,7 @@ extern void e1000_init_function_pointers_vf(struct
> e1000_hw *hw);
>  extern void e1000_power_up_fiber_serdes_link(struct e1000_hw *hw);
>  extern void e1000_shutdown_fiber_serdes_link(struct e1000_hw *hw);
>  extern void e1000_init_function_pointers_i210(struct e1000_hw *hw);
> +extern bool e1000_device_is_ich8(struct e1000_hw *hw);
>
>  s32 e1000_set_obff_timer(struct e1000_hw *hw, u32 itr);
>  s32 e1000_set_mac_type(struct e1000_hw *hw);
> diff --git a/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> b/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> index 438641e..19146a3 100644
> --- a/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> +++ b/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> @@ -95,13 +95,22 @@ typedef int         bool;
>
>  #define E1000_PCI_REG(reg) (*((volatile uint32_t *)(reg)))
>
> +#define E1000_PCI_REG16(reg) (*((volatile uint16_t *)(reg)))
> +
>  #define E1000_PCI_REG_WRITE(reg, value) do { \
>         E1000_PCI_REG((reg)) = (value); \
>  } while (0)
>
> +#define E1000_PCI_REG_WRITE16(reg, value) do { \
> +       E1000_PCI_REG16((reg)) = (value); \
> +} while (0)
> +
>  #define E1000_PCI_REG_ADDR(hw, reg) \
>         ((volatile uint32_t *)((char *)(hw)->hw_addr + (reg)))
>
> +#define E1000_PCI_REG_FLASH_ADDR(hw, reg) \
> +       ((volatile uint32_t *)((char *)(hw)->flash_address + (reg)))
> +
>  #define E1000_PCI_REG_ARRAY_ADDR(hw, reg, index) \
>         E1000_PCI_REG_ADDR((hw), (reg) + ((index) << 2))
>
> @@ -110,6 +119,11 @@ static inline uint32_t e1000_read_addr(volatile void*
> addr)
>         return E1000_PCI_REG(addr);
>  }
>
> +static inline uint32_t e1000_read_addr16(volatile void* addr)
> +{
> +       return E1000_PCI_REG16(addr);
> +}
> +
>  /* Necessary defines */
>  #define E1000_MRQC_ENABLE_MASK                  0x00000007
>  #define E1000_MRQC_RSS_FIELD_IPV6_EX           0x00080000
> @@ -155,20 +169,20 @@ static inline uint32_t e1000_read_addr(volatile
> void* addr)
>         E1000_WRITE_REG(hw, reg, value)
>
>  /*
> - * Not implemented.
> + * Tested on I217 chipset.
>   */
>
>  #define E1000_READ_FLASH_REG(hw, reg) \
> -       (E1000_ACCESS_PANIC(E1000_READ_FLASH_REG, hw, reg, 0), 0)
> +       e1000_read_addr(E1000_PCI_REG_FLASH_ADDR((hw), (reg)))
>
>  #define E1000_READ_FLASH_REG16(hw, reg)  \
> -       (E1000_ACCESS_PANIC(E1000_READ_FLASH_REG16, hw, reg, 0), 0)
> +       e1000_read_addr16(E1000_PCI_REG_FLASH_ADDR((hw), (reg)))
>
>  #define E1000_WRITE_FLASH_REG(hw, reg, value)  \
> -       E1000_ACCESS_PANIC(E1000_WRITE_FLASH_REG, hw, reg, value)
> +       E1000_PCI_REG_WRITE(E1000_PCI_REG_FLASH_ADDR((hw), (reg)), (value))
>
>  #define E1000_WRITE_FLASH_REG16(hw, reg, value) \
> -       E1000_ACCESS_PANIC(E1000_WRITE_FLASH_REG16, hw, reg, value)
> +       E1000_PCI_REG_WRITE16(E1000_PCI_REG_FLASH_ADDR((hw), (reg)),
> (value))
>
>  #define STATIC static
>
> diff --git a/lib/librte_pmd_e1000/em_ethdev.c
> b/lib/librte_pmd_e1000/em_ethdev.c
> index 3f2897e..643f5cd 100644
> --- a/lib/librte_pmd_e1000/em_ethdev.c
> +++ b/lib/librte_pmd_e1000/em_ethdev.c
> @@ -245,6 +245,9 @@ eth_em_dev_init(__attribute__((unused)) struct
> eth_driver *eth_drv,
>         hw->device_id = pci_dev->id.device_id;
>
>         /* For ICH8 support we'll need to map the flash memory BAR */
> +       if (e1000_device_is_ich8(hw))
> +               hw->flash_address = (void *)pci_dev->mem_resource[1].addr;
> +
>
>         if (e1000_setup_init_funcs(hw, TRUE) != E1000_SUCCESS ||
>                         em_hw_init(hw) != 0) {
> @@ -436,6 +439,7 @@ em_set_pba(struct e1000_hw *hw)
>                         break;
>                 case e1000_pchlan:
>                 case e1000_pch2lan:
> +               case e1000_pch_lpt:
>                         pba = E1000_PBA_26K;
>                         break;
>                 default:
> @@ -678,6 +682,8 @@ em_hardware_init(struct e1000_hw *hw)
>         /* Workaround: no TX flow ctrl for PCH */
>         if (hw->mac.type == e1000_pchlan)
>                 hw->fc.requested_mode = e1000_fc_rx_pause;
> +       else if (hw->mac.type == e1000_pch_lpt)
> +               hw->fc.requested_mode = e1000_fc_full;
>
>         /* Override - settings for PCH2LAN, ya its magic :) */
>         if (hw->mac.type == e1000_pch2lan) {
> @@ -845,6 +851,7 @@ em_get_max_pktlen(const struct e1000_hw *hw)
>         case e1000_pch2lan:
>         case e1000_82574:
>         case e1000_80003es2lan: /* 9K Jumbo Frame size */
> +       case e1000_pch_lpt:
>                 return (0x2412);
>         case e1000_pchlan:
>                 return (0x1000);
> --
> 1.9.1
>
>

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Thomas Monjalon]

2015-01-04 15:28, Ravi Kerur:
> We have a Gigabyte H97N motherboard which has I217 Intel chipset which uses
> e100e drivers. I looked into lib/librte_pmd_e1000 directory and I do see
> that e1000e code is integrated but missing some support for read/write from
> flash_address and other minor things. I have made changes shown below and
> have done some testing with testpmd utility and now have following questions
> 
> 1. What amount of testing is required to qualify patch as successfully
> tested on new chipsets

There is no good answer to this question. Generally, you must be sure that
you don't break anything.
So you must test the code paths you have changed.

> 2. FreeBSD testing, currently we have Ubuntu 14.04 installed on existing
> H97N motherboard and testing is done solely on Linux. We plan to get
> another motherboard which will have I218 chipset and still deciding whether
> to go with FreeBSD or Ubuntu. So the question I have is what amount of
> testing should be done on FreeBSD? I don't think setup.sh/dpdk_nic_bind.py
> works on FreeBSD yet hence the question on testing.

FreeBSD testing is required when patching common EAL, scripts or makefiles.

> >  lib/librte_pmd_e1000/e1000/e1000_api.c          | 21 +++++++++++++++++++++
> >  lib/librte_pmd_e1000/e1000/e1000_api.h          |  1 +
> >  lib/librte_pmd_e1000/e1000/e1000_osdep.h        | 24 +++++++++++++++++++-----

These files are part of the base driver.
The rule is to not patch them and try to do the changes in PMD only.
There can be exceptions if an Intel maintainer acknowledges it.

-- 
Thomas

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Ravi Kerur]

Inline <rk>

On Mon, Jan 5, 2015 at 12:55 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> 2015-01-04 15:28, Ravi Kerur:
> > We have a Gigabyte H97N motherboard which has I217 Intel chipset which
> uses
> > e100e drivers. I looked into lib/librte_pmd_e1000 directory and I do see
> > that e1000e code is integrated but missing some support for read/write
> from
> > flash_address and other minor things. I have made changes shown below and
> > have done some testing with testpmd utility and now have following
> questions
> >
> > 1. What amount of testing is required to qualify patch as successfully
> > tested on new chipsets
>
> There is no good answer to this question. Generally, you must be sure that
> you don't break anything.
> So you must test the code paths you have changed.
>

<rk> yes I have done testing on Ubuntu for I217 using testpmd.

>
> > 2. FreeBSD testing, currently we have Ubuntu 14.04 installed on existing
> > H97N motherboard and testing is done solely on Linux. We plan to get
> > another motherboard which will have I218 chipset and still deciding
> whether
> > to go with FreeBSD or Ubuntu. So the question I have is what amount of
> > testing should be done on FreeBSD? I don't think
> setup.sh/dpdk_nic_bind.py
> > works on FreeBSD yet hence the question on testing.
>
> FreeBSD testing is required when patching common EAL, scripts or makefiles.
>
> > >  lib/librte_pmd_e1000/e1000/e1000_api.c          | 21
> +++++++++++++++++++++
> > >  lib/librte_pmd_e1000/e1000/e1000_api.h          |  1 +
> > >  lib/librte_pmd_e1000/e1000/e1000_osdep.h        | 24
> +++++++++++++++++++-----
>
> These files are part of the base driver.
> The rule is to not patch them and try to do the changes in PMD only.
> There can be exceptions if an Intel maintainer acknowledges it.
>

<rk>  Changes in these files are modifying existing macros

E1000_READ_FLASH_REG,
E1000_WRITE_FLASH_REG
...

If it is not recommended to modify these files, should I move macros into
some PMD file?

Thanks.

>
> --
> Thomas
>

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Ravi Kerur]

Thomas,

Please let me know how I can move forward on this. If i confine changes in
e1000/ directory to e1000_osdep.h file only and the rest in PMD will that
work? The reason I ask is because of following comment  in README file.

...
Few changes to the original FreeBSD sources were made to:
- Adopt it for PMD usage mode:
        e1000_osdep.c
        e1000_osdep.h
...

Thanks.

On Mon, Jan 5, 2015 at 8:40 AM, Ravi Kerur <rkerur@gmail.com> wrote:

> Inline <rk>
>
> On Mon, Jan 5, 2015 at 12:55 AM, Thomas Monjalon <
> thomas.monjalon@6wind.com> wrote:
>
>> 2015-01-04 15:28, Ravi Kerur:
>> > We have a Gigabyte H97N motherboard which has I217 Intel chipset which
>> uses
>> > e100e drivers. I looked into lib/librte_pmd_e1000 directory and I do see
>> > that e1000e code is integrated but missing some support for read/write
>> from
>> > flash_address and other minor things. I have made changes shown below
>> and
>> > have done some testing with testpmd utility and now have following
>> questions
>> >
>> > 1. What amount of testing is required to qualify patch as successfully
>> > tested on new chipsets
>>
>> There is no good answer to this question. Generally, you must be sure that
>> you don't break anything.
>> So you must test the code paths you have changed.
>>
>
> <rk> yes I have done testing on Ubuntu for I217 using testpmd.
>
>>
>> > 2. FreeBSD testing, currently we have Ubuntu 14.04 installed on existing
>> > H97N motherboard and testing is done solely on Linux. We plan to get
>> > another motherboard which will have I218 chipset and still deciding
>> whether
>> > to go with FreeBSD or Ubuntu. So the question I have is what amount of
>> > testing should be done on FreeBSD? I don't think
>> setup.sh/dpdk_nic_bind.py
>> > works on FreeBSD yet hence the question on testing.
>>
>> FreeBSD testing is required when patching common EAL, scripts or
>> makefiles.
>>
>> > >  lib/librte_pmd_e1000/e1000/e1000_api.c          | 21
>> +++++++++++++++++++++
>> > >  lib/librte_pmd_e1000/e1000/e1000_api.h          |  1 +
>> > >  lib/librte_pmd_e1000/e1000/e1000_osdep.h        | 24
>> +++++++++++++++++++-----
>>
>> These files are part of the base driver.
>> The rule is to not patch them and try to do the changes in PMD only.
>> There can be exceptions if an Intel maintainer acknowledges it.
>>
>
> <rk>  Changes in these files are modifying existing macros
>
> E1000_READ_FLASH_REG,
> E1000_WRITE_FLASH_REG
> ...
>
> If it is not recommended to modify these files, should I move macros into
> some PMD file?
>
> Thanks.
>
>>
>> --
>> Thomas
>>
>
>

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Thomas Monjalon]

2015-01-09 04:41, Ravi Kerur:
> Thomas,
> 
> Please let me know how I can move forward on this. If i confine changes in
> e1000/ directory to e1000_osdep.h file only and the rest in PMD will that
> work? The reason I ask is because of following comment  in README file.
> 
> ...
> Few changes to the original FreeBSD sources were made to:
> - Adopt it for PMD usage mode:
>         e1000_osdep.c
>         e1000_osdep.h
> ...

This is an Intel driver so you should ask to the responsible of this code at Intel.
The problem is that there is not really an identified responsible for this driver.

The rule is to not change the base driver, even osdep files.
But it would be better to have an exception here.


PS: please avoid top-posting.

> On Mon, Jan 5, 2015 at 8:40 AM, Ravi Kerur <rkerur@gmail.com> wrote:
> 
> > Inline <rk>
> >
> > On Mon, Jan 5, 2015 at 12:55 AM, Thomas Monjalon <
> > thomas.monjalon@6wind.com> wrote:
> >
> >> 2015-01-04 15:28, Ravi Kerur:
> >> > We have a Gigabyte H97N motherboard which has I217 Intel chipset which
> >> uses
> >> > e100e drivers. I looked into lib/librte_pmd_e1000 directory and I do see
> >> > that e1000e code is integrated but missing some support for read/write
> >> from
> >> > flash_address and other minor things. I have made changes shown below
> >> and
> >> > have done some testing with testpmd utility and now have following
> >> questions
> >> >
> >> > 1. What amount of testing is required to qualify patch as successfully
> >> > tested on new chipsets
> >>
> >> There is no good answer to this question. Generally, you must be sure that
> >> you don't break anything.
> >> So you must test the code paths you have changed.
> >>
> >
> > <rk> yes I have done testing on Ubuntu for I217 using testpmd.
> >
> >>
> >> > 2. FreeBSD testing, currently we have Ubuntu 14.04 installed on existing
> >> > H97N motherboard and testing is done solely on Linux. We plan to get
> >> > another motherboard which will have I218 chipset and still deciding
> >> whether
> >> > to go with FreeBSD or Ubuntu. So the question I have is what amount of
> >> > testing should be done on FreeBSD? I don't think
> >> setup.sh/dpdk_nic_bind.py
> >> > works on FreeBSD yet hence the question on testing.
> >>
> >> FreeBSD testing is required when patching common EAL, scripts or
> >> makefiles.
> >>
> >> > >  lib/librte_pmd_e1000/e1000/e1000_api.c          | 21
> >> +++++++++++++++++++++
> >> > >  lib/librte_pmd_e1000/e1000/e1000_api.h          |  1 +
> >> > >  lib/librte_pmd_e1000/e1000/e1000_osdep.h        | 24
> >> +++++++++++++++++++-----
> >>
> >> These files are part of the base driver.
> >> The rule is to not patch them and try to do the changes in PMD only.
> >> There can be exceptions if an Intel maintainer acknowledges it.
> >>
> >
> > <rk>  Changes in these files are modifying existing macros
> >
> > E1000_READ_FLASH_REG,
> > E1000_WRITE_FLASH_REG
> > ...
> >
> > If it is not recommended to modify these files, should I move macros into
> > some PMD file?
> >
> > Thanks.

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Ravi Kerur]

On Wed, Jan 14, 2015 at 8:27 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> 2015-01-09 04:41, Ravi Kerur:
> > Thomas,
> >
> > Please let me know how I can move forward on this. If i confine changes
> in
> > e1000/ directory to e1000_osdep.h file only and the rest in PMD will that
> > work? The reason I ask is because of following comment  in README file.
> >
> > ...
> > Few changes to the original FreeBSD sources were made to:
> > - Adopt it for PMD usage mode:
> >         e1000_osdep.c
> >         e1000_osdep.h
> > ...
>
> This is an Intel driver so you should ask to the responsible of this code
> at Intel.
> The problem is that there is not really an identified responsible for this
> driver.
>
> The rule is to not change the base driver, even osdep files.
> But it would be better to have an exception here.
>
>
> PS: please avoid top-posting.
>

<rk> Please let me know who is the contact person from Intel so I can add
him/her to "To" list when I send the patch or Should I contact Jim St Leger
and ask him about this?

Thanks.

>
> > On Mon, Jan 5, 2015 at 8:40 AM, Ravi Kerur <rkerur@gmail.com> wrote:
> >
> > > Inline <rk>
> > >
> > > On Mon, Jan 5, 2015 at 12:55 AM, Thomas Monjalon <
> > > thomas.monjalon@6wind.com> wrote:
> > >
> > >> 2015-01-04 15:28, Ravi Kerur:
> > >> > We have a Gigabyte H97N motherboard which has I217 Intel chipset
> which
> > >> uses
> > >> > e100e drivers. I looked into lib/librte_pmd_e1000 directory and I
> do see
> > >> > that e1000e code is integrated but missing some support for
> read/write
> > >> from
> > >> > flash_address and other minor things. I have made changes shown
> below
> > >> and
> > >> > have done some testing with testpmd utility and now have following
> > >> questions
> > >> >
> > >> > 1. What amount of testing is required to qualify patch as
> successfully
> > >> > tested on new chipsets
> > >>
> > >> There is no good answer to this question. Generally, you must be sure
> that
> > >> you don't break anything.
> > >> So you must test the code paths you have changed.
> > >>
> > >
> > > <rk> yes I have done testing on Ubuntu for I217 using testpmd.
> > >
> > >>
> > >> > 2. FreeBSD testing, currently we have Ubuntu 14.04 installed on
> existing
> > >> > H97N motherboard and testing is done solely on Linux. We plan to get
> > >> > another motherboard which will have I218 chipset and still deciding
> > >> whether
> > >> > to go with FreeBSD or Ubuntu. So the question I have is what amount
> of
> > >> > testing should be done on FreeBSD? I don't think
> > >> setup.sh/dpdk_nic_bind.py
> > >> > works on FreeBSD yet hence the question on testing.
> > >>
> > >> FreeBSD testing is required when patching common EAL, scripts or
> > >> makefiles.
> > >>
> > >> > >  lib/librte_pmd_e1000/e1000/e1000_api.c          | 21
> > >> +++++++++++++++++++++
> > >> > >  lib/librte_pmd_e1000/e1000/e1000_api.h          |  1 +
> > >> > >  lib/librte_pmd_e1000/e1000/e1000_osdep.h        | 24
> > >> +++++++++++++++++++-----
> > >>
> > >> These files are part of the base driver.
> > >> The rule is to not patch them and try to do the changes in PMD only.
> > >> There can be exceptions if an Intel maintainer acknowledges it.
> > >>
> > >
> > > <rk>  Changes in these files are modifying existing macros
> > >
> > > E1000_READ_FLASH_REG,
> > > E1000_WRITE_FLASH_REG
> > > ...
> > >
> > > If it is not recommended to modify these files, should I move macros
> into
> > > some PMD file?
> > >
> > > Thanks.
>
>

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[O'driscoll, Tim]

> On Thursday, January 15, 2015  at 8:34 PM, Ravi Kerur <rkerur@gmail.com> wrote:
> 
> On Wed, Jan 14, 2015 at 8:27 AM, Thomas Monjalon
> <thomas.monjalon@6wind.com>
> wrote:
> 
> > 2015-01-09 04:41, Ravi Kerur:
> > > Thomas,
> > >
> > > Please let me know how I can move forward on this. If i confine changes
> > in
> > > e1000/ directory to e1000_osdep.h file only and the rest in PMD will that
> > > work? The reason I ask is because of following comment  in README file.
> > >
> > > ...
> > > Few changes to the original FreeBSD sources were made to:
> > > - Adopt it for PMD usage mode:
> > >         e1000_osdep.c
> > >         e1000_osdep.h
> > > ...
> >
> > This is an Intel driver so you should ask to the responsible of this code
> > at Intel.
> > The problem is that there is not really an identified responsible for this
> > driver.
> >
> > The rule is to not change the base driver, even osdep files.
> > But it would be better to have an exception here.
> >
> >
> > PS: please avoid top-posting.
> >
> 
> <rk> Please let me know who is the contact person from Intel so I can add
> him/her to "To" list when I send the patch or Should I contact Jim St Leger
> and ask him about this?

We're looking into this, but don't have a definitive answer yet. Somebody from Intel will reply as soon as we've confirmed whether the change you proposed is OK or not.


Tim

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Ananyev, Konstantin]

Hi,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur
> Sent: Thursday, January 15, 2015 8:34 PM
> To: Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> 
> On Wed, Jan 14, 2015 at 8:27 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> 
> > 2015-01-09 04:41, Ravi Kerur:
> > > Thomas,
> > >
> > > Please let me know how I can move forward on this. If i confine changes
> > in
> > > e1000/ directory to e1000_osdep.h file only and the rest in PMD will that
> > > work? The reason I ask is because of following comment  in README file.
> > >
> > > ...
> > > Few changes to the original FreeBSD sources were made to:
> > > - Adopt it for PMD usage mode:
> > >         e1000_osdep.c
> > >         e1000_osdep.h
> > > ...

Yes, if needed you can modify these files.
In fact, these files are the only 2 that are allowed to be modified inside e1000 sub-directory.
As I understand you plan to implement E1000_READ_FLASH_REG  and E1000_WRITE_FLASH_REG
macros properly, correct?
Konstantin

> >
> > This is an Intel driver so you should ask to the responsible of this code
> > at Intel.
> > The problem is that there is not really an identified responsible for this
> > driver.
> >
> > The rule is to not change the base driver, even osdep files.
> > But it would be better to have an exception here.
> >
> >
> > PS: please avoid top-posting.
> >
> 
> <rk> Please let me know who is the contact person from Intel so I can add
> him/her to "To" list when I send the patch or Should I contact Jim St Leger
> and ask him about this?
> 
> Thanks.
> 
> >
> > > On Mon, Jan 5, 2015 at 8:40 AM, Ravi Kerur <rkerur@gmail.com> wrote:
> > >
> > > > Inline <rk>
> > > >
> > > > On Mon, Jan 5, 2015 at 12:55 AM, Thomas Monjalon <
> > > > thomas.monjalon@6wind.com> wrote:
> > > >
> > > >> 2015-01-04 15:28, Ravi Kerur:
> > > >> > We have a Gigabyte H97N motherboard which has I217 Intel chipset
> > which
> > > >> uses
> > > >> > e100e drivers. I looked into lib/librte_pmd_e1000 directory and I
> > do see
> > > >> > that e1000e code is integrated but missing some support for
> > read/write
> > > >> from
> > > >> > flash_address and other minor things. I have made changes shown
> > below
> > > >> and
> > > >> > have done some testing with testpmd utility and now have following
> > > >> questions
> > > >> >
> > > >> > 1. What amount of testing is required to qualify patch as
> > successfully
> > > >> > tested on new chipsets
> > > >>
> > > >> There is no good answer to this question. Generally, you must be sure
> > that
> > > >> you don't break anything.
> > > >> So you must test the code paths you have changed.
> > > >>
> > > >
> > > > <rk> yes I have done testing on Ubuntu for I217 using testpmd.
> > > >
> > > >>
> > > >> > 2. FreeBSD testing, currently we have Ubuntu 14.04 installed on
> > existing
> > > >> > H97N motherboard and testing is done solely on Linux. We plan to get
> > > >> > another motherboard which will have I218 chipset and still deciding
> > > >> whether
> > > >> > to go with FreeBSD or Ubuntu. So the question I have is what amount
> > of
> > > >> > testing should be done on FreeBSD? I don't think
> > > >> setup.sh/dpdk_nic_bind.py
> > > >> > works on FreeBSD yet hence the question on testing.
> > > >>
> > > >> FreeBSD testing is required when patching common EAL, scripts or
> > > >> makefiles.
> > > >>
> > > >> > >  lib/librte_pmd_e1000/e1000/e1000_api.c          | 21
> > > >> +++++++++++++++++++++
> > > >> > >  lib/librte_pmd_e1000/e1000/e1000_api.h          |  1 +
> > > >> > >  lib/librte_pmd_e1000/e1000/e1000_osdep.h        | 24
> > > >> +++++++++++++++++++-----
> > > >>
> > > >> These files are part of the base driver.
> > > >> The rule is to not patch them and try to do the changes in PMD only.
> > > >> There can be exceptions if an Intel maintainer acknowledges it.
> > > >>
> > > >
> > > > <rk>  Changes in these files are modifying existing macros
> > > >
> > > > E1000_READ_FLASH_REG,
> > > > E1000_WRITE_FLASH_REG
> > > > ...
> > > >
> > > > If it is not recommended to modify these files, should I move macros
> > into
> > > > some PMD file?
> > > >
> > > > Thanks.
> >
> >

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur
> Sent: Monday, January 5, 2015 7:28 AM
> To: dev@dpdk.org; Neil Horman; Thomas Monjalon
> Subject: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> 
> Hi,
> 
> We have a Gigabyte H97N motherboard which has I217 Intel chipset which
> uses e100e drivers. I looked into lib/librte_pmd_e1000 directory and I do see
> that e1000e code is integrated but missing some support for read/write from
> flash_address and other minor things. I have made changes shown below and
> have done some testing with testpmd utility and now have following questions
> 
> 1. What amount of testing is required to qualify patch as successfully tested on
> new chipsets
> 
> 2. FreeBSD testing, currently we have Ubuntu 14.04 installed on existing H97N
> motherboard and testing is done solely on Linux. We plan to get another
> motherboard which will have I218 chipset and still deciding whether to go with
> FreeBSD or Ubuntu. So the question I have is what amount of testing should be
> done on FreeBSD? I don't think setup.sh/dpdk_nic_bind.py works on FreeBSD
> yet hence the question on testing.
> 
> Thanks,
> Ravi
> 
> 
> 
> On Sun, Jan 4, 2015 at 3:15 PM, Ravi Kerur <rkerur@gmail.com> wrote:
> 
> > This patch adds support for I217 and I218 Intel chipsets.
> > Gigabyte H97N motherboard has I217 Intel chipsets and changes include
> >
> > 1. Add relevant device-ids via RTE_PCI_DEV_ID_DECL_EM 2. Add support
> > for memory mapped flash address read/write
> >
> > Basic testing on Ubuntu with testpmd utility.
> >
> > Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> > ---
> >  lib/librte_eal/common/include/rte_pci_dev_ids.h |  4 ++++
> >  lib/librte_pmd_e1000/e1000/e1000_api.c          | 21
> +++++++++++++++++++++
> >  lib/librte_pmd_e1000/e1000/e1000_api.h          |  1 +
> >  lib/librte_pmd_e1000/e1000/e1000_osdep.h        | 24
> > +++++++++++++++++++-----
> >  lib/librte_pmd_e1000/em_ethdev.c                |  7 +++++++
> >  5 files changed, 52 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/include/rte_pci_dev_ids.h
> > b/lib/librte_eal/common/include/rte_pci_dev_ids.h
> > index c922de9..712793a 100644
> > --- a/lib/librte_eal/common/include/rte_pci_dev_ids.h
> > +++ b/lib/librte_eal/common/include/rte_pci_dev_ids.h
> > @@ -274,6 +274,10 @@
> RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > E1000_DEV_ID_82572EI)
> >  RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> E1000_DEV_ID_82573L)
> > RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> E1000_DEV_ID_82574L)
> > RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> E1000_DEV_ID_82574LA)
> > +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > +E1000_DEV_ID_PCH_LPT_I217_LM)
> > +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > +E1000_DEV_ID_PCH_LPT_I217_V)
> > +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > E1000_DEV_ID_PCH_LPTLP_I218_LM)
> > +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > +E1000_DEV_ID_PCH_LPTLP_I218_V)
So, you are enabling new Intel(r) NICs. That's great, and thanks!

> >
> >  /******************** Physical IGB devices from e1000_hw.h
> > ********************/
> >
> > diff --git a/lib/librte_pmd_e1000/e1000/e1000_api.c
> > b/lib/librte_pmd_e1000/e1000/e1000_api.c
> > index a064565..30ed1f3 100644
> > --- a/lib/librte_pmd_e1000/e1000/e1000_api.c
> > +++ b/lib/librte_pmd_e1000/e1000/e1000_api.c
> > @@ -1355,3 +1355,24 @@ void e1000_shutdown_fiber_serdes_link(struct
> > e1000_hw *hw)
> >                 hw->mac.ops.shutdown_serdes(hw);  }
> >
> > +/**
> > + *  e1000_device_is_ich8 - Check for ICH8 device
> > + *  @hw: pointer to the HW structure
> > + *
> > + *  return TRUE for ICH8, otherwise FALSE  **/ bool
> > +e1000_device_is_ich8(struct e1000_hw *hw) {
> > +       DEBUGFUNC("e1000_device_is_ich8");
> > +
> > +       switch (hw->device_id) {
> > +       case E1000_DEV_ID_PCH_LPT_I217_LM:
> > +       case E1000_DEV_ID_PCH_LPT_I217_V:
> > +       case E1000_DEV_ID_PCH_LPTLP_I218_LM:
> > +       case E1000_DEV_ID_PCH_LPTLP_I218_V:
> > +               return 1;
> > +
> > +       default:
> > +               return 0;
> > +       }
> > +}
As Konstantin indicated, please do not try to modify any code in any source files in e1000
sub-folder, except e1000_osdep.c and e1000_osdep.h. If this piece of code is needed, please
try to move it to em_ethdev.c or e1000_osdep.c files!

> > diff --git a/lib/librte_pmd_e1000/e1000/e1000_api.h
> > b/lib/librte_pmd_e1000/e1000/e1000_api.h
> > index 02b16da..f96a674 100644
> > --- a/lib/librte_pmd_e1000/e1000/e1000_api.h
> > +++ b/lib/librte_pmd_e1000/e1000/e1000_api.h
> > @@ -49,6 +49,7 @@ extern void e1000_init_function_pointers_vf(struct
> > e1000_hw *hw);
> >  extern void e1000_power_up_fiber_serdes_link(struct e1000_hw *hw);
> > extern void e1000_shutdown_fiber_serdes_link(struct e1000_hw *hw);
> > extern void e1000_init_function_pointers_i210(struct e1000_hw *hw);
> > +extern bool e1000_device_is_ich8(struct e1000_hw *hw);
> >
> >  s32 e1000_set_obff_timer(struct e1000_hw *hw, u32 itr);
> >  s32 e1000_set_mac_type(struct e1000_hw *hw); diff --git
> > a/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> > b/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> > index 438641e..19146a3 100644
> > --- a/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> > +++ b/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> > @@ -95,13 +95,22 @@ typedef int         bool;
> >
> >  #define E1000_PCI_REG(reg) (*((volatile uint32_t *)(reg)))
> >
> > +#define E1000_PCI_REG16(reg) (*((volatile uint16_t *)(reg)))
> > +
> >  #define E1000_PCI_REG_WRITE(reg, value) do { \
> >         E1000_PCI_REG((reg)) = (value); \  } while (0)
> >
> > +#define E1000_PCI_REG_WRITE16(reg, value) do { \
> > +       E1000_PCI_REG16((reg)) = (value); \ } while (0)
> > +
> >  #define E1000_PCI_REG_ADDR(hw, reg) \
> >         ((volatile uint32_t *)((char *)(hw)->hw_addr + (reg)))
> >
> > +#define E1000_PCI_REG_FLASH_ADDR(hw, reg) \
> > +       ((volatile uint32_t *)((char *)(hw)->flash_address + (reg)))
> > +

> >  #define E1000_PCI_REG_ARRAY_ADDR(hw, reg, index) \
> >         E1000_PCI_REG_ADDR((hw), (reg) + ((index) << 2))
> >
> > @@ -110,6 +119,11 @@ static inline uint32_t e1000_read_addr(volatile
> > void*
> > addr)
> >         return E1000_PCI_REG(addr);
> >  }
> >
> > +static inline uint32_t e1000_read_addr16(volatile void* addr) {
> > +       return E1000_PCI_REG16(addr);
> > +}
> > +
> >  /* Necessary defines */
> >  #define E1000_MRQC_ENABLE_MASK                  0x00000007
> >  #define E1000_MRQC_RSS_FIELD_IPV6_EX           0x00080000
> > @@ -155,20 +169,20 @@ static inline uint32_t e1000_read_addr(volatile
> > void* addr)
> >         E1000_WRITE_REG(hw, reg, value)
> >
> >  /*
> > - * Not implemented.
> > + * Tested on I217 chipset.
> >   */
> >
> >  #define E1000_READ_FLASH_REG(hw, reg) \
> > -       (E1000_ACCESS_PANIC(E1000_READ_FLASH_REG, hw, reg, 0), 0)
> > +       e1000_read_addr(E1000_PCI_REG_FLASH_ADDR((hw), (reg)))
> >
> >  #define E1000_READ_FLASH_REG16(hw, reg)  \
> > -       (E1000_ACCESS_PANIC(E1000_READ_FLASH_REG16, hw, reg, 0), 0)
> > +       e1000_read_addr16(E1000_PCI_REG_FLASH_ADDR((hw), (reg)))
> >
> >  #define E1000_WRITE_FLASH_REG(hw, reg, value)  \
> > -       E1000_ACCESS_PANIC(E1000_WRITE_FLASH_REG, hw, reg, value)
> > +       E1000_PCI_REG_WRITE(E1000_PCI_REG_FLASH_ADDR((hw),
> (reg)),
> > + (value))
> >
> >  #define E1000_WRITE_FLASH_REG16(hw, reg, value) \
> > -       E1000_ACCESS_PANIC(E1000_WRITE_FLASH_REG16, hw, reg,
> value)
> > +       E1000_PCI_REG_WRITE16(E1000_PCI_REG_FLASH_ADDR((hw),
> (reg)),
> > (value))
> >
> >  #define STATIC static
> >
> > diff --git a/lib/librte_pmd_e1000/em_ethdev.c
> > b/lib/librte_pmd_e1000/em_ethdev.c
> > index 3f2897e..643f5cd 100644
> > --- a/lib/librte_pmd_e1000/em_ethdev.c
> > +++ b/lib/librte_pmd_e1000/em_ethdev.c
> > @@ -245,6 +245,9 @@ eth_em_dev_init(__attribute__((unused)) struct
> > eth_driver *eth_drv,
> >         hw->device_id = pci_dev->id.device_id;
> >
> >         /* For ICH8 support we'll need to map the flash memory BAR */
> > +       if (e1000_device_is_ich8(hw))
> > +               hw->flash_address = (void
> > + *)pci_dev->mem_resource[1].addr;
> > +
> >
> >         if (e1000_setup_init_funcs(hw, TRUE) != E1000_SUCCESS ||
> >                         em_hw_init(hw) != 0) { @@ -436,6 +439,7 @@
> > em_set_pba(struct e1000_hw *hw)
> >                         break;
> >                 case e1000_pchlan:
> >                 case e1000_pch2lan:
> > +               case e1000_pch_lpt:
> >                         pba = E1000_PBA_26K;
> >                         break;
> >                 default:
> > @@ -678,6 +682,8 @@ em_hardware_init(struct e1000_hw *hw)
> >         /* Workaround: no TX flow ctrl for PCH */
> >         if (hw->mac.type == e1000_pchlan)
> >                 hw->fc.requested_mode = e1000_fc_rx_pause;
> > +       else if (hw->mac.type == e1000_pch_lpt)
> > +               hw->fc.requested_mode = e1000_fc_full;
> >
> >         /* Override - settings for PCH2LAN, ya its magic :) */
> >         if (hw->mac.type == e1000_pch2lan) { @@ -845,6 +851,7 @@
> > em_get_max_pktlen(const struct e1000_hw *hw)
> >         case e1000_pch2lan:
> >         case e1000_82574:
> >         case e1000_80003es2lan: /* 9K Jumbo Frame size */
> > +       case e1000_pch_lpt:
> >                 return (0x2412);
> >         case e1000_pchlan:
> >                 return (0x1000);
> > --
> > 1.9.1
> >
> >

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Ravi Kerur]

Thanks Tim, Konstantin and Helin. I will send out revised patch.

Thanks,
Ravi

On Thu, Jan 15, 2015 at 5:14 PM, Zhang, Helin <helin.zhang@intel.com> wrote:

>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur
> > Sent: Monday, January 5, 2015 7:28 AM
> > To: dev@dpdk.org; Neil Horman; Thomas Monjalon
> > Subject: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> >
> > Hi,
> >
> > We have a Gigabyte H97N motherboard which has I217 Intel chipset which
> > uses e100e drivers. I looked into lib/librte_pmd_e1000 directory and I
> do see
> > that e1000e code is integrated but missing some support for read/write
> from
> > flash_address and other minor things. I have made changes shown below and
> > have done some testing with testpmd utility and now have following
> questions
> >
> > 1. What amount of testing is required to qualify patch as successfully
> tested on
> > new chipsets
> >
> > 2. FreeBSD testing, currently we have Ubuntu 14.04 installed on existing
> H97N
> > motherboard and testing is done solely on Linux. We plan to get another
> > motherboard which will have I218 chipset and still deciding whether to
> go with
> > FreeBSD or Ubuntu. So the question I have is what amount of testing
> should be
> > done on FreeBSD? I don't think setup.sh/dpdk_nic_bind.py works on
> FreeBSD
> > yet hence the question on testing.
> >
> > Thanks,
> > Ravi
> >
> >
> >
> > On Sun, Jan 4, 2015 at 3:15 PM, Ravi Kerur <rkerur@gmail.com> wrote:
> >
> > > This patch adds support for I217 and I218 Intel chipsets.
> > > Gigabyte H97N motherboard has I217 Intel chipsets and changes include
> > >
> > > 1. Add relevant device-ids via RTE_PCI_DEV_ID_DECL_EM 2. Add support
> > > for memory mapped flash address read/write
> > >
> > > Basic testing on Ubuntu with testpmd utility.
> > >
> > > Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> > > ---
> > >  lib/librte_eal/common/include/rte_pci_dev_ids.h |  4 ++++
> > >  lib/librte_pmd_e1000/e1000/e1000_api.c          | 21
> > +++++++++++++++++++++
> > >  lib/librte_pmd_e1000/e1000/e1000_api.h          |  1 +
> > >  lib/librte_pmd_e1000/e1000/e1000_osdep.h        | 24
> > > +++++++++++++++++++-----
> > >  lib/librte_pmd_e1000/em_ethdev.c                |  7 +++++++
> > >  5 files changed, 52 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/lib/librte_eal/common/include/rte_pci_dev_ids.h
> > > b/lib/librte_eal/common/include/rte_pci_dev_ids.h
> > > index c922de9..712793a 100644
> > > --- a/lib/librte_eal/common/include/rte_pci_dev_ids.h
> > > +++ b/lib/librte_eal/common/include/rte_pci_dev_ids.h
> > > @@ -274,6 +274,10 @@
> > RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > > E1000_DEV_ID_82572EI)
> > >  RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > E1000_DEV_ID_82573L)
> > > RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > E1000_DEV_ID_82574L)
> > > RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > E1000_DEV_ID_82574LA)
> > > +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > > +E1000_DEV_ID_PCH_LPT_I217_LM)
> > > +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > > +E1000_DEV_ID_PCH_LPT_I217_V)
> > > +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > > E1000_DEV_ID_PCH_LPTLP_I218_LM)
> > > +RTE_PCI_DEV_ID_DECL_EM(PCI_VENDOR_ID_INTEL,
> > > +E1000_DEV_ID_PCH_LPTLP_I218_V)
> So, you are enabling new Intel(r) NICs. That's great, and thanks!
>
> > >
> > >  /******************** Physical IGB devices from e1000_hw.h
> > > ********************/
> > >
> > > diff --git a/lib/librte_pmd_e1000/e1000/e1000_api.c
> > > b/lib/librte_pmd_e1000/e1000/e1000_api.c
> > > index a064565..30ed1f3 100644
> > > --- a/lib/librte_pmd_e1000/e1000/e1000_api.c
> > > +++ b/lib/librte_pmd_e1000/e1000/e1000_api.c
> > > @@ -1355,3 +1355,24 @@ void e1000_shutdown_fiber_serdes_link(struct
> > > e1000_hw *hw)
> > >                 hw->mac.ops.shutdown_serdes(hw);  }
> > >
> > > +/**
> > > + *  e1000_device_is_ich8 - Check for ICH8 device
> > > + *  @hw: pointer to the HW structure
> > > + *
> > > + *  return TRUE for ICH8, otherwise FALSE  **/ bool
> > > +e1000_device_is_ich8(struct e1000_hw *hw) {
> > > +       DEBUGFUNC("e1000_device_is_ich8");
> > > +
> > > +       switch (hw->device_id) {
> > > +       case E1000_DEV_ID_PCH_LPT_I217_LM:
> > > +       case E1000_DEV_ID_PCH_LPT_I217_V:
> > > +       case E1000_DEV_ID_PCH_LPTLP_I218_LM:
> > > +       case E1000_DEV_ID_PCH_LPTLP_I218_V:
> > > +               return 1;
> > > +
> > > +       default:
> > > +               return 0;
> > > +       }
> > > +}
> As Konstantin indicated, please do not try to modify any code in any
> source files in e1000
> sub-folder, except e1000_osdep.c and e1000_osdep.h. If this piece of code
> is needed, please
> try to move it to em_ethdev.c or e1000_osdep.c files!
>
> > > diff --git a/lib/librte_pmd_e1000/e1000/e1000_api.h
> > > b/lib/librte_pmd_e1000/e1000/e1000_api.h
> > > index 02b16da..f96a674 100644
> > > --- a/lib/librte_pmd_e1000/e1000/e1000_api.h
> > > +++ b/lib/librte_pmd_e1000/e1000/e1000_api.h
> > > @@ -49,6 +49,7 @@ extern void e1000_init_function_pointers_vf(struct
> > > e1000_hw *hw);
> > >  extern void e1000_power_up_fiber_serdes_link(struct e1000_hw *hw);
> > > extern void e1000_shutdown_fiber_serdes_link(struct e1000_hw *hw);
> > > extern void e1000_init_function_pointers_i210(struct e1000_hw *hw);
> > > +extern bool e1000_device_is_ich8(struct e1000_hw *hw);
> > >
> > >  s32 e1000_set_obff_timer(struct e1000_hw *hw, u32 itr);
> > >  s32 e1000_set_mac_type(struct e1000_hw *hw); diff --git
> > > a/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> > > b/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> > > index 438641e..19146a3 100644
> > > --- a/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> > > +++ b/lib/librte_pmd_e1000/e1000/e1000_osdep.h
> > > @@ -95,13 +95,22 @@ typedef int         bool;
> > >
> > >  #define E1000_PCI_REG(reg) (*((volatile uint32_t *)(reg)))
> > >
> > > +#define E1000_PCI_REG16(reg) (*((volatile uint16_t *)(reg)))
> > > +
> > >  #define E1000_PCI_REG_WRITE(reg, value) do { \
> > >         E1000_PCI_REG((reg)) = (value); \  } while (0)
> > >
> > > +#define E1000_PCI_REG_WRITE16(reg, value) do { \
> > > +       E1000_PCI_REG16((reg)) = (value); \ } while (0)
> > > +
> > >  #define E1000_PCI_REG_ADDR(hw, reg) \
> > >         ((volatile uint32_t *)((char *)(hw)->hw_addr + (reg)))
> > >
> > > +#define E1000_PCI_REG_FLASH_ADDR(hw, reg) \
> > > +       ((volatile uint32_t *)((char *)(hw)->flash_address + (reg)))
> > > +
>
> > >  #define E1000_PCI_REG_ARRAY_ADDR(hw, reg, index) \
> > >         E1000_PCI_REG_ADDR((hw), (reg) + ((index) << 2))
> > >
> > > @@ -110,6 +119,11 @@ static inline uint32_t e1000_read_addr(volatile
> > > void*
> > > addr)
> > >         return E1000_PCI_REG(addr);
> > >  }
> > >
> > > +static inline uint32_t e1000_read_addr16(volatile void* addr) {
> > > +       return E1000_PCI_REG16(addr);
> > > +}
> > > +
> > >  /* Necessary defines */
> > >  #define E1000_MRQC_ENABLE_MASK                  0x00000007
> > >  #define E1000_MRQC_RSS_FIELD_IPV6_EX           0x00080000
> > > @@ -155,20 +169,20 @@ static inline uint32_t e1000_read_addr(volatile
> > > void* addr)
> > >         E1000_WRITE_REG(hw, reg, value)
> > >
> > >  /*
> > > - * Not implemented.
> > > + * Tested on I217 chipset.
> > >   */
> > >
> > >  #define E1000_READ_FLASH_REG(hw, reg) \
> > > -       (E1000_ACCESS_PANIC(E1000_READ_FLASH_REG, hw, reg, 0), 0)
> > > +       e1000_read_addr(E1000_PCI_REG_FLASH_ADDR((hw), (reg)))
> > >
> > >  #define E1000_READ_FLASH_REG16(hw, reg)  \
> > > -       (E1000_ACCESS_PANIC(E1000_READ_FLASH_REG16, hw, reg, 0), 0)
> > > +       e1000_read_addr16(E1000_PCI_REG_FLASH_ADDR((hw), (reg)))
> > >
> > >  #define E1000_WRITE_FLASH_REG(hw, reg, value)  \
> > > -       E1000_ACCESS_PANIC(E1000_WRITE_FLASH_REG, hw, reg, value)
> > > +       E1000_PCI_REG_WRITE(E1000_PCI_REG_FLASH_ADDR((hw),
> > (reg)),
> > > + (value))
> > >
> > >  #define E1000_WRITE_FLASH_REG16(hw, reg, value) \
> > > -       E1000_ACCESS_PANIC(E1000_WRITE_FLASH_REG16, hw, reg,
> > value)
> > > +       E1000_PCI_REG_WRITE16(E1000_PCI_REG_FLASH_ADDR((hw),
> > (reg)),
> > > (value))
> > >
> > >  #define STATIC static
> > >
> > > diff --git a/lib/librte_pmd_e1000/em_ethdev.c
> > > b/lib/librte_pmd_e1000/em_ethdev.c
> > > index 3f2897e..643f5cd 100644
> > > --- a/lib/librte_pmd_e1000/em_ethdev.c
> > > +++ b/lib/librte_pmd_e1000/em_ethdev.c
> > > @@ -245,6 +245,9 @@ eth_em_dev_init(__attribute__((unused)) struct
> > > eth_driver *eth_drv,
> > >         hw->device_id = pci_dev->id.device_id;
> > >
> > >         /* For ICH8 support we'll need to map the flash memory BAR */
> > > +       if (e1000_device_is_ich8(hw))
> > > +               hw->flash_address = (void
> > > + *)pci_dev->mem_resource[1].addr;
> > > +
> > >
> > >         if (e1000_setup_init_funcs(hw, TRUE) != E1000_SUCCESS ||
> > >                         em_hw_init(hw) != 0) { @@ -436,6 +439,7 @@
> > > em_set_pba(struct e1000_hw *hw)
> > >                         break;
> > >                 case e1000_pchlan:
> > >                 case e1000_pch2lan:
> > > +               case e1000_pch_lpt:
> > >                         pba = E1000_PBA_26K;
> > >                         break;
> > >                 default:
> > > @@ -678,6 +682,8 @@ em_hardware_init(struct e1000_hw *hw)
> > >         /* Workaround: no TX flow ctrl for PCH */
> > >         if (hw->mac.type == e1000_pchlan)
> > >                 hw->fc.requested_mode = e1000_fc_rx_pause;
> > > +       else if (hw->mac.type == e1000_pch_lpt)
> > > +               hw->fc.requested_mode = e1000_fc_full;
> > >
> > >         /* Override - settings for PCH2LAN, ya its magic :) */
> > >         if (hw->mac.type == e1000_pch2lan) { @@ -845,6 +851,7 @@
> > > em_get_max_pktlen(const struct e1000_hw *hw)
> > >         case e1000_pch2lan:
> > >         case e1000_82574:
> > >         case e1000_80003es2lan: /* 9K Jumbo Frame size */
> > > +       case e1000_pch_lpt:
> > >                 return (0x2412);
> > >         case e1000_pchlan:
> > >                 return (0x1000);
> > > --
> > > 1.9.1
> > >
> > >
>

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Bruce Richardson]

On Thu, Jan 15, 2015 at 11:54:52PM +0000, Ananyev, Konstantin wrote:
> Hi,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur
> > Sent: Thursday, January 15, 2015 8:34 PM
> > To: Thomas Monjalon
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> > 
> > On Wed, Jan 14, 2015 at 8:27 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
> > wrote:
> > 
> > > 2015-01-09 04:41, Ravi Kerur:
> > > > Thomas,
> > > >
> > > > Please let me know how I can move forward on this. If i confine changes
> > > in
> > > > e1000/ directory to e1000_osdep.h file only and the rest in PMD will that
> > > > work? The reason I ask is because of following comment  in README file.
> > > >
> > > > ...
> > > > Few changes to the original FreeBSD sources were made to:
> > > > - Adopt it for PMD usage mode:
> > > >         e1000_osdep.c
> > > >         e1000_osdep.h
> > > > ...
> 
> Yes, if needed you can modify these files.
> In fact, these files are the only 2 that are allowed to be modified inside e1000 sub-directory.
> As I understand you plan to implement E1000_READ_FLASH_REG  and E1000_WRITE_FLASH_REG
> macros properly, correct?
> Konstantin
> 

As a cleanup we should really look to move these two files out of the e1000
subdirectory (and similarly for the ixgbe versions etc.), so as to give a cleaner
and more manageable separation between what can be edited or not.

/Bruce

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Ananyev, Konstantin]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Friday, January 16, 2015 10:53 AM
> To: Ananyev, Konstantin
> Cc: Ravi Kerur; Thomas Monjalon; dev@dpdk.org
> Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> 
> On Thu, Jan 15, 2015 at 11:54:52PM +0000, Ananyev, Konstantin wrote:
> > Hi,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur
> > > Sent: Thursday, January 15, 2015 8:34 PM
> > > To: Thomas Monjalon
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> > >
> > > On Wed, Jan 14, 2015 at 8:27 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
> > > wrote:
> > >
> > > > 2015-01-09 04:41, Ravi Kerur:
> > > > > Thomas,
> > > > >
> > > > > Please let me know how I can move forward on this. If i confine changes
> > > > in
> > > > > e1000/ directory to e1000_osdep.h file only and the rest in PMD will that
> > > > > work? The reason I ask is because of following comment  in README file.
> > > > >
> > > > > ...
> > > > > Few changes to the original FreeBSD sources were made to:
> > > > > - Adopt it for PMD usage mode:
> > > > >         e1000_osdep.c
> > > > >         e1000_osdep.h
> > > > > ...
> >
> > Yes, if needed you can modify these files.
> > In fact, these files are the only 2 that are allowed to be modified inside e1000 sub-directory.
> > As I understand you plan to implement E1000_READ_FLASH_REG  and E1000_WRITE_FLASH_REG
> > macros properly, correct?
> > Konstantin
> >
> 
> As a cleanup we should really look to move these two files out of the e1000
> subdirectory (and similarly for the ixgbe versions etc.), so as to give a cleaner
> and more manageable separation between what can be edited or not.

It was always like that for all Intel PMDs we have:

$ find lib/ -name '*_osdep.*' | grep -v acl
lib/librte_pmd_vmxnet3/vmxnet3/vmxnet3_osdep.h
lib/librte_pmd_ixgbe/ixgbe/ixgbe_osdep.h
lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_osdep.h
lib/librte_eal/linuxapp/kni/ethtool/igb/e1000_osdep.h
lib/librte_pmd_i40e/i40e/i40e_osdep.h
lib/librte_pmd_e1000/e1000/e1000_osdep.c
lib/librte_pmd_e1000/e1000/e1000_osdep.h

As I understand ND has it's own version of <drvname>_osdep.* for each OS they support.
We obviously modify it to fit DPDK purposes.

Konstantin

> 
> /Bruce

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Bruce Richardson]

On Fri, Jan 16, 2015 at 11:08:46AM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Friday, January 16, 2015 10:53 AM
> > To: Ananyev, Konstantin
> > Cc: Ravi Kerur; Thomas Monjalon; dev@dpdk.org
> > Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> > 
> > On Thu, Jan 15, 2015 at 11:54:52PM +0000, Ananyev, Konstantin wrote:
> > > Hi,
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur
> > > > Sent: Thursday, January 15, 2015 8:34 PM
> > > > To: Thomas Monjalon
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> > > >
> > > > On Wed, Jan 14, 2015 at 8:27 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
> > > > wrote:
> > > >
> > > > > 2015-01-09 04:41, Ravi Kerur:
> > > > > > Thomas,
> > > > > >
> > > > > > Please let me know how I can move forward on this. If i confine changes
> > > > > in
> > > > > > e1000/ directory to e1000_osdep.h file only and the rest in PMD will that
> > > > > > work? The reason I ask is because of following comment  in README file.
> > > > > >
> > > > > > ...
> > > > > > Few changes to the original FreeBSD sources were made to:
> > > > > > - Adopt it for PMD usage mode:
> > > > > >         e1000_osdep.c
> > > > > >         e1000_osdep.h
> > > > > > ...
> > >
> > > Yes, if needed you can modify these files.
> > > In fact, these files are the only 2 that are allowed to be modified inside e1000 sub-directory.
> > > As I understand you plan to implement E1000_READ_FLASH_REG  and E1000_WRITE_FLASH_REG
> > > macros properly, correct?
> > > Konstantin
> > >
> > 
> > As a cleanup we should really look to move these two files out of the e1000
> > subdirectory (and similarly for the ixgbe versions etc.), so as to give a cleaner
> > and more manageable separation between what can be edited or not.
> 
> It was always like that for all Intel PMDs we have:
> 
> $ find lib/ -name '*_osdep.*' | grep -v acl
> lib/librte_pmd_vmxnet3/vmxnet3/vmxnet3_osdep.h
> lib/librte_pmd_ixgbe/ixgbe/ixgbe_osdep.h
> lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_osdep.h
> lib/librte_eal/linuxapp/kni/ethtool/igb/e1000_osdep.h
> lib/librte_pmd_i40e/i40e/i40e_osdep.h
> lib/librte_pmd_e1000/e1000/e1000_osdep.c
> lib/librte_pmd_e1000/e1000/e1000_osdep.h
> 
> As I understand ND has it's own version of <drvname>_osdep.* for each OS they support.
> We obviously modify it to fit DPDK purposes.
> 
> Konstantin
> 
> > 
> > /Bruce

Yep. Doesn't mean we haven't put it in the wrong place though! :-)

/Bruce

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Ananyev, Konstantin]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Friday, January 16, 2015 11:32 AM
> To: Ananyev, Konstantin
> Cc: Ravi Kerur; Thomas Monjalon; dev@dpdk.org
> Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> 
> On Fri, Jan 16, 2015 at 11:08:46AM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Richardson, Bruce
> > > Sent: Friday, January 16, 2015 10:53 AM
> > > To: Ananyev, Konstantin
> > > Cc: Ravi Kerur; Thomas Monjalon; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> > >
> > > On Thu, Jan 15, 2015 at 11:54:52PM +0000, Ananyev, Konstantin wrote:
> > > > Hi,
> > > >
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur
> > > > > Sent: Thursday, January 15, 2015 8:34 PM
> > > > > To: Thomas Monjalon
> > > > > Cc: dev@dpdk.org
> > > > > Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> > > > >
> > > > > On Wed, Jan 14, 2015 at 8:27 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
> > > > > wrote:
> > > > >
> > > > > > 2015-01-09 04:41, Ravi Kerur:
> > > > > > > Thomas,
> > > > > > >
> > > > > > > Please let me know how I can move forward on this. If i confine changes
> > > > > > in
> > > > > > > e1000/ directory to e1000_osdep.h file only and the rest in PMD will that
> > > > > > > work? The reason I ask is because of following comment  in README file.
> > > > > > >
> > > > > > > ...
> > > > > > > Few changes to the original FreeBSD sources were made to:
> > > > > > > - Adopt it for PMD usage mode:
> > > > > > >         e1000_osdep.c
> > > > > > >         e1000_osdep.h
> > > > > > > ...
> > > >
> > > > Yes, if needed you can modify these files.
> > > > In fact, these files are the only 2 that are allowed to be modified inside e1000 sub-directory.
> > > > As I understand you plan to implement E1000_READ_FLASH_REG  and E1000_WRITE_FLASH_REG
> > > > macros properly, correct?
> > > > Konstantin
> > > >
> > >
> > > As a cleanup we should really look to move these two files out of the e1000
> > > subdirectory (and similarly for the ixgbe versions etc.), so as to give a cleaner
> > > and more manageable separation between what can be edited or not.
> >
> > It was always like that for all Intel PMDs we have:
> >
> > $ find lib/ -name '*_osdep.*' | grep -v acl
> > lib/librte_pmd_vmxnet3/vmxnet3/vmxnet3_osdep.h
> > lib/librte_pmd_ixgbe/ixgbe/ixgbe_osdep.h
> > lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_osdep.h
> > lib/librte_eal/linuxapp/kni/ethtool/igb/e1000_osdep.h
> > lib/librte_pmd_i40e/i40e/i40e_osdep.h
> > lib/librte_pmd_e1000/e1000/e1000_osdep.c
> > lib/librte_pmd_e1000/e1000/e1000_osdep.h
> >
> > As I understand ND has it's own version of <drvname>_osdep.* for each OS they support.
> > We obviously modify it to fit DPDK purposes.
> >
> > Konstantin
> >
> > >
> > > /Bruce
> 
> Yep. Doesn't mean we haven't put it in the wrong place though! :-)

We just don't move it at all :)
It is at the same place where ND puts it, we just modify the contents.
>From my point - current location is perfectly ok.
Konstantin

> 
> /Bruce

Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.

[Ravi Kerur]

Intel team,

Please let me know what additional testing needs to be done for I217/I218?
I have confined changes only to _osdep_ files and have done basic testing
with testpmd utility. Since DPDK PMD driver supporting e1000e  has been
available for quite sometime, I have assumed basic testing for Tx/Rx
packets should suffice.

Thanks,
Ravi

On Fri, Jan 16, 2015 at 4:01 AM, Ananyev, Konstantin <
konstantin.ananyev@intel.com> wrote:

>
>
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Friday, January 16, 2015 11:32 AM
> > To: Ananyev, Konstantin
> > Cc: Ravi Kerur; Thomas Monjalon; dev@dpdk.org
> > Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel chipsets.
> >
> > On Fri, Jan 16, 2015 at 11:08:46AM +0000, Ananyev, Konstantin wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Richardson, Bruce
> > > > Sent: Friday, January 16, 2015 10:53 AM
> > > > To: Ananyev, Konstantin
> > > > Cc: Ravi Kerur; Thomas Monjalon; dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel
> chipsets.
> > > >
> > > > On Thu, Jan 15, 2015 at 11:54:52PM +0000, Ananyev, Konstantin wrote:
> > > > > Hi,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur
> > > > > > Sent: Thursday, January 15, 2015 8:34 PM
> > > > > > To: Thomas Monjalon
> > > > > > Cc: dev@dpdk.org
> > > > > > Subject: Re: [dpdk-dev] Q on Support for I217 and I218 Intel
> chipsets.
> > > > > >
> > > > > > On Wed, Jan 14, 2015 at 8:27 AM, Thomas Monjalon <
> thomas.monjalon@6wind.com>
> > > > > > wrote:
> > > > > >
> > > > > > > 2015-01-09 04:41, Ravi Kerur:
> > > > > > > > Thomas,
> > > > > > > >
> > > > > > > > Please let me know how I can move forward on this. If i
> confine changes
> > > > > > > in
> > > > > > > > e1000/ directory to e1000_osdep.h file only and the rest in
> PMD will that
> > > > > > > > work? The reason I ask is because of following comment  in
> README file.
> > > > > > > >
> > > > > > > > ...
> > > > > > > > Few changes to the original FreeBSD sources were made to:
> > > > > > > > - Adopt it for PMD usage mode:
> > > > > > > >         e1000_osdep.c
> > > > > > > >         e1000_osdep.h
> > > > > > > > ...
> > > > >
> > > > > Yes, if needed you can modify these files.
> > > > > In fact, these files are the only 2 that are allowed to be
> modified inside e1000 sub-directory.
> > > > > As I understand you plan to implement E1000_READ_FLASH_REG  and
> E1000_WRITE_FLASH_REG
> > > > > macros properly, correct?
> > > > > Konstantin
> > > > >
> > > >
> > > > As a cleanup we should really look to move these two files out of
> the e1000
> > > > subdirectory (and similarly for the ixgbe versions etc.), so as to
> give a cleaner
> > > > and more manageable separation between what can be edited or not.
> > >
> > > It was always like that for all Intel PMDs we have:
> > >
> > > $ find lib/ -name '*_osdep.*' | grep -v acl
> > > lib/librte_pmd_vmxnet3/vmxnet3/vmxnet3_osdep.h
> > > lib/librte_pmd_ixgbe/ixgbe/ixgbe_osdep.h
> > > lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_osdep.h
> > > lib/librte_eal/linuxapp/kni/ethtool/igb/e1000_osdep.h
> > > lib/librte_pmd_i40e/i40e/i40e_osdep.h
> > > lib/librte_pmd_e1000/e1000/e1000_osdep.c
> > > lib/librte_pmd_e1000/e1000/e1000_osdep.h
> > >
> > > As I understand ND has it's own version of <drvname>_osdep.* for each
> OS they support.
> > > We obviously modify it to fit DPDK purposes.
> > >
> > > Konstantin
> > >
> > > >
> > > > /Bruce
> >
> > Yep. Doesn't mean we haven't put it in the wrong place though! :-)
>
> We just don't move it at all :)
> It is at the same place where ND puts it, we just modify the contents.
> From my point - current location is perfectly ok.
> Konstantin
>
> >
> > /Bruce
>