Quoting Karthik Ramasubramanian (2018-03-02 16:58:23) > > > On 3/2/2018 1:41 PM, Stephen Boyd wrote: > > Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:07) > >> + > >> +/** > >> + * geni_se_get_qup_hw_version() - Read the QUP wrapper Hardware version > >> + * @se: Pointer to the corresponding Serial Engine. > >> + * @major: Buffer for Major Version field. > >> + * @minor: Buffer for Minor Version field. > >> + * @step: Buffer for Step Version field. > >> + */ > >> +void geni_se_get_qup_hw_version(struct geni_se *se, unsigned int *major, > >> + unsigned int *minor, unsigned int *step) > >> +{ > >> + unsigned int version; > >> + struct geni_wrapper *wrapper = se->wrapper; > >> + > >> + version = readl_relaxed(wrapper->base + QUP_HW_VER_REG); > >> + *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; > >> +} > >> +EXPORT_SYMBOL(geni_se_get_qup_hw_version); > > > > Is this used? > SPI controller driver uses this API and it will be uploaded sooner. Ok. Maybe it can also be a macro to get the u32 and then some more macros on top of that to pick out the major/minor/step out of the u32 that you read. > > > >> + > >> +/** > >> + * geni_se_read_proto() - Read the protocol configured for a Serial Engine > >> + * @se: Pointer to the concerned Serial Engine. > >> + * > >> + * Return: Protocol value as configured in the serial engine. > >> + */ > >> +u32 geni_se_read_proto(struct geni_se *se) > >> +{ > >> + u32 val; > >> + > >> + val = readl_relaxed(se->base + GENI_FW_REVISION_RO); > >> + > >> + return (val & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT; > >> +} > >> +EXPORT_SYMBOL(geni_se_read_proto); > > > > Is this API really needed outside of this file? It would seem like the > > drivers that implement the protocol, which are child devices, would only > > use this API to confirm that the protocol chosen is for their particular > > protocol. > No, this API is meant for the protocol drivers to confirm that the > serial engine is programmed with the firmware for the concerned protocol > before using the serial engine. If the check fails, the protocol drivers > stop using the serial engine. Ok maybe we don't really need it then? > >> + * RX fifo of the serial engine. > >> + * > >> + * Return: RX fifo depth in units of FIFO words > >> + */ > >> +u32 geni_se_get_rx_fifo_depth(struct geni_se *se) > >> +{ > >> + u32 val; > >> + > >> + val = readl_relaxed(se->base + SE_HW_PARAM_1); > >> + > >> + return (val & RX_FIFO_DEPTH_MSK) >> RX_FIFO_DEPTH_SHFT; > >> +} > >> +EXPORT_SYMBOL(geni_se_get_rx_fifo_depth); > > > > These ones too, can probably just be static inline. > Ok. Just for my knowledge - is there any reference guideline regarding > when to use static inline myself and when to let the compiler do the > clever thing? Not that I'm aware of. It's really up to you to decide. > > > >> + > >> + ret = geni_se_clks_on(se); > >> + if (ret) > >> + return ret; > >> + > >> + ret = pinctrl_pm_select_default_state(se->dev); > >> + if (ret) > >> + geni_se_clks_off(se); > >> + > >> + return ret; > >> +} > >> +EXPORT_SYMBOL(geni_se_resources_on); > > > > IS there a reason why we can't use runtime PM or normal linux PM > > infrastructure to power on the wrapper and keep it powered while the > > protocol driver is active? > Besides turning on the clocks & pinctrl settings, wrapper also has to do > the bus scaling votes. The bus scaling votes depend on the individual > serial interface bandwidth requirements. The bus scaling votes is not > present currently. But once the support comes in, this function enables > adding it. Ok, but that would basically be some code consolidation around picking a bandwidth and enabling/disabling? It sounds like it could go into either the serial interface drivers or into the runtime PM path of the wrapper. > > > >> + > >> +/** > >> + * geni_se_clk_tbl_get() - Get the clock table to program DFS > >> + * @se: Pointer to the concerned Serial Engine. > >> + * @tbl: Table in which the output is returned. > >> + * > >> + * This function is called by the protocol drivers to determine the different > >> + * clock frequencies supported by Serial 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 *se, unsigned long **tbl) > >> +{ > >> + struct geni_wrapper *wrapper = se->wrapper; > >> + unsigned long freq = 0; > >> + int i; > >> + int ret = 0; > >> + > >> + mutex_lock(&wrapper->lock); > >> + if (wrapper->clk_perf_tbl) { > >> + *tbl = wrapper->clk_perf_tbl; > >> + ret = wrapper->num_clk_levels; > >> + goto out_unlock; > >> + } > >> + > >> + wrapper->clk_perf_tbl = kcalloc(MAX_CLK_PERF_LEVEL, > >> + sizeof(*wrapper->clk_perf_tbl), > >> + GFP_KERNEL); > >> + if (!wrapper->clk_perf_tbl) { > >> + ret = -ENOMEM; > >> + goto out_unlock; > >> + } > >> + > >> + for (i = 0; i < MAX_CLK_PERF_LEVEL; i++) { > >> + freq = clk_round_rate(se->clk, freq + 1); > >> + if (!freq || freq == wrapper->clk_perf_tbl[i - 1]) > >> + break; > >> + wrapper->clk_perf_tbl[i] = freq; > >> + } > >> + wrapper->num_clk_levels = i; > >> + *tbl = wrapper->clk_perf_tbl; > >> + ret = wrapper->num_clk_levels; > >> +out_unlock: > >> + mutex_unlock(&wrapper->lock); > > > > Is this lock actually protecting anything? I mean to say, is any more > > than one geni protocol driver calling this function at a time? Or is > > the same geni protocol driver calling this from multiple threads at the > > same time? The lock looks almost useless. > Yes, there is a possibility of multiple I2C instances within the same > wrapper trying to get this table simultaneously. > > As Evan mentioned in the other thread, Bjorn had the comment to move it > to the probe and remove the lock. I looked into the possibility of it. > From the hardware perspective, this table belongs to the wrapper and is > shared by all the serial engines within the wrapper. But due to software > implementation reasons, clk_round_rate can be be performed only on the > clocks that are tagged as DFS compatible and only the serial engine > clocks are tagged so. At least this was the understanding based on our > earlier discussion with the concerned folks. We will revisit it and > check if anything has changed recently. Hmm sounds like the round rate should happen on the parent of the se_clk, and this wrapper DT binding should get the clk for the parent of the se->clk to run round_rate() on. Then it could all be done in probe, which sounds good. > >> + return iova; > >> +} > >> +EXPORT_SYMBOL(geni_se_tx_dma_prep); > >> + > >> +/** > >> + * geni_se_rx_dma_prep() - Prepare the Serial Engine for RX DMA transfer > >> + * @se: Pointer to the concerned Serial Engine. > >> + * @buf: Pointer to the RX buffer. > >> + * @len: Length of the RX buffer. > >> + * > >> + * This function is used to prepare the buffers for DMA RX. > >> + * > >> + * Return: Mapped DMA Address of the buffer on success, NULL on failure. > >> + */ > >> +dma_addr_t geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len) > >> +{ > >> + dma_addr_t iova; > >> + struct geni_wrapper *wrapper = se->wrapper; > >> + u32 val; > >> + > >> + iova = dma_map_single(wrapper->dev, buf, len, DMA_FROM_DEVICE); > >> + if (dma_mapping_error(wrapper->dev, iova)) > >> + return (dma_addr_t)NULL; > > > > Can't return a dma_mapping_error address to the caller and have them > > figure it out? > Earlier we used to return the DMA_ERROR_CODE which has been removed > recently in arm64 architecture. If we return the dma_mapping_error, then > the caller also needs the device which encountered the mapping error. > The serial interface drivers can use their parent currently to resolve > the mapping error. Once the wrapper starts mapping using IOMMU context > bank, then the serial interface drivers do not know which device to use > to know if there is an error. > > Having said that, the dma_ops suggestion might help with handling this > situation. I will look into it further. Ok, thanks. > >> +{ > >> + struct geni_wrapper *wrapper = se->wrapper; > >> + > >> + if (iova) > >> + dma_unmap_single(wrapper->dev, iova, len, DMA_FROM_DEVICE); > >> +} > >> +EXPORT_SYMBOL(geni_se_rx_dma_unprep); > > > > Instead of having the functions exported, could we set the dma_ops on > > all child devices of the wrapper that this driver populates and then > > implement the DMA ops for those devices here? I assume that there's > > never another DMA master between the wrapper and the serial engine, so I > > think it would work. > This suggestion looks like it will work. It would be a good idea to check with some other people on the dma_ops suggestion. Maybe add the DMA mapping subsystem folks to help out here DMA MAPPING HELPERS M: Christoph Hellwig <hch@xxxxxx> M: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> R: Robin Murphy <robin.murphy@xxxxxxx> L: iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx > > > >> + > >> +/* Transfer mode supported by GENI Serial Engines */ > >> +enum geni_se_xfer_mode { > >> + GENI_SE_INVALID, > >> + GENI_SE_FIFO, > >> + GENI_SE_DMA, > >> +}; > >> + > >> +/* Protocols supported by GENI Serial Engines */ > >> +enum geni_se_protocol_types { > >> + GENI_SE_NONE, > >> + GENI_SE_SPI, > >> + GENI_SE_UART, > >> + GENI_SE_I2C, > >> + GENI_SE_I3C, > >> +}; > >> + > >> +/** > >> + * struct geni_se - GENI Serial Engine > >> + * @base: Base Address of the Serial Engine's register block. > >> + * @dev: Pointer to the Serial Engine device. > >> + * @wrapper: Pointer to the parent QUP Wrapper core. > >> + * @clk: Handle to the core serial engine clock. > >> + */ > >> +struct geni_se { > >> + void __iomem *base; > >> + struct device *dev; > >> + void *wrapper; > > > > Can this get the geni_wrapper type? It could be opaque if you like. > I am not sure if it is ok to have the children know the details of the > parent. That is why it is kept as opaque. That's fine, but I mean to have struct geni_wrapper *wrapper, and then struct geni_wrapper; in this file. Children won't know details and we get slightly more type safety. -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html