Hello Kavya,
On 29/10/2021 14:57:03+0530, Kavyasree Kotagiri wrote:
> + LAN966X_PIN(76),
> + LAN966X_PIN(77),
> +};
> +
> +
One blank line should be removed
> static int ocelot_get_functions_count(struct pinctrl_dev *pctldev)
> {
> return ARRAY_SIZE(ocelot_function_names);
> @@ -709,6 +1056,9 @@ static int ocelot_pin_function_idx(struct ocelot_pinctrl *info,
> for (i = 0; i < OCELOT_FUNC_PER_PIN; i++) {
> if (function == p->functions[i])
> return i;
> +
> + if (function == p->a_functions[i])
> + return i + OCELOT_FUNC_PER_PIN;
> }
>
> return -1;
> @@ -744,6 +1094,36 @@ static int ocelot_pinmux_set_mux(struct pinctrl_dev *pctldev,
> return 0;
> }
>
> +static int lan966x_pinmux_set_mux(struct pinctrl_dev *pctldev,
> + unsigned int selector, unsigned int group)
> +{
> + struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> + struct ocelot_pin_caps *pin = info->desc->pins[group].drv_data;
> + unsigned int p = pin->pin % 32;
> + int f;
> +
> + f = ocelot_pin_function_idx(info, group, selector);
> + if (f < 0)
> + return -EINVAL;
> +
> + /*
> + * f is encoded on three bits.
> + * bit 0 of f goes in BIT(pin) of ALT[0], bit 1 of f goes in BIT(pin) of
> + * ALT[1], bit 2 of f goes in BIT(pin) of ALT[2]
> + * This is racy because both registers can't be updated at the same time
That's three registers, not two so I guess this sentence should be
reworked.
> + * but it doesn't matter much for now.
> + * Note: ALT0/ALT1/ALT2 are organized specially for 78 gpio targets
> + */
> + regmap_update_bits(info->map, REG_ALT(0, info, pin->pin),
> + BIT(p), f << p);
> + regmap_update_bits(info->map, REG_ALT(1, info, pin->pin),
> + BIT(p), (f >> 1) << p);
> + regmap_update_bits(info->map, REG_ALT(2, info, pin->pin),
> + BIT(p), (f >> 2) << p);
> +
I would have thought the hardware would be fixed because you now have 78
pins and this probably will get worse over time. This is really a poor
choice of interface as now you will get two transient states instead of
one.
> + return 0;
> +}
> +
> #define REG(r, info, p) ((r) * (info)->stride + (4 * ((p) / 32)))
>
> static int ocelot_gpio_set_direction(struct pinctrl_dev *pctldev,
> @@ -774,6 +1154,23 @@ static int ocelot_gpio_request_enable(struct pinctrl_dev *pctldev,
> return 0;
> }
>
> +static int lan966x_gpio_request_enable(struct pinctrl_dev *pctldev,
> + struct pinctrl_gpio_range *range,
> + unsigned int offset)
> +{
> + struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> + unsigned int p = offset % 32;
> +
> + regmap_update_bits(info->map, REG_ALT(0, info, offset),
> + BIT(p), 0);
> + regmap_update_bits(info->map, REG_ALT(1, info, offset),
> + BIT(p), 0);
> + regmap_update_bits(info->map, REG_ALT(2, info, offset),
> + BIT(p), 0);
> +
> + return 0;
> +}
> +
> static const struct pinmux_ops ocelot_pmx_ops = {
> .get_functions_count = ocelot_get_functions_count,
> .get_function_name = ocelot_get_function_name,
> @@ -783,6 +1180,15 @@ static const struct pinmux_ops ocelot_pmx_ops = {
> .gpio_request_enable = ocelot_gpio_request_enable,
> };
>
> +static const struct pinmux_ops lan966x_pmx_ops = {
> + .get_functions_count = ocelot_get_functions_count,
> + .get_function_name = ocelot_get_function_name,
> + .get_function_groups = ocelot_get_function_groups,
> + .set_mux = lan966x_pinmux_set_mux,
> + .gpio_set_direction = ocelot_gpio_set_direction,
> + .gpio_request_enable = lan966x_gpio_request_enable,
> +};
> +
> static int ocelot_pctl_get_groups_count(struct pinctrl_dev *pctldev)
> {
> struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> @@ -1074,6 +1480,14 @@ static struct pinctrl_desc sparx5_desc = {
> .npins = ARRAY_SIZE(sparx5_pins),
> .pctlops = &ocelot_pctl_ops,
> .pmxops = &ocelot_pmx_ops,
> +};
> +
> +static struct pinctrl_desc lan966x_desc = {
> + .name = "lan966x-pinctrl",
> + .pins = lan966x_pins,
> + .npins = ARRAY_SIZE(lan966x_pins),
> + .pctlops = &ocelot_pctl_ops,
> + .pmxops = &lan966x_pmx_ops,
> .confops = &ocelot_confops,
> .owner = THIS_MODULE,
> };
> @@ -1114,6 +1528,7 @@ static int ocelot_create_group_func_map(struct device *dev,
> return 0;
> }
>
> +
Useless blank line
> static int ocelot_pinctrl_register(struct platform_device *pdev,
> struct ocelot_pinctrl *info)
> {
> @@ -1337,6 +1752,7 @@ static const struct of_device_id ocelot_pinctrl_of_match[] = {
> { .compatible = "mscc,ocelot-pinctrl", .data = &ocelot_desc },
> { .compatible = "mscc,jaguar2-pinctrl", .data = &jaguar2_desc },
> { .compatible = "microchip,sparx5-pinctrl", .data = &sparx5_desc },
> + { .compatible = "microchip,lan966x-pinctrl", .data = &lan966x_desc },
> {},
> };
>
> --
> 2.17.1
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
> -----Original Message-----
> From: Alexandre Belloni [mailto:[email protected]]
> Sent: Thursday, November 18, 2021 4:30 AM
> To: Kavyasree Kotagiri - I30978 <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; UNGLinuxDriver <[email protected]>
> Subject: Re: [PATCH 2/2] pinctrl: ocelot: Extend support for lan966x
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hello Kavya,
>
> On 29/10/2021 14:57:03+0530, Kavyasree Kotagiri wrote:
> > + LAN966X_PIN(76),
> > + LAN966X_PIN(77),
> > +};
> > +
> > +
>
> One blank line should be removed
>
This is older patch series. Blank lines are already fixed in v3 patch series.
> > static int ocelot_get_functions_count(struct pinctrl_dev *pctldev)
> > {
> > return ARRAY_SIZE(ocelot_function_names);
> > @@ -709,6 +1056,9 @@ static int ocelot_pin_function_idx(struct
> ocelot_pinctrl *info,
> > for (i = 0; i < OCELOT_FUNC_PER_PIN; i++) {
> > if (function == p->functions[i])
> > return i;
> > +
> > + if (function == p->a_functions[i])
> > + return i + OCELOT_FUNC_PER_PIN;
> > }
> >
> > return -1;
> > @@ -744,6 +1094,36 @@ static int ocelot_pinmux_set_mux(struct
> pinctrl_dev *pctldev,
> > return 0;
> > }
> >
> > +static int lan966x_pinmux_set_mux(struct pinctrl_dev *pctldev,
> > + unsigned int selector, unsigned int group)
> > +{
> > + struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> > + struct ocelot_pin_caps *pin = info->desc->pins[group].drv_data;
> > + unsigned int p = pin->pin % 32;
> > + int f;
> > +
> > + f = ocelot_pin_function_idx(info, group, selector);
> > + if (f < 0)
> > + return -EINVAL;
> > +
> > + /*
> > + * f is encoded on three bits.
> > + * bit 0 of f goes in BIT(pin) of ALT[0], bit 1 of f goes in BIT(pin) of
> > + * ALT[1], bit 2 of f goes in BIT(pin) of ALT[2]
> > + * This is racy because both registers can't be updated at the same time
>
> That's three registers, not two so I guess this sentence should be
> reworked.
>
I agree. I will change it.
> > + * but it doesn't matter much for now.
> > + * Note: ALT0/ALT1/ALT2 are organized specially for 78 gpio targets
> > + */
> > + regmap_update_bits(info->map, REG_ALT(0, info, pin->pin),
> > + BIT(p), f << p);
> > + regmap_update_bits(info->map, REG_ALT(1, info, pin->pin),
> > + BIT(p), (f >> 1) << p);
> > + regmap_update_bits(info->map, REG_ALT(2, info, pin->pin),
> > + BIT(p), (f >> 2) << p);
> > +
>
> I would have thought the hardware would be fixed because you now have 78
> pins and this probably will get worse over time. This is really a poor
> choice of interface as now you will get two transient states instead of
> one.
>
Sorry, I couldn't get you. please elaborate what you meant by this comment?
> > + return 0;
> > +}
> > +
> > #define REG(r, info, p) ((r) * (info)->stride + (4 * ((p) / 32)))
> >
> > static int ocelot_gpio_set_direction(struct pinctrl_dev *pctldev,
> > @@ -774,6 +1154,23 @@ static int ocelot_gpio_request_enable(struct
> pinctrl_dev *pctldev,
> > return 0;
> > }
> >
> > +static int lan966x_gpio_request_enable(struct pinctrl_dev *pctldev,
> > + struct pinctrl_gpio_range *range,
> > + unsigned int offset)
> > +{
> > + struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> > + unsigned int p = offset % 32;
> > +
> > + regmap_update_bits(info->map, REG_ALT(0, info, offset),
> > + BIT(p), 0);
> > + regmap_update_bits(info->map, REG_ALT(1, info, offset),
> > + BIT(p), 0);
> > + regmap_update_bits(info->map, REG_ALT(2, info, offset),
> > + BIT(p), 0);
> > +
> > + return 0;
> > +}
> > +
> > static const struct pinmux_ops ocelot_pmx_ops = {
> > .get_functions_count = ocelot_get_functions_count,
> > .get_function_name = ocelot_get_function_name,
> > @@ -783,6 +1180,15 @@ static const struct pinmux_ops ocelot_pmx_ops =
> {
> > .gpio_request_enable = ocelot_gpio_request_enable,
> > };
> >
> > +static const struct pinmux_ops lan966x_pmx_ops = {
> > + .get_functions_count = ocelot_get_functions_count,
> > + .get_function_name = ocelot_get_function_name,
> > + .get_function_groups = ocelot_get_function_groups,
> > + .set_mux = lan966x_pinmux_set_mux,
> > + .gpio_set_direction = ocelot_gpio_set_direction,
> > + .gpio_request_enable = lan966x_gpio_request_enable,
> > +};
> > +
> > static int ocelot_pctl_get_groups_count(struct pinctrl_dev *pctldev)
> > {
> > struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> > @@ -1074,6 +1480,14 @@ static struct pinctrl_desc sparx5_desc = {
> > .npins = ARRAY_SIZE(sparx5_pins),
> > .pctlops = &ocelot_pctl_ops,
> > .pmxops = &ocelot_pmx_ops,
> > +};
> > +
> > +static struct pinctrl_desc lan966x_desc = {
> > + .name = "lan966x-pinctrl",
> > + .pins = lan966x_pins,
> > + .npins = ARRAY_SIZE(lan966x_pins),
> > + .pctlops = &ocelot_pctl_ops,
> > + .pmxops = &lan966x_pmx_ops,
> > .confops = &ocelot_confops,
> > .owner = THIS_MODULE,
> > };
> > @@ -1114,6 +1528,7 @@ static int ocelot_create_group_func_map(struct
> device *dev,
> > return 0;
> > }
> >
> > +
>
> Useless blank line
>
Already fixed in v3 patch series.
> > static int ocelot_pinctrl_register(struct platform_device *pdev,
> > struct ocelot_pinctrl *info)
> > {
> > @@ -1337,6 +1752,7 @@ static const struct of_device_id
> ocelot_pinctrl_of_match[] = {
> > { .compatible = "mscc,ocelot-pinctrl", .data = &ocelot_desc },
> > { .compatible = "mscc,jaguar2-pinctrl", .data = &jaguar2_desc },
> > { .compatible = "microchip,sparx5-pinctrl", .data = &sparx5_desc },
> > + { .compatible = "microchip,lan966x-pinctrl", .data = &lan966x_desc },
> > {},
> > };
> >
> > --
> > 2.17.1
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
On 18/11/2021 09:24:56+0000, [email protected] wrote:
> > > + * but it doesn't matter much for now.
> > > + * Note: ALT0/ALT1/ALT2 are organized specially for 78 gpio targets
> > > + */
> > > + regmap_update_bits(info->map, REG_ALT(0, info, pin->pin),
> > > + BIT(p), f << p);
> > > + regmap_update_bits(info->map, REG_ALT(1, info, pin->pin),
> > > + BIT(p), (f >> 1) << p);
> > > + regmap_update_bits(info->map, REG_ALT(2, info, pin->pin),
> > > + BIT(p), (f >> 2) << p);
> > > +
> >
> > I would have thought the hardware would be fixed because you now have 78
> > pins and this probably will get worse over time. This is really a poor
> > choice of interface as now you will get two transient states instead of
> > one.
> >
> Sorry, I couldn't get you. please elaborate what you meant by this comment?
>
Not mush you can do on your side, this was just a rant. I raised the
issue to Allan, hoping that this will get fixed in the next SoCs ;)
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com