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

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

[Karthik Ramasubramanian]

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?
> 
> DMA MAPPING HELPERS
> M:      Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> M:      Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> R:      Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> L:      iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> 
> 
Regards,
Karthik.
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Robin Murphy]

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.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Christoph Hellwig]

On Thu, Mar 08, 2018 at 01:24:45PM +0000, Robin Murphy wrote:
> Implementing dma_map_ops inside a driver for real hardware is almost always 
> the wrong thing to do.

Agreed.  dma_map_ops should be a platform decision based on the bus.

Even our dma_virt_ops basically just works around bad driver layering.

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

[Karthik Ramasubramanian]

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu