linux-kernel - Re: [PATCH v15 6/9] HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1519932408.10722.351.camel@linux.intel.com>
Date:   Thu, 01 Mar 2018 21:26:48 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     John Garry <john.garry@...wei.com>,
        mika.westerberg@...ux.intel.com, rafael@...nel.org,
        lorenzo.pieralisi@....com, rjw@...ysocki.net,
        hanjun.guo@...aro.org, robh+dt@...nel.org, bhelgaas@...gle.com,
        arnd@...db.de, mark.rutland@....com, olof@...om.net,
        dann.frazier@...onical.com, andy.shevchenko@...il.com,
        robh@...nel.org
Cc:     joe@...ches.com, benh@...nel.crashing.org,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-acpi@...r.kernel.org, linuxarm@...wei.com, minyard@....org,
        devicetree@...r.kernel.org, linux-arch@...r.kernel.org,
        rdunlap@...radead.org, gregkh@...uxfoundation.org,
        akpm@...ux-foundation.org, frowand.list@...il.com, agraf@...e.de
Subject: Re: [PATCH v15 6/9] HISI LPC: Support the LPC host on Hip06/Hip07
 with DT bindings

On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote:

> 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.

Few minor comments below.

> +static inline int wait_lpc_idle(unsigned char *mbase,
> +				unsigned int waitcnt) {
> +	do {
> +		u32 status;
> +
> +		status = readl(mbase + LPC_REG_OP_STATUS);
> +		if (status & LPC_REG_OP_STATUS_IDLE)
> +			return (status & LPC_REG_OP_STATUS_FINISHED)
> ? 0 : -EIO;
> +		ndelay(LPC_NSEC_PERWAIT);

> +	} while (waitcnt--);

} while (--waitcnt);

> +
> +	return -ETIME;
> +}

> +
> +/*

If you would like to have a documentation you need to use proper syntax,
i.e.

/**

Check the rest of the series for it.

> + * hisi_lpc_target_in - trigger a series of LPC cycles for read
> operation
> + * @lpcdev: pointer to hisi lpc device
> + * @para: some parameters used to control the lpc I/O operations
> + * @addr: the lpc I/O target port address
> + * @buf: where the read back data is stored
> + * @opcnt: how many I/O operations required, i.e. data width
> + *
> + * Returns 0 on success, non-zero on fail.
> + */

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

readsb() ?

> +	do {
> +		writeb(*buf++, lpcdev->membase + LPC_REG_WDATA);
> +	} while (--opcnt);

writesb() ?

> +static inline unsigned long
> +hisi_lpc_pio_to_addr(struct hisi_lpc_dev *lpcdev, unsigned long pio)
> +{
> +	return pio - lpcdev->io_host->io_start +
> +		lpcdev->io_host->hw_start;

I would rather put on one line.

> +}

> +	do {
> +		if (hisi_lpc_target_out(lpcdev, &iopara, addr, buf,
> +						dwidth))

Fancy indentation. Perhaps put to one line?

> +			break;
> +		buf += dwidth;
> +	} while (--count);


> +	int ret;
> +
> +	lpcdev = devm_kzalloc(dev, sizeof(struct hisi_lpc_dev),
> GFP_KERNEL);

sizeof(*lpcdev) ?

> +	if (!lpcdev)
> +		return -ENOMEM;

> +	dev_info(dev, "registered range[%pa - sz:%pa]\n",

This is rather non-standard. We provide for resources the pattern like
"... [start-end]\n"

> +		 &lpcdev->io_host->io_start,
> +		 &lpcdev->io_host->size);

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy