Re: [RFC PATCH 2/3] watchdog: da9062: avoid regmap in restart handler
On Sun, Oct 07, 2018 at 05:39:36PM +0200, Stefan Lengfeld wrote: > Using i2c_transfer() directly to set the shutdown bit is more reliable > than using regmap in atomic contexts, because calls to 'schedule()' or > 'sleep()' must be avoided in call code paths. > #include > #include > #include > +#include > #include > #include > #include The rule of thumb when extending a header inclusion block is that try to squeeze the new inclusion in the longest sorted part of the list. Rationale behind is easy maintenance. -- With Best Regards, Andy Shevchenko
Re: [RFC PATCH 1/3] i2c: imx: implement master_xfer_irqless callback
On Sun, Oct 07, 2018 at 05:39:35PM +0200, Stefan Lengfeld wrote: > Rework the read and write code paths in the driver to support operation > in IRQ disabled contexts. The patch is currently tested only on a > phyCORE-i.MX6 Solo board. > > The driver supports normal operation, DMA transfers and now the polling > mode or also called sleep-free or IRQ-less operation. It makes the code > not simpler or easier to read, but IRQ less I2C transfers are needed on > some hardware configurations, e.g. to trigger reboots on an external > PMIC chip. > + if (!polling) > + schedule(); > + else > + udelay(100); What the point to use negative conditional when it's pretty straightforward and positive may be used? -- With Best Regards, Andy Shevchenko
Re: [PATCH v5] usb: gadget: udc: renesas_usb3: Add register of usb role switch
On Fri, May 25, 2018 at 10:12 AM, Yoshihiro Shimoda
<yoshihiro.shimoda...@renesas.com> wrote:
> -static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
Wouldn't be better to choose another name for a new function?
> + struct renesas_usb3 *usb3 = container_of(work, struct renesas_usb3,
> +role_work);
Matter of style, though I would rather put entire container_of() on
the next line (see for the existing style in the module and use it).
> + /* This device_attach() might sleep */
> + if (device_attach(host) < 0)
> + dev_err(dev, "device_attach(usb3_port) failed\n");
can't be "usb3_port" part derived from the host variable somehow and
to some extend?
> + usb3->role_sw = usb_role_switch_register(>dev,
> + _usb3_role_switch_desc);
> + if (!IS_ERR(usb3->role_sw)) {
> + usb3->host_dev = usb_of_get_companion_dev(>dev);
Hmm... Can it possible return -EPROBE_DEFER? If so, would it be better
to use other approach to handle it?
> + if (IS_ERR_OR_NULL(usb3->host_dev)) {
> + /* If not found, this driver will not use a role sw */
> + usb_role_switch_unregister(usb3->role_sw);
> + usb3->role_sw = NULL;
> + }
> + } else {
> + usb3->role_sw = NULL;
> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v5] gpio: dwapb: Add support for 1 interrupt per port A GPIO
On Thu, Apr 26, 2018 at 7:19 PM, Phil Edworthy
<phil.edwor...@renesas.com> wrote:
Sotty fo a late response. Consider follow up fixes for below.
> if (!pp->irq_shared) {
> + int i;
> +
> + for (i = 0; i < pp->ngpio; i++) {
> + if (pp->irq[i])
> + irq_set_chained_handler_and_data(pp->irq[i],
> + dwapb_irq_handler, gpio);
> + }
> } else {
> /*
> * Request a shared IRQ since where MFD would have devices
> * using the same irq pin
> */
> + err = devm_request_irq(gpio->dev, pp->irq[0],
>dwapb_irq_handler_mfd,
>IRQF_SHARED, "gpio-dwapb-mfd", gpio);
> + if (pp->has_irq)
> dwapb_configure_irqs(gpio, port, pp);
I would rather make irq array a type of signed int and move
conditional into the function to test per IRQ based.
> /* Add GPIO-signaled ACPI event support */
> + if (pp->has_irq)
> acpi_gpiochip_request_interrupts(>gc);
Perhaps something similar.
> if (dev->of_node && pp->idx == 0 &&
> fwnode_property_read_bool(fwnode,
> "interrupt-controller")) {
> + struct device_node *np = to_of_node(fwnode);
> + unsigned int j;
> +
> + /*
> +* The IP has configuration options to allow a single
> +* combined interrupt or one per gpio. If one per
> gpio,
> +* some might not be used.
> +*/
> + for (j = 0; j < pp->ngpio; j++) {
> + int irq = of_irq_get(np, j);
> + if (irq < 0)
> + continue;
> +
> + pp->irq[j] = irq;
> + pp->has_irq = true;
> + }
for (...)
pp->irq = of_irq_get();
> }
> + if (has_acpi_companion(dev) && pp->idx == 0) {
> + unsigned int j;
> +
> + for (j = 0; j < pp->ngpio; j++) {
> + pp->irq[j] =
> platform_get_irq(to_platform_device(dev), j);
> + if (pp->irq[j])
> + pp->has_irq = true;
> + }
Ditto.
Moreover you have a bug here. See my proposal at the top of this message.
And now even better to ask, why platform_get_irq() wouldn't work for DT case?
> +
> + if (!pp->has_irq)
> dev_warn(dev, "no irq for port%d\n", pp->idx);
This could be issued in the actual function which will try to allocate
IRQs (perhaps on debug level)
P.S. Just think about it, perhaps you find even better solutions.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] usb: role: rcar-usb3-role-switch: add support for R-Car SoCs
On Tue, Apr 10, 2018 at 3:03 PM, Yoshihiro Shimoda
<yoshihiro.shimoda...@renesas.com> wrote:
> This patch adds role switch support for R-Car SoCs. Some R-Car SoCs
> (e.g. R-Car H3) have USB 3.0 dual-role device controller which has
> the USB 3.0 xHCI host and Renesas USB 3.0 peripheral.
>
> Unfortunately, the mode change register contains the USB 3.0 peripheral
> controller side only. So, the USB 3.0 peripheral driver (renesas_usb3)
> manages this register. However, in peripheral mode, the host should
> stop. Also the host hardware needs to reinitialize its own registers
> when the mode changes from peripheral to host mode. Otherwise,
> the host cannot work correctly (e.g. detect a device as high-speed).
>
> To achieve this by a driver, this role switch driver manages
> the mode change register and attach/release the xhci-plat driver.
> The renesas_usb3 udc driver should call devm_of_platform_populate()
> to probe this driver.
> +#include
> +#include
> +#include
> +#include
Do you need this one?
> +#include
> +#include
> +#include
> +#include
> +#include
> +static const struct of_device_id rcar_usb3_role_switch_of_match[] = {
> + { .compatible = "renesas,rcar-usb3-role-switch" },
> + { },
Better to remove comma at terminator line.
> +};
> +MODULE_DEVICE_TABLE(of, rcar_usb3_role_switch_of_match);
> +static int rcar_usb3_role_switch_probe(struct platform_device *pdev)
> +{
> + /* Avoid devm_request_mem_region() calling by devm_ioremap_resource()
> */
Redundant comment.
> + host_node = of_parse_phandle(pdev->dev.of_node, "renesas,host", 0);
> + if (!host_node)
> + return -ENODEV;
> +
> + pdev_host = of_find_device_by_node(host_node);
> + if (!pdev_host)
> + return -ENODEV;
> +
> + of_node_put(host_node);
Isn't what Heikki tried to solve with graphs and stuff like that?
> + dev_info(>dev, "probed\n");
Noise. (Hint: initcall_debug is a good command line option)
> +}
> +#ifdef CONFIG_PM_SLEEP
Instead...
> +static int rcar_usb3_role_switch_suspend(struct device *dev)
...use __maybe_unused annotation.
> +static int rcar_usb3_role_switch_resume(struct device *dev)
Ditto.
> +#endif
> +static struct platform_driver rcar_usb3_role_switch_driver = {
> + .probe = rcar_usb3_role_switch_probe,
> + .remove = rcar_usb3_role_switch_remove,
> + .driver = {
> + .name = "rcar_usb3_role_switch",
> + .pm = _usb3_role_switch_pm_ops,
> + .of_match_table =
> of_match_ptr(rcar_usb3_role_switch_of_match),
Is it possible to have this driver w/o OF? Does it make sense in that case?
Otherwise remove of_match_ptr() call and below modalias.
> + },
> +};
> +MODULE_ALIAS("platform:rcar_usb3_role_switch");
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] gpio: dwapb: Add support for 32 interrupts
On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy
<phil.edwor...@renesas.com> wrote:
> The DesignWare GPIO IP can be configured for either 1 or 32 interrupts,
1 to 32, or just a choice between two?
> but the driver currently only supports 1 interrupt. See the DesignWare
> DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter.
Will see after holiday and perhaps make more comments. Here is just a
brief review.
> +- interrupts : The interrupts to the parent controller raised when GPIOs
> + generate the interrupts. If the controller provides one combined interrupt
> + for all GPIOs, specify a single interrupt. If the controller provides one
> + interrupt for each GPIO, provide a list of interrupts that correspond to
> each
> + of the GPIO pins. When specifying multiple interrupts, if any of the GPIOs
> are
> + not connected to an interrupt, use the interrupt-mask property.
> +- interrupt-mask : a 32-bit bit mask that specifies which interrupts in the
> list
> + of interrupts is valid, bit is 1 for a valid irq.
So, but why one will need that in practice? GPIO driver usually
provides a pin based IRQ chip which maps each pin to the corresponding
offset inside specific IRQ domain.
> + struct device_node *np = to_of_node(fwnode);
> + u32 irq_mask = 0x;
Why? Shouldn't it be dependent to the amount of actual pins / ports?
Intel Quark has only 8 AFAIR.
> + int j;
> +
> + /* Optional irq mask */
> + fwnode_property_read_u32(fwnode, "interrupt-mask",
> _mask);
> +
> + /*
> +* The IP has configuration options to allow a single
> +* combined interrupt or one per gpio. If one per
> gpio,
> +* some might not be used.
> +*/
> + for (j = 0; j < pp->ngpio; j++) {
> + if (irq_mask & BIT(j)) {
for_each_set_bit() is in kernel for ages!
> + pp->irq[j] = irq_of_parse_and_map(np,
> j);
> + if (pp->irq[j])
> + pp->has_irq = true;
> + }
> + }
So, on the first glance the patch looks either superfluous or taking
wrong approach. Please, elaborate more why it's done in this way and
what the case for all this in practice.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
On Tue, 2017-12-05 at 16:31 +0100, Linus Walleij wrote: > On Tue, Dec 5, 2017 at 2:38 PM, Wolfram Sang <w...@the-dreams.de> > wrote: > > Two statice inlines in > named > > int gpiod_is output() > int gpiod_is_input() Ha, just proposed similar. > > should conform to Rusty Russell's API hierarchy. > > Interested in fixing it, or should I? > I can almost ACK it before you write the patch. I vote for this type of API, and agree with Wolfram !_get_direction() is confusing. -- Andy Shevchenko <andriy.shevche...@linux.intel.com> Intel Finland Oy
Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
On Mon, 2017-12-04 at 13:36 +0100, Wolfram Sang wrote:
> This will be needed when we want to create STOP conditions, too,
> later.
> Create the needed fields and populate them for the GPIO case if the
> GPIO
> is set to output.
> +#include
> #include
I think it should be not done like this. (Yes, I know why you did that)
Perhaps Linus will accept a patch to move direction flags to some header
feasible via consumer.h.
Linus?
Or if we wish to hide:
static inline bool gpiod_is_direction_out() {}
static inline bool gpiod_is_direction_in() {}
> if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
P.S. Yep, I saw some other upstreamed patch doing this kind of
comparison.
--
Andy Shevchenko <andriy.shevche...@linux.intel.com>
Intel Finland Oy
Re: [PATCH 2/6] i2c: add identifier in declarations for i2c_bus_recovery
On Mon, 2017-12-04 at 13:36 +0100, Wolfram Sang wrote: > No reason to have them undefined, so let's add them. > > int (*recover_bus)(struct i2c_adapter *); > > - int (*get_scl)(struct i2c_adapter *); > - void (*set_scl)(struct i2c_adapter *, int val); > - int (*get_sda)(struct i2c_adapter *); > + int (*get_scl)(struct i2c_adapter *adap); > + void (*set_scl)(struct i2c_adapter *adap, int val); > + int (*get_sda)(struct i2c_adapter *adap); > > void (*prepare_recovery)(struct i2c_adapter *); > void (*unprepare_recovery)(struct i2c_adapter *); It seems inconsistent with the rest of the members even from this visible piece. -- Andy Shevchenko <andriy.shevche...@linux.intel.com> Intel Finland Oy
Re: [PATCH v5 6/6] spi: slave: Add SPI slave handler controlling system state
On Mon, May 22, 2017 at 4:11 PM, Geert Uytterhoeven
<geert+rene...@glider.be> wrote:
> Add an example SPI slave handler to allow remote control of system
> reboot, power off, halt, and suspend.
>
FWIW:
Reviewed-by: Andy Shevchenko <andy.shevche...@gmail.com>
> Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>
> ---
> v5:
> - Add usage documentation to file header,
> - Use network byte order for commands, to match the "-p" parameter of
> spidev_test,
> - Replace pr_*() by dev_*(), stop printing __func__,
> - Remove spi_setup() call to configure 8 bits per word, as that's the
> default,
>
> v3, v4:
> - No changes,
>
> v2:
> - Use spi_async() instead of spi_read(),
> - Submit the next transfer from the previous transfer's completion
> callback, removing the need for a thread,
> - Let .remove() call spi_slave_abort() to cancel the current ongoing
> transfer, and wait for the completion to terminate,
> - Remove FIXME about hanging kthread_stop(),
> - Fix copy-and-pasted module description.
> ---
> drivers/spi/Kconfig| 6 ++
> drivers/spi/Makefile | 1 +
> drivers/spi/spi-slave-system-control.c | 154
> +
> 3 files changed, 161 insertions(+)
> create mode 100644 drivers/spi/spi-slave-system-control.c
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 9972ee2a8454a2fc..82cd818aa06293f5 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -803,6 +803,12 @@ config SPI_SLAVE_TIME
> SPI slave handler responding with the time of reception of the last
> SPI message.
>
> +config SPI_SLAVE_SYSTEM_CONTROL
> + tristate "SPI slave handler controlling system state"
> + help
> + SPI slave handler to allow remote control of system reboot, power
> + off, halt, and suspend.
> +
> endif # SPI_SLAVE
>
> endif # SPI
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index fb078693dbe40da4..1d7923e8c63bc22b 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -108,3 +108,4 @@ obj-$(CONFIG_SPI_ZYNQMP_GQSPI) +=
> spi-zynqmp-gqspi.o
>
> # SPI slave protocol handlers
> obj-$(CONFIG_SPI_SLAVE_TIME) += spi-slave-time.o
> +obj-$(CONFIG_SPI_SLAVE_SYSTEM_CONTROL) += spi-slave-system-control.o
> diff --git a/drivers/spi/spi-slave-system-control.c
> b/drivers/spi/spi-slave-system-control.c
> new file mode 100644
> index ..c0257e937995ec53
> --- /dev/null
> +++ b/drivers/spi/spi-slave-system-control.c
> @@ -0,0 +1,154 @@
> +/*
> + * SPI slave handler controlling system state
> + *
> + * This SPI slave handler allows remote control of system reboot, power off,
> + * halt, and suspend.
> + *
> + * Copyright (C) 2016-2017 Glider bvba
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Usage (assuming /dev/spidev2.0 corresponds to the SPI master on the remote
> + * system):
> + *
> + * # reboot='\x7c\x50'
> + * # poweroff='\x71\x3f'
> + * # halt='\x38\x76'
> + * # suspend='\x1b\x1b'
> + * # spidev_test -D /dev/spidev2.0 -p $suspend # or $reboot, $poweroff,
> $halt
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/*
> + * The numbers are chosen to display something human-readable on two
> 7-segment
> + * displays connected to two 74HC595 shift registers
> + */
> +#define CMD_REBOOT 0x7c50 /* rb */
> +#define CMD_POWEROFF 0x713f /* OF */
> +#define CMD_HALT 0x3876 /* HL */
> +#define CMD_SUSPEND0x1b1b /* ZZ */
> +
> +struct spi_slave_system_control_priv {
> + struct spi_device *spi;
> + struct completion finished;
> + struct spi_transfer xfer;
> + struct spi_message msg;
> + __be16 cmd;
> +};
> +
> +static
> +int spi_slave_system_control_submit(struct spi_slave_system_control_priv
> *priv);
> +
> +static void spi_slave_system_control_complete(void *arg)
> +{
> + struct spi_slave_system_control_priv *priv = arg;
> + u16 cmd;
> + int ret;
> +
> + if (priv->msg.status)
> + goto terminate;
> +
> + cmd = be16_to_cpu(priv->cmd);
> + switch (cmd) {
> + case CMD_REBOOT:
> + dev_info(>spi->dev, "Rebooting system...\n");
> + kernel_restart(NULL);
> +
> + case CMD_POWEROFF:
> + dev_in
Re: [PATCH v4 5/6] spi: slave: Add SPI slave handler reporting uptime at previous message
On Mon, May 22, 2017 at 1:13 PM, Geert Uytterhoeven <ge...@linux-m68k.org> wrote: > On Thu, May 18, 2017 at 6:01 PM, Andy Shevchenko > <andy.shevche...@gmail.com> wrote: >> On Wed, May 17, 2017 at 3:47 PM, Geert Uytterhoeven >> <geert+rene...@glider.be> wrote: >>> + rem_ns = do_div(ts, 10) / 1000; >> >> You divide ts by 10^9, which makes it seconds if it was nanoseconds. >> But reminder is still in nanoseconds and you divide it by 10^3. > >> If I didn't miss anything it should be called like >> rem_ns -> reminder_ms > > Thanks, that must be a remainder from before I reworked the calculation. > Will change it to rem_us (it's in microseconds, not milliseconds). Yeah, correct, thanks. -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 5/6] spi: slave: Add SPI slave handler reporting uptime at previous message
On Wed, May 17, 2017 at 3:47 PM, Geert Uytterhoeven
<geert+rene...@glider.be> wrote:
> Add an example SPI slave handler responding with the uptime at the time
> of reception of the last SPI message.
>
> This can be used by an external microcontroller as a dead man's switch.
> +static int spi_slave_time_submit(struct spi_slave_time_priv *priv)
> +{
> + u32 rem_ns;
> + int ret;
> + u64 ts;
> +
> + ts = local_clock();
> + rem_ns = do_div(ts, 10) / 1000;
You divide ts by 10^9, which makes it seconds if it was nanoseconds.
But reminder is still in nanoseconds and you divide it by 10^3.
If I didn't miss anything it should be called like
rem_ns -> reminder_ms
> +
> + priv->buf[0] = cpu_to_be32(ts);
> + priv->buf[1] = cpu_to_be32(rem_ns);
> +
> + spi_message_init_with_transfers(>msg, >xfer, 1);
> +
> + priv->msg.complete = spi_slave_time_complete;
> + priv->msg.context = priv;
> +
> + ret = spi_async(priv->spi, >msg);
> + if (ret)
> + pr_err("%s: spi_async() failed %d\n", __func__, ret);
Perhaps dev_err() ?
> +
> + return ret;
> +}
> +static int spi_slave_time_probe(struct spi_device *spi)
> +{
> + struct spi_slave_time_priv *priv;
> + int ret;
> +
> + /*
> +* bits_per_word cannot be configured in platform data
> +*/
> + spi->bits_per_word = 8;
Is it worth to define it? If so, can we use device properties for that?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
On Mon, May 8, 2017 at 8:02 PM, Chris Brandt <chris.bra...@renesas.com> wrote: > On Monday, May 08, 2017, Andy Shevchenko wrote: >> On Mon, May 8, 2017 at 7:01 PM, jmondi <jac...@jmondi.org> wrote: >> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote: >> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko >> >> <andy.shevche...@gmail.com> wrote: >> >> >> >> > Linus, for me it looks like better to revert that change, until we >> >> > will have clear picture why existing configuration parameters can't >> >> > work. >> >> >> >> Yeah I'll revert the binding for fixes. >> >> > As it seems we won't be able to proceed with the currently proposed >> solution, >> > would that be acceptable now that we use the "pinmux" property to add >> > flags as BIDIR >> >> Can you explain what does this *electrically* mean? > > Bi-Directional: > > For any pin that needs to drive (send) or sense (receive) signals, the pin > mux controller can only enable 1 direction of buffers (in this case, it > only the output buffers). Therefore the appropriate bit in the > 'bi-directional' register is set in order to enable the signal path in both > directions (ie, enable the input buffers). So, I see this way how it can be enabled: 1. IP_ENI + IP_ENO internally defines BiDi when PMC and PIPC bits are set 2. BiDi bit is set and BUFOE is set Now the question is what the real use case for 2? If we find one we need to rename and fix a description of the pin control configuration property. like: @PIN_CONFIG_OUTPUT_INPUT_ENABLE: this will configure the pin as an output. ... Note: As long as it's enabled the pin's input enabled as well and vice versa. >> > and SWIO_[INPUT|OUTPUT] directly there? > > In the hardware manual, there is a list of pin functions that if you want > to use, you cannot use the stand pinmux register method that you use for > all the other pins. Instead, you are instructed to do a different > procedure. If course electrically, input/output buffers are still turned > on/off or whatever, but the root reason of why you need to do this > differently for these specific pin/function is not known. > > The "SWIO_" portion of the naming comes from the hardware manual which > refers to this as "Software I/O Control Alternative Mode" (which in my > opinion means the HW guys couldn't get the pin directions/buffers to be set > automatically for some reason, so they decided it's the SW guys problem now > for those pins) Okay, these are related to pin muxing explicitly. So, you have 10 functions overall? What prevents you describe them accordingly and hide this implementation detail in the driver? >> Second question, what makes it differ to what already exists? > > We have 3 different flags (properties) that need to be specified for some > pins in some circumstances. > At first, we just tried to pass this additional information in when > defining what function the pin should be just for those pins whose > direction (ie, buffers) would not be set up automatically. > However, this was rejected and we were told to pick from the existing set > generic properties. > But, 3 different generic pinctrl properties that fit what we needed didn't > exist. So, we used the existing "input-enable" and "output-enable", but > then created "bi-directional". Yes, that figure helped me a lot to understand. -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
On Mon, May 8, 2017 at 8:25 PM, jmondi <jac...@jmondi.org> wrote: > Andy, > > On Mon, May 08, 2017 at 07:08:32PM +0300, Andy Shevchenko wrote: >> On Mon, May 8, 2017 at 7:01 PM, jmondi <jac...@jmondi.org> wrote: >> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote: >> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko >> >> <andy.shevche...@gmail.com> wrote: >> >> >> >> > Linus, for me it looks like better to revert that change, until we >> >> > will have clear picture why existing configuration parameters can't >> >> > work. >> >> >> >> Yeah I'll revert the binding for fixes. >> >> > As it seems we won't be able to proceed with the currently proposed >> > solution, >> > would that be acceptable now that we use the "pinmux" property to add >> > flags as BIDIR >> >> Can you explain what does this *electrically* mean? > > I really don't know what to add to what Chris said in his 2 previous > replies to the same question, and I don't know if I can even get this > information as the most detailed drawing I can provide is what you > have seen already at page 2696 Fig. 54.1 of the following document. > > https://www.renesas.com/en-us/doc/products/mpumcu/doc/rz/r01uh0403ej0300_rz_a1h.pdf?key=ccbb2d79446f1cbd015031061140507c I didn't see before this document. (I had downloaded what Chris referred to, which has less than 1200 pages). The figure you pointed to is really nice and explains it, thank you. So, BiDi in this hardware is just helper to enable Input simultaneously when you enable output. This makes me wonder what prevents you to do this in two steps in software? So, basically in terms of pin control framework you define this pin configuration as 1. PIN_CONFIG_INPUT_ENABLE: 2. PIN_CONFIG_OUTPUT: (or wise versa) > From my perspective these flags are configurations internal to the pin > controller hardware used to enable/disable input buffers when a pin needs to > perform in both direction. > The level of detail I can provide on this is the logical diagram we have > pointed > you to already. > > As I assume you are trying to get this answer from us in order to > avoid duplicating things in pin controller sub-system, and I > understand this, but my question here was "can we have those flags as part > of the pinmux property argument list, as that property description > seems to allow us to do that, instead of making them generic pin > configuration properties and upset other developers?" I guess Linus is better than me could answer to this. > Anyway, I still fail to see why those configuration flags, only > affecting the way the pin controller hardware enables/disables > its internal buffers and its internal operations have to be > described in term of their externally visible electrically characteristics. > >> Second question, what makes it differ to what already exists? > > To me, what already exists are pin configuration properties generic to > the whole pin controller subsystem, and I understand you don't want to > see duplication there. > > At the same time, to me, those flags are settings the pin controller > wants to have specified by software to overcome its hw design flaws, > and are intended to configure its internal buffers in a way it cannot > do by itself for some very specific operation modes (they are listed > in the hw reference manual, it's not something you can chose to > configure or not, if you want a pin working in i2c mode, you HAVE to > pass those flags to pin controller). So, when you configuring pinmux to use group of pins to be i2c, what does prevent you to apply those settings implicitly? -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
On Mon, May 8, 2017 at 7:01 PM, jmondi <jac...@jmondi.org> wrote: > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote: >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko >> <andy.shevche...@gmail.com> wrote: >> >> > Linus, for me it looks like better to revert that change, until we >> > will have clear picture why existing configuration parameters can't >> > work. >> >> Yeah I'll revert the binding for fixes. > As it seems we won't be able to proceed with the currently proposed solution, > would that be acceptable now that we use the "pinmux" property to add > flags as BIDIR Can you explain what does this *electrically* mean? Second question, what makes it differ to what already exists? > and SWIO_[INPUT|OUTPUT] directly there? Ditto. > This was my original proposal, rejected because we were using the "pins" > property at the time. -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties
On Fri, Apr 28, 2017 at 11:16 AM, Linus Walleij <linus.wall...@linaro.org> wrote: > On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi > <jacopo+rene...@jmondi.org> wrote: > But why. > > I have these two static inlines just below your new macros: +1. > We generally prefer static inlines over macros because they are easier > to read. Not only. It adds type checking as well AFAIUC. -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
On Fri, Apr 28, 2017 at 6:16 PM, Chris Brandt <chris.bra...@renesas.com> wrote: > On Friday, April 28, 2017, Andy Shevchenko wrote: >> Had you read the following, esp. Note there? >> >> * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input. Note that this does >> not >> * affect the pin's ability to drive output. 1 enables input, 0 >> disables >> * input. >> >> For me manual is clearly tells about this settings: >> "This register enables or disables the input buffer while the output >> buffer is enabled." > > > But, then if we use "input-enable" to get bi-directional functionality, It seems you are missing the point from electrical prospective. Standard pin buffers (electrically) means input buffer and output buffer that are driven independently in most cases. Here is one example: https://electronics.stackexchange.com/questions/96932/internal-circuitry-of-io-ports-in-mcu/96953#96953 (That's what I asked before as a schematic) > now we need something to replace what we were using "input-enable" for. No. > We were using "input-enable" to signal when the pin function that we set also > needs to be forcible set to input by the software (once again, because the HW > is not smart enough to do it on its own), but is different than the > bi-directional functionality (ie, a different register setting). You are trying to introduce an abstraction, called BiDi, which is *not* a separate thing from a set of pin properties. > So, if we replace "bi-directional" with "input-enable" (since logically > internally that is what is going on), what do we use for the special pins > that the HW manual says "hey, you need to manually set these pins to input > with SW because the pin selection HW can't do it correctly)". Yes. BiDi is an alias to output + input enable + other pin configuration parameters (a set depends on real HW needs). > Note that we added a enable-output for the same reason. > See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that > PIPCn.PIPCnm Bit Should be Set to 0" Perhaps needs to be revisited as well. P.S. It looks like more and more software engineers are going far from real hardware when developing drivers... -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
On Fri, Apr 28, 2017 at 4:18 PM, Chris Brandt <chris.bra...@renesas.com> wrote: > On Friday, April 28, 2017, Andy Shevchenko wrote: >> > In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control >> Logical Diagram (but that wasn't obvious to me at first). >> >> Please, post a link to it or copy essential parts. > The schematic is included in the "User's manual" > https://www.renesas.com/en-us/doc/products/tool/doc/003/r20ut2596ej_r7s72100evum.pdf Not that one I would like to see... > The RZ/A1H Hardware manual is here: > https://www.renesas.com/en-us/document/hw-manual?hwLayerShowFlg=true=186374=RZ%252FA1H=document-prd-search=%2Fen-us%2Fdoc%2Fproducts%2Fmpumcu%2Fdoc%2Frz%2Fr01uh0403ej0300_rz_a1h.pdf=54f335753742b5add524d4725b7242e6 > > Chapter 54 is the port/pin controller. > > "54.18 Port Control Logical Diagram" is the diagram I was talking about. Note > that is says "Note: This figure shows the logic for reference, not the > circuit." > > "54.3.13 Port Bidirection Control Register (PBDCn)" is the magic register > needed to get some pins to work. This is useful. Thanks. >> I'm quite skeptical that cheap hardware can implement something more >> costable than simplest open-source / open-drain + bias > > I don't think this is an open-source / open-drain + bias issue. It's a "the > internal signal paths are not getting hooked up correctly" issue. Had you read the following, esp. Note there? * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input. Note that this does not * affect the pin's ability to drive output. 1 enables input, 0 disables * input. For me manual is clearly tells about this settings: "This register enables or disables the input buffer while the output buffer is enabled." > Regardless, on this part, we needed a way to flag that some pins when put in > some function modes needed 'an extra register setting'. At first we tried to > sneak that info in with a simple #define in the pin/pinmux DT node > properties. But, Linus didn't want it there so we had to make up a new > generic property called "bi-directional". > > What is your end goal here? Get "bi-directional" changed to something else? My goal is to reduce amount of (useless) entities. See Occam's razor for details. Linus, for me it looks like better to revert that change, until we will have clear picture why existing configuration parameters can't work. -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
On Fri, Apr 28, 2017 at 3:07 PM, Chris Brandt <chris.bra...@renesas.com> wrote: > On Friday, April 28, 2017, Linus Walleij wrote: >> > For me it looks like you are trying to alias open-drain + bias or >> > alike. Don't actually see the benefit of it. >> >> Andy is bringing up a valid point. And I remember asking about this before. >> >> What does "bi-directional" really mean, electrically speaking? > Take the SDHI data pins. You send AND receive data over those pins (and they > are not open drain). Can you point to schematics and electrical characteristics of such buffer? (Yes, I can imagine one case where it's possible to have an "automatic" switch based on which current is bigger output of your side or remote's. But! I would like to see actual specifications to prove this or otherwise.) > The issue is that the PFC HW that enables the connections between the SDHI IP > block and the I/O pad buffers can only enable one path/signal/direction to > the buffer enables (in or out). So for a pin that needs both directions, the > PFC enables output and the "bidirectional register" is used to enable the > input buffer as well. > In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control Logical > Diagram (but that wasn't obvious to me at first). Please, post a link to it or copy essential parts. I'm quite skeptical that cheap hardware can implement something more costable than simplest open-source / open-drain + bias. -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable
If the argument is != 0 pull-up is enabled,
> * if it is 0, pull-up is total, i.e. the pin is connected to VDD.
> + * @PIN_CONFIG_BIDIRECTIONAL: the pin will be configured to allow
> simultaneous
> + * input and output operations.
> * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
> * collector) which means it is usually wired with other output ports
> * which are then pulled up with an external resistor. Setting this
> @@ -96,6 +98,7 @@ enum pin_config_param {
> PIN_CONFIG_BIAS_PULL_DOWN,
> PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
> PIN_CONFIG_BIAS_PULL_UP,
> + PIN_CONFIG_BIDIRECTIONAL,
> PIN_CONFIG_DRIVE_OPEN_DRAIN,
> PIN_CONFIG_DRIVE_OPEN_SOURCE,
> PIN_CONFIG_DRIVE_PUSH_PULL,
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
