[PATCH 1/2] i2c: ocores: add gaisler to platform data

[PATCH 1/2] i2c: ocores: add gaisler to platform data

[Mohammed Billoo]

There may be instances when the device tree is not suitable to interface
with the ocores implementation. For example, when the FPGA/ASIC is
not on the same silicon die (e.g. the communication between the CPU and
the FPGA/ASIC is over PCI), information about the ocore implementation,
such as whether the gaisler implementation is used, must be determined
during runtime. In this case, the client driver would prepopulate the
platform data during device instantiation. Thus, a boolean needs to be
added in the platform data, to instruct the i2c-ocores driver whether
the gaisler register callbacks should be used.

Signed-off-by: Mohammed Billoo <mab@mab-labs.com>
---
 drivers/i2c/busses/i2c-ocores.c          | 4 ++++
 include/linux/platform_data/i2c-ocores.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index f5fc75b65a19..0975f6797069 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -647,6 +647,10 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 			i2c->bus_clock_khz = pdata->bus_khz;
 		else
 			i2c->bus_clock_khz = 100;
+		if (pdata->gaisler) {
+			i2c->setreg = oc_setreg_grlib;
+			i2c->getreg = oc_getreg_grlib;
+		}
 	} else {
 		ret = ocores_i2c_of_probe(pdev, i2c);
 		if (ret)
diff --git a/include/linux/platform_data/i2c-ocores.h b/include/linux/platform_data/i2c-ocores.h
index e6326cbafe59..8a5849f1e267 100644
--- a/include/linux/platform_data/i2c-ocores.h
+++ b/include/linux/platform_data/i2c-ocores.h
@@ -14,6 +14,7 @@ struct ocores_i2c_platform_data {
 	u32 clock_khz; /* input clock in kHz */
 	u32 bus_khz; /* bus clock in kHz */
 	bool big_endian; /* registers are big endian */
+	bool gaisler; /* use grlib accessors */
 	u8 num_devices; /* number of devices in the devices list */
 	struct i2c_board_info const *devices; /* devices connected to the bus */
 };
-- 
2.17.1

[PATCH 2/2] i2c: ocores: add be/le support for gaisler

[Mohammed Billoo]

Add be/le accessors for grlib (as is done for the standard ocore
accessors).

Signed-off-by: Mohammed Billoo <mab@mab-labs.com>
---
 arch/arm/configs/socfpga_defconfig |  1 +
 drivers/i2c/busses/i2c-ocores.c    | 55 ++++++++++++++++++++++++++----
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/arch/arm/configs/socfpga_defconfig b/arch/arm/configs/socfpga_defconfig
index e73c97b0f5b0..55bf9cfcf75c 100644
--- a/arch/arm/configs/socfpga_defconfig
+++ b/arch/arm/configs/socfpga_defconfig
@@ -162,3 +162,4 @@ CONFIG_DETECT_HUNG_TASK=y
 # CONFIG_SCHED_DEBUG is not set
 CONFIG_FUNCTION_TRACER=y
 CONFIG_DEBUG_USER=y
+CONFIG_I2C_OCORES=y
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 0975f6797069..a5f9e6cb4814 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -492,7 +492,7 @@ MODULE_DEVICE_TABLE(of, ocores_i2c_match);
  * 32-bit big endian and the PRELOW and PREHIGH registers are merged into one
  * register. The subsequent registers have their offsets decreased accordingly.
  */
-static u8 oc_getreg_grlib(struct ocores_i2c *i2c, int reg)
+static u8 oc_getreg_grlib_be(struct ocores_i2c *i2c, int reg)
 {
 	u32 rd;
 	int rreg = reg;
@@ -506,7 +506,7 @@ static u8 oc_getreg_grlib(struct ocores_i2c *i2c, int reg)
 		return (u8)rd;
 }
 
-static void oc_setreg_grlib(struct ocores_i2c *i2c, int reg, u8 value)
+static void oc_setreg_grlib_be(struct ocores_i2c *i2c, int reg, u8 value)
 {
 	u32 curr, wr;
 	int rreg = reg;
@@ -525,6 +525,39 @@ static void oc_setreg_grlib(struct ocores_i2c *i2c, int reg, u8 value)
 	iowrite32be(wr, i2c->base + (rreg << i2c->reg_shift));
 }
 
+static u8 oc_getreg_grlib_le(struct ocores_i2c *i2c, int reg)
+{
+	u32 rd;
+	int rreg = reg;
+
+	if (reg != OCI2C_PRELOW)
+		rreg--;
+	rd = ioread32(i2c->base + (rreg << i2c->reg_shift));
+	if (reg == OCI2C_PREHIGH)
+		return (u8)(rd >> 8);
+	else
+		return (u8)rd;
+}
+
+static void oc_setreg_grlib_le(struct ocores_i2c *i2c, int reg, u8 value)
+{
+	u32 curr, wr;
+	int rreg = reg;
+
+	if (reg != OCI2C_PRELOW)
+		rreg--;
+	if (reg == OCI2C_PRELOW || reg == OCI2C_PREHIGH) {
+		curr = ioread32(i2c->base + (rreg << i2c->reg_shift));
+		if (reg == OCI2C_PRELOW)
+			wr = (curr & 0xff00) | value;
+		else
+			wr = (((u32)value) << 8) | (curr & 0xff);
+	} else {
+		wr = value;
+	}
+	iowrite32(wr, i2c->base + (rreg << i2c->reg_shift));
+}
+
 static int ocores_i2c_of_probe(struct platform_device *pdev,
 				struct ocores_i2c *i2c)
 {
@@ -592,8 +625,13 @@ static int ocores_i2c_of_probe(struct platform_device *pdev,
 	match = of_match_node(ocores_i2c_match, pdev->dev.of_node);
 	if (match && (long)match->data == TYPE_GRLIB) {
 		dev_dbg(&pdev->dev, "GRLIB variant of i2c-ocores\n");
-		i2c->setreg = oc_setreg_grlib;
-		i2c->getreg = oc_getreg_grlib;
+		if (of_device_is_big_endian(pdev->dev.of_node)) {
+			i2c->setreg = oc_setreg_grlib_be;
+			i2c->getreg = oc_getreg_grlib_be;
+		} else {
+			i2c->setreg = oc_setreg_grlib_le;
+			i2c->getreg = oc_getreg_grlib_le;
+		}
 	}
 
 	return 0;
@@ -648,8 +686,13 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 		else
 			i2c->bus_clock_khz = 100;
 		if (pdata->gaisler) {
-			i2c->setreg = oc_setreg_grlib;
-			i2c->getreg = oc_getreg_grlib;
+			if (pdata->big_endian) {
+				i2c->setreg = oc_setreg_grlib_be;
+				i2c->getreg = oc_getreg_grlib_be;
+			} else {
+				i2c->setreg = oc_setreg_grlib_le;
+				i2c->getreg = oc_getreg_grlib_le;
+			}
 		}
 	} else {
 		ret = ocores_i2c_of_probe(pdev, i2c);
-- 
2.17.1

Re: [PATCH 2/2] i2c: ocores: add be/le support for gaisler

[Andrew Lunn]

On Fri, Aug 14, 2020 at 12:31:34PM -0400, Mohammed Billoo wrote:
> Add be/le accessors for grlib (as is done for the standard ocore
> accessors).

I thought PCI standardised the endianness of the bus? So long as you
use the right wrappers, it should not matter what the endianness of
the CPU is.

    Andrew

Re: [PATCH 2/2] i2c: ocores: add be/le support for gaisler

[Andrew Lunn]

On Fri, Aug 14, 2020 at 02:56:58PM -0400, Mohammed Billoo wrote:
> Sadly, this is a case of vendor-specific implementations causing indigestion.
> When this was implemented on the FPGA, the vendor-specific AHB bridge (we're
> going from PCI to AHB to grlib) had a built-in endianness conversion which we
> didn't want and found out later. Since it was easier to add the big endian
> accessor on the software side instead of redoing the FPGA design, we opted for
> the former. 

O.K. So please document this is a workaround for broken hardware.
Otherwise other people are going to ask the same question. Add it to
the commit message at least.

    Andrew

Re: [PATCH 2/2] i2c: ocores: add be/le support for gaisler

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

On Fri, Aug 14, 2020 at 09:02:59PM +0200, Andrew Lunn wrote:
> On Fri, Aug 14, 2020 at 02:56:58PM -0400, Mohammed Billoo wrote:
> > Sadly, this is a case of vendor-specific implementations causing indigestion.
> > When this was implemented on the FPGA, the vendor-specific AHB bridge (we're
> > going from PCI to AHB to grlib) had a built-in endianness conversion which we
> > didn't want and found out later. Since it was easier to add the big endian
> > accessor on the software side instead of redoing the FPGA design, we opted for
> > the former. 
> 
> O.K. So please document this is a workaround for broken hardware.
> Otherwise other people are going to ask the same question. Add it to
> the commit message at least.

Very true, this should be documented. I think a comment in the source
code makes more sense.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: ocores: add gaisler to platform data

[Andrew Lunn]

On Fri, Aug 14, 2020 at 12:31:33PM -0400, Mohammed Billoo wrote:
> There may be instances when the device tree is not suitable to interface
> with the ocores implementation. For example, when the FPGA/ASIC is
> not on the same silicon die (e.g. the communication between the CPU and
> the FPGA/ASIC is over PCI), information about the ocore implementation,
> such as whether the gaisler implementation is used, must be determined
> during runtime. In this case, the client driver would prepopulate the
> platform data during device instantiation. Thus, a boolean needs to be
> added in the platform data, to instruct the i2c-ocores driver whether
> the gaisler register callbacks should be used.
> 
> Signed-off-by: Mohammed Billoo <mab@mab-labs.com>
> ---
>  drivers/i2c/busses/i2c-ocores.c          | 4 ++++
>  include/linux/platform_data/i2c-ocores.h | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index f5fc75b65a19..0975f6797069 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -647,6 +647,10 @@ static int ocores_i2c_probe(struct platform_device *pdev)
>  			i2c->bus_clock_khz = pdata->bus_khz;
>  		else
>  			i2c->bus_clock_khz = 100;
> +		if (pdata->gaisler) {
> +			i2c->setreg = oc_setreg_grlib;
> +			i2c->getreg = oc_getreg_grlib;
> +		}
>  	} else {
>  		ret = ocores_i2c_of_probe(pdev, i2c);
>  		if (ret)
> diff --git a/include/linux/platform_data/i2c-ocores.h b/include/linux/platform_data/i2c-ocores.h
> index e6326cbafe59..8a5849f1e267 100644
> --- a/include/linux/platform_data/i2c-ocores.h
> +++ b/include/linux/platform_data/i2c-ocores.h
> @@ -14,6 +14,7 @@ struct ocores_i2c_platform_data {
>  	u32 clock_khz; /* input clock in kHz */
>  	u32 bus_khz; /* bus clock in kHz */
>  	bool big_endian; /* registers are big endian */
> +	bool gaisler; /* use grlib accessors */

Hi Mohammed

A collection of bools does not scale too well. How about an enum. And
you could add values for all the current accessorrs.

It is also possible to add a device tree node to a PCI device. So
platform data is not the only way to do this.

	 Andrew

Re: [PATCH 1/2] i2c: ocores: add gaisler to platform data

[Andrew Lunn]

On Fri, Aug 14, 2020 at 02:53:20PM -0400, Mohammed Billoo wrote:
> Andrew,
> 
> Thanks for the feedback. That makes sense.
> 
> My use case was very specific that required these changes and I thought I'd
> push them upstream for others. Basically, we have an FPGA over PCI that has
> some IP, vendor-specific bridging to AHB, and then grlib to go to i2c (among
> other controllers). When we load the pci driver we create and add a mfd device
> that has the specific BAR as the resource, and the offset within the BAR for
> the i2c controller. Looking back, all of this could be added to the device
> tree, so what we've done so far may be an anti-pattern. 
> 
> Would you still want this patch?

I would prefer device tree.

  Andrew