Re: [PATCH 2/4] pinctrl: rockchip: Add iomux-route switching support for rk3228
From: Heiko Stuebner
Date: Thu May 25 2017 - 17:13:04 EST
Hi David,
Am Donnerstag, 25. Mai 2017, 21:12:30 CEST schrieb David Wu:
> There are 9 IP blocks pin routes need to be switched, that are
> pwm-0, pwm-1, pwm-2, pwm-3, sdio, spi, emmc, uart2, uart1.
>
> Signed-off-by: David Wu <david.wu@xxxxxxxxxxxxxx>
This series come pretty close to what I had in mind, but I do have some
change requests inline below.
The comments are in this patch but apply to the whole series.
> ---
> drivers/pinctrl/pinctrl-rockchip.c | 176 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 172 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index f5dd1c3..be4c16e 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -187,6 +187,20 @@ struct rockchip_pin_bank {
> }, \
> }
>
> +#define PIN_BANK_ROUTE(id, pins, label, route) \
> + { \
> + .bank_num = id, \
> + .nr_pins = pins, \
> + .name = label, \
> + .route_mask = route, \
> + .iomux = { \
> + { .offset = -1 }, \
> + { .offset = -1 }, \
> + { .offset = -1 }, \
> + { .offset = -1 }, \
> + }, \
> + }
> +
> #define PIN_BANK_IOMUX_FLAGS(id, pins, label, iom0, iom1, iom2, iom3) \
> { \
> .bank_num = id, \
> @@ -605,6 +619,159 @@ static void rk3328_recalc_mux(u8 bank_num, int pin, int *reg,
> *bit = data->bit;
> }
>
> +static const struct rockchip_mux_route_data rk3228_mux_route_data[] = {
> + {
> + /* pwm0-0 */
> + .bank = 0,
> + .pin = 26,
> + .func = 1,
> + .route_offset = 0x50,
> + .route_val = BIT(16),
> + }, {
> + /* pwm0-1 */
> + .bank = 3,
> + .pin = 21,
> + .func = 1,
> + .route_offset = 0x50,
> + .route_val = BIT(16) | BIT(0),
> + }, {
> + /* pwm1-0 */
> + .bank = 0,
> + .pin = 27,
> + .func = 1,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 1),
> + }, {
> + /* pwm1-1 */
> + .bank = 0,
> + .pin = 30,
> + .func = 2,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 1) | BIT(1),
> + }, {
> + /* pwm2-0 */
> + .bank = 0,
> + .pin = 28,
> + .func = 1,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 2),
> + }, {
> + /* pwm2-1 */
> + .bank = 1,
> + .pin = 12,
> + .func = 2,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 2) | BIT(2),
> + }, {
> + /* pwm3-0 */
> + .bank = 3,
> + .pin = 26,
> + .func = 1,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 3),
> + }, {
> + /* pwm3-1 */
> + .bank = 1,
> + .pin = 11,
> + .func = 2,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 3) | BIT(3),
> + }, {
> + /* sdio-0_d0 */
> + .bank = 1,
> + .pin = 1,
> + .func = 1,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 4),
> + }, {
> + /* sdio-1_d0 */
> + .bank = 3,
> + .pin = 2,
> + .func = 1,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 4) | BIT(4),
> + }, {
> + /* spi-0_rx */
> + .bank = 0,
> + .pin = 13,
> + .func = 2,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 5),
> + }, {
> + /* spi-1_rx */
> + .bank = 2,
> + .pin = 0,
> + .func = 2,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 5) | BIT(5),
> + }, {
> + /* emmc-0_cmd */
> + .bank = 1,
> + .pin = 22,
> + .func = 2,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 7),
> + }, {
> + /* emmc-1_cmd */
> + .bank = 2,
> + .pin = 4,
> + .func = 2,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 7) | BIT(7),
> + }, {
> + /* uart2-0_rx */
> + .bank = 1,
> + .pin = 19,
> + .func = 2,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 8),
> + }, {
> + /* uart2-1_rx */
> + .bank = 1,
> + .pin = 10,
> + .func = 2,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 8) | BIT(8),
> + }, {
> + /* uart1-0_rx */
> + .bank = 1,
> + .pin = 10,
> + .func = 1,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 11),
> + }, {
> + /* uart1-1_rx */
> + .bank = 3,
> + .pin = 13,
> + .func = 1,
> + .route_offset = 0x50,
> + .route_val = BIT(16 + 11) | BIT(11),
> + },
> +};
> +
> +static bool rk3228_set_mux_route(u8 bank_num, int pin,
> + int mux, u32 *reg, u32 *value)
instead of referencing this function in the per-soc struct, please make
it generic and reference the route array + size in the per-soc struct.
Except for referencing a different array, the function is the same
everywhere, so there is no need to duplicate it for each soc.
> +{
> + const struct rockchip_mux_route_data *data = NULL;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(rk3228_mux_route_data); i++)
> + if ((rk3228_mux_route_data[i].bank == bank_num) &&
> + (rk3228_mux_route_data[i].pin == pin) &&
> + (rk3228_mux_route_data[i].func == mux)) {
> + data = &rk3228_mux_route_data[i];
> + break;
> + }
> +
> + if (!data)
> + return false;
> +
> + *reg = data->route_offset;
> + *value = data->route_val;
> +
> + return true;
> +}
> +
> static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin)
> {
> struct rockchip_pinctrl *info = bank->drvdata;
> @@ -2852,10 +3019,10 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
> };
>
> static struct rockchip_pin_bank rk3228_pin_banks[] = {
> - PIN_BANK(0, 32, "gpio0"),
> - PIN_BANK(1, 32, "gpio1"),
> - PIN_BANK(2, 32, "gpio2"),
> - PIN_BANK(3, 32, "gpio3"),
> + PIN_BANK_ROUTE(0, 32, "gpio0", 0x5c002000),
> + PIN_BANK_ROUTE(1, 32, "gpio1", 0x00483c02),
> + PIN_BANK_ROUTE(2, 32, "gpio2", 0x00000011),
> + PIN_BANK_ROUTE(3, 32, "gpio3", 0x04202004),
Requiring developers to calculate this pin-bit-value for each bank
is cumbersome and error-prone. With the routes-struct known in
the driver (see above and below), you can keep the the value element
in rockchip_pin_bank, but calculate the per-bank value dynamically
when the bank gets created.
For example in rockchip_pinctrl_get_soc_data just parse the route-struct
and calculate that value when the driver probes.
This reduces possible errors and also spares us the clutter of all the
additional PIN_BANK_* defines.
> };
>
> static struct rockchip_pin_ctrl rk3228_pin_ctrl = {
> @@ -2866,6 +3033,7 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
> .grf_mux_offset = 0x0,
> .pull_calc_reg = rk3228_calc_pull_reg_and_bit,
> .drv_calc_reg = rk3228_calc_drv_reg_and_bit,
> + .iomux_route = rk3228_set_mux_route,
.iomux_routes = rk3228_mux_route_data,
.niomux_routes = ARRAY_SIZE(rk3228_mux_route_data),
Heiko
> };
>
> static struct rockchip_pin_bank rk3288_pin_banks[] = {
>