| Message ID | 1453260414-24817-1-git-send-email-inindev@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 20/01/2016 04:26, John Clark wrote: > GPIO Changes > ~~~~~~~~~~~~ > The HLK-RM04 uses i2c and the top half of the uartf port for gpio. HiLink has labeled the RT5350 gpio1 pin as gpio0 on the device pinout, so it seems to make sense to export it as "hlk-rm04:gpio0" to avoid confusing it with the microcontroller's designation. > > gpio gpio0 gpio0 (pin 10, reset) > > i2c i2c_sd gpio1 (pin 8, hlk-rm04:gpio0) > i2c i2c_sclk gpio2 (pin 9, hlk-rm04:gpio1) > > uartf dtr_n gpio11 (no pinout) > uartf dcd_n gpio12 (no pinout) > uartf dsr_n gpio13 (no pinout) > uartf rin gpio14 (pin 25, wps) > > reference: > http://www.hlktech.net/product_detail.php?ProId=39 > http://cdn.sparkfun.com/datasheets/Wireless/WiFi/RT5350.pdf > > Also note that there is no gpio to control the power led, so remove the incorrect mapping. > > Memory Model Number (trivial) > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > The dts file references memory chip s25fl064k, which seems to have ever been used for the HiLink HLK-RM04. While it only causes a nag message in the kernel log, why not change it to the w25q32 which is commonly found on these devices. > > Signed-off-by: John Clark <inindev@gmail.com> > --- > target/linux/ramips/dts/HLKRM04.dts | 46 ++++++++++++++++++++++++++----------- > 1 file changed, 32 insertions(+), 14 deletions(-) > > diff --git a/target/linux/ramips/dts/HLKRM04.dts b/target/linux/ramips/dts/HLKRM04.dts > index f90a9ac..0a0705a 100644 > --- a/target/linux/ramips/dts/HLKRM04.dts > +++ b/target/linux/ramips/dts/HLKRM04.dts > @@ -18,17 +18,17 @@ > palmbus@10000000 { > uart@500 { > status = "okay"; > + pinctrl-0; are you sure this bit is correct ? > }; > > spi@b00 { > status = "okay"; > - > m25p80@0 { > #address-cells = <1>; > #size-cells = <1>; > compatible = "jedec,spi-nor"; > reg = <0 0>; > - linux,modalias = "m25p80", "s25fl064k"; > + linux,modalias = "m25p80", "w25q32"; > spi-max-frequency = <10000000>; > > partition@0 { > @@ -60,9 +60,13 @@ > pinctrl { > state_default: pinctrl0 { > gpio { > - ralink,group = "jtag"; > + ralink,group = "jtag", "i2c"; > ralink,function = "gpio"; > }; > + uartf_gpio { > + ralink,group = "uartf"; > + ralink,function = "gpio uartf"; > + }; > }; > }; > > @@ -82,25 +86,39 @@ > status = "okay"; > }; > > + gpio-export { > + compatible = "gpio-export"; > + #size-cells = <0>; > + > + /* I2C */ > + gpio1 { > + /* I2C_I2C_SD */ > + gpio-export,name = "hlk-rm04:gpio0"; > + gpio-export,direction_may_change = <1>; > + gpios = <&gpio0 1 0>; > + }; > + gpio2 { > + /* I2C_I2C_SCLK */ > + gpio-export,name = "hlk-rm04:gpio1"; > + gpio-export,direction_may_change = <1>; > + gpios = <&gpio0 2 0>; > + }; > + }; > + > gpio-keys-polled { > compatible = "gpio-keys-polled"; > #address-cells = <1>; > #size-cells = <0>; > poll-interval = <20>; > - > - wps { > + reset { > label = "reset"; > - gpios = <&gpio0 14 1>; > + gpios = <&gpio0 0 1>; > linux,code = <0x198>; > }; > - }; > - > - gpio-leds { > - compatible = "gpio-leds"; > - > - power { > - label = "hlk-rm04:red:power"; > - gpios = <&gpio0 0 1>; > + wps { > + label = "wps"; > + gpios = <&gpio0 14 1>; > + linux,code = <0x211>; > }; > }; > }; >
> + pinctrl-0; >> are you sure this bit is correct ? That is there to override the setting from the parent dtsi file. One could argue that the parent dtsi file should not be setting up the function of the uart -- this should in fact be done at the dts file level. We can see below in the file for the mt7620a.dtsi that this is not set, so there is precedence for taking it out of the dtsi. I hesitated be edit the parent dtsi file as I thought this would pose a backward compatibility issue in CC, but perhaps this is the more appropriate PR for the main branch. I am happy to resubmit the PR for main to do it like the dtsi for the mt7620a if you like. https://dev.openwrt.org/browser/trunk/target/linux/ramips/dts/rt5350.dtsi 85 uart@500 { 86 compatible = "ralink,rt5350-uart", "ralink,rt2880-uart", "ns16550a"; 87 reg = <0x500 0x100>; 88 89 resets = <&rstctrl 12>; 90 reset-names = "uart"; 91 92 interrupt-parent = <&intc>; 93 interrupts = <5>; 94 95 reg-shift = <2>; 96 97 pinctrl-names = "default"; 98 pinctrl-0 = <&uartf_pins>; 99 100 status = "disabled"; 101 }; https://dev.openwrt.org/browser/trunk/target/linux/ramips/dts/mt7620a.dtsi 85 uart@500 { 86 compatible = "ralink,mt7620a-uart", "ralink,rt2880-uart", "ns16550a"; 87 reg = <0x500 0x100>; 88 89 resets = <&rstctrl 12>; 90 reset-names = "uart"; 91 92 interrupt-parent = <&intc>; 93 interrupts = <5>; 94 95 reg-shift = <2>; 96 97 status = "disabled"; 98 }; Without clearing the pinctrl-0 in the dts file as I have in my PR, the parent dtsi file is asking for the uartf pins to be configured for function “uartf” on line 255 of the parent dtsi: ralink,function = "uartf"; This dtsi function competes with dts function in the pull request: ralink,function = "gpio uartf"; The two conflicting settings will cause a kernel error at boot time. If I take the pinctrl-0; override out, we can see this conflict: [ 0.197413] pinctrl core: add 1 pinctrl maps [ 0.197487] rt2880-pinmux pinctrl: found group selector 2 for uartf [ 0.197526] rt2880-pinmux pinctrl: request pin 7 (io7) for 10000500.uart [ 0.197558] rt2880-pinmux pinctrl: pin io7 already requested by pinctrl; cannot claim for 10000500.uart [ 0.197895] rt2880-pinmux pinctrl: pin-7 (10000500.uart) status -22 [ 0.198130] rt2880-pinmux pinctrl: could not request pin 7 (io7) from group uartf on device rt2880-pinmux [ 0.198471] of_serial 10000500.uart: Error applying setting, reverse things back [ 0.199394] 10000500.uart: ttyS0 at MMIO 0x10000500 (irq = 13, base_baud = 2500000) is a Palmchip BK-3103
there are 4-5 patches all intermingled into 1 patch. please split it up. one change per patch John On 21/01/2016 13:52, John Clark wrote: >> + pinctrl-0; >>> are you sure this bit is correct ? > > That is there to override the setting from the parent dtsi file. One > could argue that the parent dtsi file should not be setting up the > function of the uart -- this should in fact be done at the dts file > level. We can see below in the file for the mt7620a.dtsi that this is > not set, so there is precedence for taking it out of the dtsi. I > hesitated be edit the parent dtsi file as I thought this would pose a > backward compatibility issue in CC, but perhaps this is the more > appropriate PR for the main branch. I am happy to resubmit the PR for > main to do it like the dtsi for the mt7620a if you like. > > https://dev.openwrt.org/browser/trunk/target/linux/ramips/dts/rt5350.dtsi > 85 uart@500 { > 86 compatible = "ralink,rt5350-uart", > "ralink,rt2880-uart", "ns16550a"; > 87 reg = <0x500 0x100>; > 88 > 89 resets = <&rstctrl 12>; > 90 reset-names = "uart"; > 91 > 92 interrupt-parent = <&intc>; > 93 interrupts = <5>; > 94 > 95 reg-shift = <2>; > 96 > 97 pinctrl-names = "default"; > 98 pinctrl-0 = <&uartf_pins>; > 99 > 100 status = "disabled"; > 101 }; > > > https://dev.openwrt.org/browser/trunk/target/linux/ramips/dts/mt7620a.dtsi > 85 uart@500 { > 86 compatible = "ralink,mt7620a-uart", > "ralink,rt2880-uart", "ns16550a"; > 87 reg = <0x500 0x100>; > 88 > 89 resets = <&rstctrl 12>; > 90 reset-names = "uart"; > 91 > 92 interrupt-parent = <&intc>; > 93 interrupts = <5>; > 94 > 95 reg-shift = <2>; > 96 > 97 status = "disabled"; > 98 }; > > > Without clearing the pinctrl-0 in the dts file as I have in my PR, the > parent dtsi file is asking for the uartf pins to be configured for > function “uartf” on line 255 of the parent dtsi: > > ralink,function = "uartf"; > > This dtsi function competes with dts function in the pull request: > > ralink,function = "gpio uartf"; > > The two conflicting settings will cause a kernel error at boot time. If > I take the pinctrl-0; override out, we can see this conflict: > > [ 0.197413] pinctrl core: add 1 pinctrl maps > [ 0.197487] rt2880-pinmux pinctrl: found group selector 2 for uartf > [ 0.197526] rt2880-pinmux pinctrl: request pin 7 (io7) for 10000500.uart > [ 0.197558] rt2880-pinmux pinctrl: pin io7 already requested by > pinctrl; cannot claim for 10000500.uart > [ 0.197895] rt2880-pinmux pinctrl: pin-7 (10000500.uart) status -22 > [ 0.198130] rt2880-pinmux pinctrl: could not request pin 7 (io7) from > group uartf on device rt2880-pinmux > [ 0.198471] of_serial 10000500.uart: Error applying setting, reverse > things back > [ 0.199394] 10000500.uart: ttyS0 at MMIO 0x10000500 (irq = 13, > base_baud = 2500000) is a Palmchip BK-3103
diff --git a/target/linux/ramips/dts/HLKRM04.dts b/target/linux/ramips/dts/HLKRM04.dts index f90a9ac..0a0705a 100644 --- a/target/linux/ramips/dts/HLKRM04.dts +++ b/target/linux/ramips/dts/HLKRM04.dts @@ -18,17 +18,17 @@ palmbus@10000000 { uart@500 { status = "okay"; + pinctrl-0; }; spi@b00 { status = "okay"; - m25p80@0 { #address-cells = <1>; #size-cells = <1>; compatible = "jedec,spi-nor"; reg = <0 0>; - linux,modalias = "m25p80", "s25fl064k"; + linux,modalias = "m25p80", "w25q32"; spi-max-frequency = <10000000>; partition@0 { @@ -60,9 +60,13 @@ pinctrl { state_default: pinctrl0 { gpio { - ralink,group = "jtag"; + ralink,group = "jtag", "i2c"; ralink,function = "gpio"; }; + uartf_gpio { + ralink,group = "uartf"; + ralink,function = "gpio uartf"; + }; }; }; @@ -82,25 +86,39 @@ status = "okay"; }; + gpio-export { + compatible = "gpio-export"; + #size-cells = <0>; + + /* I2C */ + gpio1 { + /* I2C_I2C_SD */ + gpio-export,name = "hlk-rm04:gpio0"; + gpio-export,direction_may_change = <1>; + gpios = <&gpio0 1 0>; + }; + gpio2 { + /* I2C_I2C_SCLK */ + gpio-export,name = "hlk-rm04:gpio1"; + gpio-export,direction_may_change = <1>; + gpios = <&gpio0 2 0>; + }; + }; + gpio-keys-polled { compatible = "gpio-keys-polled"; #address-cells = <1>; #size-cells = <0>; poll-interval = <20>; - - wps { + reset { label = "reset"; - gpios = <&gpio0 14 1>; + gpios = <&gpio0 0 1>; linux,code = <0x198>; }; - }; - - gpio-leds { - compatible = "gpio-leds"; - - power { - label = "hlk-rm04:red:power"; - gpios = <&gpio0 0 1>; + wps { + label = "wps"; + gpios = <&gpio0 14 1>; + linux,code = <0x211>; }; }; };
GPIO Changes ~~~~~~~~~~~~ The HLK-RM04 uses i2c and the top half of the uartf port for gpio. HiLink has labeled the RT5350 gpio1 pin as gpio0 on the device pinout, so it seems to make sense to export it as "hlk-rm04:gpio0" to avoid confusing it with the microcontroller's designation. gpio gpio0 gpio0 (pin 10, reset) i2c i2c_sd gpio1 (pin 8, hlk-rm04:gpio0) i2c i2c_sclk gpio2 (pin 9, hlk-rm04:gpio1) uartf dtr_n gpio11 (no pinout) uartf dcd_n gpio12 (no pinout) uartf dsr_n gpio13 (no pinout) uartf rin gpio14 (pin 25, wps) reference: http://www.hlktech.net/product_detail.php?ProId=39 http://cdn.sparkfun.com/datasheets/Wireless/WiFi/RT5350.pdf Also note that there is no gpio to control the power led, so remove the incorrect mapping. Memory Model Number (trivial) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The dts file references memory chip s25fl064k, which seems to have ever been used for the HiLink HLK-RM04. While it only causes a nag message in the kernel log, why not change it to the w25q32 which is commonly found on these devices. Signed-off-by: John Clark <inindev@gmail.com> --- target/linux/ramips/dts/HLKRM04.dts | 46 ++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 14 deletions(-)