[PATCH 1/2] dt-bindings: fsi: Add optional chip-id to CFAMs

[PATCH 1/2] dt-bindings: fsi: Add optional chip-id to CFAMs

[Benjamin Herrenschmidt]

This represents a physical chip in the system and allows
a stable numbering scheme to be passed to udev for userspace
to recognize which chip is which.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 Documentation/devicetree/bindings/fsi/fsi.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/fsi/fsi.txt b/Documentation/devicetree/bindings/fsi/fsi.txt
index ab516c673a4b..afb4eccab131 100644
--- a/Documentation/devicetree/bindings/fsi/fsi.txt
+++ b/Documentation/devicetree/bindings/fsi/fsi.txt
@@ -83,6 +83,10 @@ addresses and sizes in the slave address space:
     #address-cells = <1>;
     #size-cells = <1>;
 
+Optionally, a slave can provide a global unique chip ID which is used to
+identify the physical location of the chip in a system specific way
+
+    chip-id = <0>;
 
 FSI engines (devices)
 ---------------------
@@ -125,6 +129,7 @@ device tree if no extra platform information is required.
             reg = <0 0>;
             #address-cells = <1>;
             #size-cells = <1>;
+	    chip-id = <0>;
 
             /* FSI engine at 0xc00, using a single page. In this example,
              * it's an I2C master controller, so subnodes describe the
-- 
2.17.1

[PATCH 2/2] fsi: Add support for device-tree provided chip IDs

[Benjamin Herrenschmidt]

They get retrieved from the device-tree and exposed
as an attribute in sysfs

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/fsi/fsi-core.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 1ae5be31b4bf..3fe36673cb21 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -80,6 +80,7 @@ struct fsi_slave {
 	struct fsi_master	*master;
 	int			id;
 	int			link;
+	int			chip_id;
 	uint32_t		size;	/* size of slave address space */
 	u8			t_send_delay;
 	u8			t_echo_delay;
@@ -723,6 +724,17 @@ static ssize_t slave_send_echo_store(struct device *dev,
 static DEVICE_ATTR(send_echo_delays, 0600,
 		   slave_send_echo_show, slave_send_echo_store);
 
+static ssize_t chip_id_show(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct fsi_slave *slave = to_fsi_slave(dev);
+
+	return sprintf(buf, "%d\n", slave->chip_id);
+}
+
+static DEVICE_ATTR_RO(chip_id);
+
 static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
 {
 	uint32_t chip_id;
@@ -786,6 +798,14 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
 	slave->t_send_delay = 16;
 	slave->t_echo_delay = 16;
 
+	/* Get chip ID if any */
+	slave->chip_id = -1;
+	if (slave->dev.of_node) {
+		uint32_t prop;
+		if (!of_property_read_u32(slave->dev.of_node, "chip-id", &prop))
+			slave->chip_id = prop;
+
+	}
 	rc = fsi_slave_set_smode(slave);
 	if (rc) {
 		dev_warn(&master->dev,
@@ -820,6 +840,10 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
 	if (rc)
 		dev_warn(&slave->dev, "failed to create delay attr: %d\n", rc);
 
+	rc = device_create_file(&slave->dev, &dev_attr_chip_id);
+	if (rc)
+		dev_warn(&slave->dev, "failed to create chip id: %d\n", rc);
+
 	rc = fsi_slave_scan(slave);
 	if (rc)
 		dev_dbg(&master->dev, "failed during slave scan with: %d\n",
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: fsi: Add optional chip-id to CFAMs

[Rob Herring]

On Fri, Jun 22, 2018 at 02:37:55PM +1000, Benjamin Herrenschmidt wrote:
> This represents a physical chip in the system and allows
> a stable numbering scheme to be passed to udev for userspace
> to recognize which chip is which.

I'm sure you're aware, stable numbers is generally not something the 
kernel guarantees...

In the cases where we do have them, we've used aliases.

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  Documentation/devicetree/bindings/fsi/fsi.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/fsi/fsi.txt b/Documentation/devicetree/bindings/fsi/fsi.txt
> index ab516c673a4b..afb4eccab131 100644
> --- a/Documentation/devicetree/bindings/fsi/fsi.txt
> +++ b/Documentation/devicetree/bindings/fsi/fsi.txt
> @@ -83,6 +83,10 @@ addresses and sizes in the slave address space:
>      #address-cells = <1>;
>      #size-cells = <1>;
>  
> +Optionally, a slave can provide a global unique chip ID which is used to
> +identify the physical location of the chip in a system specific way
> +
> +    chip-id = <0>;
>  
>  FSI engines (devices)
>  ---------------------
> @@ -125,6 +129,7 @@ device tree if no extra platform information is required.
>              reg = <0 0>;
>              #address-cells = <1>;
>              #size-cells = <1>;
> +	    chip-id = <0>;
>  
>              /* FSI engine at 0xc00, using a single page. In this example,
>               * it's an I2C master controller, so subnodes describe the
> -- 
> 2.17.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] dt-bindings: fsi: Add optional chip-id to CFAMs

[Benjamin Herrenschmidt]

On Tue, 2018-07-03 at 13:30 -0600, Rob Herring wrote:
> On Fri, Jun 22, 2018 at 02:37:55PM +1000, Benjamin Herrenschmidt wrote:
> > This represents a physical chip in the system and allows
> > a stable numbering scheme to be passed to udev for userspace
> > to recognize which chip is which.
> 
> I'm sure you're aware, stable numbers is generally not something the 
> kernel guarantees...

This has nothing to do with any kernel guarantee. Not sure what you are
mixing up here :-)

The IDs will get exposed via sysfs in order to allow udev rules to
create appropriate symlinks such as by-id or by-path as is traditional
(we haven't completely decided some of the udev side details yet)

> In the cases where we do have them, we've used aliases.

This is necessary, though Aliases may do the job too. This is the
device-tree that represents the "host" system that the BMC is managing.

We need to be able to identify using a stable numbering scheme the
processors on the FSI topology otherwise we would do "interesting"
things such as turn the fan for CPU 1 when CPU 0 gets hot :-)

(This is just a silly example, there are plenty of other reasons why we
need to understand the HW topology of a given system, including
debuggers using FSI as a backend etc...)

Traditionally POWER has used ibm,chip-id properties for the host side,
so I just did something similar here for the BMC side, but I can look
into using aliases if you prefer.

Note: I'm not sure what you have against DT provided names or IDs, this
has been a rather standard way of doing things even before we did the
FDT. For example that's what slot-names properties are for, or location
codes etc... Yes we invented that alias trick later on but it's not
necessarily the best approach (in fact I don't really like it to be
honest).

Cheers,
Ben.


> 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >  Documentation/devicetree/bindings/fsi/fsi.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/fsi/fsi.txt b/Documentation/devicetree/bindings/fsi/fsi.txt
> > index ab516c673a4b..afb4eccab131 100644
> > --- a/Documentation/devicetree/bindings/fsi/fsi.txt
> > +++ b/Documentation/devicetree/bindings/fsi/fsi.txt
> > @@ -83,6 +83,10 @@ addresses and sizes in the slave address space:
> >      #address-cells = <1>;
> >      #size-cells = <1>;
> >  
> > +Optionally, a slave can provide a global unique chip ID which is used to
> > +identify the physical location of the chip in a system specific way
> > +
> > +    chip-id = <0>;
> >  
> >  FSI engines (devices)
> >  ---------------------
> > @@ -125,6 +129,7 @@ device tree if no extra platform information is required.
> >              reg = <0 0>;
> >              #address-cells = <1>;
> >              #size-cells = <1>;
> > +	    chip-id = <0>;
> >  
> >              /* FSI engine at 0xc00, using a single page. In this example,
> >               * it's an I2C master controller, so subnodes describe the
> > -- 
> > 2.17.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] dt-bindings: fsi: Add optional chip-id to CFAMs

[Rob Herring]

On Tue, Jul 3, 2018 at 7:07 PM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Tue, 2018-07-03 at 13:30 -0600, Rob Herring wrote:
> > On Fri, Jun 22, 2018 at 02:37:55PM +1000, Benjamin Herrenschmidt wrote:
> > > This represents a physical chip in the system and allows
> > > a stable numbering scheme to be passed to udev for userspace
> > > to recognize which chip is which.
> >
> > I'm sure you're aware, stable numbers is generally not something the
> > kernel guarantees...
>
> This has nothing to do with any kernel guarantee. Not sure what you are
> mixing up here :-)

I was referring to /dev node names like sda/sdb/sdc or ttyS0/ttyS1.

> The IDs will get exposed via sysfs in order to allow udev rules to
> create appropriate symlinks such as by-id or by-path as is traditional
> (we haven't completely decided some of the udev side details yet)

Yeah, that's a bit different.

> > In the cases where we do have them, we've used aliases.
>
> This is necessary, though Aliases may do the job too. This is the
> device-tree that represents the "host" system that the BMC is managing.
>
> We need to be able to identify using a stable numbering scheme the
> processors on the FSI topology otherwise we would do "interesting"
> things such as turn the fan for CPU 1 when CPU 0 gets hot :-)
>
> (This is just a silly example, there are plenty of other reasons why we
> need to understand the HW topology of a given system, including
> debuggers using FSI as a backend etc...)
>
> Traditionally POWER has used ibm,chip-id properties for the host side,
> so I just did something similar here for the BMC side, but I can look
> into using aliases if you prefer.

That is specifically for CPUs though, right? Is the same true here? If
so, I guess this is fine. A set of indexes for any device on a bus
would be more concerning.

> Note: I'm not sure what you have against DT provided names or IDs, this
> has been a rather standard way of doing things even before we did the
> FDT. For example that's what slot-names properties are for, or location
> codes etc... Yes we invented that alias trick later on but it's not
> necessarily the best approach (in fact I don't really like it to be
> honest).

I'm no fan of aliases either, but just trying to keep some consistency
in how we deal with various problems. My main concern is folks trying
to create some made-up index numbering of devices. Often it's not
really needed as there are other ways to address or identify devices.
In many cases we end up using 'reg' if there's no other means of
addressing a device.

We've generally standardized around "label" for things like slots,
ports, connectors, etc. that need to be physically identified.
"slot-names" it seems hasn't gotten used for FDT. Since there aren't
DT's published for OF based systems nor any documentation, newbies
like me (that only have 8 years of DT experience) don't have any
insight into how things used to be done.

Rob

Re: [PATCH 1/2] dt-bindings: fsi: Add optional chip-id to CFAMs

[Benjamin Herrenschmidt]

On Thu, 2018-07-05 at 12:49 -0600, Rob Herring wrote:
> On Tue, Jul 3, 2018 at 7:07 PM Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > 
> > On Tue, 2018-07-03 at 13:30 -0600, Rob Herring wrote:
> > > On Fri, Jun 22, 2018 at 02:37:55PM +1000, Benjamin Herrenschmidt wrote:
> > > > This represents a physical chip in the system and allows
> > > > a stable numbering scheme to be passed to udev for userspace
> > > > to recognize which chip is which.
> > > 
> > > I'm sure you're aware, stable numbers is generally not something the
> > > kernel guarantees...
> > 
> > This has nothing to do with any kernel guarantee. Not sure what you are
> > mixing up here :-)
> 
> I was referring to /dev node names like sda/sdb/sdc or ttyS0/ttyS1.

Ah yes. I do have code to make those "somewhat stable" using the chip
ID by default (unless overriden) as a backward compat thing for small
systems but the long term goal is indeed to not rely on this.

> > The IDs will get exposed via sysfs in order to allow udev rules to
> > create appropriate symlinks such as by-id or by-path as is traditional
> > (we haven't completely decided some of the udev side details yet)
> 
> Yeah, that's a bit different.

Yup, that's the right way actually :-)

> > > In the cases where we do have them, we've used aliases.
> > 
> > This is necessary, though Aliases may do the job too. This is the
> > device-tree that represents the "host" system that the BMC is managing.
> > 
> > We need to be able to identify using a stable numbering scheme the
> > processors on the FSI topology otherwise we would do "interesting"
> > things such as turn the fan for CPU 1 when CPU 0 gets hot :-)
> > 
> > (This is just a silly example, there are plenty of other reasons why we
> > need to understand the HW topology of a given system, including
> > debuggers using FSI as a backend etc...)
> > 
> > Traditionally POWER has used ibm,chip-id properties for the host side,
> > so I just did something similar here for the BMC side, but I can look
> > into using aliases if you prefer.
> 
> That is specifically for CPUs though, right?

No, it's for host "chips".

IE, this is the device-tree of the BMC, consumed by the BMC. So we are
in the context of Linux running on the BMC ARM SoC.

That BMC is connected to the "Host" POWER9 chips via a service
interface called FSI (think as a kind of PECI or JTAG on steroids),
which it uses to crank them up or do various monitoring/management
tasks.

So while they are CPUs they are actually "Devices" in the context of
the consumer here.

Additionally, on some systems, there can be other type of chips
connected to FSI, such as memory buffer chips etc... all get a "chip
ID" assigned.

>  Is the same true here? If
> so, I guess this is fine. A set of indexes for any device on a bus
> would be more concerning.

There can be multiple FSI links connecting to the same chips, some are
accessed via cascaded FSI masters etc... so it's not about indices on
the bus. It's really about identifying a physical package on the
motherboard.

> > Note: I'm not sure what you have against DT provided names or IDs, this
> > has been a rather standard way of doing things even before we did the
> > FDT. For example that's what slot-names properties are for, or location
> > codes etc... Yes we invented that alias trick later on but it's not
> > necessarily the best approach (in fact I don't really like it to be
> > honest).
> 
> I'm no fan of aliases either, but just trying to keep some consistency
> in how we deal with various problems. My main concern is folks trying
> to create some made-up index numbering of devices. Often it's not
> really needed as there are other ways to address or identify devices.
> In many cases we end up using 'reg' if there's no other means of
> addressing a device.
> 
> We've generally standardized around "label" for things like slots,
> ports, connectors, etc. that need to be physically identified.

Yes, label would be an option too, probably a better one that aliases.

> "slot-names" it seems hasn't gotten used for FDT. Since there aren't
> DT's published for OF based systems nor any documentation, newbies
> like me (that only have 8 years of DT experience) don't have any
> insight into how things used to be done.

In a pretty much ad-hoc way :-) In this case, though, chip-id is a
simple solution and works well (and I have the code already written and
tested :-)

Cheers,
Ben.


> Rob

Re: [PATCH 1/2] dt-bindings: fsi: Add optional chip-id to CFAMs

[Benjamin Herrenschmidt]

On Fri, 2018-07-06 at 11:48 +1000, Benjamin Herrenschmidt wrote:
> > We've generally standardized around "label" for things like slots,
> > ports, connectors, etc. that need to be physically identified.
> 
> Yes, label would be an option too, probably a better one that aliases.
> 
> > "slot-names" it seems hasn't gotten used for FDT. Since there aren't
> > DT's published for OF based systems nor any documentation, newbies
> > like me (that only have 8 years of DT experience) don't have any
> > insight into how things used to be done.
> 
> In a pretty much ad-hoc way :-) In this case, though, chip-id is a
> simple solution and works well (and I have the code already written and
> tested :-)

I want to try to get that stuff upstream. Do you still object to the
chip-id's after our discussion ? The labels aren't that great really...

Cheers,
Ben.

Re: [PATCH 1/2] dt-bindings: fsi: Add optional chip-id to CFAMs

[Rob Herring]

On Wed, Jul 11, 2018 at 8:07 PM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Fri, 2018-07-06 at 11:48 +1000, Benjamin Herrenschmidt wrote:
> > > We've generally standardized around "label" for things like slots,
> > > ports, connectors, etc. that need to be physically identified.
> >
> > Yes, label would be an option too, probably a better one that aliases.
> >
> > > "slot-names" it seems hasn't gotten used for FDT. Since there aren't
> > > DT's published for OF based systems nor any documentation, newbies
> > > like me (that only have 8 years of DT experience) don't have any
> > > insight into how things used to be done.
> >
> > In a pretty much ad-hoc way :-) In this case, though, chip-id is a
> > simple solution and works well (and I have the code already written and
> > tested :-)
>
> I want to try to get that stuff upstream. Do you still object to the
> chip-id's after our discussion ? The labels aren't that great really...

No.

Acked-by: Rob Herring <robh@kernel.org>