| Message ID | 20240930145955.4248-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | dmaengine: sh: rz-dmac: add r7s72100 support | expand |
Hi Wolfram Sang, Thanks for the patch. > -----Original Message----- > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > Sent: Monday, September 30, 2024 4:00 PM > Subject: [PATCH 1/3] dmaengine: sh: rz-dmac: handle configs where one address is zero > > Configs like the ones coming from the MMC subsystem will have either 'src' or 'dst' zeroed, resulting > in an unknown bus width. This will bail out on the RZ DMA driver because of the sanity check for a > valid bus width. Reorder the code, so that the check will only be applied when the corresponding > address is non-zero. > > Fixes: 5000d37042a6 ("dmaengine: sh: Add DMAC driver for RZ/G2L SoC") > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/dma/sh/rz-dmac.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c index 65a27c5a7bce..811389fc9cb8 > 100644 > --- a/drivers/dma/sh/rz-dmac.c > +++ b/drivers/dma/sh/rz-dmac.c > @@ -601,22 +601,25 @@ static int rz_dmac_config(struct dma_chan *chan, > struct rz_dmac_chan *channel = to_rz_dmac_chan(chan); > u32 val; > > - channel->src_per_address = config->src_addr; > channel->dst_per_address = config->dst_addr; > - > - val = rz_dmac_ds_to_val_mapping(config->dst_addr_width); > - if (val == CHCFG_DS_INVALID) > - return -EINVAL; > - > channel->chcfg &= ~CHCFG_FILL_DDS_MASK; > - channel->chcfg |= FIELD_PREP(CHCFG_FILL_DDS_MASK, val); > + if (channel->dst_per_address) { > + val = rz_dmac_ds_to_val_mapping(config->dst_addr_width); > + if (val == CHCFG_DS_INVALID) > + return -EINVAL; > > - val = rz_dmac_ds_to_val_mapping(config->src_addr_width); > - if (val == CHCFG_DS_INVALID) > - return -EINVAL; > + channel->chcfg |= FIELD_PREP(CHCFG_FILL_DDS_MASK, val); > + } > > + channel->src_per_address = config->src_addr; > channel->chcfg &= ~CHCFG_FILL_SDS_MASK; > - channel->chcfg |= FIELD_PREP(CHCFG_FILL_SDS_MASK, val); > + if (channel->src_per_address) { > + val = rz_dmac_ds_to_val_mapping(config->src_addr_width); > + if (val == CHCFG_DS_INVALID) > + return -EINVAL; > + > + channel->chcfg |= FIELD_PREP(CHCFG_FILL_SDS_MASK, val); > + } Now both code paths are identical, not sure may be introducing a helper by passing channel, CHCFG_FILL_*_MASK and *_addr_width will save some code?? Anyway, current code LGTM. So, Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> Cheers, Biju > > return 0; > } > -- > 2.45.2
> Now both code paths are identical, not sure may be introducing a helper > by passing channel, CHCFG_FILL_*_MASK and *_addr_width > will save some code?? I had a look and I don't think so because we'd need to pass so many parameters to the helper, that it doesn't really save anything, I'd say. > Anyway, current code LGTM. So, > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks!
On 30.09.2024 17:59, Wolfram Sang wrote: > Configs like the ones coming from the MMC subsystem will have either > 'src' or 'dst' zeroed, resulting in an unknown bus width. This will bail > out on the RZ DMA driver because of the sanity check for a valid bus > width. Reorder the code, so that the check will only be applied when the > corresponding address is non-zero. > > Fixes: 5000d37042a6 ("dmaengine: sh: Add DMAC driver for RZ/G2L SoC") > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/dma/sh/rz-dmac.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c > index 65a27c5a7bce..811389fc9cb8 100644 > --- a/drivers/dma/sh/rz-dmac.c > +++ b/drivers/dma/sh/rz-dmac.c > @@ -601,22 +601,25 @@ static int rz_dmac_config(struct dma_chan *chan, > struct rz_dmac_chan *channel = to_rz_dmac_chan(chan); > u32 val; > > - channel->src_per_address = config->src_addr; > channel->dst_per_address = config->dst_addr; > - > - val = rz_dmac_ds_to_val_mapping(config->dst_addr_width); > - if (val == CHCFG_DS_INVALID) > - return -EINVAL; > - > channel->chcfg &= ~CHCFG_FILL_DDS_MASK; > - channel->chcfg |= FIELD_PREP(CHCFG_FILL_DDS_MASK, val); > + if (channel->dst_per_address) { > + val = rz_dmac_ds_to_val_mapping(config->dst_addr_width); > + if (val == CHCFG_DS_INVALID) > + return -EINVAL; > > - val = rz_dmac_ds_to_val_mapping(config->src_addr_width); > - if (val == CHCFG_DS_INVALID) > - return -EINVAL; > + channel->chcfg |= FIELD_PREP(CHCFG_FILL_DDS_MASK, val); > + } > > + channel->src_per_address = config->src_addr; > channel->chcfg &= ~CHCFG_FILL_SDS_MASK; > - channel->chcfg |= FIELD_PREP(CHCFG_FILL_SDS_MASK, val); > + if (channel->src_per_address) { > + val = rz_dmac_ds_to_val_mapping(config->src_addr_width); > + if (val == CHCFG_DS_INVALID) > + return -EINVAL; > + > + channel->chcfg |= FIELD_PREP(CHCFG_FILL_SDS_MASK, val); > + } > > return 0; > }
Hi Wolfram Sang, > -----Original Message----- > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > Sent: Monday, September 30, 2024 8:25 PM > Subject: Re: [PATCH 1/3] dmaengine: sh: rz-dmac: handle configs where one address is zero > > > > Now both code paths are identical, not sure may be introducing a > > helper by passing channel, CHCFG_FILL_*_MASK and *_addr_width will > > save some code?? > > I had a look and I don't think so because we'd need to pass so many parameters to the helper, that it > doesn't really save anything, I'd say. OK, fine by me. I just meant to avoid code duplication as it is identical blocks. Current changes are tested with RZ/G2L on with RSPI and SSI interfaces. So, Tested-by: Biju Das <biju.das.jz@bp.renesas.com> Cheers, Biju
diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c index 65a27c5a7bce..811389fc9cb8 100644 --- a/drivers/dma/sh/rz-dmac.c +++ b/drivers/dma/sh/rz-dmac.c @@ -601,22 +601,25 @@ static int rz_dmac_config(struct dma_chan *chan, struct rz_dmac_chan *channel = to_rz_dmac_chan(chan); u32 val; - channel->src_per_address = config->src_addr; channel->dst_per_address = config->dst_addr; - - val = rz_dmac_ds_to_val_mapping(config->dst_addr_width); - if (val == CHCFG_DS_INVALID) - return -EINVAL; - channel->chcfg &= ~CHCFG_FILL_DDS_MASK; - channel->chcfg |= FIELD_PREP(CHCFG_FILL_DDS_MASK, val); + if (channel->dst_per_address) { + val = rz_dmac_ds_to_val_mapping(config->dst_addr_width); + if (val == CHCFG_DS_INVALID) + return -EINVAL; - val = rz_dmac_ds_to_val_mapping(config->src_addr_width); - if (val == CHCFG_DS_INVALID) - return -EINVAL; + channel->chcfg |= FIELD_PREP(CHCFG_FILL_DDS_MASK, val); + } + channel->src_per_address = config->src_addr; channel->chcfg &= ~CHCFG_FILL_SDS_MASK; - channel->chcfg |= FIELD_PREP(CHCFG_FILL_SDS_MASK, val); + if (channel->src_per_address) { + val = rz_dmac_ds_to_val_mapping(config->src_addr_width); + if (val == CHCFG_DS_INVALID) + return -EINVAL; + + channel->chcfg |= FIELD_PREP(CHCFG_FILL_SDS_MASK, val); + } return 0; }
Configs like the ones coming from the MMC subsystem will have either 'src' or 'dst' zeroed, resulting in an unknown bus width. This will bail out on the RZ DMA driver because of the sanity check for a valid bus width. Reorder the code, so that the check will only be applied when the corresponding address is non-zero. Fixes: 5000d37042a6 ("dmaengine: sh: Add DMAC driver for RZ/G2L SoC") Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/dma/sh/rz-dmac.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)