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.
