Re: [PATCH v3 1/5] dt-bindings: pci: Add ARTPEC-8 PCIe controller

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: wangseok.lee@samsung.com,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"krzk+dt@kernel.org" <krzk+dt@kernel.org>,
	"kishon@ti.com" <kishon@ti.com>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"jesper.nilsson@axis.com" <jesper.nilsson@axis.com>,
	"lars.persson@axis.com" <lars.persson@axis.com>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"linux-phy@lists.infradead.org" <linux-phy@lists.infradead.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"kw@linux.com" <kw@linux.com>,
	"linux-arm-kernel@axis.com" <linux-arm-kernel@axis.com>,
	"kernel@axis.com" <kernel@axis.com>
Cc: Moon-Ki Jun <moonki.jun@samsung.com>,
	Sang Min Kim <hypmean.kim@samsung.com>,
	Dongjin Yang <dj76.yang@samsung.com>,
	Yeeun Kim <yeeun119.kim@samsung.com>
Subject: Re: [PATCH v3 1/5] dt-bindings: pci: Add ARTPEC-8 PCIe controller
Date: Thu, 23 Jun 2022 10:27:18 +0200	[thread overview]
Message-ID: <aa5c28c8-85c8-3867-69d0-2eba693594ea@linaro.org> (raw)
In-Reply-To: <20220622072159epcms2p52a21560a7e60cffd13ea70e55ac15428@epcms2p5>

On 22/06/2022 09:21, Wangseok Lee wrote:
> On 21/06/2022 21:44, Krzysztof Kozlowski wrote:
>>  On 21/06/2022 09:42, Wangseok Lee wrote:
>>>>>   
>>>>>   samsung,syscon-bus-s-fsys:
>>>>>     description:
>>>>>       Phandle to bus-s path of fsys block, this register
>>>>>       are used for enabling bus-s.
>>>>>     $ref: /schemas/types.yaml#/definitions/phandle
>>>>>   
>>>>>   samsung,syscon-bus-p-fsys:
>>>>>     description:
>>>>>       Phandle to bus-p path of fsys block, this register
>>>>>       are used for enabling bus-p.
>>>>>     $ref: /schemas/types.yaml#/definitions/phandle
>>>>
>>>>  This two look unspecific and hacky workaround for missing drivers. Looks
>>>>  like instead of implementing interconnect or clock driver, you decided
>>>>  to poke some other registers. Why this cannot be an interconnect driver?
>>>>
>>>>
>>>  
>>>  bus-s, bus-p is a register that exists in the sysreg of the fsys block.
>>>  It is the same block as "fsys-sysreg" but is separated separately in
>>>  hardware.
>>  
>>  Two points here:
>>  1. If it is in FSYS, why it cannot be accessed with samsung,fsys-sysreg?
>>  2. If it is only register, shuld be described like this. You must
>>  describe item:
>>  https://protect2.fireeye.com/v1/url?k=0f529a57-50c9a332-0f531118-000babff32e3-50938d8198077d59&q=1&e=32284e69-bbed-4d09-b6d6-0a43428aebf5&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.18-rc1%2Fsource%2FDocumentation%2Fdevicetree%2Fbindings%2Fsoc%2Fsamsung%2Fexynos-usi.yaml%23L42
>>  
> 
> It would be better to access with fsys-sysreg, but their h/w address are
> far from each other. The fsys block consists of a system register and an
> additional control system register. "bus-s-fsys" and "bus-p-fsys" are
> additional control system register. sysreg and additional control sysreg
> addresses are far from each other and there are h/w registers that perform
> different functions between them.
> 
>>>  So, get resource is performed separately from "fsys-sysreg".
>>>  They set pcie slave, dbi related control settings,
>>>  naming "bus-x" seems to be interconnect.
>>>  I will add this description to property.
>>>  I don't think it need to use the interconnect driver,
>>>  so please let me know your opinion.
>>  
>>  Please document both in the bindings and in the driver usage of this
>>  register. Writing there "0" or "1" is not enough. If the documentation
>>  is good, I am fine with it. If the explanation is obfuscated/not
>>  sufficient, it will look like avoiding to implement a driver, which I
>>  don't want to accept.
>>  
> 
> I think i should add enough description. Is it sufficient to modify
> the name and description of property like this?
> 

Looks ok. Thank you.

Best regards,
Krzysztof