On Fri, Jan 12, 2018 at 5:05 PM, Karthikeyan Ramasubramanian
<kramasub@xxxxxxxxxxxxxx> 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 <kramasub@xxxxxxxxxxxxxx>
> Signed-off-by: Sagar Dharia <sdharia@xxxxxxxxxxxxxx>
> Signed-off-by: Girish Mahadevan <girishm@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
