| Message ID | 20220607144740.14937-1-kavyasree.kotagiri@microchip.com |
|---|---|
| Headers | show |
| Series | Add support for lan966x flexcom chip-select configuration | expand |
On 07.06.2022 17:47, Kavyasree Kotagiri wrote: > LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects. > For each chip select of each flexcom there is a configuration > register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of > configuration register is 21 because there are 21 shared pins > on each of which the chip select can be mapped. Each bit of the > register represents a different FLEXCOM_SHARED pin. > > Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com> > --- > v1 -> v2: > - use GENMASK for mask, macros for maximum allowed values. > - use u32 values for flexcom chipselects instead of strings. > - disable clock in case of errors. > > drivers/mfd/atmel-flexcom.c | 93 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 92 insertions(+), 1 deletion(-) > > diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c > index 33caa4fba6af..ac700a85b46f 100644 > --- a/drivers/mfd/atmel-flexcom.c > +++ b/drivers/mfd/atmel-flexcom.c > @@ -28,15 +28,68 @@ > #define FLEX_MR_OPMODE(opmode) (((opmode) << FLEX_MR_OPMODE_OFFSET) & \ > FLEX_MR_OPMODE_MASK) > > +/* LAN966x flexcom shared register offsets */ > +#define FLEX_SHRD_SS_MASK_0 0x0 > +#define FLEX_SHRD_SS_MASK_1 0x4 > +#define FLEX_SHRD_PIN_MAX 20 > +#define FLEX_CS_MAX 1 > +#define FLEX_SHRD_MASK GENMASK(20, 0) > + > +struct atmel_flex_caps { > + bool has_flx_cs; > +}; > + > struct atmel_flexcom { > void __iomem *base; > + void __iomem *flexcom_shared_base; > u32 opmode; > struct clk *clk; > }; > > +static int atmel_flexcom_lan966x_cs_config(struct platform_device *pdev) > +{ > + struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev); > + struct device_node *np = pdev->dev.of_node; > + u32 flx_shrd_pins[2], flx_cs[2], val; > + int err, i, count; > + > + count = of_property_count_u32_elems(np, "microchip,flx-shrd-pins"); > + if (count <= 0 || count > 2) { > + dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd-pins", > + count); > + return -EINVAL; > + } > + > + err = of_property_read_u32_array(np, "microchip,flx-shrd-pins", flx_shrd_pins, count); > + if (err) > + return err; > + > + err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs, count); > + if (err) > + return err; > + > + for (i = 0; i < count; i++) { > + if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX) > + return -EINVAL; > + > + if (flx_cs[i] > FLEX_CS_MAX) > + return -EINVAL; > + > + val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK; > + > + if (flx_cs[i] == 0) > + writel(val, ddata->flexcom_shared_base + FLEX_SHRD_SS_MASK_0); > + else > + writel(val, ddata->flexcom_shared_base + FLEX_SHRD_SS_MASK_1); There is still an open question on this topic from previous version. > + } > + > + return 0; > +} > + > static int atmel_flexcom_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > + const struct atmel_flex_caps *caps; > struct resource *res; > struct atmel_flexcom *ddata; > int err; > @@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct platform_device *pdev) > */ > writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR); > > + caps = of_device_get_match_data(&pdev->dev); > + if (!caps) { > + dev_err(&pdev->dev, "Could not retrieve flexcom caps\n"); > + clk_disable_unprepare(ddata->clk); Could you keep a common path to disable the clock? A goto label something like this: ret = -EINVAL; got clk_disable_unprepare; > + return -EINVAL; > + } > + > + if (caps->has_flx_cs) { > + ddata->flexcom_shared_base = devm_platform_get_and_ioremap_resource(pdev, 1, NULL); > + if (IS_ERR(ddata->flexcom_shared_base)) { > + clk_disable_unprepare(ddata->clk); > + return dev_err_probe(&pdev->dev, > + PTR_ERR(ddata->flexcom_shared_base), > + "failed to get flexcom shared base address\n"); ret = dev_err_probe(...); goto clk_disable_unprepare; > + } > + > + err = atmel_flexcom_lan966x_cs_config(pdev); > + if (err) { > + clk_disable_unprepare(ddata->clk); > + return err; goto clk_disable_unprepare; > + } > + } > + clk_unprepare: > clk_disable_unprepare(ddata->clk); if (ret) return ret; > > return devm_of_platform_populate(&pdev->dev); > } > > +static const struct atmel_flex_caps atmel_flexcom_caps = {}; > + > +static const struct atmel_flex_caps lan966x_flexcom_caps = { > + .has_flx_cs = true, > +}; > + > static const struct of_device_id atmel_flexcom_of_match[] = { > - { .compatible = "atmel,sama5d2-flexcom" }, > + { > + .compatible = "atmel,sama5d2-flexcom", > + .data = &atmel_flexcom_caps, > + }, > + > + { > + .compatible = "microchip,lan966x-flexcom", > + .data = &lan966x_flexcom_caps, > + }, > + > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> > LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects. > > For each chip select of each flexcom there is a configuration > > register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of > > configuration register is 21 because there are 21 shared pins > > on each of which the chip select can be mapped. Each bit of the > > register represents a different FLEXCOM_SHARED pin. > > > > Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com> > > --- > > v1 -> v2: > > - use GENMASK for mask, macros for maximum allowed values. > > - use u32 values for flexcom chipselects instead of strings. > > - disable clock in case of errors. > > > > drivers/mfd/atmel-flexcom.c | 93 > ++++++++++++++++++++++++++++++++++++- > > 1 file changed, 92 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c > > index 33caa4fba6af..ac700a85b46f 100644 > > --- a/drivers/mfd/atmel-flexcom.c > > +++ b/drivers/mfd/atmel-flexcom.c > > @@ -28,15 +28,68 @@ > > #define FLEX_MR_OPMODE(opmode) (((opmode) << > FLEX_MR_OPMODE_OFFSET) & \ > > FLEX_MR_OPMODE_MASK) > > > > +/* LAN966x flexcom shared register offsets */ > > +#define FLEX_SHRD_SS_MASK_0 0x0 > > +#define FLEX_SHRD_SS_MASK_1 0x4 > > +#define FLEX_SHRD_PIN_MAX 20 > > +#define FLEX_CS_MAX 1 > > +#define FLEX_SHRD_MASK GENMASK(20, 0) > > + > > +struct atmel_flex_caps { > > + bool has_flx_cs; > > +}; > > + > > struct atmel_flexcom { > > void __iomem *base; > > + void __iomem *flexcom_shared_base; > > u32 opmode; > > struct clk *clk; > > }; > > > > +static int atmel_flexcom_lan966x_cs_config(struct platform_device *pdev) > > +{ > > + struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev); > > + struct device_node *np = pdev->dev.of_node; > > + u32 flx_shrd_pins[2], flx_cs[2], val; > > + int err, i, count; > > + > > + count = of_property_count_u32_elems(np, "microchip,flx-shrd- > pins"); > > + if (count <= 0 || count > 2) { > > + dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd- > pins", > > + count); > > + return -EINVAL; > > + } > > + > > + err = of_property_read_u32_array(np, "microchip,flx-shrd-pins", > flx_shrd_pins, count); > > + if (err) > > + return err; > > + > > + err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs, > count); > > + if (err) > > + return err; > > + > > + for (i = 0; i < count; i++) { > > + if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX) > > + return -EINVAL; > > + > > + if (flx_cs[i] > FLEX_CS_MAX) > > + return -EINVAL; > > + > > + val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK; > > + > > + if (flx_cs[i] == 0) > > + writel(val, ddata->flexcom_shared_base + > FLEX_SHRD_SS_MASK_0); > > + else > > + writel(val, ddata->flexcom_shared_base + > FLEX_SHRD_SS_MASK_1); > > There is still an open question on this topic from previous version. > https://lore.kernel.org/linux-arm-kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.namprd11.prod.outlook.com/ As part of comments from Peter Rosin - Instead of using mux driver, This patch is introducing new dt-properties in atmel-flexom driver itlself to configure Flexcom shared registers. Based on the chip-select(0 or 1) to be mapped to flexcom shared pin, write to the respective register. If you still have any questions, please comment. > > + } > > + > > + return 0; > > +} > > + > > static int atmel_flexcom_probe(struct platform_device *pdev) > > { > > struct device_node *np = pdev->dev.of_node; > > + const struct atmel_flex_caps *caps; > > struct resource *res; > > struct atmel_flexcom *ddata; > > int err; > > @@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct > platform_device *pdev) > > */ > > writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + > FLEX_MR); > > > > + caps = of_device_get_match_data(&pdev->dev); > > + if (!caps) { > > + dev_err(&pdev->dev, "Could not retrieve flexcom caps\n"); > > + clk_disable_unprepare(ddata->clk); > > Could you keep a common path to disable the clock? A goto label something > like this: > ret = -EINVAL; > got clk_disable_unprepare; > > > + return -EINVAL; > > + } > > + > > + if (caps->has_flx_cs) { > > + ddata->flexcom_shared_base = > devm_platform_get_and_ioremap_resource(pdev, 1, NULL); > > + if (IS_ERR(ddata->flexcom_shared_base)) { > > + clk_disable_unprepare(ddata->clk); > > + return dev_err_probe(&pdev->dev, > > + PTR_ERR(ddata- > >flexcom_shared_base), > > + "failed to get flexcom shared base > address\n"); > ret = dev_err_probe(...); > goto clk_disable_unprepare; > > + } > > + > > + err = atmel_flexcom_lan966x_cs_config(pdev); > > + if (err) { > > + clk_disable_unprepare(ddata->clk); > > + return err; > goto clk_disable_unprepare; > > + } > > + } > > + > > clk_unprepare: > > clk_disable_unprepare(ddata->clk); > if (ret) > return ret; > > > > return devm_of_platform_populate(&pdev->dev); > > } > > > > +static const struct atmel_flex_caps atmel_flexcom_caps = {}; > > + > > +static const struct atmel_flex_caps lan966x_flexcom_caps = { > > + .has_flx_cs = true, > > +}; > > + > > static const struct of_device_id atmel_flexcom_of_match[] = { > > - { .compatible = "atmel,sama5d2-flexcom" }, > > + { > > + .compatible = "atmel,sama5d2-flexcom", > > + .data = &atmel_flexcom_caps, > > + }, > > + > > + { > > + .compatible = "microchip,lan966x-flexcom", > > + .data = &lan966x_flexcom_caps, > > + }, > > + > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
On 08/06/2022 11:31, Kavyasree.Kotagiri@microchip.com wrote: >>> Convert the Atmel flexcom device tree bindings to json schema. >>> >>> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com> >>> --- >>> v1 -> v2: >>> - Fix title. >>> >>> .../bindings/mfd/atmel,flexcom.yaml | 97 +++++++++++++++++++ >>> .../devicetree/bindings/mfd/atmel-flexcom.txt | 63 ------------ >>> 2 files changed, 97 insertions(+), 63 deletions(-) >>> create mode 100644 >> Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml >>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel- >> flexcom.txt >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml >> b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml >>> new file mode 100644 >>> index 000000000000..05cb6ebb4b2a >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml >>> @@ -0,0 +1,97 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/mfd/atmel,flexcom.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Atmel Flexcom (Flexible Serial Communication Unit) >>> + >>> +maintainers: >>> + - Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com> >>> + >>> +description: >>> + The Atmel Flexcom is just a wrapper which embeds a SPI controller, >>> + an I2C controller and an USART. Only one function can be used at a >>> + time and is chosen at boot time according to the device tree. >>> + >>> +properties: >>> + compatible: >>> + const: atmel,sama5d2-flexcom >> >> Same comment applies as before... Your previous set was better here and >> for some reason you decided to change it. This should be enum so you >> avoid useless change next patch. >> > Thanks for your comments. > Do you mean use "enum" instead of "const" in current patch itself and add new compatible in 2/3 patch? Yes. This is how you did it in previous patchsets. Best regards, Krzysztof
On 08.06.2022 11:20, Kavyasree Kotagiri - I30978 wrote: >>> LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects. >>> For each chip select of each flexcom there is a configuration >>> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of >>> configuration register is 21 because there are 21 shared pins >>> on each of which the chip select can be mapped. Each bit of the >>> register represents a different FLEXCOM_SHARED pin. >>> >>> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com> >>> --- >>> v1 -> v2: >>> - use GENMASK for mask, macros for maximum allowed values. >>> - use u32 values for flexcom chipselects instead of strings. >>> - disable clock in case of errors. >>> >>> drivers/mfd/atmel-flexcom.c | 93 >> ++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 92 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c >>> index 33caa4fba6af..ac700a85b46f 100644 >>> --- a/drivers/mfd/atmel-flexcom.c >>> +++ b/drivers/mfd/atmel-flexcom.c >>> @@ -28,15 +28,68 @@ >>> #define FLEX_MR_OPMODE(opmode) (((opmode) << >> FLEX_MR_OPMODE_OFFSET) & \ >>> FLEX_MR_OPMODE_MASK) >>> >>> +/* LAN966x flexcom shared register offsets */ >>> +#define FLEX_SHRD_SS_MASK_0 0x0 >>> +#define FLEX_SHRD_SS_MASK_1 0x4 >>> +#define FLEX_SHRD_PIN_MAX 20 >>> +#define FLEX_CS_MAX 1 >>> +#define FLEX_SHRD_MASK GENMASK(20, 0) >>> + >>> +struct atmel_flex_caps { >>> + bool has_flx_cs; >>> +}; >>> + >>> struct atmel_flexcom { >>> void __iomem *base; >>> + void __iomem *flexcom_shared_base; >>> u32 opmode; >>> struct clk *clk; >>> }; >>> >>> +static int atmel_flexcom_lan966x_cs_config(struct platform_device *pdev) >>> +{ >>> + struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev); >>> + struct device_node *np = pdev->dev.of_node; >>> + u32 flx_shrd_pins[2], flx_cs[2], val; >>> + int err, i, count; >>> + >>> + count = of_property_count_u32_elems(np, "microchip,flx-shrd- >> pins"); >>> + if (count <= 0 || count > 2) { >>> + dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd- >> pins", >>> + count); >>> + return -EINVAL; >>> + } >>> + >>> + err = of_property_read_u32_array(np, "microchip,flx-shrd-pins", >> flx_shrd_pins, count); >>> + if (err) >>> + return err; >>> + >>> + err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs, >> count); >>> + if (err) >>> + return err; >>> + >>> + for (i = 0; i < count; i++) { >>> + if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX) >>> + return -EINVAL; >>> + >>> + if (flx_cs[i] > FLEX_CS_MAX) >>> + return -EINVAL; >>> + >>> + val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK; >>> + >>> + if (flx_cs[i] == 0) >>> + writel(val, ddata->flexcom_shared_base + >> FLEX_SHRD_SS_MASK_0); >>> + else >>> + writel(val, ddata->flexcom_shared_base + >> FLEX_SHRD_SS_MASK_1); >> >> There is still an open question on this topic from previous version. >> > https://lore.kernel.org/linux-arm-kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.namprd11.prod.outlook.com/ "previous version" meant for me this the one at [1]... Another point that the versioning of this series is bad. The question was the following: "I may miss something but I don't see here the approach you introduced in [1]: + err = mux_control_select(flx_mux, args.args[0]); + if (!err) { + mux_control_deselect(flx_mux); " As I had in mind that you said you need mux_control_deselect() because your serial remain blocked otherwise (but I don't find that in the comments of [1]). And I don't see something similar to mux_control_deselect() being called in this patch. [1] https://lore.kernel.org/linux-arm-kernel/5f9fcc33-cc0f-c404-cf7f-cb73f60154ff@microchip.com/ > As part of comments from Peter Rosin - Instead of using mux driver, This patch is introducing > new dt-properties in atmel-flexom driver itlself to configure Flexcom shared registers. > Based on the chip-select(0 or 1) to be mapped to flexcom shared pin, write to the > respective register. > If you still have any questions, please comment. > >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static int atmel_flexcom_probe(struct platform_device *pdev) >>> { >>> struct device_node *np = pdev->dev.of_node; >>> + const struct atmel_flex_caps *caps; >>> struct resource *res; >>> struct atmel_flexcom *ddata; >>> int err; >>> @@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct >> platform_device *pdev) >>> */ >>> writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + >> FLEX_MR); >>> >>> + caps = of_device_get_match_data(&pdev->dev); >>> + if (!caps) { >>> + dev_err(&pdev->dev, "Could not retrieve flexcom caps\n"); >>> + clk_disable_unprepare(ddata->clk); >> >> Could you keep a common path to disable the clock? A goto label something >> like this: >> ret = -EINVAL; >> got clk_disable_unprepare; >> >>> + return -EINVAL; >>> + } >>> + >>> + if (caps->has_flx_cs) { >>> + ddata->flexcom_shared_base = >> devm_platform_get_and_ioremap_resource(pdev, 1, NULL); >>> + if (IS_ERR(ddata->flexcom_shared_base)) { >>> + clk_disable_unprepare(ddata->clk); >>> + return dev_err_probe(&pdev->dev, >>> + PTR_ERR(ddata- >>> flexcom_shared_base), >>> + "failed to get flexcom shared base >> address\n"); >> ret = dev_err_probe(...); >> goto clk_disable_unprepare; >>> + } >>> + >>> + err = atmel_flexcom_lan966x_cs_config(pdev); >>> + if (err) { >>> + clk_disable_unprepare(ddata->clk); >>> + return err; >> goto clk_disable_unprepare; >>> + } >>> + } >>> + >> >> clk_unprepare: >>> clk_disable_unprepare(ddata->clk); >> if (ret) >> return ret; >>> >>> return devm_of_platform_populate(&pdev->dev); >>> } >>> >>> +static const struct atmel_flex_caps atmel_flexcom_caps = {}; >>> + >>> +static const struct atmel_flex_caps lan966x_flexcom_caps = { >>> + .has_flx_cs = true, >>> +}; >>> + >>> static const struct of_device_id atmel_flexcom_of_match[] = { >>> - { .compatible = "atmel,sama5d2-flexcom" }, >>> + { >>> + .compatible = "atmel,sama5d2-flexcom", >>> + .data = &atmel_flexcom_caps, >>> + }, >>> + >>> + { >>> + .compatible = "microchip,lan966x-flexcom", >>> + .data = &lan966x_flexcom_caps, >>> + }, >>> + >>> { /* sentinel */ } >>> }; >>> MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match); >
> >>> LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects. > >>> For each chip select of each flexcom there is a configuration > >>> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of > >>> configuration register is 21 because there are 21 shared pins > >>> on each of which the chip select can be mapped. Each bit of the > >>> register represents a different FLEXCOM_SHARED pin. > >>> > >>> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com> > >>> --- > >>> v1 -> v2: > >>> - use GENMASK for mask, macros for maximum allowed values. > >>> - use u32 values for flexcom chipselects instead of strings. > >>> - disable clock in case of errors. > >>> > >>> drivers/mfd/atmel-flexcom.c | 93 > >> ++++++++++++++++++++++++++++++++++++- > >>> 1 file changed, 92 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c > >>> index 33caa4fba6af..ac700a85b46f 100644 > >>> --- a/drivers/mfd/atmel-flexcom.c > >>> +++ b/drivers/mfd/atmel-flexcom.c > >>> @@ -28,15 +28,68 @@ > >>> #define FLEX_MR_OPMODE(opmode) (((opmode) << > >> FLEX_MR_OPMODE_OFFSET) & \ > >>> FLEX_MR_OPMODE_MASK) > >>> > >>> +/* LAN966x flexcom shared register offsets */ > >>> +#define FLEX_SHRD_SS_MASK_0 0x0 > >>> +#define FLEX_SHRD_SS_MASK_1 0x4 > >>> +#define FLEX_SHRD_PIN_MAX 20 > >>> +#define FLEX_CS_MAX 1 > >>> +#define FLEX_SHRD_MASK GENMASK(20, 0) > >>> + > >>> +struct atmel_flex_caps { > >>> + bool has_flx_cs; > >>> +}; > >>> + > >>> struct atmel_flexcom { > >>> void __iomem *base; > >>> + void __iomem *flexcom_shared_base; > >>> u32 opmode; > >>> struct clk *clk; > >>> }; > >>> > >>> +static int atmel_flexcom_lan966x_cs_config(struct platform_device > *pdev) > >>> +{ > >>> + struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev); > >>> + struct device_node *np = pdev->dev.of_node; > >>> + u32 flx_shrd_pins[2], flx_cs[2], val; > >>> + int err, i, count; > >>> + > >>> + count = of_property_count_u32_elems(np, "microchip,flx-shrd- > >> pins"); > >>> + if (count <= 0 || count > 2) { > >>> + dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd- > >> pins", > >>> + count); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + err = of_property_read_u32_array(np, "microchip,flx-shrd-pins", > >> flx_shrd_pins, count); > >>> + if (err) > >>> + return err; > >>> + > >>> + err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs, > >> count); > >>> + if (err) > >>> + return err; > >>> + > >>> + for (i = 0; i < count; i++) { > >>> + if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX) > >>> + return -EINVAL; > >>> + > >>> + if (flx_cs[i] > FLEX_CS_MAX) > >>> + return -EINVAL; > >>> + > >>> + val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK; > >>> + > >>> + if (flx_cs[i] == 0) > >>> + writel(val, ddata->flexcom_shared_base + > >> FLEX_SHRD_SS_MASK_0); > >>> + else > >>> + writel(val, ddata->flexcom_shared_base + > >> FLEX_SHRD_SS_MASK_1); > >> > >> There is still an open question on this topic from previous version. > >> > > https://lore.kernel.org/linux-arm- > kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.n > amprd11.prod.outlook.com/ > > "previous version" meant for me this the one at [1]... Another point that > the versioning of this series is bad. > > The question was the following: > > "I may miss something but I don't see here the approach you introduced in > [1]: > > + err = mux_control_select(flx_mux, args.args[0]); > + if (!err) { > + mux_control_deselect(flx_mux); > " > > As I had in mind that you said you need mux_control_deselect() because > your > serial remain blocked otherwise (but I don't find that in the comments of > [1]). And I don't see something similar to mux_control_deselect() being > called in this patch. > > [1] > https://lore.kernel.org/linux-arm-kernel/5f9fcc33-cc0f-c404-cf7f- > cb73f60154ff@microchip.com/ > > > As part of comments from Peter Rosin - Instead of using mux driver, This > patch is introducing > > new dt-properties in atmel-flexom driver itlself to configure Flexcom > shared registers. > > Based on the chip-select(0 or 1) to be mapped to flexcom shared pin, write > to the > > respective register. > > If you still have any questions, please comment. > > https://lore.kernel.org/linux-arm-kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.namprd11.prod.outlook.com/ To avoid confusion, I stopped continuing with above patch versioning(mux driver approach). I started new patch series in which I am configuring FLEXCOM_SHARED[0-4]:SS_MASK[0-1] registers in atmel-flexcom.c driver using new DT-properties, mux driver approach is no more followed as suggested by Peter Rosin: " > If you are content with just programming a fixed set of values to > a couple of registers depending on how the board is wired, some > extra DT property on some node related to the flexcom seems like > a better fit than a mux driver. Based on your inputs, I planned to send a new patch with new DT properties introduced in atmel-flexcom.c driver rather than mux driver. Thanks, Kavya " Thanks, Kavya > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> static int atmel_flexcom_probe(struct platform_device *pdev) > >>> { > >>> struct device_node *np = pdev->dev.of_node; > >>> + const struct atmel_flex_caps *caps; > >>> struct resource *res; > >>> struct atmel_flexcom *ddata; > >>> int err; > >>> @@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct > >> platform_device *pdev) > >>> */ > >>> writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + > >> FLEX_MR); > >>> > >>> + caps = of_device_get_match_data(&pdev->dev); > >>> + if (!caps) { > >>> + dev_err(&pdev->dev, "Could not retrieve flexcom caps\n"); > >>> + clk_disable_unprepare(ddata->clk); > >> > >> Could you keep a common path to disable the clock? A goto label > something > >> like this: > >> ret = -EINVAL; > >> got clk_disable_unprepare; > >> > >>> + return -EINVAL; > >>> + } > >>> + > >>> + if (caps->has_flx_cs) { > >>> + ddata->flexcom_shared_base = > >> devm_platform_get_and_ioremap_resource(pdev, 1, NULL); > >>> + if (IS_ERR(ddata->flexcom_shared_base)) { > >>> + clk_disable_unprepare(ddata->clk); > >>> + return dev_err_probe(&pdev->dev, > >>> + PTR_ERR(ddata- > >>> flexcom_shared_base), > >>> + "failed to get flexcom shared base > >> address\n"); > >> ret = dev_err_probe(...); > >> goto clk_disable_unprepare; > >>> + } > >>> + > >>> + err = atmel_flexcom_lan966x_cs_config(pdev); > >>> + if (err) { > >>> + clk_disable_unprepare(ddata->clk); > >>> + return err; > >> goto clk_disable_unprepare; > >>> + } > >>> + } > >>> + > >> > >> clk_unprepare: > >>> clk_disable_unprepare(ddata->clk); > >> if (ret) > >> return ret; > >>> > >>> return devm_of_platform_populate(&pdev->dev); > >>> } > >>> > >>> +static const struct atmel_flex_caps atmel_flexcom_caps = {}; > >>> + > >>> +static const struct atmel_flex_caps lan966x_flexcom_caps = { > >>> + .has_flx_cs = true, > >>> +}; > >>> + > >>> static const struct of_device_id atmel_flexcom_of_match[] = { > >>> - { .compatible = "atmel,sama5d2-flexcom" }, > >>> + { > >>> + .compatible = "atmel,sama5d2-flexcom", > >>> + .data = &atmel_flexcom_caps, > >>> + }, > >>> + > >>> + { > >>> + .compatible = "microchip,lan966x-flexcom", > >>> + .data = &lan966x_flexcom_caps, > >>> + }, > >>> + > >>> { /* sentinel */ } > >>> }; > >>> MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match); > >
On 09.06.2022 16:34, Kavyasree Kotagiri - I30978 wrote: > >>>>>> LAN966x SoC have 5 flexcoms. Each flexcom has 2 chip-selects. >>>>>> For each chip select of each flexcom there is a configuration >>>>>> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of >>>>>> configuration register is 21 because there are 21 shared pins >>>>>> on each of which the chip select can be mapped. Each bit of the >>>>>> register represents a different FLEXCOM_SHARED pin. >>>>>> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com> >>>>>> --- >>>>>> v1 -> v2: >>>>>> - use GENMASK for mask, macros for maximum allowed values. >>>>>> - use u32 values for flexcom chipselects instead of strings. >>>>>> - disable clock in case of errors. >>>>>> drivers/mfd/atmel-flexcom.c | 93 >>>>> ++++++++++++++++++++++++++++++++++++- >>>>>> 1 file changed, 92 insertions(+), 1 deletion(-) >>>>>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c >>>>>> index 33caa4fba6af..ac700a85b46f 100644 >>>>>> --- a/drivers/mfd/atmel-flexcom.c >>>>>> +++ b/drivers/mfd/atmel-flexcom.c >>>>>> @@ -28,15 +28,68 @@ >>>>>> #define FLEX_MR_OPMODE(opmode) (((opmode) << >>>>> FLEX_MR_OPMODE_OFFSET) & \ >>>>>> FLEX_MR_OPMODE_MASK) >>>>>> +/* LAN966x flexcom shared register offsets */ >>>>>> +#define FLEX_SHRD_SS_MASK_0 0x0 >>>>>> +#define FLEX_SHRD_SS_MASK_1 0x4 >>>>>> +#define FLEX_SHRD_PIN_MAX 20 >>>>>> +#define FLEX_CS_MAX 1 >>>>>> +#define FLEX_SHRD_MASK GENMASK(20, 0) >>>>>> + >>>>>> +struct atmel_flex_caps { >>>>>> + bool has_flx_cs; >>>>>> +}; >>>>>> + >>>>>> struct atmel_flexcom { >>>>>> void __iomem *base; >>>>>> + void __iomem *flexcom_shared_base; >>>>>> u32 opmode; >>>>>> struct clk *clk; >>>>>> }; >>>>>> +static int atmel_flexcom_lan966x_cs_config(struct platform_device >>> *pdev) >>>>>> +{ >>>>>> + struct atmel_flexcom *ddata = dev_get_drvdata(&pdev->dev); >>>>>> + struct device_node *np = pdev->dev.of_node; >>>>>> + u32 flx_shrd_pins[2], flx_cs[2], val; >>>>>> + int err, i, count; >>>>>> + >>>>>> + count = of_property_count_u32_elems(np, "microchip,flx-shrd- >>>>> pins"); >>>>>> + if (count <= 0 || count > 2) { >>>>>> + dev_err(&pdev->dev, "Invalid %s property (%d)\n", "flx-shrd- >>>>> pins", >>>>>> + count); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + err = of_property_read_u32_array(np, "microchip,flx-shrd-pins", >>>>> flx_shrd_pins, count); >>>>>> + if (err) >>>>>> + return err; >>>>>> + >>>>>> + err = of_property_read_u32_array(np, "microchip,flx-cs", flx_cs, >>>>> count); >>>>>> + if (err) >>>>>> + return err; >>>>>> + >>>>>> + for (i = 0; i < count; i++) { >>>>>> + if (flx_shrd_pins[i] > FLEX_SHRD_PIN_MAX) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + if (flx_cs[i] > FLEX_CS_MAX) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + val = ~(1 << flx_shrd_pins[i]) & FLEX_SHRD_MASK; >>>>>> + >>>>>> + if (flx_cs[i] == 0) >>>>>> + writel(val, ddata->flexcom_shared_base + >>>>> FLEX_SHRD_SS_MASK_0); >>>>>> + else >>>>>> + writel(val, ddata->flexcom_shared_base + >>>>> FLEX_SHRD_SS_MASK_1); >>>>> There is still an open question on this topic from previous version. >>>> https://lore.kernel.org/linux-arm- >>> kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.n >>> amprd11.prod.outlook.com/ >>> "previous version" meant for me this the one at [1]... Another point that >>> the versioning of this series is bad. >>> The question was the following: >>> "I may miss something but I don't see here the approach you introduced in >>> [1]: >>> + err = mux_control_select(flx_mux, args.args[0]); >>> + if (!err) { >>> + mux_control_deselect(flx_mux); >>> " >>> As I had in mind that you said you need mux_control_deselect() because >>> your >>> serial remain blocked otherwise (but I don't find that in the comments of >>> [1]). And I don't see something similar to mux_control_deselect() being >>> called in this patch. >>> [1] >>> https://lore.kernel.org/linux-arm-kernel/5f9fcc33-cc0f-c404-cf7f- >>> cb73f60154ff@microchip.com/ >>>> As part of comments from Peter Rosin - Instead of using mux driver, This >>> patch is introducing >>>> new dt-properties in atmel-flexom driver itlself to configure Flexcom >>> shared registers. >>>> Based on the chip-select(0 or 1) to be mapped to flexcom shared pin, write >>> to the >>>> respective register. >>>> If you still have any questions, please comment. >> https://lore.kernel.org/linux-arm-kernel/PH0PR11MB48724DE09A50D67F1EA9FBE092D89@PH0PR11MB4872.namprd11.prod.outlook.com/ >> To avoid confusion, I stopped continuing with above patch versioning(mux driver approach). >> I started new patch series in which I am configuring FLEXCOM_SHARED[0-4]:SS_MASK[0-1] >> registers in atmel-flexcom.c driver using new DT-properties, mux driver approach is no more followed >> as suggested by Peter Rosin: >> " >>> If you are content with just programming a fixed set of values to >>> a couple of registers depending on how the board is wired, some >>> extra DT property on some node related to the flexcom seems like >>> a better fit than a mux driver. >> Based on your inputs, I planned to send a new patch with new DT properties >> introduced in atmel-flexcom.c driver rather than mux driver. >> >> Thanks, >> Kavya >> " >> >> Thanks, >> Kavya > > Hi Claudiu, > > Please let me know if you still have any comments. If not, I will send my v3 with clk_disable_unprepare moved to goto and some minor fixes(irq flags) in dt-bindings. I got it now after the talk we had on internal chat. Please go with v3. Thank you, Claudiu Beznea > >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> static int atmel_flexcom_probe(struct platform_device *pdev) >>>>>> { >>>>>> struct device_node *np = pdev->dev.of_node; >>>>>> + const struct atmel_flex_caps *caps; >>>>>> struct resource *res; >>>>>> struct atmel_flexcom *ddata; >>>>>> int err; >>>>>> @@ -76,13 +129,51 @@ static int atmel_flexcom_probe(struct >>>>> platform_device *pdev) >>>>>> */ >>>>>> writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + >>>>> FLEX_MR); >>>>>> + caps = of_device_get_match_data(&pdev->dev); >>>>>> + if (!caps) { >>>>>> + dev_err(&pdev->dev, "Could not retrieve flexcom caps\n"); >>>>>> + clk_disable_unprepare(ddata->clk); >>>>> Could you keep a common path to disable the clock? A goto label >>> something >>>>> like this: >>>>> ret = -EINVAL; >>>>> got clk_disable_unprepare; >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + if (caps->has_flx_cs) { >>>>>> + ddata->flexcom_shared_base = >>>>> devm_platform_get_and_ioremap_resource(pdev, 1, NULL); >>>>>> + if (IS_ERR(ddata->flexcom_shared_base)) { >>>>>> + clk_disable_unprepare(ddata->clk); >>>>>> + return dev_err_probe(&pdev->dev, >>>>>> + PTR_ERR(ddata- >>>>>> flexcom_shared_base), >>>>>> + "failed to get flexcom shared base >>>>> address\n"); >>>>> ret = dev_err_probe(...); >>>>> goto clk_disable_unprepare; >>>>>> + } >>>>>> + >>>>>> + err = atmel_flexcom_lan966x_cs_config(pdev); >>>>>> + if (err) { >>>>>> + clk_disable_unprepare(ddata->clk); >>>>>> + return err; >>>>> goto clk_disable_unprepare; >>>>>> + } >>>>>> + } >>>>>> + >>>>> clk_unprepare: >>>>>> clk_disable_unprepare(ddata->clk); >>>>> if (ret) >>>>> return ret; >>>>>> return devm_of_platform_populate(&pdev->dev); >>>>>> } >>>>>> +static const struct atmel_flex_caps atmel_flexcom_caps = {}; >>>>>> + >>>>>> +static const struct atmel_flex_caps lan966x_flexcom_caps = { >>>>>> + .has_flx_cs = true, >>>>>> +}; >>>>>> + >>>>>> static const struct of_device_id atmel_flexcom_of_match[] = { >>>>>> - { .compatible = "atmel,sama5d2-flexcom" }, >>>>>> + { >>>>>> + .compatible = "atmel,sama5d2-flexcom", >>>>>> + .data = &atmel_flexcom_caps, >>>>>> + }, >>>>>> + >>>>>> + { >>>>>> + .compatible = "microchip,lan966x-flexcom", >>>>>> + .data = &lan966x_flexcom_caps, >>>>>> + }, >>>>>> + >>>>>> { /* sentinel */ } >>>>>> }; >>>>>> MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
On 16/06/2022 02:20, Kavyasree.Kotagiri@microchip.com wrote: >> >> Yes. This is how you did it in previous patchsets. >> > I did so in v3 series, but below errors are reported on 1/3 patch: > dtschema/dtc warnings/errors: > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml: properties:compatible:enum: 'atmel,sama5d2-flexcom' is not of type 'array' I don't remember it but it's a simple fix of syntax. Documentation/devicetree/bindings/arm/arm,cci-400.yaml Best regards, Krzysztof
This patch series converts atmel-flexcom bindings into json-schema format. Adds support for lan966x flexcom chip-select configurations and its DT bindings. v1 -> v2: - minor fix in title of dt-bindings. - Modified new dt properties usage in atmel,flexcom.yaml. - Used GENMASK and macros for maximum allowed values. - Use u32 values for flexcom chipselects instead of strings. - disable clock in case of errors. Kavyasree Kotagiri (3): dt-bindings: mfd: atmel,flexcom: Convert to json-schema dt-bindings: mfd: atmel,flexcom: Add new compatible string for lan966x mfd: atmel-flexcom: Add support for lan966x flexcom chip-select configuration .../bindings/mfd/atmel,flexcom.yaml | 134 ++++++++++++++++++ .../devicetree/bindings/mfd/atmel-flexcom.txt | 63 -------- drivers/mfd/atmel-flexcom.c | 93 +++++++++++- 3 files changed, 226 insertions(+), 64 deletions(-) create mode 100644 Documentation/devicetree/bindings/mfd/atmel,flexcom.yaml delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-flexcom.txt