mbox series

[0/5] Support for Cadence xSPI Marvell modifications

Message ID 20240329194849.25554-1-wsadowski@marvell.com
Headers show
Series Support for Cadence xSPI Marvell modifications | expand

Message

Witold Sadowski March 29, 2024, 7:48 p.m. UTC 
This patch series is adding support for additional
Marvell HW overlay build on top of Cadence xSPI IP
It includes:
- Clock and PHY configuration
- ACPI support
- Additional MRVL HW overlay to support tranfer operations

Piyush Malgujar (1):
  driver: spi: cadence: Add ACPI support

Witold Sadowski (4):
  spi: cadence: Add new bindings documentation for Cadence XSPI
  spi: cadence: Add Marvell IP modification changes
  spi: cadence: Force single modebyte
  cadence-xspi: Add xfer capabilities

 .../devicetree/bindings/spi/cdns,xspi.yaml    |  84 ++-
 drivers/spi/spi-cadence-xspi.c                | 675 +++++++++++++++++-
 2 files changed, 738 insertions(+), 21 deletions(-)

Comments

Krzysztof Kozlowski March 30, 2024, 11:33 a.m. UTC | #1 
On 29/03/2024 20:48, Witold Sadowski wrote:
> Add support for Marvell IP modification - clock divider,
> and PHY config, and IRQ clearing.
> Clock divider block is build into Cadence XSPI controller
> and is connected directly to 800MHz clock.
> As PHY config is not set directly in IP block, driver can
> load custom PHY configuration values.
> To correctly clear interrupt in Marvell implementation
> MSI-X must be cleared too.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Signed-off-by: Witold Sadowski <wsadowski@marvell.com>
> ---


> +
> +static bool cdns_xspi_get_hw_overlay(struct platform_device *pdev)
> +{
> +	int err;
> +
> +	err = device_property_match_string(&pdev->dev,
> +					   "compatible", "mrvl,xspi-nor");

No, do not add matching in some random parts of the code, but use driver
match/data from ID table.

....

>  
> +	cdns_xspi_print_phy_config(cdns_xspi);
>  	ret = cdns_xspi_controller_init(cdns_xspi);
>  	if (ret) {
>  		dev_err(dev, "Failed to initialize controller\n");
> @@ -613,6 +911,9 @@ static const struct of_device_id cdns_xspi_of_match[] = {
>  	{
>  		.compatible = "cdns,xspi-nor",
>  	},
> +	{
> +		.compatible = "mrvl,xspi-nor",

This falsely suggest they are compatible :/

> +	},
>  	{ /* end of table */}
>  };
>  MODULE_DEVICE_TABLE(of, cdns_xspi_of_match);

Best regards,
Krzysztof
Krzysztof Kozlowski March 30, 2024, 11:36 a.m. UTC | #2 
On 29/03/2024 20:48, Witold Sadowski wrote:
> From: Piyush Malgujar <pmalgujar@marvell.com>
> 
> These changes enables to read the configs from ACPI tables as
> required for successful probing in ACPI uefi environment.
> In case of ACPI disabled/dts based environment, it will continue to
> read configs from dts as before.
> 

Random subjects... Please use subject prefixes matching the subsystem.
You can get them for example with `git log --oneline --
DIRECTORY_OR_FILE` on the directory your patch is touching.

>  }
>  
> +static const struct acpi_device_id cdns_xspi_acpi_match[] = {
> +	{"cdns,xspi-nor", 0},
> +	{"mrvl,xspi-nor", 0},

How is this ACPI?

> +	{},
> +};
> +MODULE_DEVICE_TABLE(acpi, cdns_xspi_acpi_match);
> +#ifdef CONFIG_OF

This was never compiled. I could understand not testing bindings,
because it is something relatively new - like 5 or 6 years. But not
compiling code is less understandable.

Best regards,
Krzysztof
Krzysztof Kozlowski March 30, 2024, 11:37 a.m. UTC | #3 
On 29/03/2024 20:48, Witold Sadowski wrote:
> Add support for iMRVL xfer hw_overlay of Cadence xSPI
> block.
> MRVL Xfer overlay extend xSPI capabilities, to support
> non-memory SPI operations.
> With generic xSPI command it allows to create any
> required SPI transaction

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

... and build your code because this does not compile. :(

Best regards,
Krzysztof
Krzysztof Kozlowski March 31, 2024, 10:50 a.m. UTC | #4 
On 29/03/2024 20:48, Witold Sadowski wrote:
> Add support for Marvell IP modification - clock divider,
> and PHY config, and IRQ clearing.
> Clock divider block is build into Cadence XSPI controller
> and is connected directly to 800MHz clock.
> As PHY config is not set directly in IP block, driver can
> load custom PHY configuration values.
> To correctly clear interrupt in Marvell implementation
> MSI-X must be cleared too.
> 
> Signed-off-by: Witold Sadowski <wsadowski@marvell.com>
> ---
>  drivers/spi/spi-cadence-xspi.c | 311 ++++++++++++++++++++++++++++++++-

You already sent this patchset, so this is not v1. Please version your
patches correctly. b4 does it automatically.

You also received last time feedback which it seems you just ignored.
You did not respond to any of the feedback and I do not see it being
addressed here.

That's not how collaboration in upstream projects work. Don't just
ignore reviews you receive. Please carefully read:

https://elixir.bootlin.com/linux/v6.9-rc1/source/Documentation/process/submitting-patches.rst

There is also entire section about this particular issue - responding to
reviewers.

Best regards,
Krzysztof
Witold Sadowski April 29, 2024, 2:55 p.m. UTC | #5 
> ----------------------------------------------------------------------
> On 29/03/2024 20:48, Witold Sadowski wrote:
> > Add support for Marvell IP modification - clock divider, and PHY
> > config, and IRQ clearing.
> > Clock divider block is build into Cadence XSPI controller and is
> > connected directly to 800MHz clock.
> > As PHY config is not set directly in IP block, driver can load custom
> > PHY configuration values.
> > To correctly clear interrupt in Marvell implementation MSI-X must be
> > cleared too.
> 
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__elixir.bootlin.com_linux_v6.4-
> 2Drc1_source_Documentation_process_submitting-2Dpatches.rst-
> 23L597&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=GKgcn-g6ZX-
> JmCL3S2qKgVQhvhv7hu2n8En-
> dZbLTa8&m=jNf5MYEcexHML6io2koiqn18Pmkh2qqc0FL48zdCoojbFX06omzl2_Z0CpBeHn79
> &s=B038dgBUB0Gvl63ExMDFoXuomZBSZPHLpScHOtTax0Q&e=
> 
> >
> > Signed-off-by: Witold Sadowski <wsadowski@marvell.com>
> > ---
> 
> 
> > +
> > +static bool cdns_xspi_get_hw_overlay(struct platform_device *pdev) {
> > +	int err;
> > +
> > +	err = device_property_match_string(&pdev->dev,
> > +					   "compatible", "mrvl,xspi-nor");
> 
> No, do not add matching in some random parts of the code, but use driver
> match/data from ID table.

Ok. As I have written in different mail, a little bit of manual matching
Will be necessary to handle both ACPI and device-tree case.

> 
> ....
> 
> >
> > +	cdns_xspi_print_phy_config(cdns_xspi);
> >  	ret = cdns_xspi_controller_init(cdns_xspi);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to initialize controller\n"); @@ -613,6
> +911,9
> > @@ static const struct of_device_id cdns_xspi_of_match[] = {
> >  	{
> >  		.compatible = "cdns,xspi-nor",
> >  	},
> > +	{
> > +		.compatible = "mrvl,xspi-nor",
> 
> This falsely suggest they are compatible :/

I'm not sure if I understand what do you mean.
cdns, xspi will be compatible with overlay, as it won't touch any
additional HW. It possibly fail in second direction, as overlay
handling code will not see expected values.

> 
> > +	},
> >  	{ /* end of table */}
> >  };
> >  MODULE_DEVICE_TABLE(of, cdns_xspi_of_match);
> 
> Best regards,
> Krzysztof

Regards
Witek
Krzysztof Kozlowski April 30, 2024, 7:56 a.m. UTC | #6 
On 29/04/2024 16:55, Witold Sadowski wrote:
>>
>>> +
>>> +static bool cdns_xspi_get_hw_overlay(struct platform_device *pdev) {
>>> +	int err;
>>> +
>>> +	err = device_property_match_string(&pdev->dev,
>>> +					   "compatible", "mrvl,xspi-nor");
>>
>> No, do not add matching in some random parts of the code, but use driver
>> match/data from ID table.
> 
> Ok. As I have written in different mail, a little bit of manual matching
> Will be necessary to handle both ACPI and device-tree case.

ACPI also handle variants with match data.

> 
>>
>> ....
>>
>>>
>>> +	cdns_xspi_print_phy_config(cdns_xspi);
>>>  	ret = cdns_xspi_controller_init(cdns_xspi);
>>>  	if (ret) {
>>>  		dev_err(dev, "Failed to initialize controller\n"); @@ -613,6
>> +911,9
>>> @@ static const struct of_device_id cdns_xspi_of_match[] = {
>>>  	{
>>>  		.compatible = "cdns,xspi-nor",
>>>  	},
>>> +	{
>>> +		.compatible = "mrvl,xspi-nor",
>>
>> This falsely suggest they are compatible :/
> 
> I'm not sure if I understand what do you mean.
> cdns, xspi will be compatible with overlay, as it won't touch any
> additional HW. It possibly fail in second direction, as overlay
> handling code will not see expected values.

That's clear rule for almost every driver: if you do not have any match
data, it suggests entry is redundant, because devices are compatible.
There is no different treatment for SPI. As seen in other pieces of this
code, devices are not compatible, so it points to missing match data to
handle variants.

Best regards,
Krzysztof