[U-Boot] [PATCH v2 0/7] rockchip: dts: rk3399-puma: update DTS

[U-Boot] [PATCH v2 0/7] rockchip: dts: rk3399-puma: update DTS

[Philipp Tomsich]

For the RK3399-Q7, there's been a number of changes to the DTS from
the ongoing Linux development and from recently enabled functionality
within U-Boot.

This now makes a patch-series out of what originally was a single
patch and includes the following changes:
- adds a dependent patch that had gone upstream with the RK3399 HDMI
  support, but is needed for the updated DTS to not break the build
  for the entire architecture
- fixes a number of unrelated warnings/build errors (reported from
  buildman) that showed up after rebasing onto u-boot-rockchip/master at 2b19b2f


Changes in v2:
- (new patch) rename dev_get_addr in the RV1108 clk-driver to fix a
  buildman failure for u-boot-rockchip/master at 2b19b2f
- (new patch) access of-offset of periph via dev_of_offset in the RV1108
  pinctrl driver to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
- (new patch) fix a multiple definition of usb_gadget_handle_interrupts
  for the RK3328 EVB in evb-rk3328.c to fix a buildman failure for
  u-boot-rockchip/master at 2b19b2f
- (new patch) fix a int-to-pointer cast warning for regs_otg in
  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
- (new patch) fix warnings and add some robustness for the truncation
  of ep->dma_buf in dwc2-otg for LP64 builds to fix a buildman failure
  for u-boot-rockchip/master at 2b19b2f
- cherry-picked the DTS change adding the hdmi node for the rk3399.dtsi
  from the RK3399 HDMI enabledment series (already applied there)

Philipp Tomsich (7):
  rockchip: clk: rv1108: rename dev_get_addr
  rockchip: pinctrl: rv1108: access of-offset via dev_of_offset(...)
  rockchip: rk3328: don't implement usb_gadget_handle_interrupts twice
  usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t
  usb: dwc2-otg: fix warnings for ep->dma_buf for LP64 builds
  rockchip: dts: rk3399: enable HDMI output in the DTS
  rockchip: dts: rk3399-puma: sync DTS with Linux tree

 arch/arm/dts/rk3399-puma.dts               | 540 ++++++++++++++++++++++++++---
 arch/arm/dts/rk3399.dtsi                   |  39 +++
 board/rockchip/evb_rk3328/evb-rk3328.c     |   5 -
 drivers/clk/rockchip/clk_rv1108.c          |   2 +-
 drivers/pinctrl/rockchip/pinctrl_rv1108.c  |   2 +-
 drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c |   6 +-
 include/usb/dwc2_udc.h                     |   2 +-
 7 files changed, 542 insertions(+), 54 deletions(-)

-- 
2.1.4

[U-Boot] [PATCH v2 1/7] rockchip: clk: rv1108: rename dev_get_addr

[Philipp Tomsich]

After rebasing to u-boot-rockchip/master at 2b19b2f, buildman fails for
rv1108 with:
  ../drivers/clk/rockchip/clk_rv1108.c: In function 'rv1108_clk_probe':
  ../drivers/clk/rockchip/clk_rv1108.c:191:22: warning: implicit declaration of function 'dev_get_addr' [-Wimplicit-function-declaration]
    priv->cru = (struct rv1108_cru *)dev_get_addr(dev);

This change tracks the dev_get_addr rename, which didn't make it into
the rv1108 clk driver.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

---

Changes in v2:
- (new patch) rename dev_get_addr in the RV1108 clk-driver to fix a
  buildman failure for u-boot-rockchip/master at 2b19b2f

 drivers/clk/rockchip/clk_rv1108.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk_rv1108.c b/drivers/clk/rockchip/clk_rv1108.c
index 0946dab..0a3ba3b 100644
--- a/drivers/clk/rockchip/clk_rv1108.c
+++ b/drivers/clk/rockchip/clk_rv1108.c
@@ -188,7 +188,7 @@ static int rv1108_clk_probe(struct udevice *dev)
 {
 	struct rv1108_clk_priv *priv = dev_get_priv(dev);
 
-	priv->cru = (struct rv1108_cru *)dev_get_addr(dev);
+	priv->cru = (struct rv1108_cru *)devfdt_get_addr(dev);
 
 	rkclk_init(priv->cru);
 
-- 
2.1.4

[U-Boot] [PATCH v2 2/7] rockchip: pinctrl: rv1108: access of-offset via dev_of_offset(...)

[Philipp Tomsich]

After rebasing to u-boot-rockchip/master at 2b19b2f, buildman fails for
rv1108 with:
  ../drivers/pinctrl/rockchip/pinctrl_rv1108.c: In function 'rv1108_pinctrl_get_periph_id':
  ../drivers/pinctrl/rockchip/pinctrl_rv1108.c:111:49: error: 'struct udevice' has no member named 'of_offset'
    ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
                                                     ^

This change access the of-offset of periph via the dev_of_offset()
helper-function to fix this issue and (hopefully) to ensure it doesn't
recur if there's more changes to the DM subsystem.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

---

Changes in v2:
- (new patch) access of-offset of periph via dev_of_offset in the RV1108
  pinctrl driver to fix a buildman failure for u-boot-rockchip/master at 2b19b2f

 drivers/pinctrl/rockchip/pinctrl_rv1108.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/rockchip/pinctrl_rv1108.c b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
index d98ec81..bdf3910 100644
--- a/drivers/pinctrl/rockchip/pinctrl_rv1108.c
+++ b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
@@ -108,7 +108,7 @@ static int rv1108_pinctrl_get_periph_id(struct udevice *dev,
 	u32 cell[3];
 	int ret;
 
-	ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
+	ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(periph),
 				   "interrupts", cell, ARRAY_SIZE(cell));
 	if (ret < 0)
 		return -EINVAL;
-- 
2.1.4

[U-Boot] [PATCH v2 3/7] rockchip: rk3328: don't implement usb_gadget_handle_interrupts twice

[Philipp Tomsich]

The usb_gadget_handle_interrupts()-function is already implemented by
drivers/usb/gadget/dwc2_udc_otg.c, so we need to avoid defining it
in the evb-rk3328.c board-specific file.

This change fixes the following build error (from buildman):
  drivers/usb/gadget/built-in.o: In function `usb_gadget_handle_interrupts':
  build/../drivers/usb/gadget/dwc2_udc_otg.c:850: multiple definition of `usb_gadget_handle_interrupts'
  board/rockchip/evb_rk3328/built-in.o:build/../board/rockchip/evb_rk3328/evb-rk3328.c:37: first defined here
  make[1]: *** [u-boot] Error 1

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

---

Changes in v2:
- (new patch) fix a multiple definition of usb_gadget_handle_interrupts
  for the RK3328 EVB in evb-rk3328.c to fix a buildman failure for
  u-boot-rockchip/master at 2b19b2f

 board/rockchip/evb_rk3328/evb-rk3328.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/board/rockchip/evb_rk3328/evb-rk3328.c b/board/rockchip/evb_rk3328/evb-rk3328.c
index a7895cb..0a26ed5 100644
--- a/board/rockchip/evb_rk3328/evb-rk3328.c
+++ b/board/rockchip/evb_rk3328/evb-rk3328.c
@@ -31,11 +31,6 @@ int dram_init_banksize(void)
 	return 0;
 }
 
-int usb_gadget_handle_interrupts(void)
-{
-	return 0;
-}
-
 int board_usb_init(int index, enum usb_init_type init)
 {
 	return 0;
-- 
2.1.4

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Philipp Tomsich]

The regs_otg field in uintptr_t of the platform data structure for
dwc2-otg has thus far been an unsigned int, but will eventually be
casted into a void*.

This raises the following error with GCC 6.3 and buildman:
  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
          ^

This changes regs_otg to a uintptr_t to ensure that it is large enough
to hold any valid pointer (and fix the associated warning).

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

---

Changes in v2:
- (new patch) fix a int-to-pointer cast warning for regs_otg in
  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f

 include/usb/dwc2_udc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
index 7324d8a..1a370e0 100644
--- a/include/usb/dwc2_udc.h
+++ b/include/usb/dwc2_udc.h
@@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
 	int		phy_of_node;
 	int		(*phy_control)(int on);
 	unsigned int	regs_phy;
-	unsigned int	regs_otg;
+	uintptr_t	regs_otg;
 	unsigned int    usb_phy_ctrl;
 	unsigned int    usb_flags;
 	unsigned int	usb_gusbcfg;
-- 
2.1.4

[U-Boot] [PATCH v2 5/7] usb: dwc2-otg: fix warnings for ep->dma_buf for LP64 builds

[Philipp Tomsich]

For LP64 build w/ GCC 6.3, the handling of the ep->dma_buf variable
when (truncating and) writing into the controller's registers is
unsafe and triggers the following compiler warning (thus failing by
buildman tests):
  ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c: In function 'setdma_rx':
  ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c:116:9: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
           ^

This change fixes the warning and makes the code a bit more robust by
doing the following:
- we check whether ep->dma_buf can be safely truncated to 32 bits
  (i.e. whether dma_buf is the same value when masked to 32 bits) and
  emit an error message if that is not the case
- we cast to a uintptr_t and let the writel macro truncate the
  uintptr_t (potentially a 64bit value) to 32bits

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

---

Changes in v2:
- (new patch) fix warnings and add some robustness for the truncation
  of ep->dma_buf in dwc2-otg for LP64 builds to fix a buildman failure
  for u-boot-rockchip/master at 2b19b2f

 drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
index 0d6d2fb..f5c926c 100644
--- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
+++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
@@ -113,7 +113,11 @@ static int setdma_rx(struct dwc2_ep *ep, struct dwc2_request *req)
 	invalidate_dcache_range((unsigned long) ep->dma_buf,
 				(unsigned long) ep->dma_buf + ep->len);
 
-	writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
+	/* ensure that ep->dma_buf fits into 32 bits */
+	if (((uintptr_t)ep->dma_buf & 0xffffffff) != (uintptr_t)ep->dma_buf)
+		error("ep->dma_buf does not fit into a 32 bits");
+
+	writel((uintptr_t)ep->dma_buf, &reg->out_endp[ep_num].doepdma);
 	writel(DOEPT_SIZ_PKT_CNT(pktcnt) | DOEPT_SIZ_XFER_SIZE(length),
 	       &reg->out_endp[ep_num].doeptsiz);
 	writel(DEPCTL_EPENA|DEPCTL_CNAK|ctrl, &reg->out_endp[ep_num].doepctl);
-- 
2.1.4

[U-Boot] [PATCH v2 6/7] rockchip: dts: rk3399: enable HDMI output in the DTS

[Philipp Tomsich]

This commit enables HDMI output in the DTS by adding the necessary
nodes to vopl/vopb and by adding the HDMI node.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

---

Changes in v2:
- cherry-picked the DTS change adding the hdmi node for the rk3399.dtsi
  from the RK3399 HDMI enabledment series (already applied there)

 arch/arm/dts/rk3399.dtsi | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/arch/arm/dts/rk3399.dtsi b/arch/arm/dts/rk3399.dtsi
index f3d3f53..7f1fc50 100644
--- a/arch/arm/dts/rk3399.dtsi
+++ b/arch/arm/dts/rk3399.dtsi
@@ -1419,6 +1419,11 @@
 				reg = <3>;
 				remote-endpoint = <&mipi_in_vopl>;
 			};
+
+			vopl_out_hdmi: endpoint at 1 {
+				reg = <1>;
+				remote-endpoint = <&hdmi_in_vopl>;
+			};
 		};
 	};
 
@@ -1440,6 +1445,40 @@
 				reg = <3>;
 				remote-endpoint = <&mipi_in_vopb>;
 			};
+
+			vopb_out_hdmi: endpoint at 1 {
+				reg = <1>;
+				remote-endpoint = <&hdmi_in_vopb>;
+			};
+		};
+	};
+
+	hdmi: hdmi at ff940000 {
+		compatible = "rockchip,rk3399-dw-hdmi";
+		reg = <0x0 0xff940000 0x0 0x20000>;
+		reg-io-width = <4>;
+		rockchip,grf = <&grf>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&hdmi_i2c_xfer>;
+		power-domains = <&power RK3399_PD_HDCP>;
+		interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru PCLK_HDMI_CTRL>, <&cru SCLK_HDMI_SFR>, <&cru PLL_VPLL>, <&cru PCLK_VIO_GRF>;
+		clock-names = "iahb", "isfr", "vpll", "grf";
+		status = "disabled";
+
+		ports {
+			hdmi_in: port {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				hdmi_in_vopb: endpoint at 0 {
+					reg = <0>;
+					remote-endpoint = <&vopb_out_hdmi>;
+				};
+				hdmi_in_vopl: endpoint at 1 {
+					reg = <1>;
+					remote-endpoint = <&vopl_out_hdmi>;
+				};
+			};
 		};
 	};
 
-- 
2.1.4

[U-Boot] [PATCH v2 7/7] rockchip: dts: rk3399-puma: sync DTS with Linux tree

[Philipp Tomsich]

The Linux DTS for the RK3399-Q7 has moved with the times... resync
against it to ensure a consistent configuration.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v2: None

 arch/arm/dts/rk3399-puma.dts | 540 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 495 insertions(+), 45 deletions(-)

diff --git a/arch/arm/dts/rk3399-puma.dts b/arch/arm/dts/rk3399-puma.dts
index c04a853..fca14d3 100644
--- a/arch/arm/dts/rk3399-puma.dts
+++ b/arch/arm/dts/rk3399-puma.dts
@@ -5,17 +5,18 @@
  */
 
 /dts-v1/;
+
 #include <dt-bindings/pwm/pwm.h>
 #include "rk3399.dtsi"
 #include "rk3399-sdram-ddr3-1600.dtsi"
 
 / {
 	model = "Theobroma Systems RK3399-Q7 SoM";
-	compatible = "tsd,puma", "rockchip,rk3399";
+	compatible = "tsd,rk3399-q7", "tsd,puma", "rockchip,rk3399";
 
 	config {
-		u-boot,spl-payload-offset = <0x40000>; // 256kbyte
-		u-boot,boot-led = "puma:orange:power";
+		u-boot,spl-payload-offset = <0x40000>; /* 256kbyte */
+		u-boot,boot-led = "module_led";
 	};
 
 	chosen {
@@ -28,16 +29,74 @@
 		spi1 = &spi5;
 	};
 
-	vdd_center: vdd-center {
-		compatible = "pwm-regulator";
-		pwms = <&pwm3 0 25000 0>;
-		regulator-name = "vdd_center";
-		regulator-min-microvolt = <800000>;
-		regulator-max-microvolt = <1400000>;
-		regulator-init-microvolt = <950000>;
+	leds {
+		compatible = "gpio-leds";
+		pinctrl-names = "default";
+		pinctrl-0 = <&leds_pins_puma>;
+
+		module_led {
+			label = "module_led";
+			gpios = <&gpio2 25 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "heartbeat";
+		};
+
+		sd_card_led {
+			label = "sd_card_led";
+			gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "mmc0";
+		};
+	};
+
+	clkin_gmac: external-gmac-clock {
+		compatible = "fixed-clock";
+		clock-frequency = <125000000>;
+		clock-output-names = "clkin_gmac";
+		#clock-cells = <0>;
+	};
+
+	dw_hdmi_audio: dw-hdmi-audio {
+		status = "enabled";
+		compatible = "rockchip,dw-hdmi-audio";
+		#sound-dai-cells = <0>;
+	};
+
+	hdmi_codec: hdmi-codec {
+		compatible = "simple-audio-card";
+		simple-audio-card,format = "i2s";
+		simple-audio-card,mclk-fs = <256>;
+		simple-audio-card,name = "HDMI-CODEC";
+
+		simple-audio-card,cpu {
+			sound-dai = <&i2s2>;
+		};
+
+		simple-audio-card,codec {
+			sound-dai = <&hdmi>;
+		};
+	};
+
+	hdmi_sound: hdmi-sound {
+		status = "disabled";
+		compatible = "simple-audio-card";
+		simple-audio-card,format = "i2s";
+		simple-audio-card,mclk-fs = <256>;
+		simple-audio-card,name = "rockchip,hdmi";
+
+		simple-audio-card,cpu {
+			sound-dai = <&i2s2>;
+		};
+		simple-audio-card,codec {
+			sound-dai = <&hdmi>;
+		};
+	};
+
+	vccadc_ref: vccadc-ref {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc1v8_sys";
 		regulator-always-on;
 		regulator-boot-on;
-		status = "okay";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
 	};
 
 	vcc3v3_sys: vcc3v3-sys {
@@ -49,24 +108,33 @@
 		regulator-max-microvolt = <3300000>;
 	};
 
-	vcc_phy: vcc-phy-regulator {
+	vcc5v0_otg: vcc5v0-otg-regulator {
 		compatible = "regulator-fixed";
-		regulator-name = "vcc_phy";
+		enable-active-high;
+		gpio = <&gpio0 2 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&otg_vbus_drv>;
+		regulator-name = "vcc5v0_otg";
 		regulator-always-on;
-		regulator-boot-on;
 	};
 
-	vcc5v0_host: vcc5v0-host-en {
+	vcc5v0_host: vcc5v0-host-regulator {
 		compatible = "regulator-fixed";
+		enable-active-low;
+		gpio = <&gpio4 3 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&host_vbus_drv>;
 		regulator-name = "vcc5v0_host";
-		gpio = <&gpio4 25 GPIO_ACTIVE_HIGH>;
+		regulator-always-on;
 	};
 
-	clkin_gmac: external-gmac-clock {
-		compatible = "fixed-clock";
-		clock-frequency = <125000000>;
-		clock-output-names = "clkin_gmac";
-		#clock-cells = <0>;
+	vcc5v0_sys: vcc5v0-sys {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc5v0_sys";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
 	};
 
 	vcc_phy: vcc-phy-regulator {
@@ -75,40 +143,350 @@
 		regulator-always-on;
 		regulator-boot-on;
 	};
+
+	vdd_log: vdd-log {
+		compatible = "pwm-regulator";
+		pwms = <&pwm2 0 25000 1>;
+		regulator-name = "vdd_log";
+		regulator-min-microvolt = <800000>;
+		regulator-max-microvolt = <1400000>;
+		regulator-always-on;
+		regulator-boot-on;
+
+		/* for rockchip boot on */
+		rockchip,pwm_id= <2>;
+		rockchip,pwm_voltage = <1000000>;
+	};
 };
 
 &emmc_phy {
 	status = "okay";
 };
 
-&pwm0 {
+&gmac {
+	phy-supply = <&vcc_phy>;
+	phy-mode = "rgmii";
+	clock_in_out = "input";
+	snps,reset-gpio = <&gpio3 16 GPIO_ACTIVE_LOW>;
+	snps,reset-active-low;
+	snps,reset-delays-us = <2 10000 50000>;
+	assigned-clocks = <&cru SCLK_RMII_SRC>;
+	assigned-clock-parents = <&clkin_gmac>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&rgmii_pins>;
+	tx_delay = <0x10>;
+	rx_delay = <0x10>;
 	status = "okay";
 };
 
-&pwm2 {
+&hdmi {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	#sound-dai-cells = <0>;
 	status = "okay";
 };
 
-&pwm3 {
+&i2c0 {
 	status = "okay";
+	i2c-scl-rising-time-ns = <168>;
+	i2c-scl-falling-time-ns = <4>;
+	clock-frequency = <400000>;
+
+	vdd_gpu: fan535555 at 60 {
+		compatible = "fcs,fan53555";
+		reg = <0x60>;
+		vsel-gpios = <&gpio1 14 GPIO_ACTIVE_HIGH>;
+		vin-supply = <&vcc5v0_sys>;
+		regulator-compatible = "fan53555-reg";
+		regulator-name = "vdd_gpu";
+		regulator-min-microvolt = <600000>;
+		regulator-max-microvolt = <1230000>;
+		regulator-ramp-delay = <1000>;
+		fcs,suspend-voltage-selector = <1>;
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-initial-state = <3>;
+			regulator-state-mem {
+			regulator-off-in-suspend;
+		};
+	};
+
+	rk808: pmic at 1b {
+		compatible = "rockchip,rk808";
+		reg = <0x1b>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <22 IRQ_TYPE_LEVEL_LOW>;  // TODO check interrupt?
+		pinctrl-names = "default";
+		pinctrl-0 = <&pmic_int_l>;
+		rockchip,system-power-controller;
+		wakeup-source;
+		#clock-cells = <1>;
+		clock-output-names = "xin32k", "rk808-clkout2";
+
+		vcc1-supply = <&vcc5v0_sys>;
+		vcc2-supply = <&vcc5v0_sys>;
+		vcc3-supply = <&vcc5v0_sys>;
+		vcc4-supply = <&vcc5v0_sys>;
+		vcc6-supply = <&vcc5v0_sys>;
+		vcc7-supply = <&vcc5v0_sys>;
+		vcc8-supply = <&vcc3v3_sys>;
+		vcc9-supply = <&vcc5v0_sys>;
+		vcc10-supply = <&vcc5v0_sys>;
+		vcc11-supply = <&vcc5v0_sys>;
+		vcc12-supply = <&vcc3v3_sys>;
+		vddio-supply = <&vcc1v8_pmu>;
+
+		regulators {
+			vdd_center: DCDC_REG1 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <750000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-ramp-delay = <6001>;
+				regulator-name = "vdd_center";
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vdd_cpu_l: DCDC_REG2 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <750000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-ramp-delay = <6001>;
+				regulator-name = "vdd_cpu_l";
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc_ddr: DCDC_REG3 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-name = "vcc_ddr";
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			vcc_1v8: DCDC_REG4 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-name = "vcc_1v8";
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+
+			vcc_ldo1: LDO_REG1 {
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-name = "vcc_ldo1";
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc1v8_hdmi: LDO_REG2 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-name = "vcc1v8_hdmi";
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc1v8_pmu: LDO_REG3 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-name = "vcc1v8_pmu";
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+
+			vcc_sd: LDO_REG4 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-name = "vcc_sd";
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <3300000>;
+				};
+			};
+
+			vcc_ldo5: LDO_REG5 {
+				regulator-boot-on;
+				regulator-min-microvolt = <3000000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-name = "vcc_ldo5";
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc_ldo6: LDO_REG6 {
+				regulator-boot-on;
+				regulator-min-microvolt = <1500000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-name = "vcc_ldo6";
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc0v9_hdmi: LDO_REG7 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <900000>;
+				regulator-max-microvolt = <900000>;
+				regulator-name = "vcc0v9_hdmi";
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc_efuse: LDO_REG8 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-name = "vcc_efuse";
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc3v3_s3: SWITCH_REG1 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-name = "vcc3v3_s3";
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc3v3_s0: SWITCH_REG2 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-name = "vcc3v3_s0";
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+		};
+	};
 };
 
-&sdmmc {
-        u-boot,dm-pre-reloc;
-	bus-width = <4>;
+&i2c8 {
+	status = "okay";
+	clock-frequency = <400000>;
+
+	vdd_cpu_b: fan53555 at 60 {
+		compatible = "fcs,fan53555";
+		reg = <0x60>;
+		vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
+		vin-supply = <&vcc5v0_sys>;
+		regulator-compatible = "fan53555-reg";
+		regulator-name = "vdd_cpu_b";
+		regulator-min-microvolt = <600000>;
+		regulator-max-microvolt = <1230000>;
+		regulator-ramp-delay = <1000>;
+		fcs,suspend-voltage-selector = <1>;
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-initial-state = <3>;
+			regulator-state-mem {
+			regulator-off-in-suspend;
+		};
+	};
+};
+
+&i2s0 {
+	status = "okay";
+	rockchip,i2s-broken-burst-len;
+	rockchip,playback-channels = <8>;
+	rockchip,capture-channels = <8>;
+	#sound-dai-cells = <0>;
+};
+
+&i2s2 {
+	#sound-dai-cells = <0>;
+	status = "okay";
+};
+
+&io_domains {
+	status = "okay";
+
+	bt656-supply = <&vcc_1v8>;	/* bt656_gpio2ab_ms */
+	audio-supply = <&vcc_1v8>;	/* audio_gpio3d4a_ms */
+	sdmmc-supply = <&vcc_sd>;	/* sdmmc_gpio4b_ms */
+	gpio1830-supply = <&vcc_1v8>;	/* gpio1833_gpio4cd_ms */
+};
+
+&pcie0 {
+	assigned-clocks = <&cru SCLK_PCIEPHY_REF>;
+	assigned-clock-parents = <&cru SCLK_PCIEPHY_REF100M>;
+	assigned-clock-rates = <100000000>;
+	ep-gpios = <&gpio4 22 GPIO_ACTIVE_HIGH>;
+	num-lanes = <4>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie_clkreqn>;
+	status = "okay";
+};
+
+&pcie_phy {
+	        status = "okay";
+};
+
+&pmu_io_domains {
+	status = "okay";
+	pmu1830-supply = <&vcc_1v8>;
+};
+
+&pwm0 {
+	status = "okay";
+};
+
+&pwm2 {
 	status = "okay";
 };
 
 &sdhci {
 	bus-width = <8>;
 	mmc-hs400-1_8v;
-	mmc-hs400-enhanced-strobe;
+	supports-emmc;
 	non-removable;
+	keep-power-in-suspend;
+	mmc-hs400-enhanced-strobe;
 	status = "okay";
 };
 
-&uart0 {
-	u-boot,dm-pre-reloc;
+&sdmmc {
+        u-boot,dm-pre-reloc;
+	clock-frequency = <150000000>;
+	clock-freq-min-max = <100000 150000000>;
+	supports-sd;
+	bus-width = <4>;
+	cap-mmc-highspeed;
+	cap-sd-highspeed;
+	disable-wp;
+	num-slots = <1>;
+	vqmmc-supply = <&vcc_sd>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
 	status = "okay";
 };
 
@@ -141,36 +519,107 @@
 	status = "okay";
 };
 
+&vopb {
+	status = "okay";
+};
+
 &pinctrl {
+	/* Pins that are not explicitely used by any devices */
+	pinctrl-names = "default";
+	pinctrl-0 = <&puma_pin_hog>;
+	hog {
+		puma_pin_hog: puma_pin_hog {
+			rockchip,pins =
+				/* We need pull-ups on Q7 buttons */
+				<0  4 RK_FUNC_GPIO &pcfg_pull_up>, /* LID_BTN# */
+				<0 10 RK_FUNC_GPIO &pcfg_pull_up>, /* BATLOW# */
+				<0 11 RK_FUNC_GPIO &pcfg_pull_up>, /* SLP_BTN# */
+				<0  9 RK_FUNC_GPIO &pcfg_pull_up>; /* BIOS_DISABLE# */
+		};
+	};
+
 	pmic {
 		pmic_int_l: pmic-int-l {
 			rockchip,pins =
-				<1 21 RK_FUNC_GPIO &pcfg_pull_up>;
+				<1 22 RK_FUNC_GPIO &pcfg_pull_up>;
 		};
+	};
+
+	leds_pins_puma: led_pins at 0 {
+			rockchip,pins =
+				<2 25 RK_FUNC_GPIO &pcfg_pull_none>,
+				<1 2 RK_FUNC_GPIO &pcfg_pull_none>;
+	};
 
-		pmic_dvs2: pmic-dvs2 {
+	usb2 {
+		otg_vbus_drv: otg-vbus-drv {
 			rockchip,pins =
-				<1 18 RK_FUNC_GPIO &pcfg_pull_down>;
+				<0 2 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+
+		host_vbus_drv: host-vbus-drv {
+			rockchip,pins =
+				<0 2 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	i2c8 {
+		i2c8_xfer_a: i2c8-xfer {
+			rockchip,pins = <1 21 RK_FUNC_1 &pcfg_pull_up>,
+			                <1 20 RK_FUNC_1 &pcfg_pull_up>;
 		};
 	};
 };
 
-&gmac {
-        phy-supply = <&vcc_phy>;
-	phy-mode = "rgmii";
-	clock_in_out = "input";
-	snps,reset-gpio = <&gpio3 16 GPIO_ACTIVE_LOW>;
-	snps,reset-active-low;
-	snps,reset-delays-us = <0 10000 50000>;
-	assigned-clocks = <&cru SCLK_RMII_SRC>;
-	assigned-clock-parents = <&clkin_gmac>;
+&i2c1 {
+	status = "okay";
+	clock-frequency = <400000>;
+};
+&i2c2 {
+	status = "okay";
+	clock-frequency = <400000>;
+};
+&i2c4 {
+	status = "okay";
+	clock-frequency = <400000>;
+};
+&i2c6 {
+	status = "okay";
+	clock-frequency = <400000>;
+};
+
+&i2c6_xfer {
+	/* Enable pull-ups, the pins would float otherwise. */
+	rockchip,pins =
+		<2 10 RK_FUNC_2 &pcfg_pull_up>,
+		<2 9 RK_FUNC_2 &pcfg_pull_up>;
+};
+
+&i2c7 {
+	status = "okay";
+	clock-frequency = <400000>;
+
+	rtc_twi: rtc at 6f {
+		compatible = "isil,isl1208";
+		reg = <0x6f>;
+	};
+	fan: fan at 18 {
+		compatible = "ti,amc6821";
+		reg = <0x18>;
+		cooling-min-state = <0>;
+		cooling-max-state = <9>;
+		#cooling-cells = <2>;
+	};
+};
+
+&uart0 {
+	u-boot,dm-pre-reloc;
 	pinctrl-names = "default";
-	pinctrl-0 = <&rgmii_pins>;
-	tx_delay = <0x10>;
-	rx_delay = <0x10>;
+	pinctrl-0 = <&uart0_xfer &uart0_cts>;
 	status = "okay";
 };
 
+
 &spi1 {
 	u-boot,dm-pre-reloc;
 
@@ -193,3 +642,4 @@
 &spi5 {
 	status = "okay";
 };
+
-- 
2.1.4

[U-Boot] [PATCH v2 5/7] usb: dwc2-otg: fix warnings for ep->dma_buf for LP64 builds

[Marek Vasut]

On 06/06/2017 03:42 PM, Philipp Tomsich wrote:
> For LP64 build w/ GCC 6.3, the handling of the ep->dma_buf variable
> when (truncating and) writing into the controller's registers is
> unsafe and triggers the following compiler warning (thus failing by
> buildman tests):
>   ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c: In function 'setdma_rx':
>   ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c:116:9: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
>     writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>            ^
> 
> This change fixes the warning and makes the code a bit more robust by
> doing the following:
> - we check whether ep->dma_buf can be safely truncated to 32 bits
>   (i.e. whether dma_buf is the same value when masked to 32 bits) and
>   emit an error message if that is not the case
> - we cast to a uintptr_t and let the writel macro truncate the
>   uintptr_t (potentially a 64bit value) to 32bits
> 
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> 
> ---
> 
> Changes in v2:
> - (new patch) fix warnings and add some robustness for the truncation
>   of ep->dma_buf in dwc2-otg for LP64 builds to fix a buildman failure
>   for u-boot-rockchip/master at 2b19b2f
> 
>  drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> index 0d6d2fb..f5c926c 100644
> --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> @@ -113,7 +113,11 @@ static int setdma_rx(struct dwc2_ep *ep, struct dwc2_request *req)
>  	invalidate_dcache_range((unsigned long) ep->dma_buf,
>  				(unsigned long) ep->dma_buf + ep->len);
>  
> -	writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
> +	/* ensure that ep->dma_buf fits into 32 bits */
> +	if (((uintptr_t)ep->dma_buf & 0xffffffff) != (uintptr_t)ep->dma_buf)
> +		error("ep->dma_buf does not fit into a 32 bits");

You print an error, but still continue with the function ? That'll
probably lead to a crash, you should rather abort. In fact, I suspect
you can use bounce-buffer to assure the dma buffer is in 32bit space.

> +	writel((uintptr_t)ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>  	writel(DOEPT_SIZ_PKT_CNT(pktcnt) | DOEPT_SIZ_XFER_SIZE(length),
>  	       &reg->out_endp[ep_num].doeptsiz);
>  	writel(DEPCTL_EPENA|DEPCTL_CNAK|ctrl, &reg->out_endp[ep_num].doepctl);
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Marek Vasut]

On 06/06/2017 03:42 PM, Philipp Tomsich wrote:
> The regs_otg field in uintptr_t of the platform data structure for
> dwc2-otg has thus far been an unsigned int, but will eventually be
> casted into a void*.
> 
> This raises the following error with GCC 6.3 and buildman:
>   ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>   ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>     reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>           ^
> 
> This changes regs_otg to a uintptr_t to ensure that it is large enough
> to hold any valid pointer (and fix the associated warning).
> 
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

Applied, thanks

> ---
> 
> Changes in v2:
> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>   dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
> 
>  include/usb/dwc2_udc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
> index 7324d8a..1a370e0 100644
> --- a/include/usb/dwc2_udc.h
> +++ b/include/usb/dwc2_udc.h
> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>  	int		phy_of_node;
>  	int		(*phy_control)(int on);
>  	unsigned int	regs_phy;
> -	unsigned int	regs_otg;
> +	uintptr_t	regs_otg;
>  	unsigned int    usb_phy_ctrl;
>  	unsigned int    usb_flags;
>  	unsigned int	usb_gusbcfg;
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 5/7] usb: dwc2-otg: fix warnings for ep->dma_buf for LP64 builds

[Dr. Philipp Tomsich]

Marek,

> On 06 Jun 2017, at 16:35, Marek Vasut <marex@denx.de> wrote:
> 
> On 06/06/2017 03:42 PM, Philipp Tomsich wrote:
>> For LP64 build w/ GCC 6.3, the handling of the ep->dma_buf variable
>> when (truncating and) writing into the controller's registers is
>> unsafe and triggers the following compiler warning (thus failing by
>> buildman tests):
>>  ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c: In function 'setdma_rx':
>>  ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c:116:9: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
>>    writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>           ^
>> 
>> This change fixes the warning and makes the code a bit more robust by
>> doing the following:
>> - we check whether ep->dma_buf can be safely truncated to 32 bits
>>  (i.e. whether dma_buf is the same value when masked to 32 bits) and
>>  emit an error message if that is not the case
>> - we cast to a uintptr_t and let the writel macro truncate the
>>  uintptr_t (potentially a 64bit value) to 32bits
>> 
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> 
>> ---
>> 
>> Changes in v2:
>> - (new patch) fix warnings and add some robustness for the truncation
>>  of ep->dma_buf in dwc2-otg for LP64 builds to fix a buildman failure
>>  for u-boot-rockchip/master at 2b19b2f
>> 
>> drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>> index 0d6d2fb..f5c926c 100644
>> --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>> @@ -113,7 +113,11 @@ static int setdma_rx(struct dwc2_ep *ep, struct dwc2_request *req)
>> 	invalidate_dcache_range((unsigned long) ep->dma_buf,
>> 				(unsigned long) ep->dma_buf + ep->len);
>> 
>> -	writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>> +	/* ensure that ep->dma_buf fits into 32 bits */
>> +	if (((uintptr_t)ep->dma_buf & 0xffffffff) != (uintptr_t)ep->dma_buf)
>> +		error("ep->dma_buf does not fit into a 32 bits");
> 
> You print an error, but still continue with the function ? That'll
> probably lead to a crash, you should rather abort. In fact, I suspect
> you can use bounce-buffer to assure the dma buffer is in 32bit space.

This tries to stay close to the current behaviour (we have no hardware to test this,
but I wanted to get it out of the way as it caused buildman-failures) and to provide
diagnostics for whoever first encounters an issue here.

Note that I would only expect a DMA error, if someone really hit this issue ...

Regards,
Philipp.

>> +	writel((uintptr_t)ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>> 	writel(DOEPT_SIZ_PKT_CNT(pktcnt) | DOEPT_SIZ_XFER_SIZE(length),
>> 	       &reg->out_endp[ep_num].doeptsiz);
>> 	writel(DEPCTL_EPENA|DEPCTL_CNAK|ctrl, &reg->out_endp[ep_num].doepctl);
>> 
> 
> 
> -- 
> Best regards,
> Marek Vasut

[U-Boot] [PATCH v2 1/7] rockchip: clk: rv1108: rename dev_get_addr

[Simon Glass]

On 6 June 2017 at 07:42, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> After rebasing to u-boot-rockchip/master at 2b19b2f, buildman fails for
> rv1108 with:
>   ../drivers/clk/rockchip/clk_rv1108.c: In function 'rv1108_clk_probe':
>   ../drivers/clk/rockchip/clk_rv1108.c:191:22: warning: implicit declaration of function 'dev_get_addr' [-Wimplicit-function-declaration]
>     priv->cru = (struct rv1108_cru *)dev_get_addr(dev);
>
> This change tracks the dev_get_addr rename, which didn't make it into
> the rv1108 clk driver.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
> ---
>
> Changes in v2:
> - (new patch) rename dev_get_addr in the RV1108 clk-driver to fix a
>   buildman failure for u-boot-rockchip/master at 2b19b2f
>
>  drivers/clk/rockchip/clk_rv1108.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 2/7] rockchip: pinctrl: rv1108: access of-offset via dev_of_offset(...)

[Simon Glass]

Hi,

On 6 June 2017 at 07:42, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> After rebasing to u-boot-rockchip/master at 2b19b2f, buildman fails for
> rv1108 with:
>   ../drivers/pinctrl/rockchip/pinctrl_rv1108.c: In function 'rv1108_pinctrl_get_periph_id':
>   ../drivers/pinctrl/rockchip/pinctrl_rv1108.c:111:49: error: 'struct udevice' has no member named 'of_offset'
>     ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
>                                                      ^
>
> This change access the of-offset of periph via the dev_of_offset()
> helper-function to fix this issue and (hopefully) to ensure it doesn't
> recur if there's more changes to the DM subsystem.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
> ---
>
> Changes in v2:
> - (new patch) access of-offset of periph via dev_of_offset in the RV1108
>   pinctrl driver to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>
>  drivers/pinctrl/rockchip/pinctrl_rv1108.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/rockchip/pinctrl_rv1108.c b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
> index d98ec81..bdf3910 100644
> --- a/drivers/pinctrl/rockchip/pinctrl_rv1108.c
> +++ b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
> @@ -108,7 +108,7 @@ static int rv1108_pinctrl_get_periph_id(struct udevice *dev,
>         u32 cell[3];
>         int ret;
>
> -       ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
> +       ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(periph),
>                                    "interrupts", cell, ARRAY_SIZE(cell));

Could you use dev_read_u32_array() here instead? That is the new new
way :-) I added the dev_of_offset() as a transition mechanism but it
should not be used ideally.

>         if (ret < 0)
>                 return -EINVAL;
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH v2 3/7] rockchip: rk3328: don't implement usb_gadget_handle_interrupts twice

[Simon Glass]

On 6 June 2017 at 07:42, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> The usb_gadget_handle_interrupts()-function is already implemented by
> drivers/usb/gadget/dwc2_udc_otg.c, so we need to avoid defining it
> in the evb-rk3328.c board-specific file.
>
> This change fixes the following build error (from buildman):
>   drivers/usb/gadget/built-in.o: In function `usb_gadget_handle_interrupts':
>   build/../drivers/usb/gadget/dwc2_udc_otg.c:850: multiple definition of `usb_gadget_handle_interrupts'
>   board/rockchip/evb_rk3328/built-in.o:build/../board/rockchip/evb_rk3328/evb-rk3328.c:37: first defined here
>   make[1]: *** [u-boot] Error 1
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
> ---
>
> Changes in v2:
> - (new patch) fix a multiple definition of usb_gadget_handle_interrupts
>   for the RK3328 EVB in evb-rk3328.c to fix a buildman failure for
>   u-boot-rockchip/master at 2b19b2f
>
>  board/rockchip/evb_rk3328/evb-rk3328.c | 5 -----
>  1 file changed, 5 deletions(-)
>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Simon Glass]

Hi Philipp,

On 6 June 2017 at 07:42, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> The regs_otg field in uintptr_t of the platform data structure for
> dwc2-otg has thus far been an unsigned int, but will eventually be
> casted into a void*.
>
> This raises the following error with GCC 6.3 and buildman:
>   ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>   ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>     reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>           ^
>
> This changes regs_otg to a uintptr_t to ensure that it is large enough
> to hold any valid pointer (and fix the associated warning).
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
> ---
>
> Changes in v2:
> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>   dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>
>  include/usb/dwc2_udc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
> index 7324d8a..1a370e0 100644
> --- a/include/usb/dwc2_udc.h
> +++ b/include/usb/dwc2_udc.h
> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>         int             phy_of_node;
>         int             (*phy_control)(int on);
>         unsigned int    regs_phy;
> -       unsigned int    regs_otg;
> +       uintptr_t       regs_otg;

Can you use ulong instead?

>         unsigned int    usb_phy_ctrl;
>         unsigned int    usb_flags;
>         unsigned int    usb_gusbcfg;
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Dr. Philipp Tomsich]

Simon,

> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
> 
> Hi Philipp,
> 
> On 6 June 2017 at 07:42, Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>> The regs_otg field in uintptr_t of the platform data structure for
>> dwc2-otg has thus far been an unsigned int, but will eventually be
>> casted into a void*.
>> 
>> This raises the following error with GCC 6.3 and buildman:
>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>          ^
>> 
>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>> to hold any valid pointer (and fix the associated warning).
>> 
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> 
>> ---
>> 
>> Changes in v2:
>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>> 
>> include/usb/dwc2_udc.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>> index 7324d8a..1a370e0 100644
>> --- a/include/usb/dwc2_udc.h
>> +++ b/include/usb/dwc2_udc.h
>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>        int             phy_of_node;
>>        int             (*phy_control)(int on);
>>        unsigned int    regs_phy;
>> -       unsigned int    regs_otg;
>> +       uintptr_t       regs_otg;
> 
> Can you use ulong instead?

Sure, but can I first ask “why?”.
I may reopen an old discussion with this… if so, forgive my ignorance:

uintptr_t makes the most sense for this use case in the C99 (or later) type system,
as we want this field to hold an integer (i.e. the address from the physical memory
map for one of the register blocks) that will be casted into a pointer. 
The uintptr_t type will always the matching size in any and all programming models;
in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
in the context of U-Boot anyway).

What I always found odd, was that uintptr_t is optional according to ISO9899.
I would thus not have been surprised if there’s a concern for portability between
compilers behind this, but then again … U-Boot makes extensive use of GCC
extensions (such as inline assembly).

So I am apparently missing something here.

Cheers,
Philipp.

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Simon Glass]

Hi,

On 6 June 2017 at 17:59, Dr. Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> Simon,
>
>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Philipp,
>>
>> On 6 June 2017 at 07:42, Philipp Tomsich
>> <philipp.tomsich@theobroma-systems.com> wrote:
>>> The regs_otg field in uintptr_t of the platform data structure for
>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>> casted into a void*.
>>>
>>> This raises the following error with GCC 6.3 and buildman:
>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>          ^
>>>
>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>> to hold any valid pointer (and fix the associated warning).
>>>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>
>>> include/usb/dwc2_udc.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>>> index 7324d8a..1a370e0 100644
>>> --- a/include/usb/dwc2_udc.h
>>> +++ b/include/usb/dwc2_udc.h
>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>>        int             phy_of_node;
>>>        int             (*phy_control)(int on);
>>>        unsigned int    regs_phy;
>>> -       unsigned int    regs_otg;
>>> +       uintptr_t       regs_otg;
>>
>> Can you use ulong instead?
>
> Sure, but can I first ask “why?”.
> I may reopen an old discussion with this… if so, forgive my ignorance:
>
> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
> as we want this field to hold an integer (i.e. the address from the physical memory
> map for one of the register blocks) that will be casted into a pointer.
> The uintptr_t type will always the matching size in any and all programming models;
> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
> in the context of U-Boot anyway).
>
> What I always found odd, was that uintptr_t is optional according to ISO9899.
> I would thus not have been surprised if there’s a concern for portability between
> compilers behind this, but then again … U-Boot makes extensive use of GCC
> extensions (such as inline assembly).
>
> So I am apparently missing something here.

I don't know of any deep reason except that long is defined to be able
to hold an address, and ulong makes sense since an address is
generally considered unsigned.

U-Boot by convention uses ulong for addresses. You can see this all
around the code base so I am really just arguing in favour of
consistency (and I suppose ulong is easier to type!)

Regards,
Simon

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Marek Vasut]

On 06/07/2017 02:16 AM, Simon Glass wrote:
> Hi,
> 
> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>> Simon,
>>
>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>
>>> Hi Philipp,
>>>
>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>> casted into a void*.
>>>>
>>>> This raises the following error with GCC 6.3 and buildman:
>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>          ^
>>>>
>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>> to hold any valid pointer (and fix the associated warning).
>>>>
>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>
>>>> include/usb/dwc2_udc.h | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>>>> index 7324d8a..1a370e0 100644
>>>> --- a/include/usb/dwc2_udc.h
>>>> +++ b/include/usb/dwc2_udc.h
>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>>>        int             phy_of_node;
>>>>        int             (*phy_control)(int on);
>>>>        unsigned int    regs_phy;
>>>> -       unsigned int    regs_otg;
>>>> +       uintptr_t       regs_otg;
>>>
>>> Can you use ulong instead?
>>
>> Sure, but can I first ask “why?”.
>> I may reopen an old discussion with this… if so, forgive my ignorance:
>>
>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
>> as we want this field to hold an integer (i.e. the address from the physical memory
>> map for one of the register blocks) that will be casted into a pointer.
>> The uintptr_t type will always the matching size in any and all programming models;
>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
>> in the context of U-Boot anyway).
>>
>> What I always found odd, was that uintptr_t is optional according to ISO9899.
>> I would thus not have been surprised if there’s a concern for portability between
>> compilers behind this, but then again … U-Boot makes extensive use of GCC
>> extensions (such as inline assembly).
>>
>> So I am apparently missing something here.
> 
> I don't know of any deep reason except that long is defined to be able
> to hold an address, and ulong makes sense since an address is
> generally considered unsigned.
> 
> U-Boot by convention uses ulong for addresses.

I was under the impression that u-boot by convention uses uintptr_t for
addresses.

> You can see this all
> around the code base so I am really just arguing in favour of
> consistency (and I suppose ulong is easier to type!)

Then I guess half of the codebase is inconsistent.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 5/7] usb: dwc2-otg: fix warnings for ep->dma_buf for LP64 builds

[Marek Vasut]

On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote:
> Marek,
> 
>> On 06 Jun 2017, at 16:35, Marek Vasut <marex@denx.de> wrote:
>>
>> On 06/06/2017 03:42 PM, Philipp Tomsich wrote:
>>> For LP64 build w/ GCC 6.3, the handling of the ep->dma_buf variable
>>> when (truncating and) writing into the controller's registers is
>>> unsafe and triggers the following compiler warning (thus failing by
>>> buildman tests):
>>>  ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c: In function 'setdma_rx':
>>>  ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c:116:9: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
>>>    writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>>           ^
>>>
>>> This change fixes the warning and makes the code a bit more robust by
>>> doing the following:
>>> - we check whether ep->dma_buf can be safely truncated to 32 bits
>>>  (i.e. whether dma_buf is the same value when masked to 32 bits) and
>>>  emit an error message if that is not the case
>>> - we cast to a uintptr_t and let the writel macro truncate the
>>>  uintptr_t (potentially a 64bit value) to 32bits
>>>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - (new patch) fix warnings and add some robustness for the truncation
>>>  of ep->dma_buf in dwc2-otg for LP64 builds to fix a buildman failure
>>>  for u-boot-rockchip/master at 2b19b2f
>>>
>>> drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>> index 0d6d2fb..f5c926c 100644
>>> --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>> @@ -113,7 +113,11 @@ static int setdma_rx(struct dwc2_ep *ep, struct dwc2_request *req)
>>> 	invalidate_dcache_range((unsigned long) ep->dma_buf,
>>> 				(unsigned long) ep->dma_buf + ep->len);
>>>
>>> -	writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>> +	/* ensure that ep->dma_buf fits into 32 bits */
>>> +	if (((uintptr_t)ep->dma_buf & 0xffffffff) != (uintptr_t)ep->dma_buf)
>>> +		error("ep->dma_buf does not fit into a 32 bits");
>>
>> You print an error, but still continue with the function ? That'll
>> probably lead to a crash, you should rather abort. In fact, I suspect
>> you can use bounce-buffer to assure the dma buffer is in 32bit space.
> 
> This tries to stay close to the current behaviour (we have no hardware to test this,
> but I wanted to get it out of the way as it caused buildman-failures) and to provide
> diagnostics for whoever first encounters an issue here.
> 
> Note that I would only expect a DMA error, if someone really hit this issue ...

So this patch is just some hypothetical fix ? What triggered creation of
this patch ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/7] rockchip: clk: rv1108: rename dev_get_addr

[Andy Yan]

Hi Philipp:

2017-06-06 21:42 GMT+08:00 Philipp Tomsich <
philipp.tomsich@theobroma-systems.com>:

> After rebasing to u-boot-rockchip/master at 2b19b2f, buildman fails for
> rv1108 with:
>   ../drivers/clk/rockchip/clk_rv1108.c: In function 'rv1108_clk_probe':
>   ../drivers/clk/rockchip/clk_rv1108.c:191:22: warning: implicit
> declaration of function 'dev_get_addr' [-Wimplicit-function-declaration]
>     priv->cru = (struct rv1108_cru *)dev_get_addr(dev);
>
> This change tracks the dev_get_addr rename, which didn't make it into
> the rv1108 clk driver.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
> ---
>

    Tested-by: Andy Yan <andy.yan@rock-chips.com>

    Thanks.

>
> Changes in v2:
> - (new patch) rename dev_get_addr in the RV1108 clk-driver to fix a
>   buildman failure for u-boot-rockchip/master at 2b19b2f
>
>  drivers/clk/rockchip/clk_rv1108.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/rockchip/clk_rv1108.c b/drivers/clk/rockchip/clk_
> rv1108.c
> index 0946dab..0a3ba3b 100644
> --- a/drivers/clk/rockchip/clk_rv1108.c
> +++ b/drivers/clk/rockchip/clk_rv1108.c
> @@ -188,7 +188,7 @@ static int rv1108_clk_probe(struct udevice *dev)
>  {
>         struct rv1108_clk_priv *priv = dev_get_priv(dev);
>
> -       priv->cru = (struct rv1108_cru *)dev_get_addr(dev);
> +       priv->cru = (struct rv1108_cru *)devfdt_get_addr(dev);
>
>         rkclk_init(priv->cru);
>
> --
> 2.1.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>

[U-Boot] [PATCH v2 5/7] usb: dwc2-otg: fix warnings for ep->dma_buf for LP64 builds

[Dr. Philipp Tomsich]

> On 07 Jun 2017, at 08:28, Marek Vasut <marex@denx.de> wrote:
> 
> On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote:
>> 
>>> On 06 Jun 2017, at 16:35, Marek Vasut <marex@denx.de> wrote:
>>> 
>>> On 06/06/2017 03:42 PM, Philipp Tomsich wrote:
>>>> For LP64 build w/ GCC 6.3, the handling of the ep->dma_buf variable
>>>> when (truncating and) writing into the controller's registers is
>>>> unsafe and triggers the following compiler warning (thus failing by
>>>> buildman tests):
>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c: In function 'setdma_rx':
>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c:116:9: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
>>>>   writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>>>          ^
>>>> 
>>>> This change fixes the warning and makes the code a bit more robust by
>>>> doing the following:
>>>> - we check whether ep->dma_buf can be safely truncated to 32 bits
>>>> (i.e. whether dma_buf is the same value when masked to 32 bits) and
>>>> emit an error message if that is not the case
>>>> - we cast to a uintptr_t and let the writel macro truncate the
>>>> uintptr_t (potentially a 64bit value) to 32bits
>>>> 
>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>> 
>>>> ---
>>>> 
>>>> Changes in v2:
>>>> - (new patch) fix warnings and add some robustness for the truncation
>>>> of ep->dma_buf in dwc2-otg for LP64 builds to fix a buildman failure
>>>> for u-boot-rockchip/master at 2b19b2f
>>>> 
>>>> drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 6 +++++-
>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>> index 0d6d2fb..f5c926c 100644
>>>> --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>> @@ -113,7 +113,11 @@ static int setdma_rx(struct dwc2_ep *ep, struct dwc2_request *req)
>>>> 	invalidate_dcache_range((unsigned long) ep->dma_buf,
>>>> 				(unsigned long) ep->dma_buf + ep->len);
>>>> 
>>>> -	writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>>> +	/* ensure that ep->dma_buf fits into 32 bits */
>>>> +	if (((uintptr_t)ep->dma_buf & 0xffffffff) != (uintptr_t)ep->dma_buf)
>>>> +		error("ep->dma_buf does not fit into a 32 bits");
>>> 
>>> You print an error, but still continue with the function ? That'll
>>> probably lead to a crash, you should rather abort. In fact, I suspect
>>> you can use bounce-buffer to assure the dma buffer is in 32bit space.
>> 
>> This tries to stay close to the current behaviour (we have no hardware to test this,
>> but I wanted to get it out of the way as it caused buildman-failures) and to provide
>> diagnostics for whoever first encounters an issue here.
>> 
>> Note that I would only expect a DMA error, if someone really hit this issue ...
> 
> So this patch is just some hypothetical fix ? What triggered creation of
> this patch ?

Running buildman (w/ a GCC 6.3 configured as aarch-unknown-elf) against unrelated
patch-series (for a Rockchip device that has a different USB controller) fails due to the
warnings raised from this. Just casting those warnings away (without having an
“assertion-like” diagnostic printed) doesn’t sound like the best plan either, though. 

Details of the buildman-failure are in the above commit-message.

Regards,
Philipp.

[U-Boot] [PATCH v2 5/7] usb: dwc2-otg: fix warnings for ep->dma_buf for LP64 builds

[Marek Vasut]

On 06/07/2017 09:49 AM, Dr. Philipp Tomsich wrote:
>> On 07 Jun 2017, at 08:28, Marek Vasut <marex@denx.de
>> <mailto:marex@denx.de>> wrote:
>>
>> On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote:
>>>
>>>> On 06 Jun 2017, at 16:35, Marek Vasut <marex@denx.de
>>>> <mailto:marex@denx.de>> wrote:
>>>>
>>>> On 06/06/2017 03:42 PM, Philipp Tomsich wrote:
>>>>> For LP64 build w/ GCC 6.3, the handling of the ep->dma_buf variable
>>>>> when (truncating and) writing into the controller's registers is
>>>>> unsafe and triggers the following compiler warning (thus failing by
>>>>> buildman tests):
>>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c: In function 'setdma_rx':
>>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c:116:9: warning: cast
>>>>> from pointer to integer of different size [-Wpointer-to-int-cast]
>>>>>   writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>>>>          ^
>>>>>
>>>>> This change fixes the warning and makes the code a bit more robust by
>>>>> doing the following:
>>>>> - we check whether ep->dma_buf can be safely truncated to 32 bits
>>>>> (i.e. whether dma_buf is the same value when masked to 32 bits) and
>>>>> emit an error message if that is not the case
>>>>> - we cast to a uintptr_t and let the writel macro truncate the
>>>>> uintptr_t (potentially a 64bit value) to 32bits
>>>>>
>>>>> Signed-off-by: Philipp Tomsich
>>>>> <philipp.tomsich@theobroma-systems.com
>>>>> <mailto:philipp.tomsich@theobroma-systems.com>>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - (new patch) fix warnings and add some robustness for the truncation
>>>>> of ep->dma_buf in dwc2-otg for LP64 builds to fix a buildman failure
>>>>> for u-boot-rockchip/master at 2b19b2f
>>>>>
>>>>> drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 6 +++++-
>>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>> index 0d6d2fb..f5c926c 100644
>>>>> --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>> @@ -113,7 +113,11 @@ static int setdma_rx(struct dwc2_ep *ep,
>>>>> struct dwc2_request *req)
>>>>> invalidate_dcache_range((unsigned long) ep->dma_buf,
>>>>> (unsigned long) ep->dma_buf + ep->len);
>>>>>
>>>>> -writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>>>> +/* ensure that ep->dma_buf fits into 32 bits */
>>>>> +if (((uintptr_t)ep->dma_buf & 0xffffffff) != (uintptr_t)ep->dma_buf)
>>>>> +error("ep->dma_buf does not fit into a 32 bits");
>>>>
>>>> You print an error, but still continue with the function ? That'll
>>>> probably lead to a crash, you should rather abort. In fact, I suspect
>>>> you can use bounce-buffer to assure the dma buffer is in 32bit space.
>>>
>>> This tries to stay close to the current behaviour (we have no
>>> hardware to test this,
>>> but I wanted to get it out of the way as it caused buildman-failures)
>>> and to provide
>>> diagnostics for whoever first encounters an issue here.
>>>
>>> Note that I would only expect a DMA error, if someone really hit this
>>> issue ...
>>
>> So this patch is just some hypothetical fix ? What triggered creation of
>> this patch ?
> 
> Running buildman (w/ a GCC 6.3 configured as aarch-unknown-elf) against
> unrelated
> patch-series (for a Rockchip device that has a different USB controller)
> fails due to the
> warnings raised from this. Just casting those warnings away (without
> having an
> “assertion-like” diagnostic printed) doesn’t sound like the best plan
> either, though. 
> 
> Details of the buildman-failure are in the above commit-message.

Ah, hm. In that case, you should really implement the bounce buffer here
I think. Letting the transfer continue will lead to really weird
failures and/or memory corruption.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/7] rockchip: pinctrl: rv1108: access of-offset via dev_of_offset(...)

[Dr. Philipp Tomsich]

Simon,

> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
> 
> Hi,
> 
> On 6 June 2017 at 07:42, Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>> After rebasing to u-boot-rockchip/master at 2b19b2f, buildman fails for
>> rv1108 with:
>>  ../drivers/pinctrl/rockchip/pinctrl_rv1108.c: In function 'rv1108_pinctrl_get_periph_id':
>>  ../drivers/pinctrl/rockchip/pinctrl_rv1108.c:111:49: error: 'struct udevice' has no member named 'of_offset'
>>    ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
>>                                                     ^
>> 
>> This change access the of-offset of periph via the dev_of_offset()
>> helper-function to fix this issue and (hopefully) to ensure it doesn't
>> recur if there's more changes to the DM subsystem.
>> 
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> 
>> ---
>> 
>> Changes in v2:
>> - (new patch) access of-offset of periph via dev_of_offset in the RV1108
>>  pinctrl driver to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>> 
>> drivers/pinctrl/rockchip/pinctrl_rv1108.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/pinctrl/rockchip/pinctrl_rv1108.c b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
>> index d98ec81..bdf3910 100644
>> --- a/drivers/pinctrl/rockchip/pinctrl_rv1108.c
>> +++ b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
>> @@ -108,7 +108,7 @@ static int rv1108_pinctrl_get_periph_id(struct udevice *dev,
>>        u32 cell[3];
>>        int ret;
>> 
>> -       ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
>> +       ret =fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(periph),
>>                                   "interrupts", cell, ARRAY_SIZE(cell));
> 
> Could you use dev_read_u32_array() here instead? That is the new new
> way :-) I added the dev_of_offset() as a transition mechanism but it
> should not be used ideally.

A quick grep shows that only simple-bus.c uses the new way of doing things, yet.

Please use this one as-is and I’ll send another series (to be applied on top of this)
to rewrite to dev_read_u32_array() across the entire driver population.

Regards,
Philipp.

[U-Boot] [PATCH v2 5/7] usb: dwc2-otg: fix warnings for ep->dma_buf for LP64 builds

[Dr. Philipp Tomsich]

> On 07 Jun 2017, at 09:53, Marek Vasut <marex@denx.de> wrote:
> 
> On 06/07/2017 09:49 AM, Dr. Philipp Tomsich wrote:
>>> On 07 Jun 2017, at 08:28, Marek Vasut <marex@denx.de
>>> <mailto:marex@denx.de>> wrote:
>>> 
>>> On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote:
>>>> 
>>>>> On 06 Jun 2017, at 16:35, Marek Vasut <marex@denx.de
>>>>> <mailto:marex@denx.de>> wrote:
>>>>> 
>>>>> On 06/06/2017 03:42 PM, Philipp Tomsich wrote:
>>>>>> For LP64 build w/ GCC 6.3, the handling of the ep->dma_buf variable
>>>>>> when (truncating and) writing into the controller's registers is
>>>>>> unsafe and triggers the following compiler warning (thus failing by
>>>>>> buildman tests):
>>>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c: In function 'setdma_rx':
>>>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c:116:9: warning: cast
>>>>>> from pointer to integer of different size [-Wpointer-to-int-cast]
>>>>>>  writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>>>>>         ^
>>>>>> 
>>>>>> This change fixes the warning and makes the code a bit more robust by
>>>>>> doing the following:
>>>>>> - we check whether ep->dma_buf can be safely truncated to 32 bits
>>>>>> (i.e. whether dma_buf is the same value when masked to 32 bits) and
>>>>>> emit an error message if that is not the case
>>>>>> - we cast to a uintptr_t and let the writel macro truncate the
>>>>>> uintptr_t (potentially a 64bit value) to 32bits
>>>>>> 
>>>>>> Signed-off-by: Philipp Tomsich
>>>>>> <philipp.tomsich@theobroma-systems.com
>>>>>> <mailto:philipp.tomsich@theobroma-systems.com>>
>>>>>> 
>>>>>> ---
>>>>>> 
>>>>>> Changes in v2:
>>>>>> - (new patch) fix warnings and add some robustness for the truncation
>>>>>> of ep->dma_buf in dwc2-otg for LP64 builds to fix a buildman failure
>>>>>> for u-boot-rockchip/master at 2b19b2f
>>>>>> 
>>>>>> drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 6 +++++-
>>>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>> index 0d6d2fb..f5c926c 100644
>>>>>> --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>> @@ -113,7 +113,11 @@ static int setdma_rx(struct dwc2_ep *ep,
>>>>>> struct dwc2_request *req)
>>>>>> invalidate_dcache_range((unsigned long) ep->dma_buf,
>>>>>> (unsigned long) ep->dma_buf + ep->len);
>>>>>> 
>>>>>> -writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>>>>> +/* ensure that ep->dma_buf fits into 32 bits */
>>>>>> +if (((uintptr_t)ep->dma_buf & 0xffffffff) != (uintptr_t)ep->dma_buf)
>>>>>> +error("ep->dma_buf does not fit into a 32 bits");
>>>>> 
>>>>> You print an error, but still continue with the function ? That'll
>>>>> probably lead to a crash, you should rather abort. In fact, I suspect
>>>>> you can use bounce-buffer to assure the dma buffer is in 32bit space.
>>>> 
>>>> This tries to stay close to the current behaviour (we have no
>>>> hardware to test this,
>>>> but I wanted to get it out of the way as it caused buildman-failures)
>>>> and to provide
>>>> diagnostics for whoever first encounters an issue here.
>>>> 
>>>> Note that I would only expect a DMA error, if someone really hit this
>>>> issue ...
>>> 
>>> So this patch is just some hypothetical fix ? What triggered creation of
>>> this patch ?
>> 
>> Running buildman (w/ a GCC 6.3 configured as aarch-unknown-elf) against
>> unrelated
>> patch-series (for a Rockchip device that has a different USB controller)
>> fails due to the
>> warnings raised from this. Just casting those warnings away (without
>> having an
>> “assertion-like” diagnostic printed) doesn’t sound like the best plan
>> either, though. 
>> 
>> Details of the buildman-failure are in the above commit-message.
> 
> Ah, hm. In that case, you should really implement the bounce buffer here
> I think. Letting the transfer continue will lead to really weird
> failures and/or memory corruption.

Alright. I’ll put it to (the back of) my queue and resubmit.
Someone (with hardware) will need to test, once it’s submitted.

Cheers,
Philipp.

[U-Boot] [PATCH v2 5/7] usb: dwc2-otg: fix warnings for ep->dma_buf for LP64 builds

[Marek Vasut]

On 06/07/2017 10:03 AM, Dr. Philipp Tomsich wrote:
> 
>> On 07 Jun 2017, at 09:53, Marek Vasut <marex@denx.de> wrote:
>>
>> On 06/07/2017 09:49 AM, Dr. Philipp Tomsich wrote:
>>>> On 07 Jun 2017, at 08:28, Marek Vasut <marex@denx.de
>>>> <mailto:marex@denx.de>> wrote:
>>>>
>>>> On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote:
>>>>>
>>>>>> On 06 Jun 2017, at 16:35, Marek Vasut <marex@denx.de
>>>>>> <mailto:marex@denx.de>> wrote:
>>>>>>
>>>>>> On 06/06/2017 03:42 PM, Philipp Tomsich wrote:
>>>>>>> For LP64 build w/ GCC 6.3, the handling of the ep->dma_buf variable
>>>>>>> when (truncating and) writing into the controller's registers is
>>>>>>> unsafe and triggers the following compiler warning (thus failing by
>>>>>>> buildman tests):
>>>>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c: In function 'setdma_rx':
>>>>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c:116:9: warning: cast
>>>>>>> from pointer to integer of different size [-Wpointer-to-int-cast]
>>>>>>>  writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>>>>>>         ^
>>>>>>>
>>>>>>> This change fixes the warning and makes the code a bit more robust by
>>>>>>> doing the following:
>>>>>>> - we check whether ep->dma_buf can be safely truncated to 32 bits
>>>>>>> (i.e. whether dma_buf is the same value when masked to 32 bits) and
>>>>>>> emit an error message if that is not the case
>>>>>>> - we cast to a uintptr_t and let the writel macro truncate the
>>>>>>> uintptr_t (potentially a 64bit value) to 32bits
>>>>>>>
>>>>>>> Signed-off-by: Philipp Tomsich
>>>>>>> <philipp.tomsich@theobroma-systems.com
>>>>>>> <mailto:philipp.tomsich@theobroma-systems.com>>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>> - (new patch) fix warnings and add some robustness for the truncation
>>>>>>> of ep->dma_buf in dwc2-otg for LP64 builds to fix a buildman failure
>>>>>>> for u-boot-rockchip/master at 2b19b2f
>>>>>>>
>>>>>>> drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 6 +++++-
>>>>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>>> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>>> index 0d6d2fb..f5c926c 100644
>>>>>>> --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>>> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>>>>> @@ -113,7 +113,11 @@ static int setdma_rx(struct dwc2_ep *ep,
>>>>>>> struct dwc2_request *req)
>>>>>>> invalidate_dcache_range((unsigned long) ep->dma_buf,
>>>>>>> (unsigned long) ep->dma_buf + ep->len);
>>>>>>>
>>>>>>> -writel((unsigned int) ep->dma_buf, &reg->out_endp[ep_num].doepdma);
>>>>>>> +/* ensure that ep->dma_buf fits into 32 bits */
>>>>>>> +if (((uintptr_t)ep->dma_buf & 0xffffffff) != (uintptr_t)ep->dma_buf)
>>>>>>> +error("ep->dma_buf does not fit into a 32 bits");
>>>>>>
>>>>>> You print an error, but still continue with the function ? That'll
>>>>>> probably lead to a crash, you should rather abort. In fact, I suspect
>>>>>> you can use bounce-buffer to assure the dma buffer is in 32bit space.
>>>>>
>>>>> This tries to stay close to the current behaviour (we have no
>>>>> hardware to test this,
>>>>> but I wanted to get it out of the way as it caused buildman-failures)
>>>>> and to provide
>>>>> diagnostics for whoever first encounters an issue here.
>>>>>
>>>>> Note that I would only expect a DMA error, if someone really hit this
>>>>> issue ...
>>>>
>>>> So this patch is just some hypothetical fix ? What triggered creation of
>>>> this patch ?
>>>
>>> Running buildman (w/ a GCC 6.3 configured as aarch-unknown-elf) against
>>> unrelated
>>> patch-series (for a Rockchip device that has a different USB controller)
>>> fails due to the
>>> warnings raised from this. Just casting those warnings away (without
>>> having an
>>> “assertion-like” diagnostic printed) doesn’t sound like the best plan
>>> either, though. 
>>>
>>> Details of the buildman-failure are in the above commit-message.
>>
>> Ah, hm. In that case, you should really implement the bounce buffer here
>> I think. Letting the transfer continue will lead to really weird
>> failures and/or memory corruption.
> 
> Alright. I’ll put it to (the back of) my queue and resubmit.
> Someone (with hardware) will need to test, once it’s submitted.

That's fine, thanks.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Simon Glass]

+Tom for comment

Hi Marek,

On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
> On 06/07/2017 02:16 AM, Simon Glass wrote:
>> Hi,
>>
>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>> <philipp.tomsich@theobroma-systems.com> wrote:
>>> Simon,
>>>
>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>
>>>> Hi Philipp,
>>>>
>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>> casted into a void*.
>>>>>
>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>          ^
>>>>>
>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>
>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>
>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>>>>> index 7324d8a..1a370e0 100644
>>>>> --- a/include/usb/dwc2_udc.h
>>>>> +++ b/include/usb/dwc2_udc.h
>>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>>>>        int             phy_of_node;
>>>>>        int             (*phy_control)(int on);
>>>>>        unsigned int    regs_phy;
>>>>> -       unsigned int    regs_otg;
>>>>> +       uintptr_t       regs_otg;
>>>>
>>>> Can you use ulong instead?
>>>
>>> Sure, but can I first ask “why?”.
>>> I may reopen an old discussion with this… if so, forgive my ignorance:
>>>
>>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
>>> as we want this field to hold an integer (i.e. the address from the physical memory
>>> map for one of the register blocks) that will be casted into a pointer.
>>> The uintptr_t type will always the matching size in any and all programming models;
>>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
>>> in the context of U-Boot anyway).
>>>
>>> What I always found odd, was that uintptr_t is optional according to ISO9899.
>>> I would thus not have been surprised if there’s a concern for portability between
>>> compilers behind this, but then again … U-Boot makes extensive use of GCC
>>> extensions (such as inline assembly).
>>>
>>> So I am apparently missing something here.
>>
>> I don't know of any deep reason except that long is defined to be able
>> to hold an address, and ulong makes sense since an address is
>> generally considered unsigned.
>>
>> U-Boot by convention uses ulong for addresses.
>
> I was under the impression that u-boot by convention uses uintptr_t for
> addresses.
>
>> You can see this all
>> around the code base so I am really just arguing in favour of
>> consistency (and I suppose ulong is easier to type!)
>
> Then I guess half of the codebase is inconsistent.

Actually about 10%:

git grep uintptr_t |wc -l
395
git grep ulong |wc -l
4024

Clearly we use ulong all over the place for addresses - see image.c,
most commands, etc. If we are going to change then it should be a
deliberate decision and a tree-wide update. I'm happy enough with
ulong.

Tom, what do you what to do here?

Regards,
Simon

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Marek Vasut]

On 06/07/2017 02:38 PM, Simon Glass wrote:
> +Tom for comment
> 
> Hi Marek,
> 
> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>> Hi,
>>>
>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>> Simon,
>>>>
>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>
>>>>> Hi Philipp,
>>>>>
>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>> casted into a void*.
>>>>>>
>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>          ^
>>>>>>
>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>
>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>
>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>>>>>> index 7324d8a..1a370e0 100644
>>>>>> --- a/include/usb/dwc2_udc.h
>>>>>> +++ b/include/usb/dwc2_udc.h
>>>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>>>>>        int             phy_of_node;
>>>>>>        int             (*phy_control)(int on);
>>>>>>        unsigned int    regs_phy;
>>>>>> -       unsigned int    regs_otg;
>>>>>> +       uintptr_t       regs_otg;
>>>>>
>>>>> Can you use ulong instead?
>>>>
>>>> Sure, but can I first ask “why?”.
>>>> I may reopen an old discussion with this… if so, forgive my ignorance:
>>>>
>>>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
>>>> as we want this field to hold an integer (i.e. the address from the physical memory
>>>> map for one of the register blocks) that will be casted into a pointer.
>>>> The uintptr_t type will always the matching size in any and all programming models;
>>>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
>>>> in the context of U-Boot anyway).
>>>>
>>>> What I always found odd, was that uintptr_t is optional according to ISO9899.
>>>> I would thus not have been surprised if there’s a concern for portability between
>>>> compilers behind this, but then again … U-Boot makes extensive use of GCC
>>>> extensions (such as inline assembly).
>>>>
>>>> So I am apparently missing something here.
>>>
>>> I don't know of any deep reason except that long is defined to be able
>>> to hold an address, and ulong makes sense since an address is
>>> generally considered unsigned.
>>>
>>> U-Boot by convention uses ulong for addresses.
>>
>> I was under the impression that u-boot by convention uses uintptr_t for
>> addresses.
>>
>>> You can see this all
>>> around the code base so I am really just arguing in favour of
>>> consistency (and I suppose ulong is easier to type!)
>>
>> Then I guess half of the codebase is inconsistent.
> 
> Actually about 10%:
> 
> git grep uintptr_t |wc -l
> 395
> git grep ulong |wc -l
> 4024

I don't think this is a valid way to count it at all, since uintptr_t is
only used for casting pointer to number, while ulong is used for
arbitrary numbers.

> Clearly we use ulong all over the place for addresses - see image.c,
> most commands, etc.

But none of those use ulong as device address pointers .

> If we are going to change then it should be a
> deliberate decision and a tree-wide update. I'm happy enough with
> ulong.
> 
> Tom, what do you what to do here?
> 
> Regards,
> Simon
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Simon Glass]

Hi Marek,

On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
> On 06/07/2017 02:38 PM, Simon Glass wrote:
>> +Tom for comment
>>
>> Hi Marek,
>>
>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>>> Hi,
>>>>
>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>> Simon,
>>>>>
>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>>
>>>>>> Hi Philipp,
>>>>>>
>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>>> casted into a void*.
>>>>>>>
>>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>>          ^
>>>>>>>
>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>>
>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>>
>>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>>>>>>> index 7324d8a..1a370e0 100644
>>>>>>> --- a/include/usb/dwc2_udc.h
>>>>>>> +++ b/include/usb/dwc2_udc.h
>>>>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>>>>>>        int             phy_of_node;
>>>>>>>        int             (*phy_control)(int on);
>>>>>>>        unsigned int    regs_phy;
>>>>>>> -       unsigned int    regs_otg;
>>>>>>> +       uintptr_t       regs_otg;
>>>>>>
>>>>>> Can you use ulong instead?
>>>>>
>>>>> Sure, but can I first ask “why?”.
>>>>> I may reopen an old discussion with this… if so, forgive my ignorance:
>>>>>
>>>>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
>>>>> as we want this field to hold an integer (i.e. the address from the physical memory
>>>>> map for one of the register blocks) that will be casted into a pointer.
>>>>> The uintptr_t type will always the matching size in any and all programming models;
>>>>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
>>>>> in the context of U-Boot anyway).
>>>>>
>>>>> What I always found odd, was that uintptr_t is optional according to ISO9899.
>>>>> I would thus not have been surprised if there’s a concern for portability between
>>>>> compilers behind this, but then again … U-Boot makes extensive use of GCC
>>>>> extensions (such as inline assembly).
>>>>>
>>>>> So I am apparently missing something here.
>>>>
>>>> I don't know of any deep reason except that long is defined to be able
>>>> to hold an address, and ulong makes sense since an address is
>>>> generally considered unsigned.
>>>>
>>>> U-Boot by convention uses ulong for addresses.
>>>
>>> I was under the impression that u-boot by convention uses uintptr_t for
>>> addresses.
>>>
>>>> You can see this all
>>>> around the code base so I am really just arguing in favour of
>>>> consistency (and I suppose ulong is easier to type!)
>>>
>>> Then I guess half of the codebase is inconsistent.
>>
>> Actually about 10%:
>>
>> git grep uintptr_t |wc -l
>> 395
>> git grep ulong |wc -l
>> 4024
>
> I don't think this is a valid way to count it at all, since uintptr_t is
> only used for casting pointer to number, while ulong is used for
> arbitrary numbers.
>
>> Clearly we use ulong all over the place for addresses - see image.c,
>> most commands, etc.
>
> But none of those use ulong as device address pointers .

Is there a distinction between a device address pointer and some other
type of address?

>
>> If we are going to change then it should be a
>> deliberate decision and a tree-wide update. I'm happy enough with
>> ulong.
>>
>> Tom, what do you what to do here?
>>

Regards,
Simon

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Marek Vasut]

On 06/07/2017 02:53 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
>> On 06/07/2017 02:38 PM, Simon Glass wrote:
>>> +Tom for comment
>>>
>>> Hi Marek,
>>>
>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>>>> Hi,
>>>>>
>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>> Simon,
>>>>>>
>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>>>
>>>>>>> Hi Philipp,
>>>>>>>
>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>>>> casted into a void*.
>>>>>>>>
>>>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>>>          ^
>>>>>>>>
>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>>>
>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v2:
>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>>>
>>>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>>>>>>>> index 7324d8a..1a370e0 100644
>>>>>>>> --- a/include/usb/dwc2_udc.h
>>>>>>>> +++ b/include/usb/dwc2_udc.h
>>>>>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>>>>>>>        int             phy_of_node;
>>>>>>>>        int             (*phy_control)(int on);
>>>>>>>>        unsigned int    regs_phy;
>>>>>>>> -       unsigned int    regs_otg;
>>>>>>>> +       uintptr_t       regs_otg;
>>>>>>>
>>>>>>> Can you use ulong instead?
>>>>>>
>>>>>> Sure, but can I first ask “why?”.
>>>>>> I may reopen an old discussion with this… if so, forgive my ignorance:
>>>>>>
>>>>>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
>>>>>> as we want this field to hold an integer (i.e. the address from the physical memory
>>>>>> map for one of the register blocks) that will be casted into a pointer.
>>>>>> The uintptr_t type will always the matching size in any and all programming models;
>>>>>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
>>>>>> in the context of U-Boot anyway).
>>>>>>
>>>>>> What I always found odd, was that uintptr_t is optional according to ISO9899.
>>>>>> I would thus not have been surprised if there’s a concern for portability between
>>>>>> compilers behind this, but then again … U-Boot makes extensive use of GCC
>>>>>> extensions (such as inline assembly).
>>>>>>
>>>>>> So I am apparently missing something here.
>>>>>
>>>>> I don't know of any deep reason except that long is defined to be able
>>>>> to hold an address, and ulong makes sense since an address is
>>>>> generally considered unsigned.
>>>>>
>>>>> U-Boot by convention uses ulong for addresses.
>>>>
>>>> I was under the impression that u-boot by convention uses uintptr_t for
>>>> addresses.
>>>>
>>>>> You can see this all
>>>>> around the code base so I am really just arguing in favour of
>>>>> consistency (and I suppose ulong is easier to type!)
>>>>
>>>> Then I guess half of the codebase is inconsistent.
>>>
>>> Actually about 10%:
>>>
>>> git grep uintptr_t |wc -l
>>> 395
>>> git grep ulong |wc -l
>>> 4024
>>
>> I don't think this is a valid way to count it at all, since uintptr_t is
>> only used for casting pointer to number, while ulong is used for
>> arbitrary numbers.
>>
>>> Clearly we use ulong all over the place for addresses - see image.c,
>>> most commands, etc.
>>
>> But none of those use ulong as device address pointers .
> 
> Is there a distinction between a device address pointer and some other
> type of address?

ulong is not used only for addresses, which your metric doesn't account for.


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Simon Glass]

Hi Marek,

On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
> On 06/07/2017 02:53 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
>>>> +Tom for comment
>>>>
>>>> Hi Marek,
>>>>
>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>> Simon,
>>>>>>>
>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>>>>
>>>>>>>> Hi Philipp,
>>>>>>>>
>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>>>>> casted into a void*.
>>>>>>>>>
>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>>>>          ^
>>>>>>>>>
>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>>>>
>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Changes in v2:
>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>>>>
>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>>>>>>>>> index 7324d8a..1a370e0 100644
>>>>>>>>> --- a/include/usb/dwc2_udc.h
>>>>>>>>> +++ b/include/usb/dwc2_udc.h
>>>>>>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>>>>>>>>        int             phy_of_node;
>>>>>>>>>        int             (*phy_control)(int on);
>>>>>>>>>        unsigned int    regs_phy;
>>>>>>>>> -       unsigned int    regs_otg;
>>>>>>>>> +       uintptr_t       regs_otg;
>>>>>>>>
>>>>>>>> Can you use ulong instead?
>>>>>>>
>>>>>>> Sure, but can I first ask “why?”.
>>>>>>> I may reopen an old discussion with this… if so, forgive my ignorance:
>>>>>>>
>>>>>>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
>>>>>>> as we want this field to hold an integer (i.e. the address from the physical memory
>>>>>>> map for one of the register blocks) that will be casted into a pointer.
>>>>>>> The uintptr_t type will always the matching size in any and all programming models;
>>>>>>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
>>>>>>> in the context of U-Boot anyway).
>>>>>>>
>>>>>>> What I always found odd, was that uintptr_t is optional according to ISO9899.
>>>>>>> I would thus not have been surprised if there’s a concern for portability between
>>>>>>> compilers behind this, but then again … U-Boot makes extensive use of GCC
>>>>>>> extensions (such as inline assembly).
>>>>>>>
>>>>>>> So I am apparently missing something here.
>>>>>>
>>>>>> I don't know of any deep reason except that long is defined to be able
>>>>>> to hold an address, and ulong makes sense since an address is
>>>>>> generally considered unsigned.
>>>>>>
>>>>>> U-Boot by convention uses ulong for addresses.
>>>>>
>>>>> I was under the impression that u-boot by convention uses uintptr_t for
>>>>> addresses.
>>>>>
>>>>>> You can see this all
>>>>>> around the code base so I am really just arguing in favour of
>>>>>> consistency (and I suppose ulong is easier to type!)
>>>>>
>>>>> Then I guess half of the codebase is inconsistent.
>>>>
>>>> Actually about 10%:
>>>>
>>>> git grep uintptr_t |wc -l
>>>> 395
>>>> git grep ulong |wc -l
>>>> 4024
>>>
>>> I don't think this is a valid way to count it at all, since uintptr_t is
>>> only used for casting pointer to number, while ulong is used for
>>> arbitrary numbers.
>>>
>>>> Clearly we use ulong all over the place for addresses - see image.c,
>>>> most commands, etc.
>>>
>>> But none of those use ulong as device address pointers .
>>
>> Is there a distinction between a device address pointer and some other
>> type of address?
>
> ulong is not used only for addresses, which your metric doesn't account for.

OK, but if you look around you can see my point. See for example
cmd/mem.c which uses ulong everywhere. Image handling is the same -
see e.g. image.h struct image_info and struct bootm_headers. Are you
saying those are wrong, or that this case is different, or something
else?

Regards,
Simon

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Marek Vasut]

On 06/07/2017 03:28 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
>> On 06/07/2017 02:53 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
>>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
>>>>> +Tom for comment
>>>>>
>>>>> Hi Marek,
>>>>>
>>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>> Simon,
>>>>>>>>
>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>>>>>
>>>>>>>>> Hi Philipp,
>>>>>>>>>
>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>>>>>> casted into a void*.
>>>>>>>>>>
>>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>>>>>          ^
>>>>>>>>>>
>>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> Changes in v2:
>>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>>>>>
>>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>>>>>>>>>> index 7324d8a..1a370e0 100644
>>>>>>>>>> --- a/include/usb/dwc2_udc.h
>>>>>>>>>> +++ b/include/usb/dwc2_udc.h
>>>>>>>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>>>>>>>>>        int             phy_of_node;
>>>>>>>>>>        int             (*phy_control)(int on);
>>>>>>>>>>        unsigned int    regs_phy;
>>>>>>>>>> -       unsigned int    regs_otg;
>>>>>>>>>> +       uintptr_t       regs_otg;
>>>>>>>>>
>>>>>>>>> Can you use ulong instead?
>>>>>>>>
>>>>>>>> Sure, but can I first ask “why?”.
>>>>>>>> I may reopen an old discussion with this… if so, forgive my ignorance:
>>>>>>>>
>>>>>>>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
>>>>>>>> as we want this field to hold an integer (i.e. the address from the physical memory
>>>>>>>> map for one of the register blocks) that will be casted into a pointer.
>>>>>>>> The uintptr_t type will always the matching size in any and all programming models;
>>>>>>>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
>>>>>>>> in the context of U-Boot anyway).
>>>>>>>>
>>>>>>>> What I always found odd, was that uintptr_t is optional according to ISO9899.
>>>>>>>> I would thus not have been surprised if there’s a concern for portability between
>>>>>>>> compilers behind this, but then again … U-Boot makes extensive use of GCC
>>>>>>>> extensions (such as inline assembly).
>>>>>>>>
>>>>>>>> So I am apparently missing something here.
>>>>>>>
>>>>>>> I don't know of any deep reason except that long is defined to be able
>>>>>>> to hold an address, and ulong makes sense since an address is
>>>>>>> generally considered unsigned.
>>>>>>>
>>>>>>> U-Boot by convention uses ulong for addresses.
>>>>>>
>>>>>> I was under the impression that u-boot by convention uses uintptr_t for
>>>>>> addresses.
>>>>>>
>>>>>>> You can see this all
>>>>>>> around the code base so I am really just arguing in favour of
>>>>>>> consistency (and I suppose ulong is easier to type!)
>>>>>>
>>>>>> Then I guess half of the codebase is inconsistent.
>>>>>
>>>>> Actually about 10%:
>>>>>
>>>>> git grep uintptr_t |wc -l
>>>>> 395
>>>>> git grep ulong |wc -l
>>>>> 4024
>>>>
>>>> I don't think this is a valid way to count it at all, since uintptr_t is
>>>> only used for casting pointer to number, while ulong is used for
>>>> arbitrary numbers.
>>>>
>>>>> Clearly we use ulong all over the place for addresses - see image.c,
>>>>> most commands, etc.
>>>>
>>>> But none of those use ulong as device address pointers .
>>>
>>> Is there a distinction between a device address pointer and some other
>>> type of address?
>>
>> ulong is not used only for addresses, which your metric doesn't account for.
> 
> OK, but if you look around you can see my point. See for example
> cmd/mem.c which uses ulong everywhere. Image handling is the same -
> see e.g. image.h struct image_info and struct bootm_headers. Are you
> saying those are wrong, or that this case is different, or something
> else?

I guess we could convert them to uintptr_t , but I've mostly used
uintptr_t when converting void __iomem * to numbers written to HW
registers .

I also think being explicit about "hey, this is a pointer converted to a
number" does not hurt, so I like the uintptr_t better than ulong for
such converted pointers.

re cmd/mem.c , it's legacy code , the new code and/or code which used to
trigger compiler warnings on ie. arm64 was fixed up mostly to use the
uintptr_t recently.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Simon Glass]

Hi Marek,

On 7 June 2017 at 07:33, Marek Vasut <marex@denx.de> wrote:
> On 06/07/2017 03:28 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
>>> On 06/07/2017 02:53 PM, Simon Glass wrote:
>>>> Hi Marek,
>>>>
>>>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
>>>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
>>>>>> +Tom for comment
>>>>>>
>>>>>> Hi Marek,
>>>>>>
>>>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>>>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>> Simon,
>>>>>>>>>
>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Philipp,
>>>>>>>>>>
>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>>>>>>> casted into a void*.
>>>>>>>>>>>
>>>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>>>>>>          ^
>>>>>>>>>>>
>>>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Changes in v2:
>>>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>>>>>>
>>>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>>>>>>>>>>> index 7324d8a..1a370e0 100644
>>>>>>>>>>> --- a/include/usb/dwc2_udc.h
>>>>>>>>>>> +++ b/include/usb/dwc2_udc.h
>>>>>>>>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>>>>>>>>>>        int             phy_of_node;
>>>>>>>>>>>        int             (*phy_control)(int on);
>>>>>>>>>>>        unsigned int    regs_phy;
>>>>>>>>>>> -       unsigned int    regs_otg;
>>>>>>>>>>> +       uintptr_t       regs_otg;
>>>>>>>>>>
>>>>>>>>>> Can you use ulong instead?
>>>>>>>>>
>>>>>>>>> Sure, but can I first ask “why?”.
>>>>>>>>> I may reopen an old discussion with this… if so, forgive my ignorance:
>>>>>>>>>
>>>>>>>>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
>>>>>>>>> as we want this field to hold an integer (i.e. the address from the physical memory
>>>>>>>>> map for one of the register blocks) that will be casted into a pointer.
>>>>>>>>> The uintptr_t type will always the matching size in any and all programming models;
>>>>>>>>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
>>>>>>>>> in the context of U-Boot anyway).
>>>>>>>>>
>>>>>>>>> What I always found odd, was that uintptr_t is optional according to ISO9899.
>>>>>>>>> I would thus not have been surprised if there’s a concern for portability between
>>>>>>>>> compilers behind this, but then again … U-Boot makes extensive use of GCC
>>>>>>>>> extensions (such as inline assembly).
>>>>>>>>>
>>>>>>>>> So I am apparently missing something here.
>>>>>>>>
>>>>>>>> I don't know of any deep reason except that long is defined to be able
>>>>>>>> to hold an address, and ulong makes sense since an address is
>>>>>>>> generally considered unsigned.
>>>>>>>>
>>>>>>>> U-Boot by convention uses ulong for addresses.
>>>>>>>
>>>>>>> I was under the impression that u-boot by convention uses uintptr_t for
>>>>>>> addresses.
>>>>>>>
>>>>>>>> You can see this all
>>>>>>>> around the code base so I am really just arguing in favour of
>>>>>>>> consistency (and I suppose ulong is easier to type!)
>>>>>>>
>>>>>>> Then I guess half of the codebase is inconsistent.
>>>>>>
>>>>>> Actually about 10%:
>>>>>>
>>>>>> git grep uintptr_t |wc -l
>>>>>> 395
>>>>>> git grep ulong |wc -l
>>>>>> 4024
>>>>>
>>>>> I don't think this is a valid way to count it at all, since uintptr_t is
>>>>> only used for casting pointer to number, while ulong is used for
>>>>> arbitrary numbers.
>>>>>
>>>>>> Clearly we use ulong all over the place for addresses - see image.c,
>>>>>> most commands, etc.
>>>>>
>>>>> But none of those use ulong as device address pointers .
>>>>
>>>> Is there a distinction between a device address pointer and some other
>>>> type of address?
>>>
>>> ulong is not used only for addresses, which your metric doesn't account for.
>>
>> OK, but if you look around you can see my point. See for example
>> cmd/mem.c which uses ulong everywhere. Image handling is the same -
>> see e.g. image.h struct image_info and struct bootm_headers. Are you
>> saying those are wrong, or that this case is different, or something
>> else?
>
> I guess we could convert them to uintptr_t , but I've mostly used
> uintptr_t when converting void __iomem * to numbers written to HW
> registers .
>
> I also think being explicit about "hey, this is a pointer converted to a
> number" does not hurt, so I like the uintptr_t better than ulong for
> such converted pointers.
>
> re cmd/mem.c , it's legacy code , the new code and/or code which used to
> trigger compiler warnings on ie. arm64 was fixed up mostly to use the
> uintptr_t recently.

OK, how about updating https://www.denx.de/wiki/U-Boot/CodingStyle
with what we now want?

Regards,
Simon

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Marek Vasut]

On 06/07/2017 03:37 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 7 June 2017 at 07:33, Marek Vasut <marex@denx.de> wrote:
>> On 06/07/2017 03:28 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
>>>> On 06/07/2017 02:53 PM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
>>>>>>> +Tom for comment
>>>>>>>
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>>>>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>> Simon,
>>>>>>>>>>
>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi Philipp,
>>>>>>>>>>>
>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>>>>>>>> casted into a void*.
>>>>>>>>>>>>
>>>>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>>>>>>>          ^
>>>>>>>>>>>>
>>>>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>>>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>>>>>>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>>>>>>>
>>>>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>>>>>>>>>>>> index 7324d8a..1a370e0 100644
>>>>>>>>>>>> --- a/include/usb/dwc2_udc.h
>>>>>>>>>>>> +++ b/include/usb/dwc2_udc.h
>>>>>>>>>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>>>>>>>>>>>        int             phy_of_node;
>>>>>>>>>>>>        int             (*phy_control)(int on);
>>>>>>>>>>>>        unsigned int    regs_phy;
>>>>>>>>>>>> -       unsigned int    regs_otg;
>>>>>>>>>>>> +       uintptr_t       regs_otg;
>>>>>>>>>>>
>>>>>>>>>>> Can you use ulong instead?
>>>>>>>>>>
>>>>>>>>>> Sure, but can I first ask “why?”.
>>>>>>>>>> I may reopen an old discussion with this… if so, forgive my ignorance:
>>>>>>>>>>
>>>>>>>>>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
>>>>>>>>>> as we want this field to hold an integer (i.e. the address from the physical memory
>>>>>>>>>> map for one of the register blocks) that will be casted into a pointer.
>>>>>>>>>> The uintptr_t type will always the matching size in any and all programming models;
>>>>>>>>>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
>>>>>>>>>> in the context of U-Boot anyway).
>>>>>>>>>>
>>>>>>>>>> What I always found odd, was that uintptr_t is optional according to ISO9899.
>>>>>>>>>> I would thus not have been surprised if there’s a concern for portability between
>>>>>>>>>> compilers behind this, but then again … U-Boot makes extensive use of GCC
>>>>>>>>>> extensions (such as inline assembly).
>>>>>>>>>>
>>>>>>>>>> So I am apparently missing something here.
>>>>>>>>>
>>>>>>>>> I don't know of any deep reason except that long is defined to be able
>>>>>>>>> to hold an address, and ulong makes sense since an address is
>>>>>>>>> generally considered unsigned.
>>>>>>>>>
>>>>>>>>> U-Boot by convention uses ulong for addresses.
>>>>>>>>
>>>>>>>> I was under the impression that u-boot by convention uses uintptr_t for
>>>>>>>> addresses.
>>>>>>>>
>>>>>>>>> You can see this all
>>>>>>>>> around the code base so I am really just arguing in favour of
>>>>>>>>> consistency (and I suppose ulong is easier to type!)
>>>>>>>>
>>>>>>>> Then I guess half of the codebase is inconsistent.
>>>>>>>
>>>>>>> Actually about 10%:
>>>>>>>
>>>>>>> git grep uintptr_t |wc -l
>>>>>>> 395
>>>>>>> git grep ulong |wc -l
>>>>>>> 4024
>>>>>>
>>>>>> I don't think this is a valid way to count it at all, since uintptr_t is
>>>>>> only used for casting pointer to number, while ulong is used for
>>>>>> arbitrary numbers.
>>>>>>
>>>>>>> Clearly we use ulong all over the place for addresses - see image.c,
>>>>>>> most commands, etc.
>>>>>>
>>>>>> But none of those use ulong as device address pointers .
>>>>>
>>>>> Is there a distinction between a device address pointer and some other
>>>>> type of address?
>>>>
>>>> ulong is not used only for addresses, which your metric doesn't account for.
>>>
>>> OK, but if you look around you can see my point. See for example
>>> cmd/mem.c which uses ulong everywhere. Image handling is the same -
>>> see e.g. image.h struct image_info and struct bootm_headers. Are you
>>> saying those are wrong, or that this case is different, or something
>>> else?
>>
>> I guess we could convert them to uintptr_t , but I've mostly used
>> uintptr_t when converting void __iomem * to numbers written to HW
>> registers .
>>
>> I also think being explicit about "hey, this is a pointer converted to a
>> number" does not hurt, so I like the uintptr_t better than ulong for
>> such converted pointers.
>>
>> re cmd/mem.c , it's legacy code , the new code and/or code which used to
>> trigger compiler warnings on ie. arm64 was fixed up mostly to use the
>> uintptr_t recently.
> 
> OK, how about updating https://www.denx.de/wiki/U-Boot/CodingStyle
> with what we now want?

Works for me ... so what do we want now ? I'm not gonna do decision
affecting the entire project on my own, that never works.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/7] rockchip: pinctrl: rv1108: access of-offset via dev_of_offset(...)

[Dr. Philipp Tomsich]

Simon,

> On 07 Jun 2017, at 10:00, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
> 
>> On 06 Jun 2017, at 23:09, Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>> wrote:
>> 
>> On 6 June 2017 at 07:42, Philipp Tomsich
>> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>>> After rebasing to u-boot-rockchip/master at 2b19b2f, buildman fails for
>>> rv1108 with:
>>>  ../drivers/pinctrl/rockchip/pinctrl_rv1108.c: In function 'rv1108_pinctrl_get_periph_id':
>>>  ../drivers/pinctrl/rockchip/pinctrl_rv1108.c:111:49: error: 'struct udevice' has no member named 'of_offset'
>>>    ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
>>>                                                     ^
>>> 
>>> This change access the of-offset of periph via the dev_of_offset()
>>> helper-function to fix this issue and (hopefully) to ensure it doesn't
>>> recur if there's more changes to the DM subsystem.
>>> 
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>>
>>> 
>>> ---
>>> 
>>> Changes in v2:
>>> - (new patch) access of-offset of periph via dev_of_offset in the RV1108
>>>  pinctrl driver to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>> 
>>> drivers/pinctrl/rockchip/pinctrl_rv1108.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/pinctrl/rockchip/pinctrl_rv1108.c b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
>>> index d98ec81..bdf3910 100644
>>> --- a/drivers/pinctrl/rockchip/pinctrl_rv1108.c
>>> +++ b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
>>> @@ -108,7 +108,7 @@ static int rv1108_pinctrl_get_periph_id(struct udevice *dev,
>>>        u32 cell[3];
>>>        int ret;
>>> 
>>> -       ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
>>> +       ret =fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(periph),
>>>                                   "interrupts", cell, ARRAY_SIZE(cell));
>> 
>> Could you use dev_read_u32_array() here instead? That is the new new
>> way :-) I added the dev_of_offset() as a transition mechanism but it
>> should not be used ideally.
> 
> A quick grep shows that only simple-bus.c uses the new way of doing things, yet.
> 
> Please use this one as-is and I’ll send another series (to be applied on top of this)
> to rewrite to dev_read_u32_array() across the entire driver population.

I dropped a change-set for most fdt_get occurrences in the rockchip subarch (and
common drivers used by those boards).

However, there’s at least one fdtdev_get function that I didn’t find an equivalent for:
	fdtdec_get_int_array_count
In case you are wondering: this one is used RK3188 and RK3288 pinctrl-drivers…

Regards,
Philipp.

[U-Boot] [PATCH v2 2/7] rockchip: pinctrl: rv1108: access of-offset via dev_of_offset(...)

[Simon Glass]

Hi,

