Re: [PATCH RFC lora-next 2/4] net: lora: sx1301: add minimal to get AGC working prior to tx work

[Ben Whitten]

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@xxxxxxxxxxxxx>
> > ---
> >   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