On 06/06/2024 11:14, Swathi K S wrote:

quoted

-----Original Message-----
From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
Sent: 18 August 2023 14:57
To: Sriranjani P <redacted>; davem@davemloft.net;
edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
conor+dt@kernel.org; richardcochran@gmail.com;
alexandre.torgue@foss.st.com; joabreu@synopsys.com;
mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
ravi.patel@samsung.com
Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; 'Chandrasekar R' [off-list ref];
'Suresh Siddha' [off-list ref]
Subject: Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support

On 16/08/2023 08:38, Sriranjani P wrote:

quoted

quoted

-----Original Message-----
From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
Sent: 15 August 2023 01:21
To: Sriranjani P <redacted>; davem@davemloft.net;
edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
conor+dt@kernel.org; richardcochran@gmail.com;
alexandre.torgue@foss.st.com; joabreu@synopsys.com;
mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
ravi.patel@samsung.com
Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; Chandrasekar R [off-list ref];
Suresh Siddha [off-list ref]
Subject: Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS
support

On 14/08/2023 13:25, Sriranjani P wrote:

quoted

The FSD SoC contains two instance of the Synopsys DWC ethernet QOS
IP

core.

quoted

The binding that it uses is slightly different from existing ones
because of the integration (clocks, resets).

For FSD SoC, a mux switch is needed between internal and external

clocks.

quoted

quoted

quoted

By default after reset internal clock is used but for receiving
packets properly, external clock is needed. Mux switch to external
clock happens only when the external clock is present.

Signed-off-by: Chandrasekar R <redacted>
Signed-off-by: Suresh Siddha <redacted>
Signed-off-by: Swathi K S <redacted>
Signed-off-by: Sriranjani P <redacted>
---

quoted

+static int dwc_eqos_setup_rxclock(struct platform_device *pdev, int
+ins_num) {
+	struct device_node *np = pdev->dev.of_node;
+	struct regmap *syscon;
+	unsigned int reg;
+
+	if (np && of_property_read_bool(np, "fsd-rx-clock-skew")) {
+		syscon = syscon_regmap_lookup_by_phandle_args(np,
+							      "fsd-rx-clock-

skew",

quoted

+							      1, &reg);
+		if (IS_ERR(syscon)) {
+			dev_err(&pdev->dev,
+				"couldn't get the rx-clock-skew syscon!\n");
+			return PTR_ERR(syscon);
+		}
+
+		regmap_write(syscon, reg, rx_clock_skew_val[ins_num]);
+	}
+
+	return 0;
+}
+
+static int fsd_eqos_clk_init(struct fsd_eqos_plat_data *plat,
+			     struct plat_stmmacenet_data *data) {
+	int ret = 0, i;
+
+	const struct fsd_eqos_variant *fsd_eqos_v_data =
+						plat->fsd_eqos_inst_var;
+
+	plat->clks = devm_kcalloc(plat->dev, fsd_eqos_v_data->num_clks,
+				  sizeof(*plat->clks), GFP_KERNEL);
+	if (!plat->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < fsd_eqos_v_data->num_clks; i++)
+		plat->clks[i].id = fsd_eqos_v_data->clk_list[i];
+
+	ret = devm_clk_bulk_get(plat->dev, fsd_eqos_v_data->num_clks,
+				plat->clks);

Instead of duplicating entire clock management with existing code,
you should extend/rework existing one.

This code is unfortunately great example how not to stuff vendor code
into upstream project. :(

I will check again if I can extend existing one to support FSD platform

specific requirement.

quoted

quoted

quoted

+
+	return ret;
+}
+
+static int fsd_clks_endisable(void *priv, bool enabled) {
+	int ret, num_clks;
+	struct fsd_eqos_plat_data *plat = priv;
+
+	num_clks = plat->fsd_eqos_inst_var->num_clks;
+
+	if (enabled) {
+		ret = clk_bulk_prepare_enable(num_clks, plat->clks);
+		if (ret) {
+			dev_err(plat->dev, "Clock enable failed, err = %d\n",

ret);

quoted

+			return ret;
+		}
+	} else {
+		clk_bulk_disable_unprepare(num_clks, plat->clks);
+	}
+
+	return 0;
+}
+
+static int fsd_eqos_probe(struct platform_device *pdev,
+			  struct plat_stmmacenet_data *data,
+			  struct stmmac_resources *res)
+{
+	struct fsd_eqos_plat_data *priv_plat;
+	struct device_node *np = pdev->dev.of_node;
+	int ret = 0;
+
+	priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat),

GFP_KERNEL);

quoted

+	if (!priv_plat) {
+		ret = -ENOMEM;

return -ENOMEM

Will fix this in v4.

quoted

quoted

+		goto error;
+	}
+
+	priv_plat->dev = &pdev->dev;
+	data->bus_id = of_alias_get_id(np, "eth");

No, you cannot do like this. Aliases are board specific and are based
on labeling on the board.

So if I understood this correctly, I need to move alias in the board
specific DTS file

This part: yes

quoted

and I can use this, because we have to handle rx-clock-skew differently for

the two instances in the FSD platform.

Not really. Do you expect it to work correctly if given EQoS instance receives
different alias, e.g. 5?

quoted

Another approach we took in v1, by specifying the value to be programmed

in rx-clock-skew property itself, but it seems it is not a preferred approach.

quoted

I can see that in drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c

+436 common code is already using this API and getting alias id, so I can
probably use the same rather getting same again here, but I have to specify
alias in DTS file.

Getting alias ID is okay in general. It is used to provide user-visible ID of the
devices (see mmc). Using such alias to configure hardware is abuse of the
alias, because of the reasons I said - how is it supposed to work if alias is 5
(this is perfectly valid alias)?

I suspect that all this is to substitute missing abstractions, like clocks, phys or
resets...

Will avoid using the API to get alias id to configure the HW. Will share the new implementation in v4.

That was August 2023, almost year ago.

Whatever you plan, expect having same questions in the discussion
because we forgot everything said that year ago...

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel