[rt2x00-users] [PATCH4/4] rt2x00: Add support for RT5390 chip

RA-Shiang Tu Shiang_Tu at ralinktech.com
Wed Feb 9 23:54:12 AEDT 2011


Hi Gertjan,

Thanks for your comments, originally I also want to introduce rt5390 in a single patch, but gave it up due to worry about the longer patch code for review.
Sure, for the next update patch, I can merge them together.

For the rest of the questions, please see my inline comments.

> -----Original Message-----
> From: Gertjan van Wingerde [mailto:gwingerde at gmail.com]
> Sent: Wednesday, February 09, 2011 6:04 AM
> To: RA-Shiang Tu
> Cc: users at rt2x00.serialmonkey.com; gregkh at suse.de
> Subject: Re: [rt2x00-users] [PATCH4/4] rt2x00: Add support
> for RT5390 chip
>
> On 02/08/11 11:55, RA-Shiang Tu wrote:
> > Add code segments to support RT5390 chip
> >
> > Signed-off-by: Shiang Tu <shiang_tu at ralinktech.com>
>
> I think it would be more appropriate to merge your patches 1,
> 2 and 4 into a single patch, as it seems a bit overkill to
> introduce support for a single chipset over a series of 3 patches.
> It is good for reviewing now, but I think they at least
> should be applied as a single patch.
>
> > ---
> >  drivers/net/wireless/rt2x00/rt2800.h    |    1 +
> >  drivers/net/wireless/rt2x00/rt2800lib.c |  409
> +++++++++++++++++++++++++++----
> >  drivers/net/wireless/rt2x00/rt2800pci.c |   10 +
> >  3 files changed, 377 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/net/wireless/rt2x00/rt2800.h
> > b/drivers/net/wireless/rt2x00/rt2800.h
> > index 9c06e88..7dbff68 100644
> > --- a/drivers/net/wireless/rt2x00/rt2800.h
> > +++ b/drivers/net/wireless/rt2x00/rt2800.h
> > @@ -459,6 +459,7 @@
> >  #define        RF_CSR_CFG                      0x0500
> >  #define RF_CSR_CFG_DATA                        FIELD32(0x000000ff)
> >  #define RF_CSR_CFG_REGNUM              FIELD32(0x00001f00)
> > +#define RF_CSR_CFG_REGNUM_EXT  FIELD32(0x00003f00)
> >  #define RF_CSR_CFG_WRITE               FIELD32(0x00010000)
> >  #define RF_CSR_CFG_BUSY                        FIELD32(0x00020000)
> >
> > diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c
> > b/drivers/net/wireless/rt2x00/rt2800lib.c
> > index 5592180..985629a 100644
> > --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> > +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> > @@ -400,8 +406,15 @@ int rt2800_load_firmware(struct
> rt2x00_dev *rt2x00dev,
> >         if (rt2800_wait_csr_ready(rt2x00dev))
> >                 return -EBUSY;
> >
> > -       if (rt2x00_is_pci(rt2x00dev))
> > +       if (rt2x00_is_pci(rt2x00dev)) {
> > +               if (rt2x00_rt(rt2x00dev, RT5390)) {
> > +                       rt2800_register_read(rt2x00dev,
> AUX_CTRL, &reg);
> > +                       rt2x00_set_field32(&reg,
> AUX_CTRL_FORCE_PCIE_CLK, 1);
> > +                       rt2x00_set_field32(&reg,
> AUX_CTRL_WAKE_PCIE_EN, 1);
> > +                       rt2800_register_write(rt2x00dev,
> AUX_CTRL, reg);
> > +               }
> >                 rt2800_register_write(rt2x00dev, PWR_PIN_CFG,
> > 0x00000002);
> > +       }
> >
> >         /*
> >          * Disable DMA, will be reenabled later when enabling
>
> The naming of the fields suggest they are PCI-Express
> specific fields. Are rt5390 devices always only PCI-Express
> devices, or can they also be PCI devices for which it is not
> appropriate to set these fields?
> If there can also be PCI devices, then we should have an
> appropriate check to ensure it is actually a PCI-Express device.
>

Because there are a lot of fields of this register is not related to PCI-E, if with a PCI-E prefix, it may make some misunderstanding.
And rt5390 shall be always PCI-Express device.

> > @@ -1565,6 +1578,110 @@ static void
> rt2800_config_channel_rf3xxx(struct rt2x00_dev *rt2x00dev,
> >         rt2800_rfcsr_write(rt2x00dev, 7, rfcsr);  }
> >
> > +
> > +static void rt2800_config_channel_rf53xx(struct rt2x00_dev
> *rt2x00dev,
> > +                                        struct
> ieee80211_conf *conf,
> > +                                        struct rf_channel *rf,
> > +                                        struct
> channel_info *info) {
> > +       u8 rfcsr;
> > +
> > +       /* Set N, K, R first*/
> > +       rt2800_rfcsr_write(rt2x00dev, 8, rf->rf1);
> > +       rt2800_rfcsr_write(rt2x00dev, 9, rf->rf3);
> > +       rt2800_rfcsr_read(rt2x00dev, 11, &rfcsr);
> > +       rt2x00_set_field8(&rfcsr, FIELD8(0x03), rf->rf2);
> > +       rt2800_rfcsr_write(rt2x00dev, 11, rfcsr);
> > +
> > +       rt2800_rfcsr_read(rt2x00dev, 49, &rfcsr);
> > +       if (info->default_power1 > 0x27)
> > +               rt2x00_set_field8(&rfcsr, FIELD8(0x3f), 0x27);
> > +       else
> > +               rt2x00_set_field8(&rfcsr, FIELD8(0x3f),
> info->default_power1);
> > +       rt2800_rfcsr_write(rt2x00dev, 49, rfcsr);
> > +
> > +       rt2800_rfcsr_read(rt2x00dev, 1, &rfcsr);
> > +       rt2x00_set_field8(&rfcsr, FIELD8(0xf), 0xf);
> > +       rt2800_rfcsr_write(rt2x00dev, 1, rfcsr);
> > +
> > +       /* TODO: fix for RF_R17 value setting */
> > +       rt2800_rfcsr_read(rt2x00dev, 17, &rfcsr);
> > +       rt2x00_set_field8(&rfcsr, FIELD8(0x7f),
> rt2x00dev->freq_offset);
> > +       if (rfcsr > 0x5f)
> > +               rfcsr = 0x5f;
> > +       rt2800_rfcsr_write(rt2x00dev, 17, rfcsr);
> > +
> > +       if (test_bit(CONFIG_SUPPORT_BT_COEXIST, &rt2x00dev->flags))
> > +       {
> > +               if (rt2x00_rt_rev_gte(rt2x00dev, RT5390, 0x0502)) {
> > +                       char r59_val[] = {0x0e, 0x0e, 0x0e,
> 0x0e, 0x0e,/*1~5*/
> > +                                       0x0b, /* 6 */
> > +                                       0x0a, /* 7 */
> > +                                       0x09, /* 8 */
> > +                                       0x07, 0x07, 0x07,
> 0x07, 0x07, 0x07 /* 9~14 */};
> > +                       char r55_val[] = {0x83, 0x83, 0x83,
> /* 1~3 */
> > +                                       0x73, 0x73, /* 4 ~ 5*/
> > +                                       0x63, /* 6*/
> > +                                       0x53, 0x53, 0x53, /* 7~ 9 */
> > +                                       0x43, 0x43, 0x43,
> 0x43, 0x43/*10~14*/};
> > +                       if (rf->channel <= 14) {
> > +
> rt2800_rfcsr_write(rt2x00dev, 55, r55_val[rf->channel-1]);
> > +
> rt2800_rfcsr_write(rt2x00dev, 59, r59_val[rf->channel-1]);
> > +                       }
> > +               } else {
> > +                       char r59_val[] = {0x8b, 0x8b, 0x8b,
> 0x8b, 0x8b, 0x8b, 0x8b, /* 1 ~ 7 */
> > +                                       0x8a, /* 8 */
> > +                                       0x89, /* 9 */
> > +                                       0x88, 0x88, /* 10 ~ 11 */
> > +                                       0x86, /* 12 */
> > +                                       0x85, /* 13 */
> > +                                       0x84  /* 14 */};
> > +                       if (rf->channel <= 14)
> > +
> rt2800_rfcsr_write(rt2x00dev, 59, r59_val[rf->channel-1]);
> > +               }
> > +       } else {
> > +               if (rt2x00_rt_rev_gte(rt2x00dev, RT5390, 0x0502)) {
> > +                       char r55_val[] = {0x23, 0x23, 0x23,
> 0x23, /* 1~4 */
> > +                                       0x13, 0x13, 0x13, /* 5~6*/
> > +                                       0x03, 0x03, 0x03,
> 0x03, 0x03, 0x03, 0x03, 0x03/*7~14*/};
> > +                       char r59_val[] = {0x07, 0x07, 0x07,
> 0x07, 0x07, 0x07,
> > +                                       0x07, 0x07, 0x07,
> 0x07, /* 1~10 */
> > +                                       0x06, /* 11 */
> > +                                       0x05, /* 12 */
> > +                                       0x04, 0x04 /* 13~14*/};
> > +                       if (rf->channel <= 14) {
> > +
> rt2800_rfcsr_write(rt2x00dev, 55, r55_val[rf->channel-1]);
> > +
> rt2800_rfcsr_write(rt2x00dev, 59, r59_val[rf->channel-1]);
> > +                       }
> > +               }else if (rt2x00_rt(rt2x00dev, RT5390)) {
> > +                       char r59_val[] = {0x8f, 0x8f, 0x8f,
> 0x8f, 0x8f, 0x8f, 0x8f, /* 1~7 */
> > +                                       0x8d, /* 8 */
> > +                                       0x8a, /* 9 */
> > +                                       0x88, 0x88, /* 10~11 */
> > +                                       0x87, 0x87, /* 12~13 */
> > +                                       0x86, /* 14 */};
> > +                       if (rf->channel <= 14)
> > +
> rt2800_rfcsr_write(rt2x00dev, 59, r59_val[rf->channel-1]);
> > +               }
> > +       }
> > +
> > +       // TODO: Check if need to do rx_h20M/tx_h20M
> > +       rt2800_rfcsr_read(rt2x00dev, 30, &rfcsr);
> > +       // tx_h20M and rx_h20M
> > +       rfcsr = ((rfcsr & ~0x06) |  0/*(TxRxh20M << 1) |
> (TxRxh20M << 2)*/);
> > +       rt2800_rfcsr_write(rt2x00dev, 30, rfcsr);
> > +
> > +       rt2800_rfcsr_read(rt2x00dev, 30, &rfcsr);
> > +       rfcsr = ((rfcsr & ~0x18) | 0x10); // tx_h20M and rx_h20M
> > +       rt2800_rfcsr_write(rt2x00dev, 30, rfcsr);
> > +
> > +       rt2800_rfcsr_read(rt2x00dev, 03, &rfcsr);
> > +       rfcsr = ((rfcsr & ~0x80) | 0x80); // tx_h20M and rx_h20M
> > +       rt2800_rfcsr_write(rt2x00dev, 03, rfcsr);
> > +
> > +
> > +}
> > +
> >  static void rt2800_config_channel(struct rt2x00_dev *rt2x00dev,
> >                                   struct ieee80211_conf *conf,
> >                                   struct rf_channel *rf,
>
> There's a lot of magic numbers here. Can we avoid these as
> much as possible and make proper defines for them?
> This actually holds for more hunks down below.
>

Sure, I can try to make it more readable

> >
> > @@ -3177,11 +3489,17 @@ int rt2800_init_eeprom(struct
> rt2x00_dev *rt2x00dev)
> >             !rt2x00_rt(rt2x00dev, RT3071) &&
> >             !rt2x00_rt(rt2x00dev, RT3090) &&
> >             !rt2x00_rt(rt2x00dev, RT3390) &&
> > -           !rt2x00_rt(rt2x00dev, RT3572)) {
> > +           !rt2x00_rt(rt2x00dev, RT3572) &&
> > +           !rt2x00_rt(rt2x00dev, RT5390)) {
> >                 ERROR(rt2x00dev, "Invalid RT chipset detected.\n");
> >                 return -ENODEV;
> >         }
> >
> > +       if (rt2x00_rt(rt2x00dev, RT5390)) {
> > +               rt2x00_eeprom_read(rt2x00dev,
> EEPROM_CHIP_ID, &value);
> > +               rt2x00_set_chip(rt2x00dev, RT5390, value,
> rt2x00_rev(rt2x00dev));
> > +       }
> > +
> >         if (!rt2x00_rf(rt2x00dev, RF2820) &&
> >             !rt2x00_rf(rt2x00dev, RF2850) &&
> >             !rt2x00_rf(rt2x00dev, RF2720) &&
>
> Hmm... Not sure I understand this bit, why are we overriding
> chipset information that we previously read from MAC_CSR0? It
> seems more appropriate to handle this before the RT chipset
> checks, to make sure we don't override already read
> information, and simply read the right stuff from the start.
> (BTW, it seems it is just the RF type information that moved
> inside the EEPROM, so it shouldn't be too difficult to code
> that a little bit up in this function).
>

The rf.type in "EEPROM_NIC_CONF0_RF_TYPE" is no more useful in rt53xx series and later single chips.
Sure, I can revise the code to move it to a proper place.

> > a/drivers/net/wireless/rt2x00/rt2800pci.c
> > b/drivers/net/wireless/rt2x00/rt2800pci.c
> > index 8f4dfc3..93a1785 100644
> > --- a/drivers/net/wireless/rt2x00/rt2800pci.c
> > +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
> > @@ -493,6 +493,13 @@ static int
> rt2800pci_init_registers(struct rt2x00_dev *rt2x00dev)
> >         rt2800_register_write(rt2x00dev, PBF_SYS_CTRL, 0x00000e1f);
> >         rt2800_register_write(rt2x00dev, PBF_SYS_CTRL, 0x00000e00);
> >
> > +       if (rt2x00_rt(rt2x00dev, RT5390)) {
> > +               rt2800_register_read(rt2x00dev, AUX_CTRL, &reg);
> > +               rt2x00_set_field32(&reg,
> AUX_CTRL_FORCE_PCIE_CLK, 1);
> > +               rt2x00_set_field32(&reg, AUX_CTRL_WAKE_PCIE_EN, 1);
> > +               rt2800_register_write(rt2x00dev, AUX_CTRL, reg);
> > +       }
> > +
> >         rt2800_register_write(rt2x00dev, PWR_PIN_CFG, 0x00000003);
> >
> >         rt2800_register_read(rt2x00dev, MAC_SYS_CTRL, &reg);
>
> Same comment as above on the meaning of these fields and
> PCI/PCI-express devices.
> Also, do we really have to set these register values twice
> (once here and once when loading the firmware), or is one of
> the two already enough?
>

Yes, I think so. Because both "rt2x00lib_load_firmware()" and "rt2x00lib_enable_radio()" / "rt2x00lib_disable_radio()" may affect that and we need to make sure the device in correct state.


>
CONFIDENTIALITY STATEMENT : The information, attachments and any rights attaching in this e-mail are confidential and privileged; it is intended only for the individual or entity named as the recipient hereof.Any disclosure, copying, distribution, dissemination or use of the contents of this e-mail by persons other than the intended recipient is STRICTLY PROHIBITED and may violate applicable laws.If you have received this e-mail in error, please delete the original message and notify us by return email or collect call immediately. Thank you.




More information about the users mailing list