Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Jul 2023 17:09:13 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     "Gangurde, Abhijit" <abhijit.gangurde@amd.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Gupta, Nipun" <Nipun.Gupta@amd.com>,
        "Agarwal, Nikhil" <nikhil.agarwal@amd.com>,
        "Gupta, Puneet (DCG-ENG)" <puneet.gupta@amd.com>,
        "git (AMD-Xilinx)" <git@amd.com>,
        "Simek, Michal" <michal.simek@amd.com>,
        "Jansen Van Vuuren, Pieter" <pieter.jansen-van-vuuren@amd.com>
Subject: Re: [PATCH 1/4] cdx: add support for bus enable and disable
Message-ID: <2023071223-unstopped-marsupial-9c89@gregkh>
References: <20230711121027.936487-1-abhijit.gangurde@amd.com>
 <20230711121027.936487-2-abhijit.gangurde@amd.com>
 <2023071109-saturate-purist-d854@gregkh>
 <DM4PR12MB7765F182AAF927F780DEE8848F36A@DM4PR12MB7765.namprd12.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DM4PR12MB7765F182AAF927F780DEE8848F36A@DM4PR12MB7765.namprd12.prod.outlook.com>
Precedence: bulk

On Wed, Jul 12, 2023 at 01:21:13PM +0000, Gangurde, Abhijit wrote:
> [AMD Official Use Only - General]
> 
> > > CDX bus needs to be disabled before updating/writing devices
> > > in the FPGA. Once the devices are written, the bus shall be
> > > enabled. This change provides sysfs entry to enable/disable the
> > > CDX bus.
> > >
> > > Co-developed-by: Nipun Gupta <nipun.gupta@amd.com>
> > > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> > > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> > vuuren@amd.com>
> > > Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-cdx | 11 +++++
> > >  drivers/cdx/cdx.c                       | 26 ++++++++++++
> > >  drivers/cdx/controller/cdx_controller.c | 27 +++++++++++++
> > >  drivers/cdx/controller/mc_cdx_pcol.h    | 54
> > +++++++++++++++++++++++++
> > >  drivers/cdx/controller/mcdi_functions.c | 24 +++++++++++
> > >  drivers/cdx/controller/mcdi_functions.h | 16 ++++++++
> > >  include/linux/cdx/cdx_bus.h             |  6 +++
> > >  7 files changed, 164 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx
> > b/Documentation/ABI/testing/sysfs-bus-cdx
> > > index 7af477f49998..0afa85b3c63b 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-cdx
> > > +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> > > @@ -11,6 +11,17 @@ Description:
> > >
> > >               # echo 1 > /sys/bus/cdx/rescan
> > >
> > > +What:              /sys/bus/cdx/enable
> > > +Date:              July 2023
> > > +Contact:   nipun.gupta@amd.com
> > > +Description:
> > > +           Writing y/1/on to this file enables the CDX bus and
> > > +           writing n/0/off disables the bus.
> > > +
> > > +           For example to disable CDX bus::
> > > +
> > > +             # echo 0 > /sys/bus/cdx/enable
> >
> > What could go wrong!  :)
> >
> > You don't say why disabling / enabling the bus is needed, this feels
> > like a very huge stick, why is this for all busses, and not just an
> > individual CDX bus?
> >
> 
> As said in the description of the patch, disabling/enabling is needed when FPGA is being reprogrammed.

Ok, why would this not also be in the description for when you need to
look up what this file does?  You will not be able to track it back to
the commit log very easily.

thanks,

greg k-h