On Fri, Jan 12, 2018 at 5:05 PM, Karthikeyan Ramasubramanian
[off-list ref] wrote:

This driver manages the Generic Interface (GENI) firmware based Qualcomm
Universal Peripheral (QUP) Wrapper. GENI based QUP is the next generation
programmable module composed of multiple Serial Engines (SE) and supports
a wide range of serial interfaces like UART, SPI, I2C, I3C, etc. This
driver also enables managing the serial interface independent aspects of
Serial Engines.

Signed-off-by: Karthikeyan Ramasubramanian <redacted>
Signed-off-by: Sagar Dharia <redacted>
Signed-off-by: Girish Mahadevan <redacted>
---
 drivers/soc/qcom/Kconfig        |    8 +
 drivers/soc/qcom/Makefile       |    1 +
 drivers/soc/qcom/qcom-geni-se.c | 1016 +++++++++++++++++++++++++++++++++++++++
 include/linux/qcom-geni-se.h    |  807 +++++++++++++++++++++++++++++++
 4 files changed, 1832 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom-geni-se.c
 create mode 100644 include/linux/qcom-geni-se.h

I'm a newbie here, just starting to get up to speed. Please forgive
any indiscretions. But I am interested in this driver, so I'm throwing
in my minor comments.

+/**
+ * geni_se_setup_m_cmd() - Setup the primary sequencer
+ * @base:      Base address of the serial engine's register block.
+ * @cmd:       Command/Operation to setup in the primary sequencer.
+ * @params:    Parameter for the sequencer command.
+ *
+ * This function is used to configure the primary sequencer with the
+ * command and its assoicated parameters.
+ */
+void geni_se_setup_m_cmd(void __iomem *base, u32 cmd, u32 params);
+
+/**
+ * geni_se_setup_s_cmd() - Setup the secondary sequencer
+ * @base:      Base address of the serial engine's register block.
+ * @cmd:       Command/Operation to setup in the secondary sequencer.
+ * @params:    Parameter for the sequencer command.
+ *
+ * This function is used to configure the secondary sequencer with the
+ * command and its assoicated parameters.
+ */
+void geni_se_setup_s_cmd(void __iomem *base, u32 cmd, u32 params);
+

s/assoicated/associated/ (twice)

+/**
+ * geni_se_tx_dma_prep() - Prepare the Serial Engine for TX DMA transfer
+ * @wrapper_dev:       QUP Wrapper Device to which the TX buffer is mapped.
+ * @base:              Base address of the SE register block.
+ * @tx_buf:            Pointer to the TX buffer.
+ * @tx_len:            Length of the TX buffer.
+ * @tx_dma:            Pointer to store the mapped DMA address.
+ *
+ * This function is used to prepare the buffers for DMA TX.
+ *
+ * Return:     0 on success, standard Linux error codes on error/failure.
+ */
+int geni_se_tx_dma_prep(struct device *wrapper_dev, void __iomem *base,
+                       void *tx_buf, int tx_len, dma_addr_t *tx_dma)
+{
+       int ret;
+
+       if (unlikely(!wrapper_dev || !base || !tx_buf || !tx_len || !tx_dma))
+               return -EINVAL;
+
+       ret = geni_se_map_buf(wrapper_dev, tx_dma, tx_buf, tx_len,
+                                   DMA_TO_DEVICE);
+       if (ret)
+               return ret;
+
+       geni_write_reg(7, base, SE_DMA_TX_IRQ_EN_SET);
+       geni_write_reg((u32)(*tx_dma), base, SE_DMA_TX_PTR_L);
+       geni_write_reg((u32)((*tx_dma) >> 32), base, SE_DMA_TX_PTR_H);
+       geni_write_reg(1, base, SE_DMA_TX_ATTR);

Bjorn touched on this, but as someone trying to understand what's
going on it would be great to see #defines for the magic values in
here (7 and 1)

+void geni_se_get_packing_config(int bpw, int pack_words, bool msb_to_lsb,
+                               unsigned long *cfg0, unsigned long *cfg1)
+{
+       u32 cfg[4] = {0};
+       int len;
+       int temp_bpw = bpw;
+       int idx_start = (msb_to_lsb ? (bpw - 1) : 0);
+       int idx = idx_start;
+       int idx_delta = (msb_to_lsb ? -BITS_PER_BYTE : BITS_PER_BYTE);
+       int ceil_bpw = ((bpw & (BITS_PER_BYTE - 1)) ?
+                       ((bpw & ~(BITS_PER_BYTE - 1)) + BITS_PER_BYTE) : bpw);
+       int iter = (ceil_bpw * pack_words) >> 3;

Is the shift by 3 here really meant to divide by BITS_PER_BYTE? It
might be clearer to divide by BITS_PER_BYTE and let the compiler
convert that into a shift.

+       int i;
+
+       if (unlikely(iter <= 0 || iter > 4)) {
+               *cfg0 = 0;
+               *cfg1 = 0;
+               return;
+       }
+
+       for (i = 0; i < iter; i++) {
+               len = (temp_bpw < BITS_PER_BYTE) ?
+                               (temp_bpw - 1) : BITS_PER_BYTE - 1;
+               cfg[i] = ((idx << 5) | (msb_to_lsb << 4) | (len << 1));

...more magic numbers here. These are register bitfields? It would be
great to get the field shifts defined.

+               idx = ((temp_bpw - BITS_PER_BYTE) <= 0) ?
+                               ((i + 1) * BITS_PER_BYTE) + idx_start :
+                               idx + idx_delta;
+               temp_bpw = ((temp_bpw - BITS_PER_BYTE) <= 0) ?
+                               bpw : (temp_bpw - BITS_PER_BYTE);
+       }
+       cfg[iter - 1] |= 1;
+       *cfg0 = cfg[0] | (cfg[1] << 10);
+       *cfg1 = cfg[2] | (cfg[3] << 10);
+}

...more magic shifts here.

+void geni_se_config_packing(void __iomem *base, int bpw,
+                           int pack_words, bool msb_to_lsb)
+{
+       unsigned long cfg0, cfg1;
+
+       geni_se_get_packing_config(bpw, pack_words, msb_to_lsb, &cfg0, &cfg1);
+       geni_write_reg(cfg0, base, SE_GENI_TX_PACKING_CFG0);
+       geni_write_reg(cfg1, base, SE_GENI_TX_PACKING_CFG1);
+       geni_write_reg(cfg0, base, SE_GENI_RX_PACKING_CFG0);
+       geni_write_reg(cfg1, base, SE_GENI_RX_PACKING_CFG1);
+       if (pack_words || bpw == 32)
+               geni_write_reg((bpw >> 4), base, SE_GENI_BYTE_GRAN);
+}

...Same here for the bpw >> 4.

Thanks,
Evan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html