Message-ID: <w547dmmxqqa4atd62jqiqcfzhlnpjc7n64btjmre5pmbsci4br@w45tuny3mcmn>
Date: Mon, 2 Sep 2024 21:37:31 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
Cc: Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Krzysztof Wilczyński <kw@...ux.com>, Rob Herring <robh@...nel.org>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>, 
	cros-qcom-dts-watchers@...omium.org, Bartosz Golaszewski <brgl@...ev.pl>, 
	Jingoo Han <jingoohan1@...il.com>, Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
	andersson@...nel.org, quic_vbadigan@...cinc.com, linux-arm-msm@...r.kernel.org, 
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2 8/8] PCI: pwrctl: Add power control driver for qps615

On Mon, Sep 02, 2024 at 04:17:06PM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 9/2/2024 3:42 PM, Dmitry Baryshkov wrote:
> > On Mon, 2 Sept 2024 at 11:32, Krishna Chaitanya Chundru
> > <quic_krichai@...cinc.com> wrote:
> > > 
> > > 
> > > 
> > > On 9/2/2024 12:50 PM, Dmitry Baryshkov wrote:
> > > > On Mon, 2 Sept 2024 at 10:13, Krishna Chaitanya Chundru
> > > > <quic_krichai@...cinc.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 8/8/2024 9:00 AM, Dmitry Baryshkov wrote:
> > > > > > On August 5, 2024 1:14:47 PM GMT+07:00, Krishna Chaitanya Chundru <quic_krichai@...cinc.com> wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 8/3/2024 5:04 PM, Dmitry Baryshkov wrote:
> > > > > > > > On Sat, Aug 03, 2024 at 08:52:54AM GMT, Krishna chaitanya chundru wrote:
> > > > > > > > > QPS615 switch needs to be configured after powering on and before
> > > > > > > > > PCIe link was up.
> > > > > > > > > 
> > > > > > > > > As the PCIe controller driver already enables the PCIe link training
> > > > > > > > > at the host side, stop the link training. Otherwise the moment we turn
> > > > > > > > > on the switch it will participate in the link training and link may come
> > > > > > > > > up before switch is configured through i2c.
> > > > > > > > > 
> > > > > > > > > The device tree properties are parsed per node under pci-pci bridge in the
> > > > > > > > > driver. Each node has unique bdf value in the reg property, driver
> > > > > > > > > uses this bdf to differentiate ports, as there are certain i2c writes to
> > > > > > > > > select particular port.
> > > > > > > > > 
> > > > > > > > > Based up on dt property and port, qps615 is configured through i2c.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@...cinc.com>
> > > > > > > > > ---
> > > > > > > > >      drivers/pci/pwrctl/Kconfig             |   7 +
> > > > > > > > >      drivers/pci/pwrctl/Makefile            |   1 +
> > > > > > > > >      drivers/pci/pwrctl/pci-pwrctl-qps615.c | 638 +++++++++++++++++++++++++++++++++
> > > > > > > > >      3 files changed, 646 insertions(+)
> > > > > > > > > 

> > > > > > > > > +
> > > > > > > > > +  return qps615_pwrctl_i2c_write(ctx->client,
> > > > > > > > > +                                 is_l1 ? QPS615_PORT_L1_DELAY : QPS615_PORT_L0S_DELAY, units);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static int qps615_pwrctl_set_tx_amplitude(struct qps615_pwrctl_ctx *ctx,
> > > > > > > > > +                                    enum qps615_pwrctl_ports port, u32 amp)
> > > > > > > > > +{
> > > > > > > > > +  int port_access;
> > > > > > > > > +
> > > > > > > > > +  switch (port) {
> > > > > > > > > +  case QPS615_USP:
> > > > > > > > > +          port_access = 0x1;
> > > > > > > > > +          break;
> > > > > > > > > +  case QPS615_DSP1:
> > > > > > > > > +          port_access = 0x2;
> > > > > > > > > +          break;
> > > > > > > > > +  case QPS615_DSP2:
> > > > > > > > > +          port_access = 0x8;
> > > > > > > > > +          break;
> > > > > > > > > +  default:
> > > > > > > > > +          return -EINVAL;
> > > > > > > > > +  };
> > > > > > > > > +
> > > > > > > > > +  struct qps615_pwrctl_reg_setting tx_amp_seq[] = {
> > > > > > > > > +          {QPS615_PORT_ACCESS_ENABLE, port_access},
> > > > > > > > 
> > > > > > > > Hmm, this looks like another port selection, so most likely it should
> > > > > > > > also be under the same lock.
> > > > > > > > 
> > > > > > > > > +          {QPS615_PORT_LANE_ACCESS_ENABLE, 0x3},
> > > > > > > > > +          {QPS615_TX_MARGIN, amp},
> > > > > > > > > +  };
> > > > > > > > > +
> > > > > > > > > +  return qps615_pwrctl_i2c_bulk_write(ctx->client, tx_amp_seq, ARRAY_SIZE(tx_amp_seq));
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static int qps615_pwrctl_disable_dfe(struct qps615_pwrctl_ctx *ctx,
> > > > > > > > > +                               enum qps615_pwrctl_ports port)
> > > > > > > > > +{
> > > > > > > > > +  int port_access, lane_access = 0x3;
> > > > > > > > > +  u32 phy_rate = 0x21;
> > > > > > > > > +
> > > > > > > > > +  switch (port) {
> > > > > > > > > +  case QPS615_USP:
> > > > > > > > > +          phy_rate = 0x1;
> > > > > > > > > +          port_access = 0x1;
> > > > > > > > > +          break;
> > > > > > > > > +  case QPS615_DSP1:
> > > > > > > > > +          port_access = 0x2;
> > > > > > > > > +          break;
> > > > > > > > > +  case QPS615_DSP2:
> > > > > > > > > +          port_access = 0x8;
> > > > > > > > > +          lane_access = 0x1;
> > > > > > > > > +          break;
> > > > > > > > > +  default:
> > > > > > > > > +          return -EINVAL;
> > > > > > > > > +  };
> > > > > > > > > +
> > > > > > > > > +  struct qps615_pwrctl_reg_setting disable_dfe_seq[] = {
> > > > > > > > > +          {QPS615_PORT_ACCESS_ENABLE, port_access},
> > > > > > > > > +          {QPS615_PORT_LANE_ACCESS_ENABLE, lane_access},
> > > > > > > > > +          {QPS615_DFE_ENABLE, 0x0},
> > > > > > > > > +          {QPS615_DFE_EQ0_MODE, 0x411},
> > > > > > > > > +          {QPS615_DFE_EQ1_MODE, 0x11},
> > > > > > > > > +          {QPS615_DFE_EQ2_MODE, 0x11},
> > > > > > > > > +          {QPS615_DFE_PD_MASK, 0x7},
> > > > > > > > > +          {QPS615_PHY_RATE_CHANGE_OVERRIDE, 0x10},
> > > > > > > > > +          {QPS615_PHY_RATE_CHANGE, phy_rate},
> > > > > > > > > +          {QPS615_PHY_RATE_CHANGE, 0x0},
> > > > > > > > > +          {QPS615_PHY_RATE_CHANGE_OVERRIDE, 0x0},
> > > > > > > > > +
> > > > > > > > > +  };
> > > > > > > > > +
> > > > > > > > > +  return qps615_pwrctl_i2c_bulk_write(ctx->client,
> > > > > > > > > +                                      disable_dfe_seq, ARRAY_SIZE(disable_dfe_seq));
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static int qps615_pwrctl_set_nfts(struct qps615_pwrctl_ctx *ctx,
> > > > > > > > > +                            enum qps615_pwrctl_ports port, u32 nfts)
> > > > > > > > > +{
> > > > > > > > > +  int ret;
> > > > > > > > > +  struct qps615_pwrctl_reg_setting nfts_seq[] = {
> > > > > > > > > +          {QPS615_NFTS_2_5_GT, nfts},
> > > > > > > > > +          {QPS615_NFTS_5_GT, nfts},
> > > > > > > > > +  };
> > > > > > > > > +
> > > > > > > > > +  ret =  qps615_pwrctl_i2c_write(ctx->client, QPS615_PORT_SELECT, BIT(port));
> > > > > > > > > +  if (ret)
> > > > > > > > > +          return ret;
> > > > > > > > > +
> > > > > > > > > +  return qps615_pwrctl_i2c_bulk_write(ctx->client, nfts_seq, ARRAY_SIZE(nfts_seq));
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static int qps615_pwrctl_assert_deassert_reset(struct qps615_pwrctl_ctx *ctx, bool deassert)
> > > > > > > > > +{
> > > > > > > > > +  int ret, val = 0;
> > > > > > > > > +
> > > > > > > > > +  if (deassert)
> > > > > > > > > +          val = 0xc;
> > > > > > > > > +
> > > > > > > > > +  ret = qps615_pwrctl_i2c_write(ctx->client, QPS615_GPIO_CONFIG, 0xfffffff3);
> > > > > > > > 
> > > > > > > > It's a kind of magic
> > > > > > > > 
> > > > > > > I will add a macro in next patch.
> > > > > > > > > +  if (ret)
> > > > > > > > > +          return ret;
> > > > > > > > > +
> > > > > > > > > +  return qps615_pwrctl_i2c_write(ctx->client, QPS615_RESET_GPIO, val);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static int qps615_pwrctl_parse_device_dt(struct qps615_pwrctl_ctx *ctx, struct device_node *node)
> > > > > > > > > +{
> > > > > > > > > +  enum qps615_pwrctl_ports port;
> > > > > > > > > +  struct qps615_pwrctl_cfg *cfg;
> > > > > > > > > +  struct device_node *np;
> > > > > > > > > +  int bdf, fun_no;
> > > > > > > > > +
> > > > > > > > > +  bdf = of_pci_get_bdf(node);
> > > > > > > > > +  if (bdf < 0) {
> > > > > > > > 
> > > > > > > > This is incorrect, it will fail if at any point BDF uses the most
> > > > > > > > significant bit (which is permitted by the spec, if I'm not mistaken).
> > > > > > > > 
> > > > > > > As per the reg property as described in the binding document we are not
> > > > > > > expecting any change here.
> > > > > > > https://elixir.bootlin.com/linux/v6.10.3/source/Documentation/devicetree/bindings/pci/pci.txt#L50.
> > > > > > 
> > > > > > What will this function return if the bus no is 256?
> > > > > > The supported PCI bus number is from 0x0 to 0xff only. so we
> > > > > are not expecting any numbers greater than 0xff.
> > > > > > Also please either move the function to the generic PCI code is change its name to match the rest of the driver. The of_pci_ prefix is reserved for the generic code.
> > > > > > 
> > > > > ack.
> > > > > > 
> > > > > > > > > +          dev_err(ctx->pwrctl.dev, "Getting BDF failed\n");
> > > > > > > > > +          return 0;
> > > > > > > > > +  }
> > > > > > > > > +
> > > > > > > > > +  fun_no = bdf & 0x7;
> > > > > > > > 
> > > > > > > > I assume that ARI is not supported?
> > > > > > > > 
> > > > > > > Yes this doesn't support ARI.
> > > > > > > > > +
> > > > > > > > > +  /* In multi function node, ignore function 1 node */
> > > > > > > > > +  if (of_pci_get_bdf(of_get_parent(node)) == ctx->bdf->dsp3_bdf && !fun_no)
> > > > > > > > > +          port = QPS615_ETHERNET;
> > > > > > > > > +  else if (bdf == ctx->bdf->usp_bdf)
> > > > > > > > > +          port = QPS615_USP;
> > > > > > > > 
> > > > > > > > The function is being called for child device nodes. Thus upstream
> > > > > > > > facing port (I assume that this is what USP means) can not be enumerated
> > > > > > > > in this way.
> > > > > > > Sorry, but I didn't your question.
> > > > > > > 
> > > > > > > These settings will not affect the enumeration sequence these are
> > > > > > > for configuring ports only.
> > > > > > 
> > > > > > You are handling the case of bdf equal to the USP. Is it possible at all?
> > > > > > 
> > > > > at the time of the configuration the PCI link is not enabled yet,
> > > > > once we are done with the configurations only we are resumeing the link
> > > > > training. so when we start this configuration the link is not up yet.
> > > > 
> > > > Is your answer relevant to the question I have asked?
> > > > 
> > > sorry dmitry I might got your question wrong. what I understood is
> > > "you are configuring USP port before the link is up, is that possible?"
> > > I might read your statement wrongly.
> > > 
> > > If the question is "why do we need to configure USP?" I will try to
> > > respond below.
> > > "USP also will have l0s, L1 entry delays, nfts etc which can be
> > > configured".
> > > 
> > > Sorry once again if your question doesn't fall in both can you tell
> > > me your question.
> > 
> > My question was why the function gets executed for the root port. But
> > after reading the qps615_pwrctl_parse_device_dt() I have another
> > question: you are parsing DT nodes recursively. You should stop
> > parsing at the first level, so that grandchildren nodes can not
> > override your data (and so that the time isn't spent on parsing
> > useless data). Also I have the feeling that BDF parsing isn't so
> > correct. Will it work if QPS is sitting behind a PCI-PCI bridge?
> > 
> we are not executing for root port. we are configuring for USP
> since there are some features of USP which can be configured.

What is USP? Upstream side port?

> 
> we are trying to store each configurations in below line.
> cfg = &ctx->cfg[port];
> 
> port will have enum value based upon the bdf. after filling
> the parent node we calling recursive function for child nodes.
> As the BDF is unique value for each node we not expecting to get
> same enum value for child or grand child nodes and there will
> be no overwrite. If the BDF is not matched we are just returning
> instead of looking for the properties.
> 
> QPS615 node is defined as child of the pci-pci bridge only.
> The pwrctl framework is designed to work if the device
> is represented as child node in the pci-pci bridge only.

Of course. Each PCIe device is either a child of the root port or a
child of a pci-pci bridge. So are the BDFs specific to the case of
QPS615 being a child of the root PCIe bridge?

> 
> Hope it clarifies all the queries.

Yes. Please drop recursive parsing and add explicit single
for_each_child_of_node().

> 
> - Krishna chaitanya.
> > > > > > 
> > > > > > > > 
> > > > > > > > > +  else if (bdf == ctx->bdf->dsp1_bdf)
> > > > > > > > > +          port = QPS615_DSP1;
> > > > > > > > > +  else if (bdf == ctx->bdf->dsp2_bdf)
> > > > > > > > > +          port = QPS615_DSP2;
> > > > > > > > > +  else if (bdf == ctx->bdf->dsp3_bdf)
> > > > > > > > > +          port = QPS615_DSP3;
> > > > > > > > > +  else
> > > > > > > > > +          return 0;
> > > > > > > > 
> > > > > > > > -EINVAL >
> > > > > > > There are can be nodes describing endpoints also,
> > > > > > > for those nodes bdf will not match and we are not
> > > > > > > returning since it is expected for endpoint nodes.
> > > > > > 
> > > > > > Which endpoints? Bindings don't describe them.
> > > > > > 
> > > > > The client drivers like ethernet will add them once
> > > > > this series is merged. Their drivers are not present
> > > > > in the linux as of now.
> > > > 
> > > > The bindings describe the hardware, not the drivers. Also the driver
> > > > should work with the bindings that you have submitted, not some
> > > > imaginary to-be-submitted state. Please either update the bindings
> > > > within the patchset or fix the driver to return -EINVAL.
> > > > 
> > > The qps615 bindings describes only the PCIe switch part,
> > > the endpoints binding connected to the switch should be described by the
> > > respective clients like USB hub, NVMe, ethernet etc bindings should
> > > describe their hardware and its properties. And these bindings will
> > > defined in seperate bindinds file not in qps615 bindings.
> > > 
> > > for example:-
> > > 
> > > in the following example pcie@0,0 describes usp and
> > > pcie@1,0 & pcie@2,0 describes dsp's of the switch.
> > > now if we say usb hub is connected to dsp1 i.e to the
> > > node pcie@1,0 there will be a child node to the pcie@1,0
> > > to denote usb hub hardware.
> > > And that node is external to the switch and we are not
> > > configuring it through i2c. As these are pcie devices
> > > representation is generic one we can't say if the client
> > > nodes(in this case usb hub) will be present or not. if the child
> > > node( for example usb hub) is present we can't return -EINVAL
> > > because qps615 will not configure it.
> > > 
> > > &pcieport {
> > >          pcie@0,0 {
> > >                  pcie@1,0 {
> > >                          reg = <0x20800 0x0 0x0 0x0 0x0>;
> > >                          #address-cells = <3>;
> > >                          #size-cells = <2>;
> > > 
> > >                          device_type = "pci";
> > >                          ranges;
> > >                          usb_hub@0,0 {
> > >                                  //describes USB hub
> > >                          };
> > >                  };
> > > 
> > >                  pcie@2,0 {
> > >                          reg = <0x21000 0x0 0x0 0x0 0x0>;
> > >                          #address-cells = <3>;
> > >                          #size-cells = <2>;
> > > 
> > >                          device_type = "pci";
> > >                          ranges;
> > >                  };
> > >          };
> > > };

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists