Re: [PATCH] PCI: Add power sequencing driver for PCI slots

[Sean Anderson <sean.anderson@linux.dev>]

On 1/6/26 18:14, Marek Vasut wrote:
> On 1/6/26 11:34 PM, Sean Anderson wrote:
>> Extend the PCI bridge driver to enable resources associated with PCI
>> slots like clocks, power rails, and resets. This is modeled off of the
>> PCI power control subsystem in Linux. The traditional compatible for PCI
>> slots in U-Boot is pci-bridge, but Linux uses the more-systematic
>> pciclass,0604 so add that as an option.
> 
> Oh, nice :)
> 
>> +static int __maybe_unused pci_bridge_probe(struct udevice *dev)
>> +{
>> +    struct clk clk;
>> +    struct gpio_desc perst;
>> +
>> +    if (!clk_get_by_index(dev, 0, &clk)) {
>> +        int ret = clk_enable(&clk);
>> +
>> +        if (ret)
>> +            return log_msg_ret("clk", ret);
> 
> Should we use dev_err() instead ?

I thought we weren't supposed to log by default in device drivers to reduce text size?

>> +        /* Delay for T_PERST-CLK (100 us for all slot types) */
>> +        udelay(100);
>> +    }
>> +
>> +    if (!gpio_request_by_name(dev, "reset-gpios", 0, &perst, 0)) {
> 
> Invert conditional, reduce indent.

OK

>> +        unsigned long delay = 0;
>> +        int ret;
>> +
>> +        /*
>> +         * If PERST is inactive, the following call to dm_gpio_clrset_flags
>> +         * will be the first time we assert it and we will need to
>> +         * delay for T_PERST.
>> +         */
>> +        if (dm_gpio_get_value(&perst) != 1)
>> +            delay = 100;
>> +
>> +        ret = dm_gpio_clrset_flags(&perst, GPIOD_MASK_DIR,
>> +                       GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
>> +        if (ret)
>> +            return log_msg_ret("set", ret);
>> +        mdelay(delay);
> 
> Maybe set a flag and avoid calling mdelay() altogether ?

OK

--Sean

>> +        ret = dm_gpio_set_value(&perst, 0);
>> +        if (ret)
>> +            return log_msg_ret("clr", ret);
>> +
>> +        /*
>> +         * PCIe section 6.6.1:
>> +         * > ... software must wait a minimum of 100 ms before sending a
>> +         * > Configuration Request to the device immediately below that
>> +         * > Port.
>> +         */
>> +        mdelay(100);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   U_BOOT_DRIVER(pci_bridge_drv) = {
>>       .name        = "pci_bridge_drv",
>>       .id        = UCLASS_PCI,
>>       .of_match    = pci_bridge_ids,
>> +#if CONFIG_IS_ENABLED(PCI_PWRCTRL_SLOT)
>> +    .probe        = pci_bridge_probe,
> 
> .probe = CONFIG_IS_ENABLED(PCI_PWRCTRL_SLOT, pci_bridge_probe, NULL),
> 
> Thanks !