drivers/fsi: Break and set pin states on GPIO master remove

PATCH In review Christopher Bostic 5 participants 7 messages Jun 6, 2017 – Jun 21, 2017 FSI

lore

Share & tools

lore

Christopher Bostic

author

2017-06-06 20:02 UTC #0 lore raw

PATCH 1/1 drivers/fsi: Break and set pin states on GPIO master remove

Update GPIO remove behavior to match dev-4.7.

On gpio remove: Send a break
clock, translator, mux = 0
data, enable = 1

Signed-off-by Christopher Bostic <[email protected]>

---
 drivers/fsi/fsi-master-gpio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/fsi/fsi-master-gpio.c +7 -1

index 2a6a812..47eaf61 100644
@@ -585,17 +585,23 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
 	return 0;
 }
 
-
 static int fsi_master_gpio_remove(struct platform_device *pdev)
 {
 	struct fsi_master_gpio *master = platform_get_drvdata(pdev);
 
+	fsi_master_gpio_break(&master->master, 0);
+
+	gpiod_set_value(master->gpio_clk, 0);
 	devm_gpiod_put(&pdev->dev, master->gpio_clk);
+	gpiod_set_value(master->gpio_data, 1);
 	devm_gpiod_put(&pdev->dev, master->gpio_data);
+	gpiod_set_value(master->gpio_trans, 0);
 	if (master->gpio_trans)
 		devm_gpiod_put(&pdev->dev, master->gpio_trans);
+	gpiod_set_value(master->gpio_enable, 1);
 	if (master->gpio_enable)
 		devm_gpiod_put(&pdev->dev, master->gpio_enable);
+	gpiod_set_value(master->gpio_mux, 0);
 	if (master->gpio_mux)
 		devm_gpiod_put(&pdev->dev, master->gpio_mux);
 	fsi_master_unregister(&master->master);

Signature · 1 line

1.8.2.2

Jeremy Kerr

2017-06-07 03:22 UTC #1 lore raw

Hi Chris,

Quoted 5 lines

Update GPIO remove behavior to match dev-4.7.
On gpio remove: Send a break
clock, translator, mux = 0
data, enable = 1

While this change itself seems okay (leaving GPIOs in a defined state), I'm concerned about *why* we need this.

If there is other code that need GPIOs in a certain state, then that code should be doing the required init itself.

Could you add a comment on why this is required?

Cheers,

Jeremy

Christopher Bostic

author

2017-06-07 13:55 UTC #2 lore raw

Quoted 4 lines · Jeremy Kerr

Hi Chris,
Quoted 2 lines
Update GPIO remove behavior to match dev-4.7.

Quoted 8 lines

Quoted 3 lines
On gpio remove: Send a break
clock, translator, mux = 0
data, enable = 1
While this change itself seems okay (leaving GPIOs in a defined state), I'm concerned about *why* we need this.
If there is other code that need GPIOs in a certain state, then that code should be doing the required init itself.

Hi Jeremy,

The pins are set here mainly for the benefit of the Cronus debug tools. Its common practice in test and bringup labs to switch back and forth between BMC and FSP-2 with Cronus to access the host processor CFAMs over FSI. Without the remove pin state changes Cronus cannot access.

Cronus is a user space layer and presently doesn't modify gpio directly in this situation. It could do this work I suppose but all designers involved will need to come to an agreement as to which is the proper approach.

Thanks,
Chris

Quoted 7 lines

Could you add a comment on why this is required?
Cheers,
Jeremy

Alistair Popple

2017-06-08 07:25 UTC #3 lore raw

Quoted 12 lines · Christopher Bostic

Quoted 10 lines · Jeremy Kerr
Hi Chris,
Quoted 5 lines
Update GPIO remove behavior to match dev-4.7.
On gpio remove: Send a break
clock, translator, mux = 0
data, enable = 1
While this change itself seems okay (leaving GPIOs in a defined state), I'm concerned about *why* we need this.

Quoted 8 lines

Quoted 2 lines
If there is other code that need GPIOs in a certain state, then that code should be doing the required init itself.
Hi Jeremy,
The pins are set here mainly for the benefit of the Cronus debug
tools. Its common practice in test and bringup labs to switch back and
forth between BMC and FSP-2 with Cronus to access the host processor CFAMs over FSI. Without the remove pin state changes Cronus cannot access.

As far as I know you have to run "systemctl start fsi-disable.service" to switch over to Cronus mode. I think Jeremy's point is that it should be that service which explicitly sets up the hardware in a suitable mode rather than relying on side-effects from removing the kernel driver.

- Alistair

Quoted 16 lines

Cronus is a user space layer and presently doesn't modify gpio directly
in this situation. It could do this work I suppose but all designers
involved will need to come to an agreement as to which is the proper approach.
Thanks,
Chris
Quoted 7 lines
Could you add a comment on why this is required?
Cheers,
Jeremy

Jeremy Kerr

2017-06-08 10:00 UTC #4 lore raw

Hi Alistair & Chris,

Quoted 10 lines

Quoted 4 lines
The pins are set here mainly for the benefit of the Cronus debug
tools. Its common practice in test and bringup labs to switch back and
forth between BMC and FSP-2 with Cronus to access the host processor CFAMs over FSI. Without the remove pin state changes Cronus cannot access.
As far as I know you have to run "systemctl start fsi-disable.service" to switch over to Cronus mode. I think Jeremy's point is that it should be that service which explicitly sets up the hardware in a suitable mode rather than relying on side-effects from removing the kernel driver.

Yep, spot on.

Also, those side-effects might not be how the GPIOs need to configured for the next usage of the FSI bus (which could be Cronus, but could also be any other tool). So, let those tools handle the init correctly, rather than the kernel making a guess at it.

Cheers,

Jeremy

Andrew Geissler

2017-06-12 02:09 UTC #5 lore raw

I think we had this same discussion last time Chris put this on the
mailing list for 4.7 :) I think we can do better here but we also
(as usual) have a big time crunch. Can we agree to put this in for now since our main goal is to just get 4.10 merged and of equal (or better) function to 4.7? Chris can create an issue to come up with a final solution? Without this change, cronus via the FSP debug does not work, which is a non-starter for picking this release up by IBM (and a lot of partners).

I know, in theory, this is easy to fix, but it will require new testing, potentially new documentation for our users, and is just adding new function when we're mostly looking to just get a stable base going.

Andrew

Quoted 24 lines · Jeremy Kerr

Hi Alistair & Chris,
Quoted 10 lines
Quoted 4 lines
The pins are set here mainly for the benefit of the Cronus debug
tools. Its common practice in test and bringup labs to switch back and
forth between BMC and FSP-2 with Cronus to access the host processor CFAMs over FSI. Without the remove pin state changes Cronus cannot access.
As far as I know you have to run "systemctl start fsi-disable.service" to switch over to Cronus mode. I think Jeremy's point is that it should be that service which explicitly sets up the hardware in a suitable mode rather than relying on side-effects from removing the kernel driver.
Yep, spot on.
Also, those side-effects might not be how the GPIOs need to configured for the next usage of the FSI bus (which could be Cronus, but could also be any other tool). So, let those tools handle the init correctly, rather than the kernel making a guess at it.
Cheers,
Jeremy

Milton Miller II

2017-06-21 19:32 UTC #6 lore raw

Quoted 33 lines · Andrew Geissler

I think we had this same discussion last time Chris put this on the
mailing list for 4.7 :) I think we can do better here but we also
(as usual) have a big time crunch. Can we agree to put this in for now since our main goal is to just get 4.10 merged and of equal (or better) function to 4.7? Chris can create an issue to come up with a final solution? Without this change, cronus via the FSP debug does not work, which is a non-starter for picking this release up by IBM (and a lot of partners).
I know, in theory, this is easy to fix, but it will require new testing, potentially new documentation for our users, and is just adding new function when we're mostly looking to just get a stable base going.
Andrew
Quoted 4 lines · Jeremy Kerr
Hi Alistair & Chris,
Quoted 2 lines
Quoted 2 lines
The pins are set here mainly for the benefit of the Cronus debug
tools. Its common practice in test and bringup labs to switch
back and
Quoted 1 line
Quoted 1 line
Quoted 1 line
forth between BMC and FSP-2 with Cronus to access the host
processor
Quoted 1 line
Quoted 1 line
Quoted 1 line
CFAMs over FSI. Without the remove pin state changes Cronus
cannot access.
Quoted 2 lines
Quoted 2 lines
As far as I know you have to run "systemctl start
fsi-disable.service"
Quoted 3 lines
Quoted 3 lines
to switch over to Cronus mode. I think Jeremy's point is that it should be that service which explicitly sets up the hardware in a suitable mode rather than relying on side-effects from removing
the

Quoted 11 lines

Quoted 5 lines
Quoted 1 line
kernel driver.
Yep, spot on.
Also, those side-effects might not be how the GPIOs need to
configured
Quoted 1 line
for the next usage of the FSI bus (which could be Cronus, but could
also
Quoted 3 lines
be any other tool). So, let those tools handle the init correctly, rather than the kernel making a guess at it.

Actually I think there are well defined "Idle" states and I would expect those to be asserted.

I would expect the gpios to be set such that changing the mux does not glitch the signals to the chip.

Is this the state that is being set in the patch?

milton

[catching up after vacation]