On 7 June 2017 at 10:51, Dr. Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> Simon,
>
> On 07 Jun 2017, at 10:00, Dr. Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>
> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>
> On 6 June 2017 at 07:42, Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>
> After rebasing to u-boot-rockchip/master at 2b19b2f, buildman fails for
> rv1108 with:
>  ../drivers/pinctrl/rockchip/pinctrl_rv1108.c: In function
> 'rv1108_pinctrl_get_periph_id':
>  ../drivers/pinctrl/rockchip/pinctrl_rv1108.c:111:49: error: 'struct
> udevice' has no member named 'of_offset'
>    ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
>                                                     ^
>
> This change access the of-offset of periph via the dev_of_offset()
> helper-function to fix this issue and (hopefully) to ensure it doesn't
> recur if there's more changes to the DM subsystem.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
> ---
>
> Changes in v2:
> - (new patch) access of-offset of periph via dev_of_offset in the RV1108
>  pinctrl driver to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>
> drivers/pinctrl/rockchip/pinctrl_rv1108.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/rockchip/pinctrl_rv1108.c
> b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
> index d98ec81..bdf3910 100644
> --- a/drivers/pinctrl/rockchip/pinctrl_rv1108.c
> +++ b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
> @@ -108,7 +108,7 @@ static int rv1108_pinctrl_get_periph_id(struct udevice
> *dev,
>        u32 cell[3];
>        int ret;
>
> -       ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
> +       ret =fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(periph),
>
>                                   "interrupts", cell, ARRAY_SIZE(cell));
>
>
> Could you use dev_read_u32_array() here instead? That is the new new
> way :-) I added the dev_of_offset() as a transition mechanism but it
> should not be used ideally.
>
>
> A quick grep shows that only simple-bus.c uses the new way of doing things,
> yet.
>
> Please use this one as-is and I’ll send another series (to be applied on top
> of this)
> to rewrite to dev_read_u32_array() across the entire driver population.
>
>
> I dropped a change-set for most fdt_get occurrences in the rockchip subarch
> (and
> common drivers used by those boards).

Great!

>
> However, there’s at least one fdtdev_get function that I didn’t find an
> equivalent for:
> fdtdec_get_int_array_count
> In case you are wondering: this one is used RK3188 and RK3288
> pinctrl-drivers…

OK - would you like to add one? It could be dev_read_u32_array() / ofnode_...

Regards,
Simon

[U-Boot] [PATCH v2 2/7] rockchip: pinctrl: rv1108: access of-offset via dev_of_offset(...)

[Dr. Philipp Tomsich]

> On 07 Jun 2017, at 18:56, Simon Glass <sjg@chromium.org> wrote:
> 
> Hi,
> 
> On 7 June 2017 at 10:51, Dr. Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>> Simon,
>> 
>> On 07 Jun 2017, at 10:00, Dr. Philipp Tomsich
>> <philipp.tomsich@theobroma-systems.com> wrote:
>> 
>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>> 
>> On 6 June 2017 at 07:42, Philipp Tomsich
>> <philipp.tomsich@theobroma-systems.com> wrote:
>> 
>> After rebasing to u-boot-rockchip/master at 2b19b2f, buildman fails for
>> rv1108 with:
>> ../drivers/pinctrl/rockchip/pinctrl_rv1108.c: In function
>> 'rv1108_pinctrl_get_periph_id':
>> ../drivers/pinctrl/rockchip/pinctrl_rv1108.c:111:49: error: 'struct
>> udevice' has no member named 'of_offset'
>>   ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
>>                                                    ^
>> 
>> This change access the of-offset of periph via the dev_of_offset()
>> helper-function to fix this issue and (hopefully) to ensure it doesn't
>> recur if there's more changes to the DM subsystem.
>> 
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> 
>> ---
>> 
>> Changes in v2:
>> - (new patch) access of-offset of periph via dev_of_offset in the RV1108
>> pinctrl driver to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>> 
>> drivers/pinctrl/rockchip/pinctrl_rv1108.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/pinctrl/rockchip/pinctrl_rv1108.c
>> b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
>> index d98ec81..bdf3910 100644
>> --- a/drivers/pinctrl/rockchip/pinctrl_rv1108.c
>> +++ b/drivers/pinctrl/rockchip/pinctrl_rv1108.c
>> @@ -108,7 +108,7 @@ static int rv1108_pinctrl_get_periph_id(struct udevice
>> *dev,
>>       u32 cell[3];
>>       int ret;
>> 
>> -       ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
>> +       ret =fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(periph),
>> 
>>                                  "interrupts", cell, ARRAY_SIZE(cell));
>> 
>> 
>> Could you use dev_read_u32_array() here instead? That is the new new
>> way :-) I added the dev_of_offset() as a transition mechanism but it
>> should not be used ideally.
>> 
>> 
>> A quick grep shows that only simple-bus.c uses the new way of doing things,
>> yet.
>> 
>> Please use this one as-is and I’ll send another series (to be applied on top
>> of this)
>> to rewrite to dev_read_u32_array() across the entire driver population.
>> 
>> 
>> I dropped a change-set for most fdt_get occurrences in the rockchip subarch
>> (and
>> common drivers used by those boards).
> 
> Great!
> 
>> 
>> However, there’s at least one fdtdev_get function that I didn’t find an
>> equivalent for:
>> fdtdec_get_int_array_count
>> In case you are wondering: this one is used RK3188 and RK3288
>> pinctrl-drivers…
> 
> OK - would you like to add one? It could be dev_read_u32_array() / ofnode_…

No problem, will do.
I just wanted to make sure that this wasn’t left out for a reason (e.g. to deprecate
it in the near future)…

Cheers,
Philipp.

[U-Boot] [PATCH v2 7/7] rockchip: dts: rk3399-puma: sync DTS with Linux tree

[sjg at google.com]

The Linux DTS for the RK3399-Q7 has moved with the times... resync
against it to ensure a consistent configuration.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v2: None

 arch/arm/dts/rk3399-puma.dts | 540 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 495 insertions(+), 45 deletions(-)

Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH v2 6/7] rockchip: dts: rk3399: enable HDMI output in the DTS

[sjg at google.com]

This commit enables HDMI output in the DTS by adding the necessary
nodes to vopl/vopb and by adding the HDMI node.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

---

Changes in v2:
- cherry-picked the DTS change adding the hdmi node for the rk3399.dtsi
  from the RK3399 HDMI enabledment series (already applied there)

 arch/arm/dts/rk3399.dtsi | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[sjg at google.com]

On 06/07/2017 03:37 PM, Simon Glass wrote:
> Hi Marek,
>
> On 7 June 2017 at 07:33, Marek Vasut <marex@denx.de> wrote:
>> On 06/07/2017 03:28 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
>>>> On 06/07/2017 02:53 PM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
>>>>>>> +Tom for comment
>>>>>>>
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>>>>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>> Simon,
>>>>>>>>>>
>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi Philipp,
>>>>>>>>>>>
>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>>>>>>>> casted into a void*.
>>>>>>>>>>>>
>>>>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>>>>>>>          ^
>>>>>>>>>>>>
>>>>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>>>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>>>>>>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>>>>>>>
>>>>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>>
Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH v2 3/7] rockchip: rk3328: don't implement usb_gadget_handle_interrupts twice

[sjg at google.com]

On 6 June 2017 at 07:42, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> The usb_gadget_handle_interrupts()-function is already implemented by
> drivers/usb/gadget/dwc2_udc_otg.c, so we need to avoid defining it
> in the evb-rk3328.c board-specific file.
>
> This change fixes the following build error (from buildman):
>   drivers/usb/gadget/built-in.o: In function `usb_gadget_handle_interrupts':
>   build/../drivers/usb/gadget/dwc2_udc_otg.c:850: multiple definition of `usb_gadget_handle_interrupts'
>   board/rockchip/evb_rk3328/built-in.o:build/../board/rockchip/evb_rk3328/evb-rk3328.c:37: first defined here
>   make[1]: *** [u-boot] Error 1
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
> ---
>
> Changes in v2:
> - (new patch) fix a multiple definition of usb_gadget_handle_interrupts
>   for the RK3328 EVB in evb-rk3328.c to fix a buildman failure for
>   u-boot-rockchip/master at 2b19b2f
>
>  board/rockchip/evb_rk3328/evb-rk3328.c | 5 -----
>  1 file changed, 5 deletions(-)
>

Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH v2 2/7] rockchip: pinctrl: rv1108: access of-offset via dev_of_offset(...)

[sjg at google.com]

> On 07 Jun 2017, at 18:56, Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On 7 June 2017 at 10:51, Dr. Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>> Simon,
>>
>> On 07 Jun 2017, at 10:00, Dr. Philipp Tomsich
>> <philipp.tomsich@theobroma-systems.com> wrote:
>>
>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>
>> On 6 June 2017 at 07:42, Philipp Tomsich
>> <philipp.tomsich@theobroma-systems.com> wrote:
>>
>> After rebasing to u-boot-rockchip/master at 2b19b2f, buildman fails for
>> rv1108 with:
>> ../drivers/pinctrl/rockchip/pinctrl_rv1108.c: In function
>> 'rv1108_pinctrl_get_periph_id':
>> ../drivers/pinctrl/rockchip/pinctrl_rv1108.c:111:49: error: 'struct
>> udevice' has no member named 'of_offset'
>>   ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
>>                                                    ^
>>
>> This change access the of-offset of periph via the dev_of_offset()
>> helper-function to fix this issue and (hopefully) to ensure it doesn't
>> recur if there's more changes to the DM subsystem.
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>
>> ---
>>
>> Changes in v2:
>> - (new patch) access of-offset of periph via dev_of_offset in the RV1108
>> pinctrl driver to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>
>> drivers/pinctrl/rockchip/pinctrl_rv1108.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH v2 1/7] rockchip: clk: rv1108: rename dev_get_addr

[sjg at google.com]

Hi Philipp:

2017-06-06 21:42 GMT+08:00 Philipp Tomsich <
philipp.tomsich@theobroma-systems.com>:

> After rebasing to u-boot-rockchip/master at 2b19b2f, buildman fails for
> rv1108 with:
>   ../drivers/clk/rockchip/clk_rv1108.c: In function 'rv1108_clk_probe':
>   ../drivers/clk/rockchip/clk_rv1108.c:191:22: warning: implicit
> declaration of function 'dev_get_addr' [-Wimplicit-function-declaration]
>     priv->cru = (struct rv1108_cru *)dev_get_addr(dev);
>
> This change tracks the dev_get_addr rename, which didn't make it into
> the rv1108 clk driver.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
> ---
>

    Tested-by: Andy Yan <andy.yan@rock-chips.com>

    Thanks.

>
> Changes in v2:
> - (new patch) rename dev_get_addr in the RV1108 clk-driver to fix a
>   buildman failure for u-boot-rockchip/master at 2b19b2f
>
>  drivers/clk/rockchip/clk_rv1108.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Marek Vasut]

On 06/08/2017 05:34 AM, sjg at google.com wrote:
> On 06/07/2017 03:37 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 7 June 2017 at 07:33, Marek Vasut <marex@denx.de> wrote:
>>> On 06/07/2017 03:28 PM, Simon Glass wrote:
>>>> Hi Marek,
>>>>
>>>> On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
>>>>> On 06/07/2017 02:53 PM, Simon Glass wrote:
>>>>>> Hi Marek,
>>>>>>
>>>>>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
>>>>>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
>>>>>>>> +Tom for comment
>>>>>>>>
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>>> Simon,
>>>>>>>>>>>
>>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Philipp,
>>>>>>>>>>>>
>>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>>>>>>>>> casted into a void*.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>>>>>>>>          ^
>>>>>>>>>>>>>
>>>>>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>>>>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>>>>>>>>
>>>>>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>>>
> Applied to u-boot-rockchip, thanks!
> 

This is clearly a USB patch ... why would it go through u-boot-rockchip?
But OK, yes, I see we have no structure in place and patches go through
whatever random tree these days.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Simon Glass]

Hi Marek,

On 8 June 2017 at 06:33, Marek Vasut <marex@denx.de> wrote:
> On 06/08/2017 05:34 AM, sjg at google.com wrote:
>> On 06/07/2017 03:37 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 7 June 2017 at 07:33, Marek Vasut <marex@denx.de> wrote:
>>>> On 06/07/2017 03:28 PM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 06/07/2017 02:53 PM, Simon Glass wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
>>>>>>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
>>>>>>>>> +Tom for comment
>>>>>>>>>
>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>>>> Simon,
>>>>>>>>>>>>
>>>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Philipp,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>>>>>>>>>> casted into a void*.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>>>>>>>>>          ^
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>>>>>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>>>>
>> Applied to u-boot-rockchip, thanks!
>>
>
> This is clearly a USB patch ... why would it go through u-boot-rockchip?
> But OK, yes, I see we have no structure in place and patches go through
> whatever random tree these days.

It is assigned to me in patchwork and is needed to fix a build
warning. It is tricky to deal with individual patches within a larger
series since there are often dependencies. I had the same issue with
video patches.

Don't we normally try to keep series together?

Regards,
Simon

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Marek Vasut]

On 06/08/2017 03:45 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 8 June 2017 at 06:33, Marek Vasut <marex@denx.de> wrote:
>> On 06/08/2017 05:34 AM, sjg at google.com wrote:
>>> On 06/07/2017 03:37 PM, Simon Glass wrote:
>>>> Hi Marek,
>>>>
>>>> On 7 June 2017 at 07:33, Marek Vasut <marex@denx.de> wrote:
>>>>> On 06/07/2017 03:28 PM, Simon Glass wrote:
>>>>>> Hi Marek,
>>>>>>
>>>>>> On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
>>>>>>> On 06/07/2017 02:53 PM, Simon Glass wrote:
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
>>>>>>>>>> +Tom for comment
>>>>>>>>>>
>>>>>>>>>> Hi Marek,
>>>>>>>>>>
>>>>>>>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>>>>> Simon,
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Philipp,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>>>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>>>>>>>>>>> casted into a void*.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>>>>>>>>>>          ^
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>>>>>>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>>>>>
>>> Applied to u-boot-rockchip, thanks!
>>>
>>
>> This is clearly a USB patch ... why would it go through u-boot-rockchip?
>> But OK, yes, I see we have no structure in place and patches go through
>> whatever random tree these days.
> 
> It is assigned to me in patchwork

I see, USB patch assigned not to USB maintainer ... hmmmm ...

> and is needed to fix a build
> warning. It is tricky to deal with individual patches within a larger
> series since there are often dependencies. I had the same issue with
> video patches.
> 
> Don't we normally try to keep series together?

Don't we normally at least try to get AB/RB from the maintainer before
applying patches this way ?

> Regards,
> Simon
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Simon Glass]

Hi Marek,

On 8 June 2017 at 07:48, Marek Vasut <marex@denx.de> wrote:
> On 06/08/2017 03:45 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 8 June 2017 at 06:33, Marek Vasut <marex@denx.de> wrote:
>>> On 06/08/2017 05:34 AM, sjg at google.com wrote:
>>>> On 06/07/2017 03:37 PM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 7 June 2017 at 07:33, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 06/07/2017 03:28 PM, Simon Glass wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
>>>>>>>> On 06/07/2017 02:53 PM, Simon Glass wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
>>>>>>>>>>> +Tom for comment
>>>>>>>>>>>
>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>
>>>>>>>>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>>>>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>>>>>> Simon,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Philipp,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>>>>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>>>>>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>>>>>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>>>>>>>>>>>>> casted into a void*.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>>>>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>>>>>>>>>>>>          ^
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>>>>>>>>>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>>>>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>>>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>>>>>>
>>>> Applied to u-boot-rockchip, thanks!
>>>>
>>>
>>> This is clearly a USB patch ... why would it go through u-boot-rockchip?
>>> But OK, yes, I see we have no structure in place and patches go through
>>> whatever random tree these days.
>>
>> It is assigned to me in patchwork
>
> I see, USB patch assigned not to USB maintainer ... hmmmm ...

That is pretty typical if it is part of a series. It's just too hard
to coordinate multiple maintainers picking up bits of a series.

patch 1 add board feature
patch 2 add usb feature
patch 3 enable usb on board

Patch 3 cannot be applied until both 1 and 2 are in mainline, meaning
that we end up with this sequence:

patch 1 applied by board maintainer, send pull request
patch 2 applied by USB, send pull request
patch 3 applied by board maintainer

which is very slow and the feature cannot be tested until the end. I
guess you know that already, but acking a patch is helpful as it
allows them to stay together.

>
>> and is needed to fix a build
>> warning. It is tricky to deal with individual patches within a larger
>> series since there are often dependencies. I had the same issue with
>> video patches.
>>
>> Don't we normally try to keep series together?
>
> Don't we normally at least try to get AB/RB from the maintainer before
> applying patches this way ?

Yes that is best. I took your 'applied to' as an ack ;-)

Regards,
Simon

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Tom Rini]

On Wed, Jun 07, 2017 at 03:40:30PM +0200, Marek Vasut wrote:
> On 06/07/2017 03:37 PM, Simon Glass wrote:
> > Hi Marek,
> > 
> > On 7 June 2017 at 07:33, Marek Vasut <marex@denx.de> wrote:
> >> On 06/07/2017 03:28 PM, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
> >>>> On 06/07/2017 02:53 PM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
> >>>>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
> >>>>>>> +Tom for comment
> >>>>>>>
> >>>>>>> Hi Marek,
> >>>>>>>
> >>>>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
> >>>>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
> >>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
> >>>>>>>>>> Simon,
> >>>>>>>>>>
> >>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> Hi Philipp,
> >>>>>>>>>>>
> >>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
> >>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
> >>>>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
> >>>>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
> >>>>>>>>>>>> casted into a void*.
> >>>>>>>>>>>>
> >>>>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
> >>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
> >>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> >>>>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
> >>>>>>>>>>>>          ^
> >>>>>>>>>>>>
> >>>>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
> >>>>>>>>>>>> to hold any valid pointer (and fix the associated warning).
> >>>>>>>>>>>>
> >>>>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> >>>>>>>>>>>>
> >>>>>>>>>>>> ---
> >>>>>>>>>>>>
> >>>>>>>>>>>> Changes in v2:
> >>>>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
> >>>>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
> >>>>>>>>>>>>
> >>>>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
> >>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>>>>
> >>>>>>>>>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
> >>>>>>>>>>>> index 7324d8a..1a370e0 100644
> >>>>>>>>>>>> --- a/include/usb/dwc2_udc.h
> >>>>>>>>>>>> +++ b/include/usb/dwc2_udc.h
> >>>>>>>>>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
> >>>>>>>>>>>>        int             phy_of_node;
> >>>>>>>>>>>>        int             (*phy_control)(int on);
> >>>>>>>>>>>>        unsigned int    regs_phy;
> >>>>>>>>>>>> -       unsigned int    regs_otg;
> >>>>>>>>>>>> +       uintptr_t       regs_otg;
> >>>>>>>>>>>
> >>>>>>>>>>> Can you use ulong instead?
> >>>>>>>>>>
> >>>>>>>>>> Sure, but can I first ask “why?”.
> >>>>>>>>>> I may reopen an old discussion with this… if so, forgive my ignorance:
> >>>>>>>>>>
> >>>>>>>>>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
> >>>>>>>>>> as we want this field to hold an integer (i.e. the address from the physical memory
> >>>>>>>>>> map for one of the register blocks) that will be casted into a pointer.
> >>>>>>>>>> The uintptr_t type will always the matching size in any and all programming models;
> >>>>>>>>>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
> >>>>>>>>>> in the context of U-Boot anyway).
> >>>>>>>>>>
> >>>>>>>>>> What I always found odd, was that uintptr_t is optional according to ISO9899.
> >>>>>>>>>> I would thus not have been surprised if there’s a concern for portability between
> >>>>>>>>>> compilers behind this, but then again … U-Boot makes extensive use of GCC
> >>>>>>>>>> extensions (such as inline assembly).
> >>>>>>>>>>
> >>>>>>>>>> So I am apparently missing something here.
> >>>>>>>>>
> >>>>>>>>> I don't know of any deep reason except that long is defined to be able
> >>>>>>>>> to hold an address, and ulong makes sense since an address is
> >>>>>>>>> generally considered unsigned.
> >>>>>>>>>
> >>>>>>>>> U-Boot by convention uses ulong for addresses.
> >>>>>>>>
> >>>>>>>> I was under the impression that u-boot by convention uses uintptr_t for
> >>>>>>>> addresses.
> >>>>>>>>
> >>>>>>>>> You can see this all
> >>>>>>>>> around the code base so I am really just arguing in favour of
> >>>>>>>>> consistency (and I suppose ulong is easier to type!)
> >>>>>>>>
> >>>>>>>> Then I guess half of the codebase is inconsistent.
> >>>>>>>
> >>>>>>> Actually about 10%:
> >>>>>>>
> >>>>>>> git grep uintptr_t |wc -l
> >>>>>>> 395
> >>>>>>> git grep ulong |wc -l
> >>>>>>> 4024
> >>>>>>
> >>>>>> I don't think this is a valid way to count it at all, since uintptr_t is
> >>>>>> only used for casting pointer to number, while ulong is used for
> >>>>>> arbitrary numbers.
> >>>>>>
> >>>>>>> Clearly we use ulong all over the place for addresses - see image.c,
> >>>>>>> most commands, etc.
> >>>>>>
> >>>>>> But none of those use ulong as device address pointers .
> >>>>>
> >>>>> Is there a distinction between a device address pointer and some other
> >>>>> type of address?
> >>>>
> >>>> ulong is not used only for addresses, which your metric doesn't account for.
> >>>
> >>> OK, but if you look around you can see my point. See for example
> >>> cmd/mem.c which uses ulong everywhere. Image handling is the same -
> >>> see e.g. image.h struct image_info and struct bootm_headers. Are you
> >>> saying those are wrong, or that this case is different, or something
> >>> else?
> >>
> >> I guess we could convert them to uintptr_t , but I've mostly used
> >> uintptr_t when converting void __iomem * to numbers written to HW
> >> registers .
> >>
> >> I also think being explicit about "hey, this is a pointer converted to a
> >> number" does not hurt, so I like the uintptr_t better than ulong for
> >> such converted pointers.
> >>
> >> re cmd/mem.c , it's legacy code , the new code and/or code which used to
> >> trigger compiler warnings on ie. arm64 was fixed up mostly to use the
> >> uintptr_t recently.
> > 
> > OK, how about updating https://www.denx.de/wiki/U-Boot/CodingStyle
> > with what we now want?
> 
> Works for me ... so what do we want now ? I'm not gonna do decision
> affecting the entire project on my own, that never works.

No, but you do have a clear and concise way of explaining technical
requirements, so I would appreciate your wording on what to add here, if
nothing else.  Thanks!

And yes, I'm chiming in now, to say that I do like using uintptr_t.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170608/7ada9702/attachment.sig>

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Tom Rini]

On Thu, Jun 08, 2017 at 07:59:30AM -0600, Simon Glass wrote:
> Hi Marek,
> 
> On 8 June 2017 at 07:48, Marek Vasut <marex@denx.de> wrote:
> > On 06/08/2017 03:45 PM, Simon Glass wrote:
> >> Hi Marek,
> >>
> >> On 8 June 2017 at 06:33, Marek Vasut <marex@denx.de> wrote:
> >>> On 06/08/2017 05:34 AM, sjg at google.com wrote:
> >>>> On 06/07/2017 03:37 PM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On 7 June 2017 at 07:33, Marek Vasut <marex@denx.de> wrote:
> >>>>>> On 06/07/2017 03:28 PM, Simon Glass wrote:
> >>>>>>> Hi Marek,
> >>>>>>>
> >>>>>>> On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
> >>>>>>>> On 06/07/2017 02:53 PM, Simon Glass wrote:
> >>>>>>>>> Hi Marek,
> >>>>>>>>>
> >>>>>>>>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
> >>>>>>>>>>> +Tom for comment
> >>>>>>>>>>>
> >>>>>>>>>>> Hi Marek,
> >>>>>>>>>>>
> >>>>>>>>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
> >>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
> >>>>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
> >>>>>>>>>>>>>> Simon,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Hi Philipp,
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
> >>>>>>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
> >>>>>>>>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
> >>>>>>>>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
> >>>>>>>>>>>>>>>> casted into a void*.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
> >>>>>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
> >>>>>>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> >>>>>>>>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
> >>>>>>>>>>>>>>>>          ^
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
> >>>>>>>>>>>>>>>> to hold any valid pointer (and fix the associated warning).
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Changes in v2:
> >>>>>>>>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
> >>>>>>>>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
> >>>>>>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>>>>>>>>
> >>>> Applied to u-boot-rockchip, thanks!
> >>>>
> >>>
> >>> This is clearly a USB patch ... why would it go through u-boot-rockchip?
> >>> But OK, yes, I see we have no structure in place and patches go through
> >>> whatever random tree these days.
> >>
> >> It is assigned to me in patchwork
> >
> > I see, USB patch assigned not to USB maintainer ... hmmmm ...
> 
> That is pretty typical if it is part of a series. It's just too hard
> to coordinate multiple maintainers picking up bits of a series.
> 
> patch 1 add board feature
> patch 2 add usb feature
> patch 3 enable usb on board
> 
> Patch 3 cannot be applied until both 1 and 2 are in mainline, meaning
> that we end up with this sequence:
> 
> patch 1 applied by board maintainer, send pull request
> patch 2 applied by USB, send pull request
> patch 3 applied by board maintainer
> 
> which is very slow and the feature cannot be tested until the end. I
> guess you know that already, but acking a patch is helpful as it
> allows them to stay together.

Generally speaking, and with the AB/RB of other relevant maintainers, I
endorse the idea that a logical series of reasonable size should not be
broken up into N smaller series to go in via N subtrees.  That said...

> >> and is needed to fix a build
> >> warning. It is tricky to deal with individual patches within a larger
> >> series since there are often dependencies. I had the same issue with
> >> video patches.
> >>
> >> Don't we normally try to keep series together?
> >
> > Don't we normally at least try to get AB/RB from the maintainer before
> > applying patches this way ?
> 
> Yes that is best. I took your 'applied to' as an ack ;-)

If someone wants to grab patches via their own subtree, as Marek does,
and in this case already had, it should go via that tree.  Do I fail to
get this right every time?  Yes.  Should I be better about this? Yes.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170608/5fce8230/attachment.sig>

[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Simon Glass]

Hi,

On 8 June 2017 at 08:16, Tom Rini <trini@konsulko.com> wrote:
> On Wed, Jun 07, 2017 at 03:40:30PM +0200, Marek Vasut wrote:
>> On 06/07/2017 03:37 PM, Simon Glass wrote:
>> > Hi Marek,
>> >
>> > On 7 June 2017 at 07:33, Marek Vasut <marex@denx.de> wrote:
>> >> On 06/07/2017 03:28 PM, Simon Glass wrote:
>> >>> Hi Marek,
>> >>>
>> >>> On 7 June 2017 at 06:55, Marek Vasut <marex@denx.de> wrote:
>> >>>> On 06/07/2017 02:53 PM, Simon Glass wrote:
>> >>>>> Hi Marek,
>> >>>>>
>> >>>>> On 7 June 2017 at 06:41, Marek Vasut <marex@denx.de> wrote:
>> >>>>>> On 06/07/2017 02:38 PM, Simon Glass wrote:
>> >>>>>>> +Tom for comment
>> >>>>>>>
>> >>>>>>> Hi Marek,
>> >>>>>>>
>> >>>>>>> On 7 June 2017 at 00:27, Marek Vasut <marex@denx.de> wrote:
>> >>>>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote:
>> >>>>>>>>> Hi,
>> >>>>>>>>>
>> >>>>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>> >>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>> >>>>>>>>>> Simon,
>> >>>>>>>>>>
>> >>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote:
>> >>>>>>>>>>>
>> >>>>>>>>>>> Hi Philipp,
>> >>>>>>>>>>>
>> >>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>> >>>>>>>>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>> >>>>>>>>>>>> The regs_otg field in uintptr_t of the platform data structure for
>> >>>>>>>>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>> >>>>>>>>>>>> casted into a void*.
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> This raises the following error with GCC 6.3 and buildman:
>> >>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>> >>>>>>>>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>> >>>>>>>>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>> >>>>>>>>>>>>          ^
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>> >>>>>>>>>>>> to hold any valid pointer (and fix the associated warning).
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> ---
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> Changes in v2:
>> >>>>>>>>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>> >>>>>>>>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> include/usb/dwc2_udc.h | 2 +-
>> >>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>> >>>>>>>>>>>> index 7324d8a..1a370e0 100644
>> >>>>>>>>>>>> --- a/include/usb/dwc2_udc.h
>> >>>>>>>>>>>> +++ b/include/usb/dwc2_udc.h
>> >>>>>>>>>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>> >>>>>>>>>>>>        int             phy_of_node;
>> >>>>>>>>>>>>        int             (*phy_control)(int on);
>> >>>>>>>>>>>>        unsigned int    regs_phy;
>> >>>>>>>>>>>> -       unsigned int    regs_otg;
>> >>>>>>>>>>>> +       uintptr_t       regs_otg;
>> >>>>>>>>>>>
>> >>>>>>>>>>> Can you use ulong instead?
>> >>>>>>>>>>
>> >>>>>>>>>> Sure, but can I first ask “why?”.
>> >>>>>>>>>> I may reopen an old discussion with this… if so, forgive my ignorance:
>> >>>>>>>>>>
>> >>>>>>>>>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
>> >>>>>>>>>> as we want this field to hold an integer (i.e. the address from the physical memory
>> >>>>>>>>>> map for one of the register blocks) that will be casted into a pointer.
>> >>>>>>>>>> The uintptr_t type will always the matching size in any and all programming models;
>> >>>>>>>>>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
>> >>>>>>>>>> in the context of U-Boot anyway).
>> >>>>>>>>>>
>> >>>>>>>>>> What I always found odd, was that uintptr_t is optional according to ISO9899.
>> >>>>>>>>>> I would thus not have been surprised if there’s a concern for portability between
>> >>>>>>>>>> compilers behind this, but then again … U-Boot makes extensive use of GCC
>> >>>>>>>>>> extensions (such as inline assembly).
>> >>>>>>>>>>
>> >>>>>>>>>> So I am apparently missing something here.
>> >>>>>>>>>
>> >>>>>>>>> I don't know of any deep reason except that long is defined to be able
>> >>>>>>>>> to hold an address, and ulong makes sense since an address is
>> >>>>>>>>> generally considered unsigned.
>> >>>>>>>>>
>> >>>>>>>>> U-Boot by convention uses ulong for addresses.
>> >>>>>>>>
>> >>>>>>>> I was under the impression that u-boot by convention uses uintptr_t for
>> >>>>>>>> addresses.
>> >>>>>>>>
>> >>>>>>>>> You can see this all
>> >>>>>>>>> around the code base so I am really just arguing in favour of
>> >>>>>>>>> consistency (and I suppose ulong is easier to type!)
>> >>>>>>>>
>> >>>>>>>> Then I guess half of the codebase is inconsistent.
>> >>>>>>>
>> >>>>>>> Actually about 10%:
>> >>>>>>>
>> >>>>>>> git grep uintptr_t |wc -l
>> >>>>>>> 395
>> >>>>>>> git grep ulong |wc -l
>> >>>>>>> 4024
>> >>>>>>
>> >>>>>> I don't think this is a valid way to count it at all, since uintptr_t is
>> >>>>>> only used for casting pointer to number, while ulong is used for
>> >>>>>> arbitrary numbers.
>> >>>>>>
>> >>>>>>> Clearly we use ulong all over the place for addresses - see image.c,
>> >>>>>>> most commands, etc.
>> >>>>>>
>> >>>>>> But none of those use ulong as device address pointers .
>> >>>>>
>> >>>>> Is there a distinction between a device address pointer and some other
>> >>>>> type of address?
>> >>>>
>> >>>> ulong is not used only for addresses, which your metric doesn't account for.
>> >>>
>> >>> OK, but if you look around you can see my point. See for example
>> >>> cmd/mem.c which uses ulong everywhere. Image handling is the same -
>> >>> see e.g. image.h struct image_info and struct bootm_headers. Are you
>> >>> saying those are wrong, or that this case is different, or something
>> >>> else?
>> >>
>> >> I guess we could convert them to uintptr_t , but I've mostly used
>> >> uintptr_t when converting void __iomem * to numbers written to HW
>> >> registers .
>> >>
>> >> I also think being explicit about "hey, this is a pointer converted to a
>> >> number" does not hurt, so I like the uintptr_t better than ulong for
>> >> such converted pointers.
>> >>
>> >> re cmd/mem.c , it's legacy code , the new code and/or code which used to
>> >> trigger compiler warnings on ie. arm64 was fixed up mostly to use the
>> >> uintptr_t recently.
>> >
>> > OK, how about updating https://www.denx.de/wiki/U-Boot/CodingStyle
>> > with what we now want?
>>
>> Works for me ... so what do we want now ? I'm not gonna do decision
>> affecting the entire project on my own, that never works.
>
> No, but you do have a clear and concise way of explaining technical
> requirements, so I would appreciate your wording on what to add here, if
> nothing else.  Thanks!
>
> And yes, I'm chiming in now, to say that I do like using uintptr_t.

Yes that's right. Marek's explanation was convincing to me, even
though I find uintptr_t confusing to read and longer to type. So Marek
I think it is worth putting on the coding style page. That way when it
comes up in code reviews we can point to it.

Regards,
Simon