Re: [PATCH 2/3] crypto: brcm: Add Broadcom SPU driver
Mark, Thanks for the comments. Replies below. Rob On 12/6/2016 9:18 AM, Mark Rutland wrote: On Wed, Nov 30, 2016 at 03:07:32PM -0500, Rob Rice wrote: +static const struct of_device_id bcm_spu_dt_ids[] = { + { + .compatible = "brcm,spum-crypto", + .data = &spum_ns2_types, + }, + { + .compatible = "brcm,spum-nsp-crypto", + .data = &spum_nsp_types, + }, + { + .compatible = "brcm,spu2-crypto", + .data = &spu2_types, + }, + { + .compatible = "brcm,spu2-v2-crypto", + .data = &spu2_v2_types, + }, These last two weren't in the binding document. yes, I'll add them. + { /* sentinel */ } +}; + +MODULE_DEVICE_TABLE(of, bcm_spu_dt_ids); + +static int spu_dt_read(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct spu_hw *spu = &iproc_priv.spu; + struct device_node *dn = pdev->dev.of_node; + struct resource *spu_ctrl_regs; + const struct of_device_id *match; + struct spu_type_subtype *matched_spu_type; + void __iomem *spu_reg_vbase[MAX_SPUS]; + int i; + int err; + + if (!of_device_is_available(dn)) { + dev_crit(dev, "SPU device not available"); + return -ENODEV; + } How can this happen? You are correct. This is unnecessary. I will remove. + /* Count number of mailbox channels */ + spu->num_chan = of_count_phandle_with_args(dn, "mboxes", "#mbox-cells"); + dev_dbg(dev, "Device has %d SPU channels", spu->num_chan); + + match = of_match_device(of_match_ptr(bcm_spu_dt_ids), dev); + matched_spu_type = (struct spu_type_subtype *)match->data; This cast usn't necessary. Ok, will remove. + spu->spu_type = matched_spu_type->type; + spu->spu_subtype = matched_spu_type->subtype; + + /* Read registers and count number of SPUs */ + i = 0; + while ((i < MAX_SPUS) && ((spu_ctrl_regs = + platform_get_resource(pdev, IORESOURCE_MEM, i)) != NULL)) { + dev_dbg(dev, + "SPU %d control register region res.start = %#x, res.end = %#x", + i, + (unsigned int)spu_ctrl_regs->start, + (unsigned int)spu_ctrl_regs->end); + + spu_reg_vbase[i] = devm_ioremap_resource(dev, spu_ctrl_regs); + if (IS_ERR(spu_reg_vbase[i])) { + err = PTR_ERR(spu_reg_vbase[i]); + dev_err(&pdev->dev, "Failed to map registers: %d\n", + err); + spu_reg_vbase[i] = NULL; + return err; + } + i++; + } These *really* sound like independent devices. There are no shared registers, and each has its own mbox. Why do we group them like this? As I said in the previous email, I want one instance of the driver to register crypto algos once with the crypto API and to distribute crypto requests among all available SPU hw blocks. Thanks, Mark.
Re: [PATCH 2/3] crypto: brcm: Add Broadcom SPU driver
On Wed, Nov 30, 2016 at 03:07:32PM -0500, Rob Rice wrote: > +static const struct of_device_id bcm_spu_dt_ids[] = { > + { > + .compatible = "brcm,spum-crypto", > + .data = &spum_ns2_types, > + }, > + { > + .compatible = "brcm,spum-nsp-crypto", > + .data = &spum_nsp_types, > + }, > + { > + .compatible = "brcm,spu2-crypto", > + .data = &spu2_types, > + }, > + { > + .compatible = "brcm,spu2-v2-crypto", > + .data = &spu2_v2_types, > + }, These last two weren't in the binding document. > + { /* sentinel */ } > +}; > + > +MODULE_DEVICE_TABLE(of, bcm_spu_dt_ids); > + > +static int spu_dt_read(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct spu_hw *spu = &iproc_priv.spu; > + struct device_node *dn = pdev->dev.of_node; > + struct resource *spu_ctrl_regs; > + const struct of_device_id *match; > + struct spu_type_subtype *matched_spu_type; > + void __iomem *spu_reg_vbase[MAX_SPUS]; > + int i; > + int err; > + > + if (!of_device_is_available(dn)) { > + dev_crit(dev, "SPU device not available"); > + return -ENODEV; > + } How can this happen? > + /* Count number of mailbox channels */ > + spu->num_chan = of_count_phandle_with_args(dn, "mboxes", "#mbox-cells"); > + dev_dbg(dev, "Device has %d SPU channels", spu->num_chan); > + > + match = of_match_device(of_match_ptr(bcm_spu_dt_ids), dev); > + matched_spu_type = (struct spu_type_subtype *)match->data; This cast usn't necessary. > + spu->spu_type = matched_spu_type->type; > + spu->spu_subtype = matched_spu_type->subtype; > + > + /* Read registers and count number of SPUs */ > + i = 0; > + while ((i < MAX_SPUS) && ((spu_ctrl_regs = > + platform_get_resource(pdev, IORESOURCE_MEM, i)) != NULL)) { > + dev_dbg(dev, > + "SPU %d control register region res.start = %#x, > res.end = %#x", > + i, > + (unsigned int)spu_ctrl_regs->start, > + (unsigned int)spu_ctrl_regs->end); > + > + spu_reg_vbase[i] = devm_ioremap_resource(dev, spu_ctrl_regs); > + if (IS_ERR(spu_reg_vbase[i])) { > + err = PTR_ERR(spu_reg_vbase[i]); > + dev_err(&pdev->dev, "Failed to map registers: %d\n", > + err); > + spu_reg_vbase[i] = NULL; > + return err; > + } > + i++; > + } These *really* sound like independent devices. There are no shared registers, and each has its own mbox. Why do we group them like this? Thanks, Mark.
Re: [PATCH 2/3] crypto: brcm: Add Broadcom SPU driver
Hi Rob, [auto build test ERROR on cryptodev/master] [also build test ERROR on v4.9-rc7 next-20161201] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Rob-Rice/crypto-brcm-DT-documentation-for-Broadcom-SPU-driver/20161202-010038 base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master config: arm64-defconfig (attached as .config) compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm64 All errors (new ones prefixed by >>): >> ERROR: "des_ekey" [drivers/crypto/bcm/bcm_crypto_spu.ko] undefined! --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip