diff mbox series

[v3,1/2] pinctrl: meson: meson8b: add the eth_rxd2 and eth_rxd3 pins

Message ID 20181229143556.27339-2-martin.blumenstingl@googlemail.com (mailing list archive)
State Superseded
Headers show
Series Meson8b RGMII Ethernet pin fixes | expand

Commit Message

Martin Blumenstingl Dec. 29, 2018, 2:35 p.m. UTC 
Gigabit Ethernet requires the Ethernet TXD0..3 and RXD0..3 data lines.
Add the missing eth_rxd2 and eth_rxd3 definitions so we don't have to
rely on the bootloader to set them up correctly.

The vendor u-boot sources for Odroid-C1 use the following Ethernet
pinmux configuration:
  SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_6, 0x3f4f);
  SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_7, 0xf00000);
This translates to the following pin groups in the mainline kernel:
- register 6 bit  0: eth_rxd1 (DIF_0_P)
- register 6 bit  1: eth_rxd0 (DIF_0_N)
- register 6 bit  2: eth_rx_dv (DIF_1_P)
- register 6 bit  3: eth_rx_clk (DIF_1_N)
- register 6 bit  6: eth_tx_en (DIF_3_P)
- register 6 bit  8: eth_ref_clk (DIF_3_N)
- register 6 bit  9: eth_mdc (DIF_4_P)
- register 6 bit 10: eth_mdio_en (DIF_4_N)
- register 6 bit 11: eth_tx_clk (GPIOH_9)
- register 6 bit 12: eth_txd2 (GPIOH_8)
- register 6 bit 13: eth_txd3 (GPIOH_7)
- register 7 bit 20: eth_txd0_0 (GPIOH_6)
- register 7 bit 21: eth_txd1_0 (GPIOH_5)
- register 7 bit 22: eth_rxd3 (DIF_2_P)
- register 7 bit 23: eth_rxd2 (DIF_2_N)

All functions except eth_rxd2 and eth_rxd3 are already supported by the
pinctrl-meson8b driver.

Suggested-by: Jianxin Pan <jianxin.pan@amlogic.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pinctrl/meson/pinctrl-meson8b.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Kevin Hilman Jan. 11, 2019, 1:08 a.m. UTC | #1 
Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

> Gigabit Ethernet requires the Ethernet TXD0..3 and RXD0..3 data lines.
> Add the missing eth_rxd2 and eth_rxd3 definitions so we don't have to
> rely on the bootloader to set them up correctly.
>
> The vendor u-boot sources for Odroid-C1 use the following Ethernet
> pinmux configuration:
>   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_6, 0x3f4f);
>   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_7, 0xf00000);
> This translates to the following pin groups in the mainline kernel:
> - register 6 bit  0: eth_rxd1 (DIF_0_P)
> - register 6 bit  1: eth_rxd0 (DIF_0_N)
> - register 6 bit  2: eth_rx_dv (DIF_1_P)
> - register 6 bit  3: eth_rx_clk (DIF_1_N)
> - register 6 bit  6: eth_tx_en (DIF_3_P)
> - register 6 bit  8: eth_ref_clk (DIF_3_N)
> - register 6 bit  9: eth_mdc (DIF_4_P)
> - register 6 bit 10: eth_mdio_en (DIF_4_N)
> - register 6 bit 11: eth_tx_clk (GPIOH_9)
> - register 6 bit 12: eth_txd2 (GPIOH_8)
> - register 6 bit 13: eth_txd3 (GPIOH_7)
> - register 7 bit 20: eth_txd0_0 (GPIOH_6)
> - register 7 bit 21: eth_txd1_0 (GPIOH_5)
> - register 7 bit 22: eth_rxd3 (DIF_2_P)
> - register 7 bit 23: eth_rxd2 (DIF_2_N)
>
> All functions except eth_rxd2 and eth_rxd3 are already supported by the
> pinctrl-meson8b driver.
>
> Suggested-by: Jianxin Pan <jianxin.pan@amlogic.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Reviewed-by: Kevin Hilman <khilman@baylibre.com>
Emiliano Ingrassia Jan. 11, 2019, 9:36 a.m. UTC | #2 
On Thu, Jan 10, 2019 at 05:08:39PM -0800, Kevin Hilman wrote:
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>
> > Gigabit Ethernet requires the Ethernet TXD0..3 and RXD0..3 data lines.
> > Add the missing eth_rxd2 and eth_rxd3 definitions so we don't have to
> > rely on the bootloader to set them up correctly.
> >
> > The vendor u-boot sources for Odroid-C1 use the following Ethernet
> > pinmux configuration:
> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_6, 0x3f4f);
> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_7, 0xf00000);
> > This translates to the following pin groups in the mainline kernel:
> > - register 6 bit  0: eth_rxd1 (DIF_0_P)
> > - register 6 bit  1: eth_rxd0 (DIF_0_N)
> > - register 6 bit  2: eth_rx_dv (DIF_1_P)
> > - register 6 bit  3: eth_rx_clk (DIF_1_N)
> > - register 6 bit  6: eth_tx_en (DIF_3_P)
> > - register 6 bit  8: eth_ref_clk (DIF_3_N)
> > - register 6 bit  9: eth_mdc (DIF_4_P)
> > - register 6 bit 10: eth_mdio_en (DIF_4_N)
> > - register 6 bit 11: eth_tx_clk (GPIOH_9)
> > - register 6 bit 12: eth_txd2 (GPIOH_8)
> > - register 6 bit 13: eth_txd3 (GPIOH_7)
> > - register 7 bit 20: eth_txd0_0 (GPIOH_6)
> > - register 7 bit 21: eth_txd1_0 (GPIOH_5)
> > - register 7 bit 22: eth_rxd3 (DIF_2_P)
> > - register 7 bit 23: eth_rxd2 (DIF_2_N)
> >
> > All functions except eth_rxd2 and eth_rxd3 are already supported by the
> > pinctrl-meson8b driver.
> >
> > Suggested-by: Jianxin Pan <jianxin.pan@amlogic.com>
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Tested-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
Reviewed-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
Emiliano Ingrassia Jan. 11, 2019, 10:09 a.m. UTC | #3 
Hi Kevin,

sorry to bother you.

On Thu, Jan 10, 2019 at 05:08:39PM -0800, Kevin Hilman wrote:
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>
> > Gigabit Ethernet requires the Ethernet TXD0..3 and RXD0..3 data lines.
> > Add the missing eth_rxd2 and eth_rxd3 definitions so we don't have to
> > rely on the bootloader to set them up correctly.
> >
> > The vendor u-boot sources for Odroid-C1 use the following Ethernet
> > pinmux configuration:
> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_6, 0x3f4f);
> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_7, 0xf00000);
> > This translates to the following pin groups in the mainline kernel:
> > - register 6 bit  0: eth_rxd1 (DIF_0_P)
> > - register 6 bit  1: eth_rxd0 (DIF_0_N)
> > - register 6 bit  2: eth_rx_dv (DIF_1_P)
> > - register 6 bit  3: eth_rx_clk (DIF_1_N)
> > - register 6 bit  6: eth_tx_en (DIF_3_P)
> > - register 6 bit  8: eth_ref_clk (DIF_3_N)
> > - register 6 bit  9: eth_mdc (DIF_4_P)
> > - register 6 bit 10: eth_mdio_en (DIF_4_N)
> > - register 6 bit 11: eth_tx_clk (GPIOH_9)
> > - register 6 bit 12: eth_txd2 (GPIOH_8)
> > - register 6 bit 13: eth_txd3 (GPIOH_7)
> > - register 7 bit 20: eth_txd0_0 (GPIOH_6)
> > - register 7 bit 21: eth_txd1_0 (GPIOH_5)
> > - register 7 bit 22: eth_rxd3 (DIF_2_P)
> > - register 7 bit 23: eth_rxd2 (DIF_2_N)
> >
> > All functions except eth_rxd2 and eth_rxd3 are already supported by the
> > pinctrl-meson8b driver.
> >
> > Suggested-by: Jianxin Pan <jianxin.pan@amlogic.com>
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>

For both patches of this series I gave:
Tested-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
Reviewed-by: Emiliano Ingrassia <ingrassia@epigenesys.com>

They were in the answer to the cover letter of this series.
I note that they were not included in the commit message of the first patch.

Did I miss something or did something wrong?
Please, let me know.

Thank you,

Emiliano
Kevin Hilman Jan. 11, 2019, 6:06 p.m. UTC | #4 
Emiliano Ingrassia <ingrassia@epigenesys.com> writes:

> Hi Kevin,
>
> sorry to bother you.

No need to apologize.

> On Thu, Jan 10, 2019 at 05:08:39PM -0800, Kevin Hilman wrote:
>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>
>> > Gigabit Ethernet requires the Ethernet TXD0..3 and RXD0..3 data lines.
>> > Add the missing eth_rxd2 and eth_rxd3 definitions so we don't have to
>> > rely on the bootloader to set them up correctly.
>> >
>> > The vendor u-boot sources for Odroid-C1 use the following Ethernet
>> > pinmux configuration:
>> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_6, 0x3f4f);
>> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_7, 0xf00000);
>> > This translates to the following pin groups in the mainline kernel:
>> > - register 6 bit  0: eth_rxd1 (DIF_0_P)
>> > - register 6 bit  1: eth_rxd0 (DIF_0_N)
>> > - register 6 bit  2: eth_rx_dv (DIF_1_P)
>> > - register 6 bit  3: eth_rx_clk (DIF_1_N)
>> > - register 6 bit  6: eth_tx_en (DIF_3_P)
>> > - register 6 bit  8: eth_ref_clk (DIF_3_N)
>> > - register 6 bit  9: eth_mdc (DIF_4_P)
>> > - register 6 bit 10: eth_mdio_en (DIF_4_N)
>> > - register 6 bit 11: eth_tx_clk (GPIOH_9)
>> > - register 6 bit 12: eth_txd2 (GPIOH_8)
>> > - register 6 bit 13: eth_txd3 (GPIOH_7)
>> > - register 7 bit 20: eth_txd0_0 (GPIOH_6)
>> > - register 7 bit 21: eth_txd1_0 (GPIOH_5)
>> > - register 7 bit 22: eth_rxd3 (DIF_2_P)
>> > - register 7 bit 23: eth_rxd2 (DIF_2_N)
>> >
>> > All functions except eth_rxd2 and eth_rxd3 are already supported by the
>> > pinctrl-meson8b driver.
>> >
>> > Suggested-by: Jianxin Pan <jianxin.pan@amlogic.com>
>> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>
>> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
>
> For both patches of this series I gave:
> Tested-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
> Reviewed-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
>
> They were in the answer to the cover letter of this series.
> I note that they were not included in the commit message of the first patch.
>
> Did I miss something or did something wrong?
> Please, let me know.

You did everything fine, I just missed them.

I've started relying a bit more heavily on patchwork to collect
review/test tags, and right now it doesn't notice tags to the
cover-letter, only to individual patches, so I typically add those by
hand, but in this case I missed them.

Sorry about that, and thanks for the reminder.  I'll add them and
repush.

And, while I'm thinking about it, I'll request that feature to patchwork
developers.

Kevin
Emiliano Ingrassia Jan. 11, 2019, 6:21 p.m. UTC | #5 
Hi Kevin,

On Fri, Jan 11, 2019 at 10:06:54AM -0800, Kevin Hilman wrote:
> Emiliano Ingrassia <ingrassia@epigenesys.com> writes:
>
> > Hi Kevin,
> >
> > sorry to bother you.
>
> No need to apologize.
>
> > On Thu, Jan 10, 2019 at 05:08:39PM -0800, Kevin Hilman wrote:
> >> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
> >>
> >> > Gigabit Ethernet requires the Ethernet TXD0..3 and RXD0..3 data lines.
> >> > Add the missing eth_rxd2 and eth_rxd3 definitions so we don't have to
> >> > rely on the bootloader to set them up correctly.
> >> >
> >> > The vendor u-boot sources for Odroid-C1 use the following Ethernet
> >> > pinmux configuration:
> >> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_6, 0x3f4f);
> >> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_7, 0xf00000);
> >> > This translates to the following pin groups in the mainline kernel:
> >> > - register 6 bit  0: eth_rxd1 (DIF_0_P)
> >> > - register 6 bit  1: eth_rxd0 (DIF_0_N)
> >> > - register 6 bit  2: eth_rx_dv (DIF_1_P)
> >> > - register 6 bit  3: eth_rx_clk (DIF_1_N)
> >> > - register 6 bit  6: eth_tx_en (DIF_3_P)
> >> > - register 6 bit  8: eth_ref_clk (DIF_3_N)
> >> > - register 6 bit  9: eth_mdc (DIF_4_P)
> >> > - register 6 bit 10: eth_mdio_en (DIF_4_N)
> >> > - register 6 bit 11: eth_tx_clk (GPIOH_9)
> >> > - register 6 bit 12: eth_txd2 (GPIOH_8)
> >> > - register 6 bit 13: eth_txd3 (GPIOH_7)
> >> > - register 7 bit 20: eth_txd0_0 (GPIOH_6)
> >> > - register 7 bit 21: eth_txd1_0 (GPIOH_5)
> >> > - register 7 bit 22: eth_rxd3 (DIF_2_P)
> >> > - register 7 bit 23: eth_rxd2 (DIF_2_N)
> >> >
> >> > All functions except eth_rxd2 and eth_rxd3 are already supported by the
> >> > pinctrl-meson8b driver.
> >> >
> >> > Suggested-by: Jianxin Pan <jianxin.pan@amlogic.com>
> >> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >>
> >> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> >
> > For both patches of this series I gave:
> > Tested-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
> > Reviewed-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
> >
> > They were in the answer to the cover letter of this series.
> > I note that they were not included in the commit message of the first patch.
> >
> > Did I miss something or did something wrong?
> > Please, let me know.
>
> You did everything fine, I just missed them.
>
> I've started relying a bit more heavily on patchwork to collect
> review/test tags, and right now it doesn't notice tags to the
> cover-letter, only to individual patches, so I typically add those by
> hand, but in this case I missed them.
>

Good to know, I'll keep it in mind next time.

> Sorry about that, and thanks for the reminder.  I'll add them and
> repush.
>
> And, while I'm thinking about it, I'll request that feature to patchwork
> developers.
>
> Kevin
>
>

Thank you for the explanation!

Best regards,

Emiliano
Martin Blumenstingl Jan. 11, 2019, 7:52 p.m. UTC | #6 
Hi Kevin,

On Fri, Jan 11, 2019 at 2:08 AM Kevin Hilman <khilman@baylibre.com> wrote:
>
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>
> > Gigabit Ethernet requires the Ethernet TXD0..3 and RXD0..3 data lines.
> > Add the missing eth_rxd2 and eth_rxd3 definitions so we don't have to
> > rely on the bootloader to set them up correctly.
> >
> > The vendor u-boot sources for Odroid-C1 use the following Ethernet
> > pinmux configuration:
> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_6, 0x3f4f);
> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_7, 0xf00000);
> > This translates to the following pin groups in the mainline kernel:
> > - register 6 bit  0: eth_rxd1 (DIF_0_P)
> > - register 6 bit  1: eth_rxd0 (DIF_0_N)
> > - register 6 bit  2: eth_rx_dv (DIF_1_P)
> > - register 6 bit  3: eth_rx_clk (DIF_1_N)
> > - register 6 bit  6: eth_tx_en (DIF_3_P)
> > - register 6 bit  8: eth_ref_clk (DIF_3_N)
> > - register 6 bit  9: eth_mdc (DIF_4_P)
> > - register 6 bit 10: eth_mdio_en (DIF_4_N)
> > - register 6 bit 11: eth_tx_clk (GPIOH_9)
> > - register 6 bit 12: eth_txd2 (GPIOH_8)
> > - register 6 bit 13: eth_txd3 (GPIOH_7)
> > - register 7 bit 20: eth_txd0_0 (GPIOH_6)
> > - register 7 bit 21: eth_txd1_0 (GPIOH_5)
> > - register 7 bit 22: eth_rxd3 (DIF_2_P)
> > - register 7 bit 23: eth_rxd2 (DIF_2_N)
> >
> > All functions except eth_rxd2 and eth_rxd3 are already supported by the
> > pinctrl-meson8b driver.
> >
> > Suggested-by: Jianxin Pan <jianxin.pan@amlogic.com>
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
thank you for reviewing this!

I just realized that I forgot to CC the linux-gpio mailing list as
well as Linus Walleij on this patch (fixing that with this mail) -
sorry for that.
right now this patch is applied to linux-amlogic.git's v5.1/dt branch.
please let me know whether you would like to keep it in your tree or
if you would like me to re-send it (including Linus and the linux-gpio
list this time...)


Regards
Martin
Kevin Hilman Jan. 11, 2019, 10:42 p.m. UTC | #7 
Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

> Hi Kevin,
>
> On Fri, Jan 11, 2019 at 2:08 AM Kevin Hilman <khilman@baylibre.com> wrote:
>>
>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>
>> > Gigabit Ethernet requires the Ethernet TXD0..3 and RXD0..3 data lines.
>> > Add the missing eth_rxd2 and eth_rxd3 definitions so we don't have to
>> > rely on the bootloader to set them up correctly.
>> >
>> > The vendor u-boot sources for Odroid-C1 use the following Ethernet
>> > pinmux configuration:
>> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_6, 0x3f4f);
>> >   SET_CBUS_REG_MASK(PERIPHS_PIN_MUX_7, 0xf00000);
>> > This translates to the following pin groups in the mainline kernel:
>> > - register 6 bit  0: eth_rxd1 (DIF_0_P)
>> > - register 6 bit  1: eth_rxd0 (DIF_0_N)
>> > - register 6 bit  2: eth_rx_dv (DIF_1_P)
>> > - register 6 bit  3: eth_rx_clk (DIF_1_N)
>> > - register 6 bit  6: eth_tx_en (DIF_3_P)
>> > - register 6 bit  8: eth_ref_clk (DIF_3_N)
>> > - register 6 bit  9: eth_mdc (DIF_4_P)
>> > - register 6 bit 10: eth_mdio_en (DIF_4_N)
>> > - register 6 bit 11: eth_tx_clk (GPIOH_9)
>> > - register 6 bit 12: eth_txd2 (GPIOH_8)
>> > - register 6 bit 13: eth_txd3 (GPIOH_7)
>> > - register 7 bit 20: eth_txd0_0 (GPIOH_6)
>> > - register 7 bit 21: eth_txd1_0 (GPIOH_5)
>> > - register 7 bit 22: eth_rxd3 (DIF_2_P)
>> > - register 7 bit 23: eth_rxd2 (DIF_2_N)
>> >
>> > All functions except eth_rxd2 and eth_rxd3 are already supported by the
>> > pinctrl-meson8b driver.
>> >
>> > Suggested-by: Jianxin Pan <jianxin.pan@amlogic.com>
>> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>
>> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> thank you for reviewing this!
>
> I just realized that I forgot to CC the linux-gpio mailing list as
> well as Linus Walleij on this patch (fixing that with this mail) -
> sorry for that.
> right now this patch is applied to linux-amlogic.git's v5.1/dt branch.

That was a mistake I rectified earlier today. I meant to only apply the
DT patch not the driver also.

> please let me know whether you would like to keep it in your tree or
> if you would like me to re-send it (including Linus and the linux-gpio
> list this time...)

Please resend including Linus, linux-gpio and my review tag.

Thanks,

Kevin
diff mbox series

Patch

diff --git a/drivers/pinctrl/meson/pinctrl-meson8b.c b/drivers/pinctrl/meson/pinctrl-meson8b.c
index c69ca95b1ad5..84938bd73ac2 100644
--- a/drivers/pinctrl/meson/pinctrl-meson8b.c
+++ b/drivers/pinctrl/meson/pinctrl-meson8b.c
@@ -346,6 +346,8 @@  static const unsigned int eth_rx_dv_pins[]	= { DIF_1_P };
 static const unsigned int eth_rx_clk_pins[]	= { DIF_1_N };
 static const unsigned int eth_txd0_1_pins[]	= { DIF_2_P };
 static const unsigned int eth_txd1_1_pins[]	= { DIF_2_N };
+static const unsigned int eth_rxd3_pins[]	= { DIF_2_P };
+static const unsigned int eth_rxd2_pins[]	= { DIF_2_N };
 static const unsigned int eth_tx_en_pins[]	= { DIF_3_P };
 static const unsigned int eth_ref_clk_pins[]	= { DIF_3_N };
 static const unsigned int eth_mdc_pins[]	= { DIF_4_P };
@@ -599,6 +601,8 @@  static struct meson_pmx_group meson8b_cbus_groups[] = {
 	GROUP(eth_ref_clk,	6,	8),
 	GROUP(eth_mdc,		6,	9),
 	GROUP(eth_mdio_en,	6,	10),
+	GROUP(eth_rxd3,		7,	22),
+	GROUP(eth_rxd2,		7,	23),
 };
 
 static struct meson_pmx_group meson8b_aobus_groups[] = {
@@ -748,7 +752,7 @@  static const char * const ethernet_groups[] = {
 	"eth_tx_clk", "eth_tx_en", "eth_txd1_0", "eth_txd1_1",
 	"eth_txd0_0", "eth_txd0_1", "eth_rx_clk", "eth_rx_dv",
 	"eth_rxd1", "eth_rxd0", "eth_mdio_en", "eth_mdc", "eth_ref_clk",
-	"eth_txd2", "eth_txd3"
+	"eth_txd2", "eth_txd3", "eth_rxd3", "eth_rxd2"
 };
 
 static const char * const i2c_a_groups[] = {