[PATCH v1] i2c: octeon_i2c: Add I2C controller driver for Octeon

[Heiko Schocher]

Hello Stefan,

Am 14.05.2020 um 09:23 schrieb Stefan Roese:
> From: Suneel Garapati <sgarapati at marvell.com>
> 
> Add support for I2C controllers found on Octeon II/III and Octeon TX
> TX2 SoC platforms.
> 
> Signed-off-by: Aaron Williams <awilliams at marvell.com>
> Signed-off-by: Suneel Garapati <sgarapati at marvell.com>
> Signed-off-by: Stefan Roese <sr at denx.de>
> Cc: Heiko Schocher <hs at denx.de>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
> Cc: Aaron Williams <awilliams at marvell.com>
> Cc: Chandrakala Chavva <cchavva at marvell.com>
> ---
> RFC -> v1 (Stefan):
> - Separated this patch from the OcteonTX/TX2 RFC patch series into a
>    single patch. This is useful, as the upcoming MIPS Octeon support will
>    use this I2C driver.
> - Added MIPS Octeon II/III support (big endian). Rename driver and its
>    function names from "octeontx" to "octeon" to better match all Octeon
>    platforms.
> - Moved from union to defines / bitmasks as suggested by Simon. This makes
>    the driver usage on little- and big-endian platforms much easier.
> - Enhanced Kconfig text
> - Removed all clock macros (use values from DT)
> - Removed long driver debug strings. This is only available when a debug
>    version of this driver is built. The user / developer can lookup the
>    descriptive error messages in the driver in this case anyway.
> - Removed static "last_id"
> - Dropped misc blank lines. Misc reformatting.
> - Dropped "!= 0"
> - Added missing function comments
> - Added missing strut comments
> - Changed comment style
> - Renames "result" to "ret"
> - Hex numbers uppercase
> - Minor other changes
> - Reword commit text and subject
> 
>   drivers/i2c/Kconfig      |  10 +
>   drivers/i2c/Makefile     |   1 +
>   drivers/i2c/octeon_i2c.c | 803 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 814 insertions(+)
>   create mode 100644 drivers/i2c/octeon_i2c.c

nitpick only ...

Please add a documentation of the device tree bindings.

[...]
> diff --git a/drivers/i2c/octeon_i2c.c b/drivers/i2c/octeon_i2c.c
> new file mode 100644
> index 0000000000..210f98655e
> --- /dev/null
> +++ b/drivers/i2c/octeon_i2c.c
> @@ -0,0 +1,803 @@
> +// SPDX-License-Identifier:    GPL-2.0
> +/*
> + * Copyright (C) 2018 Marvell International Ltd.
> + */
> +
> +#include <common.h>
> +#include <i2c.h>
> +#include <dm.h>
> +#include <pci_ids.h>
> +#include <asm/io.h>
> +#include <asm/arch/clock.h>
> +#include <linux/bitfield.h>
> +
> +/*
> + * Octeon II/III (MIPS) have different register offsets than the ARM based
> + * Octeon TX/TX2 SoCs
> + */
> +#if defined(CONFIG_ARCH_OCTEON)
> +#define REG_OFFS		0x0000
> +#else
> +#define REG_OFFS		0x1000
> +#endif
> +
> +#define TWSI_SW_TWSI		(REG_OFFS + 0x00)
> +#define TWSI_TWSI_SW		(REG_OFFS + 0x08)

Is there no clearer name ?

Just wonderung about "TWSI" .. we already have an i2c driver with TWSI defines:

https://gitlab.denx.de/u-boot/u-boot/-/blob/master/drivers/i2c/mvtwsi.c

But it seems they have no common parts.

[...]
> +#if defined(CONFIG_ARCH_OCTEON)
> +static int get_io_clock(void)
> +{
> +	return octeon_get_io_clock();
> +}
> +#else
> +static int get_io_clock(void)
> +{
> +	return octeontx_get_io_clock();
> +}
> +#endif

Here would be good to have the clk framework...

> +
> +static int twsi_thp(void)
> +{
> +	if (IS_ENABLED(CONFIG_ARCH_OCTEON) || IS_ENABLED(CONFIG_ARCH_OCTEONTX))
> +		return 24;
> +	else
> +		return 3;
> +}

May you can add here some comments for this magic numbers?

[...]
> +#define RST_BOOT_PNR_MUL(val)  (((val) >> 33) & 0x1F)

not used define, please remove.

[...]

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de