| Message ID | 1339474866-17805-1-git-send-email-girish.shivananjappa@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 12, 2012 at 5:21 AM, Girish K S <girish.shivananjappa@linaro.org> wrote: > > In the Current dwmmc driver there is support for selecting IDMAC from > the menu config option. If the support for IDMAC is enabled in the menu > config and Hardware configuration register's DMA_INTERFACE field is 0. > Still the driver will try to do the DMA initialization. > > The dw_mci_idmac_init function currently implemented returns only success > indicating that the DMA initialization is always successful. The current > patch will add a ciheck for existance of the DMA IP and allow the > DMA initialization. > > Signed-off-by: Girish K S <girish.shivananjappa@linaro.org> > --- > drivers/mmc/host/dw_mmc.c | 11 ++++++++++- > 1 files changed, 10 insertions(+), 1 deletions(-) This looks ok in principle. I'm wondering if we should only allow dma_support == 0x01 (DW_DMA) in the IDMAC case? It's not clear from the TRM what each of the DMA values means and I only have a system with external DMA support at the moment.
On 12 June 2012 14:25, Will Newton <will.newton@gmail.com> wrote: > On Tue, Jun 12, 2012 at 5:21 AM, Girish K S > <girish.shivananjappa@linaro.org> wrote: >> >> In the Current dwmmc driver there is support for selecting IDMAC from >> the menu config option. If the support for IDMAC is enabled in the menu >> config and Hardware configuration register's DMA_INTERFACE field is 0. >> Still the driver will try to do the DMA initialization. >> >> The dw_mci_idmac_init function currently implemented returns only success >> indicating that the DMA initialization is always successful. The current >> patch will add a ciheck for existance of the DMA IP and allow the >> DMA initialization. >> >> Signed-off-by: Girish K S <girish.shivananjappa@linaro.org> >> --- >> drivers/mmc/host/dw_mmc.c | 11 ++++++++++- >> 1 files changed, 10 insertions(+), 1 deletions(-) > > This looks ok in principle. I'm wondering if we should only allow > dma_support == 0x01 (DW_DMA) in the IDMAC case? It's not clear from > the TRM what each of the DMA values means and I only have a system > with external DMA support at the moment. My understanding from the TRM if dma_support =1; then use designware dma ip for internal dma_support =2 then use any generic dma ip for internal dma tx this config is only for IDMA support. If there is a external DMA CTRL register should be used explicitly to enable DMA (there is no config available to tell external dma supported). correct me if i am wrong.
On Tue, Jun 12, 2012 at 10:01 AM, Girish K S <girish.shivananjappa@linaro.org> wrote: > On 12 June 2012 14:25, Will Newton <will.newton@gmail.com> wrote: >> On Tue, Jun 12, 2012 at 5:21 AM, Girish K S >> <girish.shivananjappa@linaro.org> wrote: >>> >>> In the Current dwmmc driver there is support for selecting IDMAC from >>> the menu config option. If the support for IDMAC is enabled in the menu >>> config and Hardware configuration register's DMA_INTERFACE field is 0. >>> Still the driver will try to do the DMA initialization. >>> >>> The dw_mci_idmac_init function currently implemented returns only success >>> indicating that the DMA initialization is always successful. The current >>> patch will add a ciheck for existance of the DMA IP and allow the >>> DMA initialization. >>> >>> Signed-off-by: Girish K S <girish.shivananjappa@linaro.org> >>> --- >>> drivers/mmc/host/dw_mmc.c | 11 ++++++++++- >>> 1 files changed, 10 insertions(+), 1 deletions(-) >> >> This looks ok in principle. I'm wondering if we should only allow >> dma_support == 0x01 (DW_DMA) in the IDMAC case? It's not clear from >> the TRM what each of the DMA values means and I only have a system >> with external DMA support at the moment. > My understanding from the TRM > if dma_support =1; then use designware dma ip for internal > dma_support =2 then use any generic dma ip for internal dma tx > this config is only for IDMA support. If there is a external DMA CTRL > register should be used explicitly to enable DMA (there is no config > available to tell external dma supported). correct me if i am wrong. That sounds like a reasonable interpretation of the TRM. ;-) Acked-by: Will Newton <will.newton@imgtec.com>
On 12 June 2012 15:05, Will Newton <will.newton@gmail.com> wrote: > On Tue, Jun 12, 2012 at 10:01 AM, Girish K S > <girish.shivananjappa@linaro.org> wrote: >> On 12 June 2012 14:25, Will Newton <will.newton@gmail.com> wrote: >>> On Tue, Jun 12, 2012 at 5:21 AM, Girish K S >>> <girish.shivananjappa@linaro.org> wrote: >>>> >>>> In the Current dwmmc driver there is support for selecting IDMAC from >>>> the menu config option. If the support for IDMAC is enabled in the menu >>>> config and Hardware configuration register's DMA_INTERFACE field is 0. >>>> Still the driver will try to do the DMA initialization. >>>> >>>> The dw_mci_idmac_init function currently implemented returns only success >>>> indicating that the DMA initialization is always successful. The current >>>> patch will add a ciheck for existance of the DMA IP and allow the >>>> DMA initialization. >>>> >>>> Signed-off-by: Girish K S <girish.shivananjappa@linaro.org> >>>> --- >>>> drivers/mmc/host/dw_mmc.c | 11 ++++++++++- >>>> 1 files changed, 10 insertions(+), 1 deletions(-) >>> >>> This looks ok in principle. I'm wondering if we should only allow >>> dma_support == 0x01 (DW_DMA) in the IDMAC case? It's not clear from >>> the TRM what each of the DMA values means and I only have a system >>> with external DMA support at the moment. >> My understanding from the TRM >> if dma_support =1; then use designware dma ip for internal >> dma_support =2 then use any generic dma ip for internal dma tx >> this config is only for IDMA support. If there is a external DMA CTRL >> register should be used explicitly to enable DMA (there is no config >> available to tell external dma supported). correct me if i am wrong. > > That sounds like a reasonable interpretation of the TRM. ;-) > > Acked-by: Will Newton <will.newton@imgtec.com> I will resend the patch with minor change so that kernel doesnt crash but use PIO mode and continue even if IDMAC is enabled accidentally. what s your say?
On Tue, Jun 12, 2012 at 10:40 AM, Girish K S <girish.shivananjappa@linaro.org> wrote: > On 12 June 2012 15:05, Will Newton <will.newton@gmail.com> wrote: >> On Tue, Jun 12, 2012 at 10:01 AM, Girish K S >> <girish.shivananjappa@linaro.org> wrote: >>> On 12 June 2012 14:25, Will Newton <will.newton@gmail.com> wrote: >>>> On Tue, Jun 12, 2012 at 5:21 AM, Girish K S >>>> <girish.shivananjappa@linaro.org> wrote: >>>>> >>>>> In the Current dwmmc driver there is support for selecting IDMAC from >>>>> the menu config option. If the support for IDMAC is enabled in the menu >>>>> config and Hardware configuration register's DMA_INTERFACE field is 0. >>>>> Still the driver will try to do the DMA initialization. >>>>> >>>>> The dw_mci_idmac_init function currently implemented returns only success >>>>> indicating that the DMA initialization is always successful. The current >>>>> patch will add a ciheck for existance of the DMA IP and allow the >>>>> DMA initialization. >>>>> >>>>> Signed-off-by: Girish K S <girish.shivananjappa@linaro.org> >>>>> --- >>>>> drivers/mmc/host/dw_mmc.c | 11 ++++++++++- >>>>> 1 files changed, 10 insertions(+), 1 deletions(-) >>>> >>>> This looks ok in principle. I'm wondering if we should only allow >>>> dma_support == 0x01 (DW_DMA) in the IDMAC case? It's not clear from >>>> the TRM what each of the DMA values means and I only have a system >>>> with external DMA support at the moment. >>> My understanding from the TRM >>> if dma_support =1; then use designware dma ip for internal >>> dma_support =2 then use any generic dma ip for internal dma tx >>> this config is only for IDMA support. If there is a external DMA CTRL >>> register should be used explicitly to enable DMA (there is no config >>> available to tell external dma supported). correct me if i am wrong. >> >> That sounds like a reasonable interpretation of the TRM. ;-) >> >> Acked-by: Will Newton <will.newton@imgtec.com> > I will resend the patch with minor change so that kernel doesnt crash > but use PIO mode and continue even if IDMAC is enabled accidentally. > what s your say? Yes, let's make sure it is robust against errors in configuration.
On 12 June 2012 15:14, Will Newton <will.newton@gmail.com> wrote: > On Tue, Jun 12, 2012 at 10:40 AM, Girish K S > <girish.shivananjappa@linaro.org> wrote: >> On 12 June 2012 15:05, Will Newton <will.newton@gmail.com> wrote: >>> On Tue, Jun 12, 2012 at 10:01 AM, Girish K S >>> <girish.shivananjappa@linaro.org> wrote: >>>> On 12 June 2012 14:25, Will Newton <will.newton@gmail.com> wrote: >>>>> On Tue, Jun 12, 2012 at 5:21 AM, Girish K S >>>>> <girish.shivananjappa@linaro.org> wrote: >>>>>> >>>>>> In the Current dwmmc driver there is support for selecting IDMAC from >>>>>> the menu config option. If the support for IDMAC is enabled in the menu >>>>>> config and Hardware configuration register's DMA_INTERFACE field is 0. >>>>>> Still the driver will try to do the DMA initialization. >>>>>> >>>>>> The dw_mci_idmac_init function currently implemented returns only success >>>>>> indicating that the DMA initialization is always successful. The current >>>>>> patch will add a ciheck for existance of the DMA IP and allow the >>>>>> DMA initialization. >>>>>> >>>>>> Signed-off-by: Girish K S <girish.shivananjappa@linaro.org> >>>>>> --- >>>>>> drivers/mmc/host/dw_mmc.c | 11 ++++++++++- >>>>>> 1 files changed, 10 insertions(+), 1 deletions(-) >>>>> >>>>> This looks ok in principle. I'm wondering if we should only allow >>>>> dma_support == 0x01 (DW_DMA) in the IDMAC case? It's not clear from >>>>> the TRM what each of the DMA values means and I only have a system >>>>> with external DMA support at the moment. >>>> My understanding from the TRM >>>> if dma_support =1; then use designware dma ip for internal >>>> dma_support =2 then use any generic dma ip for internal dma tx >>>> this config is only for IDMA support. If there is a external DMA CTRL >>>> register should be used explicitly to enable DMA (there is no config >>>> available to tell external dma supported). correct me if i am wrong. >>> >>> That sounds like a reasonable interpretation of the TRM. ;-) >>> >>> Acked-by: Will Newton <will.newton@imgtec.com> >> I will resend the patch with minor change so that kernel doesnt crash >> but use PIO mode and continue even if IDMAC is enabled accidentally. >> what s your say? > > Yes, let's make sure it is robust against errors in configuration. i have tested the resent patch. scenario: 1. If IDMAC is enabled and if hardware actually doesnt have DMA controller. In that case it uses the PIO mode. provided the host->ring_size is initialized. thats the reson for moving the check below the ring_size initialize
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index cbd8f3d..9821293 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -405,7 +405,16 @@ static void dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len) static int dw_mci_idmac_init(struct dw_mci *host) { struct idmac_desc *p; - int i; + int i, dma_support; + + /* Check if Hardware Configuration Register has support for DMA */ + dma_support = (mci_readl(host, HCON) >> 16) & 0x3; + + if (!dma_support || dma_support > 2) { + dev_err(&host->dev, + "Host Controller does not support DMA Tx.\n"); + return -ENODEV; + } /* Number of descriptors in the ring buffer */ host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
In the Current dwmmc driver there is support for selecting IDMAC from the menu config option. If the support for IDMAC is enabled in the menu config and Hardware configuration register's DMA_INTERFACE field is 0. Still the driver will try to do the DMA initialization. The dw_mci_idmac_init function currently implemented returns only success indicating that the DMA initialization is always successful. The current patch will add a ciheck for existance of the DMA IP and allow the DMA initialization. Signed-off-by: Girish K S <girish.shivananjappa@linaro.org> --- drivers/mmc/host/dw_mmc.c | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-)