| Message ID | 2269277.HqpbCE3B5H@dabox |
|---|---|
| State | RFC |
| Headers | show |
G'day Tim, On 29/04/2017 00:14, Tim Sander wrote: > Hi > > After sending this mail i just found out how i could reset the i2c-1 controller manually with > devmem 0xffd05014 32 0x2000 > devmem 0xffd05014 32 0 > > So i took a look into the device tree file socfpga.dtsi and found that the reset lines > where not defined (although available in the corresponding reset manager). Is there a > reason for this? Other components are connected. There's a few thing like that where the bootloader has been expected to setup the resets etc. > > However with the patch below my previously sent patch works! > > If there is interest in would cleanup the patch and send it in for mainlining. > I think the most unacceptable part would be this line: > + ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN | > My gpio drivers refuse to work as output as they have no open drain mode. > So i wonder how to get this solved in a clean manner. I thought the gpio system would emulate open drain by switching the pin between an input and output driven low in this case. How are you configuring the GPIO's in the FPGA? > > Best regards > Tim > --- > arch/arm/boot/dts/socfpga.dtsi | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi > index 2c43c4d85dee..5f28632bc88c 100644 > --- a/arch/arm/boot/dts/socfpga.dtsi > +++ b/arch/arm/boot/dts/socfpga.dtsi > @@ -643,6 +643,7 @@ > #size-cells = <0>; > compatible = "snps,designware-i2c"; > reg = <0xffc04000 0x1000>; > + resets = <&rst I2C0_RESET>; > clocks = <&l4_sp_clk>; > interrupts = <0 158 0x4>; > status = "disabled"; > @@ -653,6 +654,7 @@ > #size-cells = <0>; > compatible = "snps,designware-i2c"; > reg = <0xffc05000 0x1000>; > + resets = <&rst I2C1_RESET>; > clocks = <&l4_sp_clk>; > interrupts = <0 159 0x4>; > status = "disabled"; > @@ -663,6 +665,7 @@ > #size-cells = <0>; > compatible = "snps,designware-i2c"; > reg = <0xffc06000 0x1000>; > + resets = <&rst I2C2_RESET>; > clocks = <&l4_sp_clk>; > interrupts = <0 160 0x4>; > status = "disabled"; > @@ -673,6 +676,7 @@ > #size-cells = <0>; > compatible = "snps,designware-i2c"; > reg = <0xffc07000 0x1000>; > + resets = <&rst I2C3_RESET>; > clocks = <&l4_sp_clk>; > interrupts = <0 161 0x4>; > status = "disabled"; >
Good Day Phil Am Montag, 1. Mai 2017, 09:57:35 CEST schrieb Phil Reid: > > So i took a look into the device tree file socfpga.dtsi and found that the > > reset lines where not defined (although available in the corresponding > > reset manager). Is there a reason for this? Other components are > > connected. > > There's a few thing like that where the bootloader has been expected to > setup the resets etc. Yes, but if the resets are not connected in the device tree, the linux drivers are not going to use them? > > However with the patch below my previously sent patch works! > > > > If there is interest in would cleanup the patch and send it in for > > mainlining. I think the most unacceptable part would be this line: > > + ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN | > > My gpio drivers refuse to work as output as they have no open drain mode. > > So i wonder how to get this solved in a clean manner. > > I thought the gpio system would emulate open drain by switching the pin > between an input and output driven low in this case. How are you > configuring the GPIO's in the FPGA? I don't switch to GPIO mode. As the I2C logic is only pulling active low, i only do a wired and with the gpio (implemented in the fpga) port output on the output enable line for the SCL output. SDA is only an additional input for the second in fpga gpio port. A picture should make it a clearer: gpio scl--\ i2c scl --&---i2c mode output pin (configured as fpga loan) In my case the original i2c pins where occupied by some other logic conflicting so the i2c pins had to be shifted to some other pins using fpga logic. So it was just a matter of adding a two port gpio port. > Given a couple of days I can test this on some flack i2c hardware I have > with a Cyclone-V SOC. I'm interested in the functionality as well. Sounds good. If you need some further input how i have configured the fpga drop me a line. > For i2c that are connected to the dedicated HPS pins it should be possible > to reconfigure the pin mux controller (see system manager) in the HPS to > avoid the need to go thru the fpga to get direct control. The docs say this > is "unsupport" but I've done some test and it does seem to work. As far as i know the internal jtag chain is only used in the bootloader and there is no linux driver? But it shouldn't be a too big problem to port it to linux. What i am unsure about is the fact that the internal jtag chain which controls the pinmuxing might wreak havoc on other pin states if you reconfigure it? > I'm guess > the no support is in a similar vain to the emac ptp FPGA interface couldn't > be used when the HPS pin where used. But that got changed when the user's > proved otherwise. There's just no pin ctrl driver yet to manage it. I am interested in this ptp solution too. Is there anything on the way to mainline? Best regards Tim -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
G'day Tim, On 1/05/2017 21:31, Tim Sander wrote: > Good Day Phil > > Am Montag, 1. Mai 2017, 09:57:35 CEST schrieb Phil Reid: >>> So i took a look into the device tree file socfpga.dtsi and found that the >>> reset lines where not defined (although available in the corresponding >>> reset manager). Is there a reason for this? Other components are >>> connected. >> >> There's a few thing like that where the bootloader has been expected to >> setup the resets etc. > Yes, but if the resets are not connected in the device tree, the linux drivers > are not going to use them? Yes, so they should be added. I don't think we should assume the bootloader sets things up. But that doesn't seem to have been the assumption with the Alter SOC's. > >>> However with the patch below my previously sent patch works! >>> >>> If there is interest in would cleanup the patch and send it in for >>> mainlining. I think the most unacceptable part would be this line: >>> + ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN | >>> My gpio drivers refuse to work as output as they have no open drain mode. >>> So i wonder how to get this solved in a clean manner. >> >> I thought the gpio system would emulate open drain by switching the pin >> between an input and output driven low in this case. How are you >> configuring the GPIO's in the FPGA? > I don't switch to GPIO mode. As the I2C logic is only pulling active low, i only do > a wired and with the gpio (implemented in the fpga) port output on the output > enable line for the SCL output. SDA is only an additional input for the second in > fpga gpio port. > > A picture should make it a clearer: > > gpio scl--\ > i2c scl --&---i2c mode output pin (configured as fpga loan) > > In my case the original i2c pins where occupied by some other logic conflicting > so the i2c pins had to be shifted to some other pins using fpga logic. So it was > just a matter of adding a two port gpio port. I think I understand. What soft core gpio controller are you using? > >> Given a couple of days I can test this on some flack i2c hardware I have >> with a Cyclone-V SOC. I'm interested in the functionality as well. > Sounds good. If you need some further input how i have configured the fpga > drop me a line. > >> For i2c that are connected to the dedicated HPS pins it should be possible >> to reconfigure the pin mux controller (see system manager) in the HPS to >> avoid the need to go thru the fpga to get direct control. The docs say this >> is "unsupport" but I've done some test and it does seem to work. > As far as i know the internal jtag chain is only used in the bootloader and there > is no linux driver? But it shouldn't be a too big problem to port it to linux. > > What i am unsure about is the fact that the internal jtag chain which controls the > pinmuxing might wreak havoc on other pin states if you reconfigure it? Have a look at the Cyclone V handbook "pin mux control Group REgister Descriptions" From what I can see the chain is used to configure IO standards and drive strength. But not the actual muxes > >> I'm guess >> the no support is in a similar vain to the emac ptp FPGA interface couldn't >> be used when the HPS pin where used. But that got changed when the user's >> proved otherwise. There's just no pin ctrl driver yet to manage it. > I am interested in this ptp solution too. Is there anything on the way to mainline? > This was working the last time I tried it. I submitted a couple of minor patches for it a while ago. My hardware has a DSA switch attached to the ethernet port and so far I haven't figured out how to enable ptp when using the virtual lan ports on the DSA. But it worked fine when directly connected to a phy.
Good Day Phil Am Mittwoch, 3. Mai 2017, 09:30:50 CEST schrieb Phil Reid: > G'day Tim, > > On 1/05/2017 21:31, Tim Sander wrote: > > Good Day Phil > > > > Am Montag, 1. Mai 2017, 09:57:35 CEST schrieb Phil Reid: > >>> So i took a look into the device tree file socfpga.dtsi and found that > >>> the > >>> reset lines where not defined (although available in the corresponding > >>> reset manager). Is there a reason for this? Other components are > >>> connected. > >> > >> There's a few thing like that where the bootloader has been expected to > >> setup the resets etc. > > > > Yes, but if the resets are not connected in the device tree, the linux > > drivers are not going to use them? > > Yes, so they should be added. I don't think we should assume the bootloader > sets things up. But that doesn't seem to have been the assumption with the > Alter SOC's. I will prepare a patch for this. > >>> However with the patch below my previously sent patch works! > >>> > >>> If there is interest in would cleanup the patch and send it in for > >>> mainlining. I think the most unacceptable part would be this line: > >>> + ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN | > >>> My gpio drivers refuse to work as output as they have no open drain > >>> mode. > >>> So i wonder how to get this solved in a clean manner. > >> > >> I thought the gpio system would emulate open drain by switching the pin > >> between an input and output driven low in this case. How are you > >> configuring the GPIO's in the FPGA? > > > > I don't switch to GPIO mode. As the I2C logic is only pulling active low, > > i only do a wired and with the gpio (implemented in the fpga) port output > > on the output enable line for the SCL output. SDA is only an additional > > input for the second in fpga gpio port. > > > > A picture should make it a clearer: > > > > gpio scl--\ > > i2c scl --&---i2c mode output pin (configured as fpga loan) > > > > In my case the original i2c pins where occupied by some other logic > > conflicting so the i2c pins had to be shifted to some other pins using > > fpga logic. So it was just a matter of adding a two port gpio port. > > I think I understand. What soft core gpio controller are you using? I am using the standard altera fpga gpios. > >> Given a couple of days I can test this on some flack i2c hardware I have > >> with a Cyclone-V SOC. I'm interested in the functionality as well. > > > > Sounds good. If you need some further input how i have configured the fpga > > drop me a line. > > > >> For i2c that are connected to the dedicated HPS pins it should be > >> possible > >> to reconfigure the pin mux controller (see system manager) in the HPS to > >> avoid the need to go thru the fpga to get direct control. The docs say > >> this > >> is "unsupport" but I've done some test and it does seem to work. > > > > As far as i know the internal jtag chain is only used in the bootloader > > and there is no linux driver? But it shouldn't be a too big problem to > > port it to linux. > > > > What i am unsure about is the fact that the internal jtag chain which > > controls the pinmuxing might wreak havoc on other pin states if you > > reconfigure it? > Have a look at the Cyclone V handbook "pin mux control Group REgister > Descriptions" From what I can see the chain is used to configure IO > standards and drive strength. But not the actual muxes Mh, there is not much to see in Volume 3. Just one paragraph and then a very encouraging closing line: "Do not modify the pin multiplexing selection registers after I/O configuration." I find the following lines in my favorite bootloader a little more enlightening: The following function: https://git.pengutronix.de/cgit/barebox/tree/arch/arm/mach-socfpga/system-manager.c get feed with data from e.g.: https://git.pengutronix.de/cgit/barebox/tree/arch/arm/boards/terasic-de0-nano-soc/pinmux_config.c which doesn't look like beeing very memory mapped? > >> I'm guess > >> the no support is in a similar vain to the emac ptp FPGA interface > >> couldn't > >> be used when the HPS pin where used. But that got changed when the user's > >> proved otherwise. There's just no pin ctrl driver yet to manage it. > > > > I am interested in this ptp solution too. Is there anything on the way to > > mainline? > This was working the last time I tried it. I submitted a couple of minor > patches for it a while ago. My hardware has a DSA switch attached to the > ethernet port and so far I haven't figured out how to enable ptp when using > the virtual lan ports on the DSA. But it worked fine when directly > connected to a phy. Thanks, will take a look. Best regards Tim -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
G'day Tim, Sorry for the delay in looking at this. My device is currently running a 4.9 kernel and I had to backport the cahnges to the driver to get things running with your patch. In general the code works and the bus recovers now. I've been using the i2c gpio bus driver because the dw wouldn't do recovery. But this looks alot nicer. On 4/05/2017 03:04, Tim Sander wrote: >>>>> So i took a look into the device tree file socfpga.dtsi and found that >>>>> the >>>>> reset lines where not defined (although available in the corresponding >>>>> reset manager). Is there a reason for this? Other components are >>>>> connected. >>>> >>>> There's a few thing like that where the bootloader has been expected to >>>> setup the resets etc. >>> >>> Yes, but if the resets are not connected in the device tree, the linux >>> drivers are not going to use them? >> >> Yes, so they should be added. I don't think we should assume the bootloader >> sets things up. But that doesn't seem to have been the assumption with the >> Alter SOC's. > I will prepare a patch for this. > >>>>> However with the patch below my previously sent patch works! >>>>> >>>>> If there is interest in would cleanup the patch and send it in for >>>>> mainlining. I think the most unacceptable part would be this line: >>>>> + ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN | >>>>> My gpio drivers refuse to work as output as they have no open drain >>>>> mode. >>>>> So i wonder how to get this solved in a clean manner. >>>> >>>> I thought the gpio system would emulate open drain by switching the pin >>>> between an input and output driven low in this case. How are you >>>> configuring the GPIO's in the FPGA? >>> >>> I don't switch to GPIO mode. As the I2C logic is only pulling active low, >>> i only do a wired and with the gpio (implemented in the fpga) port output >>> on the output enable line for the SCL output. SDA is only an additional >>> input for the second in fpga gpio port. >>> >>> A picture should make it a clearer: >>> >>> gpio scl--\ >>> i2c scl --&---i2c mode output pin (configured as fpga loan) >>> >>> In my case the original i2c pins where occupied by some other logic >>> conflicting so the i2c pins had to be shifted to some other pins using >>> fpga logic. So it was just a matter of adding a two port gpio port. >> >> I think I understand. What soft core gpio controller are you using? > I am using the standard altera fpga gpios. > I dug into things a little and found the following init function works without requiring modification to the core. The GPIO config (open drain or not etc) can be put in the device tree. static int i2c_dw_get_scl(struct i2c_adapter *adap) { struct platform_device *pdev = to_platform_device(&adap->dev); struct dw_i2c_dev *dev = platform_get_drvdata(pdev); return gpiod_get_value_cansleep(dev->gpio_scl); } static void i2c_dw_set_scl(struct i2c_adapter *adap, int val) { struct platform_device *pdev = to_platform_device(&adap->dev); struct dw_i2c_dev *dev = platform_get_drvdata(pdev); gpiod_set_value_cansleep(dev->gpio_scl, val); } static int i2c_dw_get_sda(struct i2c_adapter *adap) { struct platform_device *pdev = to_platform_device(&adap->dev); struct dw_i2c_dev *dev = platform_get_drvdata(pdev); return gpiod_get_value_cansleep(dev->gpio_sda); } static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev, struct i2c_adapter *adap) { struct i2c_bus_recovery_info *rinfo = &dev->rinfo; dev->gpio_scl = devm_gpiod_get_optional(dev->dev, "scl", GPIOD_OUT_HIGH); if (IS_ERR_OR_NULL(dev->gpio_scl)) return PTR_ERR(dev->gpio_scl); dev->gpio_sda = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_IN); if (IS_ERR_OR_NULL(dev->gpio_sda)) return PTR_ERR(dev->gpio_sda); rinfo->scl_gpio = desc_to_gpio(dev->gpio_scl); rinfo->sda_gpio = desc_to_gpio(dev->gpio_sda); rinfo->set_scl = i2c_dw_set_scl; rinfo->get_scl = i2c_dw_get_scl; rinfo->get_sda = i2c_dw_get_sda; rinfo->recover_bus = i2c_generic_scl_recovery; rinfo->prepare_recovery = i2c_dw_prepare_recovery; rinfo->unprepare_recovery = i2c_dw_unprepare_recovery; adap->bus_recovery_info = rinfo; dev_info(dev->dev, "adapter: %s running with gpio recovery mode! scl:%i sda:%i \n", adap->name, rinfo->scl_gpio, rinfo->sda_gpio); return 0; }; A small modification to the i2c-core could be done in i2c_init_recovery to allow: rinfo->recover_bus == i2c_generic_scl_recovery when scl_gpio is also set and fallback to using the core set / get scl / sda calls Which would remove the need for the above i2c_dw_* functions. I wouldn't think that would cause any problems.
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi index 2c43c4d85dee..5f28632bc88c 100644 --- a/arch/arm/boot/dts/socfpga.dtsi +++ b/arch/arm/boot/dts/socfpga.dtsi @@ -643,6 +643,7 @@ #size-cells = <0>; compatible = "snps,designware-i2c"; reg = <0xffc04000 0x1000>; + resets = <&rst I2C0_RESET>; clocks = <&l4_sp_clk>; interrupts = <0 158 0x4>; status = "disabled"; @@ -653,6 +654,7 @@ #size-cells = <0>; compatible = "snps,designware-i2c"; reg = <0xffc05000 0x1000>; + resets = <&rst I2C1_RESET>; clocks = <&l4_sp_clk>; interrupts = <0 159 0x4>; status = "disabled"; @@ -663,6 +665,7 @@ #size-cells = <0>; compatible = "snps,designware-i2c"; reg = <0xffc06000 0x1000>; + resets = <&rst I2C2_RESET>; clocks = <&l4_sp_clk>; interrupts = <0 160 0x4>; status = "disabled"; @@ -673,6 +676,7 @@ #size-cells = <0>; compatible = "snps,designware-i2c"; reg = <0xffc07000 0x1000>; + resets = <&rst I2C3_RESET>; clocks = <&l4_sp_clk>; interrupts = <0 161 0x4>; status = "disabled";