| Message ID | 1341484183-10757-6-git-send-email-g.liakhovetski@gmx.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c2cdb7e4d16394fc51dc5c2c5b3e7c3733bdfaac |
| Headers | show |
On Thu, 2012-07-05 at 12:29 +0200, Guennadi Liakhovetski wrote: > diff --git a/include/linux/sh_dma.h b/include/linux/sh_dma.h > index a79f10a..4e83f3e 100644 > --- a/include/linux/sh_dma.h > +++ b/include/linux/sh_dma.h > @@ -27,10 +27,10 @@ struct sh_dmae_slave { > * a certain peripheral > */ > struct sh_dmae_slave_config { > - unsigned int slave_id; > - dma_addr_t addr; > - u32 chcr; > - char mid_rid; > + int slave_id; > + dma_addr_t addr; > + u32 chcr; > + char mid_rid; > }; why would you want to keep your own slave_id and addr fields when they are already provided in struct dma_slave_config. Also while at it, what would be the use of last tow fields, what do they describe? Would it be possible to get rid of this structure completely? At least first two fields should go away.
On Mon, 16 Jul 2012, Vinod Koul wrote: > On Thu, 2012-07-05 at 12:29 +0200, Guennadi Liakhovetski wrote: > > diff --git a/include/linux/sh_dma.h b/include/linux/sh_dma.h > > index a79f10a..4e83f3e 100644 > > --- a/include/linux/sh_dma.h > > +++ b/include/linux/sh_dma.h > > @@ -27,10 +27,10 @@ struct sh_dmae_slave { > > * a certain peripheral > > */ > > struct sh_dmae_slave_config { > > - unsigned int slave_id; > > - dma_addr_t addr; > > - u32 chcr; > > - char mid_rid; > > + int slave_id; > > + dma_addr_t addr; > > + u32 chcr; > > + char mid_rid; > > }; > why would you want to keep your own slave_id and addr fields when they > are already provided in struct dma_slave_config. Modifying the driver's API would break all its clients. > Also while at it, what > would be the use of last tow fields, what do they describe? They tell the driver how the channel has to be configured to support this specific client. They are values of two specific registers. In fact, CHCR means exactly that - CHannel Control Register. > Would it be possible to get rid of this structure completely? At least > first two fields should go away. Migration has to be performed gradually. If this approach and patches are accepted, then all client drivers can be migrated, then thr legacy API can be dropped. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-07-16 at 08:37 +0200, Guennadi Liakhovetski wrote: > On Mon, 16 Jul 2012, Vinod Koul wrote: > > > On Thu, 2012-07-05 at 12:29 +0200, Guennadi Liakhovetski wrote: > > > diff --git a/include/linux/sh_dma.h b/include/linux/sh_dma.h > > > index a79f10a..4e83f3e 100644 > > > --- a/include/linux/sh_dma.h > > > +++ b/include/linux/sh_dma.h > > > @@ -27,10 +27,10 @@ struct sh_dmae_slave { > > > * a certain peripheral > > > */ > > > struct sh_dmae_slave_config { > > > - unsigned int slave_id; > > > - dma_addr_t addr; > > > - u32 chcr; > > > - char mid_rid; > > > + int slave_id; > > > + dma_addr_t addr; > > > + u32 chcr; > > > + char mid_rid; > > > }; > > why would you want to keep your own slave_id and addr fields when they > > are already provided in struct dma_slave_config. > > Modifying the driver's API would break all its clients. rightly so, they need to be moved too. > > > Also while at it, what > > would be the use of last tow fields, what do they describe? > > They tell the driver how the channel has to be configured to support this > specific client. They are values of two specific registers. In fact, CHCR > means exactly that - CHannel Control Register. what exactly does the channel control register do in shdma? Should shdma driver deduce this value rather than client giving it? Same question for mid_rid?
On Mon, 16 Jul 2012, Vinod Koul wrote: > On Mon, 2012-07-16 at 08:37 +0200, Guennadi Liakhovetski wrote: > > On Mon, 16 Jul 2012, Vinod Koul wrote: > > > > > On Thu, 2012-07-05 at 12:29 +0200, Guennadi Liakhovetski wrote: > > > > diff --git a/include/linux/sh_dma.h b/include/linux/sh_dma.h > > > > index a79f10a..4e83f3e 100644 > > > > --- a/include/linux/sh_dma.h > > > > +++ b/include/linux/sh_dma.h > > > > @@ -27,10 +27,10 @@ struct sh_dmae_slave { > > > > * a certain peripheral > > > > */ > > > > struct sh_dmae_slave_config { > > > > - unsigned int slave_id; > > > > - dma_addr_t addr; > > > > - u32 chcr; > > > > - char mid_rid; > > > > + int slave_id; > > > > + dma_addr_t addr; > > > > + u32 chcr; > > > > + char mid_rid; > > > > }; > > > why would you want to keep your own slave_id and addr fields when they > > > are already provided in struct dma_slave_config. > > > > Modifying the driver's API would break all its clients. > rightly so, they need to be moved too. Sure, that's why patch 6/7 is "providing a migration path" for them. > > > Also while at it, what > > > would be the use of last tow fields, what do they describe? > > > > They tell the driver how the channel has to be configured to support this > > specific client. They are values of two specific registers. In fact, CHCR > > means exactly that - CHannel Control Register. > what exactly does the channel control register do in shdma? Should shdma > driver deduce this value rather than client giving it? > Same question for mid_rid? See, e.g., arch/arm/mach-shmobile/setup-sh7372.c::sh7372_dmae_slaves[]. Platforms are supplying these values with shdma driver platform data, together with slave IDs. Them when slaves request channels and supply their slave IDs, the driver searches the above array, looking for the matching slave ID, then it uses the rest of the information in those structs to configure the channel for this client. Again, this is nothing new, this is how the driver has been functioning since a long time, this driver is not modifying anything there. Any changes to this procedure, like providing all thig information from clients themselves instead of keeping it with DMACs, requires these patches to be committed first. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-07-16 at 09:13 +0200, Guennadi Liakhovetski wrote: > > > They tell the driver how the channel has to be configured to > support this > > > specific client. They are values of two specific registers. In > fact, CHCR > > > means exactly that - CHannel Control Register. > > what exactly does the channel control register do in shdma? Should > shdma > > driver deduce this value rather than client giving it? > > Same question for mid_rid? > > See, e.g., > arch/arm/mach-shmobile/setup-sh7372.c::sh7372_dmae_slaves[]. > Platforms are supplying these values with shdma driver platform data, > together with slave IDs. Them when slaves request channels and supply > their slave IDs, the driver searches the above array, looking for the > matching slave ID, then it uses the rest of the information in those > structs to configure the channel for this client. Again, this is > nothing > new, this is how the driver has been functioning since a long time, > this > driver is not modifying anything there. Any changes to this > procedure, > like providing all thig information from clients themselves instead > of > keeping it with DMACs, requires these patches to be committed first. That wasn't my question. I want to know what does ccr and mid_rid mean to dmac here?
On Mon, 16 Jul 2012, Vinod Koul wrote: > On Mon, 2012-07-16 at 09:13 +0200, Guennadi Liakhovetski wrote: > > > > They tell the driver how the channel has to be configured to > > support this > > > > specific client. They are values of two specific registers. In > > fact, CHCR > > > > means exactly that - CHannel Control Register. > > > what exactly does the channel control register do in shdma? Should > > shdma > > > driver deduce this value rather than client giving it? > > > Same question for mid_rid? > > > > See, e.g., > > arch/arm/mach-shmobile/setup-sh7372.c::sh7372_dmae_slaves[]. > > Platforms are supplying these values with shdma driver platform data, > > together with slave IDs. Them when slaves request channels and supply > > their slave IDs, the driver searches the above array, looking for the > > matching slave ID, then it uses the rest of the information in those > > structs to configure the channel for this client. Again, this is > > nothing > > new, this is how the driver has been functioning since a long time, > > this > > driver is not modifying anything there. Any changes to this > > procedure, > > like providing all thig information from clients themselves instead > > of > > keeping it with DMACs, requires these patches to be committed first. > That wasn't my question. > > I want to know what does ccr and mid_rid mean to dmac here? CHCR contains a few fields, some enable various interrupt sources, some specify repeat- and renew-modes, others yet specify transfer size, source and destination address-modes (incrementing, constant, decrementing), others yet select a DMA client category (slave / memcpy / ...), and a transfer flag. Some of these fields could be calculated, others are pre-defined for various slaves, the exact layout of those fields can also vary between SoCs. MID_RID is actually a slave-selector, it contains a magic value, that cannot be calculated. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-07-16 at 10:47 +0200, Guennadi Liakhovetski wrote: > > I want to know what does ccr and mid_rid mean to dmac here? > > CHCR contains a few fields, some enable various interrupt sources, some > specify repeat- and renew-modes, others yet specify transfer size, source > and destination address-modes (incrementing, constant, decrementing), > others yet select a DMA client category (slave / memcpy / ...), and a > transfer flag. Some of these fields could be calculated, others are > pre-defined for various slaves, the exact layout of those fields can also > vary between SoCs. I do not understand how clients would provide these values. For pre-defined values, they should be dmac property why should client like spi or mmc have clue about it? For others like you mentioned, i guess they could be easily calculated, right? > MID_RID is actually a slave-selector, it contains a magic value, that > cannot be calculated. and again, how does client know this?
On Mon, 16 Jul 2012, Vinod Koul wrote: > On Mon, 2012-07-16 at 10:47 +0200, Guennadi Liakhovetski wrote: > > > I want to know what does ccr and mid_rid mean to dmac here? > > > > CHCR contains a few fields, some enable various interrupt sources, some > > specify repeat- and renew-modes, others yet specify transfer size, source > > and destination address-modes (incrementing, constant, decrementing), > > others yet select a DMA client category (slave / memcpy / ...), and a > > transfer flag. Some of these fields could be calculated, others are > > pre-defined for various slaves, the exact layout of those fields can also > > vary between SoCs. > I do not understand how clients would provide these values. > For pre-defined values, they should be dmac property why should client > like spi or mmc have clue about it? > > For others like you mentioned, i guess they could be easily calculated, > right? > > > MID_RID is actually a slave-selector, it contains a magic value, that > > cannot be calculated. > and again, how does client know this? I might be misunderstanding you, but from earlier discussions I got an impression, that the DMAC should know nothing about clients, i.e., should receive no client-specific information from its platform data. Instead clients should provide it when configuring the channel. If this is correct, then the preferred way would be to specify these values in client platform data and then pass it to the DMAC with slave-config calls? Or have I misunderstood you and this per-client information should be kept in DMAC platform data? I.e., in both cases magic values are provided by platforms, the only difference is - either keep them in DMAC platform data or move them in each individual DMA client platform data. Yes, some CHCR fields can be calculated, but location and width of those fields might vary between DMAC versions, so, they will have to be passed with DMAC platform data. But this definitely can be done in the future. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-07-16 at 12:01 +0200, Guennadi Liakhovetski wrote: > On Mon, 16 Jul 2012, Vinod Koul wrote: > > > On Mon, 2012-07-16 at 10:47 +0200, Guennadi Liakhovetski wrote: > > > > I want to know what does ccr and mid_rid mean to dmac here? > > > > > > CHCR contains a few fields, some enable various interrupt sources, some > > > specify repeat- and renew-modes, others yet specify transfer size, source > > > and destination address-modes (incrementing, constant, decrementing), > > > others yet select a DMA client category (slave / memcpy / ...), and a > > > transfer flag. Some of these fields could be calculated, others are > > > pre-defined for various slaves, the exact layout of those fields can also > > > vary between SoCs. > > I do not understand how clients would provide these values. > > For pre-defined values, they should be dmac property why should client > > like spi or mmc have clue about it? > > > > For others like you mentioned, i guess they could be easily calculated, > > right? > > > > > MID_RID is actually a slave-selector, it contains a magic value, that > > > cannot be calculated. > > and again, how does client know this? > > I might be misunderstanding you, but from earlier discussions I got an > impression, that the DMAC should know nothing about clients, i.e., should > receive no client-specific information from its platform data. Instead > clients should provide it when configuring the channel. If this is > correct, then the preferred way would be to specify these values in client > platform data and then pass it to the DMAC with slave-config calls? Or > have I misunderstood you and this per-client information should be kept in > DMAC platform data? You haven't misunderstood me. dmacs should not know of client data and should be client agnostic. BUT, are these parameters ccr and mid_rid client data, i do not think so, they seem to be dmac controller parameters which you need to configure channel or is that not the case. If not why bother passing them to dmac? Either way something looks fishy to me. > > I.e., in both cases magic values are provided by platforms, the only > difference is - either keep them in DMAC platform data or move them in > each individual DMA client platform data. > > Yes, some CHCR fields can be calculated, but location and width of those > fields might vary between DMAC versions, so, they will have to be passed > with DMAC platform data. But this definitely can be done in the future.
On Mon, 16 Jul 2012, Vinod Koul wrote: > On Mon, 2012-07-16 at 12:01 +0200, Guennadi Liakhovetski wrote: > > On Mon, 16 Jul 2012, Vinod Koul wrote: > > > > > On Mon, 2012-07-16 at 10:47 +0200, Guennadi Liakhovetski wrote: > > > > > I want to know what does ccr and mid_rid mean to dmac here? > > > > > > > > CHCR contains a few fields, some enable various interrupt sources, some > > > > specify repeat- and renew-modes, others yet specify transfer size, source > > > > and destination address-modes (incrementing, constant, decrementing), > > > > others yet select a DMA client category (slave / memcpy / ...), and a > > > > transfer flag. Some of these fields could be calculated, others are > > > > pre-defined for various slaves, the exact layout of those fields can also > > > > vary between SoCs. > > > I do not understand how clients would provide these values. > > > For pre-defined values, they should be dmac property why should client > > > like spi or mmc have clue about it? > > > > > > For others like you mentioned, i guess they could be easily calculated, > > > right? > > > > > > > MID_RID is actually a slave-selector, it contains a magic value, that > > > > cannot be calculated. > > > and again, how does client know this? > > > > I might be misunderstanding you, but from earlier discussions I got an > > impression, that the DMAC should know nothing about clients, i.e., should > > receive no client-specific information from its platform data. Instead > > clients should provide it when configuring the channel. If this is > > correct, then the preferred way would be to specify these values in client > > platform data and then pass it to the DMAC with slave-config calls? Or > > have I misunderstood you and this per-client information should be kept in > > DMAC platform data? > You haven't misunderstood me. dmacs should not know of client data and > should be client agnostic. > > BUT, are these parameters ccr and mid_rid client data, i do not think > so, they seem to be dmac controller parameters which you need to > configure channel or is that not the case. If not why bother passing > them to dmac? Yes, that's right - these values have to be written to DMAC channel configuration registers, so, we do not have to change anything, those values can remain DMAC parameters and be passed to it directly from platform data. > Either way something looks fishy to me. You can always tell me what you don't like about the driver, but I don't see why this specific patch should cause any problem - it only changes the type of the slave-id variable from unsigned to signed to be able to pass negative error values in it too. > > I.e., in both cases magic values are provided by platforms, the only > > difference is - either keep them in DMAC platform data or move them in > > each individual DMA client platform data. > > > > Yes, some CHCR fields can be calculated, but location and width of those > > fields might vary between DMAC versions, so, they will have to be passed > > with DMAC platform data. But this definitely can be done in the future. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-07-16 at 12:55 +0200, Guennadi Liakhovetski wrote: > On Mon, 16 Jul 2012, Vinod Koul wrote: > > > On Mon, 2012-07-16 at 12:01 +0200, Guennadi Liakhovetski wrote: > > > On Mon, 16 Jul 2012, Vinod Koul wrote: > > > > > > > On Mon, 2012-07-16 at 10:47 +0200, Guennadi Liakhovetski wrote: > > > > > > I want to know what does ccr and mid_rid mean to dmac here? > > > > > > > > > > CHCR contains a few fields, some enable various interrupt sources, some > > > > > specify repeat- and renew-modes, others yet specify transfer size, source > > > > > and destination address-modes (incrementing, constant, decrementing), > > > > > others yet select a DMA client category (slave / memcpy / ...), and a > > > > > transfer flag. Some of these fields could be calculated, others are > > > > > pre-defined for various slaves, the exact layout of those fields can also > > > > > vary between SoCs. > > > > I do not understand how clients would provide these values. > > > > For pre-defined values, they should be dmac property why should client > > > > like spi or mmc have clue about it? > > > > > > > > For others like you mentioned, i guess they could be easily calculated, > > > > right? > > > > > > > > > MID_RID is actually a slave-selector, it contains a magic value, that > > > > > cannot be calculated. > > > > and again, how does client know this? > > > > > > I might be misunderstanding you, but from earlier discussions I got an > > > impression, that the DMAC should know nothing about clients, i.e., should > > > receive no client-specific information from its platform data. Instead > > > clients should provide it when configuring the channel. If this is > > > correct, then the preferred way would be to specify these values in client > > > platform data and then pass it to the DMAC with slave-config calls? Or > > > have I misunderstood you and this per-client information should be kept in > > > DMAC platform data? > > You haven't misunderstood me. dmacs should not know of client data and > > should be client agnostic. > > > > BUT, are these parameters ccr and mid_rid client data, i do not think > > so, they seem to be dmac controller parameters which you need to > > configure channel or is that not the case. If not why bother passing > > them to dmac? > > Yes, that's right - these values have to be written to DMAC channel > configuration registers, so, we do not have to change anything, those > values can remain DMAC parameters and be passed to it directly from > platform data. Can you get that in future fixes. > > > Either way something looks fishy to me. > > You can always tell me what you don't like about the driver, but I don't > see why this specific patch should cause any problem - it only changes the > type of the slave-id variable from unsigned to signed to be able to pass > negative error values in it too. This question was not specific to this patchset Btw I am quite happy with this patchset. Good direction for this and thanks for taking it up.
On Mon, 16 Jul 2012, Vinod Koul wrote: > On Mon, 2012-07-16 at 12:55 +0200, Guennadi Liakhovetski wrote: > > On Mon, 16 Jul 2012, Vinod Koul wrote: > > > > > On Mon, 2012-07-16 at 12:01 +0200, Guennadi Liakhovetski wrote: > > > > On Mon, 16 Jul 2012, Vinod Koul wrote: > > > > > > > > > On Mon, 2012-07-16 at 10:47 +0200, Guennadi Liakhovetski wrote: > > > > > > > I want to know what does ccr and mid_rid mean to dmac here? > > > > > > > > > > > > CHCR contains a few fields, some enable various interrupt sources, some > > > > > > specify repeat- and renew-modes, others yet specify transfer size, source > > > > > > and destination address-modes (incrementing, constant, decrementing), > > > > > > others yet select a DMA client category (slave / memcpy / ...), and a > > > > > > transfer flag. Some of these fields could be calculated, others are > > > > > > pre-defined for various slaves, the exact layout of those fields can also > > > > > > vary between SoCs. > > > > > I do not understand how clients would provide these values. > > > > > For pre-defined values, they should be dmac property why should client > > > > > like spi or mmc have clue about it? > > > > > > > > > > For others like you mentioned, i guess they could be easily calculated, > > > > > right? > > > > > > > > > > > MID_RID is actually a slave-selector, it contains a magic value, that > > > > > > cannot be calculated. > > > > > and again, how does client know this? > > > > > > > > I might be misunderstanding you, but from earlier discussions I got an > > > > impression, that the DMAC should know nothing about clients, i.e., should > > > > receive no client-specific information from its platform data. Instead > > > > clients should provide it when configuring the channel. If this is > > > > correct, then the preferred way would be to specify these values in client > > > > platform data and then pass it to the DMAC with slave-config calls? Or > > > > have I misunderstood you and this per-client information should be kept in > > > > DMAC platform data? > > > You haven't misunderstood me. dmacs should not know of client data and > > > should be client agnostic. > > > > > > BUT, are these parameters ccr and mid_rid client data, i do not think > > > so, they seem to be dmac controller parameters which you need to > > > configure channel or is that not the case. If not why bother passing > > > them to dmac? > > > > Yes, that's right - these values have to be written to DMAC channel > > configuration registers, so, we do not have to change anything, those > > values can remain DMAC parameters and be passed to it directly from > > platform data. > Can you get that in future fixes. Sorry, what exactly would you like to have fixed here? Above I just described how the driver already functions, what changes do you see necessary? > > > Either way something looks fishy to me. > > > > You can always tell me what you don't like about the driver, but I don't > > see why this specific patch should cause any problem - it only changes the > > type of the slave-id variable from unsigned to signed to be able to pass > > negative error values in it too. > This question was not specific to this patchset > > Btw I am quite happy with this patchset. Good direction for this and > thanks for taking it up. Thanks! That's very good to know! Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-07-16 at 14:47 +0200, Guennadi Liakhovetski wrote: > > > Yes, that's right - these values have to be written to DMAC channel > > > configuration registers, so, we do not have to change anything, those > > > values can remain DMAC parameters and be passed to it directly from > > > platform data. > > Can you get that in future fixes. > > Sorry, what exactly would you like to have fixed here? Above I just > described how the driver already functions, what changes do you see > necessary? Why should client pass these two values. So the parameters which can be calculated should be, and rest should dmacs platform data and not clients. And after the removal of the slave and adddr fields, you find that you no longer need your specific slave structure and that can be elimnated completely.
Hi Vinod On Wed, 18 Jul 2012, Vinod Koul wrote: > On Mon, 2012-07-16 at 14:47 +0200, Guennadi Liakhovetski wrote: > > > > Yes, that's right - these values have to be written to DMAC channel > > > > configuration registers, so, we do not have to change anything, those > > > > values can remain DMAC parameters and be passed to it directly from > > > > platform data. > > > Can you get that in future fixes. > > > > Sorry, what exactly would you like to have fixed here? Above I just > > described how the driver already functions, what changes do you see > > necessary? > Why should client pass these two values. So the parameters which can be > calculated should be, and rest should dmacs platform data and not > clients. Hm, let me try again: > > > > those > > > > values can remain DMAC parameters and be passed to it directly from > > > > platform data. i.e., from DMAC's platform data, they are _not_ passed from clients. Am I still missing something? > And after the removal of the slave and adddr fields, you find that you > no longer need your specific slave structure and that can be elimnated > completely. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/sh/shdma-base.c b/drivers/dma/sh/shdma-base.c index f75ebfa..73db282 100644 --- a/drivers/dma/sh/shdma-base.c +++ b/drivers/dma/sh/shdma-base.c @@ -76,7 +76,6 @@ static dma_cookie_t shdma_tx_submit(struct dma_async_tx_descriptor *tx) container_of(tx, struct shdma_desc, async_tx), *last = desc; struct shdma_chan *schan = to_shdma_chan(tx->chan); - struct shdma_slave *slave = schan->slave; dma_async_tx_callback callback = tx->callback; dma_cookie_t cookie; bool power_up; @@ -138,7 +137,7 @@ static dma_cookie_t shdma_tx_submit(struct dma_async_tx_descriptor *tx) * Make it int then, on error remove chunks from the * queue again */ - ops->setup_xfer(schan, slave); + ops->setup_xfer(schan, schan->slave_id); if (schan->pm_state == SHDMA_PM_PENDING) shdma_chan_xfer_ld_queue(schan); @@ -186,7 +185,7 @@ static int shdma_alloc_chan_resources(struct dma_chan *chan) * never runs concurrently with itself or free_chan_resources. */ if (slave) { - if (slave->slave_id >= slave_num) { + if (slave->slave_id < 0 || slave->slave_id >= slave_num) { ret = -EINVAL; goto evalid; } @@ -196,9 +195,13 @@ static int shdma_alloc_chan_resources(struct dma_chan *chan) goto etestused; } - ret = ops->set_slave(schan, slave); + ret = ops->set_slave(schan, slave->slave_id); if (ret < 0) goto esetslave; + + schan->slave_id = slave->slave_id; + } else { + schan->slave_id = -EINVAL; } schan->desc = kcalloc(NR_DESCS_PER_CHANNEL, @@ -208,7 +211,6 @@ static int shdma_alloc_chan_resources(struct dma_chan *chan) goto edescalloc; } schan->desc_num = NR_DESCS_PER_CHANNEL; - schan->slave = slave; for (i = 0; i < NR_DESCS_PER_CHANNEL; i++) { desc = ops->embedded_desc(schan->desc, i); @@ -366,10 +368,9 @@ static void shdma_free_chan_resources(struct dma_chan *chan) if (!list_empty(&schan->ld_queue)) shdma_chan_ld_cleanup(schan, true); - if (schan->slave) { + if (schan->slave_id >= 0) { /* The caller is holding dma_list_mutex */ - struct shdma_slave *slave = schan->slave; - clear_bit(slave->slave_id, shdma_slave_used); + clear_bit(schan->slave_id, shdma_slave_used); chan->private = NULL; } @@ -559,7 +560,7 @@ static struct dma_async_tx_descriptor *shdma_prep_slave_sg( struct shdma_chan *schan = to_shdma_chan(chan); struct shdma_dev *sdev = to_shdma_dev(schan->dma_chan.device); const struct shdma_ops *ops = sdev->ops; - struct shdma_slave *slave = schan->slave; + int slave_id = schan->slave_id; dma_addr_t slave_addr; if (!chan) @@ -568,9 +569,9 @@ static struct dma_async_tx_descriptor *shdma_prep_slave_sg( BUG_ON(!schan->desc_num); /* Someone calling slave DMA on a generic channel? */ - if (!slave || !sg_len) { - dev_warn(schan->dev, "%s: bad parameter: %p, %d, %d\n", - __func__, slave, sg_len, slave ? slave->slave_id : -1); + if (slave_id < 0 || !sg_len) { + dev_warn(schan->dev, "%s: bad parameter: len=%d, id=%d\n", + __func__, sg_len, slave_id); return NULL; } diff --git a/drivers/dma/sh/shdma.c b/drivers/dma/sh/shdma.c index 9f0a2e5..9a10d8b 100644 --- a/drivers/dma/sh/shdma.c +++ b/drivers/dma/sh/shdma.c @@ -285,12 +285,12 @@ static bool sh_dmae_channel_busy(struct shdma_chan *schan) } static void sh_dmae_setup_xfer(struct shdma_chan *schan, - struct shdma_slave *sslave) + int slave_id) { struct sh_dmae_chan *sh_chan = container_of(schan, struct sh_dmae_chan, shdma_chan); - if (sslave) { + if (slave_id >= 0) { const struct sh_dmae_slave_config *cfg = sh_chan->config; @@ -302,7 +302,7 @@ static void sh_dmae_setup_xfer(struct shdma_chan *schan, } static const struct sh_dmae_slave_config *dmae_find_slave( - struct sh_dmae_chan *sh_chan, unsigned int slave_id) + struct sh_dmae_chan *sh_chan, int slave_id) { struct sh_dmae_device *shdev = to_sh_dev(sh_chan); struct sh_dmae_pdata *pdata = shdev->pdata; @@ -320,11 +320,11 @@ static const struct sh_dmae_slave_config *dmae_find_slave( } static int sh_dmae_set_slave(struct shdma_chan *schan, - struct shdma_slave *sslave) + int slave_id) { struct sh_dmae_chan *sh_chan = container_of(schan, struct sh_dmae_chan, shdma_chan); - const struct sh_dmae_slave_config *cfg = dmae_find_slave(sh_chan, sslave->slave_id); + const struct sh_dmae_slave_config *cfg = dmae_find_slave(sh_chan, slave_id); if (!cfg) return -ENODEV; @@ -579,7 +579,7 @@ static int sh_dmae_resume(struct device *dev) if (!sh_chan->shdma_chan.desc_num) continue; - if (sh_chan->shdma_chan.slave) { + if (sh_chan->shdma_chan.slave_id >= 0) { const struct sh_dmae_slave_config *cfg = sh_chan->config; dmae_set_dmars(sh_chan, cfg->mid_rid); dmae_set_chcr(sh_chan, cfg->chcr); diff --git a/include/linux/sh_dma.h b/include/linux/sh_dma.h index a79f10a..4e83f3e 100644 --- a/include/linux/sh_dma.h +++ b/include/linux/sh_dma.h @@ -27,10 +27,10 @@ struct sh_dmae_slave { * a certain peripheral */ struct sh_dmae_slave_config { - unsigned int slave_id; - dma_addr_t addr; - u32 chcr; - char mid_rid; + int slave_id; + dma_addr_t addr; + u32 chcr; + char mid_rid; }; struct sh_dmae_channel { diff --git a/include/linux/shdma-base.h b/include/linux/shdma-base.h index c3a19e9..6263ad2 100644 --- a/include/linux/shdma-base.h +++ b/include/linux/shdma-base.h @@ -43,7 +43,7 @@ struct device; */ struct shdma_slave { - unsigned int slave_id; + int slave_id; }; struct shdma_desc { @@ -66,7 +66,7 @@ struct shdma_chan { size_t max_xfer_len; /* max transfer length */ int id; /* Raw id of this channel */ int irq; /* Channel IRQ */ - struct shdma_slave *slave; /* Client data for slave DMA */ + int slave_id; /* Client ID for slave DMA */ enum shdma_pm_state pm_state; }; @@ -93,8 +93,8 @@ struct shdma_ops { dma_addr_t (*slave_addr)(struct shdma_chan *); int (*desc_setup)(struct shdma_chan *, struct shdma_desc *, dma_addr_t, dma_addr_t, size_t *); - int (*set_slave)(struct shdma_chan *, struct shdma_slave *); - void (*setup_xfer)(struct shdma_chan *, struct shdma_slave *); + int (*set_slave)(struct shdma_chan *, int); + void (*setup_xfer)(struct shdma_chan *, int); void (*start_xfer)(struct shdma_chan *, struct shdma_desc *); struct shdma_desc *(*embedded_desc)(void *, int); bool (*chan_irq)(struct shdma_chan *, int);
Initially struct shdma_slave has been introduced with the only member - an unsigned slave ID - to describe common properties of DMA slaves in an extensible way. However, experience shows, that a slave ID is indeed the only parameter, needed to identify DMA slaves. This is also, what is used by the core dmaengine API in struct dma_slave_config. We switch to using the slave_id directly, instead of passing a pointer to struct shdma_slave to improve compatibility with the core. We also make the slave_id signed for easier error checking. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- drivers/dma/sh/shdma-base.c | 25 +++++++++++++------------ drivers/dma/sh/shdma.c | 12 ++++++------ include/linux/sh_dma.h | 8 ++++---- include/linux/shdma-base.h | 8 ++++---- 4 files changed, 27 insertions(+), 26 deletions(-)