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

[Stefan Roese <sr@denx.de>]

Hi Heiko,

On 19.05.20 08:07, Heiko Schocher wrote:
> Hello Stefan,
> 
> Am 14.05.2020 um 09:23 schrieb Stefan Roese:
>> From: Suneel Garapati <sgarapati@marvell.com>
>>
>> Add support for I2C controllers found on Octeon II/III and Octeon TX
>> TX2 SoC platforms.
>>
>> Signed-off-by: Aaron Williams <awilliams@marvell.com>
>> Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Heiko Schocher <hs@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>> Cc: Aaron Williams <awilliams@marvell.com>
>> Cc: Chandrakala Chavva <cchavva@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.

Okay.

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

TWSI is another abbreviation for I2C: Two Wire Serial Interface. I
would like to keep these macros as they reflect the register names in
the Cavium / Marvell manuals.

> [...]
>> +#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...

Yes, it would be great. I'm thinking about adding a minimal clk driver
for Octeon - not sure, if I get it done shortly though.

>> +
>> +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?

I didn't write the initial version of this driver, so its hard for
me to come up with details here. But I'll try.

> [...]
>> +#define RST_BOOT_PNR_MUL(val)? (((val) >> 33) & 0x1F)
> 
> not used define, please remove.

Ah, thanks for spotting.

Thanks,
Stefan