This patch series fixes iomux routes on rk3308 and adds support for
pin controller found on rk3308b. According to rk3308b TRM, this pinctrl
much the same as rk3308's, but with additional iomux routes and 3bit
iomuxes selected via gpio##_sel_src_ctrl registers.
Downstream kernel [1] managed this SoC's with rk3308b_soc_data_init,
wich picked configuration based on cpuid. Upstream pinctrl patches
droped soc init function.
The function rk3308b_soc_sel_src_init sets up gpio##_sel_src_ctrl
registers, making SoC to use 3bit iomuxes over some 2bit old ones.
These patches have been tested on Radxa's ROCK Pi S, one based on rk3308
and the other on rk3308b (from the latest batches). For the new boards
fixes broken spi1 clk.
Similar effort [2] was made several years ago, but without keeping base
rk3308 SoC pinctrl support.
[1] https://github.com/radxa/kernel/blob/stable-4.4-rockpis/drivers/pinctrl/pinctrl-rockchip.c#L4388
[2] https://lore.kernel.org/linux-rockchip/[email protected]/
Dmitry Yashin (3):
pinctrl: rockchip: update rk3308 iomux routes
dt-bindings: pinctrl: rockchip: add rk3308b
pinctrl: rockchip: add rk3308b SoC support
.../bindings/pinctrl/rockchip,pinctrl.yaml | 1 +
drivers/pinctrl/pinctrl-rockchip.c | 187 ++++++++++++++++++
drivers/pinctrl/pinctrl-rockchip.h | 1 +
3 files changed, 189 insertions(+)
base-commit: ed30a4a51bb196781c8058073ea720133a65596f
--
2.39.2
Some of the rk3308 iomux routes in rk3308_mux_route_data belong to
the rk3308b SoC. Remove them and correct i2c3 routes.
Fixes: 7825aeb7b208 ("pinctrl: rockchip: add rk3308 SoC support")
Signed-off-by: Dmitry Yashin <[email protected]>
---
drivers/pinctrl/pinctrl-rockchip.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 3bedf36a0019..cc647db76927 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -870,9 +870,8 @@ static struct rockchip_mux_route_data rk3308_mux_route_data[] = {
RK_MUXROUTE_SAME(0, RK_PC3, 1, 0x314, BIT(16 + 0) | BIT(0)), /* rtc_clk */
RK_MUXROUTE_SAME(1, RK_PC6, 2, 0x314, BIT(16 + 2) | BIT(16 + 3)), /* uart2_rxm0 */
RK_MUXROUTE_SAME(4, RK_PD2, 2, 0x314, BIT(16 + 2) | BIT(16 + 3) | BIT(2)), /* uart2_rxm1 */
- RK_MUXROUTE_SAME(0, RK_PB7, 2, 0x608, BIT(16 + 8) | BIT(16 + 9)), /* i2c3_sdam0 */
- RK_MUXROUTE_SAME(3, RK_PB4, 2, 0x608, BIT(16 + 8) | BIT(16 + 9) | BIT(8)), /* i2c3_sdam1 */
- RK_MUXROUTE_SAME(2, RK_PA0, 3, 0x608, BIT(16 + 8) | BIT(16 + 9) | BIT(9)), /* i2c3_sdam2 */
+ RK_MUXROUTE_SAME(0, RK_PB7, 2, 0x314, BIT(16 + 4)), /* i2c3_sdam0 */
+ RK_MUXROUTE_SAME(3, RK_PB4, 2, 0x314, BIT(16 + 4) | BIT(4)), /* i2c3_sdam1 */
RK_MUXROUTE_SAME(1, RK_PA3, 2, 0x308, BIT(16 + 3)), /* i2s-8ch-1-sclktxm0 */
RK_MUXROUTE_SAME(1, RK_PA4, 2, 0x308, BIT(16 + 3)), /* i2s-8ch-1-sclkrxm0 */
RK_MUXROUTE_SAME(1, RK_PB5, 2, 0x308, BIT(16 + 3) | BIT(3)), /* i2s-8ch-1-sclktxm1 */
@@ -881,18 +880,6 @@ static struct rockchip_mux_route_data rk3308_mux_route_data[] = {
RK_MUXROUTE_SAME(1, RK_PB6, 4, 0x308, BIT(16 + 12) | BIT(16 + 13) | BIT(12)), /* pdm-clkm1 */
RK_MUXROUTE_SAME(2, RK_PA6, 2, 0x308, BIT(16 + 12) | BIT(16 + 13) | BIT(13)), /* pdm-clkm2 */
RK_MUXROUTE_SAME(2, RK_PA4, 3, 0x600, BIT(16 + 2) | BIT(2)), /* pdm-clkm-m2 */
- RK_MUXROUTE_SAME(3, RK_PB2, 3, 0x314, BIT(16 + 9)), /* spi1_miso */
- RK_MUXROUTE_SAME(2, RK_PA4, 2, 0x314, BIT(16 + 9) | BIT(9)), /* spi1_miso_m1 */
- RK_MUXROUTE_SAME(0, RK_PB3, 3, 0x314, BIT(16 + 10) | BIT(16 + 11)), /* owire_m0 */
- RK_MUXROUTE_SAME(1, RK_PC6, 7, 0x314, BIT(16 + 10) | BIT(16 + 11) | BIT(10)), /* owire_m1 */
- RK_MUXROUTE_SAME(2, RK_PA2, 5, 0x314, BIT(16 + 10) | BIT(16 + 11) | BIT(11)), /* owire_m2 */
- RK_MUXROUTE_SAME(0, RK_PB3, 2, 0x314, BIT(16 + 12) | BIT(16 + 13)), /* can_rxd_m0 */
- RK_MUXROUTE_SAME(1, RK_PC6, 5, 0x314, BIT(16 + 12) | BIT(16 + 13) | BIT(12)), /* can_rxd_m1 */
- RK_MUXROUTE_SAME(2, RK_PA2, 4, 0x314, BIT(16 + 12) | BIT(16 + 13) | BIT(13)), /* can_rxd_m2 */
- RK_MUXROUTE_SAME(1, RK_PC4, 3, 0x314, BIT(16 + 14)), /* mac_rxd0_m0 */
- RK_MUXROUTE_SAME(4, RK_PA2, 2, 0x314, BIT(16 + 14) | BIT(14)), /* mac_rxd0_m1 */
- RK_MUXROUTE_SAME(3, RK_PB4, 4, 0x314, BIT(16 + 15)), /* uart3_rx */
- RK_MUXROUTE_SAME(0, RK_PC1, 3, 0x314, BIT(16 + 15) | BIT(15)), /* uart3_rx_m1 */
};
static struct rockchip_mux_route_data rk3328_mux_route_data[] = {
--
2.39.2
Add compatible string for rk3308b pin controller.
Signed-off-by: Dmitry Yashin <[email protected]>
---
Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
index 20e806dce1ec..1b38b496cc8b 100644
--- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
@@ -41,6 +41,7 @@ properties:
- rockchip,rk3228-pinctrl
- rockchip,rk3288-pinctrl
- rockchip,rk3308-pinctrl
+ - rockchip,rk3308b-pinctrl
- rockchip,rk3328-pinctrl
- rockchip,rk3368-pinctrl
- rockchip,rk3399-pinctrl
--
2.39.2
Add pinctrl support for rk3308b. This pin controller much the same as
rk3308's, but with additional iomux routes and 3bit iomuxes selected
via gpio##_sel_src_ctrl registers. Set them up in the function
rk3308b_soc_sel_src_init to use new 3bit iomuxes over some 2bit old ones.
Fixes: 1f3e25a06883 ("pinctrl: rockchip: fix RK3308 pinmux bits")
Signed-off-by: Dmitry Yashin <[email protected]>
---
drivers/pinctrl/pinctrl-rockchip.c | 200 +++++++++++++++++++++++++++++
drivers/pinctrl/pinctrl-rockchip.h | 1 +
2 files changed, 201 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index cc647db76927..15d2045f929e 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -632,6 +632,115 @@ static struct rockchip_mux_recalced_data rk3308_mux_recalced_data[] = {
},
};
+static struct rockchip_mux_recalced_data rk3308b_mux_recalced_data[] = {
+ {
+ /* gpio1b6_sel */
+ .num = 1,
+ .pin = 14,
+ .reg = 0x28,
+ .bit = 12,
+ .mask = 0xf
+ }, {
+ /* gpio1b7_sel */
+ .num = 1,
+ .pin = 15,
+ .reg = 0x2c,
+ .bit = 0,
+ .mask = 0x3
+ }, {
+ /* gpio1c2_sel */
+ .num = 1,
+ .pin = 18,
+ .reg = 0x30,
+ .bit = 4,
+ .mask = 0xf
+ }, {
+ /* gpio1c3_sel */
+ .num = 1,
+ .pin = 19,
+ .reg = 0x30,
+ .bit = 8,
+ .mask = 0xf
+ }, {
+ /* gpio1c4_sel */
+ .num = 1,
+ .pin = 20,
+ .reg = 0x30,
+ .bit = 12,
+ .mask = 0xf
+ }, {
+ /* gpio1c5_sel */
+ .num = 1,
+ .pin = 21,
+ .reg = 0x34,
+ .bit = 0,
+ .mask = 0xf
+ }, {
+ /* gpio1c6_sel */
+ .num = 1,
+ .pin = 22,
+ .reg = 0x34,
+ .bit = 4,
+ .mask = 0xf
+ }, {
+ /* gpio1c7_sel */
+ .num = 1,
+ .pin = 23,
+ .reg = 0x34,
+ .bit = 8,
+ .mask = 0xf
+ }, {
+ /* gpio2a2_sel_plus */
+ .num = 2,
+ .pin = 2,
+ .reg = 0x608,
+ .bit = 0,
+ .mask = 0x7
+ }, {
+ /* gpio2a3_sel_plus */
+ .num = 2,
+ .pin = 3,
+ .reg = 0x608,
+ .bit = 4,
+ .mask = 0x7
+ }, {
+ /* gpio2c0_sel_plus */
+ .num = 2,
+ .pin = 16,
+ .reg = 0x610,
+ .bit = 8,
+ .mask = 0x7
+ }, {
+ /* gpio3b2_sel_plus */
+ .num = 3,
+ .pin = 10,
+ .reg = 0x610,
+ .bit = 0,
+ .mask = 0x7
+ }, {
+ /* gpio3b3_sel_plus */
+ .num = 3,
+ .pin = 11,
+ .reg = 0x610,
+ .bit = 4,
+ .mask = 0x7
+ }, {
+ /* gpio3b4_sel */
+ .num = 3,
+ .pin = 12,
+ .reg = 0x68,
+ .bit = 8,
+ .mask = 0xf
+ }, {
+ /* gpio3b5_sel */
+ .num = 3,
+ .pin = 13,
+ .reg = 0x68,
+ .bit = 12,
+ .mask = 0xf
+ },
+};
+
static struct rockchip_mux_recalced_data rk3328_mux_recalced_data[] = {
{
.num = 2,
@@ -882,6 +991,35 @@ static struct rockchip_mux_route_data rk3308_mux_route_data[] = {
RK_MUXROUTE_SAME(2, RK_PA4, 3, 0x600, BIT(16 + 2) | BIT(2)), /* pdm-clkm-m2 */
};
+static struct rockchip_mux_route_data rk3308b_mux_route_data[] = {
+ RK_MUXROUTE_SAME(0, RK_PC3, 1, 0x314, BIT(16 + 0) | BIT(0)), /* rtc_clk */
+ RK_MUXROUTE_SAME(1, RK_PC6, 2, 0x314, BIT(16 + 2) | BIT(16 + 3)), /* uart2_rxm0 */
+ RK_MUXROUTE_SAME(4, RK_PD2, 2, 0x314, BIT(16 + 2) | BIT(16 + 3) | BIT(2)), /* uart2_rxm1 */
+ RK_MUXROUTE_SAME(0, RK_PB7, 2, 0x608, BIT(16 + 8) | BIT(16 + 9)), /* i2c3_sdam0 */
+ RK_MUXROUTE_SAME(3, RK_PB4, 2, 0x608, BIT(16 + 8) | BIT(16 + 9) | BIT(8)), /* i2c3_sdam1 */
+ RK_MUXROUTE_SAME(2, RK_PA0, 3, 0x608, BIT(16 + 8) | BIT(16 + 9) | BIT(9)), /* i2c3_sdam2 */
+ RK_MUXROUTE_SAME(1, RK_PA3, 2, 0x308, BIT(16 + 3)), /* i2s-8ch-1-sclktxm0 */
+ RK_MUXROUTE_SAME(1, RK_PA4, 2, 0x308, BIT(16 + 3)), /* i2s-8ch-1-sclkrxm0 */
+ RK_MUXROUTE_SAME(1, RK_PB5, 2, 0x308, BIT(16 + 3) | BIT(3)), /* i2s-8ch-1-sclktxm1 */
+ RK_MUXROUTE_SAME(1, RK_PB6, 2, 0x308, BIT(16 + 3) | BIT(3)), /* i2s-8ch-1-sclkrxm1 */
+ RK_MUXROUTE_SAME(1, RK_PA4, 3, 0x308, BIT(16 + 12) | BIT(16 + 13)), /* pdm-clkm0 */
+ RK_MUXROUTE_SAME(1, RK_PB6, 4, 0x308, BIT(16 + 12) | BIT(16 + 13) | BIT(12)), /* pdm-clkm1 */
+ RK_MUXROUTE_SAME(2, RK_PA6, 2, 0x308, BIT(16 + 12) | BIT(16 + 13) | BIT(13)), /* pdm-clkm2 */
+ RK_MUXROUTE_SAME(2, RK_PA4, 3, 0x600, BIT(16 + 2) | BIT(2)), /* pdm-clkm-m2 */
+ RK_MUXROUTE_SAME(3, RK_PB2, 3, 0x314, BIT(16 + 9)), /* spi1_miso */
+ RK_MUXROUTE_SAME(2, RK_PA4, 2, 0x314, BIT(16 + 9) | BIT(9)), /* spi1_miso_m1 */
+ RK_MUXROUTE_SAME(0, RK_PB3, 3, 0x314, BIT(16 + 10) | BIT(16 + 11)), /* owire_m0 */
+ RK_MUXROUTE_SAME(1, RK_PC6, 7, 0x314, BIT(16 + 10) | BIT(16 + 11) | BIT(10)), /* owire_m1 */
+ RK_MUXROUTE_SAME(2, RK_PA2, 5, 0x314, BIT(16 + 10) | BIT(16 + 11) | BIT(11)), /* owire_m2 */
+ RK_MUXROUTE_SAME(0, RK_PB3, 2, 0x314, BIT(16 + 12) | BIT(16 + 13)), /* can_rxd_m0 */
+ RK_MUXROUTE_SAME(1, RK_PC6, 5, 0x314, BIT(16 + 12) | BIT(16 + 13) | BIT(12)), /* can_rxd_m1 */
+ RK_MUXROUTE_SAME(2, RK_PA2, 4, 0x314, BIT(16 + 12) | BIT(16 + 13) | BIT(13)), /* can_rxd_m2 */
+ RK_MUXROUTE_SAME(1, RK_PC4, 3, 0x314, BIT(16 + 14)), /* mac_rxd0_m0 */
+ RK_MUXROUTE_SAME(4, RK_PA2, 2, 0x314, BIT(16 + 14) | BIT(14)), /* mac_rxd0_m1 */
+ RK_MUXROUTE_SAME(3, RK_PB4, 4, 0x314, BIT(16 + 15)), /* uart3_rx */
+ RK_MUXROUTE_SAME(0, RK_PC1, 3, 0x314, BIT(16 + 15) | BIT(15)), /* uart3_rx_m1 */
+};
+
static struct rockchip_mux_route_data rk3328_mux_route_data[] = {
RK_MUXROUTE_SAME(1, RK_PA1, 2, 0x50, BIT(16) | BIT(16 + 1)), /* uart2dbg_rxm0 */
RK_MUXROUTE_SAME(2, RK_PA1, 1, 0x50, BIT(16) | BIT(16 + 1) | BIT(0)), /* uart2dbg_rxm1 */
@@ -2420,6 +2558,7 @@ static int rockchip_get_pull(struct rockchip_pin_bank *bank, int pin_num)
case RK3188:
case RK3288:
case RK3308:
+ case RK3308B:
case RK3368:
case RK3399:
case RK3568:
@@ -2478,6 +2617,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
case RK3188:
case RK3288:
case RK3308:
+ case RK3308B:
case RK3368:
case RK3399:
case RK3568:
@@ -2740,6 +2880,7 @@ static bool rockchip_pinconf_pull_valid(struct rockchip_pin_ctrl *ctrl,
case RK3188:
case RK3288:
case RK3308:
+ case RK3308B:
case RK3368:
case RK3399:
case RK3568:
@@ -3338,6 +3479,42 @@ static int __maybe_unused rockchip_pinctrl_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(rockchip_pinctrl_dev_pm_ops, rockchip_pinctrl_suspend,
rockchip_pinctrl_resume);
+#define RK3308B_GRF_SOC_CON13 0x608
+#define RK3308B_GRF_SOC_CON15 0x610
+
+/* RK3308B_GRF_SOC_CON13 */
+#define RK3308B_GRF_I2C3_IOFUNC_SRC_CTRL (BIT(16 + 10) | BIT(10))
+#define RK3308B_GRF_GPIO2A3_SEL_SRC_CTRL (BIT(16 + 7) | BIT(7))
+#define RK3308B_GRF_GPIO2A2_SEL_SRC_CTRL (BIT(16 + 3) | BIT(3))
+
+/* RK3308B_GRF_SOC_CON15 */
+#define RK3308B_GRF_GPIO2C0_SEL_SRC_CTRL (BIT(16 + 11) | BIT(11))
+#define RK3308B_GRF_GPIO3B3_SEL_SRC_CTRL (BIT(16 + 7) | BIT(7))
+#define RK3308B_GRF_GPIO3B2_SEL_SRC_CTRL (BIT(16 + 3) | BIT(3))
+
+/*
+ * RK3308B has 3bit gpio##_sel_plus iomuxes over some 2bit old ones.
+ * Put them in use by initializing gpio##_sel_src_ctrl registers.
+ */
+static int rk3308b_soc_sel_src_init(struct rockchip_pinctrl *info)
+{
+ int ret;
+
+ ret = regmap_write(info->regmap_base, RK3308B_GRF_SOC_CON13,
+ RK3308B_GRF_I2C3_IOFUNC_SRC_CTRL |
+ RK3308B_GRF_GPIO2A3_SEL_SRC_CTRL |
+ RK3308B_GRF_GPIO2A2_SEL_SRC_CTRL);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(info->regmap_base, RK3308B_GRF_SOC_CON15,
+ RK3308B_GRF_GPIO2C0_SEL_SRC_CTRL |
+ RK3308B_GRF_GPIO3B3_SEL_SRC_CTRL |
+ RK3308B_GRF_GPIO3B2_SEL_SRC_CTRL);
+
+ return ret;
+};
+
static int rockchip_pinctrl_probe(struct platform_device *pdev)
{
struct rockchip_pinctrl *info;
@@ -3403,6 +3580,12 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
return PTR_ERR(info->regmap_pmu);
}
+ if (ctrl->type == RK3308B) {
+ ret = rk3308b_soc_sel_src_init(info);
+ if (ret)
+ return ret;
+ }
+
ret = rockchip_pinctrl_register(pdev, info);
if (ret)
return ret;
@@ -3746,6 +3929,21 @@ static struct rockchip_pin_ctrl rk3308_pin_ctrl = {
.schmitt_calc_reg = rk3308_calc_schmitt_reg_and_bit,
};
+static struct rockchip_pin_ctrl rk3308b_pin_ctrl = {
+ .pin_banks = rk3308_pin_banks,
+ .nr_banks = ARRAY_SIZE(rk3308_pin_banks),
+ .label = "RK3308b-GPIO",
+ .type = RK3308B,
+ .grf_mux_offset = 0x0,
+ .iomux_recalced = rk3308b_mux_recalced_data,
+ .niomux_recalced = ARRAY_SIZE(rk3308b_mux_recalced_data),
+ .iomux_routes = rk3308b_mux_route_data,
+ .niomux_routes = ARRAY_SIZE(rk3308b_mux_route_data),
+ .pull_calc_reg = rk3308_calc_pull_reg_and_bit,
+ .drv_calc_reg = rk3308_calc_drv_reg_and_bit,
+ .schmitt_calc_reg = rk3308_calc_schmitt_reg_and_bit,
+};
+
static struct rockchip_pin_bank rk3328_pin_banks[] = {
PIN_BANK_IOMUX_FLAGS(0, 32, "gpio0", 0, 0, 0, 0),
PIN_BANK_IOMUX_FLAGS(1, 32, "gpio1", 0, 0, 0, 0),
@@ -3952,6 +4150,8 @@ static const struct of_device_id rockchip_pinctrl_dt_match[] = {
.data = &rk3288_pin_ctrl },
{ .compatible = "rockchip,rk3308-pinctrl",
.data = &rk3308_pin_ctrl },
+ { .compatible = "rockchip,rk3308b-pinctrl",
+ .data = &rk3308b_pin_ctrl },
{ .compatible = "rockchip,rk3328-pinctrl",
.data = &rk3328_pin_ctrl },
{ .compatible = "rockchip,rk3368-pinctrl",
diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
index 4759f336941e..3af5b1bd626b 100644
--- a/drivers/pinctrl/pinctrl-rockchip.h
+++ b/drivers/pinctrl/pinctrl-rockchip.h
@@ -193,6 +193,7 @@ enum rockchip_pinctrl_type {
RK3188,
RK3288,
RK3308,
+ RK3308B,
RK3368,
RK3399,
RK3568,
--
2.39.2
On Wed, May 15, 2024 at 05:16:33PM +0500, Dmitry Yashin wrote:
> Add compatible string for rk3308b pin controller.
>
> Signed-off-by: Dmitry Yashin <[email protected]>
Acked-by: Conor Dooley <[email protected]>
Cheers,
Conor.
Hello Dmitry,
On Wed, 15 May 2024 17:16:34 +0500
Dmitry Yashin <[email protected]> wrote:
> Add pinctrl support for rk3308b. This pin controller much the same as
> rk3308's, but with additional iomux routes and 3bit iomuxes selected
> via gpio##_sel_src_ctrl registers. Set them up in the function
> rk3308b_soc_sel_src_init to use new 3bit iomuxes over some 2bit old ones.
>
> Fixes: 1f3e25a06883 ("pinctrl: rockchip: fix RK3308 pinmux bits")
> Signed-off-by: Dmitry Yashin <[email protected]>
Thanks for the effort! I have one high-level remark, see below.
Otherwise at a superficial look it looks good.
> @@ -3952,6 +4150,8 @@ static const struct of_device_id rockchip_pinctrl_dt_match[] = {
> .data = &rk3288_pin_ctrl },
> { .compatible = "rockchip,rk3308-pinctrl",
> .data = &rk3308_pin_ctrl },
> + { .compatible = "rockchip,rk3308b-pinctrl",
> + .data = &rk3308b_pin_ctrl },
I'm skeptical about this being bound to a new DT compatible. As far as I
know the RK3308 and RK3308B are mostly equivalent, so it looks as the
pinctrl implementation could be detected at runtime. This would let
products to be built with either chip version and work on any without
any DT change.
Code for reading the chip ID is in the RK3308 codec driver [0].
[0] https://lore.kernel.org/all/[email protected]/ -> search "GRF_CHIP_ID"
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello Dmitry,
On Thu, 16 May 2024 17:06:46 +0500
Dmitry Yashin <[email protected]> wrote:
> Hi Luca,
>
> On 15.05.24 21:29, Luca Ceresoli wrote:
> > I'm skeptical about this being bound to a new DT compatible. As far as I
> > know the RK3308 and RK3308B are mostly equivalent, so it looks as the
> > pinctrl implementation could be detected at runtime. This would let
> > products to be built with either chip version and work on any without
> > any DT change.
>
>
> Thanks for your feedback.
>
> Indeed, these SoC's have a lot in common, but as I can see the rk3308b
> has more blocks, like extra PWM's (rk3308 datasheet 1.5 [0] shows only
> 1x PWM 4ch, when rk3308b and rk3308b-s have 3x PWM 4ch), 1-wire and
> CAN controller (mentioned in the TRM, but dropped from rk3308b
> datasheet for some reason).
>
> So, in my view, it really makes sense to add rk3308b.dtsi, where extra
> PWM's, pinctrl compatible and its pin functions can be moved. And if
> its not worth it, then I will try to adapt the entire series to runtime
> config based on cpuid like you suggested.
Having a rk3308b.dtsi would probably make sense, yes, as there are
several differences as you described. However for the pinctrl it seems
probably not necessary.
I've seen actual products being manufactured with two different RK3308
variants in different lots of production, but with the same DT that has
rockchip,rk3308-pinctrl in it. Those would need a _selective_ DT
upgrade in order to benefit from your changes.
And even if a product had always used the B variant, it would need DT
upgrade when upgrading to a kernel with your changes. Otherwise with
patch 1/3 of this series the pictrl driver would lose many routes after
upgrading the kernel (but not the DT): can this lead to
previously-working devices to stop working? I think this is a
fundamental question to reply.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Am Freitag, 17. Mai 2024, 08:58:32 CEST schrieb Luca Ceresoli:
> Hello Dmitry,
>
> On Thu, 16 May 2024 17:06:46 +0500
> Dmitry Yashin <[email protected]> wrote:
>
> > Hi Luca,
> >
> > On 15.05.24 21:29, Luca Ceresoli wrote:
> > > I'm skeptical about this being bound to a new DT compatible. As far as I
> > > know the RK3308 and RK3308B are mostly equivalent, so it looks as the
> > > pinctrl implementation could be detected at runtime. This would let
> > > products to be built with either chip version and work on any without
> > > any DT change.
> >
> >
> > Thanks for your feedback.
> >
> > Indeed, these SoC's have a lot in common, but as I can see the rk3308b
> > has more blocks, like extra PWM's (rk3308 datasheet 1.5 [0] shows only
> > 1x PWM 4ch, when rk3308b and rk3308b-s have 3x PWM 4ch), 1-wire and
> > CAN controller (mentioned in the TRM, but dropped from rk3308b
> > datasheet for some reason).
> >
> > So, in my view, it really makes sense to add rk3308b.dtsi, where extra
> > PWM's, pinctrl compatible and its pin functions can be moved. And if
> > its not worth it, then I will try to adapt the entire series to runtime
> > config based on cpuid like you suggested.
>
> Having a rk3308b.dtsi would probably make sense, yes, as there are
> several differences as you described. However for the pinctrl it seems
> probably not necessary.
>
> I've seen actual products being manufactured with two different RK3308
> variants in different lots of production, but with the same DT that has
> rockchip,rk3308-pinctrl in it. Those would need a _selective_ DT
> upgrade in order to benefit from your changes.
>
> And even if a product had always used the B variant, it would need DT
> upgrade when upgrading to a kernel with your changes. Otherwise with
> patch 1/3 of this series the pictrl driver would lose many routes after
> upgrading the kernel (but not the DT): can this lead to
> previously-working devices to stop working? I think this is a
> fundamental question to reply.
If things can be runtime-detectable, they should be detected at runtime.
So yes, while we need to know that it is a rk3308-something before
via the dt, if we can distinguish between the rk3308 variants at runtime
we should definitly do so.
Heiko
On 2024-05-28 10:17, Heiko Stübner wrote:
> Am Freitag, 17. Mai 2024, 08:58:32 CEST schrieb Luca Ceresoli:
>> Hello Dmitry,
>>
>> On Thu, 16 May 2024 17:06:46 +0500
>> Dmitry Yashin <[email protected]> wrote:
>>
>>> Hi Luca,
>>>
>>> On 15.05.24 21:29, Luca Ceresoli wrote:
>>>> I'm skeptical about this being bound to a new DT compatible. As far as I
>>>> know the RK3308 and RK3308B are mostly equivalent, so it looks as the
>>>> pinctrl implementation could be detected at runtime. This would let
>>>> products to be built with either chip version and work on any without
>>>> any DT change.
>>>
>>>
>>> Thanks for your feedback.
>>>
>>> Indeed, these SoC's have a lot in common, but as I can see the rk3308b
>>> has more blocks, like extra PWM's (rk3308 datasheet 1.5 [0] shows only
>>> 1x PWM 4ch, when rk3308b and rk3308b-s have 3x PWM 4ch), 1-wire and
>>> CAN controller (mentioned in the TRM, but dropped from rk3308b
>>> datasheet for some reason).
>>>
>>> So, in my view, it really makes sense to add rk3308b.dtsi, where extra
>>> PWM's, pinctrl compatible and its pin functions can be moved. And if
>>> its not worth it, then I will try to adapt the entire series to runtime
>>> config based on cpuid like you suggested.
>>
>> Having a rk3308b.dtsi would probably make sense, yes, as there are
>> several differences as you described. However for the pinctrl it seems
>> probably not necessary.
>>
>> I've seen actual products being manufactured with two different RK3308
>> variants in different lots of production, but with the same DT that has
>> rockchip,rk3308-pinctrl in it. Those would need a _selective_ DT
>> upgrade in order to benefit from your changes.
>>
>> And even if a product had always used the B variant, it would need DT
>> upgrade when upgrading to a kernel with your changes. Otherwise with
>> patch 1/3 of this series the pictrl driver would lose many routes after
>> upgrading the kernel (but not the DT): can this lead to
>> previously-working devices to stop working? I think this is a
>> fundamental question to reply.
>
> If things can be runtime-detectable, they should be detected at runtime.
> So yes, while we need to know that it is a rk3308-something before
> via the dt, if we can distinguish between the rk3308 variants at runtime
> we should definitly do so.
The GRF_CHIP_ID reg (0xFF000800) can be used to identify what model is
used at runtime:
RK3308: 0xCEA (errata: chip id value is 32'h0cea (32'd3306))
RK3308B: 0x3308
RK3308BS: 0x3308C
Vendor U-Boot make use of this reg to identify what model is running:
https://github.com/rockchip-linux/u-boot/blob/next-dev/arch/arm/include/asm/arch-rockchip/cpu.h#L68-L82
I can only validate on real hw that the reg value is 0x3308 for RK3308B.
Regards,
Jonas
>
> Heiko
>
On Wed, May 15, 2024 at 2:17 PM Dmitry Yashin <[email protected]> wrote:
> Some of the rk3308 iomux routes in rk3308_mux_route_data belong to
> the rk3308b SoC. Remove them and correct i2c3 routes.
>
> Fixes: 7825aeb7b208 ("pinctrl: rockchip: add rk3308 SoC support")
> Signed-off-by: Dmitry Yashin <[email protected]>
While you guys are thinking about the RK3308B support, is this fix
something I can just apply?
Yours,
Linus Walleij
Am Mittwoch, 15. Mai 2024, 14:16:32 CEST schrieb Dmitry Yashin:
> Some of the rk3308 iomux routes in rk3308_mux_route_data belong to
> the rk3308b SoC. Remove them and correct i2c3 routes.
>
> Fixes: 7825aeb7b208 ("pinctrl: rockchip: add rk3308 SoC support")
> Signed-off-by: Dmitry Yashin <[email protected]>
> ---
> drivers/pinctrl/pinctrl-rockchip.c | 17 ++---------------
> 1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index 3bedf36a0019..cc647db76927 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -870,9 +870,8 @@ static struct rockchip_mux_route_data rk3308_mux_route_data[] = {
> RK_MUXROUTE_SAME(0, RK_PC3, 1, 0x314, BIT(16 + 0) | BIT(0)), /* rtc_clk */
> RK_MUXROUTE_SAME(1, RK_PC6, 2, 0x314, BIT(16 + 2) | BIT(16 + 3)), /* uart2_rxm0 */
> RK_MUXROUTE_SAME(4, RK_PD2, 2, 0x314, BIT(16 + 2) | BIT(16 + 3) | BIT(2)), /* uart2_rxm1 */
> - RK_MUXROUTE_SAME(0, RK_PB7, 2, 0x608, BIT(16 + 8) | BIT(16 + 9)), /* i2c3_sdam0 */
> - RK_MUXROUTE_SAME(3, RK_PB4, 2, 0x608, BIT(16 + 8) | BIT(16 + 9) | BIT(8)), /* i2c3_sdam1 */
> - RK_MUXROUTE_SAME(2, RK_PA0, 3, 0x608, BIT(16 + 8) | BIT(16 + 9) | BIT(9)), /* i2c3_sdam2 */
> + RK_MUXROUTE_SAME(0, RK_PB7, 2, 0x314, BIT(16 + 4)), /* i2c3_sdam0 */
> + RK_MUXROUTE_SAME(3, RK_PB4, 2, 0x314, BIT(16 + 4) | BIT(4)), /* i2c3_sdam1 */
checked against TRM and no devicetree in the kernel currently enables i2c3 at all.
> RK_MUXROUTE_SAME(1, RK_PA3, 2, 0x308, BIT(16 + 3)), /* i2s-8ch-1-sclktxm0 */
> RK_MUXROUTE_SAME(1, RK_PA4, 2, 0x308, BIT(16 + 3)), /* i2s-8ch-1-sclkrxm0 */
> RK_MUXROUTE_SAME(1, RK_PB5, 2, 0x308, BIT(16 + 3) | BIT(3)), /* i2s-8ch-1-sclktxm1 */
> @@ -881,18 +880,6 @@ static struct rockchip_mux_route_data rk3308_mux_route_data[] = {
> RK_MUXROUTE_SAME(1, RK_PB6, 4, 0x308, BIT(16 + 12) | BIT(16 + 13) | BIT(12)), /* pdm-clkm1 */
> RK_MUXROUTE_SAME(2, RK_PA6, 2, 0x308, BIT(16 + 12) | BIT(16 + 13) | BIT(13)), /* pdm-clkm2 */
> RK_MUXROUTE_SAME(2, RK_PA4, 3, 0x600, BIT(16 + 2) | BIT(2)), /* pdm-clkm-m2 */
> - RK_MUXROUTE_SAME(3, RK_PB2, 3, 0x314, BIT(16 + 9)), /* spi1_miso */
> - RK_MUXROUTE_SAME(2, RK_PA4, 2, 0x314, BIT(16 + 9) | BIT(9)), /* spi1_miso_m1 */
> - RK_MUXROUTE_SAME(0, RK_PB3, 3, 0x314, BIT(16 + 10) | BIT(16 + 11)), /* owire_m0 */
> - RK_MUXROUTE_SAME(1, RK_PC6, 7, 0x314, BIT(16 + 10) | BIT(16 + 11) | BIT(10)), /* owire_m1 */
> - RK_MUXROUTE_SAME(2, RK_PA2, 5, 0x314, BIT(16 + 10) | BIT(16 + 11) | BIT(11)), /* owire_m2 */
> - RK_MUXROUTE_SAME(0, RK_PB3, 2, 0x314, BIT(16 + 12) | BIT(16 + 13)), /* can_rxd_m0 */
> - RK_MUXROUTE_SAME(1, RK_PC6, 5, 0x314, BIT(16 + 12) | BIT(16 + 13) | BIT(12)), /* can_rxd_m1 */
> - RK_MUXROUTE_SAME(2, RK_PA2, 4, 0x314, BIT(16 + 12) | BIT(16 + 13) | BIT(13)), /* can_rxd_m2 */
> - RK_MUXROUTE_SAME(1, RK_PC4, 3, 0x314, BIT(16 + 14)), /* mac_rxd0_m0 */
> - RK_MUXROUTE_SAME(4, RK_PA2, 2, 0x314, BIT(16 + 14) | BIT(14)), /* mac_rxd0_m1 */
> - RK_MUXROUTE_SAME(3, RK_PB4, 4, 0x314, BIT(16 + 15)), /* uart3_rx */
> - RK_MUXROUTE_SAME(0, RK_PC1, 3, 0x314, BIT(16 + 15) | BIT(15)), /* uart3_rx_m1 */
checked against the TRM too.
And as far as usage:
- spi1: not used at all
- owire: not used at all
- can: not used at all
- mac_rxd0: all boards use the (default) m0 variant
- uart3: not currently enabled anywhere
And I guess the rk3308 is niche enough that people either use the vendor-kernel
or have the dts in mainline, so we should have a ok grasp on not causing breakage
by some accidental rk3308b somewhere.
Reviewed-by: Heiko Stuebner <[email protected]>
Hi Linus,
Am Dienstag, 28. Mai 2024, 13:29:12 CEST schrieb Linus Walleij:
> On Wed, May 15, 2024 at 2:17 PM Dmitry Yashin <[email protected]> wrote:
>
> > Some of the rk3308 iomux routes in rk3308_mux_route_data belong to
> > the rk3308b SoC. Remove them and correct i2c3 routes.
> >
> > Fixes: 7825aeb7b208 ("pinctrl: rockchip: add rk3308 SoC support")
> > Signed-off-by: Dmitry Yashin <[email protected]>
>
> While you guys are thinking about the RK3308B support, is this fix
> something I can just apply?
I'd think so. I've detailed stuff in my Review mail I just sent.
Both the soc itself and also the affected pin functions are niche
enough that this should not cause breakage.
Heiko
Hi Heiko,
On 5/28/24 4:52 PM, Heiko Stübner wrote:
> Hi Linus,
>
> Am Dienstag, 28. Mai 2024, 13:29:12 CEST schrieb Linus Walleij:
>> On Wed, May 15, 2024 at 2:17 PM Dmitry Yashin <[email protected]> wrote:
>>
>>> Some of the rk3308 iomux routes in rk3308_mux_route_data belong to
>>> the rk3308b SoC. Remove them and correct i2c3 routes.
>>>
>>> Fixes: 7825aeb7b208 ("pinctrl: rockchip: add rk3308 SoC support")
>>> Signed-off-by: Dmitry Yashin <[email protected]>
>> While you guys are thinking about the RK3308B support, is this fix
>> something I can just apply?
> I'd think so. I've detailed stuff in my Review mail I just sent.
> Both the soc itself and also the affected pin functions are niche
> enough that this should not cause breakage.
>
>
> Heiko
>
>
>
Should i just drop 1/3 from V2 then?
Thanks everyone for the feedback on this series. I'll prepare V2 based
on runtime chip detection with use of GRF_CHIP_ID.
--
Thanks,
Dmitry
Am Dienstag, 28. Mai 2024, 14:18:35 CEST schrieb Dmitry Yashin:
> Hi Heiko,
>
> On 5/28/24 4:52 PM, Heiko Stübner wrote:
> > Hi Linus,
> >
> > Am Dienstag, 28. Mai 2024, 13:29:12 CEST schrieb Linus Walleij:
> >> On Wed, May 15, 2024 at 2:17 PM Dmitry Yashin <[email protected]> wrote:
> >>
> >>> Some of the rk3308 iomux routes in rk3308_mux_route_data belong to
> >>> the rk3308b SoC. Remove them and correct i2c3 routes.
> >>>
> >>> Fixes: 7825aeb7b208 ("pinctrl: rockchip: add rk3308 SoC support")
> >>> Signed-off-by: Dmitry Yashin <[email protected]>
> >> While you guys are thinking about the RK3308B support, is this fix
> >> something I can just apply?
> > I'd think so. I've detailed stuff in my Review mail I just sent.
> > Both the soc itself and also the affected pin functions are niche
> > enough that this should not cause breakage.
> >
> >
> > Heiko
> >
> >
> >
>
> Should i just drop 1/3 from V2 then?
I guess just check the state of affairs once your v2 is ready ;-) .
I.e. if LinusW grabs patch1 before you post v2, just drop it, otherwise
send it along.
> Thanks everyone for the feedback on this series. I'll prepare V2 based
> on runtime chip detection with use of GRF_CHIP_ID.
On Wed, May 15, 2024 at 2:17 PM Dmitry Yashin <[email protected]> wrote:
> Some of the rk3308 iomux routes in rk3308_mux_route_data belong to
> the rk3308b SoC. Remove them and correct i2c3 routes.
>
> Fixes: 7825aeb7b208 ("pinctrl: rockchip: add rk3308 SoC support")
> Signed-off-by: Dmitry Yashin <[email protected]>
This patch applied!
Yours,
Linus Walleij