[OpenWrt-Devel] [PATCH 3/3] ath79: add support for Teltonika RUT955

[Daniel Golle]

Hi Chuanhong,

thanks for the review and suggestions!

On Fri, Feb 07, 2020 at 10:35:08AM +0800, Chuanhong Guo wrote:
> Hi!
> 
> some minor comments below:
> On Thu, Feb 6, 2020 at 1:07 AM Daniel Golle <daniel at makrotopia.org> wrote:
> ...
> > +
> > +       gpio_ext_spi {
> > +               compatible = "spi-gpio";
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&pmx_led_spi_gpio>;
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               sck-gpios = <&gpio 4 GPIO_ACTIVE_HIGH>;     // 74HC595 SRCLK (Serial Clock)
> > +               mosi-gpios = <&gpio 12 GPIO_ACTIVE_HIGH>;   // 74HC595 SER (Serial)
> > +               cs-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;     // 74HC595 RCLK (Register Clock)
> > +               num-chipselects = <1>;
> > +
> > +               gpio_ext: gpio_ext at 0 {
> > +                       compatible = "fairchild,74hc595";
> > +                       reg = <0>;
> > +                       gpio-controller;
> > +                       #gpio-cells = <2>;
> > +                       registers-number = <2>;
> > +                       spi-max-frequency = <10000000>;
> > +                       gpio-line-names = "led_signal_bar1", "led_signal_bar2", "led_signal_bar3", "led_signal_bar4",
> > +                               "led_signal_bar5", "led_status_red", "led_status_green", "sim_sel",
> > +                               "DOUT1", "DOUT2", "modem_vbus", "modem_rst",
> > +                               "DOUT3", "RS485_R", "SDCS", "HWRST";
> > +               };
> > +       };
> 
> Would you mind to test whether it's possible to move this 74hc595
> under hardware spi instead? I believe using hw spi has a tiny bit less
> cpu overhead than gpio spi.
> It's possible to hook one output signal to multiple gpio pins
> simultaneously on ath79. Try setting gpio 4/12/20 to clk/mosi/cs2 in
> gpio function register respectively, move gpio_ext node under spi and
> change reg to 2.

I've tried that and it works just as good as it does with spi-gpio.
Now I don't know what's better: Sacrificing bus-access time on the
SPI bus of the NOR flash (CS0) and microSD-card (CS1) by adding the
HC959s (CS2) there in the way you described **or** having an
independent bit-banged bus just for the SPI-connected HC595 driven
LEDs. It's trading CPU-time for SPI-bus-access time for a rather
neglectable load, so I'm also biased to go with sacrificing SPI bus-
access time, ie. using CS2 and pinmux in the way you suggested and
I've just successfully tested.

> 
> > +
> > +       leds {
> > +               compatible = "gpio-leds";
> > +
> > +               signal0 {
> > +                       label = "rut955:green:signal1";
> > +                       gpios = <&gpio_ext 0 GPIO_ACTIVE_HIGH>;
> > +               };
> ...
> > +&spi {
> > +       status = "okay";
> > +
> > +       cs-gpios = <0>, <0>;
> 
> I pushed a spi driver replacement yesterday and we don't need this
> cs-gpios to fix num_chipselect in driver now :)

Ack. removed.

> 
> > +       num-cs = <2>;
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&jtag_disable_pins>, <&pmx_spi_cs1>;
> > +
> ...
> > +&eth1 {
> > +       status = "okay";
> > +
> > +       mtd-mac-address = <&config 0x0>;
> > +
> > +       gmac-config {
> > +               device = <&gmac>;
> > +       };
> 
> This gmac-config can be removed if you don't need it.

Ack. removed.

> 
> > +};
> > +
> > +&builtin_switch {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pmx_leds_switch>;
> > +};
> ...
> > +  IMAGE/factory.bin := append-kernel | pad-to $$$$(BLOCKSIZE) | append-rootfs |\
> > +       pad-rootfs | teltonika-fw-fake-checksum | append-string master |\
> > +       append-md5sum-bin | check-size $$$$(IMAGE_SIZE)
> > +  IMAGE/sysupgrade.bin := append-kernel | pad-to $$$$(BLOCKSIZE) |\
> 
> Do we need this "pad-to $$$$(BLOCKSIZE)" in
> factory.bin/sysupgrade.bin? kernel/rootfs can be split anywhere and we
> may squeeze out several kilobytes by removing this image padding.
> Also I'm wondering whether "tplink-v1-image sysupgrade" recipe could
> replace "append-kernel | append-rootfs | pad-rootfs" here.
> ("tplink-v1-header" needs to be removed from KERNEL or there'll be
> duplicated headers)

I've copied the image generation code from ar71xx/generic/rut900 which
shares the same base-board and flash layout. As pepe2k was suggesting
he would add RUT900 to ath79 after support for RUT955 got merged, I
leave it up to him so also re-organize the image generation code
because he is also the author of the original ar71xx code and I will
no longer have physical access to the device to play around with from
next week on... Hence I'd just leave it like it is for now.


> 
> > +       append-rootfs | pad-rootfs | append-metadata |\
> > +       check-size $$$$(IMAGE_SIZE)
> > +endef
> > +TARGET_DEVICES += teltonika_rut955
> > +
> >  define Device/trendnet_tew-823dru
> >    SOC := qca9558
> >    DEVICE_VENDOR := Trendnet
> > --
> 
> Regards,
> Chuanhong Guo

_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel