Hi Marek,
On 6/14/24 15:58, Marek Vasut wrote:
On 6/14/24 3:08 PM, Christophe Roullier wrote:
[...]
+static int stm32mp2_configure_syscfg(struct plat_stmmacenet_data
*plat_dat)
+{
+ struct stm32_dwmac *dwmac = plat_dat->bsp_priv;
+ u32 reg = dwmac->mode_reg;
+ int val = 0;
+
+ switch (plat_dat->mac_interface) {
+ case PHY_INTERFACE_MODE_MII:
+ break;
dwmac->enable_eth_ck does not apply to MII mode ? Why ?
It is like MP1 and MP13, nothing to set in syscfg register for case MII
mode wo crystal.
+ case PHY_INTERFACE_MODE_GMII:
+ if (dwmac->enable_eth_ck)
+ val |= SYSCFG_ETHCR_ETH_CLK_SEL;
+ break;
+ case PHY_INTERFACE_MODE_RMII:
+ val = SYSCFG_ETHCR_ETH_SEL_RMII;
+ if (dwmac->enable_eth_ck)
+ val |= SYSCFG_ETHCR_ETH_REF_CLK_SEL;
+ break;
+ case PHY_INTERFACE_MODE_RGMII:
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ case PHY_INTERFACE_MODE_RGMII_TXID:
+ val = SYSCFG_ETHCR_ETH_SEL_RGMII;
+ if (dwmac->enable_eth_ck)
+ val |= SYSCFG_ETHCR_ETH_CLK_SEL;
+ break;
+ default:
+ dev_err(dwmac->dev, "Mode %s not supported",
+ phy_modes(plat_dat->mac_interface));
+ /* Do not manage others interfaces */
+ return -EINVAL;
+ }
+
+ dev_dbg(dwmac->dev, "Mode %s", phy_modes(plat_dat->mac_interface));
+
+ /* select PTP (IEEE1588) clock selection from RCC
(ck_ker_ethxptp) */
Drop extra leading space.
Sentence starts with capital letter.
ok
+ val |= SYSCFG_ETHCR_ETH_PTP_CLK_SEL;
+
+ /* Update ETHCR (set register) */
+ return regmap_update_bits(dwmac->regmap, reg,
+ SYSCFG_MP2_ETH_MASK, val);
+}
+
static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
{
int ret;
@@ -292,6 +346,21 @@ static int stm32mp1_set_mode(struct
plat_stmmacenet_data *plat_dat)
return stm32mp1_configure_pmcr(plat_dat);
}
+static int stm32mp2_set_mode(struct plat_stmmacenet_data *plat_dat)
+{
+ int ret;
+
+ ret = stm32mp1_select_ethck_external(plat_dat);
+ if (ret)
+ return ret;
+
+ ret = stm32mp1_validate_ethck_rate(plat_dat);
+ if (ret)
+ return ret;
Is it necessary to duplicate this entire function instead of some:
if (is_mp2)
return stm32mp2_configure_syscfg(plat_dat);
else
return stm32mp1_configure_syscfg(plat_dat);
?
ok I would like to avoid to use is_mp2 boolean but you are right it is
simplify visibility of the code.
+ return stm32mp2_configure_syscfg(plat_dat);
+}
+
static int stm32mcu_set_mode(struct plat_stmmacenet_data *plat_dat)
{
struct stm32_dwmac *dwmac = plat_dat->bsp_priv;
@@ -348,12 +417,6 @@ static int stm32_dwmac_parse_data(struct
stm32_dwmac *dwmac,
return PTR_ERR(dwmac->clk_rx);
}
- if (dwmac->ops->parse_data) {
- err = dwmac->ops->parse_data(dwmac, dev);
- if (err)
- return err;
- }
-
/* Get mode register */
dwmac->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscon");
if (IS_ERR(dwmac->regmap))
@@ -365,20 +428,14 @@ static int stm32_dwmac_parse_data(struct
stm32_dwmac *dwmac,
return err;
}
- dwmac->mode_mask = SYSCFG_MP1_ETH_MASK;
- err = of_property_read_u32_index(np, "st,syscon", 2,
&dwmac->mode_mask);
- if (err) {
- if (dwmac->ops->is_mp13)
- dev_err(dev, "Sysconfig register mask must be set
(%d)\n", err);
- else
- dev_dbg(dev, "Warning sysconfig register mask not set\n");
- }
+ if (dwmac->ops->parse_data)
+ err = dwmac->ops->parse_data(dwmac, dev);
Why is this change here ? What is the purpose ?
This should be documented in commit message too.
The indirect call is not necessary either, simply do
if (is_mp2)
return err;
... do mp15/13 stuff here ...
return err;
Right, with use of is_mp2 variable it is more simple
[...]
