Re: [PATCH v2] arm64: dts: qcom: x1e80100: Switch PCIe 6a to 4 lanes mode

[Abel Vesa <abel.vesa@linaro.org>]

On 24-10-07 13:19:33, Johan Hovold wrote:
> On Fri, Oct 04, 2024 at 12:06:33PM +0300, Abel Vesa wrote:
> > The PCIe 6a controller and PHY can be configured in 4-lanes mode or
> > 2-lanes mode. For 4-lanes mode, it fetches the lanes provided by PCIe 6b.
> 
> nit: I still think you should use "uses" over "fetches" here.

Urgh, I missed that one. Will fix.

> 
> > For 2-lanes mode, PCIe 6a uses 2 lanes and then PCIe 6b uses the other 2
> > lanes. Configure it in 4-lane mode and then each board can configure it
> > depending on the design. 
> 
> To avoid confusion you could avoid the word "configure" here. pcie6a is
> a four-lane controller (and PHY) that can also be used in used in
> two-lane mode, depending on how the system is configured and this can be
> read out through a TCSR register (e.g. the PHY driver adapts
> accordingly).

OK, will that.

> 
> So you should perhaps rather say that you are fixing the description and
> compatible of pcie6a, which *is* a 4-lane controller, that can also be
> used in 2-lane mode. Or similar.

Agreed. Will reword to say fixing the description as suggested.

Just to be sure. We still don't want this backported (even with such
rewording), so no fixes tag, right?

> 
> We also discussed this here:
> 
> 	https://lore.kernel.org/lkml/ZtG2dUVkdwBpBbix@hovoldconsulting.com/
> 
> > Both the QCP and CRD boards, currently upstream,
> > use PCIe 6a for NVMe in 4-lane mode. Mark the controller as 4-lane as
> > well.
> 
> > This is the last change needed in order to support NVMe with Gen4
> > 4-lanes on all existing X Elite boards.
> 
> I'd skip comments like this, or put them in the cover letter, or just
> rephrase as "This will enable 4-lane NVMe on...".

Will rephrase as suggested.

> 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> > Changes in v2:
> > - Re-worded the commit message according to Johan's suggestions
> > - Dropped the clocks changes.
> > - Dropped the fixes tag as this relies on the Gen4 4-lanes stability
> >   patchset which has been only merged in 6.12, so backporting this patch
> >   would break NVMe support for all platforms.
> > - Link to v1: https://lore.kernel.org/r/20240531-x1e80100-dts-fixes-pcie6a-v1-2-1573ebcae1e8@linaro.org
> 
> Patch itself looks good.
> 
> Johan

Thanks for reviewing.

Abel