[PATCH] gpio: rcar: reference device instead of platform device

[Vladimir Zapolskiy]

The change simplifies dereferences to the mediated struct device, also
it allows to limit the scope of the platform device usage to probe and
remove functions only.

Non-functional change.

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpio/gpio-rcar.c | 24 +++-
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index 1322f7e0cfde..068ce25ffd28 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -35,7 +35,7 @@ struct gpio_rcar_bank_info {
 struct gpio_rcar_priv {
void __iomem *base;
spinlock_t lock;
-   struct platform_device *pdev;
+   struct device *dev;
struct gpio_chip gpio_chip;
struct irq_chip irq_chip;
unsigned int irq_parent;
@@ -140,7 +140,7 @@ static int gpio_rcar_irq_set_type(struct irq_data *d, 
unsigned int type)
struct gpio_rcar_priv *p = gpiochip_get_data(gc);
unsigned int hwirq = irqd_to_hwirq(d);
 
-   dev_dbg(>pdev->dev, "sense irq = %d, type = %d\n", hwirq, type);
+   dev_dbg(p->dev, "sense irq = %d, type = %d\n", hwirq, type);
 
switch (type & IRQ_TYPE_SENSE_MASK) {
case IRQ_TYPE_LEVEL_HIGH:
@@ -180,8 +180,7 @@ static int gpio_rcar_irq_set_wake(struct irq_data *d, 
unsigned int on)
if (p->irq_parent) {
error = irq_set_irq_wake(p->irq_parent, on);
if (error) {
-   dev_dbg(>pdev->dev,
-   "irq %u doesn't support irq_set_wake\n",
+   dev_dbg(p->dev, "irq %u doesn't support irq_set_wake\n",
p->irq_parent);
p->irq_parent = 0;
}
@@ -244,13 +243,13 @@ static int gpio_rcar_request(struct gpio_chip *chip, 
unsigned offset)
struct gpio_rcar_priv *p = gpiochip_get_data(chip);
int error;
 
-   error = pm_runtime_get_sync(>pdev->dev);
+   error = pm_runtime_get_sync(p->dev);
if (error < 0)
return error;
 
error = pinctrl_gpio_request(chip->base + offset);
if (error)
-   pm_runtime_put(>pdev->dev);
+   pm_runtime_put(p->dev);
 
return error;
 }
@@ -267,7 +266,7 @@ static void gpio_rcar_free(struct gpio_chip *chip, unsigned 
offset)
 */
gpio_rcar_config_general_input_output_mode(chip, offset, false);
 
-   pm_runtime_put(>pdev->dev);
+   pm_runtime_put(p->dev);
 }
 
 static int gpio_rcar_get_direction(struct gpio_chip *chip, unsigned int offset)
@@ -398,21 +397,20 @@ MODULE_DEVICE_TABLE(of, gpio_rcar_of_table);
 
 static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)
 {
-   struct device_node *np = p->pdev->dev.of_node;
+   struct device_node *np = p->dev->of_node;
const struct gpio_rcar_info *info;
struct of_phandle_args args;
int ret;
 
-   info = of_device_get_match_data(>pdev->dev);
+   info = of_device_get_match_data(p->dev);
 
ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, );
*npins = ret == 0 ? args.args[2] : RCAR_MAX_GPIO_PER_BANK;
p->has_both_edge_trigger = info->has_both_edge_trigger;
 
if (*npins == 0 || *npins > RCAR_MAX_GPIO_PER_BANK) {
-   dev_warn(>pdev->dev,
-"Invalid number of gpio lines %u, using %u\n", *npins,
-RCAR_MAX_GPIO_PER_BANK);
+   dev_warn(p->dev, "Invalid number of gpio lines %u, using %u\n",
+*npins, RCAR_MAX_GPIO_PER_BANK);
*npins = RCAR_MAX_GPIO_PER_BANK;
}
 
@@ -434,7 +432,7 @@ static int gpio_rcar_probe(struct platform_device *pdev)
if (!p)
return -ENOMEM;
 
-   p->pdev = pdev;
+   p->dev = dev;
spin_lock_init(>lock);
 
/* Get device configuration from DT node */
-- 
2.17.1

Re: [PATCH 04/14] arm64: dts: renesas: r8a7795-es1: ulcb-kf: Use "renesas,ulcb" compatible

[Vladimir Zapolskiy]

Hi Laurent,

[Adding Rob and DT ML]

On 08/06/2018 01:42 PM, Laurent Pinchart wrote:
> Hi Eugeniu,
> 
> Thank you for the patch.
> 
> On Sunday, 5 August 2018 02:11:04 EEST Eugeniu Rosca wrote:
>> Following the recent change in dt-bindings [1], switch from
>> "renesas,h3ulcb" to "renesas,ulcb" compatible string.
>>
>> [1] Documentation/devicetree/bindings/arm/shmobile.txt
>>
>> Signed-off-by: Eugeniu Rosca 
>> ---
>>  arch/arm64/boot/dts/renesas/r8a7795-es1-ulcb-kf.dts | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/renesas/r8a7795-es1-ulcb-kf.dts
>> b/arch/arm64/boot/dts/renesas/r8a7795-es1-ulcb-kf.dts index
>> 06deb67c36c8..7a5b1dc64090 100644
>> --- a/arch/arm64/boot/dts/renesas/r8a7795-es1-ulcb-kf.dts
>> +++ b/arch/arm64/boot/dts/renesas/r8a7795-es1-ulcb-kf.dts
>> @@ -14,6 +14,6 @@
>>
>>  / {
>>  model = "Renesas H3ULCB Kingfisher board based on r8a7795 ES1.x";
>> -compatible = "shimafuji,kingfisher", "renesas,h3ulcb",
>> +compatible = "shimafuji,kingfisher", "renesas,ulcb",
>>   "renesas,r8a7795";
> 
> This is unrelated to your patch, but due to the reason explained in my review 
> of 02/14, I think "shimafuji,kingfisher" should include the SoC name.
> 
> This brings up the topic of how to describe boards that are made of an SoC 
> "module" board plugged into an expansion "motherboard".
> 

Diving into (probably shatteredly recollected by me) history, a board
'compatible' property appears as a handle to deal with incomplete board
description represented in DTB, so that arch code or drivers can catch on it
and fill some board specific missing parts in runtime, commonly the related
code takes a shape of quirks.

If we accept it, SoC specific drivers would take their interest in SoC model
and revision, and out-of-SoC/platform/board drivers and legacy arch board
code can look at a PCB board model variant. Note that for both separated
but comparably large and important pieces of board/platform knowledge there
is a single compatible property in use, namely the compatible property of
the root node. Also note that both ePAPR and Devicetree Specification describe
'compatible' property of the root node as a special one, as "a list of
platform architectures with which this platform is compatible".

In my opinion a better board representation would be to add a 'soc' device
node as a child of the root node, and the former one has its own fixed
compatible property, but a protocol based on such view has to be agreed
and accepted firstly, and it falls into "forever unrealistic tasks" category.

Today the SoC part of compatible property value is still in active use, and
PCB\SoC part is almost abandoned, so I would propose to diminish the asset
of the latter. Since both board/platform descriptions are alloyed in one
list of values, I'd like to see a better description of acceptable values
of 'compatible' property of the root node.

The common practice is to put SoC specific values into the right tail,
and place (kind of optional) board specific values on the left, let's
continue to follow it, but unrestrict SoC agnostic string names of boards
and further board extensions, if PCB\SoC (or PCB\PCB for extension boards)
are about identical. Anyway those values are mainly unused nowadays, so
nothing is lost but another dimension in board/platform description and
naming is avoided. I do understand that likely most of PCBs are very SoC
centric, and the proposal shared above should not be considered as a rule,
but rather as a reasonable and valid exception.

--
Best wishes,
Vladimir

Re: [PATCH 3/6] ravb: remove custom .set_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

Hello Sergei,

On 05/26/2018 10:50 PM, Sergei Shtylyov wrote:
> On 05/24/2018 02:11 PM, Vladimir Zapolskiy wrote:
> 
>> The change replaces a custom implementation of .set_link_ksettings
>> callback with a shared phy_ethtool_set_link_ksettings(), this fixes
>> sleep in atomic context bug, which is encountered every time when link
>> settings are changed by ethtool.
> 
>Seeing it now...
> 
>> Now duplex mode setting is enforced in ravb_adjust_link() only, also
>> now TX/RX is disabled when link is put down or modifications to E-MAC
>> registers ECMR and GECMR are expected for both cases of checked and
>> ignored link status pin state from E-MAC interrupt handler.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 58 
>> +---
>>  1 file changed, 15 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
>> b/drivers/net/ethernet/renesas/ravb_main.c
>> index 3d91caa44176..0d811c02ff34 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -980,6 +980,13 @@ static void ravb_adjust_link(struct net_device *ndev)
>>  struct ravb_private *priv = netdev_priv(ndev);
>>  struct phy_device *phydev = ndev->phydev;
>>  bool new_state = false;
>> +unsigned long flags;
>> +
>> +spin_lock_irqsave(>lock, flags);
>> +
>> +/* Disable TX and RX right over here, if E-MAC change is ignored */
>> +if (priv->no_avb_link)
>> +ravb_rcv_snd_disable(ndev);
>>  
>>  if (phydev->link) {
>>  if (phydev->duplex != priv->duplex) {
>> @@ -997,18 +1004,21 @@ static void ravb_adjust_link(struct net_device *ndev)
>>  ravb_modify(ndev, ECMR, ECMR_TXF, 0);
>>  new_state = true;
>>  priv->link = phydev->link;
>> -if (priv->no_avb_link)
>> -ravb_rcv_snd_enable(ndev);
>>  }
>>  } else if (priv->link) {
>>  new_state = true;
>>  priv->link = 0;
>>  priv->speed = 0;
>>  priv->duplex = -1;
>> -if (priv->no_avb_link)
>> -ravb_rcv_snd_disable(ndev);
>>  }
>>  
>> +/* Enable TX and RX right over here, if E-MAC change is ignored */
>> +if (priv->no_avb_link && phydev->link)
>> +ravb_rcv_snd_enable(ndev);
>> +
>> +mmiowb();
>> +spin_unlock_irqrestore(>lock, flags);
>> +
> 
>I like this part. :-)
> 

A weight off my mind :) And I hope that this change will remain the less
questionable one, other ones from the series are trivial.

Anyway I hope it is understandable that this part of the change can not
be simply extracted from the rest one below, otherwise there'll be bugs of
another type intorduced.

>>  if (new_state && netif_msg_link(priv))
>>  phy_print_status(phydev);
>>  }
>> @@ -1096,44 +1106,6 @@ static int ravb_phy_start(struct net_device *ndev)
>>  return 0;
>>  }
>>  
>> -static int ravb_set_link_ksettings(struct net_device *ndev,
>> -   const struct ethtool_link_ksettings *cmd)
>> -{
>> -struct ravb_private *priv = netdev_priv(ndev);
>> -unsigned long flags;
>> -int error;
>> -
>> -if (!ndev->phydev)
>> -return -ENODEV;
>> -
>> -spin_lock_irqsave(>lock, flags);
>> -
>> -/* Disable TX and RX */
>> -ravb_rcv_snd_disable(ndev);
>> -
>> -error = phy_ethtool_ksettings_set(ndev->phydev, cmd);
>> -if (error)
>> -goto error_exit;
>> -
>> -if (cmd->base.duplex == DUPLEX_FULL)
>> -priv->duplex = 1;
>> -else
>> -priv->duplex = 0;
>> -
>> -ravb_set_duplex(ndev);
>> -
>> -error_exit:
>> -mdelay(1);
>> -
>> -/* Enable TX and RX */
>> -ravb_rcv_snd_enable(ndev);
>> -
>> -mmiowb();
>> -spin_unlock_irqrestore(>lock, flags);
>> -
>> -return error;
>> -}
>> -
> 
>But this part is clearly lumping it all together... 

Please elaborate.

> 
> [...]
>> @@ -1357,7 +1329,7 @@ static const struct ethtool_ops ravb_ethtool_ops = {
>>  .set_ringparam  = ravb_set

Re: [PATCH 0/6] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[Vladimir Zapolskiy]

Hello Sergei,

On 05/24/2018 08:24 PM, Sergei Shtylyov wrote:
> On 05/24/2018 07:40 PM, Sergei Shtylyov wrote:
> 
>>> For ages trivial changes to RAVB and SuperH ethernet links by means of
>>> standard 'ethtool' trigger a 'sleeping function called from invalid
>>> context' bug, to visualize it on r8a7795 ULCB:
>>>
>>>   % ethtool -r eth0
>>>   BUG: sleeping function called from invalid context at 
>>> kernel/locking/mutex.c:747
>>>   in_atomic(): 1, irqs_disabled(): 128, pid: 554, name: ethtool
>>>   INFO: lockdep is turned off.
>>>   irq event stamp: 0
>>>   hardirqs last  enabled at (0): [<>]   (null)
>>>   hardirqs last disabled at (0): [] 
>>> copy_process.isra.7.part.8+0x2cc/0x1918
>>>   softirqs last  enabled at (0): [] 
>>> copy_process.isra.7.part.8+0x2cc/0x1918
>>>   softirqs last disabled at (0): [<>]   (null)
>>>   CPU: 5 PID: 554 Comm: ethtool Not tainted 4.17.0-rc4-arm64-renesas+ #33
>>>   Hardware name: Renesas H3ULCB board based on r8a7795 ES2.0+ (DT)
>>>   Call trace:
>>>dump_backtrace+0x0/0x198
>>>show_stack+0x24/0x30
>>>dump_stack+0xb8/0xf4
>>>___might_sleep+0x1c8/0x1f8
>>>__might_sleep+0x58/0x90
>>>__mutex_lock+0x50/0x890
>>>mutex_lock_nested+0x3c/0x50
>>>phy_start_aneg_priv+0x38/0x180
>>>phy_start_aneg+0x24/0x30
>>>ravb_nway_reset+0x3c/0x68
>>>dev_ethtool+0x3dc/0x2338
>>>dev_ioctl+0x19c/0x490
>>>sock_do_ioctl+0xe0/0x238
>>>sock_ioctl+0x254/0x460
>>>do_vfs_ioctl+0xb0/0x918
>>>ksys_ioctl+0x50/0x80
>>>sys_ioctl+0x34/0x48
>>>__sys_trace_return+0x0/0x4
>>>
>>> The root cause is that an attempt to modify ECMR and GECMR registers
>>> only when RX/TX function is disabled was too overcomplicated in its
>>> original implementation, also processing of an optional Link Change
>>> interrupt added even more complexity, as a result the implementation
>>> was error prone.
>>>
>>> The new locking scheme is confirmed to be correct by dumping driver
>>> specific and generic PHY framework function calls with aid of ftrace
>>> while running more or less advanced tests.
>>>
>>> Please note that sh_eth patches from the series were built-tested only.
>>>
>>> On purpose I do not add Fixes tags, the reused PHY handlers were added
>>> way later than the fixed problems were firstly found in the drivers.
>>
>>I think you went one step too far with these fixes. On the first glance,
>> the real fixes are to remove grabbing/releasing the spinlock for the duration
>> of the phylib calls. Am I right? If so, making use of the new phylib APIs
>> would be a further enhancement, it's not needed for fixing the splats per 
>> se...
> 
>Note that I hadn't looked at the patches #3/#6 at the time of writing this;
> those seem to be more complicated than the rest.

Right, the simplistic approach of just removing the held spinlock does
not fit well into the overall lame locking model found in the driver.

The thing is that I would prefer to exhibit 'remove custom callbacks'
side of the changes as it is done now, and fixing severe 'invalid contex'
bugs is left as a valuable side effect. I may attempt to find enough
free time to follow your instructions, but frankly speaking I don't
see it beneficial to split a single good all-sufficient change into
three or more: removal of spinlocks, replacement of phy_start_aneg(),
then a non-functional clean-up. Bikeshedding isn't my preference,
but a report about technical flaws related to the published changes
is appreciated, otherwise let me ask you to accept the changes as is,
secondary optimizations can be done on top of them.

--
With best wishes,
Vladimir

Re: [PATCH 1/6] ravb: remove custom .nway_reset from ethtool ops

[Vladimir Zapolskiy]

Hello Sergei,

On 05/24/2018 08:01 PM, Sergei Shtylyov wrote:
> On 05/24/2018 07:44 PM, Andrew Lunn wrote:
> 
>> The change fixes a sleep in atomic context issue, which can be
>> always triggered by running 'ethtool -r' command, because
>> phy_start_aneg() protects phydev fields by a mutex.
>>>
>>>   You don't say that *not* grabbing the spinlock is safe...

I say both that it is the fix and it is safe, I've already described
the function of the spinlock in my comments, and it is more or less
clear from the driver code.

>>
>> For it to be unsafe, i think that would mean phylib would need to call
>> back into the MAC driver? The only way that could happen is via the
>> adjust_link call. And that will deadlock, since it takes the same
>> lock.
>>
>> Or am i/we missing something?
> 
>It doesn't take any locks currently, only patches #3/#6 makes it do so...

And that's the proper fix in my opinion, my tests don't unveil any issues.

--
With best wishes,
Vladimir

Re: [PATCH 1/6] ravb: remove custom .nway_reset from ethtool ops

[Vladimir Zapolskiy]

On 05/24/2018 04:22 PM, Andrew Lunn wrote:
> On Thu, May 24, 2018 at 02:11:53PM +0300, Vladimir Zapolskiy wrote:
>> The change fixes a sleep in atomic context issue, which can be
>> always triggered by running 'ethtool -r' command, because
>> phy_start_aneg() protects phydev fields by a mutex.
>>
>> Another note is that the change implicitly replaces phy_start_aneg()
>> with a newer phy_restart_aneg().
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 17 +
>>  1 file changed, 1 insertion(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
>> b/drivers/net/ethernet/renesas/ravb_main.c
>> index 68f122140966..4a043eb0e2aa 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1150,21 +1150,6 @@ static int ravb_set_link_ksettings(struct net_device 
>> *ndev,
>>  return error;
>>  }
>>  
>> -static int ravb_nway_reset(struct net_device *ndev)
>> -{
>> -struct ravb_private *priv = netdev_priv(ndev);
>> -int error = -ENODEV;
>> -unsigned long flags;
>> -
>> -if (ndev->phydev) {
>> -spin_lock_irqsave(>lock, flags);
>> -error = phy_start_aneg(ndev->phydev);
>> -spin_unlock_irqrestore(>lock, flags);
>> -}
> 
> Eck! phylib assumes thread context and takes a mutex while calling
> into the PHY driver.
> 
> It would be good to add some sort of fixes: tag. Maybe for the commit
> that added the generic nway_reset? That would at least cover some
> stable kernels.
> 
> Reviewed-by: Andrew Lunn <and...@lunn.ch>
> 

Hi Andrew, thank you for review.

generally it makes sense to add Fixes tag, but as I said in
the commit message the problem is present before reused phy_ethtool_*()
functions were added to the kernel, so some kind of juggling with
the proper kernel version would be required in assumption that
the fixes are backported as an unmodified changes.

Hopefully Sergei as the driver maintainer can verify the fixes on
older kernels and suggest the right kernel versions for backporting.

--
With best wishes,
Vladimir

Re: [PATCH 3/6] ravb: remove custom .set_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

Hi Andrew,

On 05/24/2018 04:29 PM, Andrew Lunn wrote:
> On Thu, May 24, 2018 at 02:11:55PM +0300, Vladimir Zapolskiy wrote:
>> The change replaces a custom implementation of .set_link_ksettings
>> callback with a shared phy_ethtool_set_link_ksettings(), this fixes
>> sleep in atomic context bug, which is encountered every time when link
>> settings are changed by ethtool.
>>
>> Now duplex mode setting is enforced in ravb_adjust_link() only, also
>> now TX/RX is disabled when link is put down or modifications to E-MAC
>> registers ECMR and GECMR are expected for both cases of checked and
>> ignored link status pin state from E-MAC interrupt handler.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 58 
>> +---
>>  1 file changed, 15 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
>> b/drivers/net/ethernet/renesas/ravb_main.c
>> index 3d91caa44176..0d811c02ff34 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -980,6 +980,13 @@ static void ravb_adjust_link(struct net_device *ndev)
>>  struct ravb_private *priv = netdev_priv(ndev);
>>  struct phy_device *phydev = ndev->phydev;
>>  bool new_state = false;
>> +unsigned long flags;
>> +
>> +spin_lock_irqsave(>lock, flags);
> 
> Hi Vladimir
> 
> It is pretty unusual to see an adjust_link callback take a lock. Is it
> clearly defined what it is protecting?
> 

thank you for review.

As the commit message says, the hardware manual claims that
any modifications to ECMR and GECMR registers, i.e. calls to
ravb_set_duplex() and ravb_set_rate() from the modified
ravb_adjust_link() function, has to be done when RX/TX is
disabled (same ECMR register bit fields), the spinlock
serializes interrupt handlers and modifications to ECMR contents,
its previous usage for ethtool handlers was obviously wrong.

The information is quite implicit, but the change emphasizes
it.

--
With best wishes,
Vladimir

[PATCH 5/6] sh_eth: remove custom .get_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

The change replaces a custom implementation of .get_link_ksettings
callback with a shared phy_ethtool_get_link_ksettings(), note that
>lock wrapping is not needed, because the lock does not
serialize access to phydev fields.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 18 +-
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c 
b/drivers/net/ethernet/renesas/sh_eth.c
index 6d1fed2b4a4a..e627b2b6c3b3 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2019,22 +2019,6 @@ static int sh_eth_phy_start(struct net_device *ndev)
return 0;
 }
 
-static int sh_eth_get_link_ksettings(struct net_device *ndev,
-struct ethtool_link_ksettings *cmd)
-{
-   struct sh_eth_private *mdp = netdev_priv(ndev);
-   unsigned long flags;
-
-   if (!ndev->phydev)
-   return -ENODEV;
-
-   spin_lock_irqsave(>lock, flags);
-   phy_ethtool_ksettings_get(ndev->phydev, cmd);
-   spin_unlock_irqrestore(>lock, flags);
-
-   return 0;
-}
-
 static int sh_eth_set_link_ksettings(struct net_device *ndev,
 const struct ethtool_link_ksettings *cmd)
 {
@@ -2411,7 +2395,7 @@ static const struct ethtool_ops sh_eth_ethtool_ops = {
.get_sset_count = sh_eth_get_sset_count,
.get_ringparam  = sh_eth_get_ringparam,
.set_ringparam  = sh_eth_set_ringparam,
-   .get_link_ksettings = sh_eth_get_link_ksettings,
+   .get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = sh_eth_set_link_ksettings,
.get_wol= sh_eth_get_wol,
.set_wol= sh_eth_set_wol,
-- 
2.8.1

[PATCH 6/6] sh_eth: remove custom .set_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

The change replaces a custom implementation of .set_link_ksettings
callback with a shared phy_ethtool_set_link_ksettings(), this fixes
sleep in atomic context bug, which is encountered every time when link
settings are changed by ethtool.

Now duplex mode setting is enforced in ravb_adjust_link() only, also
now TX/RX is disabled when link is put down or modifications to E-MAC
registers ECMR and GECMR are expected for both cases of checked and
ignored link status pin state from E-MAC interrupt handler.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 58 +--
 1 file changed, 15 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c 
b/drivers/net/ethernet/renesas/sh_eth.c
index e627b2b6c3b3..a3115888bd04 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1916,8 +1916,15 @@ static void sh_eth_adjust_link(struct net_device *ndev)
 {
struct sh_eth_private *mdp = netdev_priv(ndev);
struct phy_device *phydev = ndev->phydev;
+   unsigned long flags;
int new_state = 0;
 
+   spin_lock_irqsave(>lock, flags);
+
+   /* Disable TX and RX right over here, if E-MAC change is ignored */
+   if (mdp->cd->no_psr || mdp->no_ether_link)
+   sh_eth_rcv_snd_disable(ndev);
+
if (phydev->link) {
if (phydev->duplex != mdp->duplex) {
new_state = 1;
@@ -1936,18 +1943,21 @@ static void sh_eth_adjust_link(struct net_device *ndev)
sh_eth_modify(ndev, ECMR, ECMR_TXF, 0);
new_state = 1;
mdp->link = phydev->link;
-   if (mdp->cd->no_psr || mdp->no_ether_link)
-   sh_eth_rcv_snd_enable(ndev);
}
} else if (mdp->link) {
new_state = 1;
mdp->link = 0;
mdp->speed = 0;
mdp->duplex = -1;
-   if (mdp->cd->no_psr || mdp->no_ether_link)
-   sh_eth_rcv_snd_disable(ndev);
}
 
+   /* Enable TX and RX right over here, if E-MAC change is ignored */
+   if ((mdp->cd->no_psr || mdp->no_ether_link) && phydev->link)
+   sh_eth_rcv_snd_enable(ndev);
+
+   mmiowb();
+   spin_unlock_irqrestore(>lock, flags);
+
if (new_state && netif_msg_link(mdp))
phy_print_status(phydev);
 }
@@ -2019,44 +2029,6 @@ static int sh_eth_phy_start(struct net_device *ndev)
return 0;
 }
 
-static int sh_eth_set_link_ksettings(struct net_device *ndev,
-const struct ethtool_link_ksettings *cmd)
-{
-   struct sh_eth_private *mdp = netdev_priv(ndev);
-   unsigned long flags;
-   int ret;
-
-   if (!ndev->phydev)
-   return -ENODEV;
-
-   spin_lock_irqsave(>lock, flags);
-
-   /* disable tx and rx */
-   sh_eth_rcv_snd_disable(ndev);
-
-   ret = phy_ethtool_ksettings_set(ndev->phydev, cmd);
-   if (ret)
-   goto error_exit;
-
-   if (cmd->base.duplex == DUPLEX_FULL)
-   mdp->duplex = 1;
-   else
-   mdp->duplex = 0;
-
-   if (mdp->cd->set_duplex)
-   mdp->cd->set_duplex(ndev);
-
-error_exit:
-   mdelay(1);
-
-   /* enable tx and rx */
-   sh_eth_rcv_snd_enable(ndev);
-
-   spin_unlock_irqrestore(>lock, flags);
-
-   return ret;
-}
-
 /* If it is ever necessary to increase SH_ETH_REG_DUMP_MAX_REGS, the
  * version must be bumped as well.  Just adding registers up to that
  * limit is fine, as long as the existing register indices don't
@@ -2396,7 +2368,7 @@ static const struct ethtool_ops sh_eth_ethtool_ops = {
.get_ringparam  = sh_eth_get_ringparam,
.set_ringparam  = sh_eth_set_ringparam,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
-   .set_link_ksettings = sh_eth_set_link_ksettings,
+   .set_link_ksettings = phy_ethtool_set_link_ksettings,
.get_wol= sh_eth_get_wol,
.set_wol= sh_eth_set_wol,
 };
-- 
2.8.1

[PATCH 1/6] ravb: remove custom .nway_reset from ethtool ops

[Vladimir Zapolskiy]

The change fixes a sleep in atomic context issue, which can be
always triggered by running 'ethtool -r' command, because
phy_start_aneg() protects phydev fields by a mutex.

Another note is that the change implicitly replaces phy_start_aneg()
with a newer phy_restart_aneg().

Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 17 +
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
b/drivers/net/ethernet/renesas/ravb_main.c
index 68f122140966..4a043eb0e2aa 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1150,21 +1150,6 @@ static int ravb_set_link_ksettings(struct net_device 
*ndev,
return error;
 }
 
-static int ravb_nway_reset(struct net_device *ndev)
-{
-   struct ravb_private *priv = netdev_priv(ndev);
-   int error = -ENODEV;
-   unsigned long flags;
-
-   if (ndev->phydev) {
-   spin_lock_irqsave(>lock, flags);
-   error = phy_start_aneg(ndev->phydev);
-   spin_unlock_irqrestore(>lock, flags);
-   }
-
-   return error;
-}
-
 static u32 ravb_get_msglevel(struct net_device *ndev)
 {
struct ravb_private *priv = netdev_priv(ndev);
@@ -1377,7 +1362,7 @@ static int ravb_set_wol(struct net_device *ndev, struct 
ethtool_wolinfo *wol)
 }
 
 static const struct ethtool_ops ravb_ethtool_ops = {
-   .nway_reset = ravb_nway_reset,
+   .nway_reset = phy_ethtool_nway_reset,
.get_msglevel   = ravb_get_msglevel,
.set_msglevel   = ravb_set_msglevel,
.get_link   = ethtool_op_get_link,
-- 
2.8.1

[PATCH 0/6] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[Vladimir Zapolskiy]

For ages trivial changes to RAVB and SuperH ethernet links by means of
standard 'ethtool' trigger a 'sleeping function called from invalid
context' bug, to visualize it on r8a7795 ULCB:

  % ethtool -r eth0
  BUG: sleeping function called from invalid context at 
kernel/locking/mutex.c:747
  in_atomic(): 1, irqs_disabled(): 128, pid: 554, name: ethtool
  INFO: lockdep is turned off.
  irq event stamp: 0
  hardirqs last  enabled at (0): [<>]   (null)
  hardirqs last disabled at (0): [] 
copy_process.isra.7.part.8+0x2cc/0x1918
  softirqs last  enabled at (0): [] 
copy_process.isra.7.part.8+0x2cc/0x1918
  softirqs last disabled at (0): [<>]   (null)
  CPU: 5 PID: 554 Comm: ethtool Not tainted 4.17.0-rc4-arm64-renesas+ #33
  Hardware name: Renesas H3ULCB board based on r8a7795 ES2.0+ (DT)
  Call trace:
   dump_backtrace+0x0/0x198
   show_stack+0x24/0x30
   dump_stack+0xb8/0xf4
   ___might_sleep+0x1c8/0x1f8
   __might_sleep+0x58/0x90
   __mutex_lock+0x50/0x890
   mutex_lock_nested+0x3c/0x50
   phy_start_aneg_priv+0x38/0x180
   phy_start_aneg+0x24/0x30
   ravb_nway_reset+0x3c/0x68
   dev_ethtool+0x3dc/0x2338
   dev_ioctl+0x19c/0x490
   sock_do_ioctl+0xe0/0x238
   sock_ioctl+0x254/0x460
   do_vfs_ioctl+0xb0/0x918
   ksys_ioctl+0x50/0x80
   sys_ioctl+0x34/0x48
   __sys_trace_return+0x0/0x4

The root cause is that an attempt to modify ECMR and GECMR registers
only when RX/TX function is disabled was too overcomplicated in its
original implementation, also processing of an optional Link Change
interrupt added even more complexity, as a result the implementation
was error prone.

The new locking scheme is confirmed to be correct by dumping driver
specific and generic PHY framework function calls with aid of ftrace
while running more or less advanced tests.

Please note that sh_eth patches from the series were built-tested only.

On purpose I do not add Fixes tags, the reused PHY handlers were added
way later than the fixed problems were firstly found in the drivers.

Vladimir Zapolskiy (6):
  ravb: remove custom .nway_reset from ethtool ops
  ravb: remove custom .get_link_ksettings from ethtool ops
  ravb: remove custom .set_link_ksettings from ethtool ops
  sh_eth: remove custom .nway_reset from ethtool ops
  sh_eth: remove custom .get_link_ksettings from ethtool ops
  sh_eth: remove custom .set_link_ksettings from ethtool ops

 drivers/net/ethernet/renesas/ravb_main.c | 93 ++-
 drivers/net/ethernet/renesas/sh_eth.c| 94 ++--
 2 files changed, 34 insertions(+), 153 deletions(-)

-- 
2.8.1

[PATCH 4/6] sh_eth: remove custom .nway_reset from ethtool ops

[Vladimir Zapolskiy]

The change fixes a sleep in atomic context issue, which can be
always triggered by running 'ethtool -r' command, because
phy_start_aneg() protects phydev fields by a mutex.

Another note is that the change implicitly replaces phy_start_aneg()
with a newer phy_restart_aneg().

Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 18 +-
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c 
b/drivers/net/ethernet/renesas/sh_eth.c
index d9cadfb1bc4a..6d1fed2b4a4a 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2252,22 +2252,6 @@ static void sh_eth_get_regs(struct net_device *ndev, 
struct ethtool_regs *regs,
pm_runtime_put_sync(>pdev->dev);
 }
 
-static int sh_eth_nway_reset(struct net_device *ndev)
-{
-   struct sh_eth_private *mdp = netdev_priv(ndev);
-   unsigned long flags;
-   int ret;
-
-   if (!ndev->phydev)
-   return -ENODEV;
-
-   spin_lock_irqsave(>lock, flags);
-   ret = phy_start_aneg(ndev->phydev);
-   spin_unlock_irqrestore(>lock, flags);
-
-   return ret;
-}
-
 static u32 sh_eth_get_msglevel(struct net_device *ndev)
 {
struct sh_eth_private *mdp = netdev_priv(ndev);
@@ -2418,7 +2402,7 @@ static int sh_eth_set_wol(struct net_device *ndev, struct 
ethtool_wolinfo *wol)
 static const struct ethtool_ops sh_eth_ethtool_ops = {
.get_regs_len   = sh_eth_get_regs_len,
.get_regs   = sh_eth_get_regs,
-   .nway_reset = sh_eth_nway_reset,
+   .nway_reset = phy_ethtool_nway_reset,
.get_msglevel   = sh_eth_get_msglevel,
.set_msglevel   = sh_eth_set_msglevel,
.get_link   = ethtool_op_get_link,
-- 
2.8.1

[PATCH 2/6] ravb: remove custom .get_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

The change replaces a custom implementation of .get_link_ksettings
callback with a shared phy_ethtool_get_link_ksettings(), note that
>lock wrapping is not needed, because the lock does not
serialize access to phydev fields.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 18 +-
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
b/drivers/net/ethernet/renesas/ravb_main.c
index 4a043eb0e2aa..3d91caa44176 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1096,22 +1096,6 @@ static int ravb_phy_start(struct net_device *ndev)
return 0;
 }
 
-static int ravb_get_link_ksettings(struct net_device *ndev,
-  struct ethtool_link_ksettings *cmd)
-{
-   struct ravb_private *priv = netdev_priv(ndev);
-   unsigned long flags;
-
-   if (!ndev->phydev)
-   return -ENODEV;
-
-   spin_lock_irqsave(>lock, flags);
-   phy_ethtool_ksettings_get(ndev->phydev, cmd);
-   spin_unlock_irqrestore(>lock, flags);
-
-   return 0;
-}
-
 static int ravb_set_link_ksettings(struct net_device *ndev,
   const struct ethtool_link_ksettings *cmd)
 {
@@ -1372,7 +1356,7 @@ static const struct ethtool_ops ravb_ethtool_ops = {
.get_ringparam  = ravb_get_ringparam,
.set_ringparam  = ravb_set_ringparam,
.get_ts_info= ravb_get_ts_info,
-   .get_link_ksettings = ravb_get_link_ksettings,
+   .get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = ravb_set_link_ksettings,
.get_wol= ravb_get_wol,
.set_wol= ravb_set_wol,
-- 
2.8.1

[PATCH 3/6] ravb: remove custom .set_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

The change replaces a custom implementation of .set_link_ksettings
callback with a shared phy_ethtool_set_link_ksettings(), this fixes
sleep in atomic context bug, which is encountered every time when link
settings are changed by ethtool.

Now duplex mode setting is enforced in ravb_adjust_link() only, also
now TX/RX is disabled when link is put down or modifications to E-MAC
registers ECMR and GECMR are expected for both cases of checked and
ignored link status pin state from E-MAC interrupt handler.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 58 +---
 1 file changed, 15 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
b/drivers/net/ethernet/renesas/ravb_main.c
index 3d91caa44176..0d811c02ff34 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -980,6 +980,13 @@ static void ravb_adjust_link(struct net_device *ndev)
struct ravb_private *priv = netdev_priv(ndev);
struct phy_device *phydev = ndev->phydev;
bool new_state = false;
+   unsigned long flags;
+
+   spin_lock_irqsave(>lock, flags);
+
+   /* Disable TX and RX right over here, if E-MAC change is ignored */
+   if (priv->no_avb_link)
+   ravb_rcv_snd_disable(ndev);
 
if (phydev->link) {
if (phydev->duplex != priv->duplex) {
@@ -997,18 +1004,21 @@ static void ravb_adjust_link(struct net_device *ndev)
ravb_modify(ndev, ECMR, ECMR_TXF, 0);
new_state = true;
priv->link = phydev->link;
-   if (priv->no_avb_link)
-   ravb_rcv_snd_enable(ndev);
}
} else if (priv->link) {
new_state = true;
priv->link = 0;
priv->speed = 0;
priv->duplex = -1;
-   if (priv->no_avb_link)
-   ravb_rcv_snd_disable(ndev);
}
 
+   /* Enable TX and RX right over here, if E-MAC change is ignored */
+   if (priv->no_avb_link && phydev->link)
+   ravb_rcv_snd_enable(ndev);
+
+   mmiowb();
+   spin_unlock_irqrestore(>lock, flags);
+
if (new_state && netif_msg_link(priv))
phy_print_status(phydev);
 }
@@ -1096,44 +1106,6 @@ static int ravb_phy_start(struct net_device *ndev)
return 0;
 }
 
-static int ravb_set_link_ksettings(struct net_device *ndev,
-  const struct ethtool_link_ksettings *cmd)
-{
-   struct ravb_private *priv = netdev_priv(ndev);
-   unsigned long flags;
-   int error;
-
-   if (!ndev->phydev)
-   return -ENODEV;
-
-   spin_lock_irqsave(>lock, flags);
-
-   /* Disable TX and RX */
-   ravb_rcv_snd_disable(ndev);
-
-   error = phy_ethtool_ksettings_set(ndev->phydev, cmd);
-   if (error)
-   goto error_exit;
-
-   if (cmd->base.duplex == DUPLEX_FULL)
-   priv->duplex = 1;
-   else
-   priv->duplex = 0;
-
-   ravb_set_duplex(ndev);
-
-error_exit:
-   mdelay(1);
-
-   /* Enable TX and RX */
-   ravb_rcv_snd_enable(ndev);
-
-   mmiowb();
-   spin_unlock_irqrestore(>lock, flags);
-
-   return error;
-}
-
 static u32 ravb_get_msglevel(struct net_device *ndev)
 {
struct ravb_private *priv = netdev_priv(ndev);
@@ -1357,7 +1329,7 @@ static const struct ethtool_ops ravb_ethtool_ops = {
.set_ringparam  = ravb_set_ringparam,
.get_ts_info= ravb_get_ts_info,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
-   .set_link_ksettings = ravb_set_link_ksettings,
+   .set_link_ksettings = phy_ethtool_set_link_ksettings,
.get_wol= ravb_get_wol,
.set_wol= ravb_set_wol,
 };
-- 
2.8.1

Re: [PATCH v9 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/18/2018 05:40 PM, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Reviewed-by: Rob Herring <r...@kernel.org>
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 
> ++
>  1 file changed, 60 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> 

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

--
With best wishes,
Vladimir

Re: [PATCH v9 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/18/2018 05:40 PM, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
> 
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/Kconfig|   6 +
>  drivers/gpu/drm/bridge/Makefile   |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 206 
> ++
>  3 files changed, 213 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

--
With best wishes,
Vladimir

Re: [PATCH v8 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
> 
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

Generally I have only one pretty ignorable comment.

> +
> +enum thc63_ports {
> + THC63_LVDS_IN0,
> + THC63_LVDS_IN1,
> + THC63_RGB_OUT0,
> + THC63_RGB_OUT1,
> +};
> +

The driver uses only THC63_RGB_OUT0 value, or port@2, and MODE{0,1,2} IC
configuration is ignored.

I don't know if right from the beginning it would be better to support
dual-out modes, preferably both single-in and dual-in ones. Will it
impact port enumeration?

I do understand that the extension is possible, and likely only hardware
accessibility postpones it.

--
With best wishes,
Vladimir

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/19/2018 12:48 PM, Vladimir Zapolskiy wrote:
> Hi Jacopo,
> 
> On 04/19/2018 12:44 PM, Vladimir Zapolskiy wrote:
>> Hi Jacopo, Laurent,
>>
>> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
>>> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>>
>> Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
>>
>>> ---

[snip]

>>> +Optional properties:
>>> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
>>
> 
> sorry for a follow-up, I've just noticed it, could you please double check
> spelling PDWN vs PWDN? Thank you in advance.
> 

please ignore it, I did it myself and the datasheet describes pin as /PDWN,
I won't exclude a typo in the datasheet though...

--
With best wishes,
Vladimir

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/19/2018 12:44 PM, Vladimir Zapolskiy wrote:
> Hi Jacopo, Laurent,
> 
> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
>> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> 
> Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
> 
>> ---
>>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 
>> ++
>>  1 file changed, 60 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt 
>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>> new file mode 100644
>> index 000..0b23e70
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>> @@ -0,0 +1,60 @@
>> +Thine Electronics THC63LVD1024 LVDS decoder
>> +---
>> +
>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS 
>> streams
>> +to parallel data outputs. The chip supports single/dual input/output modes,
>> +handling up to two LVDS input streams and up to two digital CMOS/TTL 
>> outputs.
>> +
>> +Single or dual operation mode, output data mapping and DDR output modes are
>> +configured through input signals and the chip does not expose any control 
>> bus.
>> +
>> +Required properties:
>> +- compatible: Shall be "thine,thc63lvd1024"
>> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
>> +  PPL and digital circuitry
>> +
>> +Optional properties:
>> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> 

sorry for a follow-up, I've just noticed it, could you please double check
spelling PDWN vs PWDN? Thank you in advance.

--
With best wishes,
Vladimir

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo, Laurent,

On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 
> ++
>  1 file changed, 60 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> 
> diff --git 
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt 
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> new file mode 100644
> index 000..0b23e70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,60 @@
> +Thine Electronics THC63LVD1024 LVDS decoder
> +---
> +
> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS 
> streams
> +to parallel data outputs. The chip supports single/dual input/output modes,
> +handling up to two LVDS input streams and up to two digital CMOS/TTL outputs.
> +
> +Single or dual operation mode, output data mapping and DDR output modes are
> +configured through input signals and the chip does not expose any control 
> bus.
> +
> +Required properties:
> +- compatible: Shall be "thine,thc63lvd1024"
> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> +  PPL and digital circuitry
> +
> +Optional properties:
> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low

Thank you for the change.

I would suggest to rename 'pwdn-gpios' property of THC63LVDM83D as well,
as far as I understand it is only described in DT bindings documentation,
and the property is unused in the driver or board DTS files at the moment.

> +- oe-gpios: Output enable GPIO signal, pin name "OE". Active high

Okay :)

--
With best wishes,
Vladimir

[PATCH 0/2] spi: sh-msiof: Fix bit field overflow writes to TSCR/RSCR

[Vladimir Zapolskiy]

The changeset fixes a bit field overflow which allows to write to higher
bits while calculating SPI transfer clock and setting BRPS and BRDV bit
fields, the problem is reproduced if 'parent_rate' to 'spi_hz' ratio is
greater than 1024.

I attach the second patch to the series, because actually the squashed
change is the sole fix as well, the split was done only to simplify
backporting of the most essential part.

The reorganization of sh_msiof_spi_set_clk_regs() function allows to
avoid min_t()/max_t(), break/continue in the loop etc., which hopefully
is seen as a beneficial update.

Geert, I'd appreciate if you can find time to review and test the changes.

Vladimir Zapolskiy (2):
  spi: sh-msiof: Fix bit field overflow writes to TSCR/RSCR
  spi: sh-msiof: Simplify calculation of divisors for transfer rate

 drivers/spi/spi-sh-msiof.c | 66 --
 1 file changed, 35 insertions(+), 31 deletions(-)

-- 
2.8.1

[PATCH 2/2] spi: sh-msiof: Simplify calculation of divisors for transfer rate

[Vladimir Zapolskiy]

The change updates sh_msiof_spi_set_clk_regs() function by iterating
over BRDV power values. Note that the change is a functional one, namely
prescaler output x 1/1 set in BRDV bit field (0b111) for MSO division
rate set to 2 is substituted by BRDV = 0b000 and BRPS = 0b0, in terms
of written values to TSCR setting of 0x0107 is substituted by 0x,
and for all input parameter cases this is the only functional change,
which touches the controller.

As a result of the rework the function is supposed to be slightly more
efficient and more readable and maintainable in case of any further
extensions.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 drivers/spi/spi-sh-msiof.c | 67 --
 1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 8171eedbfc90..5c1ff0097e41 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -39,7 +39,7 @@ struct sh_msiof_chipdata {
u16 tx_fifo_size;
u16 rx_fifo_size;
u16 master_flags;
-   u16 min_div;
+   u16 min_div_pow;
 };
 
 struct sh_msiof_spi_priv {
@@ -51,7 +51,7 @@ struct sh_msiof_spi_priv {
struct completion done;
unsigned int tx_fifo_size;
unsigned int rx_fifo_size;
-   unsigned int min_div;
+   unsigned int min_div_pow;
void *tx_dma_page;
void *rx_dma_page;
dma_addr_t tx_dma_addr;
@@ -249,43 +249,46 @@ static irqreturn_t sh_msiof_spi_irq(int irq, void *data)
return IRQ_HANDLED;
 }
 
-static struct {
-   unsigned short div;
-   unsigned short brdv;
-} const sh_msiof_spi_div_table[] = {
-   { 1,SCR_BRDV_DIV_1 },
-   { 2,SCR_BRDV_DIV_2 },
-   { 4,SCR_BRDV_DIV_4 },
-   { 8,SCR_BRDV_DIV_8 },
-   { 16,   SCR_BRDV_DIV_16 },
-   { 32,   SCR_BRDV_DIV_32 },
+static const u32 sh_msiof_spi_div_array[] = {
+   SCR_BRDV_DIV_1, SCR_BRDV_DIV_2,  SCR_BRDV_DIV_4,
+   SCR_BRDV_DIV_8, SCR_BRDV_DIV_16, SCR_BRDV_DIV_32,
 };
 
 static void sh_msiof_spi_set_clk_regs(struct sh_msiof_spi_priv *p,
  unsigned long parent_rate, u32 spi_hz)
 {
-   unsigned long div = 1024;
+   unsigned long div;
u32 brps, scr;
-   size_t k;
+   unsigned int div_pow = p->min_div_pow;
 
-   if (!WARN_ON(!spi_hz || !parent_rate))
-   div = DIV_ROUND_UP(parent_rate, spi_hz);
-
-   div = max_t(unsigned long, div, p->min_div);
+   if (!spi_hz || !parent_rate) {
+   WARN(1, "Invalid clock rate parameters %lu and %u\n",
+parent_rate, spi_hz);
+   return;
+   }
 
-   for (k = 0; k < ARRAY_SIZE(sh_msiof_spi_div_table); k++) {
-   brps = DIV_ROUND_UP(div, sh_msiof_spi_div_table[k].div);
+   div = DIV_ROUND_UP(parent_rate, spi_hz);
+   if (div <= 1024) {
/* SCR_BRDV_DIV_1 is valid only if BRPS is x 1/1 or x 1/2 */
-   if (sh_msiof_spi_div_table[k].div == 1 && brps > 2)
-   continue;
-   if (brps <= 32) /* max of brdv is 32 */
-   break;
-   }
+   if (!div_pow && div <= 32 && div > 2)
+   div_pow = 1;
+
+   if (div_pow)
+   brps = (div + 1) >> div_pow;
+   else
+   brps = div;
 
-   k = min_t(int, k, ARRAY_SIZE(sh_msiof_spi_div_table) - 1);
-   brps = min_t(int, brps, 32);
+   for (; brps > 32; div_pow++)
+   brps = (brps + 1) >> 1;
+   } else {
+   /* Set transfer rate composite divisor to 2^5 * 32 = 1024 */
+   dev_err(>pdev->dev,
+   "Requested SPI transfer rate %d is too low\n", spi_hz);
+   div_pow = 5;
+   brps = 32;
+   }
 
-   scr = sh_msiof_spi_div_table[k].brdv | SCR_BRPS(brps);
+   scr = sh_msiof_spi_div_array[div_pow] | SCR_BRPS(brps);
sh_msiof_write(p, TSCR, scr);
if (!(p->master->flags & SPI_MASTER_MUST_TX))
sh_msiof_write(p, RSCR, scr);
@@ -1041,21 +1044,21 @@ static const struct sh_msiof_chipdata sh_data = {
.tx_fifo_size = 64,
.rx_fifo_size = 64,
.master_flags = 0,
-   .min_div = 1,
+   .min_div_pow = 0,
 };
 
 static const struct sh_msiof_chipdata rcar_gen2_data = {
.tx_fifo_size = 64,
.rx_fifo_size = 64,
.master_flags = SPI_MASTER_MUST_TX,
-   .min_div = 1,
+   .min_div_pow = 0,
 };
 
 static const struct sh_msiof_chipdata rcar_gen3_data = {
.tx_fifo_size = 64,
.rx_fifo_size = 64,
.master_flags = SPI_MASTER_MUST_TX,
-   .min_div = 2,
+   .min_div_pow = 1,
 };
 
 static const struct of_device_id sh_msiof_match[] = {
@@ -1319,7 

[PATCH 1/2] spi: sh-msiof: Fix bit field overflow writes to TSCR/RSCR

[Vladimir Zapolskiy]

The change fixes a bit field overflow which allows to write to higher
bits while calculating SPI transfer clock and setting BRPS and BRDV
bit fields, the problem is reproduced if 'parent_rate' to 'spi_hz'
ratio is greater than 1024, for instance

  p->min_div  = 2,
  MSO rate= ,
  SPI device rate = 1

results in

  k  = 5, i.e. BRDV = 0b100 or 1/32 prescaler output,
  BRPS   = 105,
  TSCR value = 0x6804, thus MSSEL and MSIMM bit fields are non-zero.

Fixes: 65d5665bb260 ("spi: sh-msiof: Update calculation of frequency dividing")
Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 drivers/spi/spi-sh-msiof.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index ae086aab57d5..8171eedbfc90 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -283,6 +283,7 @@ static void sh_msiof_spi_set_clk_regs(struct 
sh_msiof_spi_priv *p,
}
 
k = min_t(int, k, ARRAY_SIZE(sh_msiof_spi_div_table) - 1);
+   brps = min_t(int, brps, 32);
 
scr = sh_msiof_spi_div_table[k].brdv | SCR_BRPS(brps);
sh_msiof_write(p, TSCR, scr);
-- 
2.8.1

Re: [PATCH] i2c: rcar: initialize earlier using subsys_initcall()

[Vladimir Zapolskiy]

Hi Wolfram,

On 04/03/2018 06:55 PM, Wolfram Sang wrote:
> Hi Vladimir and Eugeniu,
> 
>> The purpose of this patch looks pretty similar to:
>> 104522806a7d ("i2c: designware: dw_i2c_init_driver as subsys initcall")
>> 74f56c4ad4e4 ("i2c-bfin-twi: move setup to the earlier subsys initcall")
>> b8680784875b ("i2c-gpio: Move initialization code to subsys_initcall()")
>> 5d3f33318a6c ("[PATCH] i2c-imx: make bus available early")
>> ccb3bc16b489 ("i2c-sh_mobile: change module_init() to subsys_initcall()")
>> 18dc83a6ea48 ("i2c: i2c-s3c2410: Initialise Samsung I2C controller early")
>> 2514cca06be9 ("[ARM] 5394/1: Add static bus numbering support to 
>> i2c-versatile")
>> 47a9b1379a5e ("i2c-pxa: Initialize early")
>>
>> However, the story behind it might be a bit different.
> 
> It definately is. The above drivers are very old, from the days where
> -EPROBE_DEFER was non-existant. They needed subsys_initcall to be able
> to boot. The reason they still have it is simple: reverting to
> module_initcall could cause regressions.
> 
> This patch is about boot-time. Very different.
> 
>> Experimenting with async probing in various rcar-specific drivers (e.g.
>> rcar-du, vsp1, rcar-fcp, rcar-vin), it was noticed that in most of
>> the cases enabling async probing in one driver introduced some degree of
>> inconsistency in the initialization of other builtin drivers.
> 
> I have to admit I never played with async probing...
> 
>> To give an example, with pure sequential driver initialization, based
>> on 5 dmesg logs, the builtin kernel initialization deviation is
>> around +/- 5ms, whereas after enabling async probing in e.g. vsp1
>> driver, the variance is increased to sometimes +/- 80ms or more.
> 
> ... so I can't tell if there is something which can be fixed on the
> async probe side. I would naively think so.
> 
>> This patch fixes it and keeps the startup time consistent after
>> switching certain i2c-dependent drivers to asynchronous probing on
>> H3-es20-Salvator-X target.
> 
> I am not convinced "fix" is the right word here. Why is it not just a
> workaround? Are there no other possibilities? I know other solutions are
> usually complicated, but playing with init levels is fragile and caused
> circular dependencies in the past, so I really don't like them.

in summary (and according to my understanding, Eugeniu please feel free
to correct me) the actual product boards have multitude of I2C devices
connected to the master controller. It requires to read EDID to enable
video output or init sensors to get video input and so on, and normally
device drivers of endpoint devices are executed on module_init() level.

Thus putting an I2C master controller device driver to the same late
init level means that due to the concurrency there will be lots of
probe defers of endpoint device drivers, and making "heavy" device
drivers like rcar-vin to be run in asyncronous probe increases boot
time dispersion (rcar-vin is already probed, it's time to probe a
sensor, but I2C controller is not yet ready to operate, defer).

I don't suppose that the proposed change has any single negative
side effect, well, right, probe/remove code becomes more awkward,
but in general the expected effect to boot time improvement should
cover it, I hope.

>> Another effect seems to be improving the init time of rcar_i2c_driver
>> itself from ~7ms to ~1ms (assuming CONFIG_I2C_RCAR=y).
> 
> That doesn't help the patch much ;), but still interesting because I didn't
> expect that. Do you have an idea why?
> 

--
With best wishes,
Vladimir

Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/27/2018 01:10 PM, jacopo mondi wrote:
> Hi Vladimir,
> 
> On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote:
>> Hi Jacopo,
>>
>> On 03/27/2018 11:57 AM, jacopo mondi wrote:
>>> Hi Vladimir,
>>>
>>> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote:
>>>> Hi Sergei,
>>>>
>>>> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote:
>>>>> Hello!
>>>>>
>>>>> On 3/27/2018 10:33 AM, jacopo mondi wrote:
>>>>> [...]
>>>>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>>>>>>>>>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>>>>>>>>>>> Reviewed-by: Niklas Söderlund 
>>>>>>>>>>> <niklas.soderlund+rene...@ragnatech.se>
>>>>>>>>>>> ---
>>>>>>>>>>>   .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 
>>>>>>>>>>> +++
>>>>>>>>>>>   1 file changed, 66 insertions(+)
>>>>>>>>>>>   create mode 100644
>>>>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>>>>
>>>>>>>>>>> diff --git
>>>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>>>> new file mode 100644
>>>>>>>>>>> index 000..8225c6a
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++
>>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>>>> @@ -0,0 +1,66 @@
>>>>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder
>>>>>>>>>>> +---
>>>>>>>>>>> +
>>>>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert 
>>>>>>>>>>> LVDS
>>>>>>>>>>> streams
>>>>>>>>>>> +to parallel data outputs. The chip supports single/dual 
>>>>>>>>>>> input/output modes,
>>>>>>>>>>> +handling up to two two input LVDS stream and up to two digital 
>>>>>>>>>>> CMOS/TTL
>>>>>>>>>>> outputs.
>>>>>>>>>>> +
>>>>>>>>>>> +Single or dual operation modes, output data mapping and DDR output 
>>>>>>>>>>> modes
>>>>>>>>>>> are
>>>>>>>>>>> +configured through input signals and the chip does not expose any 
>>>>>>>>>>> control
>>>>>>>>>>> bus.
>>>>>>>>>>> +
>>>>>>>>>>> +Required properties:
>>>>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024"
>>>>>>>>>>> +
>>>>>>>>>>> +Optional properties:
>>>>>>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry
>>>>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
>>>>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs
>>>>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry
>>>>>>>>>> As explained in a comment to one of the previous versions of this 
>>>>>>>>>> series, I'm
>>>>>>>>>> tempted to make vcc-supply mandatory and drop the three other power 
>>>>>>>>>> supplies
>>>>>>>>>> for now, as I believe there's very little chance they will be 
>>>>>>>>>> connected to
>>>>>>>>>> separately controllable regulators (all supplies use the same 
>>>>

Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Vladimir Zapolskiy]

Hi Andrzej,

On 03/27/2018 10:28 AM, Andrzej Hajda wrote:
> On 27.03.2018 08:24, Vladimir Zapolskiy wrote:
>> Hi Jacopo,
>>
>> On 03/16/2018 05:16 PM, Jacopo Mondi wrote:
>>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>>> output converter.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
>>> ---
>>>  drivers/gpu/drm/bridge/Kconfig|   6 +
>>>  drivers/gpu/drm/bridge/Makefile   |   1 +
>>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 255 
>>> ++
>>>  3 files changed, 262 insertions(+)
>>>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>> index 3b99d5a..5815a20 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -92,6 +92,12 @@ config DRM_SII9234
>>>   It is an I2C driver, that detects connection of MHL bridge
>>>   and starts encapsulation of HDMI signal.
>>>  
>>> +config DRM_THINE_THC63LVD1024
>>> +   tristate "Thine THC63LVD1024 LVDS decoder bridge"
>>> +   depends on OF
>>> +   ---help---
>>> + Thine THC63LVD1024 LVDS/parallel converter driver.
>>> +
>>>  config DRM_TOSHIBA_TC358767
>>> tristate "Toshiba TC358767 eDP bridge"
>>> depends on OF
>>> diff --git a/drivers/gpu/drm/bridge/Makefile 
>>> b/drivers/gpu/drm/bridge/Makefile
>>> index 373eb28..fd90b16 100644
>>> --- a/drivers/gpu/drm/bridge/Makefile
>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>>>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
>>> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>>>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
>>> b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> new file mode 100644
>>> index 000..02a54c2
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> @@ -0,0 +1,255 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
>>> + *
>>> + * Copyright (C) 2018 Jacopo Mondi <jacopo+rene...@jmondi.org>
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +enum {
>>> +   THC63_LVDS_IN0,
>>> +   THC63_LVDS_IN1,
>>> +   THC63_DIGITAL_OUT0,
>>> +   THC63_DIGITAL_OUT1,
>>> +};
>>> +
>>> +static const char * const thc63_reg_names[] = {
>>> +   "vcc", "lvcc", "pvcc", "cvcc",
>>> +};
>>> +
>>> +struct thc63_dev {
>>> +   struct device *dev;
>>> +
>>> +   struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
>>> +
>>> +   struct gpio_desc *pdwn;
>>> +   struct gpio_desc *oe;
>>> +
>>> +   struct drm_bridge bridge;
>>> +   struct drm_bridge *next;
>>> +};
>>> +
>>> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
>>> +{
>>> +   return container_of(bridge, struct thc63_dev, bridge);
>>> +}
>>> +
>>> +static int thc63_attach(struct drm_bridge *bridge)
>>> +{
>>> +   struct thc63_dev *thc63 = to_thc63(bridge);
>>> +
>>> +   return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
>>> +}
>>> +
>>> +static void thc63_enable(struct drm_bridge *bridge)
>>> +{
>>> +   struct thc63_dev *thc63 = to_thc63(bridge);
>>> +   struct regulator *vcc;
>>> +   int i;
>> unsigned int i;
> 
> Why? You are introducing silly bug this way, see few lines below.

You see that the comment was too terse to describe the context in full.
Thank you for exposing a flaw.

>>
>>> +
>>> +   for (i = 0; i < ARRAY_SIZE(thc63->vccs);

Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/27/2018 11:57 AM, jacopo mondi wrote:
> Hi Vladimir,
> 
> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote:
>> Hi Sergei,
>>
>> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote:
>>> Hello!
>>>
>>> On 3/27/2018 10:33 AM, jacopo mondi wrote:
>>> [...]
>>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>>>>>>>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>>>>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
>>>>>>>>> ---
>>>>>>>>>   .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 
>>>>>>>>> +++
>>>>>>>>>   1 file changed, 66 insertions(+)
>>>>>>>>>   create mode 100644
>>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000..8225c6a
>>>>>>>>> --- /dev/null
>>>>>>>>> +++
>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>> @@ -0,0 +1,66 @@
>>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder
>>>>>>>>> +---
>>>>>>>>> +
>>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert 
>>>>>>>>> LVDS
>>>>>>>>> streams
>>>>>>>>> +to parallel data outputs. The chip supports single/dual input/output 
>>>>>>>>> modes,
>>>>>>>>> +handling up to two two input LVDS stream and up to two digital 
>>>>>>>>> CMOS/TTL
>>>>>>>>> outputs.
>>>>>>>>> +
>>>>>>>>> +Single or dual operation modes, output data mapping and DDR output 
>>>>>>>>> modes
>>>>>>>>> are
>>>>>>>>> +configured through input signals and the chip does not expose any 
>>>>>>>>> control
>>>>>>>>> bus.
>>>>>>>>> +
>>>>>>>>> +Required properties:
>>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024"
>>>>>>>>> +
>>>>>>>>> +Optional properties:
>>>>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry
>>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
>>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs
>>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry
>>>>>>>> As explained in a comment to one of the previous versions of this 
>>>>>>>> series, I'm
>>>>>>>> tempted to make vcc-supply mandatory and drop the three other power 
>>>>>>>> supplies
>>>>>>>> for now, as I believe there's very little chance they will be 
>>>>>>>> connected to
>>>>>>>> separately controllable regulators (all supplies use the same 
>>>>>>>> voltage). In the
>>>>>>>> very unlikely event that this occurs in design we need to support in 
>>>>>>>> the
>>>>>>>> future, the cvcc, lvcc and pvcc supplies can be added later as optional
>>>>>>>> without breaking backward compatibility.
>>>>>>> I'm okay with that.
>>>>>>>
>>>>>>>> Apart from that,
>>>>>>>>
>>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>>>>>>>>
>>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low
>

Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Sergei,

On 03/27/2018 11:27 AM, Sergei Shtylyov wrote:
> Hello!
> 
> On 3/27/2018 10:33 AM, jacopo mondi wrote:
> [...]
>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>
>>> Signed-off-by: Jacopo Mondi 
>>> Reviewed-by: Andrzej Hajda 
>>> Reviewed-by: Niklas Söderlund 
>>> ---
>>>   .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 
>>> +++
>>>   1 file changed, 66 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> new file mode 100644
>>> index 000..8225c6a
>>> --- /dev/null
>>> +++
>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> @@ -0,0 +1,66 @@
>>> +Thine Electronics THC63LVD1024 LVDS decoder
>>> +---
>>> +
>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
>>> streams
>>> +to parallel data outputs. The chip supports single/dual input/output 
>>> modes,
>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL
>>> outputs.
>>> +
>>> +Single or dual operation modes, output data mapping and DDR output 
>>> modes
>>> are
>>> +configured through input signals and the chip does not expose any 
>>> control
>>> bus.
>>> +
>>> +Required properties:
>>> +- compatible: Shall be "thine,thc63lvd1024"
>>> +
>>> +Optional properties:
>>> +- vcc-supply: Power supply for TTL output and digital circuitry
>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
>>> +- lvcc-supply: Power supply for LVDS inputs
>>> +- pvcc-supply: Power supply for PLL circuitry
>> As explained in a comment to one of the previous versions of this 
>> series, I'm
>> tempted to make vcc-supply mandatory and drop the three other power 
>> supplies
>> for now, as I believe there's very little chance they will be connected 
>> to
>> separately controllable regulators (all supplies use the same voltage). 
>> In the
>> very unlikely event that this occurs in design we need to support in the
>> future, the cvcc, lvcc and pvcc supplies can be added later as optional
>> without breaking backward compatibility.
> I'm okay with that.
>
>> Apart from that,
>>
>> Reviewed-by: Laurent Pinchart 
>>
>>> +- pdwn-gpios: Power down GPIO signal. Active low
> powerdown-gpios is the semi-standard name.
>
 right, I've also noticed it. If possible please avoid shortenings in
 property names.
>>>
>>> It is not shortening, it just follow pin name from decoder's datasheet.
>>>

>>> +- oe-gpios: Output enable GPIO signal. Active high
>>> +
 And this one is also a not ever met property name, please consider to
 rename it to 'enable-gpios', for instance display panels define it.
>>>
>>>
>>> Again, it follows datasheet naming scheme. Has something changed in DT
>>> conventions?
>>
>> Seconded. My understanding is that the property name should reflect
>> what reported in the the chip manual. For THC63LVD1024 the enable and
>> power down pins are named 'OE' and 'PDWN' respectively.
> 
> But don't we need the vendor prefix in the prop names then, like 
> "renesas,oe-gpios" then?
> 

Seconded, with a correction to "thine,oe-gpios".

If vendor agnostic properties are supposed to be used, then please follow
the referenced by Rob semi-standard notations.

--
With best wishes,
Vladimir

Re: [PATCH v6 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/20/2018 03:01 PM, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Friday, 16 March 2018 17:16:39 EET Jacopo Mondi wrote:
>> The R-Car V3M Eagle board includes a transparent THC63LVD1024 LVDS
>> decoder, connected to the on-chip LVDS encoder output on one side
>> and to HDMI encoder ADV7511w on the other one.
>>
>> As the decoder does not need any configuration it has been so-far
>> omitted from DTS. Now that a driver is available, describe it in DT
>> as well.
>>
>> Signed-off-by: Jacopo Mondi 
>> Reviewed-by: Andrzej Hajda 
> 
> The patch looks OK to me, but I think it should be squashed with Niklas' 
> patch 
> that added display HDMI output support to the V3M Eagle DT.
> 
>> ---
>>
>> List of patch dependencies, as of renesas-drivers-2018-03-13-v4.16-rc5:
>>
>> - [PATCH v2 0/5] arm64: dts: renesas: r8a77970: enable HDMI output
>>which includes DU, LVDS and FCPD enablement from:
>>   [PATCH v2 0/5] Add R8A77970/V3MSK LVDS/HDMI support
>> - [PATCH v4] v4l: vsp1: Fix video output on R8A77970
>>
>> Patches to be applied on top of
>> "arm64: dts: renesas: eagle: add HDMI output using the ADV7511W"
>>
>> Thanks
>>j
>> ---
>>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 33 ---
>>  1 file changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
>> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index c0fd144..69f43b8
>> 100644
>> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
>> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
>> @@ -42,6 +42,33 @@
>>  };
>>  };
>>  };
>> +
>> +thc63lvd1024: lvds-decoder {
>> +compatible = "thine,thc63lvd1024";
>> +
>> +ports {
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +port@0 {
>> +reg = <0>;
>> +
>> +thc63lvd1024_in_0: endpoint {
>> +remote-endpoint = <_out>;
>> +};
>> +};
>> +
>> +port@2{
>> +reg = <2>;
>> +
>> +thc63lvd1024_out_2: endpoint {
>> +remote-endpoint = <_in>;
>> +};
>> +

Remove the empty line above.

>> +};
>> +

Remove the empty line above.

>> +};
>> +};
>>  };
>>

--
With best wishes,
Vladimir

Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/16/2018 05:16 PM, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
> 
> Signed-off-by: Jacopo Mondi 
> Reviewed-by: Andrzej Hajda 
> Reviewed-by: Niklas Söderlund 
> ---
>  drivers/gpu/drm/bridge/Kconfig|   6 +
>  drivers/gpu/drm/bridge/Makefile   |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 255 
> ++
>  3 files changed, 262 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..5815a20 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -92,6 +92,12 @@ config DRM_SII9234
> It is an I2C driver, that detects connection of MHL bridge
> and starts encapsulation of HDMI signal.
>  
> +config DRM_THINE_THC63LVD1024
> + tristate "Thine THC63LVD1024 LVDS decoder bridge"
> + depends on OF
> + ---help---
> +   Thine THC63LVD1024 LVDS/parallel converter driver.
> +
>  config DRM_TOSHIBA_TC358767
>   tristate "Toshiba TC358767 eDP bridge"
>   depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..fd90b16 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
> b/drivers/gpu/drm/bridge/thc63lvd1024.c
> new file mode 100644
> index 000..02a54c2
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,255 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi 
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +enum {
> + THC63_LVDS_IN0,
> + THC63_LVDS_IN1,
> + THC63_DIGITAL_OUT0,
> + THC63_DIGITAL_OUT1,
> +};
> +
> +static const char * const thc63_reg_names[] = {
> + "vcc", "lvcc", "pvcc", "cvcc",
> +};
> +
> +struct thc63_dev {
> + struct device *dev;
> +
> + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> +
> + struct gpio_desc *pdwn;
> + struct gpio_desc *oe;
> +
> + struct drm_bridge bridge;
> + struct drm_bridge *next;
> +};
> +
> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct thc63_dev, bridge);
> +}
> +
> +static int thc63_attach(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> +
> + return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> +}
> +
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + struct regulator *vcc;
> + int i;

unsigned int i;

> +
> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> + vcc = thc63->vccs[i];
> + if (!vcc)
> + continue;
> +
> + if (regulator_enable(vcc))
> + goto error_vcc_enable;
> + }
> +
> + if (thc63->pdwn)
> + gpiod_set_value(thc63->pdwn, 0);
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 1);
> +
> + return;
> +
> +error_vcc_enable:
> + dev_err(thc63->dev, "Failed to enable regulator %s\n",
> + thc63_reg_names[i]);
> +
> + for (i = i - 1; i >= 0; i--) {
> + vcc = thc63->vccs[i];
> + if (!vcc)
> + continue;
> +
> + regulator_disable(vcc);
> + }
> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + struct regulator *vcc;
> + int i;
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 0);
> +
> + if (thc63->pdwn)
> + gpiod_set_value(thc63->pdwn, 1);
> +
> + for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) {
> + vcc = thc63->vccs[i];
> + if (!vcc)
> + continue;
> +
> + if (regulator_disable(vcc))
> + dev_dbg(thc63->dev,
> + "Failed to disable regulator %s\n",
> + thc63_reg_names[i]);
> + }
> +}
> +
> +struct drm_bridge_funcs thc63_bridge_func = {

Apparently you missed all my comments from v2 review.

If you like to avoid non-NULL function pointers 

Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/27/2018 01:22 AM, Rob Herring wrote:
> On Tue, Mar 20, 2018 at 02:43:33PM +0200, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> (CC'ing Rob)
>>
>> Thank you for the patch.
>>
>> On Friday, 16 March 2018 17:16:37 EET Jacopo Mondi wrote:
>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>
>>> Signed-off-by: Jacopo Mondi 
>>> Reviewed-by: Andrzej Hajda 
>>> Reviewed-by: Niklas Söderlund 
>>> ---
>>>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++
>>>  1 file changed, 66 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> new file mode 100644
>>> index 000..8225c6a
>>> --- /dev/null
>>> +++
>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> @@ -0,0 +1,66 @@
>>> +Thine Electronics THC63LVD1024 LVDS decoder
>>> +---
>>> +
>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
>>> streams
>>> +to parallel data outputs. The chip supports single/dual input/output modes,
>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL
>>> outputs.
>>> +
>>> +Single or dual operation modes, output data mapping and DDR output modes
>>> are
>>> +configured through input signals and the chip does not expose any control
>>> bus.
>>> +
>>> +Required properties:
>>> +- compatible: Shall be "thine,thc63lvd1024"
>>> +
>>> +Optional properties:
>>> +- vcc-supply: Power supply for TTL output and digital circuitry
>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
>>> +- lvcc-supply: Power supply for LVDS inputs
>>> +- pvcc-supply: Power supply for PLL circuitry
>>
>> As explained in a comment to one of the previous versions of this series, 
>> I'm 
>> tempted to make vcc-supply mandatory and drop the three other power supplies 
>> for now, as I believe there's very little chance they will be connected to 
>> separately controllable regulators (all supplies use the same voltage). In 
>> the 
>> very unlikely event that this occurs in design we need to support in the 
>> future, the cvcc, lvcc and pvcc supplies can be added later as optional 
>> without breaking backward compatibility.
> 
> I'm okay with that.
> 
>> Apart from that,
>>
>> Reviewed-by: Laurent Pinchart 
>>
>>> +- pdwn-gpios: Power down GPIO signal. Active low
> 
> powerdown-gpios is the semi-standard name.
> 

right, I've also noticed it. If possible please avoid shortenings in
property names.

>>> +- oe-gpios: Output enable GPIO signal. Active high
>>> +

And this one is also a not ever met property name, please consider to
rename it to 'enable-gpios', for instance display panels define it.

>>> +The THC63LVD1024 video port connections are modeled according
>>> +to OF graph bindings specified by
>>> Documentation/devicetree/bindings/graph.txt

[snip]

>>> +
>>> +   port@2{
>>> +   reg = <2>;
>>> +
>>> +   lvds_dec_out_2: endpoint {
>>> +   remote-endpoint = <_in>;
>>> +   };
>>> +

Drop a surplus empty line above.

>>> +   };
>>> +

Drop a surplus empty line above.

>>> +   };
>>> +   };

--
With best wishes,
Vladimir

Re: [PATCH] watchdog: renesas-wdt: Add support for WDIOF_CARDRESET

[Vladimir Zapolskiy]

Hi Wolfram,

On 03/20/2018 11:36 PM, Wolfram Sang wrote:
> From: Veeraiyan Chidambaram <veeraiyan.chidamba...@in.bosch.com>
> 
> This patch adds the WDIOF_CARDRESET support for the Renessas platform

typo, s/Renessas/Renesas/

> watchdog, to know if the board reboot is due to a watchdog reset.
> 
> This is done via the WOVF bit (bit 4) of the RWTCSRA register, which
> indicates if RWTCNT overflowed, triggering the reset in last boot.
> 
> Signed-off-by: Veeraiyan Chidambaram <veeraiyan.chidamba...@in.bosch.com>
> [takeshi.kihara.df: changed to read the RWTCSRA register while clock is
>  enabled]
> Signed-off-by: Takeshi Kihara <takeshi.kihara...@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Reviewed-by: Vladimir Zapolskiy <v...@mleia.com>

--
With best wishes,
Vladimir

Re: [PATCH v2 2/3] drm: bridge: Add LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/09/2018 03:51 PM, Jacopo Mondi wrote:
> Add transparent LVDS decoder driver.
> 
> A transparent LVDS decoder is a DRM bridge device that does not require
> any configuration and converts LVDS input to digital CMOS/TTL parallel
> data output.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/gpu/drm/bridge/Kconfig|   8 +++
>  drivers/gpu/drm/bridge/Makefile   |   1 +
>  drivers/gpu/drm/bridge/lvds-decoder.c | 129 
> ++
>  3 files changed, 138 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..e52a5af 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -32,6 +32,14 @@ config DRM_DUMB_VGA_DAC
>   help
> Support for RGB to VGA DAC based bridges
> 
> +config DRM_LVDS_DECODER
> + tristate "Transparent LVDS to parallel decoder support"
> + depends on OF
> + select DRM_PANEL_BRIDGE
> + help
> +   Support for transparent LVDS to parallel decoders that don't require
> +   any configuration.
> +
>  config DRM_LVDS_ENCODER
>   tristate "Transparent parallel to LVDS encoder support"
>   depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..edc2332 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> +obj-$(CONFIG_DRM_LVDS_DECODER) += lvds-decoder.o
>  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += 
> megachips-stdp-ge-b850v3-fw.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> diff --git a/drivers/gpu/drm/bridge/lvds-decoder.c 
> b/drivers/gpu/drm/bridge/lvds-decoder.c
> new file mode 100644
> index 000..319f4d5
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lvds-decoder.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi 
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +struct lvds_decoder {
> + struct device *dev;
> +
> + struct drm_bridge bridge;
> + struct drm_bridge *next;
> + struct drm_encoder *bridge_encoder;
> +};
> +
> +static inline struct lvds_decoder *to_lvds_decoder(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct lvds_decoder, bridge);
> +}
> +
> +static int lvds_decoder_attach(struct drm_bridge *bridge)
> +{
> + struct lvds_decoder *lvds = to_lvds_decoder(bridge);
> +
> + return drm_bridge_attach(bridge->encoder, lvds->next, bridge);
> +}
> +
> +struct drm_bridge_funcs lvds_dec_bridge_func = {

static const struct drm_bridge_funcs lvds_dec_bridge_func

> + .attach = lvds_decoder_attach,
> +};
> +
> +static int lvds_decoder_parse_dt(struct lvds_decoder *lvds)
> +{
> + struct device_node *lvds_output;
> + struct device_node *remote;
> + int ret = 0;
> +
> + lvds_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, -1);
> + if (!lvds_output) {
> + dev_err(lvds->dev, "Missing endpoint in Port@1\n");
> + return -ENODEV;
> + }
> +
> + remote = of_graph_get_remote_port_parent(lvds_output);

Add of_node_put(lvds_output) right here to get rid of error_put_lvds_node
goto label.

> + if (!remote) {
> + dev_err(lvds->dev, "Endpoint in Port@1 unconnected\n");
> + ret = -ENODEV;
> + goto error_put_lvds_node;
> + }
> +
> + if (!of_device_is_available(remote)) {
> + dev_err(lvds->dev, "Port@1 remote endpoint is disabled\n");
> + ret = -ENODEV;
> + goto error_put_remote_node;
> + }
> +
> + lvds->next = of_drm_find_bridge(remote);
> + if (!lvds->next)
> + ret = -EPROBE_DEFER;
> +
> +error_put_remote_node:
> + of_node_put(remote);
> +error_put_lvds_node:
> + of_node_put(lvds_output);
> +
> + return ret;
> +}
> +
> +static int lvds_decoder_probe(struct platform_device *pdev)
> +{
> + struct lvds_decoder *lvds;
> + int ret;
> +
> + lvds = devm_kzalloc(>dev, sizeof(*lvds), GFP_KERNEL);
> + if (!lvds)
> + return -ENOMEM;
> +
> + lvds->dev = >dev;
> + platform_set_drvdata(pdev, lvds);
> +
> + ret = lvds_decoder_parse_dt(lvds);
> + if (ret)
> + return ret;
> +
> + lvds->bridge.driver_private = lvds;
> + lvds->bridge.of_node = pdev->dev.of_node;
> + lvds->bridge.funcs = _dec_bridge_func;
> +
> + drm_bridge_add(>bridge);
> +
> + return 0;
> +}
> +
> +static int lvds_decoder_remove(struct platform_device *pdev)
> +{
> + struct lvds_decoder *lvds = 

Re: [PATCH 1/2] PCI: rcar: Poll more often in rcar_pcie_wait_for_dl()

[Vladimir Zapolskiy]

On 03/19/2018 03:48 PM, Marek Vasut wrote:
> On 03/19/2018 02:43 PM, Vladimir Zapolskiy wrote:
>> On 03/19/2018 12:56 PM, Marek Vasut wrote:
>>> On 03/19/2018 11:53 AM, Geert Uytterhoeven wrote:
>>>> Hi Marek,
>>>>
>>>> On Mon, Mar 19, 2018 at 10:53 AM, Marek Vasut <marek.va...@gmail.com> 
>>>> wrote:
>>>>> On 03/19/2018 09:38 AM, Simon Horman wrote:
>>>>>> On Sun, Mar 18, 2018 at 11:52:52AM +0100, Marek Vasut wrote:
>>>>>>> The data link active signal usually takes ~20 uSec to be asserted,
>>>>>>> poll the bit more often to avoid useless delays in this function.
>>>>>>>
>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+rene...@gmail.com>
>>>>>>> Cc: Geert Uytterhoeven <geert+rene...@glider.be>
>>>>>>> Cc: Phil Edworthy <phil.edwor...@renesas.com>
>>>>>>> Cc: Simon Horman <horms+rene...@verge.net.au>
>>>>>>> Cc: Wolfram Sang <w...@the-dreams.de>
>>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>>
>>>>>> Unless my eyes deceive me this seems to be quite a lot (100x) more often,
>>>>>> but so be it.
>>>>>
>>>>> It's just a higher frequency to avoid slowdown when bringing the link up.
>>>>
>>>> No it isn't: you replaced a sleep by a delay, thus making it blocking.
>>>
>>> For much shorter period of time.
>>>
>>>> So this can spin for up to 50 ms (+ overhead)?
>>>
>>> That's what it did before too , it used msleep and now it uses udelay.
>>>
>>
>> msleep() does not spin, it reschedules the process.
>>
>> Instead to find a balance you may want to play with usleep_range().
> 
> Which does not work in atomic context, which will be needed when using
> this function in suspend/resume later on.
> 

IIRC suspend/resume are never executed in atomic context, and runtime
suspend/resume are invoked in atomic context only if pm_runtime_irq_safe()
is called (just a few drivers in vanilla uses it at the moment).

Nevertheless, the commit message does not say that the change is needed
for future suspend/resume add-on.

--
With best wishes,
Vladimir

Re: [PATCH 1/2] PCI: rcar: Poll more often in rcar_pcie_wait_for_dl()

[Vladimir Zapolskiy]

On 03/19/2018 12:56 PM, Marek Vasut wrote:
> On 03/19/2018 11:53 AM, Geert Uytterhoeven wrote:
>> Hi Marek,
>>
>> On Mon, Mar 19, 2018 at 10:53 AM, Marek Vasut  wrote:
>>> On 03/19/2018 09:38 AM, Simon Horman wrote:
 On Sun, Mar 18, 2018 at 11:52:52AM +0100, Marek Vasut wrote:
> The data link active signal usually takes ~20 uSec to be asserted,
> poll the bit more often to avoid useless delays in this function.
>
> Signed-off-by: Marek Vasut 
> Cc: Geert Uytterhoeven 
> Cc: Phil Edworthy 
> Cc: Simon Horman 
> Cc: Wolfram Sang 
> Cc: linux-renesas-soc@vger.kernel.org

 Unless my eyes deceive me this seems to be quite a lot (100x) more often,
 but so be it.
>>>
>>> It's just a higher frequency to avoid slowdown when bringing the link up.
>>
>> No it isn't: you replaced a sleep by a delay, thus making it blocking.
> 
> For much shorter period of time.
> 
>> So this can spin for up to 50 ms (+ overhead)?
> 
> That's what it did before too , it used msleep and now it uses udelay.
> 

msleep() does not spin, it reschedules the process.

Instead to find a balance you may want to play with usleep_range().

--
With best wishes,
Vladimir

[PATCH] i2c: rcar: initialize earlier using subsys_initcall()

[Vladimir Zapolskiy]

From: Eugeniu Rosca <ero...@de.adit-jv.com>

The purpose of this patch looks pretty similar to:
104522806a7d ("i2c: designware: dw_i2c_init_driver as subsys initcall")
74f56c4ad4e4 ("i2c-bfin-twi: move setup to the earlier subsys initcall")
b8680784875b ("i2c-gpio: Move initialization code to subsys_initcall()")
5d3f33318a6c ("[PATCH] i2c-imx: make bus available early")
ccb3bc16b489 ("i2c-sh_mobile: change module_init() to subsys_initcall()")
18dc83a6ea48 ("i2c: i2c-s3c2410: Initialise Samsung I2C controller early")
2514cca06be9 ("[ARM] 5394/1: Add static bus numbering support to i2c-versatile")
47a9b1379a5e ("i2c-pxa: Initialize early")

However, the story behind it might be a bit different.
Experimenting with async probing in various rcar-specific drivers (e.g.
rcar-du, vsp1, rcar-fcp, rcar-vin), it was noticed that in most of
the cases enabling async probing in one driver introduced some degree of
inconsistency in the initialization of other builtin drivers.

To give an example, with pure sequential driver initialization, based
on 5 dmesg logs, the builtin kernel initialization deviation is
around +/- 5ms, whereas after enabling async probing in e.g. vsp1
driver, the variance is increased to sometimes +/- 80ms or more.

This patch fixes it and keeps the startup time consistent after
switching certain i2c-dependent drivers to asynchronous probing on
H3-es20-Salvator-X target.

Another effect seems to be improving the init time of rcar_i2c_driver
itself from ~7ms to ~1ms (assuming CONFIG_I2C_RCAR=y).

Signed-off-by: Eugeniu Rosca <ero...@de.adit-jv.com>
Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 drivers/i2c/busses/i2c-rcar.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 4159ebcec2bb..502f62052b49 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -972,7 +972,18 @@ static struct platform_driver rcar_i2c_driver = {
.remove = rcar_i2c_remove,
 };
 
-module_platform_driver(rcar_i2c_driver);
+static int __init rcar_i2c_driver_init(void)
+{
+   return platform_driver_register(_i2c_driver);
+}
+
+static void __exit rcar_i2c_driver_exit(void)
+{
+   return platform_driver_unregister(_i2c_driver);
+}
+
+subsys_initcall(rcar_i2c_driver_init);
+module_exit(rcar_i2c_driver_exit);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("Renesas R-Car I2C bus driver");
-- 
2.8.1

Re: [PATCH] arm64: defconfig: enable R8A77965 SoC

[Vladimir Zapolskiy]

Hi Simon,

On 02/23/2018 10:51 AM, Simon Horman wrote:
> Enable the Renesas R-Car M3-W (R8A77965) SoC in the ARM64 defconfig.

If I'm not mistaken in my tardy note, then M3-W is R8A7796 and R8A77965 is 
M3-N, no?

--
With best wishes,
Vladimir

Re: [PATCH 02/10] dt-bindings: display: renesas: Deprecate LVDS support in the DU bindings

[Vladimir Zapolskiy]

Hi Laurent,

On 01/12/2018 02:58 AM, Laurent Pinchart wrote:
> The internal LVDS encoders now have their own DT bindings, representing
> them as part of the DU is deprecated.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  .../devicetree/bindings/display/renesas,du.txt | 26 
> +-
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt 
> b/Documentation/devicetree/bindings/display/renesas,du.txt
> index cd48aba3bc8c..8f6e0e118e3e 100644
> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> @@ -17,9 +17,7 @@ Required Properties:
>- reg: A list of base address and length of each memory resource, one for
>  each entry in the reg-names property.
>- reg-names: Name of the memory resources. The DU requires one memory
> -resource for the DU core (named "du") and one memory resource for each
> -LVDS encoder (named "lvds.x" with "x" being the LVDS controller numerical
> -index).
> +resource for the DU core (named "du").
>  
>- interrupt-parent: phandle of the parent interrupt controller.
>- interrupts: Interrupt specifiers for the DU interrupts.
> @@ -29,14 +27,13 @@ Required Properties:
>- clock-names: Name of the clocks. This property is model-dependent.
>  - R8A7779 uses a single functional clock. The clock doesn't need to be
>named.
> -- All other DU instances use one functional clock per channel and one
> -  clock per LVDS encoder (if available). The functional clocks must be
> -  named "du.x" with "x" being the channel numerical index. The LVDS 
> clocks
> -  must be named "lvds.x" with "x" being the LVDS encoder numerical index.
> -- In addition to the functional and encoder clocks, all DU versions also
> -  support externally supplied pixel clocks. Those clocks are optional.
> -  When supplied they must be named "dclkin.x" with "x" being the input
> -  clock numerical index.
> +- All other DU instances use one functional clock per channel The
> +  functional clocks must be named "du.x" with "x" being the channel
> +  numerical index.
> +- In addition to the functional clocks, all DU versions also support
> +  externally supplied pixel clocks. Those clocks are optional. When
> +  supplied they must be named "dclkin.x" with "x" being the input clock
> +  numerical index.
>  
>- vsps: A list of phandle and channel index tuples to the VSPs that handle
>  the memory interfaces for the DU channels. The phandle identifies the VSP
> @@ -71,7 +68,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU
>   compatible = "renesas,du-r8a7795";
>   reg = <0 0xfeb0 0 0x8>,
> <0 0xfeb9 0 0x14>;
> - reg-names = "du", "lvds.0";
> + reg-names = "du";

Since "reg-names" list is changed, I believe you'd like to update
the "reg" property value as well.

>   interrupts = ,
>,
>,
> @@ -79,9 +76,8 @@ Example: R8A7795 (R-Car H3) ES2.0 DU
>   clocks = < CPG_MOD 724>,
>< CPG_MOD 723>,
>< CPG_MOD 722>,
> -  < CPG_MOD 721>,
> -  < CPG_MOD 727>;
> - clock-names = "du.0", "du.1", "du.2", "du.3", "lvds.0";
> +  < CPG_MOD 721>;
> + clock-names = "du.0", "du.1", "du.2", "du.3";
>   vsps = < 0>, < 0>, < 0>, < 1>;
>  
>   ports {
> 

--
With best wishes,
Vladimir

Re: [PATCH/RFT v2] gpio: gpio-rcar: Support S2RAM

[Vladimir Zapolskiy]

On 12/24/2017 03:37 PM, Yoshihiro Kaneko wrote:
> From: Hien Dang 
> 
> This patch adds an implementation that saves and restores the state of
> GPIO configuration on suspend and resume.
> 
> Signed-off-by: Hien Dang 
> Signed-off-by: Takeshi Kihara 
> [Modify structure of the bank info to simplify a saving registers]
> Signed-off-by: Yoshihiro Kaneko 
> ---

[snip]

> +static SIMPLE_DEV_PM_OPS(gpio_rcar_pm_ops,
> + gpio_rcar_suspend, gpio_rcar_resume);
> +#define DEV_PM_OPS (_rcar_pm_ops)
> +#else
> +#define DEV_PM_OPS NULL
> +#endif /* CONFIG_PM_SLEEP*/
> +
>  static int gpio_rcar_probe(struct platform_device *pdev)
>  {
>   struct gpio_rcar_priv *p;
> @@ -536,6 +604,7 @@ static int gpio_rcar_remove(struct platform_device *pdev)
>   .remove = gpio_rcar_remove,
>   .driver = {
>   .name   = "gpio_rcar",
> + .pm = DEV_PM_OPS,
>   .of_match_table = of_match_ptr(gpio_rcar_of_table),
>   }
>  };
> 

You can safely follow the next simpler pattern (add pm functions after
gpio_rcar_remove() function and remove DEV_PM_OPS macro):

#ifdef CONFIG_PM_SLEEP
static int gpio_rcar_suspend(struct device *dev)
{
...
}

static int gpio_rcar_resume(struct device *dev)
{
...
}
#endif

static SIMPLE_DEV_PM_OPS(gpio_rcar_pm_ops, gpio_rcar_suspend, gpio_rcar_resume);

static struct platform_driver gpio_rcar_device_driver = {
.probe  = gpio_rcar_probe,
.remove = gpio_rcar_remove,
.driver = {
.name   = "gpio_rcar",
.pm = _rcar_pm_ops,
.of_match_table = of_match_ptr(gpio_rcar_of_table),
}
};

--
With best wishes,
Vladimir

Re: [PATCH v2 1/2] arm64: dts: renesas: salvator-x: Remove renesas,no-ether-link property

[Vladimir Zapolskiy]

Hi Simon,

On 12/22/2017 10:40 AM, Simon Horman wrote:
> On Fri, Dec 22, 2017 at 09:32:03AM +0100, Simon Horman wrote:
>> On Thu, Dec 21, 2017 at 05:18:58PM +0200, Vladimir Zapolskiy wrote:
>>> From: Bogdan Mirea <bogdan-stefan_mi...@mentor.com>
>>>
>>> The present change is a bug fix for AVB link iteratively up/down.
>>>
>>> Steps to reproduce:
>>> - start AVB TX stream (Using aplay via MSE),
>>> - disconnect+reconnect the eth cable,
>>> - after a reconnection the eth connection goes iteratively up/down
>>>   without user interaction,
>>> - this may heal after some seconds or even stay for minutes.
>>>
>>> As the documentation specifies, the "renesas,no-ether-link" option
>>> should be used when a board does not provide a proper AVB_LINK signal.
>>> There is no need for this option enabled on RCAR H3/M3 Salvator-X/XS
>>> and ULCB starter kits since the AVB_LINK is correctly handled by HW.
>>>
>>> Choosing to keep or remove the "renesas,no-ether-link" option will
>>> have impact on the code flow in the following ways:
>>> - keeping this option enabled may lead to unexpected behavior since
>>>   the RX & TX are enabled/disabled directly from adjust_link function
>>>   without any HW interrogation,
>>> - removing this option, the RX & TX will only be enabled/disabled after
>>>   HW interrogation. The HW check is made through the LMON pin in PSR
>>>   register which specifies AVB_LINK signal value (0 - at low level;
>>>   1 - at high level).
>>>
>>> In conclusion, the present change is also a safety improvement because
>>> it removes the "renesas,no-ether-link" option leading to a proper way
>>> of detecting the link state based on HW interrogation and not on
>>> software heuristic.
>>>
>>> Fixes: d25e8ff0d5aa ("arm64: dts: renesas: Extract common Salvator-X board 
>>> support")
>>
>> The above shuffles the code around but does not introduce the problem
>> as far as I can see. Instead I think we should use:
>>
>> Fixes: dc36965a8905 ("arm64: dts: r8a7796: salvator-x: Enable EthernetAVB")
>> Fixes: 6fa501c549aa ("arm64: dts: r8a7795: enable EthernetAVB on Salvator-X")
>>
>>> Signed-off-by: Bogdan Mirea <bogdan-stefan_mi...@mentor.com>
>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
> 
> I have applied this a fix for v4.15 with the updated Fixes tags above.
> 

thank you, I was unsure if you expected to get changes which can be used
as a base without patch application conflicts (my done choice) or changes,
which originally intorduced the bug, but formally require slightly
different fixes due to code changes in the middle. If it would be needed,
hopefully linux-stable maintainers can resolve the trivial conflicts.

Also you may find this information from the cover letter useful,
someone may want to check the relevant board schematics and send
similar fixes (if applicable after schematics review and testing):

  Note that DTS files for V3M Starter Kit, Draak and Eagle boards
  contain the same property, the files are untouched due to unavailable
  schematics to verify if the fix applies to these boards as well.

--
With best wishes,
Vladimir

[PATCH v2 2/2] arm64: dts: renesas: ulcb: Remove renesas,no-ether-link property

[Vladimir Zapolskiy]

From: Bogdan Mirea <bogdan-stefan_mi...@mentor.com>

The present change is a bug fix for AVB link iteratively up/down.

Steps to reproduce:
- start AVB TX stream (Using aplay via MSE),
- disconnect+reconnect the eth cable,
- after a reconnection the eth connection goes iteratively up/down
  without user interaction,
- this may heal after some seconds or even stay for minutes.

As the documentation specifies, the "renesas,no-ether-link" option
should be used when a board does not provide a proper AVB_LINK signal.
There is no need for this option enabled on RCAR H3/M3 Salvator-X/XS
and ULCB starter kits since the AVB_LINK is correctly handled by HW.

Choosing to keep or remove the "renesas,no-ether-link" option will
have impact on the code flow in the following ways:
- keeping this option enabled may lead to unexpected behavior since
  the RX & TX are enabled/disabled directly from adjust_link function
  without any HW interrogation,
- removing this option, the RX & TX will only be enabled/disabled after
  HW interrogation. The HW check is made through the LMON pin in PSR
  register which specifies AVB_LINK signal value (0 - at low level;
  1 - at high level).

In conclusion, the present change is also a safety improvement because
it removes the "renesas,no-ether-link" option leading to a proper way
of detecting the link state based on HW interrogation and not on
software heuristic.

Fixes: 253ed045a34d ("arm64: dts: renesas: Extract common ULCB board support")
Signed-off-by: Bogdan Mirea <bogdan-stefan_mi...@mentor.com>
Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 arch/arm64/boot/dts/renesas/ulcb.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/ulcb.dtsi 
b/arch/arm64/boot/dts/renesas/ulcb.dtsi
index be91016e0b48..3e7a6b94e9f8 100644
--- a/arch/arm64/boot/dts/renesas/ulcb.dtsi
+++ b/arch/arm64/boot/dts/renesas/ulcb.dtsi
@@ -145,7 +145,6 @@
  {
pinctrl-0 = <_pins>;
pinctrl-names = "default";
-   renesas,no-ether-link;
phy-handle = <>;
status = "okay";
 
-- 
2.8.1

[PATCH v2 0/2] arm64: dts: renesas: Remove renesas,no-ether-link property

[Vladimir Zapolskiy]

The present change is a bug fix for AVB link iteratively up/down.

Steps to reproduce:
- start AVB TX stream (Using aplay via MSE),
- disconnect+reconnect the eth cable,
- after a reconnection the eth connection goes iteratively up/down
  without user interaction,
- this may heal after some seconds or even stay for minutes.

As the documentation specifies, the "renesas,no-ether-link" option
should be used when a board does not provide a proper AVB_LINK signal.
There is no need for this option enabled on RCAR H3/M3 Salvator-X/XS
and ULCB starter kits since the AVB_LINK is correctly handled by HW.

Choosing to keep or remove the "renesas,no-ether-link" option will
have impact on the code flow in the following ways:
- keeping this option enabled may lead to unexpected behavior since
  the RX & TX are enabled/disabled directly from adjust_link function
  without any HW interrogation,
- removing this option, the RX & TX will only be enabled/disabled after
  HW interrogation. The HW check is made through the LMON pin in PSR
  register which specifies AVB_LINK signal value (0 - at low level;
  1 - at high level).

In conclusion, the change is also a safety improvement because it
removes the "renesas,no-ether-link" option leading to a proper way
of detecting the link state based on HW interrogation and not on
software heuristic.

Note that DTS files for V3M Starter Kit, Draak and Eagle boards
contain the same property, the files are untouched due to unavailable
schematics to verify if the fix applies to these boards as well.

Changes from v1 to v2:
* per a request from Simon added Fixes tags to both changes,
  the specified commits generalize Salvator-X and ULCB dtsi files
  and the fixes should be applicable without a conflict, however
  similar but virtually different fixes can be spread into Salvator-X
  and ULCB board DTS files of older kernel revisions as well.

Bogdan Mirea (2):
  arm64: dts: renesas: salvator-x: Remove renesas,no-ether-link property
  arm64: dts: renesas: ulcb: Remove renesas,no-ether-link property

 arch/arm64/boot/dts/renesas/salvator-common.dtsi | 1 -
 arch/arm64/boot/dts/renesas/ulcb.dtsi| 1 -
 2 files changed, 2 deletions(-)

-- 
2.8.1

[PATCH v2 1/2] arm64: dts: renesas: salvator-x: Remove renesas,no-ether-link property

[Vladimir Zapolskiy]

From: Bogdan Mirea <bogdan-stefan_mi...@mentor.com>

The present change is a bug fix for AVB link iteratively up/down.

Steps to reproduce:
- start AVB TX stream (Using aplay via MSE),
- disconnect+reconnect the eth cable,
- after a reconnection the eth connection goes iteratively up/down
  without user interaction,
- this may heal after some seconds or even stay for minutes.

As the documentation specifies, the "renesas,no-ether-link" option
should be used when a board does not provide a proper AVB_LINK signal.
There is no need for this option enabled on RCAR H3/M3 Salvator-X/XS
and ULCB starter kits since the AVB_LINK is correctly handled by HW.

Choosing to keep or remove the "renesas,no-ether-link" option will
have impact on the code flow in the following ways:
- keeping this option enabled may lead to unexpected behavior since
  the RX & TX are enabled/disabled directly from adjust_link function
  without any HW interrogation,
- removing this option, the RX & TX will only be enabled/disabled after
  HW interrogation. The HW check is made through the LMON pin in PSR
  register which specifies AVB_LINK signal value (0 - at low level;
  1 - at high level).

In conclusion, the present change is also a safety improvement because
it removes the "renesas,no-ether-link" option leading to a proper way
of detecting the link state based on HW interrogation and not on
software heuristic.

Fixes: d25e8ff0d5aa ("arm64: dts: renesas: Extract common Salvator-X board 
support")
Signed-off-by: Bogdan Mirea <bogdan-stefan_mi...@mentor.com>
Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 arch/arm64/boot/dts/renesas/salvator-common.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/salvator-common.dtsi 
b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
index 4e800e933944..c3095bd575d7 100644
--- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
+++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
@@ -255,7 +255,6 @@
  {
pinctrl-0 = <_pins>;
pinctrl-names = "default";
-   renesas,no-ether-link;
phy-handle = <>;
status = "okay";
 
-- 
2.8.1

Re: [PATCH 0/2] arm64: dts: renesas: Remove renesas,no-ether-link property

[Vladimir Zapolskiy]

Hi Simon,

On 12/21/2017 01:28 PM, Simon Horman wrote:
> On Wed, Dec 20, 2017 at 03:22:10PM +0200, Vladimir Zapolskiy wrote:
>> The present change is a bug fix for AVB link iteratively up/down.
> 
> If this is a bug please consider including Fixes tags in the patches.
> 

would you prefer to get v2 with the Fixes tags added or short replies
to v1 with the requested information?

--
With best wishes,
Vladimir

[PATCH 2/2] arm64: dts: renesas: ulcb: Remove renesas,no-ether-link property

[Vladimir Zapolskiy]

From: Bogdan Mirea <bogdan-stefan_mi...@mentor.com>

The present change is a bug fix for AVB link iteratively up/down.

Steps to reproduce:
- start AVB TX stream (Using aplay via MSE),
- disconnect+reconnect the eth cable,
- after a reconnection the eth connection goes iteratively up/down
  without user interaction,
- this may heal after some seconds or even stay for minutes.

As the documentation specifies, the "renesas,no-ether-link" option
should be used when a board does not provide a proper AVB_LINK signal.
There is no need for this option enabled on RCAR H3/M3 Salvator-X/XS
and ULCB starter kits since the AVB_LINK is correctly handled by HW.

Choosing to keep or remove the "renesas,no-ether-link" option will
have impact on the code flow in the following ways:
- keeping this option enabled may lead to unexpected behavior since
  the RX & TX are enabled/disabled directly from adjust_link function
  without any HW interrogation,
- removing this option, the RX & TX will only be enabled/disabled after
  HW interrogation. The HW check is made through the LMON pin in PSR
  register which specifies AVB_LINK signal value (0 - at low level;
  1 - at high level).

In conclusion, the present change is also a safety improvement because
it removes the "renesas,no-ether-link" option leading to a proper way
of detecting the link state based on HW interrogation and not on
software heuristic.

Signed-off-by: Bogdan Mirea <bogdan-stefan_mi...@mentor.com>
Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 arch/arm64/boot/dts/renesas/ulcb.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/ulcb.dtsi 
b/arch/arm64/boot/dts/renesas/ulcb.dtsi
index be91016e0b48..3e7a6b94e9f8 100644
--- a/arch/arm64/boot/dts/renesas/ulcb.dtsi
+++ b/arch/arm64/boot/dts/renesas/ulcb.dtsi
@@ -145,7 +145,6 @@
  {
pinctrl-0 = <_pins>;
pinctrl-names = "default";
-   renesas,no-ether-link;
phy-handle = <>;
status = "okay";
 
-- 
2.8.1

[PATCH 0/2] arm64: dts: renesas: Remove renesas,no-ether-link property

[Vladimir Zapolskiy]

The present change is a bug fix for AVB link iteratively up/down.

Steps to reproduce:
- start AVB TX stream (Using aplay via MSE),
- disconnect+reconnect the eth cable,
- after a reconnection the eth connection goes iteratively up/down
  without user interaction,
- this may heal after some seconds or even stay for minutes.

As the documentation specifies, the "renesas,no-ether-link" option
should be used when a board does not provide a proper AVB_LINK signal.
There is no need for this option enabled on RCAR H3/M3 Salvator-X/XS
and ULCB starter kits since the AVB_LINK is correctly handled by HW.

Choosing to keep or remove the "renesas,no-ether-link" option will
have impact on the code flow in the following ways:
- keeping this option enabled may lead to unexpected behavior since
  the RX & TX are enabled/disabled directly from adjust_link function
  without any HW interrogation,
- removing this option, the RX & TX will only be enabled/disabled after
  HW interrogation. The HW check is made through the LMON pin in PSR
  register which specifies AVB_LINK signal value (0 - at low level;
  1 - at high level).

In conclusion, the change is also a safety improvement because it
removes the "renesas,no-ether-link" option leading to a proper way
of detecting the link state based on HW interrogation and not on
software heuristic.

Note that DTS files for V3M Starter Kit, Draak and Eagle boards
contain the same property, the files are untouched due to unavailable
schematics to verify if the fix applies to these boards as well.

Bogdan Mirea (2):
  arm64: dts: renesas: salvator-x: Remove renesas,no-ether-link property
  arm64: dts: renesas: ulcb: Remove renesas,no-ether-link property

 arch/arm64/boot/dts/renesas/salvator-common.dtsi | 1 -
 arch/arm64/boot/dts/renesas/ulcb.dtsi| 1 -
 2 files changed, 2 deletions(-)

-- 
2.8.1

[PATCH 1/2] arm64: dts: renesas: salvator-x: Remove renesas,no-ether-link property

[Vladimir Zapolskiy]

From: Bogdan Mirea <bogdan-stefan_mi...@mentor.com>

The present change is a bug fix for AVB link iteratively up/down.

Steps to reproduce:
- start AVB TX stream (Using aplay via MSE),
- disconnect+reconnect the eth cable,
- after a reconnection the eth connection goes iteratively up/down
  without user interaction,
- this may heal after some seconds or even stay for minutes.

As the documentation specifies, the "renesas,no-ether-link" option
should be used when a board does not provide a proper AVB_LINK signal.
There is no need for this option enabled on RCAR H3/M3 Salvator-X/XS
and ULCB starter kits since the AVB_LINK is correctly handled by HW.

Choosing to keep or remove the "renesas,no-ether-link" option will
have impact on the code flow in the following ways:
- keeping this option enabled may lead to unexpected behavior since
  the RX & TX are enabled/disabled directly from adjust_link function
  without any HW interrogation,
- removing this option, the RX & TX will only be enabled/disabled after
  HW interrogation. The HW check is made through the LMON pin in PSR
  register which specifies AVB_LINK signal value (0 - at low level;
  1 - at high level).

In conclusion, the present change is also a safety improvement because
it removes the "renesas,no-ether-link" option leading to a proper way
of detecting the link state based on HW interrogation and not on
software heuristic.

Signed-off-by: Bogdan Mirea <bogdan-stefan_mi...@mentor.com>
Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
---
 arch/arm64/boot/dts/renesas/salvator-common.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/salvator-common.dtsi 
b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
index 4e800e933944..c3095bd575d7 100644
--- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
+++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
@@ -255,7 +255,6 @@
  {
pinctrl-0 = <_pins>;
pinctrl-names = "default";
-   renesas,no-ether-link;
phy-handle = <>;
status = "okay";
 
-- 
2.8.1

Re: [PATCH 1/8] spi: sh-msiof: Add sleep before master transfer for test

[Vladimir Zapolskiy]

Hi Dirk,

On 09/06/2017 10:05 AM, Dirk Behme wrote:
> From: Hiromitsu Yamasaki 
> 
> This patch is for debug of transfer between master and slave.
> Since the slave needs to complete a preparation in data transfer
> before the master working, the sleep wait is put before
> the data transfer of the master.
> 
> Signed-off-by: Hiromitsu Yamasaki 
> Signed-off-by: Dirk Behme 
> ---
>  drivers/spi/Kconfig| 20 
>  drivers/spi/spi-sh-msiof.c | 15 +++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index a75f2a2cf780..0139ecf8f42e 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -600,6 +600,26 @@ config SPI_SH_MSIOF
>   help
> SPI driver for SuperH and SH Mobile MSIOF blocks.
>  
> +config SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG
> + bool "Transfer Synchronization Debug support for MSIOF"
> + depends on SPI_SH_MSIOF
> + default n

Drop 'default n', it is the default per se.

> + help
> +   In data transfer, the slave needs to have completed
> +   a transfer preparation before the master.
> +   As a test environment, it was to be able to put a sleep wait
> +   before the data transfer of the master.
> +
> +config SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG_MSLEEP
> + int "Master of sleep latency (msec time)"

Master of sleep latency? Probably reformulation is wanted.

> + default 1

In addition please define and add a valid 'range' option.

> + depends on SPI_SH_MSIOF && SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG

Dependence on SPI_SH_MSIOF is inherited.

> + help
> +   Select Sleep latency of the previous data transfer
> +   at the time of master mode.
> +   Examples:
> + N => N msec
> +
>  config SPI_SH
>   tristate "SuperH SPI controller"
>   depends on SUPERH || COMPILE_TEST
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 0eb1e9583485..2b4d3a520176 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -41,6 +41,10 @@ struct sh_msiof_chipdata {
>   u16 min_div;
>  };
>  
> +#ifdef CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG
> +#define TRANSFAR_SYNC_DELAY (CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG_MSLEEP)

typo, s/TRANSFAR/TRANSFER/

Parenthesis around CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG_MSLEEP
are not needed.

> +#endif /* CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG */
> +
>  struct sh_msiof_spi_priv {
>   struct spi_master *master;
>   void __iomem *mapbase;
> @@ -910,6 +914,11 @@ static int sh_msiof_transfer_one(struct spi_master 
> *master,
>   if (tx_buf)
>   copy32(p->tx_dma_page, tx_buf, l / 4);
>  
> +#ifdef CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG
> + if (p->mode == SPI_MSIOF_MASTER)
> + msleep(TRANSFAR_SYNC_DELAY);
> +#endif /* CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG */

If SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG is unset, you may set TRANSFAR_SYNC_DELAY
to 0 and get rid of #ifdefs in the code.

if (p->mode == SPI_MSIOF_MASTER && TRANSFAR_SYNC_DELAY)
msleep(TRANSFAR_SYNC_DELAY);

> +
>   ret = sh_msiof_dma_once(p, tx_buf, rx_buf, l);
>   if (ret == -EAGAIN) {
>   pr_warn_once("%s %s: DMA not available, falling back to 
> PIO\n",
> @@ -983,6 +992,12 @@ static int sh_msiof_transfer_one(struct spi_master 
> *master,
>   words = len / bytes_per_word;
>  
>   while (words > 0) {
> +
> +#ifdef CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG
> + if (p->mode == SPI_MSIOF_MASTER)
> + msleep(TRANSFAR_SYNC_DELAY);
> +#endif /* CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG */
> +
>   n = sh_msiof_spi_txrx_once(p, tx_fifo, rx_fifo, tx_buf, rx_buf,
>  words, bits);
>   if (n < 0)
> 

In general I don't think it makes any sense to incluide this change.

--
With best wishes,
Vladimir

Re: [PATCH v2] watchdog: softdog: make pretimeout support a compile option

[Vladimir Zapolskiy]

Hi Wolfram,

On 02/07/2017 04:03 PM, Wolfram Sang wrote:
> It occurred to me that the panic pretimeout governor will stall the
> softdog, because it is purely software which simply breaks when the
> kernel panics. Testing governors with the softdog on the other hand is
> really useful, so make this feature a compile time option which nees to
> be enabled explicitly. This also removes the overhead if pretimeout
> support is not used because it will now be compiled away (saving ~10% on
> ARM32).
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

the changes looks perfect,

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

--
With best wishes,
Vladimir

Re: [PATCH 01/10] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hi Ulrich,

On 11/11/2016 07:07 PM, Ulrich Hecht wrote:

From: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

The change adds support of internal HDMI I2C master controller, this
subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

The current implementation does not support "I2C Master Interface
Extended Read Mode" to read data addressed by non-zero segment
pointer, this means that if EDID has more than 1 extension blocks,
EDID reading operation won't succeed, in my practice all tested HDMI
monitors have at maximum one extension block.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
Acked-by: Rob Herring <r...@kernel.org>


many thanks to Philipp for pushing the change, as for today, it is in drm-next:

  https://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-next=90233ee5d1

--
With best wishes,
Vladimir

Re: [PATCH v2] watchdog: softdog: implement pretimeout support

[Vladimir Zapolskiy]

Hi Wolfram,

On 08/30/2016 06:03 PM, Wolfram Sang wrote:

From: Wolfram Sang 

Give devices which do not have hardware support for pretimeout at least a
software version of it.

Signed-off-by: Wolfram Sang 
---

Change since v1: added 'else' block in softdog_ping(), to actually
disable the timer when pretimeout value got cleared.

Depends on the V4 pretimeout series from Vladimir.

Vladimir: Maybe you could add this patch to the next revision of your
pretimeout series? Then people have something to play around with
right away.


I'll add it to the series, and most probably I'll add a pretimeout
support to the iMX watchdog driver.

The v5 series will be published today tonight or tomorrow.

--
Best wishes,
Vladimir

Re: [PATCH 1/7] watchdog: add watchdog pretimeout framework

[Vladimir Zapolskiy]

Hi Wolfram,

On 05.06.2016 12:48, Wolfram Sang wrote:
> On Thu, May 26, 2016 at 03:37:17PM -0700, Guenter Roeck wrote:
>> On Thu, May 26, 2016 at 06:41:36PM +0200, Wolfram Sang wrote:
>> [ ... ]
>>>
>>> No doubt about that. I had some ideas and thought it is easier to talk
>>> over code. If you want to rebase it, too, I'd be happy to check what you
>>> came up with to solve the problems. I might still argue that I prefer
>>> the less-code approach, but it will be Guenter's / Wim's decision, of
>>> course.
>>>
>> I have a large back-log of patches to review. Simple patches with less code
>> will get preferential treating. The more complex, the higher the likelyhood
>> that the patches get pushed to the end of the queue.
>>
>> Giving a quick glance, I liked Wolfram's patches because they seemed to
>> be simple and straightforward. I hope the next version won't add too much
>> additional complexity.
> 
> Vladimir, did you have time to look into this?

yes, I managed to rebase and split my change to smaller pieces to encourage
reviewers.

My plan is to do thorough testing of the change and send it for review
tomorrow or on Tuesday, please participate in review.

> I can offer to resend this series on-top of v4.7-rc1 with the core patch
> re-attributed to Vladimir. Then we can review the basic stuff now, and
> can discuss/deal with the bottom half handling incrementally.
> 
> Sounds good?
> 

--
Best wishes,
Vladimir

Re: [PATCH 1/7] watchdog: add watchdog pretimeout framework

[Vladimir Zapolskiy]

Hi Wolfram,

On 25.05.2016 16:32, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> The change adds a simple watchdog pretimeout framework infrastructure,
> its purpose is to allow users to select a desired handling of watchdog
> pretimeout events, which may be generated by a watchdog driver.
> 
> By design every watchdog pretimeout governor may be compiled as a
> kernel module, a user selects a default watchdog pretimeout
> governor during compilation stage and can select another governor in
> runtime.
> 
> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
> attributes in sysfs: read/write pretimeout_governor attribute and read
> only pretimeout_available_governors attribute.
> 
> Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in
> sysfs.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
> ---
> 
> Changes since Vladimir's last version:
> 
> * rebased
>   adapt to the internal data reorganization, especially the now private
>   struct device *dev
> * dropped can_sleep support!
>   The additional lock, list, and workqueue made the code quite complex. The
>   only user was the userspace governor which can be reworked to let the
>   watchdog device code do the bottom half. In addition, I am not fully
>   convinced sending a uevent is the proper thing to do, but this needs to
>   be discussed in another thread. Removing this support makes the code much
>   easier to follow (locking!), saves 30% of LoC + a list + a workqueue.

The thing is that I'm particularly interested in

1) sleeping governors,
2) userspace notification of any appropriate kind, but preferably not by
   adding a clumsy .poll callback, uevent is the best IMHO.

The userspace sleeping governor is the only one proposed for a mainline,
however the whole idea of having a framework is to allow users to write
their own private governors, and that's exactly what we need and use.

So the original complexity has its state-of-the-art grounds, and for
sake of getting a solid picture for reviewers and users it is better to
introduce sleeping functionality right from the beginning. I know it
is quite complex, probably it might be better to add it to the series
as a separate patch?

> * moved pretimeout registration from watchdog_core to watchdog_dev
>   Let's handle it exactly where the device is created, so we have access to
>   the now private device pointer for adding the sysfs files.
> * don't export watchdog_(un)register_pretimeout since they are linked to the
>   core anyhow
> * whitespace cleanups
> 

Thanks for pushing it, but do you think that the authorship of the
code can be preserved?

Feel free to ask me to rebase the change and so on, patch review procedure
is well established and I'm pretty sure I can cope with it.

I believe the main problem with the original code since the time when
rebase was not required is that it didn't receive any formal technical
review from Guenter or Wim, but I'm glad to know that someone else is
interested in it.

Merge window is closing, so it's good time for me to rebase the change
and resend it, do you have any objections?

>  drivers/watchdog/Kconfig   |   8 +
>  drivers/watchdog/Makefile  |   6 +-
>  drivers/watchdog/watchdog_dev.c|   8 +
>  drivers/watchdog/watchdog_pretimeout.c | 269 
> +
>  drivers/watchdog/watchdog_pretimeout.h |  35 +
>  include/linux/watchdog.h   |  10 ++
>  6 files changed, 334 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/watchdog/watchdog_pretimeout.c
>  create mode 100644 drivers/watchdog/watchdog_pretimeout.h
> 

--
With best wishes,
Vladimir