| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <73275872-d130-bdee-7d75-8b3f83ba4cfd@gmail.com>
Date: Fri, 4 Jan 2019 21:42:59 +0900
From: Ben Whitten <ben.whitten@...il.com>
To: Andreas Färber <afaerber@...e.de>
Cc: starnight@...cu.edu.tw, jiri@...nulli.us,
linux-lpwan@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
Ben Whitten <ben.whitten@...rdtech.com>,
"David S. Miller" <davem@...emloft.net>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC lora-next 2/4] net: lora: sx1301: add minimal to get
AGC working prior to tx work
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@...rdtech.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
Powered by blists - more mailing lists