lkml.org 
[lkml]   [2022]   [Aug]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFC PATCH 1/1] pinctrl: rockchip: add support for per-pinmux io-domain dependency
    From
    Hi Quentin,

    Thank you for your efforts! This will solve several issues that are
    bound to pop up if a board deviates from the Rockchip reference design.
    It seems this does not happen very often, though, which would explain
    the lack of responses to your initial query... :-/

    A few comments inline:

    On 8/2/22 11:52, Quentin Schulz wrote:
    > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
    >
    > On some Rockchip SoCs, some SoC pins are split in what are called IO
    > domains.
    >
    > An IO domain is supplied power externally, by regulators from a PMIC for
    > example. This external power supply is then used by the IO domain as
    > "supply" for the IO pins if they are outputs.
    >
    > Each IO domain can configure which voltage the IO pins will be operating
    > on (1.8V or 3.3V).

    Just for the sake of completeness: 2.5V is possibly as well (at least on
    RK356x), right?

    > There already exists an IO domain driver for Rockchip SoCs[1]. This
    > driver allows to explicit the relationship between the external power

    ...allows to model explicitly...?

    > supplies and IO domains[2]. This makes sure the regulators are enabled
    > by the Linux kernel so the IO domains are supplied with power and
    > correctly configured as per the supplied voltage.
    > This driver is a regulator consumer and does not offer any other
    > interface for device dependency.
    >
    > However, IO pins belonging to an IO domain need to have this IO domain
    > correctly configured before they are being used otherwise they do not
    > operate correctly (in our case, a pin configured as output clock was
    > oscillating between 0 and 150mV instead of the expected 1V8).
    >
    > In order to make this dependency transparent to the consumer of those
    > pins and not add Rockchip-specific code to third party drivers (a camera
    > driver in our case), it is hooked into the pinctrl driver which is
    > Rockchip-specific obviously.

    This approach seems reasonable. But just for my understanding: Does this
    mean we need to edit e.g. rk3568-pinctrl.dtsi, iterate over all entries,
    and add rockchip,iodomains = <&corresponding_io_domain>;?

    If not, at which place are the rockchip,iodomains properties inserted?

    > [1] drivers/soc/rockchip/io-domain.c
    > [2] Documentation/devicetree/bindings/power/rockchip-io-domain.yaml
    >
    > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
    > ---
    > drivers/pinctrl/pinctrl-rockchip.c | 19 +++++++++++++++++++
    > drivers/pinctrl/pinctrl-rockchip.h | 1 +
    > 2 files changed, 20 insertions(+)
    >
    > diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
    > index 32e41395fc76..c3c2801237b5 100644
    > --- a/drivers/pinctrl/pinctrl-rockchip.c
    > +++ b/drivers/pinctrl/pinctrl-rockchip.c
    > @@ -24,6 +24,8 @@
    > #include <linux/of_address.h>
    > #include <linux/of_device.h>
    > #include <linux/of_irq.h>
    > +#include <linux/of_platform.h>
    > +#include <linux/platform_device.h>
    > #include <linux/pinctrl/machine.h>
    > #include <linux/pinctrl/pinconf.h>
    > #include <linux/pinctrl/pinctrl.h>
    > @@ -2370,6 +2372,12 @@ static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
    > dev_dbg(dev, "enable function %s group %s\n",
    > info->functions[selector].name, info->groups[group].name);
    >
    > + if (info->groups[group].io_domain &&
    > + !platform_get_drvdata(info->groups[group].io_domain)) {
    > + dev_err(info->dev, "IO domain device is required but not probed yet, deferring...");

    Probably this has been left in there for debugging, but should be
    removed to avoid spamming dmesg. IIUC this condition could occur several
    times.

    > + return -EPROBE_DEFER;
    > + }
    > +
    > /*
    > * for each pin in the pin group selected, program the corresponding
    > * pin function number in the config register.
    > @@ -2663,6 +2671,7 @@ static int rockchip_pinctrl_parse_groups(struct device_node *np,
    > {
    > struct device *dev = info->dev;
    > struct rockchip_pin_bank *bank;
    > + struct device_node *node;
    > int size;
    > const __be32 *list;
    > int num;
    > @@ -2684,6 +2693,16 @@ static int rockchip_pinctrl_parse_groups(struct device_node *np,
    > if (!size || size % 4)
    > return dev_err_probe(dev, -EINVAL, "wrong pins number or pins and configs should be by 4\n");
    >
    > + node = of_parse_phandle(np, "rockchip,io-domains", 0);
    > + if (node) {
    > + grp->io_domain = of_find_device_by_node(node);
    > + of_node_put(node);
    > + if (!grp->io_domain) {
    > + dev_err(info->dev, "couldn't find IO domain device\n");
    > + return -ENODEV;

    Again just for my understanding: The property is optional in order to
    provide compatibility with older device trees, right?

    > + }
    > + }
    > +
    > grp->npins = size / 4;
    >
    > grp->pins = devm_kcalloc(dev, grp->npins, sizeof(*grp->pins), GFP_KERNEL);
    > diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
    > index ec46f8815ac9..56bc008eb7df 100644
    > --- a/drivers/pinctrl/pinctrl-rockchip.h
    > +++ b/drivers/pinctrl/pinctrl-rockchip.h
    > @@ -434,6 +434,7 @@ struct rockchip_pin_group {
    > unsigned int npins;
    > unsigned int *pins;
    > struct rockchip_pin_config *data;
    > + struct platform_device *io_domain;
    > };
    >
    > /**

    Looking forward to the v1 of this series, which I'd be happy to test.

    Best regards,
    Michael

    \
     
     \ /
      Last update: 2022-08-11 09:53    [W:6.261 / U:0.136 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site