Hi Miquel,
> >> Does it work if there is no DMA channel provided? The bindings do not
> >> mention DMA channels as mandatory.
> >>
> >
> > The way Cadence NAND controller driver is written in such a way that
> > it uses
> > has_dma=1 as hardcoded value, indicating that slave DMA interface is
> > connected to DMA engine. However, it does not utilize the dedicated
> > DMA channel information from the device tree.
>
> This is not ok.
>
> > Driver works without external DMA interface i.e. has_dma=0.
> > However current driver does not have a mechanism to configure it from
> > device tree.
>
> What? Why are you requesting a DMA channel from a dmaengine in this case?
>
> Please make the distinction between the OS implementation (the driver) and
> the DT binding which describe the HW and only the HW.
>
Let me clarify from bindings(hw) and driver prospective.
Bindings :-
Cadence NAND controller HW has MMIO registers, so called slave DMA interface
for page programming or page read.
reg = <0x10b80000 0x10000>,
<0x10840000 0x10000>;
reg-names = "reg", "sdma"; // sdma = Slave DMA data port register set
It appears that dt bindings has captured sdma interface correctly.
Linux Driver:-
Driver can read these sdma registers directly or it can use the DMA.
Existing driver code has hardcoded has_dma with an assumption that
an external DMA is always used and relies on DMA API for data transfer.
Thant is why it requires to use DMA channel from dmaengine.
In my previous reply, I tried to describe this driver scenario but maybe I mixed up.
has_dma=0, i.e. accessing sdma register without using dmaengine is also working.
However, currently there is no option in driver to choose between using dmaengine and
direct register access.
> > cdns_ctrl->dmac = dma_request_chan_by_mask(&mask);
> > if (IS_ERR(cdns_ctrl->dmac)) {
> > ret = PTR_ERR(cdns_ctrl->dmac);
> > if (ret != -EPROBE_DEFER)
> > dev_err(cdns_ctrl->dev,
> > "Failed to get a DMA
> channel:%d\n",ret);
> > goto disable_irq;
> > }
> >
> > Is this reasonable?
>
> It is better, but maybe you can use dev_err_probe() instead to include the
> EPROBE_DEFER error handling.
>
Got it. I will update the code as below.
cdns_ctrl->dmac = dma_request_chan_by_mask(&mask);
if (IS_ERR(cdns_ctrl->dmac)) {
ret = dev_err_probe(cdns_ctrl->dev, PTR_ERR(cdns_ctrl->dmac),
"%d: Failed to get a DMA channel\n",ret);
goto disable_irq;
}
Thanks,
Nirav
