Michael, Thanks for the review and comments.
On 04/04/2017 03:55 AM, Michael Ellerman wrote:
> 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?
We use FIFO size in skibbot to allocate FIFO buffer. I should export fifo size as device tree property and use it here.
>
>> + 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.
Compatible property is created with the latest VAS changes. So as suggested by Stewart, will remove search by xscom and use compatible property for VAS.
>
>
>> + 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?
Search has to be with in node. can I use of_get_child_by_name?
>
>> + 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.
Do you prefer creating compatible property under these nodes?
>
> Normal vs high should not be encoded in the name, it should be a
> property of the node.
Both 842 and gzip will have normal or high FIFOs and each one will contain rx-fifo-address, lpid, pid, and tid properties. So ibm.nx-842-high and ibm,nx-842-normal device nodes are created.
/proc/device-tree/xscom@603fc00000000/nx@2010000/ibm,nx-842-high
lpid phandle rx-fifo-address
name pid tid
So do you prefer ibm,nx-842/high/
lpid phandle rx-fifo-address
name pid tid
>
>> + 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
>
