| Message ID | 20230306140430.28951-3-walker.chen@starfivetech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add DMA driver for StarFive JH7110 SoC | expand |
On Mon, 6 Mar 2023 at 15:04, Walker Chen <walker.chen@starfivetech.com> wrote: > > Add dma reset operation in device probe and use different configuration > on CH_CFG registers according to match data. Update all uses of > of_device_is_compatible with of_device_get_match_data. > > Reviewed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com> Hi Walker, Again please remove my Reviewed-by when you're adding a bunch of new code as you're doing here. > Signed-off-by: Walker Chen <walker.chen@starfivetech.com> > --- > .../dma/dw-axi-dmac/dw-axi-dmac-platform.c | 67 ++++++++++++++++--- > drivers/dma/dw-axi-dmac/dw-axi-dmac.h | 2 + > 2 files changed, 61 insertions(+), 8 deletions(-) > > diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c > index bf85aa0979ec..d1148f6fbcf9 100644 > --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c > +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c > @@ -21,10 +21,12 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/of_dma.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/property.h> > +#include <linux/reset.h> > #include <linux/slab.h> > #include <linux/types.h> > > @@ -46,6 +48,12 @@ > DMA_SLAVE_BUSWIDTH_32_BYTES | \ > DMA_SLAVE_BUSWIDTH_64_BYTES) > > +struct axi_dma_chip_config { > + int (*apb_setup)(struct platform_device *pdev, struct axi_dma_chip *chip); > + int (*reset_init)(struct platform_device *pdev); > + bool use_cfg2; > +}; > + > static inline void > axi_dma_iowrite32(struct axi_dma_chip *chip, u32 reg, u32 val) > { > @@ -86,7 +94,8 @@ static inline void axi_chan_config_write(struct axi_dma_chan *chan, > > cfg_lo = (config->dst_multblk_type << CH_CFG_L_DST_MULTBLK_TYPE_POS | > config->src_multblk_type << CH_CFG_L_SRC_MULTBLK_TYPE_POS); > - if (chan->chip->dw->hdata->reg_map_8_channels) { > + if (chan->chip->dw->hdata->reg_map_8_channels && > + !chan->chip->dw->hdata->use_cfg2) { > cfg_hi = config->tt_fc << CH_CFG_H_TT_FC_POS | > config->hs_sel_src << CH_CFG_H_HS_SEL_SRC_POS | > config->hs_sel_dst << CH_CFG_H_HS_SEL_DST_POS | > @@ -1142,7 +1151,7 @@ static int dma_chan_terminate_all(struct dma_chan *dchan) > axi_chan_disable(chan); > > ret = readl_poll_timeout_atomic(chan->chip->regs + DMAC_CHEN, val, > - !(val & chan_active), 1000, 10000); > + !(val & chan_active), 1000, DMAC_TIMEOUT_US); > if (ret == -ETIMEDOUT) > dev_warn(dchan2dev(dchan), > "%s failed to stop\n", axi_chan_name(chan)); > @@ -1367,13 +1376,33 @@ static int parse_device_properties(struct axi_dma_chip *chip) > return 0; > } > > +static int intel_apb_setup(struct platform_device *pdev, struct axi_dma_chip *chip) > +{ > + chip->apb_regs = devm_platform_ioremap_resource(pdev, 1); > + if (IS_ERR(chip->apb_regs)) > + return PTR_ERR(chip->apb_regs); > + else > + return 0; > +} > + > +static int jh7110_reset_init(struct platform_device *pdev) > +{ > + struct reset_control *resets; > + > + resets = devm_reset_control_array_get_exclusive(&pdev->dev); > + if (IS_ERR(resets)) > + return PTR_ERR(resets); > + > + return reset_control_deassert(resets); > +} > + > static int dw_probe(struct platform_device *pdev) > { > - struct device_node *node = pdev->dev.of_node; > struct axi_dma_chip *chip; > struct resource *mem; > struct dw_axi_dma *dw; > struct dw_axi_dma_hcfg *hdata; > + const struct axi_dma_chip_config *ccfg; > u32 i; > int ret; > > @@ -1402,10 +1431,21 @@ static int dw_probe(struct platform_device *pdev) > if (IS_ERR(chip->regs)) > return PTR_ERR(chip->regs); > > - if (of_device_is_compatible(node, "intel,kmb-axi-dma")) { > - chip->apb_regs = devm_platform_ioremap_resource(pdev, 1); > - if (IS_ERR(chip->apb_regs)) > - return PTR_ERR(chip->apb_regs); > + ccfg = of_device_get_match_data(&pdev->dev); > + if (ccfg) { > + if (ccfg->apb_setup) { > + ret = ccfg->apb_setup(pdev, chip); > + if (ret) > + return ret; > + } > + > + if (ccfg->reset_init) { > + ret = ccfg->reset_init(pdev); > + if (ret) > + return ret; > + } > + > + chip->dw->hdata->use_cfg2 = ccfg->use_cfg2; This claims and deasserts the resets before the clocks, whereas your previous versions did it after turning the clocks on. Which is the correct order? Also this certainly gets rid of of_device_is_compatible calls, but seems like a lot of code to do that. Did you consider something like +#define AXI_DMA_FLAG_HAS_APB_REGS BIT(0) +#define AXI_DMA_FLAG_HAS_RESETS BIT(1) +#define AXI_DMA_FLAG_USE_CFG2 BIT(2) +unsigned int flags = (uintptr_t)device_get_match_data(&pdev->dev); -if (of_device_is_compatible(node, "intel,kmb-axi-dma")) { +if (flags & AXI_DMA_FLAG_HAS_APB_REGS) { -if (of_device_is_compatible(node, "starfive,jh7110-axi-dma)) { +if (flags & AXI_DMA_FLAG_HAS_RESETS) { +chip->dw->hwdata->use_cfg2 = !!(flags & AXI_DMA_FLAG_USE_CFG2); -{ .compatible = "intel,kmb-axi-dma" }, +{ .compatible = "intel,kmb-axi-dma", .data = (void *)AXI_DMA_FLAG_HAS_APB_REGS }, +{ .compatible = "starive,jh7110-axi-dma", .data = (void *)(AXI_DMA_FLAG_HAS_RESETS | AXI_DMA_FLAG_USE_CFG2) }, > } > > chip->core_clk = devm_clk_get(chip->dev, "core-clk"); > @@ -1557,9 +1597,20 @@ static const struct dev_pm_ops dw_axi_dma_pm_ops = { > SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend, axi_dma_runtime_resume, NULL) > }; > > +static const struct axi_dma_chip_config intel_chip_config = { > + .apb_setup = intel_apb_setup, > + .use_cfg2 = false, > +}; > + > +static const struct axi_dma_chip_config jh7110_chip_config = { > + .reset_init = jh7110_reset_init, > + .use_cfg2 = true, > +}; > + > static const struct of_device_id dw_dma_of_id_table[] = { > { .compatible = "snps,axi-dma-1.01a" }, > - { .compatible = "intel,kmb-axi-dma" }, > + { .compatible = "intel,kmb-axi-dma", .data = &intel_chip_config }, > + { .compatible = "starfive,jh7110-axi-dma", .data = &jh7110_chip_config }, > {} > }; > MODULE_DEVICE_TABLE(of, dw_dma_of_id_table); > diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac.h b/drivers/dma/dw-axi-dmac/dw-axi-dmac.h > index e9d5eb0fd594..b906d5884efe 100644 > --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac.h > +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac.h > @@ -21,6 +21,7 @@ > #define DMAC_MAX_CHANNELS 16 > #define DMAC_MAX_MASTERS 2 > #define DMAC_MAX_BLK_SIZE 0x200000 > +#define DMAC_TIMEOUT_US 200000 > > struct dw_axi_dma_hcfg { > u32 nr_channels; > @@ -33,6 +34,7 @@ struct dw_axi_dma_hcfg { > /* Register map for DMAX_NUM_CHANNELS <= 8 */ > bool reg_map_8_channels; > bool restrict_axi_burst_len; > + bool use_cfg2; > }; > > struct axi_dma_chan { > -- > 2.17.1 >
On 2023/3/6 22:39, Emil Renner Berthing wrote: > On Mon, 6 Mar 2023 at 15:04, Walker Chen <walker.chen@starfivetech.com> wrote: >> >> Add dma reset operation in device probe and use different configuration >> on CH_CFG registers according to match data. Update all uses of >> of_device_is_compatible with of_device_get_match_data. >> >> Reviewed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com> > > Hi Walker, > > Again please remove my Reviewed-by when you're adding a bunch of new > code as you're doing here. Sorry, I made another simple mistake. > >> Signed-off-by: Walker Chen <walker.chen@starfivetech.com> >> --- >> .../dma/dw-axi-dmac/dw-axi-dmac-platform.c | 67 ++++++++++++++++--- >> drivers/dma/dw-axi-dmac/dw-axi-dmac.h | 2 + >> 2 files changed, 61 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c >> index bf85aa0979ec..d1148f6fbcf9 100644 >> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c >> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c >> @@ -21,10 +21,12 @@ >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/of.h> >> +#include <linux/of_device.h> >> #include <linux/of_dma.h> >> #include <linux/platform_device.h> >> #include <linux/pm_runtime.h> >> #include <linux/property.h> >> +#include <linux/reset.h> >> #include <linux/slab.h> >> #include <linux/types.h> >> >> @@ -46,6 +48,12 @@ >> DMA_SLAVE_BUSWIDTH_32_BYTES | \ >> DMA_SLAVE_BUSWIDTH_64_BYTES) >> >> +struct axi_dma_chip_config { >> + int (*apb_setup)(struct platform_device *pdev, struct axi_dma_chip *chip); >> + int (*reset_init)(struct platform_device *pdev); >> + bool use_cfg2; >> +}; >> + >> static inline void >> axi_dma_iowrite32(struct axi_dma_chip *chip, u32 reg, u32 val) >> { >> @@ -86,7 +94,8 @@ static inline void axi_chan_config_write(struct axi_dma_chan *chan, >> >> cfg_lo = (config->dst_multblk_type << CH_CFG_L_DST_MULTBLK_TYPE_POS | >> config->src_multblk_type << CH_CFG_L_SRC_MULTBLK_TYPE_POS); >> - if (chan->chip->dw->hdata->reg_map_8_channels) { >> + if (chan->chip->dw->hdata->reg_map_8_channels && >> + !chan->chip->dw->hdata->use_cfg2) { >> cfg_hi = config->tt_fc << CH_CFG_H_TT_FC_POS | >> config->hs_sel_src << CH_CFG_H_HS_SEL_SRC_POS | >> config->hs_sel_dst << CH_CFG_H_HS_SEL_DST_POS | >> @@ -1142,7 +1151,7 @@ static int dma_chan_terminate_all(struct dma_chan *dchan) >> axi_chan_disable(chan); >> >> ret = readl_poll_timeout_atomic(chan->chip->regs + DMAC_CHEN, val, >> - !(val & chan_active), 1000, 10000); >> + !(val & chan_active), 1000, DMAC_TIMEOUT_US); >> if (ret == -ETIMEDOUT) >> dev_warn(dchan2dev(dchan), >> "%s failed to stop\n", axi_chan_name(chan)); >> @@ -1367,13 +1376,33 @@ static int parse_device_properties(struct axi_dma_chip *chip) >> return 0; >> } >> >> +static int intel_apb_setup(struct platform_device *pdev, struct axi_dma_chip *chip) >> +{ >> + chip->apb_regs = devm_platform_ioremap_resource(pdev, 1); >> + if (IS_ERR(chip->apb_regs)) >> + return PTR_ERR(chip->apb_regs); >> + else >> + return 0; >> +} >> + >> +static int jh7110_reset_init(struct platform_device *pdev) >> +{ >> + struct reset_control *resets; >> + >> + resets = devm_reset_control_array_get_exclusive(&pdev->dev); >> + if (IS_ERR(resets)) >> + return PTR_ERR(resets); >> + >> + return reset_control_deassert(resets); >> +} >> + >> static int dw_probe(struct platform_device *pdev) >> { >> - struct device_node *node = pdev->dev.of_node; >> struct axi_dma_chip *chip; >> struct resource *mem; >> struct dw_axi_dma *dw; >> struct dw_axi_dma_hcfg *hdata; >> + const struct axi_dma_chip_config *ccfg; >> u32 i; >> int ret; >> >> @@ -1402,10 +1431,21 @@ static int dw_probe(struct platform_device *pdev) >> if (IS_ERR(chip->regs)) >> return PTR_ERR(chip->regs); >> >> - if (of_device_is_compatible(node, "intel,kmb-axi-dma")) { >> - chip->apb_regs = devm_platform_ioremap_resource(pdev, 1); >> - if (IS_ERR(chip->apb_regs)) >> - return PTR_ERR(chip->apb_regs); >> + ccfg = of_device_get_match_data(&pdev->dev); >> + if (ccfg) { >> + if (ccfg->apb_setup) { >> + ret = ccfg->apb_setup(pdev, chip); >> + if (ret) >> + return ret; >> + } >> + >> + if (ccfg->reset_init) { >> + ret = ccfg->reset_init(pdev); >> + if (ret) >> + return ret; >> + } >> + >> + chip->dw->hdata->use_cfg2 = ccfg->use_cfg2; > > This claims and deasserts the resets before the clocks, whereas your > previous versions did it after turning the clocks on. Which is the > correct order? The order has no problem, I have tested it with audio pwmdac on VisionFive2 board. > > Also this certainly gets rid of of_device_is_compatible calls, but > seems like a lot of code to do that. Did you consider something like > > +#define AXI_DMA_FLAG_HAS_APB_REGS BIT(0) > +#define AXI_DMA_FLAG_HAS_RESETS BIT(1) > +#define AXI_DMA_FLAG_USE_CFG2 BIT(2) > > +unsigned int flags = (uintptr_t)device_get_match_data(&pdev->dev); > > -if (of_device_is_compatible(node, "intel,kmb-axi-dma")) { > +if (flags & AXI_DMA_FLAG_HAS_APB_REGS) { > > -if (of_device_is_compatible(node, "starfive,jh7110-axi-dma)) { > +if (flags & AXI_DMA_FLAG_HAS_RESETS) { > > +chip->dw->hwdata->use_cfg2 = !!(flags & AXI_DMA_FLAG_USE_CFG2); > > -{ .compatible = "intel,kmb-axi-dma" }, > +{ .compatible = "intel,kmb-axi-dma", .data = (void > *)AXI_DMA_FLAG_HAS_APB_REGS }, > +{ .compatible = "starive,jh7110-axi-dma", .data = (void > *)(AXI_DMA_FLAG_HAS_RESETS | AXI_DMA_FLAG_USE_CFG2) }, Of course, it's better to have less code. I will try your method. Thank you for taking the time to review and comment. Best regards, Walker
diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c index bf85aa0979ec..d1148f6fbcf9 100644 --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c @@ -21,10 +21,12 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/of_dma.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/property.h> +#include <linux/reset.h> #include <linux/slab.h> #include <linux/types.h> @@ -46,6 +48,12 @@ DMA_SLAVE_BUSWIDTH_32_BYTES | \ DMA_SLAVE_BUSWIDTH_64_BYTES) +struct axi_dma_chip_config { + int (*apb_setup)(struct platform_device *pdev, struct axi_dma_chip *chip); + int (*reset_init)(struct platform_device *pdev); + bool use_cfg2; +}; + static inline void axi_dma_iowrite32(struct axi_dma_chip *chip, u32 reg, u32 val) { @@ -86,7 +94,8 @@ static inline void axi_chan_config_write(struct axi_dma_chan *chan, cfg_lo = (config->dst_multblk_type << CH_CFG_L_DST_MULTBLK_TYPE_POS | config->src_multblk_type << CH_CFG_L_SRC_MULTBLK_TYPE_POS); - if (chan->chip->dw->hdata->reg_map_8_channels) { + if (chan->chip->dw->hdata->reg_map_8_channels && + !chan->chip->dw->hdata->use_cfg2) { cfg_hi = config->tt_fc << CH_CFG_H_TT_FC_POS | config->hs_sel_src << CH_CFG_H_HS_SEL_SRC_POS | config->hs_sel_dst << CH_CFG_H_HS_SEL_DST_POS | @@ -1142,7 +1151,7 @@ static int dma_chan_terminate_all(struct dma_chan *dchan) axi_chan_disable(chan); ret = readl_poll_timeout_atomic(chan->chip->regs + DMAC_CHEN, val, - !(val & chan_active), 1000, 10000); + !(val & chan_active), 1000, DMAC_TIMEOUT_US); if (ret == -ETIMEDOUT) dev_warn(dchan2dev(dchan), "%s failed to stop\n", axi_chan_name(chan)); @@ -1367,13 +1376,33 @@ static int parse_device_properties(struct axi_dma_chip *chip) return 0; } +static int intel_apb_setup(struct platform_device *pdev, struct axi_dma_chip *chip) +{ + chip->apb_regs = devm_platform_ioremap_resource(pdev, 1); + if (IS_ERR(chip->apb_regs)) + return PTR_ERR(chip->apb_regs); + else + return 0; +} + +static int jh7110_reset_init(struct platform_device *pdev) +{ + struct reset_control *resets; + + resets = devm_reset_control_array_get_exclusive(&pdev->dev); + if (IS_ERR(resets)) + return PTR_ERR(resets); + + return reset_control_deassert(resets); +} + static int dw_probe(struct platform_device *pdev) { - struct device_node *node = pdev->dev.of_node; struct axi_dma_chip *chip; struct resource *mem; struct dw_axi_dma *dw; struct dw_axi_dma_hcfg *hdata; + const struct axi_dma_chip_config *ccfg; u32 i; int ret; @@ -1402,10 +1431,21 @@ static int dw_probe(struct platform_device *pdev) if (IS_ERR(chip->regs)) return PTR_ERR(chip->regs); - if (of_device_is_compatible(node, "intel,kmb-axi-dma")) { - chip->apb_regs = devm_platform_ioremap_resource(pdev, 1); - if (IS_ERR(chip->apb_regs)) - return PTR_ERR(chip->apb_regs); + ccfg = of_device_get_match_data(&pdev->dev); + if (ccfg) { + if (ccfg->apb_setup) { + ret = ccfg->apb_setup(pdev, chip); + if (ret) + return ret; + } + + if (ccfg->reset_init) { + ret = ccfg->reset_init(pdev); + if (ret) + return ret; + } + + chip->dw->hdata->use_cfg2 = ccfg->use_cfg2; } chip->core_clk = devm_clk_get(chip->dev, "core-clk"); @@ -1557,9 +1597,20 @@ static const struct dev_pm_ops dw_axi_dma_pm_ops = { SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend, axi_dma_runtime_resume, NULL) }; +static const struct axi_dma_chip_config intel_chip_config = { + .apb_setup = intel_apb_setup, + .use_cfg2 = false, +}; + +static const struct axi_dma_chip_config jh7110_chip_config = { + .reset_init = jh7110_reset_init, + .use_cfg2 = true, +}; + static const struct of_device_id dw_dma_of_id_table[] = { { .compatible = "snps,axi-dma-1.01a" }, - { .compatible = "intel,kmb-axi-dma" }, + { .compatible = "intel,kmb-axi-dma", .data = &intel_chip_config }, + { .compatible = "starfive,jh7110-axi-dma", .data = &jh7110_chip_config }, {} }; MODULE_DEVICE_TABLE(of, dw_dma_of_id_table); diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac.h b/drivers/dma/dw-axi-dmac/dw-axi-dmac.h index e9d5eb0fd594..b906d5884efe 100644 --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac.h +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac.h @@ -21,6 +21,7 @@ #define DMAC_MAX_CHANNELS 16 #define DMAC_MAX_MASTERS 2 #define DMAC_MAX_BLK_SIZE 0x200000 +#define DMAC_TIMEOUT_US 200000 struct dw_axi_dma_hcfg { u32 nr_channels; @@ -33,6 +34,7 @@ struct dw_axi_dma_hcfg { /* Register map for DMAX_NUM_CHANNELS <= 8 */ bool reg_map_8_channels; bool restrict_axi_burst_len; + bool use_cfg2; }; struct axi_dma_chan {