This patch series adds error handling for the PHY read/write path and optional
register access validation.
After adding regmap_ranges for KSZ8563 some bugs was detected, so
critical bug fixes are sorted before ragmap_range patch.
Potentially this bug fixes can be ported to stable kernels, but need to be
reworked.
Oleksij Rempel (10):
net: dsa: microchip: don't announce extended register support on non
Gbit chips
net: dsa: microchip: allow to pass return values for PHY read/write
accesses
net: dsa: microchip: forward error value on all ksz_pread/ksz_pwrite
functions
net: dsa: microchip: ksz9477: add error handling to ksz9477_r/w_phy
net: dsa: microchip: ksz8795: add error handling to ksz8_r/w_phy
net: dsa: microchip: KSZ9893: do not write to not supported Output
Clock Control Register
net: dsa: microchip: warn about not supported synclko properties on
KSZ9893 chips
net: dsa: microchip: add support for regmap_access_tables
net: dsa: microchip: add regmap_range for KSZ8563 chip
net: dsa: microchip: ksz9477: remove MII_CTRL1000 check from
ksz9477_w_phy()
drivers/net/dsa/microchip/ksz8.h | 4 +-
drivers/net/dsa/microchip/ksz8795.c | 111 +++++++++++++----
drivers/net/dsa/microchip/ksz9477.c | 41 +++++--
drivers/net/dsa/microchip/ksz9477.h | 4 +-
drivers/net/dsa/microchip/ksz_common.c | 148 ++++++++++++++++++++++-
drivers/net/dsa/microchip/ksz_common.h | 76 +++++++++---
drivers/net/dsa/microchip/ksz_spi.c | 3 +
drivers/net/dsa/microchip/lan937x.h | 4 +-
drivers/net/dsa/microchip/lan937x_main.c | 8 +-
9 files changed, 337 insertions(+), 62 deletions(-)
--
2.30.2
The reason why PHYlib may access MII_CTRL1000 on the chip without GBit
support is only if chip provides wrong information about extended caps
register. This issue is now handled by ksz9477_r_phy_quirks()
With proper regmap_ranges provided for all chips we will be able to catch this
kind of bugs any way. So, remove this sanity check.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/dsa/microchip/ksz9477.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index a3ddc9021af3..caef13ae9070 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -346,10 +346,6 @@ int ksz9477_w_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 val)
if (addr >= dev->phy_port_cnt)
return 0;
- /* No gigabit support. Do not write to this register. */
- if (!(dev->features & GBIT_SUPPORT) && reg == MII_CTRL1000)
- return -ENOTSUPP;
-
return ksz_pwrite16(dev, addr, 0x100 + (reg << 1), val);
}
--
2.30.2
Now ksz_pread/ksz_pwrite can return error value. So, make use of it.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/dsa/microchip/ksz9477.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 9d749537cf01..074d4c16d427 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -283,6 +283,7 @@ static void ksz9477_r_phy_quirks(struct ksz_device *dev, u16 addr, u16 reg,
int ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
{
u16 val = 0xffff;
+ int ret;
/* No real PHY after this. Simulate the PHY.
* A fixed PHY can be setup in the device tree, but this function is
@@ -323,7 +324,10 @@ int ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
break;
}
} else {
- ksz_pread16(dev, addr, 0x100 + (reg << 1), &val);
+ ret = ksz_pread16(dev, addr, 0x100 + (reg << 1), &val);
+ if (ret)
+ return ret;
+
ksz9477_r_phy_quirks(dev, addr, reg, &val);
}
@@ -340,11 +344,9 @@ int ksz9477_w_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 val)
/* No gigabit support. Do not write to this register. */
if (!(dev->features & GBIT_SUPPORT) && reg == MII_CTRL1000)
- return 0;
+ return -ENOTSUPP;
- ksz_pwrite16(dev, addr, 0x100 + (reg << 1), val);
-
- return 0;
+ return ksz_pwrite16(dev, addr, 0x100 + (reg << 1), val);
}
void ksz9477_cfg_port_member(struct ksz_device *dev, int port, u8 member)
--
2.30.2
This is complex driver with support for different chips with different
layouts. To detect at least some bugs earlier, we should validate register
accesses by using regmap_access_table support.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/dsa/microchip/ksz_common.h | 46 +++++++++++++++++++++++---
drivers/net/dsa/microchip/ksz_spi.c | 3 ++
2 files changed, 45 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 3a64a444fa26..d460ae6ea82d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -61,6 +61,8 @@ struct ksz_chip_data {
bool supports_rmii[KSZ_MAX_NUM_PORTS];
bool supports_rgmii[KSZ_MAX_NUM_PORTS];
bool internal_phy[KSZ_MAX_NUM_PORTS];
+ const struct regmap_access_table *wr_table;
+ const struct regmap_access_table *rd_table;
};
struct ksz_port {
@@ -329,6 +331,10 @@ static inline int ksz_read8(struct ksz_device *dev, u32 reg, u8 *val)
unsigned int value;
int ret = regmap_read(dev->regmap[0], reg, &value);
+ if (ret)
+ dev_err(dev->dev, "can't read 8bit reg: 0x%x %pe \n", reg,
+ ERR_PTR(ret));
+
*val = value;
return ret;
}
@@ -338,6 +344,10 @@ static inline int ksz_read16(struct ksz_device *dev, u32 reg, u16 *val)
unsigned int value;
int ret = regmap_read(dev->regmap[1], reg, &value);
+ if (ret)
+ dev_err(dev->dev, "can't read 16bit reg: 0x%x %pe \n", reg,
+ ERR_PTR(ret));
+
*val = value;
return ret;
}
@@ -347,6 +357,10 @@ static inline int ksz_read32(struct ksz_device *dev, u32 reg, u32 *val)
unsigned int value;
int ret = regmap_read(dev->regmap[2], reg, &value);
+ if (ret)
+ dev_err(dev->dev, "can't read 32bit reg: 0x%x %pe \n", reg,
+ ERR_PTR(ret));
+
*val = value;
return ret;
}
@@ -357,7 +371,10 @@ static inline int ksz_read64(struct ksz_device *dev, u32 reg, u64 *val)
int ret;
ret = regmap_bulk_read(dev->regmap[2], reg, value, 2);
- if (!ret)
+ if (ret)
+ dev_err(dev->dev, "can't read 64bit reg: 0x%x %pe \n", reg,
+ ERR_PTR(ret));
+ else
*val = (u64)value[0] << 32 | value[1];
return ret;
@@ -365,17 +382,38 @@ static inline int ksz_read64(struct ksz_device *dev, u32 reg, u64 *val)
static inline int ksz_write8(struct ksz_device *dev, u32 reg, u8 value)
{
- return regmap_write(dev->regmap[0], reg, value);
+ int ret;
+
+ ret = regmap_write(dev->regmap[0], reg, value);
+ if (ret)
+ dev_err(dev->dev, "can't write 8bit reg: 0x%x %pe \n", reg,
+ ERR_PTR(ret));
+
+ return ret;
}
static inline int ksz_write16(struct ksz_device *dev, u32 reg, u16 value)
{
- return regmap_write(dev->regmap[1], reg, value);
+ int ret;
+
+ ret = regmap_write(dev->regmap[1], reg, value);
+ if (ret)
+ dev_err(dev->dev, "can't write 16bit reg: 0x%x %pe \n", reg,
+ ERR_PTR(ret));
+
+ return ret;
}
static inline int ksz_write32(struct ksz_device *dev, u32 reg, u32 value)
{
- return regmap_write(dev->regmap[2], reg, value);
+ int ret;
+
+ ret = regmap_write(dev->regmap[2], reg, value);
+ if (ret)
+ dev_err(dev->dev, "can't write 32bit reg: 0x%x %pe \n", reg,
+ ERR_PTR(ret));
+
+ return ret;
}
static inline int ksz_write64(struct ksz_device *dev, u32 reg, u64 value)
diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c
index 05bd089795f8..a377454450b3 100644
--- a/drivers/net/dsa/microchip/ksz_spi.c
+++ b/drivers/net/dsa/microchip/ksz_spi.c
@@ -66,7 +66,10 @@ static int ksz_spi_probe(struct spi_device *spi)
for (i = 0; i < ARRAY_SIZE(ksz8795_regmap_config); i++) {
rc = regmap_config[i];
rc.lock_arg = &dev->regmap_mutex;
+ rc.wr_table = chip->wr_table;
+ rc.rd_table = chip->rd_table;
dev->regmap[i] = devm_regmap_init_spi(spi, &rc);
+
if (IS_ERR(dev->regmap[i])) {
ret = PTR_ERR(dev->regmap[i]);
dev_err(&spi->dev,
--
2.30.2
Now ksz_pread/ksz_pwrite can return error value. So, make use of it.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/dsa/microchip/ksz8795.c | 102 ++++++++++++++++++++++------
1 file changed, 81 insertions(+), 21 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 4b39666cfc9c..1aaf765611e8 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -560,14 +560,24 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
u8 val1, val2;
u16 data = 0;
u8 p = phy;
+ int ret;
regs = dev->info->regs;
switch (reg) {
case MII_BMCR:
- ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart);
- ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed);
- ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
+ ret = ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart);
+ if (ret)
+ return ret;
+
+ ret = ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed);
+ if (ret)
+ return ret;
+
+ ret = ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
+ if (ret)
+ return ret;
+
if (restart & PORT_PHY_LOOPBACK)
data |= BMCR_LOOPBACK;
if (ctrl & PORT_FORCE_100_MBIT)
@@ -597,7 +607,10 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
data |= KSZ886X_BMCR_DISABLE_LED;
break;
case MII_BMSR:
- ksz_pread8(dev, p, regs[P_LINK_STATUS], &link);
+ ret = ksz_pread8(dev, p, regs[P_LINK_STATUS], &link);
+ if (ret)
+ return ret;
+
data = BMSR_100FULL |
BMSR_100HALF |
BMSR_10FULL |
@@ -618,7 +631,10 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
data = KSZ8795_ID_LO;
break;
case MII_ADVERTISE:
- ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
+ ret = ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
+ if (ret)
+ return ret;
+
data = ADVERTISE_CSMA;
if (ctrl & PORT_AUTO_NEG_SYM_PAUSE)
data |= ADVERTISE_PAUSE_CAP;
@@ -632,7 +648,10 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
data |= ADVERTISE_10HALF;
break;
case MII_LPA:
- ksz_pread8(dev, p, regs[P_REMOTE_STATUS], &link);
+ ret = ksz_pread8(dev, p, regs[P_REMOTE_STATUS], &link);
+ if (ret)
+ return ret;
+
data = LPA_SLCT;
if (link & PORT_REMOTE_SYM_PAUSE)
data |= LPA_PAUSE_CAP;
@@ -648,8 +667,14 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
data |= LPA_LPACK;
break;
case PHY_REG_LINK_MD:
- ksz_pread8(dev, p, REG_PORT_LINK_MD_CTRL, &val1);
- ksz_pread8(dev, p, REG_PORT_LINK_MD_RESULT, &val2);
+ ret = ksz_pread8(dev, p, REG_PORT_LINK_MD_CTRL, &val1);
+ if (ret)
+ return ret;
+
+ ret = ksz_pread8(dev, p, REG_PORT_LINK_MD_RESULT, &val2);
+ if (ret)
+ return ret;
+
if (val1 & PORT_START_CABLE_DIAG)
data |= PHY_START_CABLE_DIAG;
@@ -664,7 +689,10 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
FIELD_GET(PORT_CABLE_FAULT_COUNTER_L, val2));
break;
case PHY_REG_PHY_CTRL:
- ksz_pread8(dev, p, regs[P_LINK_STATUS], &link);
+ ret = ksz_pread8(dev, p, regs[P_LINK_STATUS], &link);
+ if (ret)
+ return ret;
+
if (link & PORT_MDIX_STATUS)
data |= KSZ886X_CTRL_MDIX_STAT;
break;
@@ -683,6 +711,7 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
u8 restart, speed, ctrl, data;
const u16 *regs;
u8 p = phy;
+ int ret;
regs = dev->info->regs;
@@ -692,15 +721,26 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
/* Do not support PHY reset function. */
if (val & BMCR_RESET)
break;
- ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed);
+ ret = ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed);
+ if (ret)
+ return ret;
+
data = speed;
if (val & KSZ886X_BMCR_HP_MDIX)
data |= PORT_HP_MDIX;
else
data &= ~PORT_HP_MDIX;
- if (data != speed)
- ksz_pwrite8(dev, p, regs[P_SPEED_STATUS], data);
- ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
+
+ if (data != speed) {
+ ret = ksz_pwrite8(dev, p, regs[P_SPEED_STATUS], data);
+ if (ret)
+ return ret;
+ }
+
+ ret = ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
+ if (ret)
+ return ret;
+
data = ctrl;
if (ksz_is_ksz88x3(dev)) {
if ((val & BMCR_ANENABLE))
@@ -726,9 +766,17 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
data |= PORT_FORCE_FULL_DUPLEX;
else
data &= ~PORT_FORCE_FULL_DUPLEX;
- if (data != ctrl)
- ksz_pwrite8(dev, p, regs[P_FORCE_CTRL], data);
- ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart);
+
+ if (data != ctrl) {
+ ret = ksz_pwrite8(dev, p, regs[P_FORCE_CTRL], data);
+ if (ret)
+ return ret;
+ }
+
+ ret = ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart);
+ if (ret)
+ return ret;
+
data = restart;
if (val & KSZ886X_BMCR_DISABLE_LED)
data |= PORT_LED_OFF;
@@ -758,11 +806,19 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
data |= PORT_PHY_LOOPBACK;
else
data &= ~PORT_PHY_LOOPBACK;
- if (data != restart)
- ksz_pwrite8(dev, p, regs[P_NEG_RESTART_CTRL], data);
+
+ if (data != restart) {
+ ret = ksz_pwrite8(dev, p, regs[P_NEG_RESTART_CTRL],
+ data);
+ if (ret)
+ return ret;
+ }
break;
case MII_ADVERTISE:
- ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
+ ret = ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
+ if (ret)
+ return ret;
+
data = ctrl;
data &= ~(PORT_AUTO_NEG_SYM_PAUSE |
PORT_AUTO_NEG_100BTX_FD |
@@ -779,8 +835,12 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
data |= PORT_AUTO_NEG_10BT_FD;
if (val & ADVERTISE_10HALF)
data |= PORT_AUTO_NEG_10BT;
- if (data != ctrl)
- ksz_pwrite8(dev, p, regs[P_LOCAL_CTRL], data);
+
+ if (data != ctrl) {
+ ret = ksz_pwrite8(dev, p, regs[P_LOCAL_CTRL], data);
+ if (ret)
+ return ret;
+ }
break;
case PHY_REG_LINK_MD:
if (val & PHY_START_CABLE_DIAG)
--
2.30.2
KSZ9893 family of chips do not support synclko property. So warn about
without preventing driver from start.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/dsa/microchip/ksz_common.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 71b5349d006a..d3a9836c706f 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1916,6 +1916,13 @@ int ksz_switch_register(struct ksz_device *dev)
dev_err(dev->dev, "inconsistent synclko settings\n");
return -EINVAL;
}
+
+ if (dev->chip_id == KSZ9893_CHIP_ID && (dev->synclko_125 ||
+ dev->synclko_disable)) {
+ dev_warn(dev->dev, "microchip,synclko-125 and microchip,synclko-disable "
+ "properties are not supported on this chip. "
+ "Please fix you devicetree.\n");
+ }
}
ret = dsa_register_switch(dev->ds);
--
2.30.2
On Fri, Jul 29, 2022 at 03:03:40PM +0200, Oleksij Rempel wrote:
> } else {
> - ksz_pread16(dev, addr, 0x100 + (reg << 1), &val);
> + ret = ksz_pread16(dev, addr, 0x100 + (reg << 1), &val);
> + if (ret)
> + return ret;
> +
> ksz9477_r_phy_quirks(dev, addr, reg, &val);
> }
>
> @@ -340,11 +344,9 @@ int ksz9477_w_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 val)
>
> /* No gigabit support. Do not write to this register. */
> if (!(dev->features & GBIT_SUPPORT) && reg == MII_CTRL1000)
> - return 0;
> + return -ENOTSUPP;
I wonder if ENOTSUPP is the most appropriate error code, given that I
see it defined under a comment "Defined for the NFSv3 protocol".
How about -ENXIO?
>
> - ksz_pwrite16(dev, addr, 0x100 + (reg << 1), val);
> -
> - return 0;
> + return ksz_pwrite16(dev, addr, 0x100 + (reg << 1), val);
> }
>
> void ksz9477_cfg_port_member(struct ksz_device *dev, int port, u8 member)
> --
> 2.30.2
>
On Fri, Jul 29, 2022 at 03:03:43PM +0200, Oleksij Rempel wrote:
> KSZ9893 family of chips do not support synclko property. So warn about
> without preventing driver from start.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> drivers/net/dsa/microchip/ksz_common.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 71b5349d006a..d3a9836c706f 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1916,6 +1916,13 @@ int ksz_switch_register(struct ksz_device *dev)
> dev_err(dev->dev, "inconsistent synclko settings\n");
> return -EINVAL;
> }
> +
> + if (dev->chip_id == KSZ9893_CHIP_ID && (dev->synclko_125 ||
> + dev->synclko_disable)) {
> + dev_warn(dev->dev, "microchip,synclko-125 and microchip,synclko-disable "
> + "properties are not supported on this chip. "
> + "Please fix you devicetree.\n");
s/you/your/
Does KSZ8 have a REFCLK output of any sort? If it doesn't, then
"microchip,synclko-disable" is kind of supported, right?
I wonder what there is to gain by saying that you should remove some
device tree properties from non-ksz9477. After all, anyone can add any
random properties to a KSZ8 switch OF node and you won't warn about
those.
> + }
> }
>
> ret = dsa_register_switch(dev->ds);
> --
> 2.30.2
>
On Fri, Jul 29, 2022 at 03:03:41PM +0200, Oleksij Rempel wrote:
> Now ksz_pread/ksz_pwrite can return error value. So, make use of it.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Tue, Aug 02, 2022 at 02:36:33PM +0300, Vladimir Oltean wrote:
> On Fri, Jul 29, 2022 at 03:03:43PM +0200, Oleksij Rempel wrote:
> > KSZ9893 family of chips do not support synclko property. So warn about
> > without preventing driver from start.
> >
> > Signed-off-by: Oleksij Rempel <[email protected]>
> > ---
> > drivers/net/dsa/microchip/ksz_common.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> > index 71b5349d006a..d3a9836c706f 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -1916,6 +1916,13 @@ int ksz_switch_register(struct ksz_device *dev)
> > dev_err(dev->dev, "inconsistent synclko settings\n");
> > return -EINVAL;
> > }
> > +
> > + if (dev->chip_id == KSZ9893_CHIP_ID && (dev->synclko_125 ||
> > + dev->synclko_disable)) {
> > + dev_warn(dev->dev, "microchip,synclko-125 and microchip,synclko-disable "
> > + "properties are not supported on this chip. "
> > + "Please fix you devicetree.\n");
>
> s/you/your/
>
> Does KSZ8 have a REFCLK output of any sort? If it doesn't, then
> "microchip,synclko-disable" is kind of supported, right?
>
> I wonder what there is to gain by saying that you should remove some
> device tree properties from non-ksz9477. After all, anyone can add any
> random properties to a KSZ8 switch OF node and you won't warn about
> those.
Hm, if we will have any random not support OF property in the switch
node. We won't be able to warn about it anyway. So, if it is present
but not supported, we will just ignore it.
I'll drop this patch.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote:
> Hm, if we will have any random not support OF property in the switch
> node. We won't be able to warn about it anyway. So, if it is present
> but not supported, we will just ignore it.
>
> I'll drop this patch.
To continue, I think the right way to go about this is to edit the
dt-schema to say that these properties are only applicable to certain
compatible strings, rather than for all. Then due to the
"unevaluatedProperties: false", you'd get the warnings you want, at
validation time.
On Fri, Aug 05, 2022 at 04:42:34PM +0300, Vladimir Oltean wrote:
> On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote:
> > Hm, if we will have any random not support OF property in the switch
> > node. We won't be able to warn about it anyway. So, if it is present
> > but not supported, we will just ignore it.
> >
> > I'll drop this patch.
>
> To continue, I think the right way to go about this is to edit the
> dt-schema to say that these properties are only applicable to certain
> compatible strings, rather than for all. Then due to the
> "unevaluatedProperties: false", you'd get the warnings you want, at
> validation time.
Hm, with "unevaluatedProperties: false" i have no warnings. Even if I
create examples with random strings as properties. Are there some new
json libraries i should use?
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Sat, Aug 13, 2022 at 04:32:15PM +0200, Oleksij Rempel wrote:
> On Fri, Aug 05, 2022 at 04:42:34PM +0300, Vladimir Oltean wrote:
> > On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote:
> > > Hm, if we will have any random not support OF property in the switch
> > > node. We won't be able to warn about it anyway. So, if it is present
> > > but not supported, we will just ignore it.
> > >
> > > I'll drop this patch.
> >
> > To continue, I think the right way to go about this is to edit the
> > dt-schema to say that these properties are only applicable to certain
> > compatible strings, rather than for all. Then due to the
> > "unevaluatedProperties: false", you'd get the warnings you want, at
> > validation time.
>
> Hm, with "unevaluatedProperties: false" i have no warnings. Even if I
> create examples with random strings as properties. Are there some new
> json libraries i should use?
Try
additionalProperties: False
Andrew
On Sat, Aug 13, 2022 at 05:11:45PM +0200, Andrew Lunn wrote:
> On Sat, Aug 13, 2022 at 04:32:15PM +0200, Oleksij Rempel wrote:
> > On Fri, Aug 05, 2022 at 04:42:34PM +0300, Vladimir Oltean wrote:
> > > On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote:
> > > > Hm, if we will have any random not support OF property in the switch
> > > > node. We won't be able to warn about it anyway. So, if it is present
> > > > but not supported, we will just ignore it.
> > > >
> > > > I'll drop this patch.
> > >
> > > To continue, I think the right way to go about this is to edit the
> > > dt-schema to say that these properties are only applicable to certain
> > > compatible strings, rather than for all. Then due to the
> > > "unevaluatedProperties: false", you'd get the warnings you want, at
> > > validation time.
> >
> > Hm, with "unevaluatedProperties: false" i have no warnings. Even if I
> > create examples with random strings as properties. Are there some new
> > json libraries i should use?
>
> Try
>
> additionalProperties: False
Yes, it works. But in this case I'll do more changes. Just wont to make
sure I do not fix not broken things.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Sat, Aug 13, 2022 at 06:18:50PM +0200, Oleksij Rempel wrote:
> On Sat, Aug 13, 2022 at 05:11:45PM +0200, Andrew Lunn wrote:
> > On Sat, Aug 13, 2022 at 04:32:15PM +0200, Oleksij Rempel wrote:
> > > On Fri, Aug 05, 2022 at 04:42:34PM +0300, Vladimir Oltean wrote:
> > > > On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote:
> > > > > Hm, if we will have any random not support OF property in the switch
> > > > > node. We won't be able to warn about it anyway. So, if it is present
> > > > > but not supported, we will just ignore it.
> > > > >
> > > > > I'll drop this patch.
> > > >
> > > > To continue, I think the right way to go about this is to edit the
> > > > dt-schema to say that these properties are only applicable to certain
> > > > compatible strings, rather than for all. Then due to the
> > > > "unevaluatedProperties: false", you'd get the warnings you want, at
> > > > validation time.
> > >
> > > Hm, with "unevaluatedProperties: false" i have no warnings. Even if I
> > > create examples with random strings as properties. Are there some new
> > > json libraries i should use?
> >
> > Try
> >
> > additionalProperties: False
>
> Yes, it works. But in this case I'll do more changes. Just wont to make
> sure I do not fix not broken things.
I've been working on converting some old SoCs bindings from .txt to
.yaml. My observations is that the yaml is sometimes more restrictive
than what the drivers actually imposes. So you might need to change
perfectly working .dts files to get it warning free. Or you just
accept the warnings and move on. At lot will depend on the number of
warnings and how easy it is to see real problems mixed in with
warnings you never intend to fix.
Andrew
Ccing Rob Herring.
On Sat, Aug 13, 2022 at 10:42:05PM +0200, Andrew Lunn wrote:
> On Sat, Aug 13, 2022 at 06:18:50PM +0200, Oleksij Rempel wrote:
> > On Sat, Aug 13, 2022 at 05:11:45PM +0200, Andrew Lunn wrote:
> > > On Sat, Aug 13, 2022 at 04:32:15PM +0200, Oleksij Rempel wrote:
> > > > On Fri, Aug 05, 2022 at 04:42:34PM +0300, Vladimir Oltean wrote:
> > > > > On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote:
> > > > > > Hm, if we will have any random not support OF property in the switch
> > > > > > node. We won't be able to warn about it anyway. So, if it is present
> > > > > > but not supported, we will just ignore it.
> > > > > >
> > > > > > I'll drop this patch.
> > > > >
> > > > > To continue, I think the right way to go about this is to edit the
> > > > > dt-schema to say that these properties are only applicable to certain
> > > > > compatible strings, rather than for all. Then due to the
> > > > > "unevaluatedProperties: false", you'd get the warnings you want, at
> > > > > validation time.
> > > >
> > > > Hm, with "unevaluatedProperties: false" i have no warnings. Even if I
> > > > create examples with random strings as properties. Are there some new
> > > > json libraries i should use?
> > >
> > > Try
> > >
> > > additionalProperties: False
> >
> > Yes, it works. But in this case I'll do more changes. Just wont to make
> > sure I do not fix not broken things.
>
> I've been working on converting some old SoCs bindings from .txt to
> .yaml. My observations is that the yaml is sometimes more restrictive
> than what the drivers actually imposes. So you might need to change
> perfectly working .dts files to get it warning free. Or you just
> accept the warnings and move on. At lot will depend on the number of
> warnings and how easy it is to see real problems mixed in with
> warnings you never intend to fix.
Heh :) Currently with "unevaluatedProperties: false" restrictions do not
work at all. At least for me. For example with this change I have no
warnings:
diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
index 1e26d876d1463..da38ad98a152f 100644
--- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
@@ -120,6 +120,7 @@ examples:
ethernet-switch@1 {
reg = <0x1>;
compatible = "nxp,sja1105t";
+ something-random-here;
ethernet-ports {
#address-cells = <1>;
make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
So the main question is, is it broken for all or just for me? If it is
just me, what i'm doing wrong?
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Sun, Aug 14, 2022 at 06:26:08AM +0200, Oleksij Rempel wrote:
> Heh :) Currently with "unevaluatedProperties: false" restrictions do not
> work at all. At least for me. For example with this change I have no
> warnings:
> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> index 1e26d876d1463..da38ad98a152f 100644
> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> @@ -120,6 +120,7 @@ examples:
> ethernet-switch@1 {
> reg = <0x1>;
> compatible = "nxp,sja1105t";
> + something-random-here;
>
> ethernet-ports {
> #address-cells = <1>;
>
> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>
> So the main question is, is it broken for all or just for me? If it is
> just me, what i'm doing wrong?
Might it be due to the additionalProperties: true from spi-peripheral-props.yaml?