Re: [PATCH RESEND 1/2] pinctrl: ns2: add pinmux driver support for Broadcom NS2 SoC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Linus, Reddy,

On 4/17/2016 8:34 PM, Yendapally Reddy Dhananjaya Reddy wrote:
Hi Linus,

On Thu, Apr 14, 2016 at 3:12 PM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
On Thu, Apr 14, 2016 at 9:53 AM, Yendapally Reddy Dhananjaya Reddy
<yendapally.reddy@xxxxxxxxxxxx> wrote:
On Wed, Apr 13, 2016 at 6:49 PM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
On Tue, Mar 29, 2016 at 5:22 PM, Yendapally Reddy Dhananjaya Reddy
<yendapally.reddy@xxxxxxxxxxxx> wrote:

+static const unsigned int gpio_0_1_pins[] = {24, 25};
+static const unsigned int pwm_0_pins[] = {24};
+static const unsigned int pwm_1_pins[] = {25};

So either the same pins are used for GPIO or PWM.
And this pattern persists.

Do you have a brewing GPIO driver for this block as well?
Is it a separate front-end calling to pinctrl using the
pinctrl_gpio_* calls or do you plan to patch it into this
driver?

This is more of a question.


This SoC supports group based configuration and there is a top level register
to select groups. Once the gpio_0_1_pins group is selected, there is one more
register to select between gpio_0_1 and pwm (only four pins). The pins
24 and 25 are shared between nor pins and gpio at the top group level. Once
gpio group is selected, then we can select to be either gpio or pwm. I missed
these two pins to be added to nor_data_pins and will add in the next version.

static const unsigned nor_data_pins[] =  {0, 1, 2, 3, 4, 5, 6, 7, 8, 9,
        10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25};

NS2_PIN_GROUP(nand, 0, 0, 31, 1, 0),
NS2_PIN_GROUP(nor_data, 0, 0, 31, 1, 1),
NS2_PIN_GROUP(gpio_0_1, 0, 0, 31, 1, 0),

To select PWM, we need to select gpio and pwm as well.

gpio: gpio {
function = "gpio";
groups = "gpio_0_1_grp";

pwm: pwm {
function = "pwm";
groups = "pwm0_grp", "pwm1_grp";
};

Already available gpio driver "pinctrl-iproc-gpio.c" will be the gpio driver
for this soc as well without pin request.

Then you are doing something wrong. Look in pinctrl-iproc-gpio.c:

/*
 * Request the Iproc IOMUX pinmux controller to mux individual pins to GPIO
 */
static int iproc_gpio_request(struct gpio_chip *gc, unsigned offset)
{
        struct iproc_gpio *chip = gpiochip_get_data(gc);
        unsigned gpio = gc->base + offset;

        /* not all Iproc GPIO pins can be muxed individually */
        if (!chip->pinmux_is_supported)
                return 0;

NS2 does not support GPIO pins to be muxed individually. The call to iproc_gpio_request should be rejected for NS2 since 'chip->pinmux_is_supported' is false.


        return pinctrl_request_gpio(gpio);
}

static void iproc_gpio_free(struct gpio_chip *gc, unsigned offset)
{
        struct iproc_gpio *chip = gpiochip_get_data(gc);
        unsigned gpio = gc->base + offset;

        if (!chip->pinmux_is_supported)
                return;

Same here


        pinctrl_free_gpio(gpio);
}

So as you see pinctrl_request_gpio() and pinctrl_free_gpio()
are being called.

These will in turn call pinmux_request_gpio() and
pinmux_free_gpio() to make the backing pin controller
mux in the pin as GPIO.

pinmux_request_gpio() will end up in pin_request()
and at this point:

if (gpio_range && ops->gpio_request_enable)
                /* This requests and enables a single GPIO pin */
                status = ops->gpio_request_enable(pctldev, gpio_range, pin);

As you can see: it will attempt to call the .gpio_request_enable()
method of your struct pinmux_ops.

But your pinmux ops look like this:

+static struct pinmux_ops ns2_pinmux_ops = {
+       .get_functions_count = ns2_get_functions_count,
+       .get_function_name = ns2_get_function_name,
+       .get_function_groups = ns2_get_function_groups,
+       .set_mux = ns2_pinmux_enable,
+};

I.e. there is no way that GPIO can be set up as a GPIO line,
and you're relying on some other pin control entries in the
device tree to do that, which is unnecessarily complicated.

Correct, since NS2 can only support group based GPIO configuration, the above functions will be rejected because "pinmux_is_supported" is false:

chip->pinmux_is_supported = of_property_read_bool(dev->of_node, "gpio-ranges");

Here, "gpio-ranges" should never be defined on NS2 DT files.


Please consider implementing the .gpio_request_enable callback
for this pin multiplexer.

Only 4 GPIO pins are pin configurable. The other GPIO pins are group
controlled as mentioned in the NS2_PIN_GROUP. A single gpio request will affect
the other pins in that particular group as the control is available
to select either "gpio group" or the other. As shown below
0x00 selects "gpio_6_7_pins" together
0x01 selects "pcie_a3_clk_wak_pins"
0x02 selects "nor_addr_4_5_pins"

static const unsigned gpio_6_7_pins[] = {31, 32};
static const unsigned pcie_a3_clk_wak_pins[] = {31, 32};
static const unsigned nor_addr_4_5_pins[] = {31, 32};

I tested with not providing the "ranges" property in the gpio node. The
"iproc_gpio_request" and "iproc_gpio_free" functions returned back as
"rages" property is not defined.

        if (!chip->pinmux_is_supported)
                return 0;

We can support the "iproc_gpio_request" and "iproc_gpio_free" functions for the
4 gpios that share the pwm pins only. Please suggest me here.

Thanks,
Dhananjay


Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux