Re: [PATCH v3 2/4] soc: qcom: Add GENI based QUP Wrapper driver

[Karthik Ramasubramanian <kramasub@codeaurora.org>]

On 3/8/2018 6:24 AM, Robin Murphy wrote:
> On 08/03/18 06:46, Karthik Ramasubramanian wrote:
>>
>>
>> On 3/6/2018 2:56 PM, Stephen Boyd wrote:
>>> Quoting Karthik Ramasubramanian (2018-03-02 16:58:23)
>>>
>>>>>> +       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
>> I have added the DMA mapping subsystem folks to help out here.
>>
>> To present an overview, we have a wrapper controller which is composed 
>> of several serial engines. The serial engines are programmed with 
>> UART, I2C or SPI protocol and support DMA transfer. When the serial 
>> engines perform DMA transfer, the wrapper controller device is used to 
>> perform the mapping. The reason wrapper device is used is because of 
>> IOMMU and there is only one IOMMU context bank to perform the 
>> translation for the entire wrapper controller. So the wrapper 
>> controller exports map and unmap functions to the individual protocol 
>> drivers.
>>
>> There is a suggestion to make the parent wrapper controller implement 
>> the dma_map_ops, instead of exported map/unmap functions and populate 
>> those dma_map_ops on all the children serial engines. Can you please 
>> provide your inputs regarding this suggestion?
> 
> Implementing dma_map_ops inside a driver for real hardware is almost 
> always the wrong thing to do.
> 
> Based on what I could infer about the hardware from looking through the 
> whole series in the linux-arm-msm archive, this is probably more like a 
> multi-channel DMA controller where each "channel" has a configurable 
> serial interface on the other end, as opposed to an actual bus where the 
> serial engines are individually distinct AHB masters routed through the 
> wrapper. If that's true, then using the QUP platform device for DMA API 
> calls is the appropriate thing to do. Personally I'd be inclined not to 
> abstract the dma_{map,unmap} calls at all, and just have the protocol 
> drivers make them directly using dev->parent/wrapper->dev/whatever, but 
> if you do want to abstract those then just give the abstraction a saner 
> interface, i.e. pass the DMA handle by reference and return a regular 
> int for error/success status.
> 
> Robin.
Thank you Robin & Christoph for your inputs. The wrapper driver used to 
provide the recommended abstraction until v2 of this patch series. In v3 
it was tweaked to address a comment. If there are no objections, I will 
revive it back.

Regards,
Karthik.
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html