On 20/02/2018 14:50, Andy Shevchenko wrote:

On Mon, Feb 19, 2018 at 7:48 PM, John Garry [off-list ref] wrote:

quoted

From: Zhichang Yuan <redacted>

The low-pin-count(LPC) interface of Hip06/Hip07 accesses the peripherals in
I/O port addresses. This patch implements the LPC host controller driver
which perform the I/O operations on the underlying hardware.
We don't want to touch those existing peripherals' driver, such as ipmi-bt.
So this driver applies the indirect-IO introduced in the previous patch
after registering an indirect-IO node to the indirect-IO devices list which
will be searched in the I/O accessors to retrieve the host-local I/O port.

The driver config is set as a bool instead of a trisate. The reason
here is that, by the very nature of the driver providing a logical
PIO range, it does not make sense to have this driver as a loadable
module. Another more specific reason is that the Huawei D03 board
which includes hip06 SoC requires the LPC bus for UART console, so
should be built in.

Hi Andy,

quoted

+config HISILICON_LPC
+       bool "Support for ISA I/O space on Hisilicon hip06/7"
+       depends on (ARM64 && (ARCH_HISI || COMPILE_TEST))

Redundant parens.

OK, these can be removed

quoted

+       select INDIRECT_PIO
+       help
+         Driver needed for some legacy ISA devices attached to Low-Pin-Count
+         on Hisilicon hip06/7 SoC.

quoted

+#if LPC_MAX_DULEN > LPC_MAX_BURST
+#error "LPC.. MAX_DULEN must be not bigger than MAX_OPCNT!"
+#endif

But here you can easily avoid an #error, by making them equal, just
issue a warning instead.

These values are dictated by the HW. Well the burst length is. 
Regardless I have simplified the driver not to use the burst mode so I 
can remove.

quoted

+#if LPC_MAX_BURST % LPC_MAX_DULEN
+#error "LPC.. LPC_MAX_BURST must be multiple of LPC_MAX_DULEN!"
+#endif

Is it like this, or also should be power of two?

As above

quoted

+/* The command register fields */
+#define LPC_CMD_SAMEADDR       0x08
+#define LPC_CMD_TYPE_IO                0x00

quoted

+#define LPC_CMD_WRITE          0x01
+#define LPC_CMD_READ           0x00
+/* the bit attribute is W1C. 1 represents OK. */
+#define LPC_STAT_BYIRQ         0x02

BIT() ?

Not all, but I'll fix them up to be clearer

quoted

+#define LPC_STATUS_IDLE                0x01
+#define LPC_OP_FINISHED                0x02
+
+#define LPC_START_WORK         0x01

Ditto?

quoted

+static inline int wait_lpc_idle(unsigned char *mbase,
+                               unsigned int waitcnt) {
+       u32 opstatus;
+
+       while (waitcnt--) {
+               ndelay(LPC_NSEC_PERWAIT);
+               opstatus = readl(mbase + LPC_REG_OP_STATUS);
+               if (opstatus & LPC_STATUS_IDLE)
+                       return (opstatus & LPC_OP_FINISHED) ? 0 : (-EIO);
+       }
+       return -ETIME;

Personally I prefer timeout loops in a do {} while (--count) style.

OK, I think it's fine to change as mentioned

quoted

+}

quoted

