[PATCH] pinctrl: rockchip: Fix drive and input schmitt on RK3568

rk3568_set_drive configures a second reg for specific pins. Mainline linux does not do this and vendor U-Boot only run similar code when bit 14 and 15 are both 0 in PMU_GRF_SOC_CON0. Something that presumably only early revisions of the SoC have, all my RK3566/RK3568 boards read back bit 15 as 1, even on boards dated back to 21H1.
This cause e.g. ethernet PHY on Radxa CM3-IO board not to work after drive is configured according to the device tree.
Input schmitt is configured in 2-bit fields on RK3568 compared to earlier generation and 2'b10 should be used to enable input schmitt.
Remove the code that presumably was intended for early pre-production revisions of the SoC and write correct values for input schmitt setting. Also change to use regmap_update_bits to closer match linux driver.
Fixes: 1977d746aa54 ("rockchip: rk3568: add rk3568 pinctrl driver") Signed-off-by: Jonas Karlman jonas@kwiboo.se --- drivers/pinctrl/rockchip/pinctrl-rk3568.c | 52 ++++++----------------- 1 file changed, 14 insertions(+), 38 deletions(-)
diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3568.c b/drivers/pinctrl/rockchip/pinctrl-rk3568.c index 314edb5a6064..9a19dbe81cdc 100644 --- a/drivers/pinctrl/rockchip/pinctrl-rk3568.c +++ b/drivers/pinctrl/rockchip/pinctrl-rk3568.c @@ -113,11 +113,9 @@ static int rk3568_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) struct rockchip_pinctrl_priv *priv = bank->priv; int iomux_num = (pin / 8); struct regmap *regmap; - int reg, ret, mask; + int reg, mask; u8 bit; - u32 data; - - debug("setting mux of GPIO%d-%d to %d\n", bank->bank_num, pin, mux); + u32 data, rmask;
if (bank->iomux[iomux_num].type & IOMUX_SOURCE_PMU) regmap = priv->regmap_pmu; @@ -131,10 +129,10 @@ static int rk3568_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) mask = 0xf;
data = (mask << (bit + 16)); + rmask = data | (data >> 16); data |= (mux & mask) << bit; - ret = regmap_write(regmap, reg, data);
- return ret; + return regmap_update_bits(regmap, reg, rmask, data); }
#define RK3568_PULL_PMU_OFFSET 0x20 @@ -225,7 +223,7 @@ static int rk3568_set_pull(struct rockchip_pin_bank *bank, struct regmap *regmap; int reg, ret; u8 bit, type; - u32 data; + u32 data, rmask;
if (pull == PIN_CONFIG_BIAS_PULL_PIN_DEFAULT) return -ENOTSUPP; @@ -249,11 +247,10 @@ static int rk3568_set_pull(struct rockchip_pin_bank *bank,
/* enable the write to the equivalent lower bits */ data = ((1 << ROCKCHIP_PULL_BITS_PER_PIN) - 1) << (bit + 16); - + rmask = data | (data >> 16); data |= (ret << bit); - ret = regmap_write(regmap, reg, data);
- return ret; + return regmap_update_bits(regmap, reg, rmask, data); }
static int rk3568_set_drive(struct rockchip_pin_bank *bank, @@ -261,40 +258,18 @@ static int rk3568_set_drive(struct rockchip_pin_bank *bank, { struct regmap *regmap; int reg; - u32 data; + u32 data, rmask; u8 bit; int drv = (1 << (strength + 1)) - 1; - int ret = 0;
rk3568_calc_drv_reg_and_bit(bank, pin_num, ®map, ®, &bit);
/* enable the write to the equivalent lower bits */ data = ((1 << RK3568_DRV_BITS_PER_PIN) - 1) << (bit + 16); + rmask = data | (data >> 16); data |= (drv << bit);
- ret = regmap_write(regmap, reg, data); - if (ret) - return ret; - - if (bank->bank_num == 1 && pin_num == 21) - reg = 0x0840; - else if (bank->bank_num == 2 && pin_num == 2) - reg = 0x0844; - else if (bank->bank_num == 2 && pin_num == 8) - reg = 0x0848; - else if (bank->bank_num == 3 && pin_num == 0) - reg = 0x084c; - else if (bank->bank_num == 3 && pin_num == 6) - reg = 0x0850; - else if (bank->bank_num == 4 && pin_num == 0) - reg = 0x0854; - else - return 0; - - data = ((1 << RK3568_DRV_BITS_PER_PIN) - 1) << 16; - data |= drv; - - return regmap_write(regmap, reg, data); + return regmap_update_bits(regmap, reg, rmask, data); }
static int rk3568_set_schmitt(struct rockchip_pin_bank *bank, @@ -302,16 +277,17 @@ static int rk3568_set_schmitt(struct rockchip_pin_bank *bank, { struct regmap *regmap; int reg; - u32 data; + u32 data, rmask; u8 bit;
rk3568_calc_schmitt_reg_and_bit(bank, pin_num, ®map, ®, &bit);
/* enable the write to the equivalent lower bits */ data = ((1 << RK3568_SCHMITT_BITS_PER_PIN) - 1) << (bit + 16); - data |= (enable << bit); + rmask = data | (data >> 16); + data |= ((enable ? 0x2 : 0x1) << bit);
- return regmap_write(regmap, reg, data); + return regmap_update_bits(regmap, reg, rmask, data); }
static struct rockchip_pin_bank rk3568_pin_banks[] = {

On Thu, 3 Aug 2023 at 11:44, Jonas Karlman jonas@kwiboo.se wrote:
rk3568_set_drive configures a second reg for specific pins. Mainline linux does not do this and vendor U-Boot only run similar code when bit 14 and 15 are both 0 in PMU_GRF_SOC_CON0. Something that presumably only early revisions of the SoC have, all my RK3566/RK3568 boards read back bit 15 as 1, even on boards dated back to 21H1.
This cause e.g. ethernet PHY on Radxa CM3-IO board not to work after drive is configured according to the device tree.
Input schmitt is configured in 2-bit fields on RK3568 compared to earlier generation and 2'b10 should be used to enable input schmitt.
Remove the code that presumably was intended for early pre-production revisions of the SoC and write correct values for input schmitt setting. Also change to use regmap_update_bits to closer match linux driver.
Fixes: 1977d746aa54 ("rockchip: rk3568: add rk3568 pinctrl driver") Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/pinctrl/rockchip/pinctrl-rk3568.c | 52 ++++++----------------- 1 file changed, 14 insertions(+), 38 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

Add Steven Liu,
Hi Steven,
Please help to review this patch.
On 2023/8/4 01:44, Jonas Karlman wrote:
rk3568_set_drive configures a second reg for specific pins. Mainline linux does not do this and vendor U-Boot only run similar code when bit 14 and 15 are both 0 in PMU_GRF_SOC_CON0.
The base version of this driver also based on vendor U-Boot, right? But this part of
logic is different? interesting...
Thanks,
- Kever
Something that presumably only early revisions of the SoC have, all my RK3566/RK3568 boards read back bit 15 as 1, even on boards dated back to 21H1.
This cause e.g. ethernet PHY on Radxa CM3-IO board not to work after drive is configured according to the device tree.
Input schmitt is configured in 2-bit fields on RK3568 compared to earlier generation and 2'b10 should be used to enable input schmitt.
Remove the code that presumably was intended for early pre-production revisions of the SoC and write correct values for input schmitt setting. Also change to use regmap_update_bits to closer match linux driver.
Fixes: 1977d746aa54 ("rockchip: rk3568: add rk3568 pinctrl driver") Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/pinctrl/rockchip/pinctrl-rk3568.c | 52 ++++++----------------- 1 file changed, 14 insertions(+), 38 deletions(-)
diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3568.c b/drivers/pinctrl/rockchip/pinctrl-rk3568.c index 314edb5a6064..9a19dbe81cdc 100644 --- a/drivers/pinctrl/rockchip/pinctrl-rk3568.c +++ b/drivers/pinctrl/rockchip/pinctrl-rk3568.c @@ -113,11 +113,9 @@ static int rk3568_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) struct rockchip_pinctrl_priv *priv = bank->priv; int iomux_num = (pin / 8); struct regmap *regmap;
- int reg, ret, mask;
- int reg, mask; u8 bit;
- u32 data;
- debug("setting mux of GPIO%d-%d to %d\n", bank->bank_num, pin, mux);
u32 data, rmask;
if (bank->iomux[iomux_num].type & IOMUX_SOURCE_PMU) regmap = priv->regmap_pmu;
@@ -131,10 +129,10 @@ static int rk3568_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) mask = 0xf;
data = (mask << (bit + 16));
- rmask = data | (data >> 16); data |= (mux & mask) << bit;
ret = regmap_write(regmap, reg, data);
return ret;
return regmap_update_bits(regmap, reg, rmask, data); }
#define RK3568_PULL_PMU_OFFSET 0x20
@@ -225,7 +223,7 @@ static int rk3568_set_pull(struct rockchip_pin_bank *bank, struct regmap *regmap; int reg, ret; u8 bit, type;
- u32 data;
u32 data, rmask;
if (pull == PIN_CONFIG_BIAS_PULL_PIN_DEFAULT) return -ENOTSUPP;
@@ -249,11 +247,10 @@ static int rk3568_set_pull(struct rockchip_pin_bank *bank,
/* enable the write to the equivalent lower bits */ data = ((1 << ROCKCHIP_PULL_BITS_PER_PIN) - 1) << (bit + 16);
- rmask = data | (data >> 16); data |= (ret << bit);
ret = regmap_write(regmap, reg, data);
return ret;
return regmap_update_bits(regmap, reg, rmask, data); }
static int rk3568_set_drive(struct rockchip_pin_bank *bank,
@@ -261,40 +258,18 @@ static int rk3568_set_drive(struct rockchip_pin_bank *bank, { struct regmap *regmap; int reg;
- u32 data;
- u32 data, rmask; u8 bit; int drv = (1 << (strength + 1)) - 1;
int ret = 0;
rk3568_calc_drv_reg_and_bit(bank, pin_num, ®map, ®, &bit);
/* enable the write to the equivalent lower bits */ data = ((1 << RK3568_DRV_BITS_PER_PIN) - 1) << (bit + 16);
- rmask = data | (data >> 16); data |= (drv << bit);
- ret = regmap_write(regmap, reg, data);
- if (ret)
return ret;
- if (bank->bank_num == 1 && pin_num == 21)
reg = 0x0840;
- else if (bank->bank_num == 2 && pin_num == 2)
reg = 0x0844;
- else if (bank->bank_num == 2 && pin_num == 8)
reg = 0x0848;
- else if (bank->bank_num == 3 && pin_num == 0)
reg = 0x084c;
- else if (bank->bank_num == 3 && pin_num == 6)
reg = 0x0850;
- else if (bank->bank_num == 4 && pin_num == 0)
reg = 0x0854;
- else
return 0;
- data = ((1 << RK3568_DRV_BITS_PER_PIN) - 1) << 16;
- data |= drv;
- return regmap_write(regmap, reg, data);
return regmap_update_bits(regmap, reg, rmask, data); }
static int rk3568_set_schmitt(struct rockchip_pin_bank *bank,
@@ -302,16 +277,17 @@ static int rk3568_set_schmitt(struct rockchip_pin_bank *bank, { struct regmap *regmap; int reg;
- u32 data;
u32 data, rmask; u8 bit;
rk3568_calc_schmitt_reg_and_bit(bank, pin_num, ®map, ®, &bit);
/* enable the write to the equivalent lower bits */ data = ((1 << RK3568_SCHMITT_BITS_PER_PIN) - 1) << (bit + 16);
- data |= (enable << bit);
- rmask = data | (data >> 16);
- data |= ((enable ? 0x2 : 0x1) << bit);
- return regmap_write(regmap, reg, data);
return regmap_update_bits(regmap, reg, rmask, data); }
static struct rockchip_pin_bank rk3568_pin_banks[] = {

Hi Kever and Steven,
On 2023-08-12 04:50, Kever Yang wrote:
Add Steven Liu,
Hi Steven,
Please help to review this patch.
On 2023/8/4 01:44, Jonas Karlman wrote:
rk3568_set_drive configures a second reg for specific pins. Mainline linux does not do this and vendor U-Boot only run similar code when bit 14 and 15 are both 0 in PMU_GRF_SOC_CON0.
The base version of this driver also based on vendor U-Boot, right? But this part of
logic is different? interesting...
Yes, this seemed strange.
I have sent a v2 of this patch that restore part of this code after looking closer in Hardware Design Guide, PinOut and TRM documents.
If I understand correctly the first reg should contain DS[5:0] and the second reg should contain DS[11:6], not a copy of DS[5:0].
Regards, Jonas
Thanks,
- Kever
Something that presumably only early revisions of the SoC have, all my RK3566/RK3568 boards read back bit 15 as 1, even on boards dated back to 21H1.
This cause e.g. ethernet PHY on Radxa CM3-IO board not to work after drive is configured according to the device tree.
Input schmitt is configured in 2-bit fields on RK3568 compared to earlier generation and 2'b10 should be used to enable input schmitt.
Remove the code that presumably was intended for early pre-production revisions of the SoC and write correct values for input schmitt setting. Also change to use regmap_update_bits to closer match linux driver.
Fixes: 1977d746aa54 ("rockchip: rk3568: add rk3568 pinctrl driver") Signed-off-by: Jonas Karlman jonas@kwiboo.se
drivers/pinctrl/rockchip/pinctrl-rk3568.c | 52 ++++++----------------- 1 file changed, 14 insertions(+), 38 deletions(-)
[...]
participants (3)
-
Jonas Karlman
-
Kever Yang
-
Simon Glass