| Message ID | 20181219155616.9547-3-ben.whitten@lairdtech.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Get sx1301 to transmit lora packets | expand |
Hi Ben, Am 19.12.18 um 16:56 schrieb Ben Whitten: > As part of initialisation when opening the lora device after loading > the AGC firmware we need to satisfy its startup procedure which involves > a few steps; > > Loading a 16 entry lookup table. > For this I have hard coded the laird ETSI certified table for use on the > RG186-M2 (EU) cards, this will need investigation on how other devices > load calibration data. Isn't calibration performed before this firmware is initialized? I left out reading the values back from firmware previously, but that should get implemented. In the userspace implementation, do you get these from a config file or did you modify the reference code to hardcode them? If these are different calibration values from the ones returned by firmware, then a DT property would be an easy way to get hardware-specific data into the driver. However, same as with your clk config, that makes us dependent on DT, which we shouldn't be for ACPI and USB. If we consider it configuration data rather than an immutable fact, then we would need a netlink command to set them. In any case, we have some other vendors on this list, so hopefully someone can comment. :) > > Selecting the correct channel to transmit on. > Currently always 0 for the reference design. Similarly: DT or netlink depending on whether fixed, and fall back to 0 as default. > > Then ending the AGC init procedure and seeing that it has come up. > > Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com> > --- > drivers/net/lora/sx1301.c | 254 +++++++++++++++++++++++++++++++++++++- > drivers/net/lora/sx1301.h | 16 +++ > 2 files changed, 268 insertions(+), 2 deletions(-) Many thanks for working on this! Some nits inline. > diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c > index e75df93b96d8..0c7b6d0b31af 100644 > --- a/drivers/net/lora/sx1301.c > +++ b/drivers/net/lora/sx1301.c > @@ -24,6 +24,121 @@ > > #include "sx1301.h" > > +static struct sx1301_tx_gain_lut tx_gain_lut[] = { > + { > + .dig_gain = 0, > + .pa_gain = 0, > + .dac_gain = 3, > + .mix_gain = 8, > + .rf_power = -3, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 0, > + .dac_gain = 3, > + .mix_gain = 9, > + .rf_power = 0, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 0, > + .dac_gain = 3, > + .mix_gain = 12, > + .rf_power = 3, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 0, > + .dac_gain = 3, > + .mix_gain = 13, > + .rf_power = 4, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 1, > + .dac_gain = 3, > + .mix_gain = 8, > + .rf_power = 6, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 1, > + .dac_gain = 3, > + .mix_gain = 9, > + .rf_power = 9, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 1, > + .dac_gain = 3, > + .mix_gain = 10, > + .rf_power = 10, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 1, > + .dac_gain = 3, > + .mix_gain = 11, > + .rf_power = 12, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 1, > + .dac_gain = 3, > + .mix_gain = 12, > + .rf_power = 13, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 1, > + .dac_gain = 3, > + .mix_gain = 13, > + .rf_power = 14, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 1, > + .dac_gain = 3, > + .mix_gain = 15, > + .rf_power = 16, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 2, > + .dac_gain = 3, > + .mix_gain = 10, > + .rf_power = 19, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 2, > + .dac_gain = 3, > + .mix_gain = 11, > + .rf_power = 21, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 2, > + .dac_gain = 3, > + .mix_gain = 12, > + .rf_power = 22, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 2, > + .dac_gain = 3, > + .mix_gain = 13, > + .rf_power = 24, > + }, > + { > + .dig_gain = 0, > + .pa_gain = 2, > + .dac_gain = 3, > + .mix_gain = 14, > + .rf_power = 25, > + }, > +}; > + > static const struct regmap_range_cfg sx1301_regmap_ranges[] = { > { > .name = "Pages", > @@ -184,6 +299,34 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, > return 0; > } > > +static int sx1301_agc_transaction(struct sx1301_priv *priv, unsigned int val, > + unsigned int *status) > +{ > + int ret; > + > + ret = regmap_write(priv->regmap, SX1301_CHRS, SX1301_AGC_CMD_WAIT); > + if (ret) { > + dev_err(priv->dev, "AGC transaction start failed\n"); > + return ret; > + } > + usleep_range(1000, 2000); > + > + ret = regmap_write(priv->regmap, SX1301_CHRS, val); > + if (ret) { > + dev_err(priv->dev, "AGC transaction value failed\n"); > + return ret; > + } > + usleep_range(1000, 2000); Looks like CHRS needs some regmap annotation as e.g. volatile? > + > + ret = regmap_read(priv->regmap, SX1301_AGCSTS, status); > + if (ret) { > + dev_err(priv->dev, "AGC status read failed\n"); > + return ret; > + } Ditto for AGCSTS. Otherwise we won't be able to enable caching and field access will always be less performant than the previous code. > + > + return 0; > +} > + > static int sx1301_agc_calibrate(struct sx1301_priv *priv) > { > const struct firmware *fw; > @@ -356,9 +499,53 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv) > return -ENXIO; > } > > - return 0; > + return ret; Accidental change? > } > > +static int sx1301_load_tx_gain_lut(struct sx1301_priv *priv) > +{ > + struct sx1301_tx_gain_lut *lut = priv->tx_gain_lut; > + unsigned int val, status; > + int ret, i; > + > + /* HACK use internal gain table in the short term */ > + lut = tx_gain_lut; > + priv->tx_gain_lut_size = ARRAY_SIZE(tx_gain_lut); > + > + for (i = 0; i < priv->tx_gain_lut_size; i++) { > + val = lut->mix_gain + (lut->dac_gain << 4) + > + (lut->pa_gain << 6); Looks like we're writing to a bitfield? Please use constants for the shifts then (maybe add masks, too?), and I'd rather reverse the order. > + > + ret = sx1301_agc_transaction(priv, val, &status); > + if (ret) { > + dev_err(priv->dev, "AGC LUT load failed\n"); > + return ret; > + } > + if (status != (0x30 + i)) { > + dev_err(priv->dev, > + "AGC firmware LUT init error: 0x%02X\n", val); Continuing from 1/4, please avoid wasting the first like that. Also I think x is more common than X? > + return -ENXIO; > + } > + lut++; > + } > + > + /* Abort the transaction if there are less then 16 entries */ > + if (priv->tx_gain_lut_size < SX1301_TX_GAIN_LUT_MAX) { > + ret = sx1301_agc_transaction(priv, SX1301_AGC_CMD_ABORT, &val); > + if (ret) { > + dev_err(priv->dev, "AGC LUT abort failed\n"); > + return ret; > + } > + if (val != 0x30) { Any insights into the magic number that would allow for a constant? > + dev_err(priv->dev, > + "AGC firmware LUT abort error: 0x%02X\n", val); > + return -ENXIO; > + } > + } > + > + return ret; > +}; > + > static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb, > struct net_device *netdev) > { > @@ -378,6 +565,7 @@ static int sx130x_loradev_open(struct net_device *netdev) > { > struct sx1301_priv *priv = netdev_priv(netdev); > int ret; > + unsigned int val; I'd prefer to switch those two lines, as you and I have done elsewhere. > > netdev_dbg(netdev, "%s", __func__); > > @@ -416,12 +604,74 @@ static int sx130x_loradev_open(struct net_device *netdev) > if (ret) > return ret; > > - /* TODO */ > + /* TODO Load constant adjustments, patches */ > + > + /* TODO Frequency time drift */ > + > + /* TODO Configure lora multi demods, bitfield of active */ > + > + /* TODO Load concenrator multi channel frequencies */ concentrator > + > + /* TODO enale to correlator on enabled frequenies */ enale? frequencies > + > + /* TODO PPMi, and modem enable */ > > ret = sx1301_load_all_firmware(priv); > if (ret) > return ret; > > + usleep_range(1000, 2000); > + > + ret = regmap_read(priv->regmap, SX1301_AGCSTS, &val); > + if (ret) { > + dev_err(priv->dev, "AGC status read failed\n"); > + return ret; > + } > + if (val != 0x10) { Magic number > + dev_err(priv->dev, "AGC firmware init failure: 0x%02X\n", val); > + return -ENXIO; > + } > + > + ret = sx1301_load_tx_gain_lut(priv); > + if (ret) > + return ret; > + > + /* Load Tx freq MSBs > + * Always 3 if f > 768 for SX1257 or f > 384 for SX1255 > + */ That comment style seems rather uncommon. What about SX1258? Mark it as TODO/HACK or use a variable below? > + ret = sx1301_agc_transaction(priv, 3, &val); > + if (ret) { > + dev_err(priv->dev, "AGC Tx MSBs load failed\n"); > + return ret; > + } > + if (val != 0x33) { Magic number > + dev_err(priv->dev, "AGC firmware Tx MSBs error: 0x%02X\n", val); > + return -ENXIO; > + } > + > + /* Load chan_select firmware option */ > + ret = sx1301_agc_transaction(priv, 0, &val); I'm guessing this is the mentioned hardcoded channel? I.e., radio A is selected? Are there any hardware properties involved here (DT) or is that a pure configuration choice (netlink)? > + if (ret) { > + dev_err(priv->dev, "AGC chan select failed\n"); > + return ret; > + } > + if (val != 0x30) { > + dev_err(priv->dev, > + "AGC firmware chan select error: 0x%02X", val); > + return -ENXIO; > + } > + > + /* End AGC firmware init and check status */ > + ret = sx1301_agc_transaction(priv, 0, &val); > + if (ret) { > + dev_err(priv->dev, "AGC radio select failed\n"); > + return ret; > + } > + if (val != 0x40) { > + dev_err(priv->dev, "AGC firmware init error: 0x%02X", val); > + return -ENXIO; > + } Could you move all that new code into an sx130x_agc_init() helper function please? Are those operations all reentrant, or do we need code for _close, too? We should also think about locking a sequence of operations, like I did for sx1276 iirc. > + > ret = open_loradev(netdev); > if (ret) > return ret; > diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h > index dd2b7da94fcc..04c9af64c181 100644 > --- a/drivers/net/lora/sx1301.h > +++ b/drivers/net/lora/sx1301.h > @@ -20,6 +20,11 @@ > #define SX1301_MCU_AGC_FW_VERSION 4 > #define SX1301_MCU_AGC_CAL_FW_VERSION 2 > > +#define SX1301_AGC_CMD_WAIT 16 > +#define SX1301_AGC_CMD_ABORT 17 This would seem a good place for the status codes, too? > + > +#define SX1301_TX_GAIN_LUT_MAX 16 > + > /* Page independent */ > #define SX1301_PAGE 0x00 > #define SX1301_VER 0x01 > @@ -105,6 +110,14 @@ static const struct reg_field sx1301_regmap_fields[] = { > REG_FIELD(SX1301_EMERGENCY_FORCE_HOST_CTRL, 0, 0), > }; > > +struct sx1301_tx_gain_lut { > + unsigned int dig_gain; > + unsigned int pa_gain; > + unsigned int dac_gain; > + unsigned int mix_gain; > + int rf_power; /* dBm measured at board connector */ > +}; > + > struct sx1301_priv { > struct lora_dev_priv lora; > struct device *dev; > @@ -112,6 +125,9 @@ struct sx1301_priv { > struct gpio_desc *rst_gpio; > struct regmap *regmap; > struct regmap_field *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)]; > + > + struct sx1301_tx_gain_lut tx_gain_lut[SX1301_TX_GAIN_LUT_MAX]; > + u8 tx_gain_lut_size; > }; > > int __init sx130x_radio_init(void); Cheers, Andreas
On 29/12/2018 09:58, Andreas Färber wrote: > Hi Ben, > > Am 19.12.18 um 16:56 schrieb Ben Whitten: >> As part of initialisation when opening the lora device after loading >> the AGC firmware we need to satisfy its startup procedure which involves >> a few steps; >> >> Loading a 16 entry lookup table. >> For this I have hard coded the laird ETSI certified table for use on the >> RG186-M2 (EU) cards, this will need investigation on how other devices >> load calibration data. > > Isn't calibration performed before this firmware is initialized? I left > out reading the values back from firmware previously, but that should > get implemented. In the userspace implementation, do you get these from > a config file or did you modify the reference code to hardcode them? The calibration you refer to is the IQ offset calibration, as far as I can tell this is a separate thing from the power table the chip uses. In the user space implementation these values are placed in the config file. > > If these are different calibration values from the ones returned by > firmware, then a DT property would be an easy way to get > hardware-specific data into the driver. However, same as with your clk > config, that makes us dependent on DT, which we shouldn't be for ACPI > and USB. If we consider it configuration data rather than an immutable > fact, then we would need a netlink command to set them. Perhaps we can provide both mechanisms, a DT power table and a mechanism to set via netlink prior to opening the interface. As these power settings are hardware dependent and certified for our card by FCC and ETSI I would prefer to include in DT. > > In any case, we have some other vendors on this list, so hopefully > someone can comment. :) > >> >> Selecting the correct channel to transmit on. >> Currently always 0 for the reference design. > > Similarly: DT or netlink depending on whether fixed, and fall back to 0 > as default. Agreed, so on the DT would it be appropriate to have a handle in the sx1301 node with a link to the radio which can transmit, eg. tx = <&radio0 0>; Allowing for both to be transmitters if that if a hardware choice. > >> >> Then ending the AGC init procedure and seeing that it has come up. >> >> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com> >> --- >> drivers/net/lora/sx1301.c | 254 +++++++++++++++++++++++++++++++++++++- >> drivers/net/lora/sx1301.h | 16 +++ >> 2 files changed, 268 insertions(+), 2 deletions(-) > > Many thanks for working on this! Some nits inline. > >> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c >> index e75df93b96d8..0c7b6d0b31af 100644 >> --- a/drivers/net/lora/sx1301.c >> +++ b/drivers/net/lora/sx1301.c >> @@ -24,6 +24,121 @@ >> >> #include "sx1301.h" >> >> +static struct sx1301_tx_gain_lut tx_gain_lut[] = { >> + { >> + .dig_gain = 0, >> + .pa_gain = 0, >> + .dac_gain = 3, >> + .mix_gain = 8, >> + .rf_power = -3, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 0, >> + .dac_gain = 3, >> + .mix_gain = 9, >> + .rf_power = 0, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 0, >> + .dac_gain = 3, >> + .mix_gain = 12, >> + .rf_power = 3, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 0, >> + .dac_gain = 3, >> + .mix_gain = 13, >> + .rf_power = 4, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 1, >> + .dac_gain = 3, >> + .mix_gain = 8, >> + .rf_power = 6, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 1, >> + .dac_gain = 3, >> + .mix_gain = 9, >> + .rf_power = 9, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 1, >> + .dac_gain = 3, >> + .mix_gain = 10, >> + .rf_power = 10, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 1, >> + .dac_gain = 3, >> + .mix_gain = 11, >> + .rf_power = 12, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 1, >> + .dac_gain = 3, >> + .mix_gain = 12, >> + .rf_power = 13, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 1, >> + .dac_gain = 3, >> + .mix_gain = 13, >> + .rf_power = 14, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 1, >> + .dac_gain = 3, >> + .mix_gain = 15, >> + .rf_power = 16, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 2, >> + .dac_gain = 3, >> + .mix_gain = 10, >> + .rf_power = 19, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 2, >> + .dac_gain = 3, >> + .mix_gain = 11, >> + .rf_power = 21, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 2, >> + .dac_gain = 3, >> + .mix_gain = 12, >> + .rf_power = 22, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 2, >> + .dac_gain = 3, >> + .mix_gain = 13, >> + .rf_power = 24, >> + }, >> + { >> + .dig_gain = 0, >> + .pa_gain = 2, >> + .dac_gain = 3, >> + .mix_gain = 14, >> + .rf_power = 25, >> + }, >> +}; >> + >> static const struct regmap_range_cfg sx1301_regmap_ranges[] = { >> { >> .name = "Pages", >> @@ -184,6 +299,34 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, >> return 0; >> } >> >> +static int sx1301_agc_transaction(struct sx1301_priv *priv, unsigned int val, >> + unsigned int *status) >> +{ >> + int ret; >> + >> + ret = regmap_write(priv->regmap, SX1301_CHRS, SX1301_AGC_CMD_WAIT); >> + if (ret) { >> + dev_err(priv->dev, "AGC transaction start failed\n"); >> + return ret; >> + } >> + usleep_range(1000, 2000); >> + >> + ret = regmap_write(priv->regmap, SX1301_CHRS, val); >> + if (ret) { >> + dev_err(priv->dev, "AGC transaction value failed\n"); >> + return ret; >> + } >> + usleep_range(1000, 2000); > > Looks like CHRS needs some regmap annotation as e.g. volatile? > >> + >> + ret = regmap_read(priv->regmap, SX1301_AGCSTS, status); >> + if (ret) { >> + dev_err(priv->dev, "AGC status read failed\n"); >> + return ret; >> + } > > Ditto for AGCSTS. > Otherwise we won't be able to enable caching and field access will > always be less performant than the previous code. Agreed, will do. > >> + >> + return 0; >> +} >> + >> static int sx1301_agc_calibrate(struct sx1301_priv *priv) >> { >> const struct firmware *fw; >> @@ -356,9 +499,53 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv) >> return -ENXIO; >> } >> >> - return 0; >> + return ret; > > Accidental change? Whoops yes. > >> } >> >> +static int sx1301_load_tx_gain_lut(struct sx1301_priv *priv) >> +{ >> + struct sx1301_tx_gain_lut *lut = priv->tx_gain_lut; >> + unsigned int val, status; >> + int ret, i; >> + >> + /* HACK use internal gain table in the short term */ >> + lut = tx_gain_lut; >> + priv->tx_gain_lut_size = ARRAY_SIZE(tx_gain_lut); >> + >> + for (i = 0; i < priv->tx_gain_lut_size; i++) { >> + val = lut->mix_gain + (lut->dac_gain << 4) + >> + (lut->pa_gain << 6); > > Looks like we're writing to a bitfield? Please use constants for the > shifts then (maybe add masks, too?), and I'd rather reverse the order. Yes its a bit field, just needs to be written at once. Will do. > >> + >> + ret = sx1301_agc_transaction(priv, val, &status); >> + if (ret) { >> + dev_err(priv->dev, "AGC LUT load failed\n"); >> + return ret; >> + } >> + if (status != (0x30 + i)) { >> + dev_err(priv->dev, >> + "AGC firmware LUT init error: 0x%02X\n", val); > > Continuing from 1/4, please avoid wasting the first like that. > Also I think x is more common than X? Sure thing. > >> + return -ENXIO; >> + } >> + lut++; >> + } >> + >> + /* Abort the transaction if there are less then 16 entries */ >> + if (priv->tx_gain_lut_size < SX1301_TX_GAIN_LUT_MAX) { >> + ret = sx1301_agc_transaction(priv, SX1301_AGC_CMD_ABORT, &val); >> + if (ret) { >> + dev_err(priv->dev, "AGC LUT abort failed\n"); >> + return ret; >> + } >> + if (val != 0x30) { > > Any insights into the magic number that would allow for a constant? No insights to the meaning of the bits, may just be a success constant. > >> + dev_err(priv->dev, >> + "AGC firmware LUT abort error: 0x%02X\n", val); >> + return -ENXIO; >> + } >> + } >> + >> + return ret; >> +}; >> + >> static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb, >> struct net_device *netdev) >> { >> @@ -378,6 +565,7 @@ static int sx130x_loradev_open(struct net_device *netdev) >> { >> struct sx1301_priv *priv = netdev_priv(netdev); >> int ret; >> + unsigned int val; > > I'd prefer to switch those two lines, as you and I have done elsewhere. Sure will do. > >> >> netdev_dbg(netdev, "%s", __func__); >> >> @@ -416,12 +604,74 @@ static int sx130x_loradev_open(struct net_device *netdev) >> if (ret) >> return ret; >> >> - /* TODO */ >> + /* TODO Load constant adjustments, patches */ >> + >> + /* TODO Frequency time drift */ >> + >> + /* TODO Configure lora multi demods, bitfield of active */ >> + >> + /* TODO Load concenrator multi channel frequencies */ > > concentrator > >> + >> + /* TODO enale to correlator on enabled frequenies */ > > enale? > frequencies > >> + >> + /* TODO PPMi, and modem enable */ >> >> ret = sx1301_load_all_firmware(priv); >> if (ret) >> return ret; >> >> + usleep_range(1000, 2000); >> + >> + ret = regmap_read(priv->regmap, SX1301_AGCSTS, &val); >> + if (ret) { >> + dev_err(priv->dev, "AGC status read failed\n"); >> + return ret; >> + } >> + if (val != 0x10) { > > Magic number > >> + dev_err(priv->dev, "AGC firmware init failure: 0x%02X\n", val); >> + return -ENXIO; >> + } >> + >> + ret = sx1301_load_tx_gain_lut(priv); >> + if (ret) >> + return ret; >> + >> + /* Load Tx freq MSBs >> + * Always 3 if f > 768 for SX1257 or f > 384 for SX1255 >> + */ > > That comment style seems rather uncommon. An artifact of checkpatch, which wants no blank lines at the start of a comment block. > What about SX1258? > Mark it as TODO/HACK or use a variable below? Variable sounds good populated from a case of tx radio types. Note we may have a problem if we try and use a radio outside of this band as we may have to reinitialize the AGC to get this value in. > >> + ret = sx1301_agc_transaction(priv, 3, &val); >> + if (ret) { >> + dev_err(priv->dev, "AGC Tx MSBs load failed\n"); >> + return ret; >> + } >> + if (val != 0x33) { > > Magic number Looks like a success message | loaded value. I'll add that. > >> + dev_err(priv->dev, "AGC firmware Tx MSBs error: 0x%02X\n", val); >> + return -ENXIO; >> + } >> + >> + /* Load chan_select firmware option */ >> + ret = sx1301_agc_transaction(priv, 0, &val); > > I'm guessing this is the mentioned hardcoded channel? I.e., radio A is > selected? Are there any hardware properties involved here (DT) or is > that a pure configuration choice (netlink)? I believe this is the transmit choice so it is bound by hardware, if there is a board with two radios in the TX chain then it should be select-able which one to transmit. Though I'm not sure how common that use case will be. > >> + if (ret) { >> + dev_err(priv->dev, "AGC chan select failed\n"); >> + return ret; >> + } >> + if (val != 0x30) { >> + dev_err(priv->dev, >> + "AGC firmware chan select error: 0x%02X", val); >> + return -ENXIO; >> + } >> + >> + /* End AGC firmware init and check status */ >> + ret = sx1301_agc_transaction(priv, 0, &val); >> + if (ret) { >> + dev_err(priv->dev, "AGC radio select failed\n"); >> + return ret; >> + } >> + if (val != 0x40) { >> + dev_err(priv->dev, "AGC firmware init error: 0x%02X", val); >> + return -ENXIO; >> + } > > Could you move all that new code into an sx130x_agc_init() helper > function please? Yep will do. > > Are those operations all reentrant, or do we need code for _close, too? I don't know if there is any shutdown procedure, I think on any open it needs to reinitialize. > > We should also think about locking a sequence of operations, like I did > for sx1276 iirc. I see you have just sent up a patch with that. > >> + >> ret = open_loradev(netdev); >> if (ret) >> return ret; >> diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h >> index dd2b7da94fcc..04c9af64c181 100644 >> --- a/drivers/net/lora/sx1301.h >> +++ b/drivers/net/lora/sx1301.h >> @@ -20,6 +20,11 @@ >> #define SX1301_MCU_AGC_FW_VERSION 4 >> #define SX1301_MCU_AGC_CAL_FW_VERSION 2 >> >> +#define SX1301_AGC_CMD_WAIT 16 >> +#define SX1301_AGC_CMD_ABORT 17 > > This would seem a good place for the status codes, too? Yep > >> + >> +#define SX1301_TX_GAIN_LUT_MAX 16 >> + >> /* Page independent */ >> #define SX1301_PAGE 0x00 >> #define SX1301_VER 0x01 >> @@ -105,6 +110,14 @@ static const struct reg_field sx1301_regmap_fields[] = { >> REG_FIELD(SX1301_EMERGENCY_FORCE_HOST_CTRL, 0, 0), >> }; >> >> +struct sx1301_tx_gain_lut { >> + unsigned int dig_gain; >> + unsigned int pa_gain; >> + unsigned int dac_gain; >> + unsigned int mix_gain; >> + int rf_power; /* dBm measured at board connector */ >> +}; >> + >> struct sx1301_priv { >> struct lora_dev_priv lora; >> struct device *dev; >> @@ -112,6 +125,9 @@ struct sx1301_priv { >> struct gpio_desc *rst_gpio; >> struct regmap *regmap; >> struct regmap_field *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)]; >> + >> + struct sx1301_tx_gain_lut tx_gain_lut[SX1301_TX_GAIN_LUT_MAX]; >> + u8 tx_gain_lut_size; >> }; >> >> int __init sx130x_radio_init(void); Thanks! Ben Whitten
diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c index e75df93b96d8..0c7b6d0b31af 100644 --- a/drivers/net/lora/sx1301.c +++ b/drivers/net/lora/sx1301.c @@ -24,6 +24,121 @@ #include "sx1301.h" +static struct sx1301_tx_gain_lut tx_gain_lut[] = { + { + .dig_gain = 0, + .pa_gain = 0, + .dac_gain = 3, + .mix_gain = 8, + .rf_power = -3, + }, + { + .dig_gain = 0, + .pa_gain = 0, + .dac_gain = 3, + .mix_gain = 9, + .rf_power = 0, + }, + { + .dig_gain = 0, + .pa_gain = 0, + .dac_gain = 3, + .mix_gain = 12, + .rf_power = 3, + }, + { + .dig_gain = 0, + .pa_gain = 0, + .dac_gain = 3, + .mix_gain = 13, + .rf_power = 4, + }, + { + .dig_gain = 0, + .pa_gain = 1, + .dac_gain = 3, + .mix_gain = 8, + .rf_power = 6, + }, + { + .dig_gain = 0, + .pa_gain = 1, + .dac_gain = 3, + .mix_gain = 9, + .rf_power = 9, + }, + { + .dig_gain = 0, + .pa_gain = 1, + .dac_gain = 3, + .mix_gain = 10, + .rf_power = 10, + }, + { + .dig_gain = 0, + .pa_gain = 1, + .dac_gain = 3, + .mix_gain = 11, + .rf_power = 12, + }, + { + .dig_gain = 0, + .pa_gain = 1, + .dac_gain = 3, + .mix_gain = 12, + .rf_power = 13, + }, + { + .dig_gain = 0, + .pa_gain = 1, + .dac_gain = 3, + .mix_gain = 13, + .rf_power = 14, + }, + { + .dig_gain = 0, + .pa_gain = 1, + .dac_gain = 3, + .mix_gain = 15, + .rf_power = 16, + }, + { + .dig_gain = 0, + .pa_gain = 2, + .dac_gain = 3, + .mix_gain = 10, + .rf_power = 19, + }, + { + .dig_gain = 0, + .pa_gain = 2, + .dac_gain = 3, + .mix_gain = 11, + .rf_power = 21, + }, + { + .dig_gain = 0, + .pa_gain = 2, + .dac_gain = 3, + .mix_gain = 12, + .rf_power = 22, + }, + { + .dig_gain = 0, + .pa_gain = 2, + .dac_gain = 3, + .mix_gain = 13, + .rf_power = 24, + }, + { + .dig_gain = 0, + .pa_gain = 2, + .dac_gain = 3, + .mix_gain = 14, + .rf_power = 25, + }, +}; + static const struct regmap_range_cfg sx1301_regmap_ranges[] = { { .name = "Pages", @@ -184,6 +299,34 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, return 0; } +static int sx1301_agc_transaction(struct sx1301_priv *priv, unsigned int val, + unsigned int *status) +{ + int ret; + + ret = regmap_write(priv->regmap, SX1301_CHRS, SX1301_AGC_CMD_WAIT); + if (ret) { + dev_err(priv->dev, "AGC transaction start failed\n"); + return ret; + } + usleep_range(1000, 2000); + + ret = regmap_write(priv->regmap, SX1301_CHRS, val); + if (ret) { + dev_err(priv->dev, "AGC transaction value failed\n"); + return ret; + } + usleep_range(1000, 2000); + + ret = regmap_read(priv->regmap, SX1301_AGCSTS, status); + if (ret) { + dev_err(priv->dev, "AGC status read failed\n"); + return ret; + } + + return 0; +} + static int sx1301_agc_calibrate(struct sx1301_priv *priv) { const struct firmware *fw; @@ -356,9 +499,53 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv) return -ENXIO; } - return 0; + return ret; } +static int sx1301_load_tx_gain_lut(struct sx1301_priv *priv) +{ + struct sx1301_tx_gain_lut *lut = priv->tx_gain_lut; + unsigned int val, status; + int ret, i; + + /* HACK use internal gain table in the short term */ + lut = tx_gain_lut; + priv->tx_gain_lut_size = ARRAY_SIZE(tx_gain_lut); + + for (i = 0; i < priv->tx_gain_lut_size; i++) { + val = lut->mix_gain + (lut->dac_gain << 4) + + (lut->pa_gain << 6); + + ret = sx1301_agc_transaction(priv, val, &status); + if (ret) { + dev_err(priv->dev, "AGC LUT load failed\n"); + return ret; + } + if (status != (0x30 + i)) { + dev_err(priv->dev, + "AGC firmware LUT init error: 0x%02X\n", val); + return -ENXIO; + } + lut++; + } + + /* Abort the transaction if there are less then 16 entries */ + if (priv->tx_gain_lut_size < SX1301_TX_GAIN_LUT_MAX) { + ret = sx1301_agc_transaction(priv, SX1301_AGC_CMD_ABORT, &val); + if (ret) { + dev_err(priv->dev, "AGC LUT abort failed\n"); + return ret; + } + if (val != 0x30) { + dev_err(priv->dev, + "AGC firmware LUT abort error: 0x%02X\n", val); + return -ENXIO; + } + } + + return ret; +}; + static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb, struct net_device *netdev) { @@ -378,6 +565,7 @@ static int sx130x_loradev_open(struct net_device *netdev) { struct sx1301_priv *priv = netdev_priv(netdev); int ret; + unsigned int val; netdev_dbg(netdev, "%s", __func__); @@ -416,12 +604,74 @@ static int sx130x_loradev_open(struct net_device *netdev) if (ret) return ret; - /* TODO */ + /* TODO Load constant adjustments, patches */ + + /* TODO Frequency time drift */ + + /* TODO Configure lora multi demods, bitfield of active */ + + /* TODO Load concenrator multi channel frequencies */ + + /* TODO enale to correlator on enabled frequenies */ + + /* TODO PPMi, and modem enable */ ret = sx1301_load_all_firmware(priv); if (ret) return ret; + usleep_range(1000, 2000); + + ret = regmap_read(priv->regmap, SX1301_AGCSTS, &val); + if (ret) { + dev_err(priv->dev, "AGC status read failed\n"); + return ret; + } + if (val != 0x10) { + dev_err(priv->dev, "AGC firmware init failure: 0x%02X\n", val); + return -ENXIO; + } + + ret = sx1301_load_tx_gain_lut(priv); + if (ret) + return ret; + + /* Load Tx freq MSBs + * Always 3 if f > 768 for SX1257 or f > 384 for SX1255 + */ + ret = sx1301_agc_transaction(priv, 3, &val); + if (ret) { + dev_err(priv->dev, "AGC Tx MSBs load failed\n"); + return ret; + } + if (val != 0x33) { + dev_err(priv->dev, "AGC firmware Tx MSBs error: 0x%02X\n", val); + return -ENXIO; + } + + /* Load chan_select firmware option */ + ret = sx1301_agc_transaction(priv, 0, &val); + if (ret) { + dev_err(priv->dev, "AGC chan select failed\n"); + return ret; + } + if (val != 0x30) { + dev_err(priv->dev, + "AGC firmware chan select error: 0x%02X", val); + return -ENXIO; + } + + /* End AGC firmware init and check status */ + ret = sx1301_agc_transaction(priv, 0, &val); + if (ret) { + dev_err(priv->dev, "AGC radio select failed\n"); + return ret; + } + if (val != 0x40) { + dev_err(priv->dev, "AGC firmware init error: 0x%02X", val); + return -ENXIO; + } + ret = open_loradev(netdev); if (ret) return ret; diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h index dd2b7da94fcc..04c9af64c181 100644 --- a/drivers/net/lora/sx1301.h +++ b/drivers/net/lora/sx1301.h @@ -20,6 +20,11 @@ #define SX1301_MCU_AGC_FW_VERSION 4 #define SX1301_MCU_AGC_CAL_FW_VERSION 2 +#define SX1301_AGC_CMD_WAIT 16 +#define SX1301_AGC_CMD_ABORT 17 + +#define SX1301_TX_GAIN_LUT_MAX 16 + /* Page independent */ #define SX1301_PAGE 0x00 #define SX1301_VER 0x01 @@ -105,6 +110,14 @@ static const struct reg_field sx1301_regmap_fields[] = { REG_FIELD(SX1301_EMERGENCY_FORCE_HOST_CTRL, 0, 0), }; +struct sx1301_tx_gain_lut { + unsigned int dig_gain; + unsigned int pa_gain; + unsigned int dac_gain; + unsigned int mix_gain; + int rf_power; /* dBm measured at board connector */ +}; + struct sx1301_priv { struct lora_dev_priv lora; struct device *dev; @@ -112,6 +125,9 @@ struct sx1301_priv { struct gpio_desc *rst_gpio; struct regmap *regmap; struct regmap_field *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)]; + + struct sx1301_tx_gain_lut tx_gain_lut[SX1301_TX_GAIN_LUT_MAX]; + u8 tx_gain_lut_size; }; int __init sx130x_radio_init(void);
As part of initialisation when opening the lora device after loading the AGC firmware we need to satisfy its startup procedure which involves a few steps; Loading a 16 entry lookup table. For this I have hard coded the laird ETSI certified table for use on the RG186-M2 (EU) cards, this will need investigation on how other devices load calibration data. Selecting the correct channel to transmit on. Currently always 0 for the reference design. Then ending the AGC init procedure and seeing that it has come up. Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com> --- drivers/net/lora/sx1301.c | 254 +++++++++++++++++++++++++++++++++++++- drivers/net/lora/sx1301.h | 16 +++ 2 files changed, 268 insertions(+), 2 deletions(-)