+static int
+hisi_lpc_target_in(struct hisi_lpc_dev *lpcdev, struct lpc_cycle_para *para,
+                 unsigned long addr, unsigned char *buf,
+                 unsigned long opcnt)
+{
+       unsigned int cmd_word;
+       unsigned int waitcnt;
+       unsigned long flags;
+       int ret;
+
+       if (!buf || !opcnt || !para || !para->csize || !lpcdev)
+               return -EINVAL;

quoted

+
+       cmd_word = LPC_CMD_TYPE_IO | LPC_CMD_READ;
+       waitcnt = LPC_PEROP_WAITCNT;
+       if (!(para->opflags & FG_INCRADDR_LPC)) {
+               cmd_word |= LPC_CMD_SAMEADDR;
+               waitcnt = LPC_MAX_WAITCNT;
+       }
+

quoted

+       ret = 0;
+

Sounds redundant.

it is, so I'll fix

quoted

+       /* whole operation must be atomic */
+       spin_lock_irqsave(&lpcdev->cycle_lock, flags);
+
+       writel_relaxed(opcnt, lpcdev->membase + LPC_REG_OP_LEN);
+
+       writel_relaxed(cmd_word, lpcdev->membase + LPC_REG_CMD);
+
+       writel_relaxed(addr, lpcdev->membase + LPC_REG_ADDR);
+
+       writel(LPC_START_WORK, lpcdev->membase + LPC_REG_START);
+
+       /* whether the operation is finished */
+       ret = wait_lpc_idle(lpcdev->membase, waitcnt);

quoted

+       if (!ret) {

I would rather go with usual pattern
if (ret) {
 ...
 return ret;
}

The intention was to not have 2 calls to free the spinlock. But we can 
go with the usual pattern.

quoted

+               for (; opcnt; opcnt--, buf++)
+                       *buf = readb(lpcdev->membase + LPC_REG_RDATA);

Looks like a do {} while (slightly better for my opinion).

do {
              *buf++ = readb(lpcdev->membase + LPC_REG_RDATA);
} while (--opcnt);

ok

quoted

+       }
+
+       spin_unlock_irqrestore(&lpcdev->cycle_lock, flags);
+
+       return ret;
+}

quoted

+       for (; opcnt; buf++, opcnt--)
+               writeb(*buf, lpcdev->membase + LPC_REG_WDATA);

Ditto.

Ditto

quoted

+static u32 hisi_lpc_comm_in(void *hostdata, unsigned long pio, size_t dwidth)

quoted

+       if (!lpcdev || !dwidth || dwidth > LPC_MAX_DULEN)
+               return -1;

~0 ?

I need to check the patchset for all of these :)

quoted

+       if (ret)
+               return -1;

Ditto.

quoted

+       do {
+               int ret;
+
+               ret = hisi_lpc_target_in(lpcdev, &iopara, addr,
+                                       buf, dwidth);
+               if (ret)
+                       return ret;
+               buf += dwidth;

quoted

+               count--;
+       } while (count);

} while (--count);

quoted

+       do {
+               if (hisi_lpc_target_out(lpcdev, &iopara, addr, buf,
+                                               dwidth))
+                       break;
+               buf += dwidth;
+               count--;
+       } while (count);

Ditto.

ok, we can use the do-while loop

quoted

+static int hisi_lpc_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct acpi_device *acpi_device = ACPI_COMPANION(dev);
+       struct logic_pio_hwaddr *range;
+       struct hisi_lpc_dev *lpcdev;
+       struct resource *res;

quoted

+       int ret = 0;

Redundant assignment.

Right

quoted

+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

quoted

+       if (!res)
+               return -ENODEV;

Redundant.

Right, devm_ioremap_resource() can deal with res = NULL.

quoted

+
+       lpcdev->membase = devm_ioremap_resource(dev, res);
+       if (IS_ERR(lpcdev->membase)) {

quoted

+               dev_err(dev, "remap failed\n");

Redundant.

right, an error is printed in devm_ioremap_resource()

quoted

+               return PTR_ERR(lpcdev->membase);
+       }

quoted

+       /* register the LPC host PIO resources */

quoted

+       if (!acpi_device)
+               ret = of_platform_populate(dev->of_node, NULL, NULL, dev);

quoted

+       if (ret) {

quoted

+               dev_err(dev, "populate children failed (%d)\n", ret);

JFYI: ret is printed by device core if ->probe() fails.

OK, so then this is superfluous

quoted

+               return ret;
+       }

This condition should go under if (!acpi_device) case.

Thanks,
John