| Message ID | 20211006223603.18858-13-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Multiple improvement for qca8337 switch | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Series has a cover letter |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | warning | WARNING: line length of 85 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
> +static int > +qca8k_setup_of_pws_reg(struct qca8k_priv *priv) > +{ > + struct device_node *node = priv->dev->of_node; > + u32 val = 0; > + > + if (priv->switch_id == QCA8K_ID_QCA8327) > + if (of_property_read_bool(node, "qca,package48")) > + val |= QCA8327_PWS_PACKAGE48_EN; What does this actually do? How is PACKAGE48 different to normal mode? > + > + if (of_property_read_bool(node, "qca,power-on-sel")) > + val |= QCA8K_PWS_POWER_ON_SEL; What happens if you unconditionally do this? Why is a DT property required? > + > + if (of_property_read_bool(node, "qca,led-open-drain")) > + /* POWER_ON_SEL needs to be set when configuring led to open drain */ > + val |= QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL; This is getting into territory of adding LED support for PHYs, which we want to do via the LED subsystem. Andrew
On Thu, Oct 07, 2021 at 02:41:10AM +0200, Andrew Lunn wrote: > > +static int > > +qca8k_setup_of_pws_reg(struct qca8k_priv *priv) > > +{ > > + struct device_node *node = priv->dev->of_node; > > + u32 val = 0; > > + > > + if (priv->switch_id == QCA8K_ID_QCA8327) > > + if (of_property_read_bool(node, "qca,package48")) > > + val |= QCA8327_PWS_PACKAGE48_EN; > > What does this actually do? How is PACKAGE48 different to normal mode? > I actually made a typo. Anyway the difference is that they made 2 different package version of the qca8327. One with 176 pin and one with 148 pin. Setting the wrong layout cause the switch malfunction (no traffic, we found this on one xiaomi device). This is from Documenation and it does toggle the MAC interface configuration for the 2 different package. > > + > > + if (of_property_read_bool(node, "qca,power-on-sel")) > > + val |= QCA8K_PWS_POWER_ON_SEL; > > What happens if you unconditionally do this? Why is a DT property > required? > This is needed to bypass the power on strapping and use the regs config. The switch can use hardware pin to set eeprom presence and leds open drain. Setting this bit on bypass the hardware strapping and sets these 2 thing based on the regs. We didn't add the eeprom binding as we didn't find any switch using it and we don't have any support for it. > > + > > + if (of_property_read_bool(node, "qca,led-open-drain")) > > + /* POWER_ON_SEL needs to be set when configuring led to open drain */ > > + val |= QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL; > > This is getting into territory of adding LED support for PHYs, which > we want to do via the LED subsystem. > Don't know if it would be the correct way. Without this the switch leds chaese to work. I think this should be handled in a dedicated way than defined in a binding in the leds configuration. But I could be wrong. > Andrew
> Anyway the difference is that they made 2 different package version of > the qca8327. One with 176 pin and one with 148 pin. I assume they have different product numbers. So you can quote them in the DT binding? Are they BGP or QFP? Can somebody easily count the pins? > > > + > > > + if (of_property_read_bool(node, "qca,power-on-sel")) > > > + val |= QCA8K_PWS_POWER_ON_SEL; > > > > What happens if you unconditionally do this? Why is a DT property > > required? > > > > This is needed to bypass the power on strapping and use the regs config. > The switch can use hardware pin to set eeprom presence and leds open > drain. Setting this bit on bypass the hardware strapping and sets these > 2 thing based on the regs. So first off, it sounds like you have the DT property named wrong. It should be 'qca,ignore-power-on-sel'. However, why do you even need this? Generally, strapping gives you the defaults. Registers get loaded with a value determined by the strapping. But after that, you can change the value, based on additional information. Or are you saying the register is read only when strapping is used? > > > + if (of_property_read_bool(node, "qca,led-open-drain")) > > > + /* POWER_ON_SEL needs to be set when configuring led to open drain */ > > > + val |= QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL; At minimum, you need to clearly document that qca,led-open-drain implies 'qca,ignore-power-on-sel'. I would probably go further and return -EINVAL if qca,led-open-drain is set and 'qca,ignore-power-on-sel' is not. Andrew
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c index 01b05dfeae2b..209f8d3c9ea8 100644 --- a/drivers/net/dsa/qca8k.c +++ b/drivers/net/dsa/qca8k.c @@ -1014,6 +1014,28 @@ qca8k_setup_port0_pad_ctrl_reg(struct qca8k_priv *priv) return ret; } +static int +qca8k_setup_of_pws_reg(struct qca8k_priv *priv) +{ + struct device_node *node = priv->dev->of_node; + u32 val = 0; + + if (priv->switch_id == QCA8K_ID_QCA8327) + if (of_property_read_bool(node, "qca,package48")) + val |= QCA8327_PWS_PACKAGE48_EN; + + if (of_property_read_bool(node, "qca,power-on-sel")) + val |= QCA8K_PWS_POWER_ON_SEL; + + if (of_property_read_bool(node, "qca,led-open-drain")) + /* POWER_ON_SEL needs to be set when configuring led to open drain */ + val |= QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL; + + return qca8k_rmw(priv, QCA8K_REG_PWS, + QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL, + val); +} + static int qca8k_setup(struct dsa_switch *ds) { @@ -1039,6 +1061,10 @@ qca8k_setup(struct dsa_switch *ds) if (ret) return ret; + ret = qca8k_setup_of_pws_reg(priv); + if (ret) + return ret; + ret = qca8k_setup_of_rgmii_delay(priv); if (ret) return ret; diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h index 3fded69a6839..90f4616c33f1 100644 --- a/drivers/net/dsa/qca8k.h +++ b/drivers/net/dsa/qca8k.h @@ -48,6 +48,9 @@ #define QCA8K_MAX_DELAY 3 #define QCA8K_PORT_PAD_SGMII_EN BIT(7) #define QCA8K_REG_PWS 0x010 +#define QCA8K_PWS_POWER_ON_SEL BIT(31) +#define QCA8327_PWS_PACKAGE48_EN BIT(30) +#define QCA8K_PWS_LED_OPEN_EN_CSR BIT(24) #define QCA8K_PWS_SERDES_AEN_DIS BIT(7) #define QCA8K_REG_MODULE_EN 0x030 #define QCA8K_MODULE_EN_MIB BIT(0)
Some qca8327 switch require the power_on_sel enabled for the pws_reg or sets the led to open drain. Also qca8327 switch have a special mode to declare reduced 48pin layout. This special mode is only present in qca8327 as qca8337 have a different pws_reg table. Introduce a new binding and support these special configuration. Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> --- drivers/net/dsa/qca8k.c | 26 ++++++++++++++++++++++++++ drivers/net/dsa/qca8k.h | 3 +++ 2 files changed, 29 insertions(+)