diff mbox series

[v2,02/10] dma-engine: sun4i: Add has_reset option to quirk

Message ID 20241027091440.1913863-2-csokas.bence@prolan.hu (mailing list archive)
State New
Headers show
Series [v2,01/10] dma-engine: sun4i: Add a quirk to support different chips | expand

Commit Message

Csókás Bence Oct. 27, 2024, 9:14 a.m. UTC 
From: Mesih Kilinc <mesihkilinc@gmail.com>

Allwinner suniv F1C100s has a reset bit for DMA in CCU. Sun4i do not
has this bit but in order to support suniv we need to add it. So add
support for reset bit.

Signed-off-by: Mesih Kilinc <mesihkilinc@gmail.com>
[ csokas.bence: Rebased and addressed comments ]
Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
---

Notes:
    Changes in v2:
    * Call reset_control_deassert() unconditionally, as it supports optional resets
    * Use dev_err_probe()
    * Whitespace

 drivers/dma/sun4i-dma.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Csókás Bence Oct. 27, 2024, 6:03 p.m. UTC | #1 
On 2024. 10. 27. 10:14, Csókás, Bence wrote:
> From: Mesih Kilinc <mesihkilinc@gmail.com>
> 
> Allwinner suniv F1C100s has a reset bit for DMA in CCU. Sun4i do not
> has this bit but in order to support suniv we need to add it. So add
> support for reset bit.
> 
> Signed-off-by: Mesih Kilinc <mesihkilinc@gmail.com>
> [ csokas.bence: Rebased and addressed comments ]
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
> ---
> 
> Notes:
>      Changes in v2:
>      * Call reset_control_deassert() unconditionally, as it supports optional resets
>      * Use dev_err_probe()

I missed one, namely:

> +		dev_err(&pdev->dev,
> +			"Failed to deassert the reset control\n");
> +		goto err_clk_disable;
> +	}

For now I'll resubmit just this patch, and then wait for more comments 
that may arise during the week, then resubmit the whole amended series.
kernel test robot Oct. 27, 2024, 7:12 p.m. UTC | #2 
Hi Bence,

kernel test robot noticed the following build errors:

[auto build test ERROR on sunxi/sunxi/for-next]
[also build test ERROR on vkoul-dmaengine/next broonie-sound/for-next arm64/for-next/core clk/clk-next kvmarm/next rockchip/for-next shawnguo/for-next soc/for-next arm/for-next arm/fixes linus/master v6.12-rc4 next-20241025]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Cs-k-s-Bence/dma-engine-sun4i-Add-has_reset-option-to-quirk/20241027-172307
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
patch link:    https://lore.kernel.org/r/20241027091440.1913863-2-csokas.bence%40prolan.hu
patch subject: [PATCH v2 02/10] dma-engine: sun4i: Add has_reset option to quirk
config: arm-multi_v7_defconfig (https://download.01.org/0day-ci/archive/20241028/202410280225.baqFmTsa-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241028/202410280225.baqFmTsa-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410280225.baqFmTsa-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/dma/sun4i-dma.c: In function 'sun4i_dma_probe':
>> drivers/dma/sun4i-dma.c:1225:51: warning: passing argument 2 of 'dev_err_probe' makes integer from pointer without a cast [-Wint-conversion]
    1225 |                         dev_err_probe(&pdev->dev, "Failed to get reset control\n");
         |                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                                   |
         |                                                   char *
   In file included from include/linux/device.h:15,
                    from include/linux/dma-mapping.h:8,
                    from drivers/dma/sun4i-dma.c:10:
   include/linux/dev_printk.h:278:64: note: expected 'int' but argument is of type 'char *'
     278 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
         |                                                            ~~~~^~~
>> drivers/dma/sun4i-dma.c:1225:25: error: too few arguments to function 'dev_err_probe'
    1225 |                         dev_err_probe(&pdev->dev, "Failed to get reset control\n");
         |                         ^~~~~~~~~~~~~
   include/linux/dev_printk.h:278:20: note: declared here
     278 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
         |                    ^~~~~~~~~~~~~


vim +/dev_err_probe +1225 drivers/dma/sun4i-dma.c

  1193	
  1194	static int sun4i_dma_probe(struct platform_device *pdev)
  1195	{
  1196		struct sun4i_dma_dev *priv;
  1197		int i, j, ret;
  1198	
  1199		priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
  1200		if (!priv)
  1201			return -ENOMEM;
  1202	
  1203		priv->cfg = of_device_get_match_data(&pdev->dev);
  1204		if (!priv->cfg)
  1205			return -ENODEV;
  1206	
  1207		priv->base = devm_platform_ioremap_resource(pdev, 0);
  1208		if (IS_ERR(priv->base))
  1209			return PTR_ERR(priv->base);
  1210	
  1211		priv->irq = platform_get_irq(pdev, 0);
  1212		if (priv->irq < 0)
  1213			return priv->irq;
  1214	
  1215		priv->clk = devm_clk_get(&pdev->dev, NULL);
  1216		if (IS_ERR(priv->clk)) {
  1217			dev_err(&pdev->dev, "No clock specified\n");
  1218			return PTR_ERR(priv->clk);
  1219		}
  1220	
  1221		if (priv->cfg->has_reset) {
  1222			priv->rst = devm_reset_control_get_exclusive(&pdev->dev,
  1223								     NULL);
  1224			if (IS_ERR(priv->rst)) {
> 1225				dev_err_probe(&pdev->dev, "Failed to get reset control\n");
  1226				return PTR_ERR(priv->rst);
  1227			}
  1228		}
  1229	
  1230		platform_set_drvdata(pdev, priv);
  1231		spin_lock_init(&priv->lock);
  1232	
  1233		dma_set_max_seg_size(&pdev->dev, SUN4I_DMA_MAX_SEG_SIZE);
  1234	
  1235		dma_cap_zero(priv->slave.cap_mask);
  1236		dma_cap_set(DMA_PRIVATE, priv->slave.cap_mask);
  1237		dma_cap_set(DMA_MEMCPY, priv->slave.cap_mask);
  1238		dma_cap_set(DMA_CYCLIC, priv->slave.cap_mask);
  1239		dma_cap_set(DMA_SLAVE, priv->slave.cap_mask);
  1240	
  1241		INIT_LIST_HEAD(&priv->slave.channels);
  1242		priv->slave.device_free_chan_resources	= sun4i_dma_free_chan_resources;
  1243		priv->slave.device_tx_status		= sun4i_dma_tx_status;
  1244		priv->slave.device_issue_pending	= sun4i_dma_issue_pending;
  1245		priv->slave.device_prep_slave_sg	= sun4i_dma_prep_slave_sg;
  1246		priv->slave.device_prep_dma_memcpy	= sun4i_dma_prep_dma_memcpy;
  1247		priv->slave.device_prep_dma_cyclic	= sun4i_dma_prep_dma_cyclic;
  1248		priv->slave.device_config		= sun4i_dma_config;
  1249		priv->slave.device_terminate_all	= sun4i_dma_terminate_all;
  1250		priv->slave.copy_align			= 2;
  1251		priv->slave.src_addr_widths		= BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
  1252							  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
  1253							  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
  1254		priv->slave.dst_addr_widths		= BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
  1255							  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
  1256							  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
  1257		priv->slave.directions			= BIT(DMA_DEV_TO_MEM) |
  1258							  BIT(DMA_MEM_TO_DEV);
  1259		priv->slave.residue_granularity		= DMA_RESIDUE_GRANULARITY_BURST;
  1260	
  1261		priv->slave.dev = &pdev->dev;
  1262	
  1263		priv->pchans = devm_kcalloc(&pdev->dev, priv->cfg->dma_nr_max_channels,
  1264					    sizeof(struct sun4i_dma_pchan), GFP_KERNEL);
  1265		priv->vchans = devm_kcalloc(&pdev->dev, SUN4I_DMA_NR_MAX_VCHANS,
  1266					    sizeof(struct sun4i_dma_vchan), GFP_KERNEL);
  1267		priv->pchans_used = devm_kcalloc(&pdev->dev,
  1268						 BITS_TO_LONGS(priv->cfg->dma_nr_max_channels),
  1269						 sizeof(unsigned long), GFP_KERNEL);
  1270		if (!priv->vchans || !priv->pchans || !priv->pchans_used)
  1271			return -ENOMEM;
  1272	
  1273		/*
  1274		 * [0..priv->cfg->ndma_nr_max_channels) are normal pchans, and
  1275		 * [priv->cfg->ndma_nr_max_channels..priv->cfg->dma_nr_max_channels) are
  1276		 * dedicated ones
  1277		 */
  1278		for (i = 0; i < priv->cfg->ndma_nr_max_channels; i++)
  1279			priv->pchans[i].base = priv->base +
  1280				SUN4I_NDMA_CHANNEL_REG_BASE(i);
  1281	
  1282		for (j = 0; i < priv->cfg->dma_nr_max_channels; i++, j++) {
  1283			priv->pchans[i].base = priv->base +
  1284				SUN4I_DDMA_CHANNEL_REG_BASE(j);
  1285			priv->pchans[i].is_dedicated = 1;
  1286		}
  1287	
  1288		for (i = 0; i < SUN4I_DMA_NR_MAX_VCHANS; i++) {
  1289			struct sun4i_dma_vchan *vchan = &priv->vchans[i];
  1290	
  1291			spin_lock_init(&vchan->vc.lock);
  1292			vchan->vc.desc_free = sun4i_dma_free_contract;
  1293			vchan_init(&vchan->vc, &priv->slave);
  1294		}
  1295	
  1296		ret = clk_prepare_enable(priv->clk);
  1297		if (ret) {
  1298			dev_err(&pdev->dev, "Couldn't enable the clock\n");
  1299			return ret;
  1300		}
  1301	
  1302		/* Deassert the reset control */
  1303		ret = reset_control_deassert(priv->rst);
  1304		if (ret) {
  1305			dev_err(&pdev->dev,
  1306				"Failed to deassert the reset control\n");
  1307			goto err_clk_disable;
  1308		}
  1309	
  1310		/*
  1311		 * Make sure the IRQs are all disabled and accounted for. The bootloader
  1312		 * likes to leave these dirty
  1313		 */
  1314		writel(0, priv->base + SUN4I_DMA_IRQ_ENABLE_REG);
  1315		writel(0xFFFFFFFF, priv->base + SUN4I_DMA_IRQ_PENDING_STATUS_REG);
  1316	
  1317		ret = devm_request_irq(&pdev->dev, priv->irq, sun4i_dma_interrupt,
  1318				       0, dev_name(&pdev->dev), priv);
  1319		if (ret) {
  1320			dev_err(&pdev->dev, "Cannot request IRQ\n");
  1321			goto err_clk_disable;
  1322		}
  1323	
  1324		ret = dma_async_device_register(&priv->slave);
  1325		if (ret) {
  1326			dev_warn(&pdev->dev, "Failed to register DMA engine device\n");
  1327			goto err_clk_disable;
  1328		}
  1329	
  1330		ret = of_dma_controller_register(pdev->dev.of_node, sun4i_dma_of_xlate,
  1331						 priv);
  1332		if (ret) {
  1333			dev_err(&pdev->dev, "of_dma_controller_register failed\n");
  1334			goto err_dma_unregister;
  1335		}
  1336	
  1337		dev_dbg(&pdev->dev, "Successfully probed SUN4I_DMA\n");
  1338	
  1339		return 0;
  1340	
  1341	err_dma_unregister:
  1342		dma_async_device_unregister(&priv->slave);
  1343	err_clk_disable:
  1344		clk_disable_unprepare(priv->clk);
  1345		return ret;
  1346	}
  1347
kernel test robot Oct. 27, 2024, 8:05 p.m. UTC | #3 
Hi Bence,

kernel test robot noticed the following build errors:

[auto build test ERROR on sunxi/sunxi/for-next]
[also build test ERROR on vkoul-dmaengine/next broonie-sound/for-next arm64/for-next/core clk/clk-next kvmarm/next rockchip/for-next shawnguo/for-next soc/for-next arm/for-next arm/fixes linus/master v6.12-rc4 next-20241025]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Cs-k-s-Bence/dma-engine-sun4i-Add-has_reset-option-to-quirk/20241027-172307
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
patch link:    https://lore.kernel.org/r/20241027091440.1913863-2-csokas.bence%40prolan.hu
patch subject: [PATCH v2 02/10] dma-engine: sun4i: Add has_reset option to quirk
config: arm-sunxi_defconfig (https://download.01.org/0day-ci/archive/20241028/202410280330.S1S4TKbz-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241028/202410280330.S1S4TKbz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410280330.S1S4TKbz-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/dma/sun4i-dma.c: In function 'sun4i_dma_probe':
>> drivers/dma/sun4i-dma.c:1225:51: error: passing argument 2 of 'dev_err_probe' makes integer from pointer without a cast [-Wint-conversion]
    1225 |                         dev_err_probe(&pdev->dev, "Failed to get reset control\n");
         |                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                                   |
         |                                                   char *
   In file included from include/linux/device.h:15,
                    from include/linux/dma-mapping.h:8,
                    from drivers/dma/sun4i-dma.c:10:
   include/linux/dev_printk.h:278:64: note: expected 'int' but argument is of type 'char *'
     278 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
         |                                                            ~~~~^~~
   drivers/dma/sun4i-dma.c:1225:25: error: too few arguments to function 'dev_err_probe'
    1225 |                         dev_err_probe(&pdev->dev, "Failed to get reset control\n");
         |                         ^~~~~~~~~~~~~
   include/linux/dev_printk.h:278:20: note: declared here
     278 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
         |                    ^~~~~~~~~~~~~


vim +/dev_err_probe +1225 drivers/dma/sun4i-dma.c

  1193	
  1194	static int sun4i_dma_probe(struct platform_device *pdev)
  1195	{
  1196		struct sun4i_dma_dev *priv;
  1197		int i, j, ret;
  1198	
  1199		priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
  1200		if (!priv)
  1201			return -ENOMEM;
  1202	
  1203		priv->cfg = of_device_get_match_data(&pdev->dev);
  1204		if (!priv->cfg)
  1205			return -ENODEV;
  1206	
  1207		priv->base = devm_platform_ioremap_resource(pdev, 0);
  1208		if (IS_ERR(priv->base))
  1209			return PTR_ERR(priv->base);
  1210	
  1211		priv->irq = platform_get_irq(pdev, 0);
  1212		if (priv->irq < 0)
  1213			return priv->irq;
  1214	
  1215		priv->clk = devm_clk_get(&pdev->dev, NULL);
  1216		if (IS_ERR(priv->clk)) {
  1217			dev_err(&pdev->dev, "No clock specified\n");
  1218			return PTR_ERR(priv->clk);
  1219		}
  1220	
  1221		if (priv->cfg->has_reset) {
  1222			priv->rst = devm_reset_control_get_exclusive(&pdev->dev,
  1223								     NULL);
  1224			if (IS_ERR(priv->rst)) {
> 1225				dev_err_probe(&pdev->dev, "Failed to get reset control\n");
  1226				return PTR_ERR(priv->rst);
  1227			}
  1228		}
  1229	
  1230		platform_set_drvdata(pdev, priv);
  1231		spin_lock_init(&priv->lock);
  1232	
  1233		dma_set_max_seg_size(&pdev->dev, SUN4I_DMA_MAX_SEG_SIZE);
  1234	
  1235		dma_cap_zero(priv->slave.cap_mask);
  1236		dma_cap_set(DMA_PRIVATE, priv->slave.cap_mask);
  1237		dma_cap_set(DMA_MEMCPY, priv->slave.cap_mask);
  1238		dma_cap_set(DMA_CYCLIC, priv->slave.cap_mask);
  1239		dma_cap_set(DMA_SLAVE, priv->slave.cap_mask);
  1240	
  1241		INIT_LIST_HEAD(&priv->slave.channels);
  1242		priv->slave.device_free_chan_resources	= sun4i_dma_free_chan_resources;
  1243		priv->slave.device_tx_status		= sun4i_dma_tx_status;
  1244		priv->slave.device_issue_pending	= sun4i_dma_issue_pending;
  1245		priv->slave.device_prep_slave_sg	= sun4i_dma_prep_slave_sg;
  1246		priv->slave.device_prep_dma_memcpy	= sun4i_dma_prep_dma_memcpy;
  1247		priv->slave.device_prep_dma_cyclic	= sun4i_dma_prep_dma_cyclic;
  1248		priv->slave.device_config		= sun4i_dma_config;
  1249		priv->slave.device_terminate_all	= sun4i_dma_terminate_all;
  1250		priv->slave.copy_align			= 2;
  1251		priv->slave.src_addr_widths		= BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
  1252							  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
  1253							  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
  1254		priv->slave.dst_addr_widths		= BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
  1255							  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
  1256							  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
  1257		priv->slave.directions			= BIT(DMA_DEV_TO_MEM) |
  1258							  BIT(DMA_MEM_TO_DEV);
  1259		priv->slave.residue_granularity		= DMA_RESIDUE_GRANULARITY_BURST;
  1260	
  1261		priv->slave.dev = &pdev->dev;
  1262	
  1263		priv->pchans = devm_kcalloc(&pdev->dev, priv->cfg->dma_nr_max_channels,
  1264					    sizeof(struct sun4i_dma_pchan), GFP_KERNEL);
  1265		priv->vchans = devm_kcalloc(&pdev->dev, SUN4I_DMA_NR_MAX_VCHANS,
  1266					    sizeof(struct sun4i_dma_vchan), GFP_KERNEL);
  1267		priv->pchans_used = devm_kcalloc(&pdev->dev,
  1268						 BITS_TO_LONGS(priv->cfg->dma_nr_max_channels),
  1269						 sizeof(unsigned long), GFP_KERNEL);
  1270		if (!priv->vchans || !priv->pchans || !priv->pchans_used)
  1271			return -ENOMEM;
  1272	
  1273		/*
  1274		 * [0..priv->cfg->ndma_nr_max_channels) are normal pchans, and
  1275		 * [priv->cfg->ndma_nr_max_channels..priv->cfg->dma_nr_max_channels) are
  1276		 * dedicated ones
  1277		 */
  1278		for (i = 0; i < priv->cfg->ndma_nr_max_channels; i++)
  1279			priv->pchans[i].base = priv->base +
  1280				SUN4I_NDMA_CHANNEL_REG_BASE(i);
  1281	
  1282		for (j = 0; i < priv->cfg->dma_nr_max_channels; i++, j++) {
  1283			priv->pchans[i].base = priv->base +
  1284				SUN4I_DDMA_CHANNEL_REG_BASE(j);
  1285			priv->pchans[i].is_dedicated = 1;
  1286		}
  1287	
  1288		for (i = 0; i < SUN4I_DMA_NR_MAX_VCHANS; i++) {
  1289			struct sun4i_dma_vchan *vchan = &priv->vchans[i];
  1290	
  1291			spin_lock_init(&vchan->vc.lock);
  1292			vchan->vc.desc_free = sun4i_dma_free_contract;
  1293			vchan_init(&vchan->vc, &priv->slave);
  1294		}
  1295	
  1296		ret = clk_prepare_enable(priv->clk);
  1297		if (ret) {
  1298			dev_err(&pdev->dev, "Couldn't enable the clock\n");
  1299			return ret;
  1300		}
  1301	
  1302		/* Deassert the reset control */
  1303		ret = reset_control_deassert(priv->rst);
  1304		if (ret) {
  1305			dev_err(&pdev->dev,
  1306				"Failed to deassert the reset control\n");
  1307			goto err_clk_disable;
  1308		}
  1309	
  1310		/*
  1311		 * Make sure the IRQs are all disabled and accounted for. The bootloader
  1312		 * likes to leave these dirty
  1313		 */
  1314		writel(0, priv->base + SUN4I_DMA_IRQ_ENABLE_REG);
  1315		writel(0xFFFFFFFF, priv->base + SUN4I_DMA_IRQ_PENDING_STATUS_REG);
  1316	
  1317		ret = devm_request_irq(&pdev->dev, priv->irq, sun4i_dma_interrupt,
  1318				       0, dev_name(&pdev->dev), priv);
  1319		if (ret) {
  1320			dev_err(&pdev->dev, "Cannot request IRQ\n");
  1321			goto err_clk_disable;
  1322		}
  1323	
  1324		ret = dma_async_device_register(&priv->slave);
  1325		if (ret) {
  1326			dev_warn(&pdev->dev, "Failed to register DMA engine device\n");
  1327			goto err_clk_disable;
  1328		}
  1329	
  1330		ret = of_dma_controller_register(pdev->dev.of_node, sun4i_dma_of_xlate,
  1331						 priv);
  1332		if (ret) {
  1333			dev_err(&pdev->dev, "of_dma_controller_register failed\n");
  1334			goto err_dma_unregister;
  1335		}
  1336	
  1337		dev_dbg(&pdev->dev, "Successfully probed SUN4I_DMA\n");
  1338	
  1339		return 0;
  1340	
  1341	err_dma_unregister:
  1342		dma_async_device_unregister(&priv->slave);
  1343	err_clk_disable:
  1344		clk_disable_unprepare(priv->clk);
  1345		return ret;
  1346	}
  1347
Krzysztof Kozlowski Oct. 27, 2024, 8:42 p.m. UTC | #4 
On Sun, Oct 27, 2024 at 10:14:32AM +0100, Csókás, Bence wrote:
> From: Mesih Kilinc <mesihkilinc@gmail.com>
> 
> Allwinner suniv F1C100s has a reset bit for DMA in CCU. Sun4i do not
> has this bit but in order to support suniv we need to add it. So add
> support for reset bit.
>  
>  static struct sun4i_dma_dev *to_sun4i_dma_dev(struct dma_device *dev)
> @@ -1215,6 +1218,15 @@ static int sun4i_dma_probe(struct platform_device *pdev)
>  		return PTR_ERR(priv->clk);
>  	}
>  
> +	if (priv->cfg->has_reset) {
> +		priv->rst = devm_reset_control_get_exclusive(&pdev->dev,
> +							     NULL);
> +		if (IS_ERR(priv->rst)) {
> +			dev_err_probe(&pdev->dev, "Failed to get reset control\n");

syntax is: return dev_err_probe()

Best regards,
Krzysztof
Csókás Bence Oct. 28, 2024, 7:37 a.m. UTC | #5 
Hi,

On 2024. 10. 27. 21:42, Krzysztof Kozlowski wrote:
> On Sun, Oct 27, 2024 at 10:14:32AM +0100, Csókás, Bence wrote:
>> From: Mesih Kilinc <mesihkilinc@gmail.com>
>>
>> Allwinner suniv F1C100s has a reset bit for DMA in CCU. Sun4i do not
>> has this bit but in order to support suniv we need to add it. So add
>> support for reset bit.
>>   
>>   static struct sun4i_dma_dev *to_sun4i_dma_dev(struct dma_device *dev)
>> @@ -1215,6 +1218,15 @@ static int sun4i_dma_probe(struct platform_device *pdev)
>>   		return PTR_ERR(priv->clk);
>>   	}
>>   
>> +	if (priv->cfg->has_reset) {
>> +		priv->rst = devm_reset_control_get_exclusive(&pdev->dev,
>> +							     NULL);
>> +		if (IS_ERR(priv->rst)) {
>> +			dev_err_probe(&pdev->dev, "Failed to get reset control\n");
> 
> syntax is: return dev_err_probe()
> 
> Best regards,
> Krzysztof

Thanks! And regarding v3 of this patch, I have `clk_disable_unprepare()` 
after `dev_err_probe()`, I assume I have to let that be i.e. not return 
immediately?
Chen-Yu Tsai Oct. 28, 2024, 7:44 a.m. UTC | #6 
On Mon, Oct 28, 2024 at 3:37 PM Csókás Bence <csokas.bence@prolan.hu> wrote:
>
> Hi,
>
> On 2024. 10. 27. 21:42, Krzysztof Kozlowski wrote:
> > On Sun, Oct 27, 2024 at 10:14:32AM +0100, Csókás, Bence wrote:
> >> From: Mesih Kilinc <mesihkilinc@gmail.com>
> >>
> >> Allwinner suniv F1C100s has a reset bit for DMA in CCU. Sun4i do not
> >> has this bit but in order to support suniv we need to add it. So add
> >> support for reset bit.
> >>
> >>   static struct sun4i_dma_dev *to_sun4i_dma_dev(struct dma_device *dev)
> >> @@ -1215,6 +1218,15 @@ static int sun4i_dma_probe(struct platform_device *pdev)
> >>              return PTR_ERR(priv->clk);
> >>      }
> >>
> >> +    if (priv->cfg->has_reset) {
> >> +            priv->rst = devm_reset_control_get_exclusive(&pdev->dev,
> >> +                                                         NULL);
> >> +            if (IS_ERR(priv->rst)) {
> >> +                    dev_err_probe(&pdev->dev, "Failed to get reset control\n");
> >
> > syntax is: return dev_err_probe()
> >
> > Best regards,
> > Krzysztof
>
> Thanks! And regarding v3 of this patch, I have `clk_disable_unprepare()`
> after `dev_err_probe()`, I assume I have to let that be i.e. not return
> immediately?

I suggest adding a patch to switch the clk API calls to devm_clk_get_enabled()
which handles all the cleanup. Similarly you can switch to

    devm_reset_control_get_exclusive_deasserted()

for this patch.


ChenYu
Krzysztof Kozlowski Oct. 28, 2024, 8:08 a.m. UTC | #7 
On 28/10/2024 08:37, Csókás Bence wrote:
> Hi,
> 
> On 2024. 10. 27. 21:42, Krzysztof Kozlowski wrote:
>> On Sun, Oct 27, 2024 at 10:14:32AM +0100, Csókás, Bence wrote:
>>> From: Mesih Kilinc <mesihkilinc@gmail.com>
>>>
>>> Allwinner suniv F1C100s has a reset bit for DMA in CCU. Sun4i do not
>>> has this bit but in order to support suniv we need to add it. So add
>>> support for reset bit.
>>>   
>>>   static struct sun4i_dma_dev *to_sun4i_dma_dev(struct dma_device *dev)
>>> @@ -1215,6 +1218,15 @@ static int sun4i_dma_probe(struct platform_device *pdev)
>>>   		return PTR_ERR(priv->clk);
>>>   	}
>>>   
>>> +	if (priv->cfg->has_reset) {
>>> +		priv->rst = devm_reset_control_get_exclusive(&pdev->dev,
>>> +							     NULL);
>>> +		if (IS_ERR(priv->rst)) {
>>> +			dev_err_probe(&pdev->dev, "Failed to get reset control\n");
>>
>> syntax is: return dev_err_probe()
>>
>> Best regards,
>> Krzysztof
> 
> Thanks! And regarding v3 of this patch, I have `clk_disable_unprepare()` 

No, you do not. Read your code correctly.

+			dev_err_probe(&pdev->dev, "Failed to get reset control\n");
+			return PTR_ERR(priv->rst);

Where is here clk_disable_unprepare()?


Best regards,
Krzysztof
Csókás Bence Oct. 28, 2024, 1:12 p.m. UTC | #8 
On 2024. 10. 28. 8:44, Chen-Yu Tsai wrote:
> I suggest adding a patch to switch the clk API calls to devm_clk_get_enabled()
> which handles all the cleanup. Similarly you can switch to
> 
>      devm_reset_control_get_exclusive_deasserted()
> 
> for this patch.
> 
> 
> ChenYu

Huh, that's a new API! Thanks, I'll switch to that then.

Regarding the change to devm_clk_get_enabled(), I think that should be a 
separate patch from this series, where all the pre-existing dev_err()'s 
get changed as well. If someone wants to work on that, go ahead, but if 
no one does then after this series is merged I might get around to that too.

Bence
Csókás Bence Oct. 28, 2024, 4:22 p.m. UTC | #9 
On 2024. 10. 28. 14:12, Csókás Bence wrote:
> 
> 
> On 2024. 10. 28. 8:44, Chen-Yu Tsai wrote:
>> I suggest adding a patch to switch the clk API calls to 
>> devm_clk_get_enabled()
>> which handles all the cleanup. Similarly you can switch to
>>
>>      devm_reset_control_get_exclusive_deasserted()
>>
>> for this patch.
>>
>>
>> ChenYu
> 
> Huh, that's a new API! Thanks, I'll switch to that then.

Actually, it doesn't seem to be merged to Linus' tree yet. I'll probably 
just switch it after it is merged.

Bence
diff mbox series

Patch

diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c
index d472f57a39ea..0a45089461e1 100644
--- a/drivers/dma/sun4i-dma.c
+++ b/drivers/dma/sun4i-dma.c
@@ -15,6 +15,7 @@ 
 #include <linux/of_dma.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
@@ -159,6 +160,7 @@  struct sun4i_dma_config {
 	u8 ddma_drq_sdram;
 
 	u8 max_burst;
+	bool has_reset;
 };
 
 struct sun4i_dma_pchan {
@@ -208,6 +210,7 @@  struct sun4i_dma_dev {
 	int				irq;
 	spinlock_t			lock;
 	const struct sun4i_dma_config *cfg;
+	struct reset_control *rst;
 };
 
 static struct sun4i_dma_dev *to_sun4i_dma_dev(struct dma_device *dev)
@@ -1215,6 +1218,15 @@  static int sun4i_dma_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->clk);
 	}
 
+	if (priv->cfg->has_reset) {
+		priv->rst = devm_reset_control_get_exclusive(&pdev->dev,
+							     NULL);
+		if (IS_ERR(priv->rst)) {
+			dev_err_probe(&pdev->dev, "Failed to get reset control\n");
+			return PTR_ERR(priv->rst);
+		}
+	}
+
 	platform_set_drvdata(pdev, priv);
 	spin_lock_init(&priv->lock);
 
@@ -1287,6 +1299,14 @@  static int sun4i_dma_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* Deassert the reset control */
+	ret = reset_control_deassert(priv->rst);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Failed to deassert the reset control\n");
+		goto err_clk_disable;
+	}
+
 	/*
 	 * Make sure the IRQs are all disabled and accounted for. The bootloader
 	 * likes to leave these dirty
@@ -1356,6 +1376,7 @@  static struct sun4i_dma_config sun4i_a10_dma_cfg = {
 	.ddma_drq_sdram		= SUN4I_DDMA_DRQ_TYPE_SDRAM,
 
 	.max_burst		= SUN4I_MAX_BURST,
+	.has_reset		= false,
 };
 
 static const struct of_device_id sun4i_dma_match[] = {