From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id EF3F1C531DC for ; Fri, 23 Aug 2024 10:26:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Cc:List-Subscribe: List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Type:MIME-Version:References:Message-ID:Subject:To:Date:From:Reply-To :Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Uy9tQ1TYcZbcuxiuWon4EXX8Ukt9Xc78KSZT2kpFAzw=; b=s3Pp6V2v1FN3zEQ5yTeeYWOe61 GAI6P87WwxdWkwDW91pNUEkicLWmkmkGsn7w37I1qF3aae8dgY7eMgK43NjKmpPrOWZqdqjU7BZ2x tAL+sNWJatc8+/MICe/inb8+tp6Qf2YSDsMQAR1j6HXoB+Y4qMkMzS64VwvM1saxl69/KHSc1XRN3 7wg6CPCs463EQGuAdbO/om2nkFrk1YzTcNjgBQz9ex6WpiFTjMy0j2bBHLLysl7QSrGjA9RZOjHng wdaVDo8i1UrpXGfkSP0HWkyU2rkTIbX5gPBfclL5onGfbaKOK8paDzv1xY1GStS61lCOadLmj0p6T Hdl6MOtw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1shRUJ-0000000GKRc-2Tbg; Fri, 23 Aug 2024 10:25:47 +0000 Received: from mail-ej1-x62b.google.com ([2a00:1450:4864:20::62b]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1shRQC-0000000GJbt-1UG2 for linux-arm-kernel@lists.infradead.org; Fri, 23 Aug 2024 10:21:34 +0000 Received: by mail-ej1-x62b.google.com with SMTP id a640c23a62f3a-a8657900fc1so284852366b.1 for ; Fri, 23 Aug 2024 03:21:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1724408490; x=1725013290; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:date:from:from:to:cc :subject:date:message-id:reply-to; bh=Uy9tQ1TYcZbcuxiuWon4EXX8Ukt9Xc78KSZT2kpFAzw=; b=FUErY6LA55wiV6f5m/+1mrT/0GR3P2MiJEhWElkH5rBS1FKh4bJw42g9vVGwp0jBqo Duu0GQ8LqZDEWKOG6WutYNVCkOugSulyUfqDeIMkpAMhYCKr1M6NGjJ8RBnnhegeVT99 zoq/v7GDYgdgXbFuSven0p4hFpOFVqhJjGg1zTo1B8644Xq8emvn4De5xrs51VyrmzRd 2YAXUAHVLsSJpjdEwQK69xi3jOcCqb6let97sXivyPEYXoOXh5GgpX5G/B+pihpVcBmD OdKCxawuuyAewb3OOmOiv/HzF6Nmvdp3ZimP9RM9l7+e7KIOazgLWt2tY8zVGNxG1f2A 8zhw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724408490; x=1725013290; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:date:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Uy9tQ1TYcZbcuxiuWon4EXX8Ukt9Xc78KSZT2kpFAzw=; b=AqHu2K8kX6EPyK/k0xbrl4wrRFdbcyWmvW6Pgbjm8RNzZAJZTnWh0p0KCeSvtlJ/Uz YqLTiJLU00FE3NF47NxcoG4WQW2SRGfhfGM1dr8KsrmzfxGjYY9OSWQDoL5KPp8E0IxI 3QYrLbzrJUN7IUmkECuh4zJfLcvOb+b1k/F+ZZuAO48BppbluDRnGGwTZnzBLff8Fge4 3VM4NBXa1HdJSAxD32cePL5oAe9xCF6jr2mKSj+/8KEqfq2Bd6VaLE2UBz7JVwm5Dhmy zj7pq1X9dHAVI8g7wOSJ9/eC7BrbnILedncHzLJgVgDDt7VX5DKcLKKq4gH1E5Yh86i2 +dhA== X-Forwarded-Encrypted: i=1; AJvYcCWjfENSLrN4N4uWS86VijbAUafadgmwd/bWyUB0VzYxTqIKGwmYdS80FzpH+hDdacZMz6KT/ZV7WE73KYSOW9I5@lists.infradead.org X-Gm-Message-State: AOJu0Ywlmy8NqoJSHLqntnB9QDr5lcFfhVswdJFL0g837OHfx1mLfr3F S9Yc8dgwzlJGjwkvQnVvK7phDlDFwH5jascd2k8XGzQ21jjePKOrxtJUVKiAtw0= X-Google-Smtp-Source: AGHT+IEsqzlf61vWm+813vG7vMkMePBztenb+9XkV5XyCVO9mztSFfT2RcUPMSZK6WV+NNZgP3DJpQ== X-Received: by 2002:a17:907:7e95:b0:a86:94e2:2a47 with SMTP id a640c23a62f3a-a86a51b24b5mr152679166b.15.1724408490036; Fri, 23 Aug 2024 03:21:30 -0700 (PDT) Received: from localhost ([87.13.33.30]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a868f436322sm240714966b.117.2024.08.23.03.21.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 23 Aug 2024 03:21:29 -0700 (PDT) From: Andrea della Porta X-Google-Original-From: Andrea della Porta Date: Fri, 23 Aug 2024 12:21:36 +0200 To: Bjorn Helgaas Subject: Re: [PATCH 08/11] misc: rp1: RaspberryPi RP1 misc driver Message-ID: Mail-Followup-To: Bjorn Helgaas , Andrea della Porta , Michael Turquette , Stephen Boyd , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Florian Fainelli , Broadcom internal kernel review list , Linus Walleij , Catalin Marinas , Will Deacon , Derek Kiernan , Dragan Cvetic , Arnd Bergmann , Greg Kroah-Hartman , Nicolas Ferre , Claudiu Beznea , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Saravana Kannan , Bjorn Helgaas , linux-clk@vger.kernel.org, devicetree@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, netdev@vger.kernel.org, linux-pci@vger.kernel.org, linux-arch@vger.kernel.org, Lee Jones , Andrew Lunn , Stefan Wahren References: <5954e4dccc0e158cf434d2c281ad57120538409b.1724159867.git.andrea.porta@suse.com> <20240821165541.GA254124@bhelgaas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240821165541.GA254124@bhelgaas> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240823_032132_429650_C84509FE X-CRM114-Status: GOOD ( 34.13 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Andrew Lunn , Catalin Marinas , Michael Turquette , Claudiu Beznea , Eric Dumazet , Dragan Cvetic , Will Deacon , linux-clk@vger.kernel.org, linux-arch@vger.kernel.org, Rob Herring , Florian Fainelli , Lee Jones , Saravana Kannan , Broadcom internal kernel review list , linux-pci@vger.kernel.org, Jakub Kicinski , Paolo Abeni , Linus Walleij , devicetree@vger.kernel.org, Conor Dooley , Arnd Bergmann , linux-gpio@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, Bjorn Helgaas , Andrea della Porta , linux-arm-kernel@lists.infradead.org, Derek Kiernan , Stephen Boyd , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Stefan Wahren , netdev@vger.kernel.org, Krzysztof Kozlowski , "David S. Miller" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 11:55 Wed 21 Aug , Bjorn Helgaas wrote: > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > The RaspberryPi RP1 is ia PCI multi function device containing > > s/ia/a/ > > > peripherals ranging from Ethernet to USB controller, I2C, SPI > > and others. > > Add blank lines between paragraphs. > > > Implement a bare minimum driver to operate the RP1, leveraging > > actual OF based driver implementations for the on-borad peripherals > > s/on-borad/on-board/ > > > by loading a devicetree overlay during driver probe. > > The peripherals are accessed by mapping MMIO registers starting > > from PCI BAR1 region. > > As a minimum driver, the peripherals will not be added to the > > dtbo here, but in following patches. > > > +config MISC_RP1 > > + tristate "RaspberryPi RP1 PCIe support" > > + depends on PCI && PCI_QUIRKS > > + select OF > > + select OF_OVERLAY > > + select IRQ_DOMAIN > > + select PCI_DYNAMIC_OF_NODES > > + help > > + Support for the RP1 peripheral chip found on Raspberry Pi 5 board. > > + This device supports several sub-devices including e.g. Ethernet controller, > > + USB controller, I2C, SPI and UART. > > + The driver is responsible for enabling the DT node once the PCIe endpoint > > + has been configured, and handling interrupts. > > + This driver uses an overlay to load other drivers to support for RP1 > > + internal sub-devices. > > s/support for/support/ > > Add blank lines between paragraphs. Consider wrapping to fit in 80 > columns. Current width of 86 seems random. > > > diff --git a/drivers/misc/rp1/Makefile b/drivers/misc/rp1/Makefile > > new file mode 100644 > > index 000000000000..e83854b4ed2c > > --- /dev/null > > +++ b/drivers/misc/rp1/Makefile > > @@ -0,0 +1,3 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +rp1-pci-objs := rp1-pci.o rp1-pci.dtbo.o > > +obj-$(CONFIG_MISC_RP1) += rp1-pci.o > > diff --git a/drivers/misc/rp1/rp1-pci.c b/drivers/misc/rp1/rp1-pci.c > > new file mode 100644 > > index 000000000000..a6093ba7e19a > > --- /dev/null > > +++ b/drivers/misc/rp1/rp1-pci.c > > @@ -0,0 +1,333 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018-22 Raspberry Pi Ltd. > > s/22/24/ ? > > > +#define RP1_B0_CHIP_ID 0x10001927 > > +#define RP1_C0_CHIP_ID 0x20001927 > > Drop; both unused. > > > +#define RP1_PLATFORM_ASIC BIT(1) > > +#define RP1_PLATFORM_FPGA BIT(0) > > Drop; both unused. > > > +#define RP1_SYSCLK_RATE 200000000 > > +#define RP1_SYSCLK_FPGA_RATE 60000000 > > Drop; both unused. > > > +enum { > > + SYSINFO_CHIP_ID_OFFSET = 0, > > + SYSINFO_PLATFORM_OFFSET = 4, > > +}; > > Drop; unused. > > > +/* MSIX CFG registers start at 0x8 */ > > s/MSIX/MSI-X/ > > > +#define MSIX_CFG_TEST BIT(1) > > Unused. > > > +#define INTSTATL 0x108 > > +#define INTSTATH 0x10c > > Drop; both unused. > > > +static void dump_bar(struct pci_dev *pdev, unsigned int bar) > > +{ > > + dev_info(&pdev->dev, > > + "bar%d len 0x%llx, start 0x%llx, end 0x%llx, flags, 0x%lx\n", > > %pR does most of this for you. > > > +static int rp1_irq_set_type(struct irq_data *irqd, unsigned int type) > > +{ > > + struct rp1_dev *rp1 = irqd->domain->host_data; > > + unsigned int hwirq = (unsigned int)irqd->hwirq; > > + int ret = 0; > > + > > + switch (type) { > > + case IRQ_TYPE_LEVEL_HIGH: > > + dev_dbg(rp1->dev, "MSIX IACK EN for irq %d\n", hwirq); > > + msix_cfg_set(rp1, hwirq, MSIX_CFG_IACK_EN); > > + rp1->level_triggered_irq[hwirq] = true; > > + break; > > + case IRQ_TYPE_EDGE_RISING: > > + msix_cfg_clr(rp1, hwirq, MSIX_CFG_IACK_EN); > > + rp1->level_triggered_irq[hwirq] = false; > > + break; > > + default: > > + ret = -EINVAL; > > If you "return -EINVAL" directly here, I think you can drop "ret" and > just "return 0" below. > > > + break; > > + } > > + > > + return ret; > > +} > > > +static int rp1_irq_xlate(struct irq_domain *d, struct device_node *node, > > + const u32 *intspec, unsigned int intsize, > > + unsigned long *out_hwirq, unsigned int *out_type) > > +{ > > + struct rp1_dev *rp1 = d->host_data; > > + struct irq_data *pcie_irqd; > > + unsigned long hwirq; > > + int pcie_irq; > > + int ret; > > + > > + ret = irq_domain_xlate_twocell(d, node, intspec, intsize, > > + &hwirq, out_type); > > + if (!ret) { > > + pcie_irq = pci_irq_vector(rp1->pdev, hwirq); > > + pcie_irqd = irq_get_irq_data(pcie_irq); > > + rp1->pcie_irqds[hwirq] = pcie_irqd; > > + *out_hwirq = hwirq; > > + } > > + > > + return ret; > > if (ret) > return ret; > > ... > return 0; > > would make this easier to read and unindent the normal path. > > > + rp1->bar1 = pci_iomap(pdev, 1, 0); > > pcim_iomap() > > > + if (!rp1->bar1) { > > + dev_err(&pdev->dev, "Cannot map PCI bar\n"); > > s/bar/BAR/ > > > +#define PCI_VENDOR_ID_RPI 0x1de4 > > +#define PCI_DEVICE_ID_RP1_C0 0x0001 > > Device ID should include "RPI" as well. Ack to all suggestions. Fixed in the next release, thanks. Andrea