Re: [PATCH v4 2/2] PCI: rockchip: add DesignWare based PCIe controller

["Krzysztof Wilczyński" <kw@linux.com>]

Hi Simon,

The subject should start with a capital letter.

[...]
> pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host
> is Rockchip designed IP which is only used for RK3399. So all the following
> non-RK3399 SoCs should use this driver.

You might need to wrap the long line in the above.

The commit message above could use some polish in terms of wording and
adding more context - what are you adding, what is this driver going to
support.  For example, what are the "all the following" SoCs?  Should
there be a list?  Or did you mean "all the other (...)", etc.

[...]
> +config PCIE_ROCKCHIP_DW_HOST
> +	bool "Rockchip DesignWare PCIe controller"
> +	select PCIE_DW
> +	select PCIE_DW_HOST
> +	depends on ARCH_ROCKCHIP || COMPILE_TEST
> +	depends on OF
> +	help
> +	  Enables support for the DW PCIe controller in the Rockchip SoC.

Perhaps replacing "DW" with "DesignWare" would be better.  Also, do you
want to mention here - for the sake of the future user - that this not
intended to support RK3399 as per the commit message.

[...]
> +/*
> + * PCIe host controller driver for Rockchip SoCs

A nitpick.  Missing period at the end of the sentence.

[...]
> +static int rockchip_pcie_link_up(struct dw_pcie *pci)
> +{
> +	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
> +	u32 val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS);
> +
> +	if ((val & (PCIE_RDLH_LINKUP | PCIE_SMLH_LINKUP)) == 0x30000 &&
[...]

A suggestion.  Would it make sense to add a definition for this
open-coded value of 0x30000 above?

> +static void rockchip_pcie_fast_link_setup(struct rockchip_pcie *rockchip)
> +{
> +	u32 val;
> +
> +	/* LTSSM EN ctrl mode */
[...]

Does this comment above stands for "LTSSM enable control mode"?

> +static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip)
> +{
> +	int ret;
> +	struct device *dev = rockchip->pci.dev;

These two variables should swap place to keep order of use, and to match
how it has been done everywhere else in this drivers.

> +
> +	rockchip->phy = devm_phy_get(dev, "pcie-phy");
> +	if (IS_ERR(rockchip->phy))
> +		return dev_err_probe(dev, PTR_ERR(rockchip->phy),
> +				     "missing phy\n");

I would be "PHY" here.

> +	ret = phy_init(rockchip->phy);
> +	if (ret < 0)
> +		return ret;
> +
> +	phy_power_on(rockchip->phy);
[...]

We should probably check phy_power_on() for a possible failure.  Some
platforms also clean up on a failure from phy_init() and phy_power_on(),
but I am not sure if this particular platform would require anything.

[...]
> +	/* DON'T MOVE ME: must be enable before phy init */

It would be "PHY" here.

> +	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> +	if (IS_ERR(rockchip->vpcie3v3))
> +		return dev_err_probe(dev, PTR_ERR(rockchip->rst),
> +				     "failed to get vpcie3v3 regulator\n");
> +
> +	if (rockchip->vpcie3v3) {
> +		ret = regulator_enable(rockchip->vpcie3v3);
> +		if (ret) {
> +			dev_err(dev, "fail to enable vpcie3v3 regulator\n");

It would be "failed" here.

[...]
> +static const struct of_device_id rockchip_pcie_of_match[] = {
> +	{ .compatible = "rockchip,rk3568-pcie", },
> +	{ /* sentinel */ },
> +};

We could probably drop the comment about the "sentinel" from the above.

Krzysztof