Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB03.amd.com; pr=C
Message-ID: <e59cb465-3f0d-4d11-8585-f0716cb09061@amd.com>
Date:   Mon, 23 Jan 2023 18:30:39 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver
Content-Language: en-US
To:     Rob Herring <robh@kernel.org>,
        Vincent Whitchurch <vincent.whitchurch@axis.com>
CC:     Lee Jones <lee@kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        kernel <kernel@axis.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Zhen, Max" <Max.Zhen@amd.com>,
        "Santan, Sonal" <sonal.santan@amd.com>
References: <20230120-simple-mfd-pci-v1-1-c46b3d6601ef@axis.com>
 <Y86op9oh5ldrZQyG@google.com> <Y86vm0zDOeb1G3zh@axis.com>
 <CAL_JsqLHi_1QJ4s28uM1b_Z2f+Nsu_CusAsqsx6vVOCCAPLMgw@mail.gmail.com>
From:   Lizhi Hou <lizhi.hou@amd.com>
In-Reply-To: <CAL_JsqLHi_1QJ4s28uM1b_Z2f+Nsu_CusAsqsx6vVOCCAPLMgw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Jan 2023 02:31:01.9775
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 3ac95a85-160a-433f-f8f9-08dafdb30926
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB03.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: CY4PEPF0000C973.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4422
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 1/23/23 08:36, Rob Herring wrote:
> On Mon, Jan 23, 2023 at 10:02 AM Vincent Whitchurch
> <vincent.whitchurch@axis.com> wrote:
>> On Mon, Jan 23, 2023 at 04:32:55PM +0100, Lee Jones wrote:
>>> On Mon, 23 Jan 2023, Vincent Whitchurch wrote:
>>>> Add a PCI driver which registers all child nodes specified in the
>>>> devicetree.  It will allow platform devices to be used on virtual
>>>> systems which already support PCI and devicetree, such as UML with
>>>> virt-pci.
>>>>
>>>> The driver has no id_table by default; user space needs to provide one
>>>> using the new_id mechanism in sysfs.
>>> This feels wrong for several reasons.
>>>
>>> Firstly, I think Greg (Cc:ed) will have something to say about this.
>>>
>>> Secondly, this driver does literally nothing.
>> Well, it does do what the commit message says.  If there's another way
>> of accomplishing that, I'm all ears.
>>
>>> Why can't you use of of the other, pre-existing "also register my
>>> children" compatibles?
>>>
>>> See: drivers/bus/simple-pm-bus.c
>>>       drivers/of/platform.c
>> simple-pm-bus registers a platform driver, and drivers/of/platform.c
>> works on the platform bus.  The driver added by this patch is a PCI
>> driver.  So I don't understand how the files you mention could be used
>> here?
>>
>> In case it helps, the relevant nodes in my UML devicetree look something
>> like this:
>>
>>      virtio@2 {
> dtc should complain about this...
>
>>          compatible = "virtio,uml";
> Binding?
>
>>          virtio-device-id = <1234>;
>>          ranges;
>>
>>          pci {
>>                  #address-cells = <3>;
>>                  #size-cells = <2>;
>>                  ranges = <0x0000000 0 0 0 0xf0000000 0 0x20000>;
>>                  compatible = "virtio,device4d2", "pci";
> "pci" is not a valid compatible string.
>
>>                  device_type = "pci";
>>                  bus-range = <0 0>;
>>
>>                  platform_parent: device@0,0 {
>>                          compatible = "pci494f,dc8";
>>                          reg = <0x00000 0 0 0x0 0x10000>;
>>                          ranges;
>>
>>                          uart@10000 {
>>                                  compatible = "google,goldfish-tty";
>>                                  reg = <0x00000 0 0x10000 0 0x10000>;
> This is not a PCI device, so it shouldn't be using PCI addressing.
> 'ranges' needs an entry (for each BAR) to translate to just a normal
> MMIO bus with 1 or 2 address/size cells. Maybe we want a 'simple-bus'
> node for each BAR. The FPGA series needs the same things, but that
> aspect hasn't really been addressed as the first issue is populating
> the PCI devices dynamically.
>
> The DT address translation code should support all this
> (MMIO->PCI->MMIO), but I don't think there's any existing examples. An
> example (that I can test) would be great. If the unittest had that
> example, I'd be thrilled.
(I tried to reply with my comment this morning, but it did not post to
kernel mail alias. I am re-sending it. Please ignore if you already
received my reply)

I have proposed the address format for hardware apertures on PCI BAR.
The address consists of BAR index and offset. It uses the following
encoding:

     0xIooooooo 0xoooooooo

   Where:

     I = BAR index
     oooooo oooooooo = BAR offset

   The endpoint is compatible with 'simple-bus' and contains 'ranges'
   property for translating aperture address to CPU address.

Ref: 
https://lore.kernel.org/lkml/20220305052304.726050-3-lizhi.hou@xilinx.com/
The 64-bit address can use the default translator defined of/address.c

I implemented an example of top of my latest patch
https://lore.kernel.org/lkml/1674183732-5157-1-git-send-email-lizhi.hou@amd.com/
to verify the address translation. The test device tree nodes are defined
to present two hardware apertures on our alveo PCI device:

   pr-isolation:  PCI BAR 0, offset 0x41000, len 0x1000
   xdma: PCI BAR 2, offset 0x0, len 0x40000

     / {
         fragment@0 {
             target-path="";
             __overlay__ {
                 pci-ep-bus@0 {
                     compatible = "simple-bus";
                     #address-cells = <2>;
                     #size-cells = <2>;
                     ranges = <0x0 0x0 0x0 0x0 0x0 0x2000000>;
                     pr_isolate_ulp@41000 {
                         compatible = "xlnx,alveo-pr-isolation";
                         reg = <0x0 0x41000 0x0 0x1000>;
                     };
                 };
                 pci-ep-bus@2 {
                     compatible = "simple-bus";
                     #address-cells = <2>;
                     #size-cells = <2>;
                     ranges = <0x0 0x0 0x20000000 0x0 0x0 0x40000>;
                     alveo-xdma@0 {
                         compatible = "xlnx,alveo-xdma";
                         reg = <0x0 0x0 0x0 0x40000>;
                     };
                 };
             };
         };
     };

Overall, the test is as below

1) added code to generate 'ranges' for PCI endpoint dt node

    00000000 00000000 C30B0000 00000080 10000000 00000000 02000000 (BAR 0)

    20000000 00000000 C30B0000 00000080 14000000 00000000 00040000 (BAR 2)

    ^ bar index                ^^^^^^^^^^^^^BAR IOMEM address

    code link: 
https://github.com/houlz0507/linux-xoclv2/compare/29031e597fd6272f825dd04ba41a38defb77a99a...pci-dt-v6?diff=unified#diff-bf1b86155c18e04c439b74f5a02bad99c91a8c04f3c21243afce996c2174be56

2) overlay the test device tree fragment under PCI endpoint.

3) Alveo pci device driver probe function calls 
of_platform_default_populate().

I can see the BAR index+offset is translated to IO address correctly.

# ls /sys/bus/platform/devices/
0.flash
3f000000.pcie
3f000000.pcie:pci@2,0:pci@0,0:pci@0,0:dev@0,0:pci-ep-bus@0
3f000000.pcie:pci@2,0:pci@0,0:pci@0,0:dev@0,0:pci-ep-bus@2
8010041000.pr_isolate_ulp
8014000000.alveo-xdma

# lspci -vd 10ee:5020 | grep Memory
     Memory at 8010000000 (64-bit, prefetchable) [disabled] [size=32M]
     Memory at 8014000000 (64-bit, prefetchable) [disabled] [size=256K]

This test needs our Alveo PCI device. It might be a reference to create 
unittest.

Thanks,
Lizhi

>
> Rob