The DMA controller currently takes configuration information from
information passed though dma_channel_request(), but it shouldn't.
Using the API, the DMA channel should only be configured during
a dma_slave_config() call.
Cc: Herbert Xu <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Andreas Westin <[email protected]>
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
drivers/crypto/ux500/cryp/cryp.h | 7 ++++++-
drivers/crypto/ux500/cryp/cryp_core.c | 17 +++++++++++++++++
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/ux500/cryp/cryp.h b/drivers/crypto/ux500/cryp/cryp.h
index 14cfd05..49b2095 100644
--- a/drivers/crypto/ux500/cryp/cryp.h
+++ b/drivers/crypto/ux500/cryp/cryp.h
@@ -114,6 +114,9 @@ enum cryp_status_id {
};
/* Cryp DMA interface */
+#define HASH_DMA_TX_FIFO 0x08
+#define HASH_DMA_RX_FIFO 0x10
+
enum cryp_dma_req_type {
CRYP_DMA_DISABLE_BOTH,
CRYP_DMA_ENABLE_IN_DATA,
@@ -217,7 +220,8 @@ struct cryp_dma {
/**
* struct cryp_device_data - structure for a cryp device.
- * @base: Pointer to the hardware base address.
+ * @base: Pointer to virtual base address of the cryp device.
+ * @phybase: Pointer to physical memory location of the cryp device.
* @dev: Pointer to the devices dev structure.
* @clk: Pointer to the device's clock control.
* @pwr_regulator: Pointer to the device's power control.
@@ -232,6 +236,7 @@ struct cryp_dma {
*/
struct cryp_device_data {
struct cryp_register __iomem *base;
+ phys_addr_t phybase;
struct device *dev;
struct clk *clk;
struct regulator *pwr_regulator;
diff --git a/drivers/crypto/ux500/cryp/cryp_core.c b/drivers/crypto/ux500/cryp/cryp_core.c
index 444deaf..6c4f872 100644
--- a/drivers/crypto/ux500/cryp/cryp_core.c
+++ b/drivers/crypto/ux500/cryp/cryp_core.c
@@ -476,6 +476,19 @@ static int cryp_get_device_data(struct cryp_ctx *ctx,
static void cryp_dma_setup_channel(struct cryp_device_data *device_data,
struct device *dev)
{
+ struct dma_slave_config mem2cryp = {
+ .direction = DMA_MEM_TO_DEV,
+ .dst_addr = device_data->phybase + HASH_DMA_TX_FIFO,
+ .dst_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES,
+ .dst_maxburst = 4,
+ };
+ struct dma_slave_config cryp2mem = {
+ .direction = DMA_DEV_TO_MEM,
+ .src_addr = device_data->phybase + HASH_DMA_RX_FIFO,
+ .src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES,
+ .src_maxburst = 4,
+ };
+
dma_cap_zero(device_data->dma.mask);
dma_cap_set(DMA_SLAVE, device_data->dma.mask);
@@ -491,6 +504,9 @@ static void cryp_dma_setup_channel(struct cryp_device_data *device_data,
stedma40_filter,
device_data->dma.cfg_cryp2mem);
+ dmaengine_slave_config(device_data->dma.chan_mem2cryp, &mem2cryp);
+ dmaengine_slave_config(device_data->dma.chan_cryp2mem, &cryp2mem);
+
init_completion(&device_data->dma.cryp_dma_complete);
}
@@ -1432,6 +1448,7 @@ static int ux500_cryp_probe(struct platform_device *pdev)
goto out_kfree;
}
+ device_data->phybase = res->start;
device_data->base = ioremap(res->start, resource_size(res));
if (!device_data->base) {
dev_err(dev, "[%s]: ioremap failed!", __func__);
--
1.7.10.4
On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <[email protected]> wrote:
> The DMA controller currently takes configuration information from
> information passed though dma_channel_request(), but it shouldn't.
> Using the API, the DMA channel should only be configured during
> a dma_slave_config() call.
>
> Cc: Herbert Xu <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Andreas Westin <[email protected]>
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>
(...)
> /* Cryp DMA interface */
> +#define HASH_DMA_TX_FIFO 0x08
> +#define HASH_DMA_RX_FIFO 0x10
Yes, this is nice address notation :-)
> /**
> * struct cryp_device_data - structure for a cryp device.
> - * @base: Pointer to the hardware base address.
> + * @base: Pointer to virtual base address of the cryp device.
> + * @phybase: Pointer to physical memory location of the cryp device.
> * @dev: Pointer to the devices dev structure.
> * @clk: Pointer to the device's clock control.
> * @pwr_regulator: Pointer to the device's power control.
> @@ -232,6 +236,7 @@ struct cryp_dma {
> */
> struct cryp_device_data {
> struct cryp_register __iomem *base;
> + phys_addr_t phybase;
Use dma_addr_t. Maybe "phybase" is misleading,
"dmabase" is probably better. (Also applies to the
cryp patch).
Apart from that it looks allright.
Yours,
Linus Walleij
On Thu, 25 Apr 2013, Linus Walleij wrote:
> On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <[email protected]> wrote:
>
> > The DMA controller currently takes configuration information from
> > information passed though dma_channel_request(), but it shouldn't.
> > Using the API, the DMA channel should only be configured during
> > a dma_slave_config() call.
> >
> > Cc: Herbert Xu <[email protected]>
> > Cc: David S. Miller <[email protected]>
> > Cc: Andreas Westin <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Lee Jones <[email protected]>
>
> (...)
> > /* Cryp DMA interface */
> > +#define HASH_DMA_TX_FIFO 0x08
> > +#define HASH_DMA_RX_FIFO 0x10
>
> Yes, this is nice address notation :-)
>
> > /**
> > * struct cryp_device_data - structure for a cryp device.
> > - * @base: Pointer to the hardware base address.
> > + * @base: Pointer to virtual base address of the cryp device.
> > + * @phybase: Pointer to physical memory location of the cryp device.
> > * @dev: Pointer to the devices dev structure.
> > * @clk: Pointer to the device's clock control.
> > * @pwr_regulator: Pointer to the device's power control.
> > @@ -232,6 +236,7 @@ struct cryp_dma {
> > */
> > struct cryp_device_data {
> > struct cryp_register __iomem *base;
> > + phys_addr_t phybase;
>
> Use dma_addr_t. Maybe "phybase" is misleading,
> "dmabase" is probably better. (Also applies to the
> cryp patch).
Accept it's not the dmabase.
It's the phybase (U8500_CRYP1_BASE) i.e. the physical base address of
the device's regs.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, Apr 25, 2013 at 3:44 PM, Lee Jones <[email protected]> wrote:
> On Thu, 25 Apr 2013, Linus Walleij wrote:
>> On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <[email protected]> wrote:
>>
>> > The DMA controller currently takes configuration information from
>> > information passed though dma_channel_request(), but it shouldn't.
>> > Using the API, the DMA channel should only be configured during
>> > a dma_slave_config() call.
>> >
>> > Cc: Herbert Xu <[email protected]>
>> > Cc: David S. Miller <[email protected]>
>> > Cc: Andreas Westin <[email protected]>
>> > Cc: [email protected]
>> > Signed-off-by: Lee Jones <[email protected]>
>>
>> (...)
>> > /* Cryp DMA interface */
>> > +#define HASH_DMA_TX_FIFO 0x08
>> > +#define HASH_DMA_RX_FIFO 0x10
>>
>> Yes, this is nice address notation :-)
>>
>> > /**
>> > * struct cryp_device_data - structure for a cryp device.
>> > - * @base: Pointer to the hardware base address.
>> > + * @base: Pointer to virtual base address of the cryp device.
>> > + * @phybase: Pointer to physical memory location of the cryp device.
>> > * @dev: Pointer to the devices dev structure.
>> > * @clk: Pointer to the device's clock control.
>> > * @pwr_regulator: Pointer to the device's power control.
>> > @@ -232,6 +236,7 @@ struct cryp_dma {
>> > */
>> > struct cryp_device_data {
>> > struct cryp_register __iomem *base;
>> > + phys_addr_t phybase;
>>
>> Use dma_addr_t. Maybe "phybase" is misleading,
>> "dmabase" is probably better. (Also applies to the
>> cryp patch).
>
> Accept it's not the dmabase.
>
> It's the phybase (U8500_CRYP1_BASE) i.e. the physical base address of
> the device's regs.
So when you assign the src_addr or dst_addr in struct
dmaengine_slave_config in this code,
you notice that this looks like so:
struct dma_slave_config {
enum dma_transfer_direction direction;
dma_addr_t src_addr;
dma_addr_t dst_addr;
enum dma_slave_buswidth src_addr_width;
enum dma_slave_buswidth dst_addr_width;
u32 src_maxburst;
u32 dst_maxburst;
bool device_fc;
};
So when you do this:
+ struct dma_slave_config mem2cryp = {
+ .direction = DMA_MEM_TO_DEV,
+ .dst_addr = device_data->phybase + HASH_DMA_TX_FIFO,
+ .dst_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES,
+ .dst_maxburst = 4,
+ };
on .dst_addr you are effectively casting a phys_addr_t
into a dma_addr_t.
But we must do this at some point, and now I think
that doing it here may be just as good (because we might
just add a physical-to-dma memory translation if need
be).
So allright.
Yours,
Linus Walleij
On Thursday 25 April 2013, Lee Jones wrote:
> > > @@ -232,6 +236,7 @@ struct cryp_dma {
> > > */
> > > struct cryp_device_data {
> > > struct cryp_register __iomem *base;
> > > + phys_addr_t phybase;
> >
> > Use dma_addr_t. Maybe "phybase" is misleading,
> > "dmabase" is probably better. (Also applies to the
> > cryp patch).
>
> Accept it's not the dmabase.
>
> It's the phybase (U8500_CRYP1_BASE) i.e. the physical base address of
> the device's regs.
Right, this recently came up in a different context and I agree:
The dma engine driver must know the address in its dma space, while the
slave driver has it available in physical space. These two are often the
same, but there is no generic way to convert between the two, especially
if the dma engine resides behind an IOMMU.
The best assumption we can make is that the dma engine driver knows
how to convert between the two. Interestingly the documentation for
dma_slave_config talks about "physical address", while the structure
itself uses a dma_addr_t. Linus Walleij introduced the structure in
c156d0a5b0 "DMAENGINE: generic slave channel control v3", so I assume
he can shed some light on what he was thinking. I assume the documentation
is right but the structure is not and should be converted to use
phys_add_t or resource_size_t.
Arnd
On Thu, Apr 25, 2013 at 4:11 PM, Arnd Bergmann <[email protected]> wrote:
> The dma engine driver must know the address in its dma space, while the
> slave driver has it available in physical space. These two are often the
> same, but there is no generic way to convert between the two, especially
> if the dma engine resides behind an IOMMU.
>
> The best assumption we can make is that the dma engine driver knows
> how to convert between the two. Interestingly the documentation for
> dma_slave_config talks about "physical address", while the structure
> itself uses a dma_addr_t. Linus Walleij introduced the structure in
> c156d0a5b0 "DMAENGINE: generic slave channel control v3", so I assume
> he can shed some light on what he was thinking. I assume the documentation
> is right but the structure is not and should be converted to use
> phys_add_t or resource_size_t.
OK I could cook a patch for that, but I think I need some input from
Vinod and/or Russell on this.
It does make sense to me that if anything knows how to map a physical
address into the DMA address space it'll be the DMA engine.
However this rings a bell that there may be a possible relation to
DMA-API, since that API syncs memory buffers to the DMA
address space if there is some MMU inbetween the DMA and the
(ordinary, non-device) memory.
So if we think one step ahead, assuming the DMAC is actually behind
an MMU making it see the device in some other address than the
physical (bus) space, where would the address be resolved?
Yours,
Linus Walleij
On Fri, Apr 26, 2013 at 10:28:39AM +0200, Linus Walleij wrote:
> On Thu, Apr 25, 2013 at 4:11 PM, Arnd Bergmann <[email protected]> wrote:
>
> > The dma engine driver must know the address in its dma space, while the
> > slave driver has it available in physical space. These two are often the
> > same, but there is no generic way to convert between the two, especially
> > if the dma engine resides behind an IOMMU.
> >
> > The best assumption we can make is that the dma engine driver knows
> > how to convert between the two. Interestingly the documentation for
> > dma_slave_config talks about "physical address", while the structure
> > itself uses a dma_addr_t. Linus Walleij introduced the structure in
> > c156d0a5b0 "DMAENGINE: generic slave channel control v3", so I assume
> > he can shed some light on what he was thinking. I assume the documentation
> > is right but the structure is not and should be converted to use
> > phys_add_t or resource_size_t.
>
> OK I could cook a patch for that, but I think I need some input from
> Vinod and/or Russell on this.
the dma_slave_config is physical address that should be passed directly to the
controller. Obviosuly it should phys_addr_t :)
The mapping & unmapping of dma buffer (memcpy and memory buffer in this txn) is
required to be performed by the client driver. The dmanegine or dmaengine driver
will not do that for you...
This is true for slave usage and not for async case.
>
> It does make sense to me that if anything knows how to map a physical
> address into the DMA address space it'll be the DMA engine.
>
> However this rings a bell that there may be a possible relation to
> DMA-API, since that API syncs memory buffers to the DMA
> address space if there is some MMU inbetween the DMA and the
> (ordinary, non-device) memory.
>
> So if we think one step ahead, assuming the DMAC is actually behind
> an MMU making it see the device in some other address than the
> physical (bus) space, where would the address be resolved?
>
> Yours,
> Linus Walleij
On Fri, Apr 26, 2013 at 10:16 AM, Vinod Koul <[email protected]> wrote:
>> OK I could cook a patch for that, but I think I need some input from
>> Vinod and/or Russell on this.
> the dma_slave_config is physical address that should be passed directly to the
> controller. Obviosuly it should phys_addr_t :)
OK! Sent a patch for this, check it out.
Yours,
Linus Walleij
On Friday 26 April 2013 10:28:39 Linus Walleij wrote:
>
> However this rings a bell that there may be a possible relation to
> DMA-API, since that API syncs memory buffers to the DMA
> address space if there is some MMU inbetween the DMA and the
> (ordinary, non-device) memory.
>
> So if we think one step ahead, assuming the DMAC is actually behind
> an MMU making it see the device in some other address than the
> physical (bus) space, where would the address be resolved?
We don't currently have the infrastructure for that I think.
The dma-mapping API has some of the required parts but not all,
in particular it's only designed for mapping pages from the linear
kernel memory into the bus address space, not for devices.
The iommu API could do it for devices that have an IOMMU, but
it's not the best fit, because it does not abstract away the
presence of an IOMMU.
Another missing part is parsing the "dma-ranges" properties in
device tree, which you need to do if the address space translation
is not 1:1, and to find out which side of the IOMMU the DMA master
is connected to: if it's on the bus side, you need 1:1 mapping
and if it's on the host side, you need an IO page table entry.
Arnd
On Friday 26 April 2013 13:46:46 Vinod Koul wrote:
>
> The mapping & unmapping of dma buffer (memcpy and memory buffer in this txn) is
> required to be performed by the client driver. The dmanegine or dmaengine driver
> will not do that for you...
I've been wondering about this part: since we need to pass the 'struct device' of
the dma engine (rather than the slave) into dma_map_single, what is the official
way to do that? Should the slave driver just rely on dma_chan->device->dev to
work correctly for the purpose of dma-mapping.h interfaces?
Arnd
On Fri, Apr 26, 2013 at 01:46:46PM +0530, Vinod Koul wrote:
> On Fri, Apr 26, 2013 at 10:28:39AM +0200, Linus Walleij wrote:
> > On Thu, Apr 25, 2013 at 4:11 PM, Arnd Bergmann <[email protected]> wrote:
> >
> > > The dma engine driver must know the address in its dma space, while the
> > > slave driver has it available in physical space. These two are often the
> > > same, but there is no generic way to convert between the two, especially
> > > if the dma engine resides behind an IOMMU.
> > >
> > > The best assumption we can make is that the dma engine driver knows
> > > how to convert between the two. Interestingly the documentation for
> > > dma_slave_config talks about "physical address", while the structure
> > > itself uses a dma_addr_t. Linus Walleij introduced the structure in
> > > c156d0a5b0 "DMAENGINE: generic slave channel control v3", so I assume
> > > he can shed some light on what he was thinking. I assume the documentation
> > > is right but the structure is not and should be converted to use
> > > phys_add_t or resource_size_t.
> >
> > OK I could cook a patch for that, but I think I need some input from
> > Vinod and/or Russell on this.
> the dma_slave_config is physical address that should be passed directly to the
> controller. Obviosuly it should phys_addr_t :)
What you've just said is actually confusing.
"physical address" is normally the term used to describe the addresses
seen to the RAM. phys_addr_t describes this. This is not necessarily
what needs to be programmed into the DMA controller.
For RAM addresses, they must be mapped via the DMA API - and this gives
you a dma_addr_t.
"DMA address" is the address to be programmed into a DMA controller to
access a particular address in RAM or device, and has type dma_addr_t.
When you're programming a DMA controller to access a device, you are
clearly telling it the address on the _DMA controller's bus_ to access
that register, which may or may not be the same as the physical address.
There are platforms in existence where phys_addr_t can be 32-bit but
dma_addr_t can be 64-bit. Getting this stuff wrong can cause problems.
On Fri, Apr 26, 2013 at 11:39:20AM +0200, Arnd Bergmann wrote:
> On Friday 26 April 2013 13:46:46 Vinod Koul wrote:
> >
> > The mapping & unmapping of dma buffer (memcpy and memory buffer in this txn) is
> > required to be performed by the client driver. The dmanegine or dmaengine driver
> > will not do that for you...
>
> I've been wondering about this part: since we need to pass the 'struct device' of
> the dma engine (rather than the slave) into dma_map_single, what is the official
> way to do that? Should the slave driver just rely on dma_chan->device->dev to
> work correctly for the purpose of dma-mapping.h interfaces?
Yes.
On Fri, Apr 26, 2013 at 10:41:23AM +0100, Russell King - ARM Linux wrote:
> On Fri, Apr 26, 2013 at 01:46:46PM +0530, Vinod Koul wrote:
> > On Fri, Apr 26, 2013 at 10:28:39AM +0200, Linus Walleij wrote:
> > > On Thu, Apr 25, 2013 at 4:11 PM, Arnd Bergmann <[email protected]> wrote:
> > >
> > > > The dma engine driver must know the address in its dma space, while the
> > > > slave driver has it available in physical space. These two are often the
> > > > same, but there is no generic way to convert between the two, especially
> > > > if the dma engine resides behind an IOMMU.
> > > >
> > > > The best assumption we can make is that the dma engine driver knows
> > > > how to convert between the two. Interestingly the documentation for
> > > > dma_slave_config talks about "physical address", while the structure
> > > > itself uses a dma_addr_t. Linus Walleij introduced the structure in
> > > > c156d0a5b0 "DMAENGINE: generic slave channel control v3", so I assume
> > > > he can shed some light on what he was thinking. I assume the documentation
> > > > is right but the structure is not and should be converted to use
> > > > phys_add_t or resource_size_t.
> > >
> > > OK I could cook a patch for that, but I think I need some input from
> > > Vinod and/or Russell on this.
> > the dma_slave_config is physical address that should be passed directly to the
> > controller. Obviosuly it should phys_addr_t :)
>
> What you've just said is actually confusing.
>
> "physical address" is normally the term used to describe the addresses
> seen to the RAM. phys_addr_t describes this. This is not necessarily
> what needs to be programmed into the DMA controller.
Yes that would be true when you have MMU
>
> For RAM addresses, they must be mapped via the DMA API - and this gives
> you a dma_addr_t.
>
> "DMA address" is the address to be programmed into a DMA controller to
> access a particular address in RAM or device, and has type dma_addr_t.
> When you're programming a DMA controller to access a device, you are
> clearly telling it the address on the _DMA controller's bus_ to access
> that register, which may or may not be the same as the physical address.
>
> There are platforms in existence where phys_addr_t can be 32-bit but
> dma_addr_t can be 64-bit. Getting this stuff wrong can cause problems.
Sure, thanks for pointing, so we wont do this change.
--
~Vinod