On 2016/5/26 4:34, Doug Ledford wrote:
-------- Forwarded Message --------
Subject: Re: [PATCH v8 07/22] IB/hns: Add event queue support
Date: Wed, 25 May 2016 15:22:05 -0400
From: Doug Ledford <dledford@xxxxxxxxxx>
Organization: Red Hat, Inc.
To: Lijun Ou <oulijun@xxxxxxxxxx>
On 05/25/2016 11:05 AM, Lijun Ou wrote:
This patch added event queue support for RoCE driver. It is used
for RoCE interrupt. RoCE includes 32 synchronous event irqs, 1
asynchronous event irq and 1 common overflow irq.
+#define roce_writel(value, addr) writel((value), (addr))
+#define roce_readl(addr) readl((addr))
These two defines are useless. They just make the code width longer for
no real purpose. Here's an example from my last email
+ ((u64)le32_to_cpu(roce_readl(hr_dev->reg_base +
+ ROCEE_SYS_IMAGE_GUID_H_REG)) <<
+ ADDR_SHIFT_32);
If you are going to do a #define for this, at least let the #define do
some work for you. Something like:
#define hns_readl(dev, reg) readl((dev)->reg_base + (reg))
then the above can read more like this
+ ((u64)le32_to_cpu(hns_readl(hr_dev,
+ ROCEE_SYS_IMAGE_GUID_H_REG)) <<
+ ADDR_SHIFT_32);
You could do additional tricks to make the overall length of lines
shorter while loosing none of the readability if you altered your
register names a little. And if all of your ROCEE registers have a
common prefix or postfix, you can embedd that in the #define to keep the
register names shorter in your file. For instance, they appear to all
start with ROCEE and end with REG, so maybe something like this:
#define hns_readl(dev, reg) readl((dev->)reg_base + ROCEE_##reg##_REG)
so that the above starts to look like this:
+ ((u64)le32_to_cpu(hns_readl(hr_dev,
+ SYS_IMAGE_GUID_H)) << 32);
which is much more readable in my opinion.
+/* Address shift 12bit with the special hardware address operation of RoCEE */
+#define ADDR_SHIFT_12 12
+
And this is the one that should talk about the register that requires
the address be shifted as though using a 4K page size even when you aren't.
Yes, we use a 4K page, but we can't use PAGE_SHIFT in the code.
PAGE_SHIFT maybe is not equal 12.
your meaning is that we use literal number 12, right?
Regards
Wei Hu
/* Address shift 32bit with the special hardware address operation of RoCEE */
#define ADDR_SHIFT_32 32
I really don't know when you actually need a define for a 32 bit shift....
+/* Address shift 44bit with the special hardware address operation of RoCEE */
+#define ADDR_SHIFT_44 44
This one, just like the 12 shift, needs an explanation of some sort.
44 = 32 + 12
When evaluating addr to hardware, shift 12 because of using 4K page,
and shift more 32 because of caculating the high 32 bit value evaluated
to hardware.
We prepare to delete the definition of macro ADDR_SHIFT_44, use literal
44 directly in code,
add comments.
Regards
Wei HU
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
