quoted

quoted

quoted

+ * The port size of legacy I/O devices is normally less than 0x400.
+ * Defining the I/O range size as 0x400 here should be sufficient for
+ * all peripherals under one bus.
+ */

This comment doesn't make a lot of sense. What is the limit? Is there a
hardware limit?

We don't dynamically allocate devices on the lpc bus, so why imply a
limit at all?

IIRC from previously asking Zhichang this before, this is the upper
range we can address devices on the LPC bus. But the value was 0x1000
then.

Well, all devices that we want to address are defined by firmware (via
device tree or dsdt). So I'm not quite sure what this arbitrary limit
buys us.

Will check with Zhichang.

quoted

quoted

quoted

+#define LPC_BUS_IO_SIZE        0x400
+

<snip>

quoted

quoted

quoted

+    ret = 0;
+    cnt_per_trans = (para->csize == 1) ? opcnt : para->csize;
+    for (; opcnt && !ret; cnt_per_trans = para->csize) {
+        unsigned long flags;
+
+        /* whole operation must be atomic */
+        spin_lock_irqsave(&lpcdev->cycle_lock, flags);

Ouch. This is going to kill your RT jitter. Is there no better way?

Obviously the bus register driving is non-atomic, so we need some way to
lock out.

I think that it is not so critical for low-speed/infrequent-access bus.

If we were going to use virtual UART in the BMC on the LPC bus then we
could consider more.

Well, it basically means that an arbitrary daemon running in user space
that checks your temperature readings via the ipmi interface could
create a lot of jitter. That could be very critical if you want to use
this hardware for real time critical applications, such as telecom.

I bet that if you leave it like that and postpone the decision to fix it
to "later", in 1 or 2 years you will cause someone weeks of debugging to
track down why their voip gateway loses packets from time to time.

quoted

We need to consider this more. There may a way to access the registers 
and drive the bus without requiring a lock, but I doubt it.

Note: I think that we could make some readl/writel relaxed, which would 
help.

quoted

quoted

quoted

+
+        writel(cnt_per_trans, lpcdev->membase + LPC_REG_OP_LEN);
+
+        writel(cmd_word, lpcdev->membase + LPC_REG_CMD);
+
+        writel(ptaddr, lpcdev->membase + LPC_REG_ADDR);
+
+        writel(START_WORK, lpcdev->membase + LPC_REG_START);
+
+        /* whether the operation is finished */
+        ret = wait_lpc_idle(lpcdev->membase, waitcnt);
+        if (!ret) {
+            opcnt -= cnt_per_trans;
+            for (; cnt_per_trans--; buf++)
+                *buf = readl(lpcdev->membase + LPC_REG_RDATA);
+        }
+
+        spin_unlock_irqrestore(&lpcdev->cycle_lock, flags);
+    }
+
+    return ret;
+}

<snip>

quoted

quoted

quoted

+ * hisilpc_comm_in - read/input the data from the I/O peripheral
+ *             through LPC.
+ * @devobj: pointer to the device information relevant to LPC
controller.
+ * @pio: the target I/O port address.
+ * @dlen: the data length required to read from the target I/O port.
+ *
+ * when succeed, the data read back is stored in buffer pointed by
inbuf.
+ * For inb, return the data read from I/O or -1 when error occur.
+ */
+static u64 hisilpc_comm_in(void *devobj, unsigned long pio, size_t
dlen)
+{
+    struct hisilpc_dev *lpcdev = devobj;
+    struct lpc_cycle_para iopara;
+    u32 rd_data;

rd_data needs to be initialized to 0. Otherwise it may contain stale
stack contents and corrupt non-32bit dlen returns.

I think so, since we read into this value byte-by-byte. We also seem to
return a 32b value but should return 64b value according to the
prototype.

IIRC LPC (well, PIO) doesn't support bigger requests than 32bit. At
least I can't think of an x86 instruction that would allow bigger
transactions. So there's no need to make it 64bit. However, the question
is why the prototype is 64bit then. Hm. :)

Maybe the prototype should be only 32bit.

Will check with Zhichang.

quoted

quoted

quoted

+    unsigned char *newbuf;
+    int ret = 0;
+    unsigned long ptaddr;
+
+    if (!lpcdev || !dlen || dlen > LPC_MAX_DULEN ||    (dlen & (dlen
- 1)))
+        return -1;

Isn't this -EINVAL?

Not sure. This value is returned directly to the inb/outb caller, which
would not check this value for error.

It could be argued that the checking is paranoia. If not, we should
treat the failure as a more severe event.

Oh, I see. In that case -1 makes a lot of sense since it's the default
read value on x86 for unallocated space.

This probably deserves a comment (and/or maybe a #define)

We can add a comment

Alex

.