| Message ID | 1515805547-22816-4-git-send-email-kramasub@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian 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@codeaurora.org> > Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > --- > drivers/soc/qcom/Kconfig | 8 + > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/qcom-geni-se.c | 1016 +++++++++++++++++++++++++++++++++++++++ I'm not aware of any non-serial-engine "geni" at Qualcomm, so can we drop the "se" throughout this driver? > 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 > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index b81374b..b306d51 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -3,6 +3,14 @@ > # > menu "Qualcomm SoC drivers" > > +config QCOM_GENI_SE > + tristate "QCOM GENI Serial Engine Driver" Options in drivers/soc/qcom/Kconfig should have "depends on ARCH_QCOM", as the makefile in this directory won't be processed when !ARCH_QCOM. > + help > + This module is used to manage Generic Interface (GENI) firmware based > + Qualcomm Technologies, Inc. Universal Peripheral (QUP) Wrapper. This > + module is also used to manage the common aspects of multiple Serial > + Engines present in the QUP. > + > config QCOM_GLINK_SSR > tristate "Qualcomm Glink SSR driver" > depends on RPMSG > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > index 40c56f6..74d5db8 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -1,4 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o > obj-$(CONFIG_QCOM_GLINK_SSR) += glink_ssr.o > obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o > obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > new file mode 100644 > index 0000000..3f43582 > --- /dev/null > +++ b/drivers/soc/qcom/qcom-geni-se.c > @@ -0,0 +1,1016 @@ > +/* > + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ Please use SPDX style license header, i.e. this file should start with: // SPDX-License-Identifier: GPL-2.0 /* * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. */ > + > +#include <linux/clk.h> > +#include <linux/slab.h> > +#include <linux/dma-mapping.h> > +#include <linux/io.h> > +#include <linux/list.h> This isn't used. > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/pm_runtime.h> Is this used? > +#include <linux/qcom-geni-se.h> > +#include <linux/spinlock.h> Is this used? > + > +#define MAX_CLK_PERF_LEVEL 32 > + > +/** > + * @struct geni_se_device - Data structure to represent the QUP Wrapper Core Make this name indicate that this is the wrapper; e.g. struct geni_wrapper > + * @dev: Device pointer of the QUP wrapper core. > + * @base: Base address of this instance of QUP wrapper core. > + * @m_ahb_clk: Handle to the primary AHB clock. > + * @s_ahb_clk: Handle to the secondary AHB clock. > + * @geni_dev_lock: Lock to protect the device elements. > + * @num_clk_levels: Number of valid clock levels in clk_perf_tbl. > + * @clk_perf_tbl: Table of clock frequency input to Serial Engine clock. > + */ > +struct geni_se_device { > + struct device *dev; > + void __iomem *base; > + struct clk *m_ahb_clk; > + struct clk *s_ahb_clk; As these two clocks always seems to be operated in tandem use clk_bulk_data to keep track of them and use the clk_bulk*() operations to simplify your prepare/unprepare sites. > + struct mutex geni_dev_lock; There's only one lock and it should be obvious from context that it's the lock of the geni_se_device, so naming this "lock" should be sufficient. > + unsigned int num_clk_levels; > + unsigned long *clk_perf_tbl; > +}; > + > +/* Offset of QUP Hardware Version Register */ > +#define QUP_HW_VER (0x4) Drop the parenthesis. It would be nice if this define indicated that this is a register and not a value. E.g. call it QUP_HW_VER_REG. > + > +#define HW_VER_MAJOR_MASK GENMASK(31, 28) > +#define HW_VER_MAJOR_SHFT 28 > +#define HW_VER_MINOR_MASK GENMASK(27, 16) > +#define HW_VER_MINOR_SHFT 16 > +#define HW_VER_STEP_MASK GENMASK(15, 0) > + > +/** > + * geni_read_reg_nolog() - Helper function to read from a GENI register > + * @base: Base address of the serial engine's register block. > + * @offset: Offset within the serial engine's register block. > + * > + * Return: Return the contents of the register. > + */ > +unsigned int geni_read_reg_nolog(void __iomem *base, int offset) Remove this function. > +{ > + return readl_relaxed(base + offset); > +} > +EXPORT_SYMBOL(geni_read_reg_nolog); > + > +/** > + * geni_write_reg_nolog() - Helper function to write into a GENI register > + * @value: Value to be written into the register. > + * @base: Base address of the serial engine's register block. > + * @offset: Offset within the serial engine's register block. > + */ > +void geni_write_reg_nolog(unsigned int value, void __iomem *base, int offset) Ditto > +{ > + return writel_relaxed(value, (base + offset)); > +} > +EXPORT_SYMBOL(geni_write_reg_nolog); > + > +/** > + * geni_read_reg() - Helper function to read from a GENI register > + * @base: Base address of the serial engine's register block. > + * @offset: Offset within the serial engine's register block. > + * > + * Return: Return the contents of the register. Drop the extra spaces after "Return:" in all your kerneldoc. > + */ > +unsigned int geni_read_reg(void __iomem *base, int offset) > +{ > + return readl_relaxed(base + offset); > +} > +EXPORT_SYMBOL(geni_read_reg); > + > +/** > + * geni_write_reg() - Helper function to write into a GENI register > + * @value: Value to be written into the register. > + * @base: Base address of the serial engine's register block. > + * @offset: Offset within the serial engine's register block. > + */ > +void geni_write_reg(unsigned int value, void __iomem *base, int offset) > +{ > + return writel_relaxed(value, (base + offset)); As written, this function adds no value and I find it quite confusing that this is used both for read/writes on the wrapper as well as the individual functions. Compare: geni_write_reg(common_geni_m_irq_en, base, SE_GENI_M_IRQ_EN); with just inlining: writel(common_geni_m_irq_en, base + SE_GENI_M_IRQ_EN); > +} > +EXPORT_SYMBOL(geni_write_reg); > + > +/** > + * geni_get_qup_hw_version() - Read the QUP wrapper Hardware version > + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. > + * @major: Buffer for Major Version field. > + * @minor: Buffer for Minor Version field. > + * @step: Buffer for Step Version field. > + * > + * Return: 0 on success, standard Linux error codes on failure/error. > + */ > +int geni_get_qup_hw_version(struct device *wrapper_dev, unsigned int *major, > + unsigned int *minor, unsigned int *step) > +{ > + unsigned int version; > + struct geni_se_device *geni_se_dev; > + > + if (!wrapper_dev || !major || !minor || !step) > + return -EINVAL; This would only happen during development, as the engineer calls the function incorrectly. Consider it a contract that these has to be valid and skip the checks. > + > + geni_se_dev = dev_get_drvdata(wrapper_dev); > + if (unlikely(!geni_se_dev)) > + return -ENODEV; Make the children acquire the geni_se_dev handle (keep the type opaque) and pass that instead of wrapper_dev. Then you can just drop this chunk. > + > + version = geni_read_reg(geni_se_dev->base, QUP_HW_VER); > + *major = (version & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT; > + *minor = (version & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT; > + *step = version & HW_VER_STEP_MASK; > + return 0; If you implement these two changes there's no way this function can fail, so you don't have to return a value here. > +} > +EXPORT_SYMBOL(geni_get_qup_hw_version); > + > +/** > + * geni_se_get_proto() - Read the protocol configured for a serial engine > + * @base: Base address of the serial engine's register block. > + * > + * Return: Protocol value as configured in the serial engine. > + */ > +int geni_se_get_proto(void __iomem *base) I keep reading this as "geni set get proto", you should be fine just dropping _se_ from these function names. And if you want to denote that it's the Qualcomm GENI then make it qcom_geni_*. But more importantly, it is not obvious when reading this driver that the typeless "base" being passed is that of the individual functional block, and not the wrapper. If you want to do this in an "object oriented" fashion create a struct that's common for all geni instances, include it in the context of the individual function devices and pass it into these functions. > +{ > + int proto; > + > + proto = ((geni_read_reg(base, GENI_FW_REVISION_RO) > + & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT); Don't do everything in one go, as you can't fit it on one line anyways. Writing it like this instead makes it easier to read: u32 val; val = readl(base + GENI_FW_S_REVISION_RO); return (val & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT; > + return proto; > +} > +EXPORT_SYMBOL(geni_se_get_proto); > + > +static int geni_se_irq_en(void __iomem *base) > +{ > + unsigned int common_geni_m_irq_en; > + unsigned int common_geni_s_irq_en; The size of these values isn't unsigned int, it's u32. And if you give them a shorter name the function becomes easier to read. Further more, as you don't care about ordering you don't need two of them either. So you should be able to just do: u32 val; val = readl(base + SE_GENI_M_IRQ_EN); val |= M_COMMON_GENI_M_IRQ_EN; writel(val, base + SE_GENI_M_IRQ_EN); val = readl(base + SE_GENI_S_IRQ_EN); val |= S_COMMON_GENI_S_IRQ_EN; writel(val, base + SE_GENI_S_IRQ_EN); > + > + common_geni_m_irq_en = geni_read_reg(base, SE_GENI_M_IRQ_EN); > + common_geni_s_irq_en = geni_read_reg(base, SE_GENI_S_IRQ_EN); > + /* Common to all modes */ > + common_geni_m_irq_en |= M_COMMON_GENI_M_IRQ_EN; > + common_geni_s_irq_en |= S_COMMON_GENI_S_IRQ_EN; > + > + geni_write_reg(common_geni_m_irq_en, base, SE_GENI_M_IRQ_EN); > + geni_write_reg(common_geni_s_irq_en, base, SE_GENI_S_IRQ_EN); > + return 0; And as this can't fail there's no reason to have a return value. > +} > + > + > +static void geni_se_set_rx_rfr_wm(void __iomem *base, unsigned int rx_wm, > + unsigned int rx_rfr) > +{ > + geni_write_reg(rx_wm, base, SE_GENI_RX_WATERMARK_REG); > + geni_write_reg(rx_rfr, base, SE_GENI_RX_RFR_WATERMARK_REG); > +} > + > +static int geni_se_io_set_mode(void __iomem *base) > +{ > + unsigned int io_mode; > + unsigned int geni_dma_mode; > + > + io_mode = geni_read_reg(base, SE_IRQ_EN); > + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN); > + > + io_mode |= (GENI_M_IRQ_EN | GENI_S_IRQ_EN); > + io_mode |= (DMA_TX_IRQ_EN | DMA_RX_IRQ_EN); > + geni_dma_mode &= ~GENI_DMA_MODE_EN; > + > + geni_write_reg(io_mode, base, SE_IRQ_EN); > + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN); > + geni_write_reg(0, base, SE_GSI_EVENT_EN); > + return 0; As this function can't fail, don't return a value. > +} > + > +static void geni_se_io_init(void __iomem *base) > +{ > + unsigned int io_op_ctrl; > + unsigned int geni_cgc_ctrl; > + unsigned int dma_general_cfg; These are all u32, be explicit. > + > + geni_cgc_ctrl = geni_read_reg(base, GENI_CGC_CTRL); > + dma_general_cfg = geni_read_reg(base, SE_DMA_GENERAL_CFG); > + geni_cgc_ctrl |= DEFAULT_CGC_EN; > + dma_general_cfg |= (AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CFG_ON | > + DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON); Drop the parenthesis and there's no harm in making multiple assignments in favour of splitting the line. > + io_op_ctrl = DEFAULT_IO_OUTPUT_CTRL_MSK; > + geni_write_reg(geni_cgc_ctrl, base, GENI_CGC_CTRL); > + geni_write_reg(dma_general_cfg, base, SE_DMA_GENERAL_CFG); Is there a reason why this chunk of code is a mix of 3 independent register updates? > + > + geni_write_reg(io_op_ctrl, base, GENI_OUTPUT_CTRL); > + geni_write_reg(FORCE_DEFAULT, base, GENI_FORCE_DEFAULT_REG); > +} > + > +/** > + * geni_se_init() - Initialize the GENI Serial Engine > + * @base: Base address of the serial engine's register block. > + * @rx_wm: Receive watermark to be configured. > + * @rx_rfr_wm: Ready-for-receive watermark to be configured. What's the unit for these? > + * > + * This function is used to initialize the GENI serial engine, configure > + * receive watermark and ready-for-receive watermarks. > + * > + * Return: 0 on success, standard Linux error codes on failure/error. As neither geni_se_io_set_mode() nor geni_se_irq_en() can fail there's no way geni_se_init() can fail and as such you don't need a return value of this function. > + */ > +int geni_se_init(void __iomem *base, unsigned int rx_wm, unsigned int rx_rfr) > +{ > + int ret; > + > + geni_se_io_init(base); > + ret = geni_se_io_set_mode(base); > + if (ret) > + return ret; > + > + geni_se_set_rx_rfr_wm(base, rx_wm, rx_rfr); > + ret = geni_se_irq_en(base); Just inline these two functions here. > + return ret; > +} > +EXPORT_SYMBOL(geni_se_init); This is an excellent candidate for initializing a common "geni function"-struct shared among the children, i.e. make this: void geni_init_port(struct geni_port *port, struct device *dev, size_t rx_wm, rfr_wm); And have this do the ioremap of the memory of dev and initialize some common "geni_port" struct, then you can pass this to some set of geni_port_*() helper functions. > + > +static int geni_se_select_fifo_mode(void __iomem *base) > +{ > + int proto = geni_se_get_proto(base); > + unsigned int common_geni_m_irq_en; > + unsigned int common_geni_s_irq_en; > + unsigned int geni_dma_mode; These are of type u32. If you use more succinct names the function will be easier to read; what about master, slave and mode? (Or m_en, s_en and mode) > + > + geni_write_reg(0, base, SE_GSI_EVENT_EN); > + geni_write_reg(0xFFFFFFFF, base, SE_GENI_M_IRQ_CLEAR); > + geni_write_reg(0xFFFFFFFF, base, SE_GENI_S_IRQ_CLEAR); > + geni_write_reg(0xFFFFFFFF, base, SE_DMA_TX_IRQ_CLR); > + geni_write_reg(0xFFFFFFFF, base, SE_DMA_RX_IRQ_CLR); > + geni_write_reg(0xFFFFFFFF, base, SE_IRQ_EN); Use lowercase for the hex constants. > + > + common_geni_m_irq_en = geni_read_reg(base, SE_GENI_M_IRQ_EN); > + common_geni_s_irq_en = geni_read_reg(base, SE_GENI_S_IRQ_EN); > + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN); > + if (proto != UART) { > + common_geni_m_irq_en |= > + (M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN | > + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); Drop the parenthesis, split the assignment in multiple to make things not span 3 lines. > + common_geni_s_irq_en |= S_CMD_DONE_EN; > + } > + geni_dma_mode &= ~GENI_DMA_MODE_EN; Please group things that are related. The compiler doesn't care if you stretch the life time of your local variables through your functions, but the brain does. > + > + geni_write_reg(common_geni_m_irq_en, base, SE_GENI_M_IRQ_EN); > + geni_write_reg(common_geni_s_irq_en, base, SE_GENI_S_IRQ_EN); > + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN); > + return 0; This can't fail, and you ignore the returned value in geni_se_select_mode(). > +} > + > +static int geni_se_select_dma_mode(void __iomem *base) > +{ > + unsigned int geni_dma_mode = 0; This is u32, can be shorten to "mode" and it's first use is an assignment - so you don't have to initialize it here. > + > + geni_write_reg(0, base, SE_GSI_EVENT_EN); > + geni_write_reg(0xFFFFFFFF, base, SE_GENI_M_IRQ_CLEAR); > + geni_write_reg(0xFFFFFFFF, base, SE_GENI_S_IRQ_CLEAR); > + geni_write_reg(0xFFFFFFFF, base, SE_DMA_TX_IRQ_CLR); > + geni_write_reg(0xFFFFFFFF, base, SE_DMA_RX_IRQ_CLR); > + geni_write_reg(0xFFFFFFFF, base, SE_IRQ_EN); Please lower case your hex constants. > + > + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN); > + geni_dma_mode |= GENI_DMA_MODE_EN; > + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN); > + return 0; Can't fail. > +} > + > +/** > + * geni_se_select_mode() - Select the serial engine transfer mode > + * @base: Base address of the serial engine's register block. > + * @mode: Transfer mode to be selected. > + * > + * Return: 0 on success, standard Linux error codes on failure. > + */ > +int geni_se_select_mode(void __iomem *base, int mode) > +{ > + int ret = 0; Drop the return value and "ret", if you want to help the developer of child devices you can start off by a WARN_ON(mode != FIFO_MODE && mode != SE_DMA); > + > + switch (mode) { > + case FIFO_MODE: > + geni_se_select_fifo_mode(base); > + break; > + case SE_DMA: > + geni_se_select_dma_mode(base); > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; > +} > +EXPORT_SYMBOL(geni_se_select_mode); > + > +/** > + * 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) > +{ > + u32 m_cmd = (cmd << M_OPCODE_SHFT); > + > + m_cmd |= (params & M_PARAMS_MSK); Rather than initializing the variable during declaration and then amending the value it's cleaner if you do: val = (cmd << M_OPCODE_SHFT) | (params & M_PARAMS_MSK); > + geni_write_reg(m_cmd, base, SE_GENI_M_CMD0); > +} > +EXPORT_SYMBOL(geni_se_setup_m_cmd); > + [..] > +/** > + * geni_se_get_tx_fifo_depth() - Get the TX fifo depth of the serial engine > + * @base: Base address of the serial engine's register block. > + * > + * This function is used to get the depth i.e. number of elements in the > + * TX fifo of the serial engine. > + * > + * Return: TX fifo depth in units of FIFO words. > + */ > +int geni_se_get_tx_fifo_depth(void __iomem *base) > +{ > + int tx_fifo_depth; > + > + tx_fifo_depth = ((geni_read_reg(base, SE_HW_PARAM_0) > + & TX_FIFO_DEPTH_MSK) >> TX_FIFO_DEPTH_SHFT); It easier to read this way: u32 val; val = readl(base, SE_HW_PARAM_0); return (val & TX_FIFO_DEPTH_MSK) >> TX_FIFO_DEPTH_SHFT; > + return tx_fifo_depth; > +} > +EXPORT_SYMBOL(geni_se_get_tx_fifo_depth); > + > +/** > + * geni_se_get_tx_fifo_width() - Get the TX fifo width of the serial engine > + * @base: Base address of the serial engine's register block. > + * > + * This function is used to get the width i.e. word size per element in the > + * TX fifo of the serial engine. > + * > + * Return: TX fifo width in bits > + */ > +int geni_se_get_tx_fifo_width(void __iomem *base) > +{ > + int tx_fifo_width; > + > + tx_fifo_width = ((geni_read_reg(base, SE_HW_PARAM_0) > + & TX_FIFO_WIDTH_MSK) >> TX_FIFO_WIDTH_SHFT); > + return tx_fifo_width; Ditto. > +} > +EXPORT_SYMBOL(geni_se_get_tx_fifo_width); > + > +/** > + * geni_se_get_rx_fifo_depth() - Get the RX fifo depth of the serial engine > + * @base: Base address of the serial engine's register block. > + * > + * This function is used to get the depth i.e. number of elements in the > + * RX fifo of the serial engine. > + * > + * Return: RX fifo depth in units of FIFO words > + */ > +int geni_se_get_rx_fifo_depth(void __iomem *base) > +{ > + int rx_fifo_depth; > + > + rx_fifo_depth = ((geni_read_reg(base, SE_HW_PARAM_1) > + & RX_FIFO_DEPTH_MSK) >> RX_FIFO_DEPTH_SHFT); > + return rx_fifo_depth; Ditto. > +} > +EXPORT_SYMBOL(geni_se_get_rx_fifo_depth); > + > +/** > + * geni_se_get_packing_config() - Get the packing configuration based on input > + * @bpw: Bits of data per transfer word. > + * @pack_words: Number of words per fifo element. > + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa. > + * @cfg0: Output buffer to hold the first half of configuration. > + * @cfg1: Output buffer to hold the second half of configuration. > + * > + * This function is used to calculate the packing configuration based on > + * the input packing requirement and the configuration logic. > + */ > +void geni_se_get_packing_config(int bpw, int pack_words, bool msb_to_lsb, > + unsigned long *cfg0, unsigned long *cfg1) This function is used only from geni_se_config_packing() which writes the new config to the associated registers and from the serial driver that does the same - but here pack_words differ for rx and tx. If you improve geni_se_config_packing() to take rx and tx fifo sizes independently you don't have to expose this function to the client drivers and you don't need to return cfg0 and cfg1 like you do here. > +{ > + 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; > + int i; > + > + if (unlikely(iter <= 0 || iter > 4)) { Don't use unlikely(), this function is not called one per port, there's no need to clutter the code to "optimize" it. > + *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)); > + 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); Each operation in this loop depend on temp_bpw being smaller or larger than BITS_PER_BYTE, please rewrite it with a single if/else based on this. > + } > + cfg[iter - 1] |= 1; The 1 you're writing here looks like an "EOF". It would be nice with a comment describing the format of these 4 registers and the meaning of BIT(0). > + *cfg0 = cfg[0] | (cfg[1] << 10); > + *cfg1 = cfg[2] | (cfg[3] << 10); > +} > +EXPORT_SYMBOL(geni_se_get_packing_config); > + > +/** > + * geni_se_config_packing() - Packing configuration of the serial engine > + * @base: Base address of the serial engine's register block. > + * @bpw: Bits of data per transfer word. > + * @pack_words: Number of words per fifo element. > + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa. > + * > + * This function is used to configure the packing rules for the current > + * transfer. > + */ > +void geni_se_config_packing(void __iomem *base, int bpw, > + int pack_words, bool msb_to_lsb) > +{ > + unsigned long cfg0, cfg1; These are u32. > + > + 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); > +} > +EXPORT_SYMBOL(geni_se_config_packing); > + > +static void geni_se_clks_off(struct geni_se_rsc *rsc) > +{ > + struct geni_se_device *geni_se_dev; > + > + if (unlikely(!rsc || !rsc->wrapper_dev)) Drop the unlikely(). Why would you be passed a rsc with wrapper_dev NULL? > + return; > + > + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); > + if (unlikely(!geni_se_dev)) > + return; > + > + clk_disable_unprepare(rsc->se_clk); > + clk_disable_unprepare(geni_se_dev->s_ahb_clk); > + clk_disable_unprepare(geni_se_dev->m_ahb_clk); > +} > + > +/** > + * geni_se_resources_off() - Turn off resources associated with the serial > + * engine > + * @rsc: Handle to resources associated with the serial engine. > + * > + * Return: 0 on success, standard Linux error codes on failure/error. > + */ > +int geni_se_resources_off(struct geni_se_rsc *rsc) > +{ > + int ret = 0; No need to initialize ret. > + struct geni_se_device *geni_se_dev; > + > + if (unlikely(!rsc || !rsc->wrapper_dev)) > + return -EINVAL; > + > + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); > + if (unlikely(!geni_se_dev)) Only child devices would call this, so it's unlikely that this is a probe defer. Also the returned value is not used. And drop the unlikely() > + return -ENODEV; > + > + ret = pinctrl_select_state(rsc->geni_pinctrl, rsc->geni_gpio_sleep); > + if (ret) > + return ret; > + > + geni_se_clks_off(rsc); > + return 0; > +} > +EXPORT_SYMBOL(geni_se_resources_off); > + > +static int geni_se_clks_on(struct geni_se_rsc *rsc) > +{ > + int ret; > + struct geni_se_device *geni_se_dev; If you name this "geni" instead, or "wrapper" the rest will be cleaner. > + > + if (unlikely(!rsc || !rsc->wrapper_dev)) > + return -EINVAL; > + > + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); > + if (unlikely(!geni_se_dev)) > + return -EPROBE_DEFER; > + > + ret = clk_prepare_enable(geni_se_dev->m_ahb_clk); > + if (ret) > + return ret; > + > + ret = clk_prepare_enable(geni_se_dev->s_ahb_clk); > + if (ret) { > + clk_disable_unprepare(geni_se_dev->m_ahb_clk); > + return ret; > + } These two could be dealt with in a single clk_bulk_prepare_enable(). > + > + ret = clk_prepare_enable(rsc->se_clk); > + if (ret) { > + clk_disable_unprepare(geni_se_dev->s_ahb_clk); > + clk_disable_unprepare(geni_se_dev->m_ahb_clk); > + } > + return ret; > +} > + > +/** > + * geni_se_resources_on() - Turn on resources associated with the serial > + * engine > + * @rsc: Handle to resources associated with the serial engine. > + * > + * Return: 0 on success, standard Linux error codes on failure/error. > + */ > +int geni_se_resources_on(struct geni_se_rsc *rsc) > +{ > + int ret = 0; > + struct geni_se_device *geni_se_dev; > + > + if (unlikely(!rsc || !rsc->wrapper_dev)) > + return -EINVAL; > + > + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); > + if (unlikely(!geni_se_dev)) As with geni_se_resources_off() > + return -EPROBE_DEFER; > + > + ret = geni_se_clks_on(rsc); > + if (ret) > + return ret; > + > + ret = pinctrl_select_state(rsc->geni_pinctrl, rsc->geni_gpio_active); pinctrl_pm_select_default_state(rsc->dev); So you need the dev associated with the rsc. > + if (ret) > + geni_se_clks_off(rsc); > + > + return ret; > +} > +EXPORT_SYMBOL(geni_se_resources_on); > + > +/** > + * geni_se_clk_tbl_get() - Get the clock table to program DFS > + * @rsc: Resource for which the clock table is requested. > + * @tbl: Table in which the output is returned. > + * > + * This function is called by the protocol drivers to determine the different > + * clock frequencies supported by Serail Engine Core Clock. The protocol s/Serail/Serial/ > + * drivers use the output to determine the clock frequency index to be > + * programmed into DFS. > + * > + * Return: number of valid performance levels in the table on success, > + * standard Linux error codes on failure. > + */ > +int geni_se_clk_tbl_get(struct geni_se_rsc *rsc, unsigned long **tbl) > +{ > + struct geni_se_device *geni_se_dev; > + int i; > + unsigned long prev_freq = 0; > + int ret = 0; > + > + if (unlikely(!rsc || !rsc->wrapper_dev || !rsc->se_clk || !tbl)) Drop the "unlikely", you're only calling this once. > + return -EINVAL; > + > + *tbl = NULL; Don't touch tbl on error. > + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); > + if (unlikely(!geni_se_dev)) > + return -EPROBE_DEFER; How would this even be possible? This function should only be called by child devices, which per definition means that this device been probed. > + > + mutex_lock(&geni_se_dev->geni_dev_lock); > + if (geni_se_dev->clk_perf_tbl) { > + *tbl = geni_se_dev->clk_perf_tbl; > + ret = geni_se_dev->num_clk_levels; > + goto exit_se_clk_tbl_get; > + } You're never freeing clk_perf_tbl, and the only reason you have geni_dev_lock is to protect the "cached" array in geni_se_dev. Move the allocation and generation of clk_perf_tbl to geni_se_probe() and store the value, then make this function just return that array. That way you can drop the lock and the code becomes cleaner. > + > + geni_se_dev->clk_perf_tbl = kzalloc(sizeof(*geni_se_dev->clk_perf_tbl) * > + MAX_CLK_PERF_LEVEL, GFP_KERNEL); Use kcalloc() > + if (!geni_se_dev->clk_perf_tbl) { > + ret = -ENOMEM; > + goto exit_se_clk_tbl_get; > + } > + > + for (i = 0; i < MAX_CLK_PERF_LEVEL; i++) { > + geni_se_dev->clk_perf_tbl[i] = clk_round_rate(rsc->se_clk, > + prev_freq + 1); > + if (geni_se_dev->clk_perf_tbl[i] == prev_freq) { > + geni_se_dev->clk_perf_tbl[i] = 0; Use a local variable to keep the return value of clk_round_rate(), that way you don't have to revert the value here (not that it matters...) and you don't need to line wrap the assignment above. > + break; > + } > + prev_freq = geni_se_dev->clk_perf_tbl[i]; ...and then you don't need a separate local variable to keep track of the last value... > + } > + geni_se_dev->num_clk_levels = i; > + *tbl = geni_se_dev->clk_perf_tbl; > + ret = geni_se_dev->num_clk_levels; > +exit_se_clk_tbl_get: Name your labels based on what action they perform, e.g. "out_unlock" (not err_unlock here because it's the successful path as well) > + mutex_unlock(&geni_se_dev->geni_dev_lock); > + return ret; > +} > +EXPORT_SYMBOL(geni_se_clk_tbl_get); > + > +/** > + * geni_se_clk_freq_match() - Get the matching or closest SE clock frequency > + * @rsc: Resource for which the clock frequency is requested. > + * @req_freq: Requested clock frequency. > + * @index: Index of the resultant frequency in the table. > + * @res_freq: Resultant frequency which matches or is closer to the > + * requested frequency. > + * @exact: Flag to indicate exact multiple requirement of the requested > + * frequency . > + * > + * This function is called by the protocol drivers to determine the matching > + * or closest frequency of the Serial Engine clock to be selected in order > + * to meet the performance requirements. What's the benefit compared to calling clk_round_rate()? Given that the function can return a rate that is a fraction of the requested frequency even though "exact" is set, this description isn't explaining the entire picture. > + * > + * Return: 0 on success, standard Linux error codes on failure. Returning the new clockrate would make more sense. > + */ > +int geni_se_clk_freq_match(struct geni_se_rsc *rsc, unsigned long req_freq, > + unsigned int *index, unsigned long *res_freq, > + bool exact) > +{ > + unsigned long *tbl; > + int num_clk_levels; > + int i; > + > + num_clk_levels = geni_se_clk_tbl_get(rsc, &tbl); > + if (num_clk_levels < 0) > + return num_clk_levels; > + > + if (num_clk_levels == 0) > + return -EFAULT; > + > + *res_freq = 0; > + for (i = 0; i < num_clk_levels; i++) { > + if (!(tbl[i] % req_freq)) { > + *index = i; > + *res_freq = tbl[i]; So this will return a frequency that divides the requested frequency without a remainder, will index be used to calculate a multiplier from this? > + return 0; > + } > + > + if (!(*res_freq) || ((tbl[i] > *res_freq) && > + (tbl[i] < req_freq))) { > + *index = i; > + *res_freq = tbl[i]; What if there's a previous value in tbl that given some multiplier has a smaller difference to the requested frequency? > + } > + } > + > + if (exact || !(*res_freq)) *res_freq can't be 0, because in the case that num_clk_levels is 0 you already returned -EFAULT above, in all other cases you will assign it at least once. > + return -ENOKEY; > + > + return 0; Why not use the return value for the calculated frequency? > +} > +EXPORT_SYMBOL(geni_se_clk_freq_match); > + > +/** > + * 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) All child devices will have a "base", so if you move that into what you today call a geni_se_rsc and you pass that to all these functions operating on behalf of the child device you have the first two parameters passed in the same object. A "tx_len" is typically of type size_t. Also note that there are no other buf than tx_buf, no other len than tx_len and no other dma address than tx_dma in this function, so you can drop "tx_" without loosing any information - but improving code cleanliness. > +{ > + int ret; > + > + if (unlikely(!wrapper_dev || !base || !tx_buf || !tx_len || !tx_dma)) > + return -EINVAL; Reduce the error checking here. > + > + ret = geni_se_map_buf(wrapper_dev, tx_dma, tx_buf, tx_len, > + DMA_TO_DEVICE); I think you should pass the wrapper itself to geni_se_tx_dma_prep() and if you name this "wrapper" (instead of wrapper_dev) you're under 80 chars and don't need the line break. > + if (ret) > + return ret; > + > + geni_write_reg(7, base, SE_DMA_TX_IRQ_EN_SET); So that's DMA_RX_IRQ_EN | DMA_TX_IRQ_EN | GENI_M_IRQ_EN? > + 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); If you return the dma_addr_t from this function you will have the happy side effect of being able to use tx_dma directly instead of *tx_dma and you can remove some craziness from these calls. > + geni_write_reg(1, base, SE_DMA_TX_ATTR); This "1" would be nice to have some clarification on. > + geni_write_reg(tx_len, base, SE_DMA_TX_LEN); > + return 0; > +} > +EXPORT_SYMBOL(geni_se_tx_dma_prep); > + > +/** > + * geni_se_rx_dma_prep() - Prepare the Serial Engine for RX DMA transfer > + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped. > + * @base: Base address of the SE register block. > + * @rx_buf: Pointer to the RX buffer. > + * @rx_len: Length of the RX buffer. > + * @rx_dma: Pointer to store the mapped DMA address. > + * > + * This function is used to prepare the buffers for DMA RX. > + * > + * Return: 0 on success, standard Linux error codes on error/failure. The only real error that can come out of this is dma_mapping_error(), so just return the dma_addr_t. > + */ > +int geni_se_rx_dma_prep(struct device *wrapper_dev, void __iomem *base, > + void *rx_buf, int rx_len, dma_addr_t *rx_dma) As with tx, drop all the "rx_". And pass that geni_se_rsc object instead. > +{ > + int ret; > + > + if (unlikely(!wrapper_dev || !base || !rx_buf || !rx_len || !rx_dma)) > + return -EINVAL; > + > + ret = geni_se_map_buf(wrapper_dev, rx_dma, rx_buf, rx_len, > + DMA_FROM_DEVICE); > + if (ret) > + return ret; > + > + geni_write_reg(7, base, SE_DMA_RX_IRQ_EN_SET); We enable same interrupts for rx as tx? Why enable TX interrupt here? > + geni_write_reg((u32)(*rx_dma), base, SE_DMA_RX_PTR_L); > + geni_write_reg((u32)((*rx_dma) >> 32), base, SE_DMA_RX_PTR_H); > + /* RX does not have EOT bit */ Okay? How does this relate to the 0 being written into RX_ATTR? > + geni_write_reg(0, base, SE_DMA_RX_ATTR); > + geni_write_reg(rx_len, base, SE_DMA_RX_LEN); > + return 0; > +} > +EXPORT_SYMBOL(geni_se_rx_dma_prep); > + > +/** > + * geni_se_tx_dma_unprep() - Unprepare the Serial Engine after TX DMA transfer > + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped. > + * @tx_dma: DMA address of the TX buffer. > + * @tx_len: Length of the TX buffer. > + * > + * This function is used to unprepare the DMA buffers after DMA TX. > + */ > +void geni_se_tx_dma_unprep(struct device *wrapper_dev, > + dma_addr_t tx_dma, int tx_len) > +{ > + if (tx_dma) > + geni_se_unmap_buf(wrapper_dev, &tx_dma, tx_len, > + DMA_TO_DEVICE); > +} > +EXPORT_SYMBOL(geni_se_tx_dma_unprep); > + > +/** > + * geni_se_rx_dma_unprep() - Unprepare the Serial Engine after RX DMA transfer > + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped. > + * @rx_dma: DMA address of the RX buffer. > + * @rx_len: Length of the RX buffer. > + * > + * This function is used to unprepare the DMA buffers after DMA RX. > + */ > +void geni_se_rx_dma_unprep(struct device *wrapper_dev, > + dma_addr_t rx_dma, int rx_len) > +{ > + if (rx_dma) > + geni_se_unmap_buf(wrapper_dev, &rx_dma, rx_len, > + DMA_FROM_DEVICE); > +} > +EXPORT_SYMBOL(geni_se_rx_dma_unprep); > + > +/** > + * geni_se_map_buf() - Map a single buffer into QUP wrapper device I find the mixture of prepare and map in the API (where prepare also maps) confusing, but no-one calls genbi_se_map_buf() can it be made internal? > + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. > + * @iova: Pointer in which the mapped virtual address is stored. > + * @buf: Address of the buffer that needs to be mapped. > + * @size: Size of the buffer. > + * @dir: Direction of the DMA transfer. > + * > + * This function is used to map an already allocated buffer into the > + * QUP device space. > + * > + * Return: 0 on success, standard Linux error codes on failure/error. > + */ > +int geni_se_map_buf(struct device *wrapper_dev, dma_addr_t *iova, > + void *buf, size_t size, enum dma_data_direction dir) > +{ > + struct device *dev_p; > + struct geni_se_device *geni_se_dev; > + > + if (!wrapper_dev || !iova || !buf || !size) > + return -EINVAL; No need to check that the programmer of the intermediate function checked that the programmer of the client filled all these out correctly. > + > + geni_se_dev = dev_get_drvdata(wrapper_dev); > + if (!geni_se_dev || !geni_se_dev->dev) > + return -ENODEV; Pass the geni_se_device from the child driver, to remove the need for this. > + > + dev_p = geni_se_dev->dev; Name your variables more succinct and you don't need this local alias. > + > + *iova = dma_map_single(dev_p, buf, size, dir); Just inline dma_map_single() in the functions calling this. > + if (dma_mapping_error(dev_p, *iova)) > + return -EIO; > + return 0; > +} > +EXPORT_SYMBOL(geni_se_map_buf); > + > +/** > + * geni_se_unmap_buf() - Unmap a single buffer from QUP wrapper device > + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. > + * @iova: Pointer in which the mapped virtual address is stored. > + * @size: Size of the buffer. > + * @dir: Direction of the DMA transfer. > + * > + * This function is used to unmap an already mapped buffer from the > + * QUP device space. > + * > + * Return: 0 on success, standard Linux error codes on failure/error. > + */ > +int geni_se_unmap_buf(struct device *wrapper_dev, dma_addr_t *iova, > + size_t size, enum dma_data_direction dir) There's no reason for iova to be a pointer. Just pass the dma_addr_t as is. Should this function really be exposed to the clients? > +{ > + struct device *dev_p; > + struct geni_se_device *geni_se_dev; > + > + if (!wrapper_dev || !iova || !size) > + return -EINVAL; Reduce the error checking. > + > + geni_se_dev = dev_get_drvdata(wrapper_dev); > + if (!geni_se_dev || !geni_se_dev->dev) > + return -ENODEV; And pass the geni_se_rsc to this function for symmetry purposes, which would give you the wrapper by just following the pointer and then the device from there. > + > + dev_p = geni_se_dev->dev; > + dma_unmap_single(dev_p, *iova, size, dir); Just inline dma_unmap_single() in the calling functions. > + return 0; > +} > +EXPORT_SYMBOL(geni_se_unmap_buf); > + > +static const struct of_device_id geni_se_dt_match[] = { > + { .compatible = "qcom,geni-se-qup", }, > + {} > +}; Move this next to the geni_se_driver below and don't forget the MODULE_DEVICE_TABLE() > + > +static int geni_se_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct resource *res; > + struct geni_se_device *geni_se_dev; Just name this "geni". > + int ret = 0; No need to initialize ret, it's only ever used after assignment. > + > + geni_se_dev = devm_kzalloc(dev, sizeof(*geni_se_dev), GFP_KERNEL); > + if (!geni_se_dev) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(dev, "%s: Mandatory resource info not found\n", > + __func__); > + devm_kfree(dev, geni_se_dev); > + return -EINVAL; > + } It's idiomatic to not check for errors of platform_get_resource() as devm_ioremap_resource() will fail gracefully if this happens. > + > + geni_se_dev->base = devm_ioremap_resource(dev, res); > + if (IS_ERR_OR_NULL(geni_se_dev->base)) { This should be IS_ERR() only. > + dev_err(dev, "%s: Error mapping the resource\n", __func__); > + devm_kfree(dev, geni_se_dev); > + return -EFAULT; > + } > + > + geni_se_dev->m_ahb_clk = devm_clk_get(dev, "m-ahb"); > + if (IS_ERR(geni_se_dev->m_ahb_clk)) { > + ret = PTR_ERR(geni_se_dev->m_ahb_clk); > + dev_err(dev, "Err getting M AHB clk %d\n", ret); > + devm_iounmap(dev, geni_se_dev->base); > + devm_kfree(dev, geni_se_dev); The reason we use the devm_ versions of these is so that we don't have to release our resources explicitly. > + return ret; > + } > + > + geni_se_dev->s_ahb_clk = devm_clk_get(dev, "s-ahb"); > + if (IS_ERR(geni_se_dev->s_ahb_clk)) { > + ret = PTR_ERR(geni_se_dev->s_ahb_clk); > + dev_err(dev, "Err getting S AHB clk %d\n", ret); > + devm_clk_put(dev, geni_se_dev->m_ahb_clk); > + devm_iounmap(dev, geni_se_dev->base); > + devm_kfree(dev, geni_se_dev); > + return ret; > + } Use devm_clk_bulk_get(). > + > + geni_se_dev->dev = dev; > + mutex_init(&geni_se_dev->geni_dev_lock); > + dev_set_drvdata(dev, geni_se_dev); > + dev_dbg(dev, "GENI SE Driver probed\n"); > + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); You're not depopulating these devices when the wrapper goes away. Use devm_of_platform_populate() here instead to make that happen. > +} > + > +static int geni_se_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct geni_se_device *geni_se_dev = dev_get_drvdata(dev); > + > + devm_clk_put(dev, geni_se_dev->s_ahb_clk); > + devm_clk_put(dev, geni_se_dev->m_ahb_clk); > + devm_iounmap(dev, geni_se_dev->base); > + devm_kfree(dev, geni_se_dev); Again, the reason to use devm_* is so that you don't have to free things...so if this is what you have here you don't even need a remove function. > + return 0; > +} > + > +static struct platform_driver geni_se_driver = { > + .driver = { > + .name = "geni_se_qup", > + .of_match_table = geni_se_dt_match, > + }, > + .probe = geni_se_probe, > + .remove = geni_se_remove, > +}; > + > +static int __init geni_se_driver_init(void) > +{ > + return platform_driver_register(&geni_se_driver); > +} > +arch_initcall(geni_se_driver_init); > + > +static void __exit geni_se_driver_exit(void) > +{ > + platform_driver_unregister(&geni_se_driver); > +} > +module_exit(geni_se_driver_exit); Should be fine to just use module_platform_driver(), you need separate support for earlycon regardless. > + > +MODULE_DESCRIPTION("GENI Serial Engine Driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h > new file mode 100644 > index 0000000..5695da9 > --- /dev/null > +++ b/include/linux/qcom-geni-se.h > @@ -0,0 +1,807 @@ > +/* > + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. [..] > + */ > + Use SPDX header as above. > +#ifndef _LINUX_QCOM_GENI_SE > +#define _LINUX_QCOM_GENI_SE > +#include <linux/clk.h> > +#include <linux/dma-direction.h> > +#include <linux/io.h> > +#include <linux/list.h> I don't think there's a need for io.h or list.h here. > + > +/* Transfer mode supported by GENI Serial Engines */ > +enum geni_se_xfer_mode { > + INVALID, > + FIFO_MODE, > + SE_DMA, These are quite bad names for things in a header file. > +}; > + > +/* Protocols supported by GENI Serial Engines */ > +enum geni_se_protocol_types { > + NONE, > + SPI, > + UART, > + I2C, > + I3C Ditto > +}; > + > +/** > + * struct geni_se_rsc - GENI Serial Engine Resource > + * @wrapper_dev: Pointer to the parent QUP Wrapper core. > + * @se_clk: Handle to the core serial engine clock. > + * @geni_pinctrl: Handle to the pinctrl configuration. > + * @geni_gpio_active: Handle to the default/active pinctrl state. > + * @geni_gpi_sleep: Handle to the sleep pinctrl state. > + */ > +struct geni_se_rsc { This looks like the common geni_port or geni_device I requested above. > + struct device *wrapper_dev; > + struct clk *se_clk; The one and only clock can be named "clk". > + struct pinctrl *geni_pinctrl; > + struct pinctrl_state *geni_gpio_active; > + struct pinctrl_state *geni_gpio_sleep; The associated struct device already has init, default, idle and sleep pinctrl states associated through the device->pins struct. Typically this means that if you provide a "default" and "sleep" state, the default will be selected when the device probes. In order to select between the states call pinctrl_pm_select_{default,sleep,idle}_state(dev); > +}; > + > +#define PINCTRL_DEFAULT "default" > +#define PINCTRL_SLEEP "sleep" > + > +/* Common SE registers */ The purpose of the helper functions in the wrapper driver is to hide the common details from the individual function drivers, so move these defines into the c-file as they shouldn't be needed in the individual drivers. > +#define GENI_INIT_CFG_REVISION (0x0) Drop the parenthesis. [..] > +#ifdef CONFIG_QCOM_GENI_SE > +/** > + * geni_read_reg_nolog() - Helper function to read from a GENI register > + * @base: Base address of the serial engine's register block. > + * @offset: Offset within the serial engine's register block. > + * > + * Return: Return the contents of the register. > + */ The kerneldoc goes in the implementation, not the header file. And you already have a copy there, so remove it from here. > +unsigned int geni_read_reg_nolog(void __iomem *base, int offset); [..] > +#else I see no point in providing dummy functions for these, just make the individual drivers either depend or select the core helpers. > +static inline unsigned int geni_read_reg_nolog(void __iomem *base, int offset) > +{ > + return 0; > +} Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[].. >> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c >> new file mode 100644 >> index 0000000..3f43582 >> --- /dev/null >> +++ b/drivers/soc/qcom/qcom-geni-se.c >> @@ -0,0 +1,1016 @@ >> +/* >> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ > > Please use SPDX style license header, i.e. this file should start with: > > // SPDX-License-Identifier: GPL-2.0 > /* > * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > */ Looks like Mark Brown commented elsewhere [1] that we should use the C++ commenting style even for the Linux Foundation copyright? [1] https://marc.info/?l=linux-clk&m=151497978626135&w=2
On Thu 18 Jan 01:13 PST 2018, Rajendra Nayak wrote: > [].. > > >> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > >> new file mode 100644 > >> index 0000000..3f43582 > >> --- /dev/null > >> +++ b/drivers/soc/qcom/qcom-geni-se.c > >> @@ -0,0 +1,1016 @@ > >> +/* > >> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > >> + * > >> + * This program is free software; you can redistribute it and/or modify > >> + * it under the terms of the GNU General Public License version 2 and > >> + * only version 2 as published by the Free Software Foundation. > >> + * > >> + * This program is distributed in the hope that it will be useful, > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >> + * GNU General Public License for more details. > >> + * > >> + */ > > > > Please use SPDX style license header, i.e. this file should start with: > > > > // SPDX-License-Identifier: GPL-2.0 > > /* > > * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > > */ > > Looks like Mark Brown commented elsewhere [1] that we should use the C++ > commenting style even for the Linux Foundation copyright? > > [1] https://marc.info/?l=linux-clk&m=151497978626135&w=2 > While I can agree with Mark on the ugliness of the mixed commenting style, this is the style that I found communicated and is what you find in other files. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 18, 2018 at 08:57:45AM -0800, Bjorn Andersson wrote: > On Thu 18 Jan 01:13 PST 2018, Rajendra Nayak wrote: > > > [].. > > > > >> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > > >> new file mode 100644 > > >> index 0000000..3f43582 > > >> --- /dev/null > > >> +++ b/drivers/soc/qcom/qcom-geni-se.c > > >> @@ -0,0 +1,1016 @@ > > >> +/* > > >> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > > >> + * > > >> + * This program is free software; you can redistribute it and/or modify > > >> + * it under the terms of the GNU General Public License version 2 and > > >> + * only version 2 as published by the Free Software Foundation. > > >> + * > > >> + * This program is distributed in the hope that it will be useful, > > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > >> + * GNU General Public License for more details. > > >> + * > > >> + */ > > > > > > Please use SPDX style license header, i.e. this file should start with: > > > > > > // SPDX-License-Identifier: GPL-2.0 > > > /* > > > * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > > > */ > > > > Looks like Mark Brown commented elsewhere [1] that we should use the C++ > > commenting style even for the Linux Foundation copyright? > > > > [1] https://marc.info/?l=linux-clk&m=151497978626135&w=2 > > > > While I can agree with Mark on the ugliness of the mixed commenting > style, this is the style that I found communicated and is what you find > in other files. Well, that's pretty new guidance. Moving target... Given that Linus said '//' comments are the only thing C++ got right, I expect to see more of them. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 12, 2018 at 5:05 PM, Karthikeyan Ramasubramanian <kramasub@codeaurora.org> 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@codeaurora.org> > Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > --- > 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 linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Jan 12, 2018 at 5:05 PM, Karthikeyan Ramasubramanian <kramasub@codeaurora.org> 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@codeaurora.org> > Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > --- > 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 > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index b81374b..b306d51 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -3,6 +3,14 @@ > # > menu "Qualcomm SoC drivers" > > +config QCOM_GENI_SE > + tristate "QCOM GENI Serial Engine Driver" One tiny nit here: maybe it makes sense to add: depends on ARCH_QCOM ...like all the other stuff in this file? Otherwise this ends up polluting .config files for non-Qualcomm architectures. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/16/2018 11:20 PM, Bjorn Andersson wrote: > On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian 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@codeaurora.org> >> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> >> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> >> --- >> drivers/soc/qcom/Kconfig | 8 + >> drivers/soc/qcom/Makefile | 1 + >> drivers/soc/qcom/qcom-geni-se.c | 1016 +++++++++++++++++++++++++++++++++++++++ > > I'm not aware of any non-serial-engine "geni" at Qualcomm, so can we > drop the "se" throughout this driver? Currently GENI is used to program just the serial engines. But it is not restricted to that. It can be used to program other hardware cores. Hence keeping "se" will help to clarify that this driver is meant for GENI based serial engines only. > >> 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 >> >> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig >> index b81374b..b306d51 100644 >> --- a/drivers/soc/qcom/Kconfig >> +++ b/drivers/soc/qcom/Kconfig >> @@ -3,6 +3,14 @@ >> # >> menu "Qualcomm SoC drivers" >> >> +config QCOM_GENI_SE >> + tristate "QCOM GENI Serial Engine Driver" > > Options in drivers/soc/qcom/Kconfig should have "depends on ARCH_QCOM", > as the makefile in this directory won't be processed when !ARCH_QCOM. I will update the config to depend on ARCH_QCOM. > >> + help >> + This module is used to manage Generic Interface (GENI) firmware based >> + Qualcomm Technologies, Inc. Universal Peripheral (QUP) Wrapper. This >> + module is also used to manage the common aspects of multiple Serial >> + Engines present in the QUP. >> + >> config QCOM_GLINK_SSR >> tristate "Qualcomm Glink SSR driver" >> depends on RPMSG >> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile >> index 40c56f6..74d5db8 100644 >> --- a/drivers/soc/qcom/Makefile >> +++ b/drivers/soc/qcom/Makefile >> @@ -1,4 +1,5 @@ >> # SPDX-License-Identifier: GPL-2.0 >> +obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o >> obj-$(CONFIG_QCOM_GLINK_SSR) += glink_ssr.o >> obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o >> obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o >> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c >> new file mode 100644 >> index 0000000..3f43582 >> --- /dev/null >> +++ b/drivers/soc/qcom/qcom-geni-se.c >> @@ -0,0 +1,1016 @@ >> +/* >> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ > > Please use SPDX style license header, i.e. this file should start with: > > // SPDX-License-Identifier: GPL-2.0 > /* > * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > */ > >> + >> +#include <linux/clk.h> >> +#include <linux/slab.h> >> +#include <linux/dma-mapping.h> >> +#include <linux/io.h> >> +#include <linux/list.h> > > This isn't used. > >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> +#include <linux/pm_runtime.h> > > Is this used? > >> +#include <linux/qcom-geni-se.h> >> +#include <linux/spinlock.h> > > Is this used? I will remove including unused header files. > >> + >> +#define MAX_CLK_PERF_LEVEL 32 >> + >> +/** >> + * @struct geni_se_device - Data structure to represent the QUP Wrapper Core > > Make this name indicate that this is the wrapper; e.g. struct geni_wrapper geni_wrapper makes sense. > >> + * @dev: Device pointer of the QUP wrapper core. >> + * @base: Base address of this instance of QUP wrapper core. >> + * @m_ahb_clk: Handle to the primary AHB clock. >> + * @s_ahb_clk: Handle to the secondary AHB clock. >> + * @geni_dev_lock: Lock to protect the device elements. >> + * @num_clk_levels: Number of valid clock levels in clk_perf_tbl. >> + * @clk_perf_tbl: Table of clock frequency input to Serial Engine clock. >> + */ >> +struct geni_se_device { >> + struct device *dev; >> + void __iomem *base; >> + struct clk *m_ahb_clk; >> + struct clk *s_ahb_clk; > > As these two clocks always seems to be operated in tandem use > clk_bulk_data to keep track of them and use the clk_bulk*() operations > to simplify your prepare/unprepare sites. I will use the clk_bulk* approach. > >> + struct mutex geni_dev_lock; > > There's only one lock and it should be obvious from context that it's > the lock of the geni_se_device, so naming this "lock" should be > sufficient. I will rename it to "lock". > >> + unsigned int num_clk_levels; >> + unsigned long *clk_perf_tbl; >> +}; >> + >> +/* Offset of QUP Hardware Version Register */ >> +#define QUP_HW_VER (0x4) > > Drop the parenthesis. It would be nice if this define indicated that > this is a register and not a value. E.g. call it QUP_HW_VER_REG. Ok. > >> + >> +#define HW_VER_MAJOR_MASK GENMASK(31, 28) >> +#define HW_VER_MAJOR_SHFT 28 >> +#define HW_VER_MINOR_MASK GENMASK(27, 16) >> +#define HW_VER_MINOR_SHFT 16 >> +#define HW_VER_STEP_MASK GENMASK(15, 0) >> + >> +/** >> + * geni_read_reg_nolog() - Helper function to read from a GENI register >> + * @base: Base address of the serial engine's register block. >> + * @offset: Offset within the serial engine's register block. >> + * >> + * Return: Return the contents of the register. >> + */ >> +unsigned int geni_read_reg_nolog(void __iomem *base, int offset) > > Remove this function. Ok. > >> +{ >> + return readl_relaxed(base + offset); >> +} >> +EXPORT_SYMBOL(geni_read_reg_nolog); >> + >> +/** >> + * geni_write_reg_nolog() - Helper function to write into a GENI register >> + * @value: Value to be written into the register. >> + * @base: Base address of the serial engine's register block. >> + * @offset: Offset within the serial engine's register block. >> + */ >> +void geni_write_reg_nolog(unsigned int value, void __iomem *base, int offset) > > Ditto Ok. > >> +{ >> + return writel_relaxed(value, (base + offset)); >> +} >> +EXPORT_SYMBOL(geni_write_reg_nolog); >> + >> +/** >> + * geni_read_reg() - Helper function to read from a GENI register >> + * @base: Base address of the serial engine's register block. >> + * @offset: Offset within the serial engine's register block. >> + * >> + * Return: Return the contents of the register. > > Drop the extra spaces after "Return:" in all your kerneldoc. Ok. > >> + */ >> +unsigned int geni_read_reg(void __iomem *base, int offset) >> +{ >> + return readl_relaxed(base + offset); >> +} >> +EXPORT_SYMBOL(geni_read_reg); >> + >> +/** >> + * geni_write_reg() - Helper function to write into a GENI register >> + * @value: Value to be written into the register. >> + * @base: Base address of the serial engine's register block. >> + * @offset: Offset within the serial engine's register block. >> + */ >> +void geni_write_reg(unsigned int value, void __iomem *base, int offset) >> +{ >> + return writel_relaxed(value, (base + offset)); > > As written, this function adds no value and I find it quite confusing > that this is used both for read/writes on the wrapper as well as the > individual functions.Ok. > > Compare: > > geni_write_reg(common_geni_m_irq_en, base, SE_GENI_M_IRQ_EN); > > with just inlining: > > writel(common_geni_m_irq_en, base + SE_GENI_M_IRQ_EN); > >> +} >> +EXPORT_SYMBOL(geni_write_reg); >> + >> +/** >> + * geni_get_qup_hw_version() - Read the QUP wrapper Hardware version >> + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. >> + * @major: Buffer for Major Version field. >> + * @minor: Buffer for Minor Version field. >> + * @step: Buffer for Step Version field. >> + * >> + * Return: 0 on success, standard Linux error codes on failure/error. >> + */ >> +int geni_get_qup_hw_version(struct device *wrapper_dev, unsigned int *major, >> + unsigned int *minor, unsigned int *step) >> +{ >> + unsigned int version; >> + struct geni_se_device *geni_se_dev; >> + >> + if (!wrapper_dev || !major || !minor || !step) >> + return -EINVAL; > > This would only happen during development, as the engineer calls the > function incorrectly. Consider it a contract that these has to be valid > and skip the checks. Ok. > >> + >> + geni_se_dev = dev_get_drvdata(wrapper_dev); >> + if (unlikely(!geni_se_dev)) >> + return -ENODEV; > > Make the children acquire the geni_se_dev handle (keep the type opaque) > and pass that instead of wrapper_dev. Then you can just drop this chunk. Ok. > >> + >> + version = geni_read_reg(geni_se_dev->base, QUP_HW_VER); >> + *major = (version & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT; >> + *minor = (version & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT; >> + *step = version & HW_VER_STEP_MASK; >> + return 0; > > If you implement these two changes there's no way this function can > fail, so you don't have to return a value here. Ok. > >> +} >> +EXPORT_SYMBOL(geni_get_qup_hw_version); >> + >> +/** >> + * geni_se_get_proto() - Read the protocol configured for a serial engine >> + * @base: Base address of the serial engine's register block. >> + * >> + * Return: Protocol value as configured in the serial engine. >> + */ >> +int geni_se_get_proto(void __iomem *base) > > I keep reading this as "geni set get proto", you should be fine just > dropping _se_ from these function names. And if you want to denote that > it's the Qualcomm GENI then make it qcom_geni_*. "_se" prefix is used to help differentiate operations on a serial engine from those on the wrapper. I can rename it as geni_se_read_proto to avoid the confusion. > > But more importantly, it is not obvious when reading this driver that > the typeless "base" being passed is that of the individual functional > block, and not the wrapper. > > If you want to do this in an "object oriented" fashion create a struct > that's common for all geni instances, include it in the context of the > individual function devices and pass it into these functions. There is a structure named geni_se_rsc to group resources associated with a serial engine. I can rename it to geni_serial_engine and include the opaque "base" address in that structure. That structure can be passed as an input parameter to all the serial engine operations. > >> +{ >> + int proto; >> + >> + proto = ((geni_read_reg(base, GENI_FW_REVISION_RO) >> + & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT); > > Don't do everything in one go, as you can't fit it on one line anyways. > Writing it like this instead makes it easier to read: > > u32 val; > > val = readl(base + GENI_FW_S_REVISION_RO); > > return (val & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT; Ok. > >> + return proto; >> +} >> +EXPORT_SYMBOL(geni_se_get_proto); >> + >> +static int geni_se_irq_en(void __iomem *base) >> +{ >> + unsigned int common_geni_m_irq_en; >> + unsigned int common_geni_s_irq_en; > > The size of these values isn't unsigned int, it's u32. And if you give > them a shorter name the function becomes easier to read. > > Further more, as you don't care about ordering you don't need two of > them either. So you should be able to just do: > > u32 val; > > val = readl(base + SE_GENI_M_IRQ_EN); > val |= M_COMMON_GENI_M_IRQ_EN; > writel(val, base + SE_GENI_M_IRQ_EN); > > val = readl(base + SE_GENI_S_IRQ_EN); > val |= S_COMMON_GENI_S_IRQ_EN; > writel(val, base + SE_GENI_S_IRQ_EN); Ok. > >> + >> + common_geni_m_irq_en = geni_read_reg(base, SE_GENI_M_IRQ_EN); >> + common_geni_s_irq_en = geni_read_reg(base, SE_GENI_S_IRQ_EN); >> + /* Common to all modes */ >> + common_geni_m_irq_en |= M_COMMON_GENI_M_IRQ_EN; >> + common_geni_s_irq_en |= S_COMMON_GENI_S_IRQ_EN; >> + >> + geni_write_reg(common_geni_m_irq_en, base, SE_GENI_M_IRQ_EN); >> + geni_write_reg(common_geni_s_irq_en, base, SE_GENI_S_IRQ_EN); >> + return 0; > > And as this can't fail there's no reason to have a return value. Ok. > >> +} >> + >> + >> +static void geni_se_set_rx_rfr_wm(void __iomem *base, unsigned int rx_wm, >> + unsigned int rx_rfr) >> +{ >> + geni_write_reg(rx_wm, base, SE_GENI_RX_WATERMARK_REG); >> + geni_write_reg(rx_rfr, base, SE_GENI_RX_RFR_WATERMARK_REG); >> +} >> + >> +static int geni_se_io_set_mode(void __iomem *base) >> +{ >> + unsigned int io_mode; >> + unsigned int geni_dma_mode; >> + >> + io_mode = geni_read_reg(base, SE_IRQ_EN); >> + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN); >> + >> + io_mode |= (GENI_M_IRQ_EN | GENI_S_IRQ_EN); >> + io_mode |= (DMA_TX_IRQ_EN | DMA_RX_IRQ_EN); >> + geni_dma_mode &= ~GENI_DMA_MODE_EN; >> + >> + geni_write_reg(io_mode, base, SE_IRQ_EN); >> + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN); >> + geni_write_reg(0, base, SE_GSI_EVENT_EN); >> + return 0; > > As this function can't fail, don't return a value. Ok. > >> +} >> + >> +static void geni_se_io_init(void __iomem *base) >> +{ >> + unsigned int io_op_ctrl; >> + unsigned int geni_cgc_ctrl; >> + unsigned int dma_general_cfg; > > These are all u32, be explicit. Ok. > >> + >> + geni_cgc_ctrl = geni_read_reg(base, GENI_CGC_CTRL); >> + dma_general_cfg = geni_read_reg(base, SE_DMA_GENERAL_CFG); >> + geni_cgc_ctrl |= DEFAULT_CGC_EN; >> + dma_general_cfg |= (AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CFG_ON | >> + DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON); > > Drop the parenthesis and there's no harm in making multiple assignments > in favour of splitting the line. Ok. > >> + io_op_ctrl = DEFAULT_IO_OUTPUT_CTRL_MSK; >> + geni_write_reg(geni_cgc_ctrl, base, GENI_CGC_CTRL); >> + geni_write_reg(dma_general_cfg, base, SE_DMA_GENERAL_CFG); > > Is there a reason why this chunk of code is a mix of 3 independent > register updates? I am not sure I understand the context of your question. This is how the hardware programming manual is describing to program the registers as part of initializing a serial engine. Please let me know if this is not the information you are looking for. > >> + >> + geni_write_reg(io_op_ctrl, base, GENI_OUTPUT_CTRL); >> + geni_write_reg(FORCE_DEFAULT, base, GENI_FORCE_DEFAULT_REG); >> +} >> + >> +/** >> + * geni_se_init() - Initialize the GENI Serial Engine >> + * @base: Base address of the serial engine's register block. >> + * @rx_wm: Receive watermark to be configured. >> + * @rx_rfr_wm: Ready-for-receive watermark to be configured. > > What's the unit for these? The unit is the number of hardware FIFO words. > >> + * >> + * This function is used to initialize the GENI serial engine, configure >> + * receive watermark and ready-for-receive watermarks. >> + * >> + * Return: 0 on success, standard Linux error codes on failure/error. > > As neither geni_se_io_set_mode() nor geni_se_irq_en() can fail there's > no way geni_se_init() can fail and as such you don't need a return value > of this function. Ok. > >> + */ >> +int geni_se_init(void __iomem *base, unsigned int rx_wm, unsigned int rx_rfr) >> +{ >> + int ret; >> + >> + geni_se_io_init(base); >> + ret = geni_se_io_set_mode(base); >> + if (ret) >> + return ret; >> + >> + geni_se_set_rx_rfr_wm(base, rx_wm, rx_rfr); >> + ret = geni_se_irq_en(base); > > Just inline these two functions here. > >> + return ret; >> +} >> +EXPORT_SYMBOL(geni_se_init); > > This is an excellent candidate for initializing a common "geni > function"-struct shared among the children, i.e. make this: > > void geni_init_port(struct geni_port *port, struct device *dev, > size_t rx_wm, rfr_wm); > > And have this do the ioremap of the memory of dev and initialize some > common "geni_port" struct, then you can pass this to some set of > geni_port_*() helper functions. Sure, this sounds like a good place to do ioremap of children and initialize common serial engine resources. > >> + >> +static int geni_se_select_fifo_mode(void __iomem *base) >> +{ >> + int proto = geni_se_get_proto(base); >> + unsigned int common_geni_m_irq_en; >> + unsigned int common_geni_s_irq_en; >> + unsigned int geni_dma_mode; > > These are of type u32. > > If you use more succinct names the function will be easier to read; what > about master, slave and mode? (Or m_en, s_en and mode) Ok. > >> + >> + geni_write_reg(0, base, SE_GSI_EVENT_EN); >> + geni_write_reg(0xFFFFFFFF, base, SE_GENI_M_IRQ_CLEAR); >> + geni_write_reg(0xFFFFFFFF, base, SE_GENI_S_IRQ_CLEAR); >> + geni_write_reg(0xFFFFFFFF, base, SE_DMA_TX_IRQ_CLR); >> + geni_write_reg(0xFFFFFFFF, base, SE_DMA_RX_IRQ_CLR); >> + geni_write_reg(0xFFFFFFFF, base, SE_IRQ_EN); > > Use lowercase for the hex constants. Ok. > >> + >> + common_geni_m_irq_en = geni_read_reg(base, SE_GENI_M_IRQ_EN); >> + common_geni_s_irq_en = geni_read_reg(base, SE_GENI_S_IRQ_EN); >> + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN); >> + if (proto != UART) { >> + common_geni_m_irq_en |= >> + (M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN | >> + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); > > Drop the parenthesis, split the assignment in multiple to make things > not span 3 lines. Ok. > >> + common_geni_s_irq_en |= S_CMD_DONE_EN; >> + } >> + geni_dma_mode &= ~GENI_DMA_MODE_EN; > > Please group things that are related. > > The compiler doesn't care if you stretch the life time of your local > variables through your functions, but the brain does. Ok. > >> + >> + geni_write_reg(common_geni_m_irq_en, base, SE_GENI_M_IRQ_EN); >> + geni_write_reg(common_geni_s_irq_en, base, SE_GENI_S_IRQ_EN); >> + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN); >> + return 0; > > This can't fail, and you ignore the returned value in > geni_se_select_mode(). Ok. > >> +} >> + >> +static int geni_se_select_dma_mode(void __iomem *base) >> +{ >> + unsigned int geni_dma_mode = 0; > > This is u32, can be shorten to "mode" and it's first use is an > assignment - so you don't have to initialize it here. Ok. > >> + >> + geni_write_reg(0, base, SE_GSI_EVENT_EN); >> + geni_write_reg(0xFFFFFFFF, base, SE_GENI_M_IRQ_CLEAR); >> + geni_write_reg(0xFFFFFFFF, base, SE_GENI_S_IRQ_CLEAR); >> + geni_write_reg(0xFFFFFFFF, base, SE_DMA_TX_IRQ_CLR); >> + geni_write_reg(0xFFFFFFFF, base, SE_DMA_RX_IRQ_CLR); >> + geni_write_reg(0xFFFFFFFF, base, SE_IRQ_EN); > > Please lower case your hex constants. Ok. > >> + >> + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN); >> + geni_dma_mode |= GENI_DMA_MODE_EN; >> + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN); >> + return 0; > > Can't fail. Ok. > >> +} >> + >> +/** >> + * geni_se_select_mode() - Select the serial engine transfer mode >> + * @base: Base address of the serial engine's register block. >> + * @mode: Transfer mode to be selected. >> + * >> + * Return: 0 on success, standard Linux error codes on failure. >> + */ >> +int geni_se_select_mode(void __iomem *base, int mode) >> +{ >> + int ret = 0; > > Drop the return value and "ret", if you want to help the developer of > child devices you can start off by a WARN_ON(mode != FIFO_MODE && mode > != SE_DMA); Ok. > >> + >> + switch (mode) { >> + case FIFO_MODE: >> + geni_se_select_fifo_mode(base); >> + break; >> + case SE_DMA: >> + geni_se_select_dma_mode(base); >> + break; >> + default: >> + ret = -EINVAL; >> + break; >> + } >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(geni_se_select_mode); >> + >> +/** >> + * 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) >> +{ >> + u32 m_cmd = (cmd << M_OPCODE_SHFT); >> + >> + m_cmd |= (params & M_PARAMS_MSK); > > Rather than initializing the variable during declaration and then > amending the value it's cleaner if you do: > > val = (cmd << M_OPCODE_SHFT) | (params & M_PARAMS_MSK); Ok. > >> + geni_write_reg(m_cmd, base, SE_GENI_M_CMD0); >> +} >> +EXPORT_SYMBOL(geni_se_setup_m_cmd); >> + > [..] >> +/** >> + * geni_se_get_tx_fifo_depth() - Get the TX fifo depth of the serial engine >> + * @base: Base address of the serial engine's register block. >> + * >> + * This function is used to get the depth i.e. number of elements in the >> + * TX fifo of the serial engine. >> + * >> + * Return: TX fifo depth in units of FIFO words. >> + */ >> +int geni_se_get_tx_fifo_depth(void __iomem *base) >> +{ >> + int tx_fifo_depth; >> + >> + tx_fifo_depth = ((geni_read_reg(base, SE_HW_PARAM_0) >> + & TX_FIFO_DEPTH_MSK) >> TX_FIFO_DEPTH_SHFT); > > It easier to read this way: > > u32 val; > > val = readl(base, SE_HW_PARAM_0); > > return (val & TX_FIFO_DEPTH_MSK) >> TX_FIFO_DEPTH_SHFT; Ok. > >> + return tx_fifo_depth; >> +} >> +EXPORT_SYMBOL(geni_se_get_tx_fifo_depth); >> + >> +/** >> + * geni_se_get_tx_fifo_width() - Get the TX fifo width of the serial engine >> + * @base: Base address of the serial engine's register block. >> + * >> + * This function is used to get the width i.e. word size per element in the >> + * TX fifo of the serial engine. >> + * >> + * Return: TX fifo width in bits >> + */ >> +int geni_se_get_tx_fifo_width(void __iomem *base) >> +{ >> + int tx_fifo_width; >> + >> + tx_fifo_width = ((geni_read_reg(base, SE_HW_PARAM_0) >> + & TX_FIFO_WIDTH_MSK) >> TX_FIFO_WIDTH_SHFT); >> + return tx_fifo_width; > > Ditto. Ok. > >> +} >> +EXPORT_SYMBOL(geni_se_get_tx_fifo_width); >> + >> +/** >> + * geni_se_get_rx_fifo_depth() - Get the RX fifo depth of the serial engine >> + * @base: Base address of the serial engine's register block. >> + * >> + * This function is used to get the depth i.e. number of elements in the >> + * RX fifo of the serial engine. >> + * >> + * Return: RX fifo depth in units of FIFO words >> + */ >> +int geni_se_get_rx_fifo_depth(void __iomem *base) >> +{ >> + int rx_fifo_depth; >> + >> + rx_fifo_depth = ((geni_read_reg(base, SE_HW_PARAM_1) >> + & RX_FIFO_DEPTH_MSK) >> RX_FIFO_DEPTH_SHFT); >> + return rx_fifo_depth; > > Ditto. Ok. > >> +} >> +EXPORT_SYMBOL(geni_se_get_rx_fifo_depth); >> + >> +/** >> + * geni_se_get_packing_config() - Get the packing configuration based on input >> + * @bpw: Bits of data per transfer word. >> + * @pack_words: Number of words per fifo element. >> + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa. >> + * @cfg0: Output buffer to hold the first half of configuration. >> + * @cfg1: Output buffer to hold the second half of configuration. >> + * >> + * This function is used to calculate the packing configuration based on >> + * the input packing requirement and the configuration logic. >> + */ >> +void geni_se_get_packing_config(int bpw, int pack_words, bool msb_to_lsb, >> + unsigned long *cfg0, unsigned long *cfg1) > > This function is used only from geni_se_config_packing() which writes > the new config to the associated registers and from the serial driver > that does the same - but here pack_words differ for rx and tx. > > If you improve geni_se_config_packing() to take rx and tx fifo sizes > independently you don't have to expose this function to the client > drivers and you don't need to return cfg0 and cfg1 like you do here. Ok. > >> +{ >> + 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; >> + int i; >> + >> + if (unlikely(iter <= 0 || iter > 4)) { > > Don't use unlikely(), this function is not called one per port, there's > no need to clutter the code to "optimize" it. > >> + *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)); >> + 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); > > Each operation in this loop depend on temp_bpw being smaller or larger > than BITS_PER_BYTE, please rewrite it with a single if/else based on > this. > >> + } >> + cfg[iter - 1] |= 1; > > The 1 you're writing here looks like an "EOF". It would be nice with a > comment describing the format of these 4 registers and the meaning of > BIT(0). > >> + *cfg0 = cfg[0] | (cfg[1] << 10); >> + *cfg1 = cfg[2] | (cfg[3] << 10); >> +} >> +EXPORT_SYMBOL(geni_se_get_packing_config); >> + >> +/** >> + * geni_se_config_packing() - Packing configuration of the serial engine >> + * @base: Base address of the serial engine's register block. >> + * @bpw: Bits of data per transfer word. >> + * @pack_words: Number of words per fifo element. >> + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa. >> + * >> + * This function is used to configure the packing rules for the current >> + * transfer. >> + */ >> +void geni_se_config_packing(void __iomem *base, int bpw, >> + int pack_words, bool msb_to_lsb) >> +{ >> + unsigned long cfg0, cfg1; > > These are u32. Ok. > >> + >> + 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); >> +} >> +EXPORT_SYMBOL(geni_se_config_packing); >> + >> +static void geni_se_clks_off(struct geni_se_rsc *rsc) >> +{ >> + struct geni_se_device *geni_se_dev; >> + >> + if (unlikely(!rsc || !rsc->wrapper_dev)) > > Drop the unlikely(). Why would you be passed a rsc with wrapper_dev > NULL? Used it just to handle any development time issues. I will remove the unlikely(). > >> + return; >> + >> + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); >> + if (unlikely(!geni_se_dev)) >> + return; >> + >> + clk_disable_unprepare(rsc->se_clk); >> + clk_disable_unprepare(geni_se_dev->s_ahb_clk); >> + clk_disable_unprepare(geni_se_dev->m_ahb_clk); >> +} >> + >> +/** >> + * geni_se_resources_off() - Turn off resources associated with the serial >> + * engine >> + * @rsc: Handle to resources associated with the serial engine. >> + * >> + * Return: 0 on success, standard Linux error codes on failure/error. >> + */ >> +int geni_se_resources_off(struct geni_se_rsc *rsc) >> +{ >> + int ret = 0; > > No need to initialize ret. Ok. > >> + struct geni_se_device *geni_se_dev; >> + >> + if (unlikely(!rsc || !rsc->wrapper_dev)) >> + return -EINVAL; >> + >> + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); >> + if (unlikely(!geni_se_dev)) > > > Only child devices would call this, so it's unlikely that this is a > probe defer. > > Also the returned value is not used. > > And drop the unlikely() Ok. > >> + return -ENODEV; >> + >> + ret = pinctrl_select_state(rsc->geni_pinctrl, rsc->geni_gpio_sleep); >> + if (ret) >> + return ret; >> + >> + geni_se_clks_off(rsc); >> + return 0; >> +} >> +EXPORT_SYMBOL(geni_se_resources_off); >> + >> +static int geni_se_clks_on(struct geni_se_rsc *rsc) >> +{ >> + int ret; >> + struct geni_se_device *geni_se_dev; > > If you name this "geni" instead, or "wrapper" the rest will be cleaner. Ok. > >> + >> + if (unlikely(!rsc || !rsc->wrapper_dev)) >> + return -EINVAL; >> + >> + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); >> + if (unlikely(!geni_se_dev)) >> + return -EPROBE_DEFER; >> + >> + ret = clk_prepare_enable(geni_se_dev->m_ahb_clk); >> + if (ret) >> + return ret; >> + >> + ret = clk_prepare_enable(geni_se_dev->s_ahb_clk); >> + if (ret) { >> + clk_disable_unprepare(geni_se_dev->m_ahb_clk); >> + return ret; >> + } > > These two could be dealt with in a single clk_bulk_prepare_enable(). Ok. > >> + >> + ret = clk_prepare_enable(rsc->se_clk); >> + if (ret) { >> + clk_disable_unprepare(geni_se_dev->s_ahb_clk); >> + clk_disable_unprepare(geni_se_dev->m_ahb_clk); >> + } >> + return ret; >> +} >> + >> +/** >> + * geni_se_resources_on() - Turn on resources associated with the serial >> + * engine >> + * @rsc: Handle to resources associated with the serial engine. >> + * >> + * Return: 0 on success, standard Linux error codes on failure/error. >> + */ >> +int geni_se_resources_on(struct geni_se_rsc *rsc) >> +{ >> + int ret = 0; >> + struct geni_se_device *geni_se_dev; >> + >> + if (unlikely(!rsc || !rsc->wrapper_dev)) >> + return -EINVAL; >> + >> + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); >> + if (unlikely(!geni_se_dev)) > > As with geni_se_resources_off() Ok. > >> + return -EPROBE_DEFER; >> + >> + ret = geni_se_clks_on(rsc); >> + if (ret) >> + return ret; >> + >> + ret = pinctrl_select_state(rsc->geni_pinctrl, rsc->geni_gpio_active); > > pinctrl_pm_select_default_state(rsc->dev); > > So you need the dev associated with the rsc. > >> + if (ret) >> + geni_se_clks_off(rsc); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(geni_se_resources_on); >> + >> +/** >> + * geni_se_clk_tbl_get() - Get the clock table to program DFS >> + * @rsc: Resource for which the clock table is requested. >> + * @tbl: Table in which the output is returned. >> + * >> + * This function is called by the protocol drivers to determine the different >> + * clock frequencies supported by Serail Engine Core Clock. The protocol > > s/Serail/Serial/ Ok. > >> + * drivers use the output to determine the clock frequency index to be >> + * programmed into DFS. >> + * >> + * Return: number of valid performance levels in the table on success, >> + * standard Linux error codes on failure. >> + */ >> +int geni_se_clk_tbl_get(struct geni_se_rsc *rsc, unsigned long **tbl) >> +{ >> + struct geni_se_device *geni_se_dev; >> + int i; >> + unsigned long prev_freq = 0; >> + int ret = 0; >> + >> + if (unlikely(!rsc || !rsc->wrapper_dev || !rsc->se_clk || !tbl)) > > Drop the "unlikely", you're only calling this once. Ok. > >> + return -EINVAL; >> + >> + *tbl = NULL; > > Don't touch tbl on error. Ok. > >> + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); >> + if (unlikely(!geni_se_dev)) >> + return -EPROBE_DEFER; > > How would this even be possible? This function should only be called by > child devices, which per definition means that this device been probed. I agree. Prior to this patchset, the serial engine nodes were not children nodes of wrapper devices. Hence the check existed. Now that the device tree nodes are re-structured, this check can be removed. > >> + >> + mutex_lock(&geni_se_dev->geni_dev_lock); >> + if (geni_se_dev->clk_perf_tbl) { >> + *tbl = geni_se_dev->clk_perf_tbl; >> + ret = geni_se_dev->num_clk_levels; >> + goto exit_se_clk_tbl_get; >> + } > > You're never freeing clk_perf_tbl, and the only reason you have > geni_dev_lock is to protect the "cached" array in geni_se_dev. > > Move the allocation and generation of clk_perf_tbl to geni_se_probe() > and store the value, then make this function just return that array. > That way you can drop the lock and the code becomes cleaner. Currently this driver is initializing in arch_init. It can be moved to module_init stage as long as the clk_round_rate() returns -EPROBE_DEFER when it is not ready. I will look into it and do the appropriate change. > >> + >> + geni_se_dev->clk_perf_tbl = kzalloc(sizeof(*geni_se_dev->clk_perf_tbl) * >> + MAX_CLK_PERF_LEVEL, GFP_KERNEL); > > Use kcalloc() Ok. > >> + if (!geni_se_dev->clk_perf_tbl) { >> + ret = -ENOMEM; >> + goto exit_se_clk_tbl_get; >> + } >> + >> + for (i = 0; i < MAX_CLK_PERF_LEVEL; i++) { >> + geni_se_dev->clk_perf_tbl[i] = clk_round_rate(rsc->se_clk, >> + prev_freq + 1); >> + if (geni_se_dev->clk_perf_tbl[i] == prev_freq) { >> + geni_se_dev->clk_perf_tbl[i] = 0; > > Use a local variable to keep the return value of clk_round_rate(), that > way you don't have to revert the value here (not that it matters...) and > you don't need to line wrap the assignment above. Ok. > >> + break; >> + } >> + prev_freq = geni_se_dev->clk_perf_tbl[i]; > > ...and then you don't need a separate local variable to keep track of > the last value... Ok. > >> + } >> + geni_se_dev->num_clk_levels = i; >> + *tbl = geni_se_dev->clk_perf_tbl; >> + ret = geni_se_dev->num_clk_levels; >> +exit_se_clk_tbl_get: > > Name your labels based on what action they perform, e.g. "out_unlock" > (not err_unlock here because it's the successful path as well) Ok. > >> + mutex_unlock(&geni_se_dev->geni_dev_lock); >> + return ret; >> +} >> +EXPORT_SYMBOL(geni_se_clk_tbl_get); >> + >> +/** >> + * geni_se_clk_freq_match() - Get the matching or closest SE clock frequency >> + * @rsc: Resource for which the clock frequency is requested. >> + * @req_freq: Requested clock frequency. >> + * @index: Index of the resultant frequency in the table. >> + * @res_freq: Resultant frequency which matches or is closer to the >> + * requested frequency. >> + * @exact: Flag to indicate exact multiple requirement of the requested >> + * frequency . >> + * >> + * This function is called by the protocol drivers to determine the matching >> + * or closest frequency of the Serial Engine clock to be selected in order >> + * to meet the performance requirements. > > What's the benefit compared to calling clk_round_rate()? > > Given that the function can return a rate that is a fraction of the > requested frequency even though "exact" is set, this description isn't > explaining the entire picture. I agree, the description makes it look like clk_round_rate(). But it is different from clk_round_rate() in the sense that it will try to return the matching or exact multiple of the requested frequency. If there is no matching or exact multiple value found, then it returns the closest floor frequency available. I will update the description accordingly. > >> + * >> + * Return: 0 on success, standard Linux error codes on failure. > > Returning the new clockrate would make more sense. > >> + */ >> +int geni_se_clk_freq_match(struct geni_se_rsc *rsc, unsigned long req_freq, >> + unsigned int *index, unsigned long *res_freq, >> + bool exact) >> +{ >> + unsigned long *tbl; >> + int num_clk_levels; >> + int i; >> + >> + num_clk_levels = geni_se_clk_tbl_get(rsc, &tbl); >> + if (num_clk_levels < 0) >> + return num_clk_levels; >> + >> + if (num_clk_levels == 0) >> + return -EFAULT; >> + >> + *res_freq = 0; >> + for (i = 0; i < num_clk_levels; i++) { >> + if (!(tbl[i] % req_freq)) { >> + *index = i; >> + *res_freq = tbl[i]; > > So this will return a frequency that divides the requested frequency > without a remainder, will index be used to calculate a multiplier from > this? The serial interface drivers need to program the hardware register with the index of the clock rate from the clock controller source. Hence the index is returned. > >> + return 0; >> + } >> + >> + if (!(*res_freq) || ((tbl[i] > *res_freq) && >> + (tbl[i] < req_freq))) { >> + *index = i; >> + *res_freq = tbl[i]; > > What if there's a previous value in tbl that given some multiplier has a > smaller difference to the requested frequency? At this point the requirement is to return matching or exact multiple of the requested frequency. If those criteria are not met, return the closest floor frequency. > >> + } >> + } >> + >> + if (exact || !(*res_freq)) > > *res_freq can't be 0, because in the case that num_clk_levels is 0 you > already returned -EFAULT above, in all other cases you will assign it at > least once. Ok. > >> + return -ENOKEY; >> + >> + return 0; > > Why not use the return value for the calculated frequency? With frequency being unsigned long, I dont see a clean way to return any error. > >> +} >> +EXPORT_SYMBOL(geni_se_clk_freq_match); >> + >> +/** >> + * 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) > > All child devices will have a "base", so if you move that into what you > today call a geni_se_rsc and you pass that to all these functions > operating on behalf of the child device you have the first two > parameters passed in the same object. > > A "tx_len" is typically of type size_t. > > Also note that there are no other buf than tx_buf, no other len than > tx_len and no other dma address than tx_dma in this function, so you can > drop "tx_" without loosing any information - but improving code > cleanliness. Ok. > >> +{ >> + int ret; >> + >> + if (unlikely(!wrapper_dev || !base || !tx_buf || !tx_len || !tx_dma)) >> + return -EINVAL; > > Reduce the error checking here. Ok. > >> + >> + ret = geni_se_map_buf(wrapper_dev, tx_dma, tx_buf, tx_len, >> + DMA_TO_DEVICE); > > I think you should pass the wrapper itself to geni_se_tx_dma_prep() and > if you name this "wrapper" (instead of wrapper_dev) you're under 80 > chars and don't need the line break. Ok. > >> + if (ret) >> + return ret; >> + >> + geni_write_reg(7, base, SE_DMA_TX_IRQ_EN_SET); > > So that's DMA_RX_IRQ_EN | DMA_TX_IRQ_EN | GENI_M_IRQ_EN? No, that is DMA_DONE_EN (BIT 0), EOT_EN (BIT 1) and AHB_ERR_EN (BIT 2). > >> + 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); > > If you return the dma_addr_t from this function you will have the happy > side effect of being able to use tx_dma directly instead of *tx_dma and > you can remove some craziness from these calls. > > > >> + geni_write_reg(1, base, SE_DMA_TX_ATTR); > > This "1" would be nice to have some clarification on. This indicates to the hardware that the current buffer is of End-of-Transfer Type. I will add a comment here or replace the magic 1 with an appropriate preprocessor macro. > >> + geni_write_reg(tx_len, base, SE_DMA_TX_LEN); >> + return 0; >> +} >> +EXPORT_SYMBOL(geni_se_tx_dma_prep); >> + >> +/** >> + * geni_se_rx_dma_prep() - Prepare the Serial Engine for RX DMA transfer >> + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped. >> + * @base: Base address of the SE register block. >> + * @rx_buf: Pointer to the RX buffer. >> + * @rx_len: Length of the RX buffer. >> + * @rx_dma: Pointer to store the mapped DMA address. >> + * >> + * This function is used to prepare the buffers for DMA RX. >> + * >> + * Return: 0 on success, standard Linux error codes on error/failure. > > The only real error that can come out of this is dma_mapping_error(), so > just return the dma_addr_t. Ok. > >> + */ >> +int geni_se_rx_dma_prep(struct device *wrapper_dev, void __iomem *base, >> + void *rx_buf, int rx_len, dma_addr_t *rx_dma) > > As with tx, drop all the "rx_". And pass that geni_se_rsc object > instead. Ok. > >> +{ >> + int ret; >> + >> + if (unlikely(!wrapper_dev || !base || !rx_buf || !rx_len || !rx_dma)) >> + return -EINVAL; >> + >> + ret = geni_se_map_buf(wrapper_dev, rx_dma, rx_buf, rx_len, >> + DMA_FROM_DEVICE); >> + if (ret) >> + return ret; >> + >> + geni_write_reg(7, base, SE_DMA_RX_IRQ_EN_SET); > > We enable same interrupts for rx as tx? Why enable TX interrupt here? No, the interrupts enabled are DMA_DONE_EN, EOT_EN and AHB_ERR_EN. > >> + geni_write_reg((u32)(*rx_dma), base, SE_DMA_RX_PTR_L); >> + geni_write_reg((u32)((*rx_dma) >> 32), base, SE_DMA_RX_PTR_H); >> + /* RX does not have EOT bit */ > > Okay? How does this relate to the 0 being written into RX_ATTR? Instead of EOT bit, this register has RX DMA mode bits. Writing 0 means the buffer is a single buffer. Writing 1 means the data is present as part of the command itself and 2 means the buffer is scatter-gather buffer. > >> + geni_write_reg(0, base, SE_DMA_RX_ATTR); >> + geni_write_reg(rx_len, base, SE_DMA_RX_LEN); >> + return 0; >> +} >> +EXPORT_SYMBOL(geni_se_rx_dma_prep); >> + >> +/** >> + * geni_se_tx_dma_unprep() - Unprepare the Serial Engine after TX DMA transfer >> + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped. >> + * @tx_dma: DMA address of the TX buffer. >> + * @tx_len: Length of the TX buffer. >> + * >> + * This function is used to unprepare the DMA buffers after DMA TX. >> + */ >> +void geni_se_tx_dma_unprep(struct device *wrapper_dev, >> + dma_addr_t tx_dma, int tx_len) >> +{ >> + if (tx_dma) >> + geni_se_unmap_buf(wrapper_dev, &tx_dma, tx_len, >> + DMA_TO_DEVICE); >> +} >> +EXPORT_SYMBOL(geni_se_tx_dma_unprep); >> + >> +/** >> + * geni_se_rx_dma_unprep() - Unprepare the Serial Engine after RX DMA transfer >> + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped. >> + * @rx_dma: DMA address of the RX buffer. >> + * @rx_len: Length of the RX buffer. >> + * >> + * This function is used to unprepare the DMA buffers after DMA RX. >> + */ >> +void geni_se_rx_dma_unprep(struct device *wrapper_dev, >> + dma_addr_t rx_dma, int rx_len) >> +{ >> + if (rx_dma) >> + geni_se_unmap_buf(wrapper_dev, &rx_dma, rx_len, >> + DMA_FROM_DEVICE); >> +} >> +EXPORT_SYMBOL(geni_se_rx_dma_unprep); >> + >> +/** >> + * geni_se_map_buf() - Map a single buffer into QUP wrapper device > > I find the mixture of prepare and map in the API (where prepare also > maps) confusing, but no-one calls genbi_se_map_buf() can it be made > internal? Sure it can be made internal and later exposed when the serial interface drivers need it. > >> + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. >> + * @iova: Pointer in which the mapped virtual address is stored. >> + * @buf: Address of the buffer that needs to be mapped. >> + * @size: Size of the buffer. >> + * @dir: Direction of the DMA transfer. >> + * >> + * This function is used to map an already allocated buffer into the >> + * QUP device space. >> + * >> + * Return: 0 on success, standard Linux error codes on failure/error. >> + */ >> +int geni_se_map_buf(struct device *wrapper_dev, dma_addr_t *iova, >> + void *buf, size_t size, enum dma_data_direction dir) >> +{ >> + struct device *dev_p; >> + struct geni_se_device *geni_se_dev; >> + >> + if (!wrapper_dev || !iova || !buf || !size) >> + return -EINVAL; > > No need to check that the programmer of the intermediate function > checked that the programmer of the client filled all these out > correctly. Ok. > >> + >> + geni_se_dev = dev_get_drvdata(wrapper_dev); >> + if (!geni_se_dev || !geni_se_dev->dev) >> + return -ENODEV; > > Pass the geni_se_device from the child driver, to remove the need for > this. Ok. > >> + >> + dev_p = geni_se_dev->dev; > > Name your variables more succinct and you don't need this local alias. Ok. > >> + >> + *iova = dma_map_single(dev_p, buf, size, dir); > > Just inline dma_map_single() in the functions calling this. Ok. > >> + if (dma_mapping_error(dev_p, *iova)) >> + return -EIO; >> + return 0; >> +} >> +EXPORT_SYMBOL(geni_se_map_buf); >> + >> +/** >> + * geni_se_unmap_buf() - Unmap a single buffer from QUP wrapper device >> + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. >> + * @iova: Pointer in which the mapped virtual address is stored. >> + * @size: Size of the buffer. >> + * @dir: Direction of the DMA transfer. >> + * >> + * This function is used to unmap an already mapped buffer from the >> + * QUP device space. >> + * >> + * Return: 0 on success, standard Linux error codes on failure/error. >> + */ >> +int geni_se_unmap_buf(struct device *wrapper_dev, dma_addr_t *iova, >> + size_t size, enum dma_data_direction dir) > > There's no reason for iova to be a pointer. Just pass the dma_addr_t as > is. Ok. > > Should this function really be exposed to the clients? It can be made internal based on the current use-case. > >> +{ >> + struct device *dev_p; >> + struct geni_se_device *geni_se_dev; >> + >> + if (!wrapper_dev || !iova || !size) >> + return -EINVAL; > > Reduce the error checking. Ok. > >> + >> + geni_se_dev = dev_get_drvdata(wrapper_dev); >> + if (!geni_se_dev || !geni_se_dev->dev) >> + return -ENODEV; > > And pass the geni_se_rsc to this function for symmetry purposes, which > would give you the wrapper by just following the pointer and then the > device from there. Ok. > >> + >> + dev_p = geni_se_dev->dev; >> + dma_unmap_single(dev_p, *iova, size, dir); > > Just inline dma_unmap_single() in the calling functions. Ok. > >> + return 0; >> +} >> +EXPORT_SYMBOL(geni_se_unmap_buf); >> + >> +static const struct of_device_id geni_se_dt_match[] = { >> + { .compatible = "qcom,geni-se-qup", }, >> + {} >> +}; > > Move this next to the geni_se_driver below and don't forget the > MODULE_DEVICE_TABLE() Ok. > >> + >> +static int geni_se_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct resource *res; >> + struct geni_se_device *geni_se_dev; > > Just name this "geni". Ok. > >> + int ret = 0; > > No need to initialize ret, it's only ever used after assignment. Ok. > >> + >> + geni_se_dev = devm_kzalloc(dev, sizeof(*geni_se_dev), GFP_KERNEL); >> + if (!geni_se_dev) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) { >> + dev_err(dev, "%s: Mandatory resource info not found\n", >> + __func__); >> + devm_kfree(dev, geni_se_dev); >> + return -EINVAL; >> + } > > It's idiomatic to not check for errors of platform_get_resource() as > devm_ioremap_resource() will fail gracefully if this happens. Ok, I will remove the check and use the error returned by devm_ioremap_resource, if any. > >> + >> + geni_se_dev->base = devm_ioremap_resource(dev, res); >> + if (IS_ERR_OR_NULL(geni_se_dev->base)) { > > This should be IS_ERR() only. Ok. > >> + dev_err(dev, "%s: Error mapping the resource\n", __func__); >> + devm_kfree(dev, geni_se_dev); >> + return -EFAULT; >> + } >> + >> + geni_se_dev->m_ahb_clk = devm_clk_get(dev, "m-ahb"); >> + if (IS_ERR(geni_se_dev->m_ahb_clk)) { >> + ret = PTR_ERR(geni_se_dev->m_ahb_clk); >> + dev_err(dev, "Err getting M AHB clk %d\n", ret); >> + devm_iounmap(dev, geni_se_dev->base); >> + devm_kfree(dev, geni_se_dev); > > The reason we use the devm_ versions of these is so that we don't have > to release our resources explicitly. Ok, I will remove releasing the resources explicitly. > >> + return ret; >> + } >> + >> + geni_se_dev->s_ahb_clk = devm_clk_get(dev, "s-ahb"); >> + if (IS_ERR(geni_se_dev->s_ahb_clk)) { >> + ret = PTR_ERR(geni_se_dev->s_ahb_clk); >> + dev_err(dev, "Err getting S AHB clk %d\n", ret); >> + devm_clk_put(dev, geni_se_dev->m_ahb_clk); >> + devm_iounmap(dev, geni_se_dev->base); >> + devm_kfree(dev, geni_se_dev); >> + return ret; >> + } > > Use devm_clk_bulk_get(). Ok. > >> + >> + geni_se_dev->dev = dev; >> + mutex_init(&geni_se_dev->geni_dev_lock); >> + dev_set_drvdata(dev, geni_se_dev); >> + dev_dbg(dev, "GENI SE Driver probed\n"); >> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > > You're not depopulating these devices when the wrapper goes away. Use > devm_of_platform_populate() here instead to make that happen. Ok. > >> +} >> + >> +static int geni_se_remove(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct geni_se_device *geni_se_dev = dev_get_drvdata(dev); >> + >> + devm_clk_put(dev, geni_se_dev->s_ahb_clk); >> + devm_clk_put(dev, geni_se_dev->m_ahb_clk); >> + devm_iounmap(dev, geni_se_dev->base); >> + devm_kfree(dev, geni_se_dev); > > Again, the reason to use devm_* is so that you don't have to free > things...so if this is what you have here you don't even need a remove > function. Ok. > >> + return 0; >> +} >> + >> +static struct platform_driver geni_se_driver = { >> + .driver = { >> + .name = "geni_se_qup", >> + .of_match_table = geni_se_dt_match, >> + }, >> + .probe = geni_se_probe, >> + .remove = geni_se_remove, >> +}; >> + >> +static int __init geni_se_driver_init(void) >> +{ >> + return platform_driver_register(&geni_se_driver); >> +} >> +arch_initcall(geni_se_driver_init); >> + >> +static void __exit geni_se_driver_exit(void) >> +{ >> + platform_driver_unregister(&geni_se_driver); >> +} >> +module_exit(geni_se_driver_exit); > > Should be fine to just use module_platform_driver(), you need separate > support for earlycon regardless. Ok. > >> + >> +MODULE_DESCRIPTION("GENI Serial Engine Driver"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h >> new file mode 100644 >> index 0000000..5695da9 >> --- /dev/null >> +++ b/include/linux/qcom-geni-se.h >> @@ -0,0 +1,807 @@ >> +/* >> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > [..] >> + */ >> + > > Use SPDX header as above. Ok. > >> +#ifndef _LINUX_QCOM_GENI_SE >> +#define _LINUX_QCOM_GENI_SE >> +#include <linux/clk.h> >> +#include <linux/dma-direction.h> >> +#include <linux/io.h> >> +#include <linux/list.h> > > I don't think there's a need for io.h or list.h here. Ok. > >> + >> +/* Transfer mode supported by GENI Serial Engines */ >> +enum geni_se_xfer_mode { >> + INVALID, >> + FIFO_MODE, >> + SE_DMA, > > These are quite bad names for things in a header file. I am not sure if you are suggesting me to use a prefix to make these definitions look GENI specific. I would prefer some clarification in this regard. > >> +}; >> + >> +/* Protocols supported by GENI Serial Engines */ >> +enum geni_se_protocol_types { >> + NONE, >> + SPI, >> + UART, >> + I2C, >> + I3C > > Ditto > >> +}; >> + >> +/** >> + * struct geni_se_rsc - GENI Serial Engine Resource >> + * @wrapper_dev: Pointer to the parent QUP Wrapper core. >> + * @se_clk: Handle to the core serial engine clock. >> + * @geni_pinctrl: Handle to the pinctrl configuration. >> + * @geni_gpio_active: Handle to the default/active pinctrl state. >> + * @geni_gpi_sleep: Handle to the sleep pinctrl state. >> + */ >> +struct geni_se_rsc { > > This looks like the common geni_port or geni_device I requested above. Yes that is right. I will use it to include the serial engine base address. > >> + struct device *wrapper_dev; >> + struct clk *se_clk; > > The one and only clock can be named "clk". Ok. > >> + struct pinctrl *geni_pinctrl; >> + struct pinctrl_state *geni_gpio_active; >> + struct pinctrl_state *geni_gpio_sleep; > > The associated struct device already has init, default, idle and sleep > pinctrl states associated through the device->pins struct. Typically > this means that if you provide a "default" and "sleep" state, the > default will be selected when the device probes. > > In order to select between the states call > pinctrl_pm_select_{default,sleep,idle}_state(dev); Ok. > >> +}; >> + >> +#define PINCTRL_DEFAULT "default" >> +#define PINCTRL_SLEEP "sleep" >> + >> +/* Common SE registers */ > > The purpose of the helper functions in the wrapper driver is to hide > the common details from the individual function drivers, so move these > defines into the c-file as they shouldn't be needed in the individual > drivers. I will move the register definitions that are accessed through helper functions inside the source file. I may have to keep those register definitions that are common and are directly accessed by the serial interface drivers here so that there are no duplicate definitions. > >> +#define GENI_INIT_CFG_REVISION (0x0) > > Drop the parenthesis. Ok. > > [..] >> +#ifdef CONFIG_QCOM_GENI_SE >> +/** >> + * geni_read_reg_nolog() - Helper function to read from a GENI register >> + * @base: Base address of the serial engine's register block. >> + * @offset: Offset within the serial engine's register block. >> + * >> + * Return: Return the contents of the register. >> + */ > > The kerneldoc goes in the implementation, not the header file. And you > already have a copy there, so remove it from here. Ok. > >> +unsigned int geni_read_reg_nolog(void __iomem *base, int offset); > [..] >> +#else > > I see no point in providing dummy functions for these, just make the > individual drivers either depend or select the core helpers. Ok. > >> +static inline unsigned int geni_read_reg_nolog(void __iomem *base, int offset) >> +{ >> + return 0; >> +} > > Regards, > Bjorn > Regards, Karthik.
On 1/19/2018 3:57 PM, Rob Herring wrote: > On Thu, Jan 18, 2018 at 08:57:45AM -0800, Bjorn Andersson wrote: >> On Thu 18 Jan 01:13 PST 2018, Rajendra Nayak wrote: >> >>> [].. >>> >>>>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c >>>>> new file mode 100644 >>>>> index 0000000..3f43582 >>>>> --- /dev/null >>>>> +++ b/drivers/soc/qcom/qcom-geni-se.c >>>>> @@ -0,0 +1,1016 @@ >>>>> +/* >>>>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. >>>>> + * >>>>> + * This program is free software; you can redistribute it and/or modify >>>>> + * it under the terms of the GNU General Public License version 2 and >>>>> + * only version 2 as published by the Free Software Foundation. >>>>> + * >>>>> + * This program is distributed in the hope that it will be useful, >>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>> + * GNU General Public License for more details. >>>>> + * >>>>> + */ >>>> >>>> Please use SPDX style license header, i.e. this file should start with: >>>> >>>> // SPDX-License-Identifier: GPL-2.0 >>>> /* >>>> * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. >>>> */ >>> >>> Looks like Mark Brown commented elsewhere [1] that we should use the C++ >>> commenting style even for the Linux Foundation copyright? >>> >>> [1] https://marc.info/?l=linux-clk&m=151497978626135&w=2 >>> >> >> While I can agree with Mark on the ugliness of the mixed commenting >> style, this is the style that I found communicated and is what you find >> in other files. > > Well, that's pretty new guidance. Moving target... > > Given that Linus said '//' comments are the only thing C++ got right, I > expect to see more of them. I believe that in the source file I have to use C++ style comments(as per this discussion) and in the header file I have to use C-style comments (as per https://lwn.net/Articles/739183/ for reasons related to tooling). Please correct me otherwise. > > Rob > Regards, Karthik.
On Wed 31 Jan 10:58 PST 2018, Karthik Ramasubramanian wrote: > On 1/16/2018 11:20 PM, Bjorn Andersson wrote: > > On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian wrote: [..] > > I'm not aware of any non-serial-engine "geni" at Qualcomm, so can we > > drop the "se" throughout this driver? > Currently GENI is used to program just the serial engines. But it is not > restricted to that. It can be used to program other hardware cores. Hence > keeping "se" will help to clarify that this driver is meant for GENI based > serial engines only. Okay, fair enough. [..] > > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c [..] > > > + geni_cgc_ctrl = geni_read_reg(base, GENI_CGC_CTRL); > > > + dma_general_cfg = geni_read_reg(base, SE_DMA_GENERAL_CFG); > > > + geni_cgc_ctrl |= DEFAULT_CGC_EN; > > > + dma_general_cfg |= (AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CFG_ON | > > > + DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON); > > > > Drop the parenthesis and there's no harm in making multiple assignments > > in favour of splitting the line. > Ok. > > > > > + io_op_ctrl = DEFAULT_IO_OUTPUT_CTRL_MSK; > > > + geni_write_reg(geni_cgc_ctrl, base, GENI_CGC_CTRL); > > > + geni_write_reg(dma_general_cfg, base, SE_DMA_GENERAL_CFG); > > > > Is there a reason why this chunk of code is a mix of 3 independent > > register updates? > I am not sure I understand the context of your question. This is how the > hardware programming manual is describing to program the registers as part > of initializing a serial engine. Please let me know if this is not the > information you are looking for. Can you please double check with the hardware guys if it really is required that you do: a = read(A) b = read(B) modify a modify b assign c write(a) write(b) write(c) And if that is the case, then try to make things as easy to read as possible - e.g. by inlining the value of "c" and adding an empty line between reads, modifications and writes as I did here. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 31 Jan 11:02 PST 2018, Karthik Ramasubramanian wrote: > On 1/19/2018 3:57 PM, Rob Herring wrote: > > On Thu, Jan 18, 2018 at 08:57:45AM -0800, Bjorn Andersson wrote: > > > On Thu 18 Jan 01:13 PST 2018, Rajendra Nayak wrote: [..] > > > > > > > > > > Please use SPDX style license header, i.e. this file should start with: > > > > > > > > > > // SPDX-License-Identifier: GPL-2.0 > > > > > /* > > > > > * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > > > > > */ > > > > > > > > Looks like Mark Brown commented elsewhere [1] that we should use the C++ > > > > commenting style even for the Linux Foundation copyright? > > > > > > > > [1] https://marc.info/?l=linux-clk&m=151497978626135&w=2 > > > > > > > > > > While I can agree with Mark on the ugliness of the mixed commenting > > > style, this is the style that I found communicated and is what you find > > > in other files. > > > > Well, that's pretty new guidance. Moving target... > > > > Given that Linus said '//' comments are the only thing C++ got right, I > > expect to see more of them. > I believe that in the source file I have to use C++ style comments(as per > this discussion) and in the header file I have to use C-style comments (as > per https://lwn.net/Articles/739183/ for reasons related to tooling). Please > correct me otherwise. I had missed that detail, you're right. Thanks, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jan 13, 2018 at 6:35 AM, Karthikeyan Ramasubramanian <kramasub@codeaurora.org> 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@codeaurora.org> > Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > +int geni_se_resources_off(struct geni_se_rsc *rsc) > +{ > + int ret = 0; > + struct geni_se_device *geni_se_dev; > + > + if (unlikely(!rsc || !rsc->wrapper_dev)) > + return -EINVAL; > + > + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); > + if (unlikely(!geni_se_dev)) > + return -ENODEV; > + > + ret = pinctrl_select_state(rsc->geni_pinctrl, rsc->geni_gpio_sleep); You need to include linux/pinctrl/consumer.h for devm_pinctrl_get I couldn't compile test it w/o it. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/14/2018 4:07 AM, Amit Kucheria wrote: > On Sat, Jan 13, 2018 at 6:35 AM, Karthikeyan Ramasubramanian > <kramasub@codeaurora.org> 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@codeaurora.org> >> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> >> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > > >> +int geni_se_resources_off(struct geni_se_rsc *rsc) >> +{ >> + int ret = 0; >> + struct geni_se_device *geni_se_dev; >> + >> + if (unlikely(!rsc || !rsc->wrapper_dev)) >> + return -EINVAL; >> + >> + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); >> + if (unlikely(!geni_se_dev)) >> + return -ENODEV; >> + >> + ret = pinctrl_select_state(rsc->geni_pinctrl, rsc->geni_gpio_sleep); > > You need to include linux/pinctrl/consumer.h for devm_pinctrl_get > > I couldn't compile test it w/o it. > I think marking the dependencies might help with this compilation issue. Else I will include the concerned header file. One way or the other, I will ensure that this issue does not get hit in my follow-up patch series. Regards, Karthik.
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index b81374b..b306d51 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -3,6 +3,14 @@ # menu "Qualcomm SoC drivers" +config QCOM_GENI_SE + tristate "QCOM GENI Serial Engine Driver" + help + This module is used to manage Generic Interface (GENI) firmware based + Qualcomm Technologies, Inc. Universal Peripheral (QUP) Wrapper. This + module is also used to manage the common aspects of multiple Serial + Engines present in the QUP. + config QCOM_GLINK_SSR tristate "Qualcomm Glink SSR driver" depends on RPMSG diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index 40c56f6..74d5db8 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o obj-$(CONFIG_QCOM_GLINK_SSR) += glink_ssr.o obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c new file mode 100644 index 0000000..3f43582 --- /dev/null +++ b/drivers/soc/qcom/qcom-geni-se.c @@ -0,0 +1,1016 @@ +/* + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include <linux/clk.h> +#include <linux/slab.h> +#include <linux/dma-mapping.h> +#include <linux/io.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/pm_runtime.h> +#include <linux/qcom-geni-se.h> +#include <linux/spinlock.h> + +#define MAX_CLK_PERF_LEVEL 32 + +/** + * @struct geni_se_device - Data structure to represent the QUP Wrapper Core + * @dev: Device pointer of the QUP wrapper core. + * @base: Base address of this instance of QUP wrapper core. + * @m_ahb_clk: Handle to the primary AHB clock. + * @s_ahb_clk: Handle to the secondary AHB clock. + * @geni_dev_lock: Lock to protect the device elements. + * @num_clk_levels: Number of valid clock levels in clk_perf_tbl. + * @clk_perf_tbl: Table of clock frequency input to Serial Engine clock. + */ +struct geni_se_device { + struct device *dev; + void __iomem *base; + struct clk *m_ahb_clk; + struct clk *s_ahb_clk; + struct mutex geni_dev_lock; + unsigned int num_clk_levels; + unsigned long *clk_perf_tbl; +}; + +/* Offset of QUP Hardware Version Register */ +#define QUP_HW_VER (0x4) + +#define HW_VER_MAJOR_MASK GENMASK(31, 28) +#define HW_VER_MAJOR_SHFT 28 +#define HW_VER_MINOR_MASK GENMASK(27, 16) +#define HW_VER_MINOR_SHFT 16 +#define HW_VER_STEP_MASK GENMASK(15, 0) + +/** + * geni_read_reg_nolog() - Helper function to read from a GENI register + * @base: Base address of the serial engine's register block. + * @offset: Offset within the serial engine's register block. + * + * Return: Return the contents of the register. + */ +unsigned int geni_read_reg_nolog(void __iomem *base, int offset) +{ + return readl_relaxed(base + offset); +} +EXPORT_SYMBOL(geni_read_reg_nolog); + +/** + * geni_write_reg_nolog() - Helper function to write into a GENI register + * @value: Value to be written into the register. + * @base: Base address of the serial engine's register block. + * @offset: Offset within the serial engine's register block. + */ +void geni_write_reg_nolog(unsigned int value, void __iomem *base, int offset) +{ + return writel_relaxed(value, (base + offset)); +} +EXPORT_SYMBOL(geni_write_reg_nolog); + +/** + * geni_read_reg() - Helper function to read from a GENI register + * @base: Base address of the serial engine's register block. + * @offset: Offset within the serial engine's register block. + * + * Return: Return the contents of the register. + */ +unsigned int geni_read_reg(void __iomem *base, int offset) +{ + return readl_relaxed(base + offset); +} +EXPORT_SYMBOL(geni_read_reg); + +/** + * geni_write_reg() - Helper function to write into a GENI register + * @value: Value to be written into the register. + * @base: Base address of the serial engine's register block. + * @offset: Offset within the serial engine's register block. + */ +void geni_write_reg(unsigned int value, void __iomem *base, int offset) +{ + return writel_relaxed(value, (base + offset)); +} +EXPORT_SYMBOL(geni_write_reg); + +/** + * geni_get_qup_hw_version() - Read the QUP wrapper Hardware version + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. + * @major: Buffer for Major Version field. + * @minor: Buffer for Minor Version field. + * @step: Buffer for Step Version field. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_get_qup_hw_version(struct device *wrapper_dev, unsigned int *major, + unsigned int *minor, unsigned int *step) +{ + unsigned int version; + struct geni_se_device *geni_se_dev; + + if (!wrapper_dev || !major || !minor || !step) + return -EINVAL; + + geni_se_dev = dev_get_drvdata(wrapper_dev); + if (unlikely(!geni_se_dev)) + return -ENODEV; + + version = geni_read_reg(geni_se_dev->base, QUP_HW_VER); + *major = (version & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT; + *minor = (version & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT; + *step = version & HW_VER_STEP_MASK; + return 0; +} +EXPORT_SYMBOL(geni_get_qup_hw_version); + +/** + * geni_se_get_proto() - Read the protocol configured for a serial engine + * @base: Base address of the serial engine's register block. + * + * Return: Protocol value as configured in the serial engine. + */ +int geni_se_get_proto(void __iomem *base) +{ + int proto; + + proto = ((geni_read_reg(base, GENI_FW_REVISION_RO) + & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT); + return proto; +} +EXPORT_SYMBOL(geni_se_get_proto); + +static int geni_se_irq_en(void __iomem *base) +{ + unsigned int common_geni_m_irq_en; + unsigned int common_geni_s_irq_en; + + common_geni_m_irq_en = geni_read_reg(base, SE_GENI_M_IRQ_EN); + common_geni_s_irq_en = geni_read_reg(base, SE_GENI_S_IRQ_EN); + /* Common to all modes */ + common_geni_m_irq_en |= M_COMMON_GENI_M_IRQ_EN; + common_geni_s_irq_en |= S_COMMON_GENI_S_IRQ_EN; + + geni_write_reg(common_geni_m_irq_en, base, SE_GENI_M_IRQ_EN); + geni_write_reg(common_geni_s_irq_en, base, SE_GENI_S_IRQ_EN); + return 0; +} + + +static void geni_se_set_rx_rfr_wm(void __iomem *base, unsigned int rx_wm, + unsigned int rx_rfr) +{ + geni_write_reg(rx_wm, base, SE_GENI_RX_WATERMARK_REG); + geni_write_reg(rx_rfr, base, SE_GENI_RX_RFR_WATERMARK_REG); +} + +static int geni_se_io_set_mode(void __iomem *base) +{ + unsigned int io_mode; + unsigned int geni_dma_mode; + + io_mode = geni_read_reg(base, SE_IRQ_EN); + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN); + + io_mode |= (GENI_M_IRQ_EN | GENI_S_IRQ_EN); + io_mode |= (DMA_TX_IRQ_EN | DMA_RX_IRQ_EN); + geni_dma_mode &= ~GENI_DMA_MODE_EN; + + geni_write_reg(io_mode, base, SE_IRQ_EN); + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN); + geni_write_reg(0, base, SE_GSI_EVENT_EN); + return 0; +} + +static void geni_se_io_init(void __iomem *base) +{ + unsigned int io_op_ctrl; + unsigned int geni_cgc_ctrl; + unsigned int dma_general_cfg; + + geni_cgc_ctrl = geni_read_reg(base, GENI_CGC_CTRL); + dma_general_cfg = geni_read_reg(base, SE_DMA_GENERAL_CFG); + geni_cgc_ctrl |= DEFAULT_CGC_EN; + dma_general_cfg |= (AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CFG_ON | + DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON); + io_op_ctrl = DEFAULT_IO_OUTPUT_CTRL_MSK; + geni_write_reg(geni_cgc_ctrl, base, GENI_CGC_CTRL); + geni_write_reg(dma_general_cfg, base, SE_DMA_GENERAL_CFG); + + geni_write_reg(io_op_ctrl, base, GENI_OUTPUT_CTRL); + geni_write_reg(FORCE_DEFAULT, base, GENI_FORCE_DEFAULT_REG); +} + +/** + * geni_se_init() - Initialize the GENI Serial Engine + * @base: Base address of the serial engine's register block. + * @rx_wm: Receive watermark to be configured. + * @rx_rfr_wm: Ready-for-receive watermark to be configured. + * + * This function is used to initialize the GENI serial engine, configure + * receive watermark and ready-for-receive watermarks. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_se_init(void __iomem *base, unsigned int rx_wm, unsigned int rx_rfr) +{ + int ret; + + geni_se_io_init(base); + ret = geni_se_io_set_mode(base); + if (ret) + return ret; + + geni_se_set_rx_rfr_wm(base, rx_wm, rx_rfr); + ret = geni_se_irq_en(base); + return ret; +} +EXPORT_SYMBOL(geni_se_init); + +static int geni_se_select_fifo_mode(void __iomem *base) +{ + int proto = geni_se_get_proto(base); + unsigned int common_geni_m_irq_en; + unsigned int common_geni_s_irq_en; + unsigned int geni_dma_mode; + + geni_write_reg(0, base, SE_GSI_EVENT_EN); + geni_write_reg(0xFFFFFFFF, base, SE_GENI_M_IRQ_CLEAR); + geni_write_reg(0xFFFFFFFF, base, SE_GENI_S_IRQ_CLEAR); + geni_write_reg(0xFFFFFFFF, base, SE_DMA_TX_IRQ_CLR); + geni_write_reg(0xFFFFFFFF, base, SE_DMA_RX_IRQ_CLR); + geni_write_reg(0xFFFFFFFF, base, SE_IRQ_EN); + + common_geni_m_irq_en = geni_read_reg(base, SE_GENI_M_IRQ_EN); + common_geni_s_irq_en = geni_read_reg(base, SE_GENI_S_IRQ_EN); + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN); + if (proto != UART) { + common_geni_m_irq_en |= + (M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN | + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); + common_geni_s_irq_en |= S_CMD_DONE_EN; + } + geni_dma_mode &= ~GENI_DMA_MODE_EN; + + geni_write_reg(common_geni_m_irq_en, base, SE_GENI_M_IRQ_EN); + geni_write_reg(common_geni_s_irq_en, base, SE_GENI_S_IRQ_EN); + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN); + return 0; +} + +static int geni_se_select_dma_mode(void __iomem *base) +{ + unsigned int geni_dma_mode = 0; + + geni_write_reg(0, base, SE_GSI_EVENT_EN); + geni_write_reg(0xFFFFFFFF, base, SE_GENI_M_IRQ_CLEAR); + geni_write_reg(0xFFFFFFFF, base, SE_GENI_S_IRQ_CLEAR); + geni_write_reg(0xFFFFFFFF, base, SE_DMA_TX_IRQ_CLR); + geni_write_reg(0xFFFFFFFF, base, SE_DMA_RX_IRQ_CLR); + geni_write_reg(0xFFFFFFFF, base, SE_IRQ_EN); + + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN); + geni_dma_mode |= GENI_DMA_MODE_EN; + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN); + return 0; +} + +/** + * geni_se_select_mode() - Select the serial engine transfer mode + * @base: Base address of the serial engine's register block. + * @mode: Transfer mode to be selected. + * + * Return: 0 on success, standard Linux error codes on failure. + */ +int geni_se_select_mode(void __iomem *base, int mode) +{ + int ret = 0; + + switch (mode) { + case FIFO_MODE: + geni_se_select_fifo_mode(base); + break; + case SE_DMA: + geni_se_select_dma_mode(base); + break; + default: + ret = -EINVAL; + break; + } + + return ret; +} +EXPORT_SYMBOL(geni_se_select_mode); + +/** + * 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) +{ + u32 m_cmd = (cmd << M_OPCODE_SHFT); + + m_cmd |= (params & M_PARAMS_MSK); + geni_write_reg(m_cmd, base, SE_GENI_M_CMD0); +} +EXPORT_SYMBOL(geni_se_setup_m_cmd); + +/** + * 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) +{ + u32 s_cmd = geni_read_reg(base, SE_GENI_S_CMD0); + + s_cmd &= ~(S_OPCODE_MSK | S_PARAMS_MSK); + s_cmd |= (cmd << S_OPCODE_SHFT); + s_cmd |= (params & S_PARAMS_MSK); + geni_write_reg(s_cmd, base, SE_GENI_S_CMD0); +} +EXPORT_SYMBOL(geni_se_setup_s_cmd); + +/** + * geni_se_cancel_m_cmd() - Cancel the command configured in the primary + * sequencer + * @base: Base address of the serial engine's register block. + * + * This function is used to cancel the currently configured command in the + * primary sequencer. + */ +void geni_se_cancel_m_cmd(void __iomem *base) +{ + geni_write_reg(M_GENI_CMD_CANCEL, base, SE_GENI_M_CMD_CTRL_REG); +} +EXPORT_SYMBOL(geni_se_cancel_m_cmd); + +/** + * geni_se_cancel_s_cmd() - Cancel the command configured in the secondary + * sequencer + * @base: Base address of the serial engine's register block. + * + * This function is used to cancel the currently configured command in the + * secondary sequencer. + */ +void geni_se_cancel_s_cmd(void __iomem *base) +{ + geni_write_reg(S_GENI_CMD_CANCEL, base, SE_GENI_S_CMD_CTRL_REG); +} +EXPORT_SYMBOL(geni_se_cancel_s_cmd); + +/** + * geni_se_abort_m_cmd() - Abort the command configured in the primary sequencer + * @base: Base address of the serial engine's register block. + * + * This function is used to force abort the currently configured command in the + * primary sequencer. + */ +void geni_se_abort_m_cmd(void __iomem *base) +{ + geni_write_reg(M_GENI_CMD_ABORT, base, SE_GENI_M_CMD_CTRL_REG); +} +EXPORT_SYMBOL(geni_se_abort_m_cmd); + +/** + * geni_se_abort_s_cmd() - Abort the command configured in the secondary + * sequencer + * @base: Base address of the serial engine's register block. + * + * This function is used to force abort the currently configured command in the + * secondary sequencer. + */ +void geni_se_abort_s_cmd(void __iomem *base) +{ + geni_write_reg(S_GENI_CMD_ABORT, base, SE_GENI_S_CMD_CTRL_REG); +} +EXPORT_SYMBOL(geni_se_abort_s_cmd); + +/** + * geni_se_get_tx_fifo_depth() - Get the TX fifo depth of the serial engine + * @base: Base address of the serial engine's register block. + * + * This function is used to get the depth i.e. number of elements in the + * TX fifo of the serial engine. + * + * Return: TX fifo depth in units of FIFO words. + */ +int geni_se_get_tx_fifo_depth(void __iomem *base) +{ + int tx_fifo_depth; + + tx_fifo_depth = ((geni_read_reg(base, SE_HW_PARAM_0) + & TX_FIFO_DEPTH_MSK) >> TX_FIFO_DEPTH_SHFT); + return tx_fifo_depth; +} +EXPORT_SYMBOL(geni_se_get_tx_fifo_depth); + +/** + * geni_se_get_tx_fifo_width() - Get the TX fifo width of the serial engine + * @base: Base address of the serial engine's register block. + * + * This function is used to get the width i.e. word size per element in the + * TX fifo of the serial engine. + * + * Return: TX fifo width in bits + */ +int geni_se_get_tx_fifo_width(void __iomem *base) +{ + int tx_fifo_width; + + tx_fifo_width = ((geni_read_reg(base, SE_HW_PARAM_0) + & TX_FIFO_WIDTH_MSK) >> TX_FIFO_WIDTH_SHFT); + return tx_fifo_width; +} +EXPORT_SYMBOL(geni_se_get_tx_fifo_width); + +/** + * geni_se_get_rx_fifo_depth() - Get the RX fifo depth of the serial engine + * @base: Base address of the serial engine's register block. + * + * This function is used to get the depth i.e. number of elements in the + * RX fifo of the serial engine. + * + * Return: RX fifo depth in units of FIFO words + */ +int geni_se_get_rx_fifo_depth(void __iomem *base) +{ + int rx_fifo_depth; + + rx_fifo_depth = ((geni_read_reg(base, SE_HW_PARAM_1) + & RX_FIFO_DEPTH_MSK) >> RX_FIFO_DEPTH_SHFT); + return rx_fifo_depth; +} +EXPORT_SYMBOL(geni_se_get_rx_fifo_depth); + +/** + * geni_se_get_packing_config() - Get the packing configuration based on input + * @bpw: Bits of data per transfer word. + * @pack_words: Number of words per fifo element. + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa. + * @cfg0: Output buffer to hold the first half of configuration. + * @cfg1: Output buffer to hold the second half of configuration. + * + * This function is used to calculate the packing configuration based on + * the input packing requirement and the configuration logic. + */ +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; + 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)); + 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); +} +EXPORT_SYMBOL(geni_se_get_packing_config); + +/** + * geni_se_config_packing() - Packing configuration of the serial engine + * @base: Base address of the serial engine's register block. + * @bpw: Bits of data per transfer word. + * @pack_words: Number of words per fifo element. + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa. + * + * This function is used to configure the packing rules for the current + * transfer. + */ +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); +} +EXPORT_SYMBOL(geni_se_config_packing); + +static void geni_se_clks_off(struct geni_se_rsc *rsc) +{ + struct geni_se_device *geni_se_dev; + + if (unlikely(!rsc || !rsc->wrapper_dev)) + return; + + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); + if (unlikely(!geni_se_dev)) + return; + + clk_disable_unprepare(rsc->se_clk); + clk_disable_unprepare(geni_se_dev->s_ahb_clk); + clk_disable_unprepare(geni_se_dev->m_ahb_clk); +} + +/** + * geni_se_resources_off() - Turn off resources associated with the serial + * engine + * @rsc: Handle to resources associated with the serial engine. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_se_resources_off(struct geni_se_rsc *rsc) +{ + int ret = 0; + struct geni_se_device *geni_se_dev; + + if (unlikely(!rsc || !rsc->wrapper_dev)) + return -EINVAL; + + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); + if (unlikely(!geni_se_dev)) + return -ENODEV; + + ret = pinctrl_select_state(rsc->geni_pinctrl, rsc->geni_gpio_sleep); + if (ret) + return ret; + + geni_se_clks_off(rsc); + return 0; +} +EXPORT_SYMBOL(geni_se_resources_off); + +static int geni_se_clks_on(struct geni_se_rsc *rsc) +{ + int ret; + struct geni_se_device *geni_se_dev; + + if (unlikely(!rsc || !rsc->wrapper_dev)) + return -EINVAL; + + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); + if (unlikely(!geni_se_dev)) + return -EPROBE_DEFER; + + ret = clk_prepare_enable(geni_se_dev->m_ahb_clk); + if (ret) + return ret; + + ret = clk_prepare_enable(geni_se_dev->s_ahb_clk); + if (ret) { + clk_disable_unprepare(geni_se_dev->m_ahb_clk); + return ret; + } + + ret = clk_prepare_enable(rsc->se_clk); + if (ret) { + clk_disable_unprepare(geni_se_dev->s_ahb_clk); + clk_disable_unprepare(geni_se_dev->m_ahb_clk); + } + return ret; +} + +/** + * geni_se_resources_on() - Turn on resources associated with the serial + * engine + * @rsc: Handle to resources associated with the serial engine. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_se_resources_on(struct geni_se_rsc *rsc) +{ + int ret = 0; + struct geni_se_device *geni_se_dev; + + if (unlikely(!rsc || !rsc->wrapper_dev)) + return -EINVAL; + + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); + if (unlikely(!geni_se_dev)) + return -EPROBE_DEFER; + + ret = geni_se_clks_on(rsc); + if (ret) + return ret; + + ret = pinctrl_select_state(rsc->geni_pinctrl, rsc->geni_gpio_active); + if (ret) + geni_se_clks_off(rsc); + + return ret; +} +EXPORT_SYMBOL(geni_se_resources_on); + +/** + * geni_se_clk_tbl_get() - Get the clock table to program DFS + * @rsc: Resource for which the clock table is requested. + * @tbl: Table in which the output is returned. + * + * This function is called by the protocol drivers to determine the different + * clock frequencies supported by Serail Engine Core Clock. The protocol + * drivers use the output to determine the clock frequency index to be + * programmed into DFS. + * + * Return: number of valid performance levels in the table on success, + * standard Linux error codes on failure. + */ +int geni_se_clk_tbl_get(struct geni_se_rsc *rsc, unsigned long **tbl) +{ + struct geni_se_device *geni_se_dev; + int i; + unsigned long prev_freq = 0; + int ret = 0; + + if (unlikely(!rsc || !rsc->wrapper_dev || !rsc->se_clk || !tbl)) + return -EINVAL; + + *tbl = NULL; + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev); + if (unlikely(!geni_se_dev)) + return -EPROBE_DEFER; + + mutex_lock(&geni_se_dev->geni_dev_lock); + if (geni_se_dev->clk_perf_tbl) { + *tbl = geni_se_dev->clk_perf_tbl; + ret = geni_se_dev->num_clk_levels; + goto exit_se_clk_tbl_get; + } + + geni_se_dev->clk_perf_tbl = kzalloc(sizeof(*geni_se_dev->clk_perf_tbl) * + MAX_CLK_PERF_LEVEL, GFP_KERNEL); + if (!geni_se_dev->clk_perf_tbl) { + ret = -ENOMEM; + goto exit_se_clk_tbl_get; + } + + for (i = 0; i < MAX_CLK_PERF_LEVEL; i++) { + geni_se_dev->clk_perf_tbl[i] = clk_round_rate(rsc->se_clk, + prev_freq + 1); + if (geni_se_dev->clk_perf_tbl[i] == prev_freq) { + geni_se_dev->clk_perf_tbl[i] = 0; + break; + } + prev_freq = geni_se_dev->clk_perf_tbl[i]; + } + geni_se_dev->num_clk_levels = i; + *tbl = geni_se_dev->clk_perf_tbl; + ret = geni_se_dev->num_clk_levels; +exit_se_clk_tbl_get: + mutex_unlock(&geni_se_dev->geni_dev_lock); + return ret; +} +EXPORT_SYMBOL(geni_se_clk_tbl_get); + +/** + * geni_se_clk_freq_match() - Get the matching or closest SE clock frequency + * @rsc: Resource for which the clock frequency is requested. + * @req_freq: Requested clock frequency. + * @index: Index of the resultant frequency in the table. + * @res_freq: Resultant frequency which matches or is closer to the + * requested frequency. + * @exact: Flag to indicate exact multiple requirement of the requested + * frequency . + * + * This function is called by the protocol drivers to determine the matching + * or closest frequency of the Serial Engine clock to be selected in order + * to meet the performance requirements. + * + * Return: 0 on success, standard Linux error codes on failure. + */ +int geni_se_clk_freq_match(struct geni_se_rsc *rsc, unsigned long req_freq, + unsigned int *index, unsigned long *res_freq, + bool exact) +{ + unsigned long *tbl; + int num_clk_levels; + int i; + + num_clk_levels = geni_se_clk_tbl_get(rsc, &tbl); + if (num_clk_levels < 0) + return num_clk_levels; + + if (num_clk_levels == 0) + return -EFAULT; + + *res_freq = 0; + for (i = 0; i < num_clk_levels; i++) { + if (!(tbl[i] % req_freq)) { + *index = i; + *res_freq = tbl[i]; + return 0; + } + + if (!(*res_freq) || ((tbl[i] > *res_freq) && + (tbl[i] < req_freq))) { + *index = i; + *res_freq = tbl[i]; + } + } + + if (exact || !(*res_freq)) + return -ENOKEY; + + return 0; +} +EXPORT_SYMBOL(geni_se_clk_freq_match); + +/** + * 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); + geni_write_reg(tx_len, base, SE_DMA_TX_LEN); + return 0; +} +EXPORT_SYMBOL(geni_se_tx_dma_prep); + +/** + * geni_se_rx_dma_prep() - Prepare the Serial Engine for RX DMA transfer + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped. + * @base: Base address of the SE register block. + * @rx_buf: Pointer to the RX buffer. + * @rx_len: Length of the RX buffer. + * @rx_dma: Pointer to store the mapped DMA address. + * + * This function is used to prepare the buffers for DMA RX. + * + * Return: 0 on success, standard Linux error codes on error/failure. + */ +int geni_se_rx_dma_prep(struct device *wrapper_dev, void __iomem *base, + void *rx_buf, int rx_len, dma_addr_t *rx_dma) +{ + int ret; + + if (unlikely(!wrapper_dev || !base || !rx_buf || !rx_len || !rx_dma)) + return -EINVAL; + + ret = geni_se_map_buf(wrapper_dev, rx_dma, rx_buf, rx_len, + DMA_FROM_DEVICE); + if (ret) + return ret; + + geni_write_reg(7, base, SE_DMA_RX_IRQ_EN_SET); + geni_write_reg((u32)(*rx_dma), base, SE_DMA_RX_PTR_L); + geni_write_reg((u32)((*rx_dma) >> 32), base, SE_DMA_RX_PTR_H); + /* RX does not have EOT bit */ + geni_write_reg(0, base, SE_DMA_RX_ATTR); + geni_write_reg(rx_len, base, SE_DMA_RX_LEN); + return 0; +} +EXPORT_SYMBOL(geni_se_rx_dma_prep); + +/** + * geni_se_tx_dma_unprep() - Unprepare the Serial Engine after TX DMA transfer + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped. + * @tx_dma: DMA address of the TX buffer. + * @tx_len: Length of the TX buffer. + * + * This function is used to unprepare the DMA buffers after DMA TX. + */ +void geni_se_tx_dma_unprep(struct device *wrapper_dev, + dma_addr_t tx_dma, int tx_len) +{ + if (tx_dma) + geni_se_unmap_buf(wrapper_dev, &tx_dma, tx_len, + DMA_TO_DEVICE); +} +EXPORT_SYMBOL(geni_se_tx_dma_unprep); + +/** + * geni_se_rx_dma_unprep() - Unprepare the Serial Engine after RX DMA transfer + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped. + * @rx_dma: DMA address of the RX buffer. + * @rx_len: Length of the RX buffer. + * + * This function is used to unprepare the DMA buffers after DMA RX. + */ +void geni_se_rx_dma_unprep(struct device *wrapper_dev, + dma_addr_t rx_dma, int rx_len) +{ + if (rx_dma) + geni_se_unmap_buf(wrapper_dev, &rx_dma, rx_len, + DMA_FROM_DEVICE); +} +EXPORT_SYMBOL(geni_se_rx_dma_unprep); + +/** + * geni_se_map_buf() - Map a single buffer into QUP wrapper device + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. + * @iova: Pointer in which the mapped virtual address is stored. + * @buf: Address of the buffer that needs to be mapped. + * @size: Size of the buffer. + * @dir: Direction of the DMA transfer. + * + * This function is used to map an already allocated buffer into the + * QUP device space. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_se_map_buf(struct device *wrapper_dev, dma_addr_t *iova, + void *buf, size_t size, enum dma_data_direction dir) +{ + struct device *dev_p; + struct geni_se_device *geni_se_dev; + + if (!wrapper_dev || !iova || !buf || !size) + return -EINVAL; + + geni_se_dev = dev_get_drvdata(wrapper_dev); + if (!geni_se_dev || !geni_se_dev->dev) + return -ENODEV; + + dev_p = geni_se_dev->dev; + + *iova = dma_map_single(dev_p, buf, size, dir); + if (dma_mapping_error(dev_p, *iova)) + return -EIO; + return 0; +} +EXPORT_SYMBOL(geni_se_map_buf); + +/** + * geni_se_unmap_buf() - Unmap a single buffer from QUP wrapper device + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. + * @iova: Pointer in which the mapped virtual address is stored. + * @size: Size of the buffer. + * @dir: Direction of the DMA transfer. + * + * This function is used to unmap an already mapped buffer from the + * QUP device space. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_se_unmap_buf(struct device *wrapper_dev, dma_addr_t *iova, + size_t size, enum dma_data_direction dir) +{ + struct device *dev_p; + struct geni_se_device *geni_se_dev; + + if (!wrapper_dev || !iova || !size) + return -EINVAL; + + geni_se_dev = dev_get_drvdata(wrapper_dev); + if (!geni_se_dev || !geni_se_dev->dev) + return -ENODEV; + + dev_p = geni_se_dev->dev; + dma_unmap_single(dev_p, *iova, size, dir); + return 0; +} +EXPORT_SYMBOL(geni_se_unmap_buf); + +static const struct of_device_id geni_se_dt_match[] = { + { .compatible = "qcom,geni-se-qup", }, + {} +}; + +static int geni_se_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct resource *res; + struct geni_se_device *geni_se_dev; + int ret = 0; + + geni_se_dev = devm_kzalloc(dev, sizeof(*geni_se_dev), GFP_KERNEL); + if (!geni_se_dev) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_err(dev, "%s: Mandatory resource info not found\n", + __func__); + devm_kfree(dev, geni_se_dev); + return -EINVAL; + } + + geni_se_dev->base = devm_ioremap_resource(dev, res); + if (IS_ERR_OR_NULL(geni_se_dev->base)) { + dev_err(dev, "%s: Error mapping the resource\n", __func__); + devm_kfree(dev, geni_se_dev); + return -EFAULT; + } + + geni_se_dev->m_ahb_clk = devm_clk_get(dev, "m-ahb"); + if (IS_ERR(geni_se_dev->m_ahb_clk)) { + ret = PTR_ERR(geni_se_dev->m_ahb_clk); + dev_err(dev, "Err getting M AHB clk %d\n", ret); + devm_iounmap(dev, geni_se_dev->base); + devm_kfree(dev, geni_se_dev); + return ret; + } + + geni_se_dev->s_ahb_clk = devm_clk_get(dev, "s-ahb"); + if (IS_ERR(geni_se_dev->s_ahb_clk)) { + ret = PTR_ERR(geni_se_dev->s_ahb_clk); + dev_err(dev, "Err getting S AHB clk %d\n", ret); + devm_clk_put(dev, geni_se_dev->m_ahb_clk); + devm_iounmap(dev, geni_se_dev->base); + devm_kfree(dev, geni_se_dev); + return ret; + } + + geni_se_dev->dev = dev; + mutex_init(&geni_se_dev->geni_dev_lock); + dev_set_drvdata(dev, geni_se_dev); + dev_dbg(dev, "GENI SE Driver probed\n"); + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); +} + +static int geni_se_remove(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct geni_se_device *geni_se_dev = dev_get_drvdata(dev); + + devm_clk_put(dev, geni_se_dev->s_ahb_clk); + devm_clk_put(dev, geni_se_dev->m_ahb_clk); + devm_iounmap(dev, geni_se_dev->base); + devm_kfree(dev, geni_se_dev); + return 0; +} + +static struct platform_driver geni_se_driver = { + .driver = { + .name = "geni_se_qup", + .of_match_table = geni_se_dt_match, + }, + .probe = geni_se_probe, + .remove = geni_se_remove, +}; + +static int __init geni_se_driver_init(void) +{ + return platform_driver_register(&geni_se_driver); +} +arch_initcall(geni_se_driver_init); + +static void __exit geni_se_driver_exit(void) +{ + platform_driver_unregister(&geni_se_driver); +} +module_exit(geni_se_driver_exit); + +MODULE_DESCRIPTION("GENI Serial Engine Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h new file mode 100644 index 0000000..5695da9 --- /dev/null +++ b/include/linux/qcom-geni-se.h @@ -0,0 +1,807 @@ +/* + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#ifndef _LINUX_QCOM_GENI_SE +#define _LINUX_QCOM_GENI_SE +#include <linux/clk.h> +#include <linux/dma-direction.h> +#include <linux/io.h> +#include <linux/list.h> + +/* Transfer mode supported by GENI Serial Engines */ +enum geni_se_xfer_mode { + INVALID, + FIFO_MODE, + SE_DMA, +}; + +/* Protocols supported by GENI Serial Engines */ +enum geni_se_protocol_types { + NONE, + SPI, + UART, + I2C, + I3C +}; + +/** + * struct geni_se_rsc - GENI Serial Engine Resource + * @wrapper_dev: Pointer to the parent QUP Wrapper core. + * @se_clk: Handle to the core serial engine clock. + * @geni_pinctrl: Handle to the pinctrl configuration. + * @geni_gpio_active: Handle to the default/active pinctrl state. + * @geni_gpi_sleep: Handle to the sleep pinctrl state. + */ +struct geni_se_rsc { + struct device *wrapper_dev; + struct clk *se_clk; + struct pinctrl *geni_pinctrl; + struct pinctrl_state *geni_gpio_active; + struct pinctrl_state *geni_gpio_sleep; +}; + +#define PINCTRL_DEFAULT "default" +#define PINCTRL_SLEEP "sleep" + +/* Common SE registers */ +#define GENI_INIT_CFG_REVISION (0x0) +#define GENI_S_INIT_CFG_REVISION (0x4) +#define GENI_FORCE_DEFAULT_REG (0x20) +#define GENI_OUTPUT_CTRL (0x24) +#define GENI_CGC_CTRL (0x28) +#define SE_GENI_STATUS (0x40) +#define GENI_SER_M_CLK_CFG (0x48) +#define GENI_SER_S_CLK_CFG (0x4C) +#define GENI_CLK_CTRL_RO (0x60) +#define GENI_IF_DISABLE_RO (0x64) +#define GENI_FW_REVISION_RO (0x68) +#define GENI_FW_S_REVISION_RO (0x6C) +#define SE_GENI_CLK_SEL (0x7C) +#define SE_GENI_BYTE_GRAN (0x254) +#define SE_GENI_DMA_MODE_EN (0x258) +#define SE_GENI_TX_PACKING_CFG0 (0x260) +#define SE_GENI_TX_PACKING_CFG1 (0x264) +#define SE_GENI_RX_PACKING_CFG0 (0x284) +#define SE_GENI_RX_PACKING_CFG1 (0x288) +#define SE_GENI_M_CMD0 (0x600) +#define SE_GENI_M_CMD_CTRL_REG (0x604) +#define SE_GENI_M_IRQ_STATUS (0x610) +#define SE_GENI_M_IRQ_EN (0x614) +#define SE_GENI_M_IRQ_CLEAR (0x618) +#define SE_GENI_S_CMD0 (0x630) +#define SE_GENI_S_CMD_CTRL_REG (0x634) +#define SE_GENI_S_IRQ_STATUS (0x640) +#define SE_GENI_S_IRQ_EN (0x644) +#define SE_GENI_S_IRQ_CLEAR (0x648) +#define SE_GENI_TX_FIFOn (0x700) +#define SE_GENI_RX_FIFOn (0x780) +#define SE_GENI_TX_FIFO_STATUS (0x800) +#define SE_GENI_RX_FIFO_STATUS (0x804) +#define SE_GENI_TX_WATERMARK_REG (0x80C) +#define SE_GENI_RX_WATERMARK_REG (0x810) +#define SE_GENI_RX_RFR_WATERMARK_REG (0x814) +#define SE_GENI_IOS (0x908) +#define SE_GENI_M_GP_LENGTH (0x910) +#define SE_GENI_S_GP_LENGTH (0x914) +#define SE_GSI_EVENT_EN (0xE18) +#define SE_IRQ_EN (0xE1C) +#define SE_HW_PARAM_0 (0xE24) +#define SE_HW_PARAM_1 (0xE28) +#define SE_DMA_GENERAL_CFG (0xE30) + +/* GENI_OUTPUT_CTRL fields */ +#define DEFAULT_IO_OUTPUT_CTRL_MSK (GENMASK(6, 0)) + +/* GENI_FORCE_DEFAULT_REG fields */ +#define FORCE_DEFAULT (BIT(0)) + +/* GENI_CGC_CTRL fields */ +#define CFG_AHB_CLK_CGC_ON (BIT(0)) +#define CFG_AHB_WR_ACLK_CGC_ON (BIT(1)) +#define DATA_AHB_CLK_CGC_ON (BIT(2)) +#define SCLK_CGC_ON (BIT(3)) +#define TX_CLK_CGC_ON (BIT(4)) +#define RX_CLK_CGC_ON (BIT(5)) +#define EXT_CLK_CGC_ON (BIT(6)) +#define PROG_RAM_HCLK_OFF (BIT(8)) +#define PROG_RAM_SCLK_OFF (BIT(9)) +#define DEFAULT_CGC_EN (GENMASK(6, 0)) + +/* GENI_STATUS fields */ +#define M_GENI_CMD_ACTIVE (BIT(0)) +#define S_GENI_CMD_ACTIVE (BIT(12)) + +/* GENI_SER_M_CLK_CFG/GENI_SER_S_CLK_CFG */ +#define SER_CLK_EN (BIT(0)) +#define CLK_DIV_MSK (GENMASK(15, 4)) +#define CLK_DIV_SHFT (4) + +/* CLK_CTRL_RO fields */ + +/* IF_DISABLE_RO fields */ + +/* FW_REVISION_RO fields */ +#define FW_REV_PROTOCOL_MSK (GENMASK(15, 8)) +#define FW_REV_PROTOCOL_SHFT (8) + +/* GENI_CLK_SEL fields */ +#define CLK_SEL_MSK (GENMASK(2, 0)) + +/* SE_GENI_DMA_MODE_EN */ +#define GENI_DMA_MODE_EN (BIT(0)) + +/* GENI_M_CMD0 fields */ +#define M_OPCODE_MSK (GENMASK(31, 27)) +#define M_OPCODE_SHFT (27) +#define M_PARAMS_MSK (GENMASK(26, 0)) + +/* GENI_M_CMD_CTRL_REG */ +#define M_GENI_CMD_CANCEL BIT(2) +#define M_GENI_CMD_ABORT BIT(1) +#define M_GENI_DISABLE BIT(0) + +/* GENI_S_CMD0 fields */ +#define S_OPCODE_MSK (GENMASK(31, 27)) +#define S_OPCODE_SHFT (27) +#define S_PARAMS_MSK (GENMASK(26, 0)) + +/* GENI_S_CMD_CTRL_REG */ +#define S_GENI_CMD_CANCEL (BIT(2)) +#define S_GENI_CMD_ABORT (BIT(1)) +#define S_GENI_DISABLE (BIT(0)) + +/* GENI_M_IRQ_EN fields */ +#define M_CMD_DONE_EN (BIT(0)) +#define M_CMD_OVERRUN_EN (BIT(1)) +#define M_ILLEGAL_CMD_EN (BIT(2)) +#define M_CMD_FAILURE_EN (BIT(3)) +#define M_CMD_CANCEL_EN (BIT(4)) +#define M_CMD_ABORT_EN (BIT(5)) +#define M_TIMESTAMP_EN (BIT(6)) +#define M_RX_IRQ_EN (BIT(7)) +#define M_GP_SYNC_IRQ_0_EN (BIT(8)) +#define M_GP_IRQ_0_EN (BIT(9)) +#define M_GP_IRQ_1_EN (BIT(10)) +#define M_GP_IRQ_2_EN (BIT(11)) +#define M_GP_IRQ_3_EN (BIT(12)) +#define M_GP_IRQ_4_EN (BIT(13)) +#define M_GP_IRQ_5_EN (BIT(14)) +#define M_IO_DATA_DEASSERT_EN (BIT(22)) +#define M_IO_DATA_ASSERT_EN (BIT(23)) +#define M_RX_FIFO_RD_ERR_EN (BIT(24)) +#define M_RX_FIFO_WR_ERR_EN (BIT(25)) +#define M_RX_FIFO_WATERMARK_EN (BIT(26)) +#define M_RX_FIFO_LAST_EN (BIT(27)) +#define M_TX_FIFO_RD_ERR_EN (BIT(28)) +#define M_TX_FIFO_WR_ERR_EN (BIT(29)) +#define M_TX_FIFO_WATERMARK_EN (BIT(30)) +#define M_SEC_IRQ_EN (BIT(31)) +#define M_COMMON_GENI_M_IRQ_EN (GENMASK(6, 1) | \ + M_IO_DATA_DEASSERT_EN | \ + M_IO_DATA_ASSERT_EN | M_RX_FIFO_RD_ERR_EN | \ + M_RX_FIFO_WR_ERR_EN | M_TX_FIFO_RD_ERR_EN | \ + M_TX_FIFO_WR_ERR_EN) + +/* GENI_S_IRQ_EN fields */ +#define S_CMD_DONE_EN (BIT(0)) +#define S_CMD_OVERRUN_EN (BIT(1)) +#define S_ILLEGAL_CMD_EN (BIT(2)) +#define S_CMD_FAILURE_EN (BIT(3)) +#define S_CMD_CANCEL_EN (BIT(4)) +#define S_CMD_ABORT_EN (BIT(5)) +#define S_GP_SYNC_IRQ_0_EN (BIT(8)) +#define S_GP_IRQ_0_EN (BIT(9)) +#define S_GP_IRQ_1_EN (BIT(10)) +#define S_GP_IRQ_2_EN (BIT(11)) +#define S_GP_IRQ_3_EN (BIT(12)) +#define S_GP_IRQ_4_EN (BIT(13)) +#define S_GP_IRQ_5_EN (BIT(14)) +#define S_IO_DATA_DEASSERT_EN (BIT(22)) +#define S_IO_DATA_ASSERT_EN (BIT(23)) +#define S_RX_FIFO_RD_ERR_EN (BIT(24)) +#define S_RX_FIFO_WR_ERR_EN (BIT(25)) +#define S_RX_FIFO_WATERMARK_EN (BIT(26)) +#define S_RX_FIFO_LAST_EN (BIT(27)) +#define S_COMMON_GENI_S_IRQ_EN (GENMASK(5, 1) | GENMASK(13, 9) | \ + S_RX_FIFO_RD_ERR_EN | S_RX_FIFO_WR_ERR_EN) + +/* GENI_/TX/RX/RX_RFR/_WATERMARK_REG fields */ +#define WATERMARK_MSK (GENMASK(5, 0)) + +/* GENI_TX_FIFO_STATUS fields */ +#define TX_FIFO_WC (GENMASK(27, 0)) + +/* GENI_RX_FIFO_STATUS fields */ +#define RX_LAST (BIT(31)) +#define RX_LAST_BYTE_VALID_MSK (GENMASK(30, 28)) +#define RX_LAST_BYTE_VALID_SHFT (28) +#define RX_FIFO_WC_MSK (GENMASK(24, 0)) + +/* SE_GSI_EVENT_EN fields */ +#define DMA_RX_EVENT_EN (BIT(0)) +#define DMA_TX_EVENT_EN (BIT(1)) +#define GENI_M_EVENT_EN (BIT(2)) +#define GENI_S_EVENT_EN (BIT(3)) + +/* SE_GENI_IOS fields */ +#define IO2_DATA_IN (BIT(1)) +#define RX_DATA_IN (BIT(0)) + +/* SE_IRQ_EN fields */ +#define DMA_RX_IRQ_EN (BIT(0)) +#define DMA_TX_IRQ_EN (BIT(1)) +#define GENI_M_IRQ_EN (BIT(2)) +#define GENI_S_IRQ_EN (BIT(3)) + +/* SE_HW_PARAM_0 fields */ +#define TX_FIFO_WIDTH_MSK (GENMASK(29, 24)) +#define TX_FIFO_WIDTH_SHFT (24) +#define TX_FIFO_DEPTH_MSK (GENMASK(21, 16)) +#define TX_FIFO_DEPTH_SHFT (16) + +/* SE_HW_PARAM_1 fields */ +#define RX_FIFO_WIDTH_MSK (GENMASK(29, 24)) +#define RX_FIFO_WIDTH_SHFT (24) +#define RX_FIFO_DEPTH_MSK (GENMASK(21, 16)) +#define RX_FIFO_DEPTH_SHFT (16) + +/* SE_DMA_GENERAL_CFG */ +#define DMA_RX_CLK_CGC_ON (BIT(0)) +#define DMA_TX_CLK_CGC_ON (BIT(1)) +#define DMA_AHB_SLV_CFG_ON (BIT(2)) +#define AHB_SEC_SLV_CLK_CGC_ON (BIT(3)) +#define DUMMY_RX_NON_BUFFERABLE (BIT(4)) +#define RX_DMA_ZERO_PADDING_EN (BIT(5)) +#define RX_DMA_IRQ_DELAY_MSK (GENMASK(8, 6)) +#define RX_DMA_IRQ_DELAY_SHFT (6) + +#define SE_DMA_TX_PTR_L (0xC30) +#define SE_DMA_TX_PTR_H (0xC34) +#define SE_DMA_TX_ATTR (0xC38) +#define SE_DMA_TX_LEN (0xC3C) +#define SE_DMA_TX_IRQ_STAT (0xC40) +#define SE_DMA_TX_IRQ_CLR (0xC44) +#define SE_DMA_TX_IRQ_EN (0xC48) +#define SE_DMA_TX_IRQ_EN_SET (0xC4C) +#define SE_DMA_TX_IRQ_EN_CLR (0xC50) +#define SE_DMA_TX_LEN_IN (0xC54) +#define SE_DMA_TX_FSM_RST (0xC58) +#define SE_DMA_TX_MAX_BURST (0xC5C) + +#define SE_DMA_RX_PTR_L (0xD30) +#define SE_DMA_RX_PTR_H (0xD34) +#define SE_DMA_RX_ATTR (0xD38) +#define SE_DMA_RX_LEN (0xD3C) +#define SE_DMA_RX_IRQ_STAT (0xD40) +#define SE_DMA_RX_IRQ_CLR (0xD44) +#define SE_DMA_RX_IRQ_EN (0xD48) +#define SE_DMA_RX_IRQ_EN_SET (0xD4C) +#define SE_DMA_RX_IRQ_EN_CLR (0xD50) +#define SE_DMA_RX_LEN_IN (0xD54) +#define SE_DMA_RX_FSM_RST (0xD58) +#define SE_DMA_RX_MAX_BURST (0xD5C) +#define SE_DMA_RX_FLUSH (0xD60) + +/* SE_DMA_TX_IRQ_STAT Register fields */ +#define TX_DMA_DONE (BIT(0)) +#define TX_EOT (BIT(1)) +#define TX_SBE (BIT(2)) +#define TX_RESET_DONE (BIT(3)) + +/* SE_DMA_RX_IRQ_STAT Register fields */ +#define RX_DMA_DONE (BIT(0)) +#define RX_EOT (BIT(1)) +#define RX_SBE (BIT(2)) +#define RX_RESET_DONE (BIT(3)) +#define RX_FLUSH_DONE (BIT(4)) +#define RX_GENI_GP_IRQ (GENMASK(10, 5)) +#define RX_GENI_CANCEL_IRQ (BIT(11)) +#define RX_GENI_GP_IRQ_EXT (GENMASK(13, 12)) + +#ifdef CONFIG_QCOM_GENI_SE +/** + * geni_read_reg_nolog() - Helper function to read from a GENI register + * @base: Base address of the serial engine's register block. + * @offset: Offset within the serial engine's register block. + * + * Return: Return the contents of the register. + */ +unsigned int geni_read_reg_nolog(void __iomem *base, int offset); + +/** + * geni_write_reg_nolog() - Helper function to write into a GENI register + * @value: Value to be written into the register. + * @base: Base address of the serial engine's register block. + * @offset: Offset within the serial engine's register block. + */ +void geni_write_reg_nolog(unsigned int value, void __iomem *base, int offset); + +/** + * geni_read_reg() - Helper function to read from a GENI register + * @base: Base address of the serial engine's register block. + * @offset: Offset within the serial engine's register block. + * + * Return: Return the contents of the register. + */ +unsigned int geni_read_reg(void __iomem *base, int offset); + +/** + * geni_write_reg() - Helper function to write into a GENI register + * @value: Value to be written into the register. + * @base: Base address of the serial engine's register block. + * @offset: Offset within the serial engine's register block. + */ +void geni_write_reg(unsigned int value, void __iomem *base, int offset); + +/** + * geni_get_qup_hw_version() - Read the QUP Wrapper Hardware version + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. + * @major: Buffer for Major Version field. + * @minor: Buffer for Minor Version field. + * @step: Buffer for Step Version field. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_get_qup_hw_version(struct device *wrapper_dev, unsigned int *major, + unsigned int *minor, unsigned int *step); + +/** + * geni_se_get_proto() - Read the protocol configured for a serial engine + * @base: Base address of the serial engine's register block. + * + * Return: Protocol value as configured in the serial engine. + */ +int geni_se_get_proto(void __iomem *base); + +/** + * geni_se_init() - Initialize the GENI Serial Engine + * @base: Base address of the serial engine's register block. + * @rx_wm: Receive watermark to be configured. + * @rx_rfr_wm: Ready-for-receive watermark to be configured. + * + * This function is used to initialize the GENI serial engine, configure + * the transfer mode, receive watermark and ready-for-receive watermarks. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_se_init(void __iomem *base, unsigned int rx_wm, unsigned int rx_rfr); + +/** + * geni_se_select_mode() - Select the serial engine transfer mode + * @base: Base address of the serial engine's register block. + * @mode: Transfer mode to be selected. + * + * Return: 0 on success, standard Linux error codes on failure. + */ +int geni_se_select_mode(void __iomem *base, int mode); + +/** + * 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); + +/** + * geni_se_cancel_m_cmd() - Cancel the command configured in the primary + * sequencer + * @base: Base address of the serial engine's register block. + * + * This function is used to cancel the currently configured command in the + * primary sequencer. + */ +void geni_se_cancel_m_cmd(void __iomem *base); + +/** + * geni_se_cancel_s_cmd() - Cancel the command configured in the secondary + * sequencer + * @base: Base address of the serial engine's register block. + * + * This function is used to cancel the currently configured command in the + * secondary sequencer. + */ +void geni_se_cancel_s_cmd(void __iomem *base); + +/** + * geni_se_abort_m_cmd() - Abort the command configured in the primary sequencer + * @base: Base address of the serial engine's register block. + * + * This function is used to force abort the currently configured command in the + * primary sequencer. + */ +void geni_se_abort_m_cmd(void __iomem *base); + +/** + * geni_se_abort_s_cmd() - Abort the command configured in the secondary + * sequencer + * @base: Base address of the serial engine's register block. + * + * This function is used to force abort the currently configured command in the + * secondary sequencer. + */ +void geni_se_abort_s_cmd(void __iomem *base); + +/** + * geni_se_get_tx_fifo_depth() - Get the TX fifo depth of the serial engine + * @base: Base address of the serial engine's register block. + * + * This function is used to get the depth i.e. number of elements in the + * TX fifo of the serial engine. + * + * Return: TX fifo depth in units of FIFO words. + */ +int geni_se_get_tx_fifo_depth(void __iomem *base); + +/** + * geni_se_get_tx_fifo_width() - Get the TX fifo width of the serial engine + * @base: Base address of the serial engine's register block. + * + * This function is used to get the width i.e. word size per element in the + * TX fifo of the serial engine. + * + * Return: TX fifo width in bits. + */ +int geni_se_get_tx_fifo_width(void __iomem *base); + +/** + * geni_se_get_rx_fifo_depth() - Get the RX fifo depth of the serial engine + * @base: Base address of the serial engine's register block. + * + * This function is used to get the depth i.e. number of elements in the + * RX fifo of the serial engine. + * + * Return: RX fifo depth in units of FIFO words. + */ +int geni_se_get_rx_fifo_depth(void __iomem *base); + +/** + * geni_se_get_packing_config() - Get the packing configuration based on input + * @bpw: Bits of data per transfer word. + * @pack_words: Number of words per fifo element. + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa. + * @cfg0: Output buffer to hold the first half of configuration. + * @cfg1: Output buffer to hold the second half of configuration. + * + * This function is used to calculate the packing configuration based on + * the input packing requirement and the configuration logic. + */ +void geni_se_get_packing_config(int bpw, int pack_words, bool msb_to_lsb, + unsigned long *cfg0, unsigned long *cfg1); + +/** + * geni_se_config_packing() - Packing configuration of the serial engine + * @base: Base address of the serial engine's register block. + * @bpw: Bits of data per transfer word. + * @pack_words: Number of words per fifo element. + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa. + * + * This function is used to configure the packing rules for the current + * transfer. + */ +void geni_se_config_packing(void __iomem *base, int bpw, int pack_words, + bool msb_to_lsb); + +/** + * geni_se_resources_off() - Turn off resources associated with the serial + * engine + * @rsc: Handle to resources associated with the serial engine. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_se_resources_off(struct geni_se_rsc *rsc); + +/** + * geni_se_resources_on() - Turn on resources associated with the serial + * engine + * @rsc: Handle to resources associated with the serial engine. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_se_resources_on(struct geni_se_rsc *rsc); + +/** + * geni_se_clk_tbl_get() - Get the clock table to program DFS + * @rsc: Resource for which the clock table is requested. + * @tbl: Table in which the output is returned. + * + * This function is called by the protocol drivers to determine the different + * clock frequencies supported by Serail Engine Core Clock. The protocol + * drivers use the output to determine the clock frequency index to be + * programmed into DFS. + * + * Return: number of valid performance levels in the table on success, + * standard Linux error codes on failure. + */ +int geni_se_clk_tbl_get(struct geni_se_rsc *rsc, unsigned long **tbl); + +/** + * geni_se_clk_freq_match() - Get the matching or closest SE clock frequency + * @rsc: Resource for which the clock frequency is requested. + * @req_freq: Requested clock frequency. + * @index: Index of the resultant frequency in the table. + * @res_freq: Resultant frequency which matches or is closer to the + * requested frequency. + * @exact: Flag to indicate exact multiple requirement of the requested + * frequency . + * + * This function is called by the protocol drivers to determine the matching + * or closest frequency of the Serial Engine clock to be selected in order + * to meet the performance requirements. + * + * Return: 0 on success, standard Linux error codes on failure. + */ +int geni_se_clk_freq_match(struct geni_se_rsc *rsc, unsigned long req_freq, + unsigned int *index, unsigned long *res_freq, + bool exact); + +/** + * 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); + +/** + * geni_se_rx_dma_prep() - Prepare the Serial Engine for RX DMA transfer + * @wrapper_dev: QUP Wrapper Device to which the TX buffer is mapped. + * @base: Base address of the SE register block. + * @rx_buf: Pointer to the RX buffer. + * @rx_len: Length of the RX buffer. + * @rx_dma: Pointer to store the mapped DMA address. + * + * This function is used to prepare the buffers for DMA RX. + * + * Return: 0 on success, standard Linux error codes on error/failure. + */ +int geni_se_rx_dma_prep(struct device *wrapper_dev, void __iomem *base, + void *rx_buf, int rx_len, dma_addr_t *rx_dma); + +/** + * geni_se_tx_dma_unprep() - Unprepare the Serial Engine after TX DMA transfer + * @wrapper_dev: QUP Wrapper Device to which the TX buffer is mapped. + * @tx_dma: DMA address of the TX buffer. + * @tx_len: Length of the TX buffer. + * + * This function is used to unprepare the DMA buffers after DMA TX. + */ +void geni_se_tx_dma_unprep(struct device *wrapper_dev, + dma_addr_t tx_dma, int tx_len); + +/** + * geni_se_rx_dma_unprep() - Unprepare the Serial Engine after RX DMA transfer + * @wrapper_dev: QUP Wrapper Device to which the TX buffer is mapped. + * @rx_dma: DMA address of the RX buffer. + * @rx_len: Length of the RX buffer. + * + * This function is used to unprepare the DMA buffers after DMA RX. + */ +void geni_se_rx_dma_unprep(struct device *wrapper_dev, + dma_addr_t rx_dma, int rx_len); + +/** + * geni_se_map_buf() - Map a single buffer into QUP wrapper device + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. + * @iova: Pointer in which the mapped virtual address is stored. + * @buf: Address of the buffer that needs to be mapped. + * @size: Size of the buffer. + * @dir: Direction of the DMA transfer. + * + * This function is used to map an already allocated buffer into the + * QUP device space. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_se_map_buf(struct device *wrapper_dev, dma_addr_t *iova, + void *buf, size_t size, enum dma_data_direction dir); + +/** + * geni_se_unmap_buf() - Unmap a single buffer from QUP wrapper device + * @wrapper_dev: Pointer to the corresponding QUP wrapper core. + * @iova: Pointer in which the mapped virtual address is stored. + * @size: Size of the buffer. + * @dir: Direction of the DMA transfer. + * + * This function is used to unmap an already mapped buffer from the + * QUP device space. + * + * Return: 0 on success, standard Linux error codes on failure/error. + */ +int geni_se_unmap_buf(struct device *wrapper_dev, dma_addr_t *iova, + size_t size, enum dma_data_direction dir); +#else +static inline unsigned int geni_read_reg_nolog(void __iomem *base, int offset) +{ + return 0; +} + +static inline void geni_write_reg_nolog(unsigned int value, + void __iomem *base, int offset) +{ +} + +static inline unsigned int geni_read_reg(void __iomem *base, int offset) +{ + return 0; +} + +static inline void geni_write_reg(unsigned int value, void __iomem *base, + int offset) +{ +} + +static inline int geni_get_qup_hw_version(struct device *wrapper_dev, + unsigned int *major, + unsigned int *minor, + unsigned int *step) +{ + return -ENXIO; +} + +static inline int geni_se_get_proto(void __iomem *base) +{ + return -ENXIO; +} + +static inline int geni_se_init(void __iomem *base, + unsigned int rx_wm, unsigned int rx_rfr) +{ + return -ENXIO; +} + +static inline int geni_se_select_mode(void __iomem *base, int mode) +{ + return -ENXIO; +} + +static inline void geni_se_setup_m_cmd(void __iomem *base, u32 cmd, + u32 params) +{ +} + +static inline void geni_se_setup_s_cmd(void __iomem *base, u32 cmd, + u32 params) +{ +} + +static inline void geni_se_cancel_m_cmd(void __iomem *base) +{ +} + +static inline void geni_se_cancel_s_cmd(void __iomem *base) +{ +} + +static inline void geni_se_abort_m_cmd(void __iomem *base) +{ +} + +static inline void geni_se_abort_s_cmd(void __iomem *base) +{ +} + +static inline int geni_se_get_tx_fifo_depth(void __iomem *base) +{ + return -ENXIO; +} + +static inline int geni_se_get_tx_fifo_width(void __iomem *base) +{ + return -ENXIO; +} + +static inline int geni_se_get_rx_fifo_depth(void __iomem *base) +{ + return -ENXIO; +} + +static inline void geni_se_get_packing_config(int bpw, int pack_words, + bool msb_to_lsb, + unsigned long *cfg0, + unsigned long *cfg1) +{ +} + +static inline void geni_se_config_packing(void __iomem *base, int bpw, + int pack_words, bool msb_to_lsb) +{ +} + +static inline int geni_se_resources_on(struct geni_se_rsc *rsc) +{ + return -ENXIO; +} + +static inline int geni_se_resources_off(struct geni_se_rsc *rsc) +{ + return -ENXIO; +} + +static inline int geni_se_clk_tbl_get(struct geni_se_rsc *rsc, + unsigned long **tbl) +{ + return -ENXIO; +} + +static inline int geni_se_clk_freq_match(struct geni_se_rsc *rsc, + unsigned long req_freq, + unsigned int *index, + unsigned long *res_freq, bool exact) +{ + return -ENXIO; +} + +static inline int geni_se_tx_dma_prep(struct device *wrapper_dev, + void __iomem *base, void *tx_buf, + int tx_len, dma_addr_t *tx_dma) +{ + return -ENXIO; +} + +static inline int geni_se_rx_dma_prep(struct device *wrapper_dev, + void __iomem *base, void *rx_buf, + int rx_len, dma_addr_t *rx_dma) +{ + return -ENXIO; +} + +static inline void geni_se_tx_dma_unprep(struct device *wrapper_dev, + dma_addr_t tx_dma, int tx_len) +{ +} + +static inline void geni_se_rx_dma_unprep(struct device *wrapper_dev, + dma_addr_t rx_dma, int rx_len) +{ +} + +static inline int geni_se_map_buf(struct device *wrapper_dev, + dma_addr_t *iova, void *buf, size_t size, + enum dma_data_direction dir) +{ + return -ENXIO; +} + +static inline int geni_se_unmap_buf(struct device *wrapper_dev, + dma_addr_t *iova, size_t size, + enum dma_data_direction dir) +{ + return -ENXIO; + +} + +#endif +#endif