i2c: Driver to expose PowerNV platform i2c busses
Subsystems (2)
PATCH v2 1/1 i2c: Driver to expose PowerNV platform i2c busses
The patch exposes the available i2c busses on the PowerNV platform to the kernel and implements the bus driver to support i2c and smbus commands.
The driver uses the platform device infrastructure to probe the busses on the platform and registers them with the i2c driver framework.
--- arch/powerpc/include/asm/opal.h | 29 ++ arch/powerpc/platforms/powernv/opal-wrappers.S | 1 arch/powerpc/platforms/powernv/opal.c | 11 + drivers/i2c/busses/Kconfig | 11 + drivers/i2c/busses/Makefile | 1 drivers/i2c/busses/i2c-opal.c | 295 ++++++++++++++++++++++++ 6 files changed, 348 insertions(+) create mode 100644 drivers/i2c/busses/i2c-opal.c
arch/powerpc/include/asm/opal.h +29 -0
@@ -56,6 +56,14 @@ struct opal_sg_list {#define OPAL_HARDWARE_FROZEN -13#define OPAL_WRONG_STATE -14#define OPAL_ASYNC_COMPLETION -15 +#define OPAL_I2C_TIMEOUT -17 +#define OPAL_I2C_INVALID_CMD -18 +#define OPAL_I2C_LBUS_PARITY -19 +#define OPAL_I2C_BKEND_OVERRUN -20 +#define OPAL_I2C_BKEND_ACCESS -21 +#define OPAL_I2C_ARBT_LOST -22 +#define OPAL_I2C_NACK_RCVD -23 +#define OPAL_I2C_STOP_ERR -24/* API Tokens (in r0) */ #define OPAL_INVALID_CALL -1
@@ -154,6 +162,7 @@ struct opal_sg_list {#define OPAL_HANDLE_HMI 98#define OPAL_REGISTER_DUMP_REGION 101#define OPAL_UNREGISTER_DUMP_REGION 102 +#define OPAL_I2C_REQUEST 109#ifndef __ASSEMBLY__
@@ -801,6 +810,24 @@ typedef struct oppanel_line {uint64_t line_len; } oppanel_line_t; +/* OPAL I2C request */ +struct opal_i2c_request { +uint8_t type; +#define OPAL_I2C_RAW_READ 0 +#define OPAL_I2C_RAW_WRITE 1 +#define OPAL_I2C_SM_READ 2 +#define OPAL_I2C_SM_WRITE 3 +uint8_t flags; +#define OPAL_I2C_ADDR_10 0x01/* Not supported yet */ +uint8_t subaddr_sz;/* Max 4 */ +uint8_t reserved; +__be16 addr;/* 7 or 10 bit address */ +__be16 reserved2; +__be32 subaddr;/* Sub-address if any */ +__be32 size;/* Data size */ +__be64 buffer_ra;/* Buffer real address */ +}; +/* /sys/firmware/opal */ extern struct kobject *opal_kobj;
@@ -963,6 +990,8 @@ int64_t opal_handle_hmi(void);int64_t opal_register_dump_region (uint32_t id,uint64_t start,uint64_t end);int64_t opal_unregister_dump_region (uint32_t id);int64_t opal_pci_set_phb_cxl_mode (uint64_t phb_id,uint64_t mode,uint64_t pe_number); +int64_t opal_i2c_request (uint64_t async_token,uint32_t bus_id, +struct opal_i2c_request *oreq);/* Internal functions */ extern int early_init_dt_scan_opal(unsigned long node, const char *uname,
arch/powerpc/platforms/powernv/opal-wrappers.S +1 -0
@@ -250,3 +250,4 @@ OPAL_CALL(opal_handle_hmi, OPAL_HANDLE_HMI); OPAL_CALL(opal_register_dump_region, OPAL_REGISTER_DUMP_REGION); OPAL_CALL(opal_unregister_dump_region, OPAL_UNREGISTER_DUMP_REGION); OPAL_CALL(opal_pci_set_phb_cxl_mode, OPAL_PCI_SET_PHB_CXL_MODE); +OPAL_CALL(opal_i2c_request, OPAL_I2C_REQUEST);
arch/powerpc/platforms/powernv/opal.c +11 -0
@@ -667,6 +667,14 @@ static void opal_console_create_devs(void) } +static void opal_i2c_create_devs (void ) +{ +struct device_node *np; + +for_each_compatible_node (np,NULL ,"ibm,opal-i2c" ) +of_platform_device_create (np,NULL ,NULL ); +} +static void opal_request_interrupts (void ) {const __be32 *irqs;
@@ -732,6 +740,9 @@ static int __init opal_init(void)/* Create console platform devices */ opal_console_create_devs (); +/* Create i2c platform devices */ +opal_i2c_create_devs (); +/* Register OPAL interrupts */ opal_request_interrupts ();
drivers/i2c/busses/Kconfig +11 -0
@@ -1044,4 +1044,15 @@ config SCx200_ACB This support is also available as a module. If so, the module will be called scx200_acb. +config I2C_OPAL + tristate "IBM OPAL I2C driver" + depends on PPC_POWERNV + default y + help + This exposes the PowerNV platform i2c busses to the linux i2c layer, + the driver is based on the OPAL interfaces. + + This driver can also be built as a module. If so, the module will be + called as i2c-opal. + endmenu
drivers/i2c/busses/Makefile +1 -0
@@ -102,5 +102,6 @@ obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o obj-$(CONFIG_SCx200_ACB) += scx200_acb.o +obj-$(CONFIG_I2C_OPAL) += i2c-opal.o ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
drivers/i2c/busses/i2c-opal.c new file +295 -0
295 lines not on this page · raw message
A message that is not in the archive (sent off-list or not yet received): 20141116171605.4750.17472.stgit-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
Quoted 9 lines · Neelesh Gupta
The patch exposes the available i2c busses on the PowerNV platform to the kernel and implements the bus driver to support i2c and smbus commands.
The driver uses the platform device infrastructure to probe the busses on the platform and registers them with the i2c driver framework.Signed-off-by Neelesh Gupta <[email protected]>Signed-off-by Benjamin Herrenschmidt <[email protected]>---
Hi Wolfram,
Did you get the chance to look at it?
The patch addresses the comments and discussions on v1.
Thanks,
Neelesh
Quoted 14 lines
arch/powerpc/include/asm/opal.h | 29 ++ arch/powerpc/platforms/powernv/opal-wrappers.S | 1 arch/powerpc/platforms/powernv/opal.c | 11 + drivers/i2c/busses/Kconfig | 11 + drivers/i2c/busses/Makefile | 1 drivers/i2c/busses/i2c-opal.c | 295 ++++++++++++++++++++++++ 6 files changed, 348 insertions(+)create mode 100644
drivers/i2c/busses/i2c-opal.carch/powerpc/include/asm/opal.h +0
-0
A message that is not in the archive (sent off-list or not yet received): 546DF910.4060802-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org
Quoted 8 lines · Neelesh Gupta
Quoted 6 lines · Neelesh Gupta
The patch exposes the available i2c busses on the PowerNV platform to the kernel and implements the bus driver to support i2c and smbus commands.
The driver uses the platform device infrastructure to probe the busses on the platform and registers them with the i2c driver framework.
Quoted 8 lines
Quoted 3 lines
Signed-off-by Neelesh Gupta <[email protected]>Signed-off-by Benjamin Herrenschmidt <[email protected]>---
Hi Wolfram,
Did you get the chance to look at it?
The patch addresses the comments and discussions on v1.
I think there are now 3 drivers in my queue which are not fully I2C compatible but more supporting the very minimum to, say, read an eeprom. I am not feeling well to allow them to use I2C_FUNC_I2C. So, I want to think about ways how to communicate deficiencies like "only 255 byte" or "only WRRD messages" to users of that I2C controller. This is most likely not happening before 3.19. But assistance is very welcome.
Quoted 9 lines · Wolfram Sang
I think there are now 3 drivers in my queue which are not fully I2C compatible but more supporting the very minimum to, say, read an eeprom.
I am not feeling well to allow them to useI2C_FUNC_I2C. So, I want to think about ways how to communicate deficiencies like "only 255 byte" or
"only WRRD messages" to users of that I2C controller. This is most likely not happening before 3.19. But assistance is very welcome.
There are drivers doing that already, this is afaik common practice.
Please don't gate the merging process to some hypothetical rework that will imply reworking also a number of user space tools and in-kernel i2c device drivers. Basically that would mean "your platform won't be supported upstream for the next year and btw, rewrite all userspace".
By all means let's create new "smbus" APIs for >1 bytes offset but let's not make it a gate to merging drivers using the existing way of doing things that has been so far perfectly functional.
Cheers,
Ben.
A message that is not in the archive (sent off-list or not yet received): 1416889937.4998.54.camel-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org
Quoted 11 lines · Benjamin Herrenschmidt
Quoted 9 lines · Wolfram Sang
I think there are now 3 drivers in my queue which are not fully I2C compatible but more supporting the very minimum to, say, read an eeprom.
I am not feeling well to allow them to useI2C_FUNC_I2C. So, I want to think about ways how to communicate deficiencies like "only 255 byte" or
"only WRRD messages" to users of that I2C controller. This is most likely not happening before 3.19. But assistance is very welcome.
Quoted 1 line
There are drivers doing that already, this is afaik common practice.
Well, there are drivers doing it, but to me this is somewhere between a hack and a workaround. Somehow like using subsys_initcall() to overcome probe ordering issues. That has never been nice and I still don't have a solution how to convert those drivers back to module_init() + deferred probe without causing regressions. As a result, people still want to copy that behaviour.
Quoted 4 lines
Please don't gate the merging process to some hypothetical rework that will imply reworking also a number of user space tools and in-kernel i2c device drivers. Basically that would mean "your platform won't be supported upstream for the next year and btw, rewrite all userspace".
Call me naive but I don't think we need to rewrite all userspace. That is what I am at least aiming for.
Quoted 3 lines
By all means let's create new "smbus" APIs for >1 bytes offset but let's not make it a gate to merging drivers using the existing way of doing things that has been so far perfectly functional.
You say "let's", yet my experience is that once I accept a patch like yours, people are off with their support leaving me alone with the topic. Not because they are evil, but because they also have tons of other stuff to do. I perfectly understand I know this myself. And then, more and more people will come asking for the same as you and the situation gets harder and harder to fix.
That all being said, the driver has other issues which I will mention in a seperate thread.
Quoted 8 lines · Neelesh Gupta
The patch exposes the available i2c busses on the PowerNV platform to the kernel and implements the bus driver to support i2c and smbus commands.
The driver uses the platform device infrastructure to probe the busses on the platform and registers them with the i2c driver framework.Signed-off-by Neelesh Gupta <[email protected]>Signed-off-by Benjamin Herrenschmidt <[email protected]>
...
Quoted 25 lines
drivers/i2c/busses/Kconfig +11
-0@@ -1044,4 +1044,15 @@ config SCx200_ACB This support is also available as a module. If so, the module will be called scx200_acb. +config I2C_OPAL + tristate "IBM OPAL I2C driver" + depends on PPC_POWERNV + default y + help + This exposes the PowerNV platform i2c busses to the linux i2c layer, + the driver is based on the OPAL interfaces. + + This driver can also be built as a module. If so, the module will be + called as i2c-opal. + endmenudrivers/i2c/busses/Makefile +0
-0
drivers/i2c/busses/Makefile obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o · line 105
Quoted 4 lines
drivers/i2c/busses/Makefile +1
-0@@ -102,5 +102,6 @@ obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o obj-$(CONFIG_SCx200_ACB) += scx200_acb.o +obj-$(CONFIG_I2C_OPAL) += i2c-opal.o
Please keep it proprly sorted.
drivers/i2c/busses/Makefile obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o
Quoted 5 lines
drivers/i2c/busses/Makefile +5
-0+ rc = of_property_read_u32(pdev->dev.of_node, "ibm,opal-id", &opal_id); + if (rc) { + dev_err(&pdev->dev, "Missing ibm,opal-id property !\n"); + return -EIO; + }
You introduce new bindings which need to be documented in Docuemntation/devicetree/bindings/i2c.
They should be posted as a seperate patch with [email protected] CCed, so they can comment on it. This is required these days and especially important..
drivers/i2c/busses/Makefile
Quoted 13 lines
drivers/i2c/busses/Makefile +13
-0+ adapter = devm_kzalloc(&pdev->dev, sizeof(*adapter), GFP_KERNEL); + if (!adapter) + return -ENOMEM; + + adapter->algo = &i2c_opal_algo; + adapter->algo_data = (void *)(unsigned long)opal_id; + adapter->dev.parent = &pdev->dev; + adapter->dev.of_node = of_node_get(pdev->dev.of_node); + pname = of_get_property(pdev->dev.of_node, "ibm,port-name", NULL); + if (pname) + strlcpy(adapter->name, pname, sizeof(adapter->name)); + else + strlcpy(adapter->name, "opal", sizeof(adapter->name));
... because I'd like to get an ack from them because of this binding. I don't know if we can just say "this comes from firmware, so we must support it" (although you wrote the firmware IIUC) or if we have to judge if this is a HW description which should go into DT? I am open meanwhile that the adapter name does not need to be static anymore. However, I don't know much about server world and FW, so maybe they can assist.
An example binding in that document would also be very helpful.
drivers/i2c/busses/Makefile
Quoted 6 lines
drivers/i2c/busses/Makefile +6
-0+static struct platform_driver i2c_opal_driver = { + .probe = i2c_opal_probe, + .remove = i2c_opal_remove, + .driver = { + .name = "i2c-opal", + .owner = THIS_MODULE,
owner not needed.
Thanks,
Wolfram
Quoted 44 lines · Wolfram Sang
Quoted 8 lines · Neelesh Gupta
The patch exposes the available i2c busses on the PowerNV platform to the kernel and implements the bus driver to support i2c and smbus commands.
The driver uses the platform device infrastructure to probe the busses on the platform and registers them with the i2c driver framework.Signed-off-by Neelesh Gupta <[email protected]>Signed-off-by Benjamin Herrenschmidt <[email protected]>...
Quoted 25 lines
drivers/i2c/busses/Kconfig +11
-0@@ -1044,4 +1044,15 @@ config SCx200_ACB This support is also available as a module. If so, the module will be called scx200_acb. +config I2C_OPAL + tristate "IBM OPAL I2C driver" + depends on PPC_POWERNV + default y + help + This exposes the PowerNV platform i2c busses to the linux i2c layer, + the driver is based on the OPAL interfaces. + + This driver can also be built as a module. If so, the module will be + called as i2c-opal. + endmenudrivers/i2c/busses/Makefile +0
-0drivers/i2c/busses/Makefile obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o · line 105
Quoted 4 lines
drivers/i2c/busses/Makefile +1
-0@@ -102,5 +102,6 @@ obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o obj-$(CONFIG_SCx200_ACB) += scx200_acb.o +obj-$(CONFIG_I2C_OPAL) += i2c-opal.oPlease keep it proprly sorted.
Quoted 8 lines
drivers/i2c/busses/Makefile obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o
Quoted 5 lines
drivers/i2c/busses/Makefile +5
-0+ rc = of_property_read_u32(pdev->dev.of_node, "ibm,opal-id", &opal_id); + if (rc) { + dev_err(&pdev->dev, "Missing ibm,opal-id property !\n"); + return -EIO; + }You introduce new bindings which need to be documented in Docuemntation/devicetree/bindings/i2c.
Ugh ... thanks god the powerpc maintainer (me) didn't request binding document for everything that went into the device-tree on those platforms or we would still be writing them....
Quoted 3 lines
They should be posted as a seperate patch with [email protected] CCed, so they can comment on it. This is required these days and especially important..
Suuure, let's create more process and committees, and make sure nothing gets done in any reasonable amount of time. Have we gone completely insane ?
This is completely useless as an exercise. It's not like I'm going to change the firmware interfaces anymore anyway so the "comments" are going to be at best bike shed painting.
I'm getting close to just put that driver in powerpc.git ...
The point of publicly discussing bindings in the ARM world was in part because we attacked a community with no understanding of the DT, but in LARGE part because we had to define them for components and SoC that would potentially be re-used in different platforms etc..
Here we are talking about a representation specific to a given FW interface on PowerPC only which isn't going to be used by any other platform that PowerNV (since the OPAL fw is what makes PowerNV) by the people who invented the frigging flat device tree in the first place, so give me a break.
It's getting quite tempting to just throw that driver into powerpc.git
Quoted 7 lines
Quoted 7 lines
drivers/i2c/busses/Makefile +7
-0+ adapter = devm_kzalloc(&pdev->dev, sizeof(*adapter), GFP_KERNEL); + if (!adapter) + return -ENOMEM; + + adapter->algo = &i2c_opal_algo; + adapter->algo_data = (void *)(unsigned long)opal_id; + adapter->dev.parent = &pdev->dev;
Quoted 8 lines
drivers/i2c/busses/Makefile
Quoted 6 lines
drivers/i2c/busses/Makefile +6
-0+ adapter->dev.of_node = of_node_get(pdev->dev.of_node); + pname = of_get_property(pdev->dev.of_node, "ibm,port-name", NULL); + if (pname) + strlcpy(adapter->name, pname, sizeof(adapter->name)); + else + strlcpy(adapter->name, "opal", sizeof(adapter->name));... because I'd like to get an ack from them because of this binding.
And I don't give a flying crap about what random ARM SOC vendor thinks of my powerpc FW interface for a powerpc unique FW interface.
Quoted 7 lines
I
don't know if we can just say "this comes from firmware, so we must support it" (although you wrote the firmware IIUC) or if we have to judge if this is a HW description which should go into DT? I am open meanwhile that the adapter name does not need to be static anymore. However, I don't know much about server world and FW, so maybe they can assist.
I doubt it, all we're going to do is waste a few more monthes and make it more painful for us to get our support into distros in time with 0 benefit whatsoever.
Quoted 15 lines
An example binding in that document would also be very helpful.
drivers/i2c/busses/Makefile
Quoted 6 lines
drivers/i2c/busses/Makefile +6
-0+static struct platform_driver i2c_opal_driver = { + .probe = i2c_opal_probe, + .remove = i2c_opal_remove, + .driver = { + .name = "i2c-opal", + .owner = THIS_MODULE,owner not needed.
Thanks,
Wolfram
A message that is not in the archive (sent off-list or not yet received): 1416947765.4998.69.camel-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org
Quoted 4 lines
Suuure, let's create more process and committees, and make sure nothing gets done in any reasonable amount of time. Have we gone completely insane ?
I did not invent DT bindings. I did not invent that DT is/should be a hardware description. For me, it is a burden that I (as a subsystem maintainer for mainly drivers) have to prevent people from using DT for software configuration (some people use it as an 1:1 mapping for platform data even.) Since there are no guidelines (probably there can't be), I developed a set of rules out of experience and when those don't match I ask for help. Having a different set of rules for powerpc/arm/... (or server/embedded for that matter) will increase this burden a lot. People will come and say "But they did it as well..."
Quoted 1 line
It's getting quite tempting to just throw that driver into powerpc.git
Maybe this is the easiest. Just make sure that MAINTAINERS also point this driver to you or PowerNV maintainers. And no Ack from me, please. Then, I can always say "I dunno" if people start asking questions.
Quoted 10 lines
Quoted 7 lines
drivers/i2c/busses/i2c-opal.c line 241
Quoted 5 lines
drivers/i2c/busses/i2c-opal.c new file +5
-0+ pname =of_get_property (pdev->dev.of_node,"ibm,port-name" ,NULL ); +if (pname) +strlcpy (adapter->name, pname,sizeof (adapter->name)); +else +strlcpy (adapter->name,"opal" ,sizeof (adapter->name));... because I'd like to get an ack from them because of this binding.
And I don't give a flying crap about what random ARM SOC vendor thinks of my powerpc FW interface for a powerpc unique FW interface.
But you are not alone here. If you open the box for giving busses a configurable name, I can see other people (without FW) wanting this, too. So, this discussion will come anyhow IMO.