From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934116AbeAXPbj (ORCPT ); Wed, 24 Jan 2018 10:31:39 -0500 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:50948 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933936AbeAXPbh (ORCPT ); Wed, 24 Jan 2018 10:31:37 -0500 Subject: Re: [PATCH 4/8] mfd: stm32-timers: add support for dmas To: Lee Jones CC: , , , , , , , , , , , References: <1516106631-18722-1-git-send-email-fabrice.gasnier@st.com> <1516106631-18722-5-git-send-email-fabrice.gasnier@st.com> <20180123133234.xmep76cgdremnj47@dell> <20180123153035.2rvhspe2m5ej57op@dell> <20180123164121.xjzhypqe2u5c7zza@dell> <2e6afc46-9ad4-a858-29c7-692798ece82f@st.com> <20180124145633.tnblnqau7ncju7dp@dell> From: Fabrice Gasnier Message-ID: <0c6a185c-cc12-38ae-3909-5c0e82f8da3f@st.com> Date: Wed, 24 Jan 2018 16:30:58 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20180124145633.tnblnqau7ncju7dp@dell> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.75.127.45] X-ClientProxiedBy: SFHDAG5NODE3.st.com (10.75.127.15) To SFHDAG5NODE3.st.com (10.75.127.15) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-01-24_06:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/24/2018 03:56 PM, Lee Jones wrote: > On Wed, 24 Jan 2018, Fabrice Gasnier wrote: >> On 01/23/2018 05:41 PM, Lee Jones wrote: >>> On Tue, 23 Jan 2018, Fabrice Gasnier wrote: >>>> On 01/23/2018 04:30 PM, Lee Jones wrote: >>>>> On Tue, 23 Jan 2018, Fabrice Gasnier wrote: >>>>> >>>>>> On 01/23/2018 02:32 PM, Lee Jones wrote: >>>>>>> On Tue, 16 Jan 2018, Fabrice Gasnier wrote: >>>>>>> >>>>>>>> STM32 Timers can support up to 7 dma requests: >>>>>>>> 4 channels, update, compare and trigger. >>>>>>>> Optionally request part, or all dmas from stm32-timers MFD core. >>>>>>>> Also, keep reference of device's bus address to allow child drivers to >>>>>>>> transfer data from/to device by using dma. >>>>>>>> >>>>>>>> Signed-off-by: Fabrice Gasnier >>>>>>>> --- >>>>>>>> drivers/mfd/stm32-timers.c | 37 ++++++++++++++++++++++++++++++++++++- >>>>>>>> include/linux/mfd/stm32-timers.h | 14 ++++++++++++++ >>>>>>>> 2 files changed, 50 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c >>>>>>>> static int stm32_timers_probe(struct platform_device *pdev) >>>>>>>> { >>>>>>>> struct device *dev = &pdev->dev; >>>>>>>> @@ -44,6 +61,7 @@ static int stm32_timers_probe(struct platform_device *pdev) >>>>>>>> mmio = devm_ioremap_resource(dev, res); >>>>>>>> if (IS_ERR(mmio)) >>>>>>>> return PTR_ERR(mmio); >>>>>>>> + ddata->phys_base = res->start; >>>>>>> >>>>>>> What do you use this for? >>>>>> >>>>>> This is used in in child driver (pwm) for capture data transfer by dma. >>>>> >>>>> Might be worth being clear about that. >>>>> >>>>> Perhaps pass in 'dma_base' (phys_base + offset) instead? >>>> >>>> I guess you've had a look at [PATCH 5/8] pwm: stm32: add capture >>>> support. Are you talking about passing phys_base + TIM_DMAR ? >>> >>> I have and I am. >>> >>>> If this is the case, I'd prefer to keep phys base only if you don't >>>> mind, and handle TIM_DMAR offset in pwm driver. This way, all dma slave >>>> config is kept locally at one place. >>>> Or do you mean something else ? >>>> >>>> Maybe I can add a comment here about this ? >>>> Something like: >>>> /* phys_base to be used by child driver, e.g. DMA burst mode */ >>> >>> I haven't seen the memory map for this device, so it's not easy for me >>> to comment, but passing in the physical address of the parent MFD into >>> a child device doesn't quite sit right with me. >>> >>> At what level does TIM_DMAR sit? Is it a child (PWM) specific >>> property, or is it described at parent (Timer) level? >>> >> Hi Lee, >> >> This isn't child (PWM) specific. TIM_DMAR is described at timer level as >> well as all timers DMA requests lines. Current patchset make it useful >> for PWM capture. Basically, I think this can be seen as interrupts, as >> each (0..7) dma request has an enable bit (in DIER: interrupt enable >> register). This is similar as interrupts at timer level. >> >> So, I understand your point regarding passing physical address of the >> parent MFD... Speaking of interrupts, I'd probably have looked at >> irq_chip. Regarding dma, i'm not sure what is preferred way ? >> >> Another way maybe to export a routine (export symbol) from MFD core, to >> handle dma transfer from there? >> By looking into drivers/mfd, I found similar approach, e.g. >> rtsx_pci_dma_transfer(). Do you think this is better approach ? >> >> Please let me know your opinion. > > It sounds fine in principle. You are in a better position to make > that decision as you know the H/W more intimately than I, however it > does sound like a good idea to abstract the DMA handling from the > device if these aren't device-{level|specific} operations. > Hi Lee, Thanks, I'll rework this in v2 to follow your advice. Best Regards, Fabrice