Re: [PATCH V2 2/4] firmware: tegra: refactor bpmp driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jon,


On 24.1.2019 13.45, Jon Hunter wrote:

On 21/01/2019 12:28, Timo Alho wrote:
Split bpmp driver into common and chip specific parts to facilitae

s/facilitae/facilitate

adding support for previous and futurue Tegra chips that are using

s/futurue/future >

Thanks for pointing out. I think I need to setup up some spell checker...

...

  static int tegra_bpmp_probe(struct platform_device *pdev)
  {
  	struct tegra_bpmp *bpmp;
-	unsigned int i;
  	char tag[TAG_SZ];
  	size_t size;
  	int err;
@@ -766,32 +703,6 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
  	bpmp->soc = of_device_get_match_data(&pdev->dev);
  	bpmp->dev = &pdev->dev;
- bpmp->tx.pool = of_gen_pool_get(pdev->dev.of_node, "shmem", 0);
-	if (!bpmp->tx.pool) {
-		dev_err(&pdev->dev, "TX shmem pool not found\n");
-		return -ENOMEM;
-	}
-
-	bpmp->tx.virt = gen_pool_dma_alloc(bpmp->tx.pool, 4096, &bpmp->tx.phys);
-	if (!bpmp->tx.virt) {
-		dev_err(&pdev->dev, "failed to allocate from TX pool\n");
-		return -ENOMEM;
-	}
-
-	bpmp->rx.pool = of_gen_pool_get(pdev->dev.of_node, "shmem", 1);
-	if (!bpmp->rx.pool) {
-		dev_err(&pdev->dev, "RX shmem pool not found\n");
-		err = -ENOMEM;
-		goto free_tx;
-	}
-
-	bpmp->rx.virt = gen_pool_dma_alloc(bpmp->rx.pool, 4096, &bpmp->rx.phys);
-	if (!bpmp->rx.virt) {
-		dev_err(&pdev->dev, "failed to allocate from RX pool\n");
-		err = -ENOMEM;
-		goto free_tx;
-	}
-
  	INIT_LIST_HEAD(&bpmp->mrqs);
  	spin_lock_init(&bpmp->lock);
@@ -801,81 +712,38 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
  	size = BITS_TO_LONGS(bpmp->threaded.count) * sizeof(long);
bpmp->threaded.allocated = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
-	if (!bpmp->threaded.allocated) {
-		err = -ENOMEM;
-		goto free_rx;
-	}
+	if (!bpmp->threaded.allocated)
+		return -ENOMEM;
bpmp->threaded.busy = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
-	if (!bpmp->threaded.busy) {
-		err = -ENOMEM;
-		goto free_rx;
-	}
+	if (!bpmp->threaded.busy)
+		return -ENOMEM;
spin_lock_init(&bpmp->atomic_tx_lock);
  	bpmp->tx_channel = devm_kzalloc(&pdev->dev, sizeof(*bpmp->tx_channel),
  					GFP_KERNEL);
-	if (!bpmp->tx_channel) {
-		err = -ENOMEM;
-		goto free_rx;
-	}
+	if (!bpmp->tx_channel)
+		return -ENOMEM;
bpmp->rx_channel = devm_kzalloc(&pdev->dev, sizeof(*bpmp->rx_channel),
  	                                GFP_KERNEL);
-	if (!bpmp->rx_channel) {
-		err = -ENOMEM;
-		goto free_rx;
-	}
+	if (!bpmp->rx_channel)
+		return -ENOMEM;
bpmp->threaded_channels = devm_kcalloc(&pdev->dev, bpmp->threaded.count,
  					       sizeof(*bpmp->threaded_channels),
  					       GFP_KERNEL);
-	if (!bpmp->threaded_channels) {
-		err = -ENOMEM;
-		goto free_rx;
-	}
-
-	err = tegra_bpmp_channel_init(bpmp->tx_channel, bpmp,
-				      bpmp->soc->channels.cpu_tx.offset);
-	if (err < 0)
-		goto free_rx;
+	if (!bpmp->threaded_channels)
+		return -ENOMEM;
- err = tegra_bpmp_channel_init(bpmp->rx_channel, bpmp,
-				      bpmp->soc->channels.cpu_rx.offset);
+	err = bpmp->soc->ops->init(bpmp);
  	if (err < 0)
-		goto cleanup_tx_channel;
-
-	for (i = 0; i < bpmp->threaded.count; i++) {
-		err = tegra_bpmp_channel_init(
-			&bpmp->threaded_channels[i], bpmp,
-			bpmp->soc->channels.thread.offset + i);
-		if (err < 0)
-			goto cleanup_threaded_channels;
-	}
-
-	/* mbox registration */
-	bpmp->mbox.client.dev = &pdev->dev;
-	bpmp->mbox.client.rx_callback = tegra_bpmp_handle_rx;
-	bpmp->mbox.client.tx_block = false;
-	bpmp->mbox.client.knows_txdone = false;
-
-	bpmp->mbox.channel = mbox_request_channel(&bpmp->mbox.client, 0);
-	if (IS_ERR(bpmp->mbox.channel)) {
-		err = PTR_ERR(bpmp->mbox.channel);
-		dev_err(&pdev->dev, "failed to get HSP mailbox: %d\n", err);
-		goto cleanup_threaded_channels;
-	}
-
-	/* reset message channels */
-	tegra_bpmp_channel_reset(bpmp->tx_channel);
-	tegra_bpmp_channel_reset(bpmp->rx_channel);
-	for (i = 0; i < bpmp->threaded.count; i++)
-		tegra_bpmp_channel_reset(&bpmp->threaded_channels[i]);
+		return err;
err = tegra_bpmp_request_mrq(bpmp, MRQ_PING,
  				     tegra_bpmp_mrq_handle_ping, bpmp);
  	if (err < 0)
-		goto free_mbox;
+		goto deinit;
err = tegra_bpmp_ping(bpmp);
  	if (err < 0) {
@@ -917,37 +785,17 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
free_mrq:
  	tegra_bpmp_free_mrq(bpmp, MRQ_PING, bpmp);
-free_mbox:
-	mbox_free_channel(bpmp->mbox.channel);
-cleanup_threaded_channels:
-	for (i = 0; i < bpmp->threaded.count; i++) {
-		if (bpmp->threaded_channels[i].bpmp)
-			tegra_bpmp_channel_cleanup(&bpmp->threaded_channels[i]);
-	}
+deinit:
+	bpmp->soc->ops->deinit(bpmp);
- tegra_bpmp_channel_cleanup(bpmp->rx_channel);
-cleanup_tx_channel:
-	tegra_bpmp_channel_cleanup(bpmp->tx_channel);
-free_rx:
-	gen_pool_free(bpmp->rx.pool, (unsigned long)bpmp->rx.virt, 4096);
-free_tx:
-	gen_pool_free(bpmp->tx.pool, (unsigned long)bpmp->tx.virt, 4096);
  	return err;
  }
static int __maybe_unused tegra_bpmp_resume(struct device *dev)
  {
  	struct tegra_bpmp *bpmp = dev_get_drvdata(dev);
-	unsigned int i;
-
-	/* reset message channels */
-	tegra_bpmp_channel_reset(bpmp->tx_channel);
-	tegra_bpmp_channel_reset(bpmp->rx_channel);
- for (i = 0; i < bpmp->threaded.count; i++)
-		tegra_bpmp_channel_reset(&bpmp->threaded_channels[i]);
-
-	return 0;
+	return bpmp->soc->ops->resume(bpmp);
  }
static SIMPLE_DEV_PM_OPS(tegra_bpmp_pm_ops, NULL, tegra_bpmp_resume);

Looks fine to me, but I do wonder if you need any checks to see if the
ops handler is populated? For example, looking at the Tegra210 BPMP
driver it does not populate resume, yet resume I believe would be called?


Good point, I'll add checks for ops that clearly can be optional. In this case it would be deinit() and resume() ops.


Cheers
Jon




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux