Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752722AbcDRSY7 (ORCPT ); Mon, 18 Apr 2016 14:24:59 -0400 Received: from mail-gw2-out.broadcom.com ([216.31.210.63]:34028 "EHLO mail-gw2-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751560AbcDRSY5 (ORCPT ); Mon, 18 Apr 2016 14:24:57 -0400 X-IronPort-AV: E=Sophos;i="5.24,503,1455004800"; d="scan'208";a="93521617" Subject: Re: [PATCH RESEND 1/2] pinctrl: ns2: add pinmux driver support for Broadcom NS2 SoC To: Yendapally Reddy Dhananjaya Reddy , Linus Walleij References: Cc: Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Florian Fainelli , Ray Jui , Scott Branden , Catalin Marinas , Will Deacon , Pramod Kumar , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , bcm-kernel-feedback-list , "linux-arm-kernel@lists.infradead.org" , "linux-gpio@vger.kernel.org" , Paul Gortmaker From: Ray Jui Message-ID: Date: Mon, 18 Apr 2016 11:24:47 -0700 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5696 Lines: 161 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 wrote: >> On Thu, Apr 14, 2016 at 9:53 AM, Yendapally Reddy Dhananjaya Reddy >> wrote: >>> On Wed, Apr 13, 2016 at 6:49 PM, Linus Walleij wrote: >>>> On Tue, Mar 29, 2016 at 5:22 PM, Yendapally Reddy Dhananjaya Reddy >>>> 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