Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1211636 > unrolled thread
| Started by | Vinod Koul <vinod.koul@intel.com> |
|---|---|
| First post | 2015-08-23 16:40 +0200 |
| Last post | 2015-08-24 11:30 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [RFC PATCH 7/7] DMA: tegra-adma: Add support for Tegra210 ADMA Vinod Koul <vinod.koul@intel.com> - 2015-08-23 16:40 +0200
Re: [RFC PATCH 7/7] DMA: tegra-adma: Add support for Tegra210 ADMA Jon Hunter <jonathanh@nvidia.com> - 2015-08-24 11:00 +0200
Re: [RFC PATCH 7/7] DMA: tegra-adma: Add support for Tegra210 ADMA Vinod Koul <vinod.koul@intel.com> - 2015-08-24 11:30 +0200
| From | Vinod Koul <vinod.koul@intel.com> |
|---|---|
| Date | 2015-08-23 16:40 +0200 |
| Subject | Re: [RFC PATCH 7/7] DMA: tegra-adma: Add support for Tegra210 ADMA |
| Message-ID | <q0EBc-3yk-23@gated-at.bofh.it> |
On Tue, Aug 18, 2015 at 02:49:15PM +0100, Jon Hunter wrote:
> +#define AHUB_TO_MEMORY 2
> +#define MEMORY_TO_AHUB 4
namespace this aptly as well
> +static void tegra_adma_stop(struct tegra_dma_channel *tdc)
> +{
> + u32 status;
> +
> + /* TODO: Do we need to disable interrupts here? */
when?
> +static void tegra_adma_start(struct tegra_dma_channel *tdc,
> + struct tegra_dma_sg_req *sg_req)
> +{
> + struct tegra_adma_chan_regs *ch_regs = &sg_req->adma_ch_regs;
> +
> + /* Update transfer done count for position calculation */
> + tdc->adma_ch_regs.tc = ch_regs->tc;
> + tdc_write(tdc, ADMA_CH_TC, ch_regs->tc);
> + tdc_write(tdc, ADMA_CH_CTRL, ch_regs->ctrl);
> + tdc_write(tdc, ADMA_CH_LOWER_SOURCE_ADDR, ch_regs->src_ptr);
> + tdc_write(tdc, ADMA_CH_LOWER_TARGET_ADDR, ch_regs->tgt_ptr);
> + tdc_write(tdc, ADMA_CH_AHUB_FIFO_CTRL, ch_regs->ahub_fifo_ctrl);
> + tdc_write(tdc, ADMA_CH_CONFIG, ch_regs->config);
empty line here please
> +static int tegra_adma_get_xfer_params(struct tegra_dma_channel *tdc,
> + struct tegra_adma_chan_regs *ch_regs,
> + enum dma_transfer_direction direction)
> +{
> + u32 burst_size, ctrl, ctrl_mask, slave_id, fifo_mask, fifo_shift;
> +
> + ch_regs->ahub_fifo_ctrl = tdc_read(tdc, ADMA_CH_AHUB_FIFO_CTRL);
> + ch_regs->config = tdc_read(tdc, ADMA_CH_CONFIG);
> + ch_regs->ctrl = tdc_read(tdc, ADMA_CH_CTRL);
> + slave_id = tdc->dma_sconfig.slave_id;
> +
> + switch (direction) {
> + case DMA_MEM_TO_DEV:
> + burst_size = fls(tdc->dma_sconfig.dst_maxburst);
> + ctrl_mask = ADMA_CH_CTRL_TX_REQUEST_SELECT_MASK;
> + ctrl = MEMORY_TO_AHUB << ADMA_CH_CTRL_TRANSFER_DIRECTION_SHIFT;
> + ctrl |= slave_id << ADMA_CH_CTRL_TX_REQUEST_SELECT_SHIFT;
> + fifo_mask = ADMA_CH_AHUB_FIFO_CTRL_TX_FIFO_SIZE_MASK;
> + fifo_shift = ADMA_CH_AHUB_FIFO_CTRL_TX_FIFO_SIZE_SHIFT;
> + break;
Empty line here pls
> + case DMA_DEV_TO_MEM:
> + burst_size = fls(tdc->dma_sconfig.src_maxburst);
> + ctrl_mask = ADMA_CH_CTRL_RX_REQUEST_SELECT_MASK;
> + ctrl = AHUB_TO_MEMORY << ADMA_CH_CTRL_TRANSFER_DIRECTION_SHIFT;
> + ctrl |= slave_id << ADMA_CH_CTRL_RX_REQUEST_SELECT_SHIFT;
> + fifo_mask = ADMA_CH_AHUB_FIFO_CTRL_RX_FIFO_SIZE_MASK;
> + fifo_shift = ADMA_CH_AHUB_FIFO_CTRL_RX_FIFO_SIZE_SHIFT;
> + break;
here too...
> + default:
> + dev_err(tdc2dev(tdc), "Dma direction is not supported\n");
> + return -EINVAL;
> + }
> +
> + if (!burst_size || burst_size > ADMA_BURSTSIZE_16)
> + burst_size = ADMA_BURSTSIZE_16;
> +
> + ch_regs->ahub_fifo_ctrl &= ~fifo_mask;
> + ch_regs->ahub_fifo_ctrl |= ADMA_FIFO_DEFAULT_SIZE << fifo_shift;
> + ch_regs->config &= ~ADMA_CH_CONFIG_BURST_SIZE_MASK;
> + ch_regs->config |= burst_size << ADMA_CH_CONFIG_BURST_SIZE_SHIFT;
> + ch_regs->ctrl &= ~(ctrl_mask | ADMA_CH_CTRL_TRANSFER_DIRECTION_MASK);
> + ch_regs->ctrl |= ctrl;
> +
> + return -EINVAL;
??
> +static int tegra_adma_pm_suspend(struct device *dev)
> +{
> + struct tegra_dma *tdma = dev_get_drvdata(dev);
> + int i;
> + int ret;
> +
> + ret = pm_runtime_get_sync(dev);
why is this required :)
> +static int tegra_adma_pm_resume(struct device *dev)
> +{
> + struct tegra_dma *tdma = dev_get_drvdata(dev);
> + int i;
> + int ret;
> +
> + ret = pm_runtime_get_sync(dev);
and this
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Jon Hunter <jonathanh@nvidia.com> |
|---|---|
| Date | 2015-08-24 11:00 +0200 |
| Message-ID | <q0VLH-2M2-1@gated-at.bofh.it> |
| In reply to | #1211636 |
On 23/08/15 15:33, Vinod Koul wrote:
> On Tue, Aug 18, 2015 at 02:49:15PM +0100, Jon Hunter wrote:
>> +#define AHUB_TO_MEMORY 2
>> +#define MEMORY_TO_AHUB 4
>
> namespace this aptly as well
>
>> +static void tegra_adma_stop(struct tegra_dma_channel *tdc)
>> +{
>> + u32 status;
>> +
>> + /* TODO: Do we need to disable interrupts here? */
>
> when?
Once everyone is happy with the RFC in general.
>> +static void tegra_adma_start(struct tegra_dma_channel *tdc,
>> + struct tegra_dma_sg_req *sg_req)
>> +{
>> + struct tegra_adma_chan_regs *ch_regs = &sg_req->adma_ch_regs;
>> +
>> + /* Update transfer done count for position calculation */
>> + tdc->adma_ch_regs.tc = ch_regs->tc;
>> + tdc_write(tdc, ADMA_CH_TC, ch_regs->tc);
>> + tdc_write(tdc, ADMA_CH_CTRL, ch_regs->ctrl);
>> + tdc_write(tdc, ADMA_CH_LOWER_SOURCE_ADDR, ch_regs->src_ptr);
>> + tdc_write(tdc, ADMA_CH_LOWER_TARGET_ADDR, ch_regs->tgt_ptr);
>> + tdc_write(tdc, ADMA_CH_AHUB_FIFO_CTRL, ch_regs->ahub_fifo_ctrl);
>> + tdc_write(tdc, ADMA_CH_CONFIG, ch_regs->config);
> empty line here please
Ok.
>> +static int tegra_adma_get_xfer_params(struct tegra_dma_channel *tdc,
>> + struct tegra_adma_chan_regs *ch_regs,
>> + enum dma_transfer_direction direction)
>> +{
>> + u32 burst_size, ctrl, ctrl_mask, slave_id, fifo_mask, fifo_shift;
>> +
>> + ch_regs->ahub_fifo_ctrl = tdc_read(tdc, ADMA_CH_AHUB_FIFO_CTRL);
>> + ch_regs->config = tdc_read(tdc, ADMA_CH_CONFIG);
>> + ch_regs->ctrl = tdc_read(tdc, ADMA_CH_CTRL);
>> + slave_id = tdc->dma_sconfig.slave_id;
>> +
>> + switch (direction) {
>> + case DMA_MEM_TO_DEV:
>> + burst_size = fls(tdc->dma_sconfig.dst_maxburst);
>> + ctrl_mask = ADMA_CH_CTRL_TX_REQUEST_SELECT_MASK;
>> + ctrl = MEMORY_TO_AHUB << ADMA_CH_CTRL_TRANSFER_DIRECTION_SHIFT;
>> + ctrl |= slave_id << ADMA_CH_CTRL_TX_REQUEST_SELECT_SHIFT;
>> + fifo_mask = ADMA_CH_AHUB_FIFO_CTRL_TX_FIFO_SIZE_MASK;
>> + fifo_shift = ADMA_CH_AHUB_FIFO_CTRL_TX_FIFO_SIZE_SHIFT;
>> + break;
> Empty line here pls
Ok, any reason why? Other dma drivers don't appear to do this.
>> + case DMA_DEV_TO_MEM:
>> + burst_size = fls(tdc->dma_sconfig.src_maxburst);
>> + ctrl_mask = ADMA_CH_CTRL_RX_REQUEST_SELECT_MASK;
>> + ctrl = AHUB_TO_MEMORY << ADMA_CH_CTRL_TRANSFER_DIRECTION_SHIFT;
>> + ctrl |= slave_id << ADMA_CH_CTRL_RX_REQUEST_SELECT_SHIFT;
>> + fifo_mask = ADMA_CH_AHUB_FIFO_CTRL_RX_FIFO_SIZE_MASK;
>> + fifo_shift = ADMA_CH_AHUB_FIFO_CTRL_RX_FIFO_SIZE_SHIFT;
>> + break;
>
> here too...
>
>> + default:
>> + dev_err(tdc2dev(tdc), "Dma direction is not supported\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!burst_size || burst_size > ADMA_BURSTSIZE_16)
>> + burst_size = ADMA_BURSTSIZE_16;
>> +
>> + ch_regs->ahub_fifo_ctrl &= ~fifo_mask;
>> + ch_regs->ahub_fifo_ctrl |= ADMA_FIFO_DEFAULT_SIZE << fifo_shift;
>> + ch_regs->config &= ~ADMA_CH_CONFIG_BURST_SIZE_MASK;
>> + ch_regs->config |= burst_size << ADMA_CH_CONFIG_BURST_SIZE_SHIFT;
>> + ch_regs->ctrl &= ~(ctrl_mask | ADMA_CH_CTRL_TRANSFER_DIRECTION_MASK);
>> + ch_regs->ctrl |= ctrl;
>> +
>> + return -EINVAL;
> ??
Thanks. That's an error. Will fix.
>> +static int tegra_adma_pm_suspend(struct device *dev)
>> +{
>> + struct tegra_dma *tdma = dev_get_drvdata(dev);
>> + int i;
>> + int ret;
>> +
>> + ret = pm_runtime_get_sync(dev);
> why is this required :)
To ensure that the clocks are enabled before the registers are read.
This function saves the dma context before suspend, in case the hardware
state is lost.
>> +static int tegra_adma_pm_resume(struct device *dev)
>> +{
>> + struct tegra_dma *tdma = dev_get_drvdata(dev);
>> + int i;
>> + int ret;
>> +
>> + ret = pm_runtime_get_sync(dev);
> and this
Same here.
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Vinod Koul <vinod.koul@intel.com> |
|---|---|
| Date | 2015-08-24 11:30 +0200 |
| Message-ID | <q0WeM-3A1-65@gated-at.bofh.it> |
| In reply to | #1211925 |
On Mon, Aug 24, 2015 at 09:55:03AM +0100, Jon Hunter wrote:
> >> +static int tegra_adma_get_xfer_params(struct tegra_dma_channel *tdc,
> >> + struct tegra_adma_chan_regs *ch_regs,
> >> + enum dma_transfer_direction direction)
> >> +{
> >> + u32 burst_size, ctrl, ctrl_mask, slave_id, fifo_mask, fifo_shift;
> >> +
> >> + ch_regs->ahub_fifo_ctrl = tdc_read(tdc, ADMA_CH_AHUB_FIFO_CTRL);
> >> + ch_regs->config = tdc_read(tdc, ADMA_CH_CONFIG);
> >> + ch_regs->ctrl = tdc_read(tdc, ADMA_CH_CTRL);
> >> + slave_id = tdc->dma_sconfig.slave_id;
> >> +
> >> + switch (direction) {
> >> + case DMA_MEM_TO_DEV:
> >> + burst_size = fls(tdc->dma_sconfig.dst_maxburst);
> >> + ctrl_mask = ADMA_CH_CTRL_TX_REQUEST_SELECT_MASK;
> >> + ctrl = MEMORY_TO_AHUB << ADMA_CH_CTRL_TRANSFER_DIRECTION_SHIFT;
> >> + ctrl |= slave_id << ADMA_CH_CTRL_TX_REQUEST_SELECT_SHIFT;
> >> + fifo_mask = ADMA_CH_AHUB_FIFO_CTRL_TX_FIFO_SIZE_MASK;
> >> + fifo_shift = ADMA_CH_AHUB_FIFO_CTRL_TX_FIFO_SIZE_SHIFT;
> >> + break;
> > Empty line here pls
>
> Ok, any reason why? Other dma drivers don't appear to do this.
This improves readablity. It is a good idea to sprinkle empty lines between
logial blocks
> >> +static int tegra_adma_pm_suspend(struct device *dev)
> >> +{
> >> + struct tegra_dma *tdma = dev_get_drvdata(dev);
> >> + int i;
> >> + int ret;
> >> +
> >> + ret = pm_runtime_get_sync(dev);
> > why is this required :)
>
> To ensure that the clocks are enabled before the registers are read.
> This function saves the dma context before suspend, in case the hardware
> state is lost.
I think same reason here too, so based on my other argument, this may not be
required!
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web