Hi Haren,
A few comments ...
Haren Myneni <haren@xxxxxxxxxxxxxxxxxx> writes:
> diff --git a/arch/powerpc/include/asm/vas.h b/arch/powerpc/include/asm/vas.h
> index 4e5a470..7315621 100644
> --- a/arch/powerpc/include/asm/vas.h
> +++ b/arch/powerpc/include/asm/vas.h
> @@ -19,6 +19,8 @@
> #define VAS_RX_FIFO_SIZE_MIN (1 << 10) /* 1KB */
> #define VAS_RX_FIFO_SIZE_MAX (8 << 20) /* 8MB */
>
> +#define is_vas_available() (cpu_has_feature(CPU_FTR_ARCH_300))
You shouldn't need that, it should all come from the device tree.
> diff --git a/drivers/crypto/nx/Kconfig b/drivers/crypto/nx/Kconfig
> index ad7552a..4ad7fdb 100644
> --- a/drivers/crypto/nx/Kconfig
> +++ b/drivers/crypto/nx/Kconfig
> @@ -38,6 +38,7 @@ config CRYPTO_DEV_NX_COMPRESS_PSERIES
> config CRYPTO_DEV_NX_COMPRESS_POWERNV
> tristate "Compression acceleration support on PowerNV platform"
> depends on PPC_POWERNV
> + select VAS
Don't select symbols that are user visible.
I'm not sure we actually want CONFIG_VAS to be user visible, but
currently it is so this should be 'depends on VAS'.
> diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c
> index 8737e90..66efd39 100644
> --- a/drivers/crypto/nx/nx-842-powernv.c
> +++ b/drivers/crypto/nx/nx-842-powernv.c
> @@ -554,6 +662,164 @@ static int nx842_powernv_decompress(const unsigned char *in, unsigned int inlen,
> wmem, CCW_FC_842_DECOMP_CRC);
> }
>
> +
> +static int __init vas_cfg_coproc_info(struct device_node *dn, int chip_id,
> + int vasid, int ct)
> +{
> + struct vas_window *rxwin, *txwin = NULL;
> + struct vas_rx_win_attr rxattr;
> + struct vas_tx_win_attr txattr;
> + struct nx842_coproc *coproc;
> + u32 lpid, pid, tid;
> + u64 rx_fifo;
> + int ret;
> +#define RX_FIFO_SIZE 0x8000
Where's that come from?
> + if (of_property_read_u64(dn, "rx-fifo-address", (void *)&rx_fifo)) {
> + pr_err("ibm,nx-842: Missing rx-fifo-address property\n");
The driver already declares pr_fmt(), so do you need the prefixes on
these pr_err()s ?
> + return -EINVAL;
> + }
> +
> + if (of_property_read_u32(dn, "lpid", &lpid)) {
> + pr_err("ibm,nx-842: Missing lpid property\n");
> + return -EINVAL;
> + }
> +
> + if (of_property_read_u32(dn, "pid", &pid)) {
> + pr_err("ibm,nx-842: Missing pid property\n");
> + return -EINVAL;
> + }
> +
> + if (of_property_read_u32(dn, "tid", &tid)) {
> + pr_err("ibm,nx-842: Missing tid property\n");
> + return -EINVAL;
> + }
> +
> + vas_init_rx_win_attr(&rxattr, ct);
> + rxattr.rx_fifo = (void *)rx_fifo;
> + rxattr.rx_fifo_size = RX_FIFO_SIZE;
> + rxattr.lnotify_lpid = lpid;
> + rxattr.lnotify_pid = pid;
> + rxattr.lnotify_tid = tid;
> + rxattr.wcreds_max = 64;
> +
> + /*
> + * Open a VAS receice window which is used to configure RxFIFO
> + * for NX.
> + */
> + rxwin = vas_rx_win_open(vasid, ct, &rxattr);
> + if (IS_ERR(rxwin)) {
> + pr_err("ibm,nx-842: setting RxFIFO with VAS failed: %ld\n",
> + PTR_ERR(rxwin));
> + return PTR_ERR(rxwin);
> + }
> +
> + /*
> + * Kernel requests will be high priority. So open send
> + * windows only for high priority RxFIFO entries.
> + */
> + if (ct == VAS_COP_TYPE_842_HIPRI) {
This if body looks like it should be a separate function to me.
> + vas_init_tx_win_attr(&txattr, ct);
> + txattr.lpid = 0; /* lpid is 0 for kernel requests */
> + txattr.pid = mfspr(SPRN_PID);
> +
> + /*
> + * Open a VAS send window which is used to send request to NX.
> + */
> + txwin = vas_tx_win_open(vasid, ct, &txattr);
> + if (IS_ERR(txwin)) {
> + pr_err("ibm,nx-842: Can not open TX window: %ld\n",
> + PTR_ERR(txwin));
> + ret = PTR_ERR(txwin);
> + goto err_out;
> + }
> + }
> +
> + coproc = kmalloc(sizeof(*coproc), GFP_KERNEL);
> + if (!coproc) {
> + ret = -ENOMEM;
> + goto err_out;
> + }
The error handling would be simpler if you did that earlier, before you
open the RX/TX windows.
> + coproc->chip_id = chip_id;
> + coproc->vas.rxwin = rxwin;
> + coproc->vas.txwin = txwin;
> +
> + INIT_LIST_HEAD(&coproc->list);
> + list_add(&coproc->list, &nx842_coprocs);
That duplicates logic in the non-vas probe, so ideally would be shared
or in a helper.
> +
> + return 0;
> +
> +err_out:
> + if (txwin)
> + vas_win_close(txwin);
> +
> + vas_win_close(rxwin);
> +
> + return ret;
> +}
> +
> +
> +static int __init nx842_powernv_probe_vas(struct device_node *dn)
> +{
> + struct device_node *nxdn, *np;
There's too many device nodes in this function.
> + int chip_id, vasid, rc;
> +
> + chip_id = of_get_ibm_chip_id(dn);
> + if (chip_id < 0) {
> + pr_err("ibm,chip-id missing\n");
> + return -EINVAL;
> + }
> +
> + np = of_find_node_by_name(dn, "vas");
You should always search by compatible when possible. I don't see why
you wouldn't here.
> + if (!np) {
> + pr_err("ibm,xscom: Missing VAS device node\n");
> + return -EINVAL;
> + }
> +
> + if (of_property_read_u32(np, "vas-id", &vasid)) {
> + pr_err("ibm,vas: Missing vas-id device property\n");
> + of_node_put(np);
> + return -EINVAL;
> + }
> +
> + of_node_put(np);
> +
> + nxdn = of_find_compatible_node(dn, NULL, "ibm,power-nx");
What are you trying to do here?
This will find any node in the device tree that is compatible with
"ibm,power-nx". It will start searching after dn in the device tree. But
it doesn't search the children of dn necessarily, is that what you're
trying to do?
> + if (!nxdn) {
> + pr_err("ibm,xscom: Missing NX device node\n");
> + return -EINVAL;
> + }
> +
> + np = of_find_node_by_name(nxdn, "ibm,nx-842-high");
Search by name again.
> + if (!np) {
> + pr_err("ibm,nx-842-high device node is missing\n");
> + rc = -EINVAL;
> + goto out_nd_put;
> + }
> +
> + rc = vas_cfg_coproc_info(np, chip_id, vasid, VAS_COP_TYPE_842_HIPRI);
> + of_node_put(np);
> + if (rc)
> + goto out_nd_put;
> +
> + np = of_find_node_by_name(nxdn, "ibm,nx-842-normal");
Search by name again.
Normal vs high should not be encoded in the name, it should be a
property of the node.
> + if (!np) {
> + pr_err("ibm,nx-842-normal device node is missing\n");
> + rc = -EINVAL;
> + goto out_nd_put;
> + }
> +
> + rc = vas_cfg_coproc_info(np, chip_id, vasid, VAS_COP_TYPE_842);
> + of_node_put(np);
> + if (!rc)
> + return 0;
> +
> +out_nd_put:
> + of_node_put(nxdn);
> + return rc;
> +}
> +
> static int __init nx842_powernv_probe(struct device_node *dn)
> {
> struct nx842_coproc *coproc;
> @@ -602,11 +868,42 @@ static void nx842_delete_coproc(void)
> struct nx842_coproc *coproc, *n;
>
> list_for_each_entry_safe(coproc, n, &nx842_coprocs, list) {
> + if (is_vas_available()) {
That should just be a check of coproc->vas.rxwin != NULL or similar.
> + vas_win_close(coproc->vas.rxwin);
> + /*
> + * txwin opened only for high priority RxFIFOs
> + */
> + if (coproc->vas.txwin)
> + vas_win_close(coproc->vas.txwin);
> + }
That should be pulled out into a helper, not in the middle of the loop
here.
> list_del(&coproc->list);
> kfree(coproc);
> }
> }
cheers
