Re: [PATCH v2 8/8] PCI: pwrctl: Add power control driver for qps615

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 9/3/2024 12:07 AM, Dmitry Baryshkov wrote:
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@xxxxxxxxxxx> wrote:



On 9/2/2024 12:50 PM, Dmitry Baryshkov wrote:
On Mon, 2 Sept 2024 at 10:13, Krishna Chaitanya Chundru
<quic_krichai@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
      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?

yes these are specific to 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().

Dimitry, the ethernet nodes which are child of dsp3 need extra
for_each_child_of_node and also we are going to add support for cascade
switch once this patch lands, in that cascade switch one more QPS615
switch will be connected to the one of the downstream port of the first
switch in that case we might need to do for_each_child_of_node twice
from  the dsp node where cascade switch is connected.
So it will good if we have this recursive parsing.

- Krishna Chaitanya.

- 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;
                  };
          };
};





[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux